2015-04-15 12:37 GMT-03:00 Todd Previte <tprevite@xxxxxxxxx>:
>
>
> On 4/14/2015 9:53 AM, Paulo Zanoni wrote:
>>
>> 2015-04-13 11:53 GMT-03:00 Todd Previte <tprevite@xxxxxxxxx>:
>>>
>>> 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@xxxxxxxxx>
>>> ---
>>> 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.
> 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@xxxxxxxxxxxxxxxxxxxxx
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
