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

[Todd Previte <tprevite@gmail.com>]

On 4/8/2015 11:53 AM, Paulo Zanoni wrote:
> 2015-04-01 15:00 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Update the hot plug function to handle the SST case. Instead of placing
>> the SST case within the long/short pulse block, it is now handled after
>> determining that MST mode is not in use. This way, the topology management
>> layer can handle any MST-related operations while SST operations are still
>> correctly handled afterwards.
>>
>> This patch also corrects the problem of SST mode only being handled in the
>> case of a short (0.5ms - 1.0ms) HPD pulse.
> It's not clear to me what exactly is the problem with the current
> code. Can you please clarify?
The main problem is that the comment in the code there is wrong. We 
*don't* check the link status later as it says it will. The legacy HPD 
handler (intel_dp_hot_plug) has been gutted but the function is still 
there in the code. It probably should have been removed when the new 
handler was implemented, but that's another matter entirely.

So as things stand, the only time we actually check the link status is 
when we get a short pulse on the HPD line. Long pulses (i.e. 
connect/disconnect events) don't get processed. This code corrects that 
problem and properly handles both long and short HPD pulses for the SST 
mode.

>> For compliance testing purposes
>> both short and long pulses are used by the different tests, thus both cases
>> need to be addressed for SST.
>>
>> 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.
>>
>> For the SST case, the main stream is disabled for long HPD pulses as this
>> generally indicates either a connect/disconnect event or link failure. For
>> a number of case in compliance testing, the source is required to disable
>> the main link upon detection of a long HPD.
>>
>> V2:
>> - N/A
>> V3:
>> - Place the SST mode link status check into the mst_fail case
>> - Remove obsolete comment regarding SST mode operation
>> - Removed an erroneous line of code that snuck in during rebasing
>> V4:
>> - Added a disable of the main stream (DP transport) for the long pulse case
>>    for SST to support compliance testing
>> V5:
>> - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 28 +++++++++++-----------------
>>   1 file changed, 11 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 16d35903..0a77f5a 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4622,16 +4622,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
>> -                        */
>> -                       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 = IRQ_HANDLED;
>> @@ -4639,18 +4629,22 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>          goto put_power;
>>   mst_fail:
>>          /* if we were in MST mode, and device is not there get out of MST mode */
>> -       if (intel_dp->is_mst) {
>> -               DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
>> -               intel_dp->is_mst = false;
>> -               drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
>> -       } else {
>> -               /* SST mode - handle short/long pulses here */
>> +       DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> So now aren't we going to get MST log messages on non-MST cases, such
> as a disconnect with long_hpd? I mean, even non-MST jumps to the
> mst_fail label.

Yeah that got removed by mistake. I corrected this for the next version 
of the patch.

>
>> +       intel_dp->is_mst = false;
>> +       drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
>> +
>> +put_power:
>> +       /* SST mode - handle short/long pulses here */
>> +       if (!intel_dp->is_mst) {
>> +               /* TO DO: Handle short/long pulses individually
>> +                       Can save on training times and overhead by not doing
>> +                       full link status updating/processing for short pulses
>> +                */
>>                  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 = IRQ_HANDLED;
>>          }
>> -put_power:
>>          intel_display_power_put(dev_priv, power_domain);
>>
>>          return ret;
>> --
>> 1.9.1
>>
>> _______________________________________________
>> 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