[PATCH] drm/i915: don't return -ENXIO from gmbus xfer

[PATCH] drm/i915: don't return -ENXIO from gmbus xfer

[Daniel Vetter]

... too much risk for flaky edid transfers.

Cc: Daniel Kurtz <djkurtz@chromium.org>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49518
Reported-and-Tested-by: Julian Simioni <julian.simioni@gmail.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_i2c.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index e04255e..0588d8e 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -418,10 +418,11 @@ clear_err:
 	 * If no ACK is received during the address phase of a transaction,
 	 * the adapter must report -ENXIO.
 	 * It is not clear what to return if no ACK is received at other times.
-	 * So, we always return -ENXIO in all NAK cases, to ensure we send
-	 * it at least during the one case that is specified.
+	 *
+	 * Unfortunately we can't afford false positives in returning -ENXIO,
+	 * hence never return -ENXIO.
 	 */
-	ret = -ENXIO;
+	ret = i;
 	goto out;
 
 timeout:
-- 
1.7.10

[PATCH] drm/i915: don't return -ENXIO from gmbus xfer

[Daniel Vetter]

... too much risk for flaky edid transfers.

This regression has been introduced in

commit e646d5773572bf52017983d758bdf05777dc5600
Author: Daniel Kurtz <djkurtz@chromium.org>
Date:   Fri Mar 30 19:46:38 2012 +0800

    drm/i915/intel_i2c: always wait for IDLE before clearing NAK

This patch keeps the improved NAK handling on the hw side, but reverts
the change to return -ENXIO in case the gmbus controller reports a
NAK.

Cc: Daniel Kurtz <djkurtz@chromium.org>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49518
Reported-and-Tested-by: Julian Simioni <julian.simioni@gmail.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_i2c.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index e04255e..0588d8e 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -418,10 +418,11 @@ clear_err:
 	 * If no ACK is received during the address phase of a transaction,
 	 * the adapter must report -ENXIO.
 	 * It is not clear what to return if no ACK is received at other times.
-	 * So, we always return -ENXIO in all NAK cases, to ensure we send
-	 * it at least during the one case that is specified.
+	 *
+	 * Unfortunately we can't afford false positives in returning -ENXIO,
+	 * hence never return -ENXIO.
 	 */
-	ret = -ENXIO;
+	ret = i;
 	goto out;
 
 timeout:
-- 
1.7.10

Re: [PATCH] drm/i915: don't return -ENXIO from gmbus xfer

[Daniel Vetter]

On Sat, May 19, 2012 at 10:10:12PM +0200, Daniel Vetter wrote:
> ... too much risk for flaky edid transfers.
> 
> This regression has been introduced in
> 
> commit e646d5773572bf52017983d758bdf05777dc5600
> Author: Daniel Kurtz <djkurtz@chromium.org>
> Date:   Fri Mar 30 19:46:38 2012 +0800
> 
>     drm/i915/intel_i2c: always wait for IDLE before clearing NAK
> 
> This patch keeps the improved NAK handling on the hw side, but reverts
> the change to return -ENXIO in case the gmbus controller reports a
> NAK.
> 
> Cc: Daniel Kurtz <djkurtz@chromium.org>

Hi Daniel,

Can you please take a look at this one and smash your r-b onto it if you
agree?

Thanks, Daniel

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49518
> Reported-and-Tested-by: Julian Simioni <julian.simioni@gmail.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index e04255e..0588d8e 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -418,10 +418,11 @@ clear_err:
>  	 * If no ACK is received during the address phase of a transaction,
>  	 * the adapter must report -ENXIO.
>  	 * It is not clear what to return if no ACK is received at other times.
> -	 * So, we always return -ENXIO in all NAK cases, to ensure we send
> -	 * it at least during the one case that is specified.
> +	 *
> +	 * Unfortunately we can't afford false positives in returning -ENXIO,
> +	 * hence never return -ENXIO.
>  	 */
> -	ret = -ENXIO;
> +	ret = i;
>  	goto out;
>  
>  timeout:
> -- 
> 1.7.10
> 

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: don't return -ENXIO from gmbus xfer

[Daniel Vetter]

On Sun, May 20, 2012 at 11:07:46AM -0700, Daniel Kurtz wrote:
> On Sun, May 20, 2012 at 8:19 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Sat, May 19, 2012 at 10:10:12PM +0200, Daniel Vetter wrote:
> > > ... too much risk for flaky edid transfers.
> > >
> > > This regression has been introduced in
> > >
> > > commit e646d5773572bf52017983d758bdf05777dc5600
> > > Author: Daniel Kurtz <djkurtz@chromium.org>
> > > Date:   Fri Mar 30 19:46:38 2012 +0800
> > >
> > >     drm/i915/intel_i2c: always wait for IDLE before clearing NAK
> > >
> > > This patch keeps the improved NAK handling on the hw side, but reverts
> > > the change to return -ENXIO in case the gmbus controller reports a
> > > NAK.
> > >
> > > Cc: Daniel Kurtz <djkurtz@chromium.org>
> >
> > Hi Daniel,
> >
> > Can you please take a look at this one and smash your r-b onto it if you
> > agree?
> >
> > Thanks, Daniel
> >
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49518
> > > Reported-and-Tested-by: Julian Simioni <julian.simioni@gmail.com>
> > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/intel_i2c.c |    7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > > index e04255e..0588d8e 100644
> > > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > > @@ -418,10 +418,11 @@ clear_err:
> > >        * If no ACK is received during the address phase of a transaction,
> > >        * the adapter must report -ENXIO.
> > >        * It is not clear what to return if no ACK is received at other times.
> > > -      * So, we always return -ENXIO in all NAK cases, to ensure we send
> > > -      * it at least during the one case that is specified.
> > > +      *
> > > +      * Unfortunately we can't afford false positives in returning -ENXIO,
> > > +      * hence never return -ENXIO.
> > >        */
> > > -     ret = -ENXIO;
> > > +     ret = i;
> 
> 
> The bugzilla report shows that in the old case, the gmbus failure was:
> [    2.528812] vgaarb: device changed decodes:
> PCI:0000:00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem
> [    2.540753] [drm] GMBUS [i915 gmbus panel] timed out waiting for idle
> [    2.580813] [drm:intel_panel_get_backlight], get backlight PWM = 0
> 
> And now the failure is:
> [    2.523015] vgaarb: device changed decodes:
> PCI:0000:00:02.0,olddecodes=io+mem,decodes=io+mem:owns=io+mem
> [    2.534770] [drm] GMBUS [i915 gmbus panel] timed out after NAK
> [    2.534797] [drm:gmbus_xfer], GMBUS [i915 gmbus panel] NAK for
> addr: 0050 w(1)
> [    2.534883] [drm:intel_panel_get_backlight], get backlight PWM = 0
> 
> From these logs, it looks like in both cases, there is an i2c
> communication error between host and panel.  The difference is that:
>   * in the old case, the return value was 0, which triggered a silent
> retry (the 40ms gap between GMBUS and PWM messages)
>   * but, now that the return value is -ENXIO, the caller does not retry.
> 
> Actually, in the 'new' case, there are two errors happening: a NAK and
> then a timeout after the NAK, while waiting for the controller to
> clear the ACTIVE bit.  I'm not actually sure what causes "timeout
> after NAK", but I think it means the controller is waiting for entire
> transaction to complete (perhaps the STOP bit after NAK?).
> 
> Since this reported issue is happening in this double error path, I'd
> rather a patch that fixes it without disabling the more generic NAK
> path.  Maybe something like this:

Hm, good suggestion. This way, if we get a NAK but no issues later on,
we'll still fail faster if gmbus detected that no device is responding.
Which should speed up boot a bit. I'll run this past the bug reporter.

Thanks, Daniel

> 
> clear_err:
> 	/*
> 	 * Wait for bus to IDLE before clearing NAK.
> 	 * If we clear the NAK while bus is still active, then it will stay
> 	 * active and the next transaction may fail.
> 	 */
> +      ret = -ENXIO;
> 	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
> -		     10))
> +		     10)) {
> 		DRM_DEBUG_KMS("GMBUS [%s] timed out after NAK\n",
> 			      adapter->name);
> +		ret = -ETIMEDOUT;  // Or 0 ?
> +	}
> 
> 	/* Toggle the Software Clear Interrupt bit. This has the effect
> 	 * of resetting the GMBUS controller and so clearing the
> 	 * BUS_ERROR raised by the slave's NAK.
> 	 */
> 	I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
> 	I915_WRITE(GMBUS1 + reg_offset, 0);
> 	I915_WRITE(GMBUS0 + reg_offset, 0);
> 
> 	DRM_DEBUG_KMS("GMBUS [%s] NAK for addr: %04x %c(%d)\n",
> 			 adapter->name, msgs[i].addr,
> 			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
> 
> 	/*
> 	 * If no ACK is received during the address phase of a transaction,
> 	 * the adapter must report -ENXIO.
> 	 * It is not clear what to return if no ACK is received at other times.
> -	 * So, we always return -ENXIO in all NAK cases, to ensure we send
> -	 * it at least during the one case that is specified.
> +	 * So, return -ENXIO for NAK after any byte, unless there was a timeout
> +	 * while waiting for IDLE after NAK.
> 	 */
> -	ret = -ENXIO;
> 	goto out;
> 
> -Daniel
> 
> >
> > >       goto out;
> > >
> > >  timeout:
> > > --
> > > 1.7.10
> > >
> >
> > --
> > Daniel Vetter
> > Mail: daniel@ffwll.ch
> > Mobile: +41 (0)79 365 57 48

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] drm/i915: be more careful when returning -ENXIO in gmbus transfer

[Daniel Vetter]

... flaky ddc hardware can cause a spurious NAK, resulting in the i2c
core and drm edid functions not trying to retry the edid transfer.

Luckily the gmbus quiescenting also times out for these cases, so we
can get out of this mess by returning -ETIMEDOUT for this specific
case. This way we keep the fast-fail of returning -ENXIO if there is
no device present, speeding up the boot process.

This regression has been introduced in

commit e646d5773572bf52017983d758bdf05777dc5600
Author: Daniel Kurtz <djkurtz@chromium.org>
Date:   Fri Mar 30 19:46:38 2012 +0800

    drm/i915/intel_i2c: always wait for IDLE before clearing NAK

v2: Return -ETIMEDOUT for this case and keep the -ENXIO for real NAKs,
suggested by Daniel Kurtz.

Cc: Daniel Kurtz <djkurtz@chromium.org>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49518
Reported-and-Tested-by: Julian Simioni <julian.simioni@gmail.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_i2c.c |   21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index e04255e..892477b 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -396,11 +396,22 @@ clear_err:
 	 * Wait for bus to IDLE before clearing NAK.
 	 * If we clear the NAK while bus is still active, then it will stay
 	 * active and the next transaction may fail.
+	 *
+	 * If no ACK is received during the address phase of a transaction, the
+	 * adapter must report -ENXIO. It is not clear what to return if no ACK
+	 * is received at other times. But we have to be careful to not return
+	 * spurious -ENXIO because that will prevent i2c and drm edid functions
+	 * from retrying. So return -ENXIO only when gmbus properly quiescents -
+	 * timing out seems to happen when there _is_ a ddc chip present, but
+	 * it's slow responding and only answers on the 2nd retry.
 	 */
+	ret = -ENXIO;
 	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
-		     10))
+		     10)) {
 		DRM_DEBUG_KMS("GMBUS [%s] timed out after NAK\n",
 			      adapter->name);
+		ret = -ETIMEDOUT;
+	}
 
 	/* Toggle the Software Clear Interrupt bit. This has the effect
 	 * of resetting the GMBUS controller and so clearing the
@@ -414,14 +425,6 @@ clear_err:
 			 adapter->name, msgs[i].addr,
 			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
 
-	/*
-	 * If no ACK is received during the address phase of a transaction,
-	 * the adapter must report -ENXIO.
-	 * It is not clear what to return if no ACK is received at other times.
-	 * So, we always return -ENXIO in all NAK cases, to ensure we send
-	 * it at least during the one case that is specified.
-	 */
-	ret = -ENXIO;
 	goto out;
 
 timeout:
-- 
1.7.10

Re: [PATCH] drm/i915: don't return -ENXIO from gmbus xfer

[Daniel Vetter]

On Sun, May 20, 2012 at 08:16:46PM +0200, Daniel Vetter wrote:
> On Sun, May 20, 2012 at 11:07:46AM -0700, Daniel Kurtz wrote:
> > Since this reported issue is happening in this double error path, I'd
> > rather a patch that fixes it without disabling the more generic NAK
> > path.  Maybe something like this:
> 
> Hm, good suggestion. This way, if we get a NAK but no issues later on,
> we'll still fail faster if gmbus detected that no device is responding.
> Which should speed up boot a bit. I'll run this past the bug reporter.

We're lucky, and the modified patch tested ok by the bug reporter. I'll
follow up with v2, please smash your r-b onto it if you're satisfied.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: be more careful when returning -ENXIO in gmbus transfer

[Chris Wilson]

On Mon, 21 May 2012 20:19:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> ... flaky ddc hardware can cause a spurious NAK, resulting in the i2c
> core and drm edid functions not trying to retry the edid transfer.
> 
> Luckily the gmbus quiescenting also times out for these cases, so we
> can get out of this mess by returning -ETIMEDOUT for this specific
> case. This way we keep the fast-fail of returning -ENXIO if there is
> no device present, speeding up the boot process.
> 
> This regression has been introduced in
> 
> commit e646d5773572bf52017983d758bdf05777dc5600
> Author: Daniel Kurtz <djkurtz@chromium.org>
> Date:   Fri Mar 30 19:46:38 2012 +0800
> 
>     drm/i915/intel_i2c: always wait for IDLE before clearing NAK
> 
> v2: Return -ETIMEDOUT for this case and keep the -ENXIO for real NAKs,
> suggested by Daniel Kurtz.
> 
> Cc: Daniel Kurtz <djkurtz@chromium.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49518
> Reported-and-Tested-by: Julian Simioni <julian.simioni@gmail.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: be more careful when returning -ENXIO in gmbus transfer

[Daniel Vetter]

On Mon, May 21, 2012 at 11:31:20AM -0700, Daniel Kurtz wrote:
> On Mon, May 21, 2012 at 11:24 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, 21 May 2012 20:19:48 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >> ... flaky ddc hardware can cause a spurious NAK, resulting in the i2c
> >> core and drm edid functions not trying to retry the edid transfer.
> >>
> >> Luckily the gmbus quiescenting also times out for these cases, so we
> >> can get out of this mess by returning -ETIMEDOUT for this specific
> >> case. This way we keep the fast-fail of returning -ENXIO if there is
> >> no device present, speeding up the boot process.
> >>
> >> This regression has been introduced in
> >>
> >> commit e646d5773572bf52017983d758bdf05777dc5600
> >> Author: Daniel Kurtz <djkurtz@chromium.org>
> >> Date:   Fri Mar 30 19:46:38 2012 +0800
> >>
> >>     drm/i915/intel_i2c: always wait for IDLE before clearing NAK
> >>
> >> v2: Return -ETIMEDOUT for this case and keep the -ENXIO for real NAKs,
> >> suggested by Daniel Kurtz.
> >>
> >> Cc: Daniel Kurtz <djkurtz@chromium.org>
> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49518
> >> Reported-and-Tested-by: Julian Simioni <julian.simioni@gmail.com>
> >> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
> > -Chris
> >
> > --
> > Chris Wilson, Intel Open Source Technology Centre
> 
> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
Picked up for -fixes, thanks for the review.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48