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

[Ander Conselvan De Oliveira]

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

> 
> > >   	/* 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;
> > >   	}
>