[Intel-gfx] [PATCH 5/6] drm/i915: read sink_count dpcd always

[Shubhangi Shrivastava]

On Monday 18 January 2016 06:30 PM, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-01-18 at 18:14 +0530, Shubhangi Shrivastava wrote:
>> On Thursday 14 January 2016 06:34 PM, Ander Conselvan De Oliveira wrote:
>>> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
>>>> 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
>>>>
>>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu at intel.com>
>>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani at intel.com>
>>>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dp.c | 11 +++++++----
>>>>    1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>> index c2e8516..0d58bfd 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -3865,6 +3865,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,
>>>> +				    &intel_dp->sink_count, 1) < 0)
>>>> +		return false;
>>>> +
>>>> +	if (!DP_GET_SINK_COUNT(intel_dp->sink_count))
>>>> +		return false;
>>>> +
>>> My understanding is that this function should only read the DPCD data while
>>> detection based on that data is done in intel_dp_detect_dpcd(). With the
>>> return
>>> on sink_count == 0 here, we skip the end of the function, which updates the
>>> cached downstream port information. Is there a reason why we need this early
>>> return here?
>>>
>>> Also, I think this could be squashed with the previous patch.
>>>
>>> Ander
>> As described in the commit message, if sink_count is 0, then there is no
>> display present. So, irrespective of value of downstream port, we should
>> terminate the function and thus, an early return is present here.
> You wrote that "SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT
> dpcd". Now, the get_dpcd() function is called from different places with the
> purpose of retrieving information stored in dpcd. By adding the early return,
> the downstream port information, which you claimed is independent from sink
> count, is not updated.
>
> The way I see it, you should terminate detection when sink count is 0, not the
> reading of DPCD. That way the logical split between intel_dp_get_dpcd() and
> intel_dp_detect_dpcd() is maintained. The former reads DPCD and the latter
> reasons about it.
>
> Ander
Yes, that's how it is.. But, SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT 
== 1 implies that a dongle is present but no display. Unless we require 
to know if a dongle is present or not, we don't need to update 
downstream port information. So, an early return here saves time from 
performing other operations which are not required.
>>>>    	/* Check if the panel supports PSR */
>>>>    	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>>>    	if (is_edp(intel_dp)) {
>>>> @@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>>>    	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>>>    	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>>>>    
>>>> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
>>>> DP_SINK_COUNT,
>>>> -					    &intel_dp->sink_count, 1) <
>>>> 0)
>>>> -			return connector_status_unknown;
>>>> -
>>>>    		return DP_GET_SINK_COUNT(intel_dp->sink_count) ?
>>>>    		connector_status_connected :
>>>> connector_status_disconnected;
>>>>    	}