[Intel-gfx] [PATCH 04/13] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1

[Paulo Zanoni]

2015-04-13 11:53 GMT-03:00 Todd Previte <tprevite at gmail.com>:
> Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the
> Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for
> all HPD plug events. To reduce the amount of code, this EDID read is also
> used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual
> support for these tests is implemented in later patches in this series.
>
> V2:
> - Fixed compilation error introduced during rework
>
> Signed-off-by: Todd Previte <tprevite at gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 23184b0..75df3e2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  {
>         struct drm_device *dev = intel_dp_to_dev(intel_dp);
>         struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
> +       struct edid *edid_read = NULL;
>         u8 sink_irq_vector;
>         u8 link_status[DP_LINK_STATUS_SIZE];
>
> @@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>                 return;
>         }
>
> +       /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
> +        * 4.2.2.1 - EDID read required for all HPD events
> +         */
> +        edid_read = drm_get_edid(connector, adapter);
> +        if (!edid_read) {
> +                DRM_DEBUG_DRIVER("Invalid EDID detected\n");
> +        }
> +

We already briefly discussed this patch in private, so I'm going to
summarize the discussion and also add some more points here.

Frist, the actual detailed review: the indentation here is using
spaces and we're leaking the EDID. This will cause rebases to a few of
the next patches.

Back to the hight level architecture: your initial versions of the
series contained just 1 extra EDID read, and it was contained inside
the compliance testing function. Then the versions submitted a few
days ago had 2 extra EDID reads, then after some discussion you
reduced to 1 extra EDID read (the one on this patch). I previously
asked "But what about the automatic EDID read we do when we get a
hotplug? Can't we just rely on it?". I got some answers to the
question, but I was not really convinced.

Yesterday I was arguing that this extra EDID read is going to add a
small delay to every hotplug event we get, so my initial suggestion
was to organize the compliance testing in a way that would require the
user space program to call the GetResources() IOCTL to force the EDID
when needed. Your argument was that then the DP compliance testing
procedure would be testing our app for compliance, not the Kernel.

But today I decided to finally do some debugging regarding this, and I
was able to confirm that we do follow the DP requirements: we do have
an automatic EDID read done by the Kernel whenever we do a hotplug:
i915_hotplug_work_func() calls intel_dp_detect(), which ends calling
drm_get_edid() at some point. This function also does other stuff that
is required by the compliance testing, such as the DPCD reads.

Now there's a problem with using i915_hotplug_work_func(), which could
the reason why you rejected it: it only happens after
intel_dp_hpd_pulse(), which means that we only really do the EDID read
after intel_dp_handle_test_request().

I consider i915_hotplug_work_func() a fundamental part of our DP
framework, and the DP compliance testing seems to be just ignoring its
existence. So my idea for a solution here would be to make
intel_dp_handle_test_request() run on its own delayed work function.
It would wait for both i915_digport_work_func() and
i915_hotplug_work_func() to finish, and only then it would do the
normal processing. With this, we would be able to avoid the edid read
on this patch, we would maybe be able to avoid at least part of patch
2, we would maybe be able to completely avoid patch 7, and then on
patch 8 we would start touching intel_dp_get_edid() instead.

I know this is sort of a fundamental change that is being requested a
little late in the review process, and it can be frustrating, but this
aspect of the code only recently changed (I was fine with the EDID
reads just in the compliance testing function), and since the DP
compliance code is quite complex, it took me a while to realize
everything that's going on and what is the purpose of each piece. I
also think that, since this idea will allow the compliance testing to
take into consideration the work done by i915_hotplug_work_func(),
compliance testing will better reflect the behavior that is actually
done by the Kernel when DP devices are plugged/unplugged. And I did
ask about those new EDID reads as soon as I started reviewing the
patch that introduced them.

Now, since I know how frustrating it is to have to change a
significant portion of the code once again, I will leave to the
maintainers the decision of whether the current proposed
implementation is acceptable or if we want to make the DP compliance
testing code take into consideration the work done by
i915_hotplug_work_func(). I would also like to know your opinion on
this. Maybe my idea just doesn't make sense because of something else
I didn't realize :)

>         /* Try to read the source of the interrupt */
>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>             intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni