Re: [PATCH 1/3] drm/i915: read sink_count dpcd always

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

On 8/26/2015 5:21 PM, Ville Syrjälä wrote:
> On Tue, Aug 25, 2015 at 05:20:36PM +0530, Sivakumar Thulasimani wrote:
>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>
>> This patch reads sink_count dpcd always and removes its
>> read operation based on values in downstream port dpcd. Also
>> we should read it irrespective of current status of sink.
>>
>> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
>> SINK_COUNT denotes if a display is attached, while
>> DOWNSTREAM_PORT_PRESET indicates how many ports are available
>> in the dongle where display can be attached. so it is possible
>> for sink count to change irrespective of value in downstream
>> port dpcd.
>>
>> Here is a table of possible values and scenarios
>>
>> sink_count	downstream_port
>> 		present
>> 0		0		no display is attached
>> 0		1		dongle is connected without display
>> 1		0		display connected directly
>> 1		1		display connected through dongle
>>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |   33 ++++++++++++++-------------------
>>   1 file changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 8a66a44..9e4e27d 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3898,6 +3898,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>   	struct drm_device *dev = dig_port->base.base.dev;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	uint8_t rev;
>> +	uint8_t reg;
>>   
>>   	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>>   				    sizeof(intel_dp->dpcd)) < 0)
>> @@ -3908,6 +3909,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>   	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
>>   		return false; /* DPCD not present */
>>   
>> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>> +					    &reg, 1) < 0)
>> +			return false;
>> +
>> +	if (!DP_GET_SINK_COUNT(reg))
>> +		return false;
>> +
>>   	/* Check if the panel supports PSR */
>>   	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>   	if (is_edp(intel_dp)) {
>> @@ -4374,12 +4382,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   
>>   	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>   
>> -	if (!intel_encoder->base.crtc)
>> -		return;
>> -
>> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> -		return;
>> -
> I don't like trying to overload intel_dp_check_link_status() to do other
> stuff besides link retraining. The sink irq stuff already snuck in
> somehow, but I think we should pull it out and try to make the function
> names match what they do.
>
> The entire DP hpd locking also needs more though. We now grab the
> connection_mutex for the retraining to prevent racing against modeset,
> but it doesn't prevent racing against ->detect(). And for MST we take
> no locks whatsoever, even for the retraining (I have a patch for that
> that I'll send out soon).
Agree, i am still thinking on how to extract the Automated test request 
part so
we can avoid having same code twice in intel_dp_detect() and in
intel_dp_check_link_status. if your patch also cleans up this function i can
rebase on top of it.
>>   	/* Try to read receiver status if the link appears to be up */
>>   	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>   		return;
>> @@ -4390,6 +4392,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   		return;
>>   	}
>>   
>> +	if (!intel_encoder->base.crtc)
>> +		return;
>> +
>> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> +		return;
>> +
>>   	/* 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)) {
>> @@ -4427,19 +4435,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>   	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>>   		return connector_status_connected;
>>   
>> -	/* If we're HPD-aware, SINK_COUNT changes dynamically */
>> -	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>> -	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>> -		uint8_t reg;
>> -
>> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>> -					    &reg, 1) < 0)
>> -			return connector_status_unknown;
>> -
>> -		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
>> -					      : connector_status_disconnected;
>> -	}
>> -
>>   	/* If no HPD, poke DDC gently */
>>   	if (drm_probe_ddc(&intel_dp->aux.ddc))
>>   		return connector_status_connected;
>> -- 
>> 1.7.9.5

-- 
regards,
Sivakumar

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