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

[Todd Previte]

On 4/14/2015 9:53 AM, Paulo Zanoni wrote:
> 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 :)
I don't think this is a good idea. The work loop aspect seems like a 
very complex solution solution to a problem that is relatively simple. 
In a discussion with Daniel, he indicated that adding a work loop is 
something to be avoided unless it's *really* necessary, as they are 
prone to race conditions. In this case, I just don't see that it's 
necessary. Additionally, you have been keen to note invasive solutions 
and this seems like a highly invasive solution especially in light of 
having a viable one right here.

This solution adds very little overhead along the HPD path and that 
overhead is a single read of the EDID for each event. To further address 
any concerns about performance, for V6 I've propagated the long_hpd flag 
forward into check_link_status so that the EDID read is only called for 
a hot plug event, not short pulses. Another plus to this solution is 
that if/when this needs to moved to userspace, this one is a lot easier 
to unwind and pull out than the work loop you're suggesting.

With respect to the user space solution, that's something worth 
investigating in the future. I have some concerns about doing that, the 
primary one being the ability to detect the corrupted header, but I 
think it's something to consider for a follow-on update.


>>          /* 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
>
>