Re: [PATCH 13/17] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation

[Todd Previte <tprevite@gmail.com>]

On 12/17/14 1:30 PM, Daniel Vetter wrote:
> On Wed, Dec 17, 2014 at 03:57:21PM -0200, Paulo Zanoni wrote:
>> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>:
>>> Moves the non-MST case out of the if-statement and places it at the beginning
>>> of the function to handle HPD events for SST mode. The reasoning behind this
>>> is to accommodate link status checks for compliance testing. Some test devices
>>> use long pulses to perform test requests so link status must be checked
>>> regardless of the pulse width for the SST operational mode.
>> Can you please elaborate a little more on what do you see on these
>> devices? The test spec is very clear about short vs long HPD pulses,
>> so it's hard to believe a test device would get this wrong. We have
>> some registers on the PCH that allow us to redefine short vs long
>> durations. Have you tried to play with them?
>>
>> More below:
>>
>>> This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the
>>> previous compliance testing patch sequence. Review feedback on that patch
>>> indicated that updating intel_dp_hot_plug() was not the correct place for
>>> the test handler.
>>>
>>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c | 22 ++++++++++++----------
>>>   1 file changed, 12 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 4a55ca6..73014d8 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4613,6 +4613,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>>          power_domain = intel_display_port_power_domain(intel_encoder);
>>>          intel_display_power_get(dev_priv, power_domain);
>>>
>>> +       if (!intel_dp->is_mst) {
>>> +               /*
>>> +                *  Pulse width doesn't matter for SST mode
>>> +                *  Handle the HPD event now
>>> +               */
>>> +               drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>> +               intel_dp_check_link_status(intel_dp);
>> The very first thing intel_dp_check_link_status() does is to return in
>> case "connector->base.status != connected". If we're getting a long
>> HPD, it doesn't seem make sense to check this field because the status
>> might be changing due to the long HPD.
> I don't think we can unconditionally run SST hpd logic before we've
> correctly handled mst mode. It likely screws up the accounting.

Upon further review, it looks like the best solution here is to place 
the SST code in the mst_fail case as the 'else' clause. That way SST 
mode stuff gets handled regardless and if a need arises in the future 
where differentiating between short and long becomes necessary, there's 
a place to handle it. This will be in V3.

>>> +               drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>> +               ret = false;
>>> +               goto put_power;
>>> +       }
>>> +
>>>          if (long_hpd) {
>>>
>>>                  if (HAS_PCH_SPLIT(dev)) {
>>> @@ -4637,16 +4649,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>>                          if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
>>>                                  goto mst_fail;
>>>                  }
>>> -
>>> -               if (!intel_dp->is_mst) {
>>> -                       /*
>>> -                        * we'll check the link status via the normal hot plug path later -
>>> -                        * but for short hpds we should check it now
>>> -                        */
> Just aside: The above comment is outdated and can be remove. This is now
> the only place where we handle link retraining. The function could be made
> static and dropped from headers, too.
> -Daniel
Can't be static and pulled from the header. It's called in intel_ddi.c 
as well.

But the comment and the code is gone, in light of the above changes.

>>> -                       drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>> -                       intel_dp_check_link_status(intel_dp);
>>> -                       drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>> -               }
>>>          }
>>>          ret = false;
>>>          goto put_power;
>>> --
>>> 1.9.1
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> -- 
>> Paulo Zanoni
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx