[Intel-gfx] [PATCH] drm/i915: Improve DP downstream HPD handling

[Sivakumar Thulasimani]

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 at linux.intel.com wrote:
>>>>>> From: Ville Syrjälä <ville.syrjala at 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 at 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.


-- 
regards,
Sivakumar