[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
tprevite at gmail.com
Thu Apr 16 08:41:33 PDT 2015
On 4/15/2015 10:42 AM, Paulo Zanoni wrote:
> 2015-04-15 12:37 GMT-03:00 Todd Previte <tprevite at gmail.com>:
>>
>> 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.
> The workqueue thing was just an idea to implement a solution for the
> real problem. I think we should be focusing about discussing the fact
> that we're not taking i915_hotplug_work_func() into consideration when
> doing the compliance testing, not on the fact that one of the possible
> implementations could use a workqueue. I'd still like to hear your
> arguments on that.
Fair enough.
So I've been looking into this and why the i915_hotplug_work_func wasn't
part of this. It is, as you said, a relatively fundamental code path for
Displayport through the driver. What I discovered was that this function
is never called on HSW (my primary test vehicle), mainly because
check_link_status() returns IRQ_HANDLED instead of IRQ_NONE. The work
function for HSW is i915_digport_work_func, so when it gets the
IRQ_HANDLED return code, it doesn't fall through to the legacy
i915_hotplug_work_func handler. This is important because this handler
calls intel_hpd_irq_event which is where the ->detect connector function
is called. And intel_dp_detect() is where all the happy goodness for
Displayport begins.
Up until I discovered this, I had mistakenly propagated that problem
forward in to the SST case in intel_dp_hot_pulse() in patch 6 by
returning IRQ_HANDLED instead of IRQ_NONE, which is what the code was
doing for SST prior to patch 6. With this problem corrected (as it is in
the latest update in patch set V6) the work functions are now called as
they should be. The point being that this opens up the possibility of
using elements along this path to pass compliance testing, thereby
creating a more valid test case.
With this in mind, I am not opposed to using elements along that path to
satisfy compliance requirements (that's the spirit of the tests,
anyways) but as I've indicated, there are cases where we need to take
special steps (like the edid_corrupt flag) in order to do the right
things to pass the tests. I have concerns about trying to do that at
this point, as it requires substantial rework to that code path that
have a significant chance of breaking things. So to avoid that, I
propose that this patch be merged now so that a working solution is in
place. This discussion should continue and we can decide where to put
things in the hotplug_work path to satisfy the compliance requires over
the course of some followup patches.
>> 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
>>>
>>>
>
>
More information about the Intel-gfx
mailing list