Re: [PATCH] drm/i915: Improve DP downstream HPD handling

[Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>]

On 7/7/2015 5:50 PM, Sivakumar Thulasimani wrote:
>
>
> On 7/7/2015 5:24 PM, Ville Syrjälä wrote:
>> On Tue, Jul 07, 2015 at 02:37:46PM +0300, Ville Syrjälä wrote:
>>> On Tue, Jul 07, 2015 at 04:45:11PM +0530, Sivakumar Thulasimani wrote:
>>>>
>>>> On 7/7/2015 4:40 PM, Ville Syrjälä wrote:
>>>>> On Tue, Jul 07, 2015 at 03:26:36PM +0530, Sivakumar Thulasimani 
>>>>> wrote:
>>>>>> On 7/6/2015 5:42 PM, ville.syrjala@linux.intel.com wrote:
>>>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>>>
>>>>>>> DP dongles may signal downstream HPD via short HPD pulses. If we 
>>>>>>> know
>>>>>>> the device has a HPD capable downstream port, make sure we kick 
>>>>>>> off the
>>>>>>> full hotplug processing even for short HPDs.
>>>>>>>
>>>>>>> Additonally setting the sink to DPMS off kills the downstream 
>>>>>>> HPD (at
>>>>>>> least on my DP->VGA dongle), so skip the DPMS off for such dongles
>>>>>>> when we turn off the port.
>>>>>>>
>>>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/i915/intel_dp.c | 18 +++++++++++++++++-
>>>>>>>     1 file changed, 17 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
>>>>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>> index e88cec2..f424833 100644
>>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>>> @@ -2324,6 +2324,13 @@ static void intel_dp_get_config(struct 
>>>>>>> intel_encoder *encoder,
>>>>>>>         }
>>>>>>>     }
>>>>>>>     +static bool intel_dp_has_downstream_hpd(struct intel_dp 
>>>>>>> *intel_dp)
>>>>>>> +{
>>>>>>> +    return intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] & 
>>>>>>> DP_DWN_STRM_PORT_PRESENT &&
>>>>>>> +        intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>>>>>> +        intel_dp->downstream_ports[0] & DP_DS_PORT_HPD;
>>>>>>> +}
>>>>>>> +
>>>>>>>     static void intel_disable_dp(struct intel_encoder *encoder)
>>>>>>>     {
>>>>>>>         struct intel_dp *intel_dp = 
>>>>>>> enc_to_intel_dp(&encoder->base);
>>>>>>> @@ -2340,7 +2347,9 @@ static void intel_disable_dp(struct 
>>>>>>> intel_encoder *encoder)
>>>>>>>          * ensure that we have vdd while we switch off the 
>>>>>>> panel. */
>>>>>>>         intel_edp_panel_vdd_on(intel_dp);
>>>>>>>         intel_edp_backlight_off(intel_dp);
>>>>>>> -    intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>>>>>>> +    /* Skip power down to keep downstream HPD working */
>>>>>>> +    if (!intel_dp_has_downstream_hpd(intel_dp))
>>>>>>> +        intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
>>>>>>>         intel_edp_panel_off(intel_dp);
>>>>>>>             /* disable the port before the pipe on g4x */
>>>>>>> @@ -4944,6 +4953,13 @@ intel_dp_hpd_pulse(struct 
>>>>>>> intel_digital_port *intel_dig_port, bool long_hpd)
>>>>>>> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>>>>>>                 intel_dp_check_link_status(intel_dp);
>>>>>>> drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>>>>>> +
>>>>>>> +            /*
>>>>>>> +             * Downstream HPD will generate a short HPD,
>>>>>>> +             * so we want full hotplug processing here.
>>>>>>> +             */
>>>>>>> +            if (intel_dp_has_downstream_hpd(intel_dp))
>>>>>>> +                goto put_power;
>>>>>>>             }
>>>>>>>         }
>>>>>> I am looking into compliance changes for DP and this seems a 
>>>>>> relevant
>>>>>> change for compliance as well. but as per Link CTS 1.2 section 
>>>>>> 4.2.2.8,
>>>>>> we are supposed to read the sink_count and do full detection if
>>>>>> sink_count is >1.  So instead of checking for DP_DS_PORT_HPD can 
>>>>>> we just
>>>>>> check SINK_COUNT and do full detect ?
>>>>> ->detect() will be called from the hotplug work and that will
>>>>> check SINK_COUNT.
>>>>>
>>>> No, the Compliance Sink tool, will not set the DP_DS_PORT_HPD 
>>>> resulting
>>>> in detect not getting executed for
>>>> the short pulse generated. The spec requires the sink to set only the
>>>> sink count so it is not a must for
>>>> the sink to update the DP_DOWNSTREAM_PORT_0. so only a check for
>>>> SINK_COUNT will pass the
>>>> compliance test.
>>> That seems stupid. If the downstream port isn't HPD capable then we 
>>> have
>>> no reason to check SINK_COUNT after a short HPD as the short HPD
>>> coudln't have been caused by a downstram HPD. Obviuously we still
>>> check SINK_COUNT after a long HPD to figure out if anything is 
>>> connected
>>> when the branch device itself gets connected to the source.
>> Actually that's not correct. We don't check SINK_COUNT unless the 
>> downstream
>> port is HPD capable.
>>
>> The spec says:
>> "If the DFP does not provide for means for plug/unplug detection, the
>> adaptor must set the SINK_COUNT field bits, as if those Sink devices 
>> were
>> all permanently plugged."
>>
>> So according to the there can't be any changes in SINK_COUNT if the
>> downstream port is not HPD capable.
>>
>>
>>
> yes, agree on the no changes for SINK_COUNT if HPD is 0. i'll check 
> with DP Compliance test
> tomorrow and confirm the exact reason for its failure may be my 
> understanding of it was incorrect.
>
confirmed that the compliance sink is not setting HPD bit during detect. 
so this looks to be a bug in
the sink tool. i'll file a bug with their team instead.

coming back to this patch, i will get back once i understand the complex 
scenario of all short pulse
is treated as long pulse post this change, for example: we will do full 
detection even if the sink requested
retraining of link.

-- 
regards,
Sivakumar

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