Re: [PATCH 4/4] drm/i915: Kill intel_dp_dpcd_read_wake

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Wed, 2015-10-21 at 12:23 +0300, Ville Syrjälä wrote:
> On Wed, Oct 21, 2015 at 10:28:54AM -0700, Rodrigo Vivi wrote:
> > We have an inconsistency on our code on using 
> > intel_dp_dpcd_read_wake with
> > retries and when using drm_dp_dpcd_read helper without retry.
> > 
> > Must probably with the case were aux message size 0 wasn't being
> > properly handled.
> > 
> > With this in place and with drm level now taking care of all 
> > retries
> > let's kill this last random retry.
> > 
> > So in case we find more corner cases on aux transactions we face 
> > that
> > on aux_ch transaction directly returning EBUSY so drm level takes 
> > care
> > of any retry.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 95 ++++++++++++++---------------
> > ------------
> >  1 file changed, 32 insertions(+), 63 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 80850d6..ff5e2a1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -992,7 +992,8 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, 
> > struct drm_dp_aux_msg *msg)
> >  		if (WARN_ON(rxsize > 20))
> >  			return -E2BIG;
> >  
> > -		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, 
> > rxbuf, rxsize);
> > +		ret = intel_dp_aux_ch(intel_dp, txbuf, txsize,
> > +				      rxbuf, rxsize);
> >  		if (ret > 0) {
> >  			msg->reply = rxbuf[0] >> 4;
> >  			/*
> > @@ -3018,47 +3019,16 @@ static void chv_dp_post_pll_disable(struct 
> > intel_encoder *encoder)
> >  }
> >  
> >  /*
> > - * Native read with retry for link status and receiver capability 
> > reads for
> > - * cases where the sink may still be asleep.
> > - *
> > - * Sinks are *supposed* to come up within 1ms from an off state, 
> > but we're also
> > - * supposed to retry 3 times per the spec.
> > - */
> > -static ssize_t
> > -intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int 
> > offset,
> > -			void *buffer, size_t size)
> > -{
> > -	ssize_t ret;
> > -	int i;
> > -
> > -	/*
> > -	 * Sometime we just get the same incorrect byte repeated
> > -	 * over the entire buffer. Doing just one throw away read
> > -	 * initially seems to "solve" it.
> > -	 */
> > -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> 
> Jani already noted that this can't be killed unless someone can 
> explain
> why we need it, and how we can make do without it.

My research goes up to Jani's patches... sorry I coulnd't go further
than that. I don't know why that retry was there before that but I
assume or it was forgotten there when retry was added to drm level or
it was being used to solve that cases where aux ch ctl message size was
zero and we never handled.

But yes, it is a fact that we need some QA involved to see if this
removal doesn't break other cases.

Locally here on BDW and SKL -t basic looks fine without this
function...

> 
> > -
> > -	for (i = 0; i < 3; i++) {
> > -		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> > -		if (ret == size)
> > -			return ret;
> > -		msleep(1);
> > -	}
> > -
> > -	return ret;
> > -}
> > -
> > -/*
> >   * Fetch AUX CH registers 0x202 - 0x207 which contain
> >   * link status information
> >   */
> >  static bool
> >  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t 
> > link_status[DP_LINK_STATUS_SIZE])
> >  {
> > -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> > -				       DP_LANE0_1_STATUS,
> > -				       link_status,
> > -				       DP_LINK_STATUS_SIZE) == 
> > DP_LINK_STATUS_SIZE;
> > +	return drm_dp_dpcd_read(&intel_dp->aux,
> > +				DP_LANE0_1_STATUS,
> > +				link_status,
> > +				DP_LINK_STATUS_SIZE) == 
> > DP_LINK_STATUS_SIZE;
> >  }
> >  
> >  /* These are source-specific values. */
> > @@ -3985,8 +3955,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	uint8_t rev;
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, 
> > intel_dp->dpcd,
> > -				    sizeof(intel_dp->dpcd)) < 0)
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp
> > ->dpcd,
> > +			     sizeof(intel_dp->dpcd)) < 0)
> >  		return false; /* aux transfer failed */
> >  
> >  	DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp
> > ->dpcd), intel_dp->dpcd);
> > @@ -3997,9 +3967,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	/* Check if the panel supports PSR */
> >  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
> >  	if (is_edp(intel_dp)) {
> > -		intel_dp_dpcd_read_wake(&intel_dp->aux, 
> > DP_PSR_SUPPORT,
> > -					intel_dp->psr_dpcd,
> > -					sizeof(intel_dp
> > ->psr_dpcd));
> > +		drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT,
> > +				 intel_dp->psr_dpcd,
> > +				 sizeof(intel_dp->psr_dpcd));
> >  		if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> >  			dev_priv->psr.sink_support = true;
> >  			DRM_DEBUG_KMS("Detected EDP PSR 
> > Panel.\n");
> > @@ -4010,9 +3980,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  			uint8_t frame_sync_cap;
> >  
> >  			dev_priv->psr.sink_support = true;
> > -			intel_dp_dpcd_read_wake(&intel_dp->aux,
> > -					DP_SINK_DEVICE_AUX_FRAME_S
> > YNC_CAP,
> > -					&frame_sync_cap, 1);
> > +			drm_dp_dpcd_readb(&intel_dp->aux,
> > +					 
> >  DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > +					  &frame_sync_cap);
> >  			dev_priv->psr.aux_frame_sync = 
> > frame_sync_cap ? true : false;
> >  			/* PSR2 needs frame sync as well */
> >  			dev_priv->psr.psr2_support = dev_priv
> > ->psr.aux_frame_sync;
> > @@ -4028,15 +3998,15 @@ intel_dp_get_dpcd(struct intel_dp 
> > *intel_dp)
> >  	/* Intermediate frequency support */
> >  	if (is_edp(intel_dp) &&
> >  	    (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &	DP_
> > DPCD_DISPLAY_CONTROL_CAPABLE) &&
> > -	    (intel_dp_dpcd_read_wake(&intel_dp->aux, 
> > DP_EDP_DPCD_REV, &rev, 1) == 1) &&
> > +	    (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_DPCD_REV, 
> > &rev) == 1) &&
> >  	    (rev >= 0x03)) { /* eDp v1.4 or higher */
> >  		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
> >  		int i;
> >  
> > -		intel_dp_dpcd_read_wake(&intel_dp->aux,
> > -				DP_SUPPORTED_LINK_RATES,
> > -				sink_rates,
> > -				sizeof(sink_rates));
> > +		drm_dp_dpcd_read(&intel_dp->aux,
> > +				 DP_SUPPORTED_LINK_RATES,
> > +				 sink_rates,
> > +				 sizeof(sink_rates));
> >  
> >  		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
> >  			int val = le16_to_cpu(sink_rates[i]);
> > @@ -4059,9 +4029,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
> >  		return true; /* no per-port downstream info */
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 
> > DP_DOWNSTREAM_PORT_0,
> > -				    intel_dp->downstream_ports,
> > -				    DP_MAX_DOWNSTREAM_PORTS) < 0)
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> > +			     intel_dp->downstream_ports,
> > +			     DP_MAX_DOWNSTREAM_PORTS) < 0)
> >  		return false; /* downstream port status fetch 
> > failed */
> >  
> >  	return true;
> > @@ -4075,11 +4045,11 @@ intel_dp_probe_oui(struct intel_dp 
> > *intel_dp)
> >  	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & 
> > DP_OUI_SUPPORT))
> >  		return;
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, 
> > buf, 3) == 3)
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) 
> > == 3)
> >  		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
> >  			      buf[0], buf[1], buf[2]);
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, 
> > buf, 3) == 3)
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 
> > 3) == 3)
> >  		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
> >  			      buf[0], buf[1], buf[2]);
> >  }
> > @@ -4095,7 +4065,7 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
> >  	if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
> >  		return false;
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_MSTM_CAP, 
> > buf, 1)) {
> > +	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, buf)) {
> >  		if (buf[0] & DP_MST_CAP) {
> >  			DRM_DEBUG_KMS("Sink is MST capable\n");
> >  			intel_dp->is_mst = true;
> > @@ -4234,9 +4204,9 @@ stop:
> >  static bool
> >  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 
> > *sink_irq_vector)
> >  {
> > -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> > -				      
> >  DP_DEVICE_SERVICE_IRQ_VECTOR,
> > -				       sink_irq_vector, 1) == 1;
> > +	return drm_dp_dpcd_readb(&intel_dp->aux,
> > +				 DP_DEVICE_SERVICE_IRQ_VECTOR,
> > +				 sink_irq_vector) == 1;
> >  }
> >  
> >  static bool
> > @@ -4244,9 +4214,9 @@ intel_dp_get_sink_irq_esi(struct intel_dp 
> > *intel_dp, u8 *sink_irq_vector)
> >  {
> >  	int ret;
> >  
> > -	ret = intel_dp_dpcd_read_wake(&intel_dp->aux,
> > -					     DP_SINK_COUNT_ESI,
> > -					     sink_irq_vector, 14);
> > +	ret = drm_dp_dpcd_read(&intel_dp->aux,
> > +			       DP_SINK_COUNT_ESI,
> > +			       sink_irq_vector, 14);
> >  	if (ret != 14)
> >  		return false;
> >  
> > @@ -4502,8 +4472,7 @@ intel_dp_detect_dpcd(struct intel_dp 
> > *intel_dp)
> >  	    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)
> > +		if (drm_dp_dpcd_readb(&intel_dp->aux, 
> > DP_SINK_COUNT, &reg) < 0)
> >  			return connector_status_unknown;
> >  
> >  		return DP_GET_SINK_COUNT(reg) ? 
> > connector_status_connected
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx