[2/4] drm/i915: read sink_count dpcd always
diff mbox

Message ID 1440665312-26698-3-git-send-email-sivakumar.thulasimani@intel.com
State New
Headers show

Commit Message

Sivakumar Thulasimani Aug. 27, 2015, 8:48 a.m. UTC
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.

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

v2: moved out crtc enabled checks to prior patch(Jani)

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

Comments

Jani Nikula Sept. 1, 2015, 10:29 a.m. UTC | #1
On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> 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.
>
> 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.

This makes sense.

I'm scared this may break something, if there are displays/adapters out
there that don't set sink count properly. I guess only one way to find
out... And this might actually fix something else.

> 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

Do you think you handle this last case properly now? Previously we'd
return with "connected" at the sink count check, but now we'll go for
the "pke ddc gently" phase.

See below.

>
> v2: moved out crtc enabled checks to prior patch(Jani)
>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 76561e0..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)) {
> @@ -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;
> -	}
> -

Why do we proceed here if we know that we have sink count > 0 and we
have a downstream port present? Admittedly I'm not sure I understand (or
remember) why we had this logic in the first place...

BR,
Jani.

>  	/* If no HPD, poke DDC gently */
>  	if (drm_probe_ddc(&intel_dp->aux.ddc))
>  		return connector_status_connected;
> -- 
> 1.7.9.5
>
Sivakumar Thulasimani Sept. 1, 2015, 11:54 a.m. UTC | #2
On 9/1/2015 3:59 PM, Jani Nikula wrote:
> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> 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.
>>
>> 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.
> This makes sense.
>
> I'm scared this may break something, if there are displays/adapters out
> there that don't set sink count properly. I guess only one way to find
> out... And this might actually fix something else.
>
>> 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
> Do you think you handle this last case properly now? Previously we'd
> return with "connected" at the sink count check, but now we'll go for
> the "pke ddc gently" phase.
>
> See below.
>
>> v2: moved out crtc enabled checks to prior patch(Jani)
>>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |   21 ++++++++-------------
>>   1 file changed, 8 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 76561e0..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)) {
>> @@ -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;
>> -	}
>> -
> Why do we proceed here if we know that we have sink count > 0 and we
> have a downstream port present? Admittedly I'm not sure I understand (or
> remember) why we had this logic in the first place...
>
> BR,
> Jani.
based on what little i can dig up, we proceed in case of dongles to 
check if the
display is CRT or not.
if edid read succeeded, it is DP or any display with proper edid to work 
with.
If edid read failed, then it could be CRT, without edid

we can make it work with a fake edid, but it seems current code just
sets it as connector_status_unknown and ignores the display

>>   	/* If no HPD, poke DDC gently */
>>   	if (drm_probe_ddc(&intel_dp->aux.ddc))
>>   		return connector_status_connected;
>> -- 
>> 1.7.9.5
>>
Jani Nikula Sept. 1, 2015, 1:15 p.m. UTC | #3
On Tue, 01 Sep 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> On 9/1/2015 3:59 PM, Jani Nikula wrote:
>> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> 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.
>>>
>>> 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.
>> This makes sense.
>>
>> I'm scared this may break something, if there are displays/adapters out
>> there that don't set sink count properly. I guess only one way to find
>> out... And this might actually fix something else.
>>
>>> 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
>> Do you think you handle this last case properly now? Previously we'd
>> return with "connected" at the sink count check, but now we'll go for
>> the "pke ddc gently" phase.
>>
>> See below.
>>
>>> v2: moved out crtc enabled checks to prior patch(Jani)
>>>
>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c |   21 ++++++++-------------
>>>   1 file changed, 8 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 76561e0..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)) {
>>> @@ -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;
>>> -	}
>>> -
>> Why do we proceed here if we know that we have sink count > 0 and we
>> have a downstream port present? Admittedly I'm not sure I understand (or
>> remember) why we had this logic in the first place...
>>
>> BR,
>> Jani.
> based on what little i can dig up, we proceed in case of dongles to 
> check if the
> display is CRT or not.
> if edid read succeeded, it is DP or any display with proper edid to work 
> with.
> If edid read failed, then it could be CRT, without edid

My point is, with the current code, if we have downstream port present
and sink count > 0 and the first downsteam port supports hpd, we'll
directly say status is connected. The hpd check may be bogus, but we
still won't go probing ddc on them.

With your patch, we'd always do the ddc probe if there is a downstream
port present and the sink count > 0.

BR,
Jani.



>
> we can make it work with a fake edid, but it seems current code just
> sets it as connector_status_unknown and ignores the display
>
>>>   	/* If no HPD, poke DDC gently */
>>>   	if (drm_probe_ddc(&intel_dp->aux.ddc))
>>>   		return connector_status_connected;
>>> -- 
>>> 1.7.9.5
>>>
>
> -- 
> regards,
> Sivakumar
>
Sivakumar Thulasimani Sept. 1, 2015, 2:24 p.m. UTC | #4
On 9/1/2015 6:45 PM, Jani Nikula wrote:
> On Tue, 01 Sep 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> On 9/1/2015 3:59 PM, Jani Nikula wrote:
>>> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> 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.
>>>>
>>>> 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.
>>> This makes sense.
>>>
>>> I'm scared this may break something, if there are displays/adapters out
>>> there that don't set sink count properly. I guess only one way to find
>>> out... And this might actually fix something else.
>>>
>>>> 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
>>> Do you think you handle this last case properly now? Previously we'd
>>> return with "connected" at the sink count check, but now we'll go for
>>> the "pke ddc gently" phase.
>>>
>>> See below.
>>>
>>>> v2: moved out crtc enabled checks to prior patch(Jani)
>>>>
>>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dp.c |   21 ++++++++-------------
>>>>    1 file changed, 8 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 76561e0..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)) {
>>>> @@ -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;
>>>> -	}
>>>> -
>>> Why do we proceed here if we know that we have sink count > 0 and we
>>> have a downstream port present? Admittedly I'm not sure I understand (or
>>> remember) why we had this logic in the first place...
>>>
>>> BR,
>>> Jani.
>> based on what little i can dig up, we proceed in case of dongles to
>> check if the
>> display is CRT or not.
>> if edid read succeeded, it is DP or any display with proper edid to work
>> with.
>> If edid read failed, then it could be CRT, without edid
> My point is, with the current code, if we have downstream port present
> and sink count > 0 and the first downsteam port supports hpd, we'll
> directly say status is connected. The hpd check may be bogus, but we
> still won't go probing ddc on them.
>
> With your patch, we'd always do the ddc probe if there is a downstream
> port present and the sink count > 0.
>
> BR,
> Jani.
understood, will check once by adding the old check if hpd supported 
downstream
is present and share new patch.
>
>
>> we can make it work with a fake edid, but it seems current code just
>> sets it as connector_status_unknown and ignores the display
>>
>>>>    	/* If no HPD, poke DDC gently */
>>>>    	if (drm_probe_ddc(&intel_dp->aux.ddc))
>>>>    		return connector_status_connected;
>>>> -- 
>>>> 1.7.9.5
>>>>
>> -- 
>> regards,
>> Sivakumar
>>

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 76561e0..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)) {
@@ -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;