Re: [PATCH 5/6] drm/i915: read sink_count dpcd always

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

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@intel.com>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@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

>  	/* 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;
>  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx