[PATCH 0/8] Organize and offload aux retries to drm. (v2)

[PATCH 0/8] Organize and offload aux retries to drm. (v2)

[Rodrigo Vivi]

The goal of this series is to remove many different retries we have
for aux communication and offload them to drm.

However on first attempt I was only returning EBUSY to use drm retries
but there was no waiting there. So this series also introduce a new
approach on drm level to retry on aux communication failures:

1 - EAGAIN: Immediatelly retry since HW might already taken care of the
    needed waitings.
2 - EBUSY: Something is probably busy and we are not getting what we
    expected so wait 1ms before trying again so we give time to aux
    channels to recover.

Thanks,
Rodrigo.

Rodrigo Vivi (8):
  drm: Introduce EAGAIN handling for immediatelly aux retries
  drm/nouveau: Use EAGAIN instead EBUSY for aux retry.
  drm/i915: Use EAGAIN instead EBUSY for aux retry.
  drm: Wait 1ms before retrying aux transactions on EBUSY.
  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/drm_dp_helper.c                    |   9 +-
 drivers/gpu/drm/i915/intel_dp.c                    | 192 +++++++++------------
 drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxg94.c   |   4 +-
 drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxgm204.c |   4 +-
 4 files changed, 93 insertions(+), 116 deletions(-)

-- 
2.4.3

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

[PATCH 1/8] drm: Introduce EAGAIN handling for immediatelly aux retries

[Rodrigo Vivi]

The goal here is to offload aux retries handling from the
drivers to drm.

However there are 2 differents cases for retry:
1. Immediately retry - Hardware already took care of needed timeouts
   	       	       before answering that a retry is possible.
2. Busy - Wait some time and retry.

This driver introduce the support for EAGAIN that can handle the
first case and a following patch will introduce EBUSY waits,
after all drivers counting on immediately retries are changed.

Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 9535c5b..ee7c955 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -198,6 +198,11 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 		err = aux->transfer(aux, &msg);
 		mutex_unlock(&aux->hw_mutex);
 		if (err < 0) {
+			/* Immediately retry */
+			if (err == -EAGAIN)
+				continue;
+
+			/* FIXME: On BUSY we could wait before retrying */
 			if (err == -EBUSY)
 				continue;
 
-- 
2.4.3

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

Re: [PATCH 1/8] drm: Introduce EAGAIN handling for immediatelly aux retries

[Jani Nikula]

On Sat, 21 Nov 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> The goal here is to offload aux retries handling from the
> drivers to drm.
>
> However there are 2 differents cases for retry:
> 1. Immediately retry - Hardware already took care of needed timeouts
>    	       	       before answering that a retry is possible.
> 2. Busy - Wait some time and retry.
>
> This driver introduce the support for EAGAIN that can handle the
> first case and a following patch will introduce EBUSY waits,
> after all drivers counting on immediately retries are changed.
>
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 9535c5b..ee7c955 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -198,6 +198,11 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>  		err = aux->transfer(aux, &msg);
>  		mutex_unlock(&aux->hw_mutex);
>  		if (err < 0) {
> +			/* Immediately retry */
> +			if (err == -EAGAIN)
> +				continue;

How about the ->transfer call in drm_dp_i2c_do_msg? If we leave that for
the i2c layer (which does do retries on -EAGAIN, but also handles
timeouts for repeated -EAGAINs) it deserves a comment in in
drm_dp_i2c_do_msg.

BR,
Jani.

> +
> +			/* FIXME: On BUSY we could wait before retrying */
>  			if (err == -EBUSY)
>  				continue;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/8] drm/nouveau: Use EAGAIN instead EBUSY for aux retry.

[Rodrigo Vivi]

Current EBUSY meaning is immediately retry, but this is
about to change. DRM aux transfer is about to change and
make EAGAIN the immediately retry and use EBUSY to wait
a bit for aux channels to recover for any error or wake up.

This has no functional change if the EAGAIN support is in
place already for drm aux transfer.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxg94.c   | 4 ++--
 drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxgm204.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxg94.c b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxg94.c
index 954f5b7..a6cd729 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxg94.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxg94.c
@@ -52,7 +52,7 @@ g94_i2c_aux_init(struct g94_i2c_aux *aux)
 		udelay(1);
 		if (!timeout--) {
 			AUX_ERR(&aux->base, "begin idle timeout %08x", ctrl);
-			return -EBUSY;
+			return -EAGAIN;
 		}
 	} while (ctrl & 0x03010000);
 
@@ -65,7 +65,7 @@ g94_i2c_aux_init(struct g94_i2c_aux *aux)
 		if (!timeout--) {
 			AUX_ERR(&aux->base, "magic wait %08x", ctrl);
 			g94_i2c_aux_fini(aux);
-			return -EBUSY;
+			return -EAGAIN;
 		}
 	} while ((ctrl & 0x03000000) != urep);
 
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxgm204.c b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxgm204.c
index bed231b..6814e5b 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxgm204.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxgm204.c
@@ -52,7 +52,7 @@ gm204_i2c_aux_init(struct gm204_i2c_aux *aux)
 		udelay(1);
 		if (!timeout--) {
 			AUX_ERR(&aux->base, "begin idle timeout %08x", ctrl);
-			return -EBUSY;
+			return -EAGAIN;
 		}
 	} while (ctrl & 0x03010000);
 
@@ -65,7 +65,7 @@ gm204_i2c_aux_init(struct gm204_i2c_aux *aux)
 		if (!timeout--) {
 			AUX_ERR(&aux->base, "magic wait %08x", ctrl);
 			gm204_i2c_aux_fini(aux);
-			return -EBUSY;
+			return -EAGAIN;
 		}
 	} while ((ctrl & 0x03000000) != urep);
 
-- 
2.4.3

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

[PATCH 3/8] drm/i915: Use EAGAIN instead EBUSY for aux retry.

[Rodrigo Vivi]

Current EBUSY meaning is immediately retry, but this is
about to change. DRM aux transfer is about to change and
make EAGAIN the immediately retry and use EBUSY to wait
a bit for aux channels to recover for any error or wake up.

This has no functional change if the EAGAIN support is in
place already for drm aux transfer.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index bec443a..ed07f0a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -835,7 +835,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 			last_status = status;
 		}
 
-		ret = -EBUSY;
+		ret = -EAGAIN;
 		goto out;
 	}
 
@@ -890,7 +890,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 
 	if ((status & DP_AUX_CH_CTL_DONE) == 0) {
 		DRM_ERROR("dp_aux_ch not done status 0x%08x\n", status);
-		ret = -EBUSY;
+		ret = -EAGAIN;
 		goto out;
 	}
 
-- 
2.4.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] drm/i915: Use EAGAIN instead EBUSY for aux retry.

[Daniel Vetter]

On Fri, Nov 20, 2015 at 04:46:25PM -0800, Rodrigo Vivi wrote:
> Current EBUSY meaning is immediately retry, but this is
> about to change. DRM aux transfer is about to change and
> make EAGAIN the immediately retry and use EBUSY to wait
> a bit for aux channels to recover for any error or wake up.
> 
> This has no functional change if the EAGAIN support is in
> place already for drm aux transfer.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Please document the EAGAIN and EBUSY return codes for the ->transfer
member fo drm_dp_aux in the kerneldoc properly. While at it would be great
if you could document any other error codes that are treated specially for
this function.

Since this will be a bit more text please convert the kerneldoc to the new
per-member comment layout that 4.4 supports, i.e.

/**
 * struct foo - foo structure
 *
 * top-level blabla
 */
struct foo {
	/**
	 * @bar:
	 *
	 * Long text (with paragraphs) explaining bar.
	 */
};

Otherwise this looks really tidy and I really like how this allows us to
get rid of the intel_read_wake hack.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index bec443a..ed07f0a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -835,7 +835,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  			last_status = status;
>  		}
>  
> -		ret = -EBUSY;
> +		ret = -EAGAIN;
>  		goto out;
>  	}
>  
> @@ -890,7 +890,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  
>  	if ((status & DP_AUX_CH_CTL_DONE) == 0) {
>  		DRM_ERROR("dp_aux_ch not done status 0x%08x\n", status);
> -		ret = -EBUSY;
> +		ret = -EAGAIN;
>  		goto out;
>  	}
>  
> -- 
> 2.4.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
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/8] drm/i915: Use EAGAIN instead EBUSY for aux retry.

[Jani Nikula]

On Mon, 23 Nov 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Nov 20, 2015 at 04:46:25PM -0800, Rodrigo Vivi wrote:
>> Current EBUSY meaning is immediately retry, but this is
>> about to change. DRM aux transfer is about to change and
>> make EAGAIN the immediately retry and use EBUSY to wait
>> a bit for aux channels to recover for any error or wake up.
>> 
>> This has no functional change if the EAGAIN support is in
>> place already for drm aux transfer.
>> 
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> Please document the EAGAIN and EBUSY return codes for the ->transfer
> member fo drm_dp_aux in the kerneldoc properly. While at it would be great
> if you could document any other error codes that are treated specially for
> this function.

See my comment in reply to patch 1 about the difference in handling of
the ->transfer return values for native aux and i2c-over-aux.

BR,
Jani.

>
> Since this will be a bit more text please convert the kerneldoc to the new
> per-member comment layout that 4.4 supports, i.e.
>
> /**
>  * struct foo - foo structure
>  *
>  * top-level blabla
>  */
> struct foo {
> 	/**
> 	 * @bar:
> 	 *
> 	 * Long text (with paragraphs) explaining bar.
> 	 */
> };
>
> Otherwise this looks really tidy and I really like how this allows us to
> get rid of the intel_read_wake hack.
> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index bec443a..ed07f0a 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -835,7 +835,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>>  			last_status = status;
>>  		}
>>  
>> -		ret = -EBUSY;
>> +		ret = -EAGAIN;
>>  		goto out;
>>  	}
>>  
>> @@ -890,7 +890,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>>  
>>  	if ((status & DP_AUX_CH_CTL_DONE) == 0) {
>>  		DRM_ERROR("dp_aux_ch not done status 0x%08x\n", status);
>> -		ret = -EBUSY;
>> +		ret = -EAGAIN;
>>  		goto out;
>>  	}
>>  
>> -- 
>> 2.4.3
>> 
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/8] drm: Wait 1ms before retrying aux transactions on EBUSY.

[Rodrigo Vivi]

DP Specs isn't really clear about the amount of retries,
but for cases it mentions retries it also mention times that
vary from 300us to 1ms.

For many cases hardware can handled the timeouts before retry
is possible and allowed, but for many other cases it is better
to wait and give time so the aux channels can recover.

For instance one general case there is when downstream device
is waking up from sleep states generating HPD so it might take
up to 1ms before getting responsive.

I believe with this msleep we could minimize the 32 times retries
and still let Dell monitors happy, but I don't have this monitor
to test here so let's just add the sleep for now and still retry
32 times.

Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index ee7c955..1d6016d 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -202,9 +202,11 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 			if (err == -EAGAIN)
 				continue;
 
-			/* FIXME: On BUSY we could wait before retrying */
-			if (err == -EBUSY)
+			/* Give a time for aux channels to recover */
+			if (err == -EBUSY) {
+				msleep(1);
 				continue;
+			}
 
 			return err;
 		}
-- 
2.4.3

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

Re: [PATCH 4/8] drm: Wait 1ms before retrying aux transactions on EBUSY.

[Jani Nikula]

On Sat, 21 Nov 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> DP Specs isn't really clear about the amount of retries,
> but for cases it mentions retries it also mention times that
> vary from 300us to 1ms.
>
> For many cases hardware can handled the timeouts before retry
> is possible and allowed, but for many other cases it is better
> to wait and give time so the aux channels can recover.
>
> For instance one general case there is when downstream device
> is waking up from sleep states generating HPD so it might take
> up to 1ms before getting responsive.
>
> I believe with this msleep we could minimize the 32 times retries
> and still let Dell monitors happy, but I don't have this monitor
> to test here so let's just add the sleep for now and still retry
> 32 times.
>
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index ee7c955..1d6016d 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -202,9 +202,11 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>  			if (err == -EAGAIN)
>  				continue;
>  
> -			/* FIXME: On BUSY we could wait before retrying */
> -			if (err == -EBUSY)
> +			/* Give a time for aux channels to recover */
> +			if (err == -EBUSY) {
> +				msleep(1);

usleep_range please; msleep(1) may take *much* longer than 1 ms, and
that could throw us off with our retry logic.

BR,
Jani.

>  				continue;
> +			}
>  
>  			return err;
>  		}

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/8] 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.

v2: rebase on top of EAGAIN x EBUSY retries change at drm.
    EBUSY retry at DRM is now handling the msleep(1).

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ed07f0a..a8ba243 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -817,25 +817,9 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 
 	intel_dp_check_edp(intel_dp);
 
-	/* 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;
-		}
-
-		ret = -EAGAIN;
+	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 6/8] 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 the retry is possible immediately we return EAGAIN. Otherwise,
if we have no idea what caused the error let's assume something
was busy and let drm level handle the wait and retries.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a8ba243..35048d6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -795,7 +795,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;
 
@@ -835,41 +835,47 @@ 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(intel_dp->aux_ch_data_reg[i >> 2],
-					   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(intel_dp->aux_ch_data_reg[i >> 2],
+				   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) {
+			/*
+			 * 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
+			 */
+			ret = -EAGAIN;
+			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
+		if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
+			/*
+			 * We don't know what caused the error, so let's
+			 * return -EBUSY so drm level takes care of
+			 * the necessary wait for recover and 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

Re: [PATCH 6/8] drm/i915: Remove remaining retries from intel_dp_aux_ch.

[Jani Nikula]

On Sat, 21 Nov 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> drm level already takes care of the needed retries so instead of
> duplicate the effort here.
>
> If the retry is possible immediately we return EAGAIN. Otherwise,
> if we have no idea what caused the error let's assume something
> was busy and let drm level handle the wait and retries.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 64 ++++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a8ba243..35048d6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -795,7 +795,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;
>  
> @@ -835,41 +835,47 @@ 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++) {

This now inverses the retries wrt the aux clock divider retries, which
probably breaks the non-ULT HSW workaround:

"WA: For the PCH Aux channels (Aux B/C/D) use an aux divider value of 63
decimal (03Fh). If there is a failure, retry at least three times with
63, then retry at least three times with 72 decimal (048h). See South
Display Engine Registers, DP_AUX_CTL."

It is sad.

BR,
Jani.


> -			/* Load the send data into the aux channel data registers */
> -			for (i = 0; i < send_bytes; i += 4)
> -				I915_WRITE(intel_dp->aux_ch_data_reg[i >> 2],
> -					   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(intel_dp->aux_ch_data_reg[i >> 2],
> +				   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) {
> +			/*
> +			 * 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
> +			 */
> +			ret = -EAGAIN;
> +			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
> +		if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
> +			/*
> +			 * We don't know what caused the error, so let's
> +			 * return -EBUSY so drm level takes care of
> +			 * the necessary wait for recover and 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) {

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/8] 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 35048d6..c87e937 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -905,6 +905,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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/8] drm/i915: Fix random aux transactions failures.

[Jani Nikula]

On Sat, 21 Nov 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> 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 35048d6..c87e937 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -905,6 +905,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;

This deserves debug logging at the very least.

BR,
Jani.


> +		goto out;
> +	}
> +
>  	if (recv_bytes > recv_size)
>  		recv_bytes = recv_size;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/8] drm/i915: Fix random aux transactions failures.

[Rodrigo Vivi]

This read wake with retries were initially added by 2 commits:

commit 61da5fab ("drm/i915/dp: retry link status read 3 times on failure")
commit 899526d9 ("drm/i915/dp: try to read receiver capabilities 3 times when detecting")

Both mentioning section 9.1 of the 1.1a DisplayPort spec, that actually
tell us to retry three times on certain case when "writing 01h to DPCD address 600h"
and this code is already in place in our driver. Added by:

commit c7ad3810 ("drm/i915/dp: manage sink power state if possible")

At this point we have no visibility if those patches were added to workaround certain
corner cases like lazy dongles or what, but also at that time there wasn't enough
retries on the proper places.

So my proposal is to remove these retries for now that we have drm handling the retries
and if we face any corner case back again we study it to return EAGAIN or EBUSY
to force retries at drm instead of handling them here.

v2: Improve commit message trying to explain the origin of the retries.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
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 c87e937..2ce6527 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -985,7 +985,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;
 			/*
@@ -3150,47 +3151,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
  */
 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. */
@@ -3825,8 +3795,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);
@@ -3837,9 +3807,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");
@@ -3850,9 +3820,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;
@@ -3868,15 +3838,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]);
@@ -3899,9 +3869,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;
@@ -3915,11 +3885,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]);
 }
@@ -3935,7 +3905,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;
@@ -4072,9 +4042,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
@@ -4082,9 +4052,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;
 
@@ -4343,8 +4313,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 7/8] drm/i915: Fix random aux transactions failures.

[Jani Nikula]

I don't see how the subject matches the commit.

On Sat, 21 Nov 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> This read wake with retries were initially added by 2 commits:
>
> commit 61da5fab ("drm/i915/dp: retry link status read 3 times on failure")
> commit 899526d9 ("drm/i915/dp: try to read receiver capabilities 3 times when detecting")
>
> Both mentioning section 9.1 of the 1.1a DisplayPort spec, that actually
> tell us to retry three times on certain case when "writing 01h to DPCD address 600h"
> and this code is already in place in our driver. Added by:
>
> commit c7ad3810 ("drm/i915/dp: manage sink power state if possible")

I still think what we currently do for the sink power state management
works by coincidence. We should still look into it.

However, I think this series overall (apart from patch 6/8 which really
is a bummer, the comment inline below, and the minor other comments)
looks like worthwhile changes. We can leave the power state management
for later. Or rip it out for now...

> At this point we have no visibility if those patches were added to workaround certain
> corner cases like lazy dongles or what, but also at that time there wasn't enough
> retries on the proper places.
>
> So my proposal is to remove these retries for now that we have drm handling the retries
> and if we face any corner case back again we study it to return EAGAIN or EBUSY
> to force retries at drm instead of handling them here.
>
> v2: Improve commit message trying to explain the origin of the retries.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> 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 c87e937..2ce6527 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -985,7 +985,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;
>  			/*
> @@ -3150,47 +3151,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);

This still needs to be addressed somehow. Maybe it's sufficient for
Ville to test with his monitor?

commit f6a1906674005377b64ee5431c1418077c1b2425
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
Date:   Thu Oct 16 20:46:09 2014 +0300

    drm/i915: Do a dummy DPCD read before the actual read

> -
> -	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
>   */
>  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. */
> @@ -3825,8 +3795,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);
> @@ -3837,9 +3807,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");
> @@ -3850,9 +3820,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;
> @@ -3868,15 +3838,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]);
> @@ -3899,9 +3869,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;
> @@ -3915,11 +3885,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]);
>  }
> @@ -3935,7 +3905,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;
> @@ -4072,9 +4042,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
> @@ -4082,9 +4052,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;
>  
> @@ -4343,8 +4313,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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 0/8] Organize and offload aux retries to drm. (v2)

[Daniel Stone]

Hi Rodrigo,

On 21 November 2015 at 00:46, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> The goal of this series is to remove many different retries we have
> for aux communication and offload them to drm.
>
> However on first attempt I was only returning EBUSY to use drm retries
> but there was no waiting there. So this series also introduce a new
> approach on drm level to retry on aux communication failures:
>
> 1 - EAGAIN: Immediatelly retry since HW might already taken care of the
>     needed waitings.
> 2 - EBUSY: Something is probably busy and we are not getting what we
>     expected so wait 1ms before trying again so we give time to aux
>     channels to recover.

I know there are still some review comments to resolve, but for the
series (except Nouveau):
Tested-by: Daniel Stone <daniels@collabora.com> # SKL

I used to see this pretty frequently, but not since.

Cheers,
Daniel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel