Re: [PATCH 3/4] drm/i915: Fix random aux transactions failures.

Re: [PATCH 3/4] drm/i915: Fix random aux transactions failures.

[Daniel Vetter]

On Wed, Oct 21, 2015 at 10:28:53AM -0700, Rodrigo Vivi wrote:
> Mainly aux communications on sink_crc
> were failing a lot randomly on recent platforms.
> The first solution was to try to use intel_dp_dpcd_read_wake, but then
> it was suggested to move retries to drm level.
> 
> Since drm level was already taking care of retries and didn't want
> to through random retries on that level the second solution was to
> put the retries at aux_transfer layer what was nacked.
> 
> So I realized we had so many retries in different places and
> started to organize that a bit. During this organization I noticed
> that we weren't handing at all the case were the message size was
> zeroed. And this was exactly the case that was affecting sink_crc.
> 
> Also we weren't respect BSPec who says this size message = 0 or > 20
> are forbidden.
> 
> It is a fact that we still have no clue why we are getting this
> forbidden value there. But anyway we need to handle that for now
> so we return -EBUSY and drm level takes care of the retries that
> are already in place.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index aa3d8f6..80850d6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -911,6 +911,17 @@ done:
>  	/* Unload any bytes sent back from the other side */
>  	recv_bytes = ((status & DP_AUX_CH_CTL_MESSAGE_SIZE_MASK) >>
>  		      DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT);
> +
> +	/*
> +	 * By BSpec: "Message sizes of 0 or >20 are not allowed."
> +	 * We have no idea of what happened so we return -EBUSY so
> +	 * drm layer takes care for the necessary retries.
> +	 */
> +	if (recv_bytes == 0 || recv_bytes > 20) {
> +		ret = -EBUSY;
> +		goto out;
> +	}

Hm, this should be caught be the dp aux helper library. Both callers for
->transfer should check for this and reject with -EINVAL (since such a
transaction is simply not allowed by dp aux). In the case of
drm_dp_i2c_do_msg maybe even with a WARN_ON since the i2c logic should
split things up correctly.
-Daniel

> +
>  	if (recv_bytes > recv_size)
>  		recv_bytes = recv_size;
>  
> -- 
> 2.4.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Fix random aux transactions failures.

[Daniel Vetter]

On Wed, Oct 21, 2015 at 09:18:06AM +0200, Daniel Vetter wrote:
> On Wed, Oct 21, 2015 at 10:28:53AM -0700, Rodrigo Vivi wrote:
> > Mainly aux communications on sink_crc
> > were failing a lot randomly on recent platforms.
> > The first solution was to try to use intel_dp_dpcd_read_wake, but then
> > it was suggested to move retries to drm level.
> > 
> > Since drm level was already taking care of retries and didn't want
> > to through random retries on that level the second solution was to
> > put the retries at aux_transfer layer what was nacked.
> > 
> > So I realized we had so many retries in different places and
> > started to organize that a bit. During this organization I noticed
> > that we weren't handing at all the case were the message size was
> > zeroed. And this was exactly the case that was affecting sink_crc.
> > 
> > Also we weren't respect BSPec who says this size message = 0 or > 20
> > are forbidden.
> > 
> > It is a fact that we still have no clue why we are getting this
> > forbidden value there. But anyway we need to handle that for now
> > so we return -EBUSY and drm level takes care of the retries that
> > are already in place.
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index aa3d8f6..80850d6 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -911,6 +911,17 @@ done:
> >  	/* Unload any bytes sent back from the other side */
> >  	recv_bytes = ((status & DP_AUX_CH_CTL_MESSAGE_SIZE_MASK) >>
> >  		      DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT);
> > +
> > +	/*
> > +	 * By BSpec: "Message sizes of 0 or >20 are not allowed."
> > +	 * We have no idea of what happened so we return -EBUSY so
> > +	 * drm layer takes care for the necessary retries.
> > +	 */
> > +	if (recv_bytes == 0 || recv_bytes > 20) {
> > +		ret = -EBUSY;
> > +		goto out;
> > +	}
> 
> Hm, this should be caught be the dp aux helper library. Both callers for
> ->transfer should check for this and reject with -EINVAL (since such a
> transaction is simply not allowed by dp aux). In the case of
> drm_dp_i2c_do_msg maybe even with a WARN_ON since the i2c logic should
> split things up correctly.

Meh, totally misread what's going on here, this is from the hardware. How
does this even happen? Do you get some kind of garbage value? Should we
maybe clear this register field first? It certainly would explain a lot of
our random dp aux retry fun ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Avoid EBUSY retry on intel_dp_aux_ch.

[Ville Syrjälä]

On Wed, Oct 21, 2015 at 10:28:51AM -0700, Rodrigo Vivi wrote:
> EBUSY retries are already in place at drm level.
> We don't need to replicate the job here.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 09bdd94..d054129 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -818,24 +818,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  
>  	intel_aux_display_runtime_get(dev_priv);
>  
> -	/* Try to wait for any previous AUX channel activity */
> -	for (try = 0; try < 3; try++) {
> -		status = I915_READ_NOTRACE(ch_ctl);
> -		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> -			break;
> -		msleep(1);

The dp helper doesn't sleep at all between the retries, so dropping the
sleep from here may change the behaviour.

> -	}
> -
> -	if (try == 3) {
> -		static u32 last_status = -1;
> -		const u32 status = I915_READ(ch_ctl);
> -
> -		if (status != last_status) {
> -			WARN(1, "dp_aux_ch not started status 0x%08x\n",
> -			     status);
> -			last_status = status;
> -		}
> -
> +	status = I915_READ_NOTRACE(ch_ctl);
> +	if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0) {
>  		ret = -EBUSY;
>  		goto out;
>  	}
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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.

> -
> -	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_SYNC_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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Avoid EBUSY retry on intel_dp_aux_ch.

[Vivi, Rodrigo]

On Wed, 2015-10-21 at 12:19 +0300, Ville Syrjälä wrote:
> On Wed, Oct 21, 2015 at 10:28:51AM -0700, Rodrigo Vivi wrote:
> > EBUSY retries are already in place at drm level.
> > We don't need to replicate the job here.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 20 ++------------------
> >  1 file changed, 2 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 09bdd94..d054129 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -818,24 +818,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> >  
> >  	intel_aux_display_runtime_get(dev_priv);
> >  
> > -	/* Try to wait for any previous AUX channel activity */
> > -	for (try = 0; try < 3; try++) {
> > -		status = I915_READ_NOTRACE(ch_ctl);
> > -		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> > -			break;
> > -		msleep(1);
> 
> The dp helper doesn't sleep at all between the retries, so dropping 
> the
> sleep from here may change the behaviour.

hm indeed... what about 
+ msleep(1);
+ ret = -EBUSY
+ goto out;
?
> 
> > -	}
> > -
> > -	if (try == 3) {
> > -		static u32 last_status = -1;
> > -		const u32 status = I915_READ(ch_ctl);
> > -
> > -		if (status != last_status) {
> > -			WARN(1, "dp_aux_ch not started status 
> > 0x%08x\n",
> > -			     status);
> > -			last_status = status;
> > -		}
> > -
> > +	status = I915_READ_NOTRACE(ch_ctl);
> > +	if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0) {
> >  		ret = -EBUSY;
> >  		goto out;
> >  	}
> > -- 
> > 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

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

[Vivi, Rodrigo]

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

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

[Ville Syrjälä]

On Wed, Oct 21, 2015 at 02:31:33PM +0000, Vivi, Rodrigo wrote:
> 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.

This isn't a retry of a failed transfer.

> 
> 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
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 0/4] Organize aux retries.

[Rodrigo Vivi]

drm level already takes care of retries on EBUSY and we had
retries all over intel layer. So let's organize it a bit.

Everything started with aux communications on sink_crc
were failing a lot randomly on recent platforms.
The first solution was to try to use intel_dp_dpcd_read_wake, but then
it was suggested to move retries to drm level.

Since drm level was already taking care of retries and didn't want
to through random retries on that level the second solution was to
put the retries at aux_transfer layer what was nacked.

So I realized we had so many retries in different places and
started to organize that a bit. Instead of duplicating the retries
on our code, whenever we need to retry or we don't know what failed
we can return EBUSY so drm level will take care of retries.

Also during this investigation I noticed we weren't handling
forbiddens message size. This is fixed with patch 3/4.

Rodrigo Vivi (4):
  drm/i915: Avoid EBUSY retry on intel_dp_aux_ch.
  drm/i915: Remove remaining retries from intel_dp_aux_ch.
  drm/i915: Fix random aux transactions failures.
  drm/i915: Kill intel_dp_dpcd_read_wake

 drivers/gpu/drm/i915/intel_dp.c | 195 ++++++++++++++++++----------------------
 1 file changed, 85 insertions(+), 110 deletions(-)

-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/4] drm/i915: Avoid EBUSY retry on intel_dp_aux_ch.

[Rodrigo Vivi]

EBUSY retries are already in place at drm level.
We don't need to replicate the job here.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 09bdd94..d054129 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -818,24 +818,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 
 	intel_aux_display_runtime_get(dev_priv);
 
-	/* Try to wait for any previous AUX channel activity */
-	for (try = 0; try < 3; try++) {
-		status = I915_READ_NOTRACE(ch_ctl);
-		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
-			break;
-		msleep(1);
-	}
-
-	if (try == 3) {
-		static u32 last_status = -1;
-		const u32 status = I915_READ(ch_ctl);
-
-		if (status != last_status) {
-			WARN(1, "dp_aux_ch not started status 0x%08x\n",
-			     status);
-			last_status = status;
-		}
-
+	status = I915_READ_NOTRACE(ch_ctl);
+	if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0) {
 		ret = -EBUSY;
 		goto out;
 	}
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: Remove remaining retries from intel_dp_aux_ch.

[Rodrigo Vivi]

drm level already takes care of the needed retries so instead of
duplicate the effort here.

If we have no idea what caused the error let's assume something
was busy and let drm level handle the retries.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 69 ++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d054129..aa3d8f6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -794,7 +794,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 	uint32_t aux_clock_divider;
 	int i, ret, recv_bytes;
 	uint32_t status;
-	int try, clock = 0;
+	int clock = 0;
 	bool has_aux_irq = HAS_AUX_IRQ(dev);
 	bool vdd;
 
@@ -836,41 +836,52 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 							  send_bytes,
 							  aux_clock_divider);
 
-		/* Must try at least 3 times according to DP spec */
-		for (try = 0; try < 5; try++) {
-			/* Load the send data into the aux channel data registers */
-			for (i = 0; i < send_bytes; i += 4)
-				I915_WRITE(ch_data + i,
-					   intel_dp_pack_aux(send + i,
-							     send_bytes - i));
+		/* Load the send data into the aux channel data registers */
+		for (i = 0; i < send_bytes; i += 4)
+			I915_WRITE(ch_data + i,
+				   intel_dp_pack_aux(send + i,
+						     send_bytes - i));
 
-			/* Send the command and wait for it to complete */
-			I915_WRITE(ch_ctl, send_ctl);
+		/* Send the command and wait for it to complete */
+		I915_WRITE(ch_ctl, send_ctl);
 
-			status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
+		status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
 
-			/* Clear done status and any errors */
-			I915_WRITE(ch_ctl,
-				   status |
-				   DP_AUX_CH_CTL_DONE |
-				   DP_AUX_CH_CTL_TIME_OUT_ERROR |
-				   DP_AUX_CH_CTL_RECEIVE_ERROR);
+		/* Clear done status and any errors */
+		I915_WRITE(ch_ctl,
+			   status |
+			   DP_AUX_CH_CTL_DONE |
+			   DP_AUX_CH_CTL_TIME_OUT_ERROR |
+			   DP_AUX_CH_CTL_RECEIVE_ERROR);
 
-			if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR)
-				continue;
+		if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR) {
+			/*
+			 * We don't know what caused the error, so let's
+			 * return -EBUSY so drm level takes care of
+			 * the necessary retries
+			 */
+			ret = -EBUSY;
+			goto out;
+		}
 
-			/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
-			 *   400us delay required for errors and timeouts
-			 *   Timeout errors from the HW already meet this
-			 *   requirement so skip to next iteration
+		/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
+		 *   400us delay required for errors and timeouts
+		 *   Timeout errors from the HW already meet this
+		 *   requirement so skip to next iteration
+		 */
+		if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
+			usleep_range(400, 500);
+			/*
+			 * We don't know what caused the error, so let's
+			 * return -EBUSY so drm level takes care of
+			 * the necessary retries
 			 */
-			if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
-				usleep_range(400, 500);
-				continue;
-			}
-			if (status & DP_AUX_CH_CTL_DONE)
-				goto done;
+			ret = -EBUSY;
+			goto out;
 		}
+
+		if (status & DP_AUX_CH_CTL_DONE)
+			goto done;
 	}
 
 	if ((status & DP_AUX_CH_CTL_DONE) == 0) {
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: Fix random aux transactions failures.

[Rodrigo Vivi]

Mainly aux communications on sink_crc
were failing a lot randomly on recent platforms.
The first solution was to try to use intel_dp_dpcd_read_wake, but then
it was suggested to move retries to drm level.

Since drm level was already taking care of retries and didn't want
to through random retries on that level the second solution was to
put the retries at aux_transfer layer what was nacked.

So I realized we had so many retries in different places and
started to organize that a bit. During this organization I noticed
that we weren't handing at all the case were the message size was
zeroed. And this was exactly the case that was affecting sink_crc.

Also we weren't respect BSPec who says this size message = 0 or > 20
are forbidden.

It is a fact that we still have no clue why we are getting this
forbidden value there. But anyway we need to handle that for now
so we return -EBUSY and drm level takes care of the retries that
are already in place.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index aa3d8f6..80850d6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -911,6 +911,17 @@ done:
 	/* Unload any bytes sent back from the other side */
 	recv_bytes = ((status & DP_AUX_CH_CTL_MESSAGE_SIZE_MASK) >>
 		      DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT);
+
+	/*
+	 * By BSpec: "Message sizes of 0 or >20 are not allowed."
+	 * We have no idea of what happened so we return -EBUSY so
+	 * drm layer takes care for the necessary retries.
+	 */
+	if (recv_bytes == 0 || recv_bytes > 20) {
+		ret = -EBUSY;
+		goto out;
+	}
+
 	if (recv_bytes > recv_size)
 		recv_bytes = recv_size;
 
-- 
2.4.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Rodrigo Vivi]

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);
-
-	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_SYNC_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

Re: [PATCH 1/4] drm/i915: Avoid EBUSY retry on intel_dp_aux_ch.

[Thulasimani, Sivakumar]

On 10/21/2015 7:54 PM, Vivi, Rodrigo wrote:
> On Wed, 2015-10-21 at 12:19 +0300, Ville Syrjälä wrote:
>> On Wed, Oct 21, 2015 at 10:28:51AM -0700, Rodrigo Vivi wrote:
>>> EBUSY retries are already in place at drm level.
>>> We don't need to replicate the job here.
>>>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c | 20 ++------------------
>>>   1 file changed, 2 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index 09bdd94..d054129 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -818,24 +818,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>>>   
>>>   	intel_aux_display_runtime_get(dev_priv);
>>>   
>>> -	/* Try to wait for any previous AUX channel activity */
>>> -	for (try = 0; try < 3; try++) {
>>> -		status = I915_READ_NOTRACE(ch_ctl);
>>> -		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
>>> -			break;
>>> -		msleep(1);
>> The dp helper doesn't sleep at all between the retries, so dropping
>> the
>> sleep from here may change the behaviour.
> hm indeed... what about
> + msleep(1);
> + ret = -EBUSY
> + goto out;
> ?
the current code is one of few test cases that pass during compliance 
testing
so please confirm this change does not break that :).

regards,
Sivakumar
>>> -	}
>>> -
>>> -	if (try == 3) {
>>> -		static u32 last_status = -1;
>>> -		const u32 status = I915_READ(ch_ctl);
>>> -
>>> -		if (status != last_status) {
>>> -			WARN(1, "dp_aux_ch not started status
>>> 0x%08x\n",
>>> -			     status);
>>> -			last_status = status;
>>> -		}
>>> -
>>> +	status = I915_READ_NOTRACE(ch_ctl);
>>> +	if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0) {
>>>   		ret = -EBUSY;
>>>   		goto out;
>>>   	}
>>> -- 
>>> 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Fix random aux transactions failures.

[Thulasimani, Sivakumar]

On 10/21/2015 12:53 PM, Daniel Vetter wrote:
> On Wed, Oct 21, 2015 at 09:18:06AM +0200, Daniel Vetter wrote:
>> On Wed, Oct 21, 2015 at 10:28:53AM -0700, Rodrigo Vivi wrote:
>>> Mainly aux communications on sink_crc
>>> were failing a lot randomly on recent platforms.
>>> The first solution was to try to use intel_dp_dpcd_read_wake, but then
>>> it was suggested to move retries to drm level.
>>>
>>> Since drm level was already taking care of retries and didn't want
>>> to through random retries on that level the second solution was to
>>> put the retries at aux_transfer layer what was nacked.
>>>
>>> So I realized we had so many retries in different places and
>>> started to organize that a bit. During this organization I noticed
>>> that we weren't handing at all the case were the message size was
>>> zeroed. And this was exactly the case that was affecting sink_crc.
>>>
>>> Also we weren't respect BSPec who says this size message = 0 or > 20
>>> are forbidden.
>>>
>>> It is a fact that we still have no clue why we are getting this
>>> forbidden value there. But anyway we need to handle that for now
>>> so we return -EBUSY and drm level takes care of the retries that
>>> are already in place.
>>>
>>> Cc: Jani Nikula <jani.nikula@intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index aa3d8f6..80850d6 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -911,6 +911,17 @@ done:
>>>   	/* Unload any bytes sent back from the other side */
>>>   	recv_bytes = ((status & DP_AUX_CH_CTL_MESSAGE_SIZE_MASK) >>
>>>   		      DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT);
>>> +
>>> +	/*
>>> +	 * By BSpec: "Message sizes of 0 or >20 are not allowed."
>>> +	 * We have no idea of what happened so we return -EBUSY so
>>> +	 * drm layer takes care for the necessary retries.
>>> +	 */
>>> +	if (recv_bytes == 0 || recv_bytes > 20) {
>>> +		ret = -EBUSY;
>>> +		goto out;
>>> +	}
>> Hm, this should be caught be the dp aux helper library. Both callers for
>> ->transfer should check for this and reject with -EINVAL (since such a
>> transaction is simply not allowed by dp aux). In the case of
>> drm_dp_i2c_do_msg maybe even with a WARN_ON since the i2c logic should
>> split things up correctly.
> Meh, totally misread what's going on here, this is from the hardware. How
> does this even happen? Do you get some kind of garbage value? Should we
> maybe clear this register field first? It certainly would explain a lot of
> our random dp aux retry fun ...
> -Daniel
we are already checking for read size in intel_dp_aux_transfer

  if (WARN_ON(rxsize > 20))
                return -E2BIG;

and again inside intel_dp_aux_ch

  843         /* Only 5 data registers! */
  844         if (WARN_ON(send_bytes > 20 || recv_size > 20)) {
  845                 ret = -E2BIG;
  846                 goto out;
  847         }

Also isn't it possible that a simple write command will have 0 for 
receive size ?
can you share bit more details on what scenario this patch is helping ?

regards,
Sivakumar

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: Avoid EBUSY retry on intel_dp_aux_ch.

[Vivi, Rodrigo]

On Wed, 2015-10-21 at 23:31 +0530, Thulasimani, Sivakumar wrote:
> 
> On 10/21/2015 7:54 PM, Vivi, Rodrigo wrote:
> > On Wed, 2015-10-21 at 12:19 +0300, Ville Syrjälä wrote:
> > > On Wed, Oct 21, 2015 at 10:28:51AM -0700, Rodrigo Vivi wrote:
> > > > EBUSY retries are already in place at drm level.
> > > > We don't need to replicate the job here.
> > > > 
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_dp.c | 20 ++------------------
> > > >   1 file changed, 2 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 09bdd94..d054129 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -818,24 +818,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> > > >   
> > > >   	intel_aux_display_runtime_get(dev_priv);
> > > >   
> > > > -	/* Try to wait for any previous AUX channel activity 
> > > > */
> > > > -	for (try = 0; try < 3; try++) {
> > > > -		status = I915_READ_NOTRACE(ch_ctl);
> > > > -		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> > > > -			break;
> > > > -		msleep(1);
> > > The dp helper doesn't sleep at all between the retries, so 
> > > dropping
> > > the
> > > sleep from here may change the behaviour.
> > hm indeed... what about
> > + msleep(1);
> > + ret = -EBUSY
> > + goto out;
> > ?
> the current code is one of few test cases that pass during compliance 
> testing
> so please confirm this change does not break that :).

which one specifically are you telling about?
anyway basic tests here were at the same rate (128 pass/skip and 2
fails on bdw 1 fail on skl) without and with this series.

> 
> regards,
> Sivakumar
> > > > -	}
> > > > -
> > > > -	if (try == 3) {
> > > > -		static u32 last_status = -1;
> > > > -		const u32 status = I915_READ(ch_ctl);
> > > > -
> > > > -		if (status != last_status) {
> > > > -			WARN(1, "dp_aux_ch not started status
> > > > 0x%08x\n",
> > > > -			     status);
> > > > -			last_status = status;
> > > > -		}
> > > > -
> > > > +	status = I915_READ_NOTRACE(ch_ctl);
> > > > +	if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0) {
> > > >   		ret = -EBUSY;
> > > >   		goto out;
> > > >   	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Fix random aux transactions failures.

[Vivi, Rodrigo]

On Wed, 2015-10-21 at 23:47 +0530, Thulasimani, Sivakumar wrote:
> 
> On 10/21/2015 12:53 PM, Daniel Vetter wrote:
> > On Wed, Oct 21, 2015 at 09:18:06AM +0200, Daniel Vetter wrote:
> > > On Wed, Oct 21, 2015 at 10:28:53AM -0700, Rodrigo Vivi wrote:
> > > > Mainly aux communications on sink_crc
> > > > were failing a lot randomly on recent platforms.
> > > > The first solution was to try to use intel_dp_dpcd_read_wake, 
> > > > but then
> > > > it was suggested to move retries to drm level.
> > > > 
> > > > Since drm level was already taking care of retries and didn't 
> > > > want
> > > > to through random retries on that level the second solution was 
> > > > to
> > > > put the retries at aux_transfer layer what was nacked.
> > > > 
> > > > So I realized we had so many retries in different places and
> > > > started to organize that a bit. During this organization I 
> > > > noticed
> > > > that we weren't handing at all the case were the message size 
> > > > was
> > > > zeroed. And this was exactly the case that was affecting 
> > > > sink_crc.
> > > > 
> > > > Also we weren't respect BSPec who says this size message = 0 or 
> > > > > 20
> > > > are forbidden.
> > > > 
> > > > It is a fact that we still have no clue why we are getting this
> > > > forbidden value there. But anyway we need to handle that for 
> > > > now
> > > > so we return -EBUSY and drm level takes care of the retries 
> > > > that
> > > > are already in place.
> > > > 
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++
> > > >   1 file changed, 11 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index aa3d8f6..80850d6 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -911,6 +911,17 @@ done:
> > > >   	/* Unload any bytes sent back from the other side */
> > > >   	recv_bytes = ((status & 
> > > > DP_AUX_CH_CTL_MESSAGE_SIZE_MASK) >>
> > > >   		      DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT);
> > > > +
> > > > +	/*
> > > > +	 * By BSpec: "Message sizes of 0 or >20 are not 
> > > > allowed."
> > > > +	 * We have no idea of what happened so we return 
> > > > -EBUSY so
> > > > +	 * drm layer takes care for the necessary retries.
> > > > +	 */
> > > > +	if (recv_bytes == 0 || recv_bytes > 20) {
> > > > +		ret = -EBUSY;
> > > > +		goto out;
> > > > +	}
> > > Hm, this should be caught be the dp aux helper library. Both 
> > > callers for
> > > ->transfer should check for this and reject with -EINVAL (since 
> > > such a
> > > transaction is simply not allowed by dp aux). In the case of
> > > drm_dp_i2c_do_msg maybe even with a WARN_ON since the i2c logic 
> > > should
> > > split things up correctly.
> > Meh, totally misread what's going on here, this is from the 
> > hardware. How
> > does this even happen? Do you get some kind of garbage value? 
> > Should we
> > maybe clear this register field first? It certainly would explain a 
> > lot of
> > our random dp aux retry fun ...
> > -Daniel
> we are already checking for read size in intel_dp_aux_transfer
> 
>   if (WARN_ON(rxsize > 20))
>                 return -E2BIG;
> 
> and again inside intel_dp_aux_ch
> 
>   843         /* Only 5 data registers! */
>   844         if (WARN_ON(send_bytes > 20 || recv_size > 20)) {
>   845                 ret = -E2BIG;
>   846                 goto out;
>   847         }
> 
> Also isn't it possible that a simple write command will have 0 for 
> receive size ?

no, this is not possible. I'm sure I'm doing the proper read/writes on
sink_crc code.

> can you share bit more details on what scenario this patch is helping 
> ?

automated psr and frontbuffer cases on SKL. sink_crc is failing without
this patch or without retrying reads few times.

> 
> regards,
> Sivakumar
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx