[PATCH] drm/i915: Fix single msg gmbus_xfers writes

[PATCH] drm/i915: Fix single msg gmbus_xfers writes

[Benson Leung]

gmbus_xfer with a single message (particularly a single message write) would
set Bus Cycle Select to 100b, the Gen Stop cycle, instead of 101b,
No Index, Stop cycle. This would not start single message i2c transactions.

Also, gmbus_xfer done: will disable the interface without checking if
it is idle. In the case of writes, there will be no wait on status or delay
to ensure the write starts and completes before the interface is turned off.

Fixed the former issue by using the same cycle selection as used in the
I2C_M_RD for the write case.
GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0)
Fixed the latter by waiting on GMBUS_ACTIVE to deassert before disable.

Signed-off-by: Benson Leung <bleung@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d30cccc..64bb9cd 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -249,7 +249,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
 
 		if (msgs[i].flags & I2C_M_RD) {
 			I915_WRITE(GMBUS1 + reg_offset,
-				   GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
+				   GMBUS_CYCLE_WAIT |
+				   (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
 				   (len << GMBUS_BYTE_COUNT_SHIFT) |
 				   (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
 				   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
@@ -278,7 +279,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
 
 			I915_WRITE(GMBUS3 + reg_offset, val);
 			I915_WRITE(GMBUS1 + reg_offset,
-				   (i + 1 == num ? GMBUS_CYCLE_STOP : GMBUS_CYCLE_WAIT) |
+				   GMBUS_CYCLE_WAIT |
+				   (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
 				   (msgs[i].len << GMBUS_BYTE_COUNT_SHIFT) |
 				   (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
 				   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
@@ -317,9 +319,12 @@ clear_err:
 	I915_WRITE(GMBUS1 + reg_offset, 0);
 
 done:
-	/* Mark the GMBUS interface as disabled. We will re-enable it at the
-	 * start of the next xfer, till then let it sleep.
+	/* Mark the GMBUS interface as disabled after waiting for idle.
+	 * We will re-enable it at the start of the next xfer,
+	 * till then let it sleep.
 	 */
+	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
+		DRM_INFO("GMBUS timed out waiting for idle\n");
 	I915_WRITE(GMBUS0 + reg_offset, 0);
 	return i;
 
-- 
1.7.1

Re: [PATCH] drm/i915: Fix single msg gmbus_xfers writes

[Daniel Vetter]

On Thu, Feb 09, 2012 at 12:03:17PM -0800, Benson Leung wrote:
> gmbus_xfer with a single message (particularly a single message write) would
> set Bus Cycle Select to 100b, the Gen Stop cycle, instead of 101b,
> No Index, Stop cycle. This would not start single message i2c transactions.
> 
> Also, gmbus_xfer done: will disable the interface without checking if
> it is idle. In the case of writes, there will be no wait on status or delay
> to ensure the write starts and completes before the interface is turned off.
> 
> Fixed the former issue by using the same cycle selection as used in the
> I2C_M_RD for the write case.
> GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0)
> Fixed the latter by waiting on GMBUS_ACTIVE to deassert before disable.
> 
> Signed-off-by: Benson Leung <bleung@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index d30cccc..64bb9cd 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -249,7 +249,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  
>  		if (msgs[i].flags & I2C_M_RD) {
>  			I915_WRITE(GMBUS1 + reg_offset,
> -				   GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
> +				   GMBUS_CYCLE_WAIT |
> +				   (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
>  				   (len << GMBUS_BYTE_COUNT_SHIFT) |
>  				   (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
>  				   GMBUS_SLAVE_READ | GMBUS_SW_RDY);

This here looks like just a white-space change. But your commit message
sounds like it's not jsut write's that are affected by this issue and need
to be fixed. Can you please clear up this for easily confused me?

Thanks, Daniel

> @@ -278,7 +279,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  
>  			I915_WRITE(GMBUS3 + reg_offset, val);
>  			I915_WRITE(GMBUS1 + reg_offset,
> -				   (i + 1 == num ? GMBUS_CYCLE_STOP : GMBUS_CYCLE_WAIT) |
> +				   GMBUS_CYCLE_WAIT |
> +				   (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
>  				   (msgs[i].len << GMBUS_BYTE_COUNT_SHIFT) |
>  				   (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
>  				   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
> @@ -317,9 +319,12 @@ clear_err:
>  	I915_WRITE(GMBUS1 + reg_offset, 0);
>  
>  done:
> -	/* Mark the GMBUS interface as disabled. We will re-enable it at the
> -	 * start of the next xfer, till then let it sleep.
> +	/* Mark the GMBUS interface as disabled after waiting for idle.
> +	 * We will re-enable it at the start of the next xfer,
> +	 * till then let it sleep.
>  	 */
> +	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
> +		DRM_INFO("GMBUS timed out waiting for idle\n");
>  	I915_WRITE(GMBUS0 + reg_offset, 0);
>  	return i;
>  
> -- 
> 1.7.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH] drm/i915: Fix single msg gmbus_xfers writes

[Chris Wilson]

On Mon, 13 Feb 2012 21:38:38 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> This here looks like just a white-space change. But your commit message
> sounds like it's not jsut write's that are affected by this issue and need
> to be fixed. Can you please clear up this for easily confused me?

It makes the write path similar to the read path. I admit to overlooking
the single write message as everything I saw was always a write followed
by a read. The patch looks correct, but I need to stare at it a bit more
before I r-b.

However, the outstanding issue is that we need to separate the gmbus
i2c adapter from the gpio i2c adapter in order for the pairing to work
with the expectations of the i2c core.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Fix single msg gmbus_xfers writes

[Daniel Vetter]

On Mon, Feb 13, 2012 at 09:49:35PM +0000, Chris Wilson wrote:
> On Mon, 13 Feb 2012 21:38:38 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > This here looks like just a white-space change. But your commit message
> > sounds like it's not jsut write's that are affected by this issue and need
> > to be fixed. Can you please clear up this for easily confused me?
> 
> It makes the write path similar to the read path. I admit to overlooking
> the single write message as everything I saw was always a write followed
> by a read. The patch looks correct, but I need to stare at it a bit more
> before I r-b.
> 
> However, the outstanding issue is that we need to separate the gmbus
> i2c adapter from the gpio i2c adapter in order for the pairing to work
> with the expectations of the i2c core.

There's another patch floating on dri-devel to add a gmbus_mutex. The
issue it tries to avoid is that multiple users corrupting each anothers
state when using the single gmbus controller. See "[PATCH] [PATCH]
drm/i915: Fix race condition in accessing GMBUS". Or have you thought of
another interaction that we need to sort out?
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: Fix single msg gmbus_xfers writes

[Chris Wilson]

On Mon, 13 Feb 2012 23:26:23 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Feb 13, 2012 at 09:49:35PM +0000, Chris Wilson wrote:
> > On Mon, 13 Feb 2012 21:38:38 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > However, the outstanding issue is that we need to separate the gmbus
> > i2c adapter from the gpio i2c adapter in order for the pairing to work
> > with the expectations of the i2c core.
> 
> There's another patch floating on dri-devel to add a gmbus_mutex. The
> issue it tries to avoid is that multiple users corrupting each anothers
> state when using the single gmbus controller. See "[PATCH] [PATCH]
> drm/i915: Fix race condition in accessing GMBUS". Or have you thought of
> another interaction that we need to sort out?

The GMBUS interface is currently disabled because we confused the i2c
core.
  https://bugzilla.kernel.org/show_bug.cgi?id=35572
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Fix single msg gmbus_xfers writes

[Daniel Vetter]

On Thu, Feb 09, 2012 at 12:03:17PM -0800, Benson Leung wrote:
> gmbus_xfer with a single message (particularly a single message write) would
> set Bus Cycle Select to 100b, the Gen Stop cycle, instead of 101b,
> No Index, Stop cycle. This would not start single message i2c transactions.
> 
> Also, gmbus_xfer done: will disable the interface without checking if
> it is idle. In the case of writes, there will be no wait on status or delay
> to ensure the write starts and completes before the interface is turned off.
> 
> Fixed the former issue by using the same cycle selection as used in the
> I2C_M_RD for the write case.
> GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0)
> Fixed the latter by waiting on GMBUS_ACTIVE to deassert before disable.
> 
> Signed-off-by: Benson Leung <bleung@chromium.org>

Can you clarify the commit message a bit and say that the first hunk is
just for optics and the issue is only with the write path (because the
read path is correct already). Silly me is just to easily confused ;-)

Btw, I've reworked the gmbus -> gpio bit-banging fallback code a bit and
if that passes review and all I'll reenable gmbus by default again. See

http://cgit.freedesktop.org/~danvet/drm/log/?h=gmbus

Yours, Daniel

> ---
>  drivers/gpu/drm/i915/intel_i2c.c |   13 +++++++++----
>  1 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index d30cccc..64bb9cd 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -249,7 +249,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  
>  		if (msgs[i].flags & I2C_M_RD) {
>  			I915_WRITE(GMBUS1 + reg_offset,
> -				   GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
> +				   GMBUS_CYCLE_WAIT |
> +				   (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
>  				   (len << GMBUS_BYTE_COUNT_SHIFT) |
>  				   (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
>  				   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
> @@ -278,7 +279,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  
>  			I915_WRITE(GMBUS3 + reg_offset, val);
>  			I915_WRITE(GMBUS1 + reg_offset,
> -				   (i + 1 == num ? GMBUS_CYCLE_STOP : GMBUS_CYCLE_WAIT) |
> +				   GMBUS_CYCLE_WAIT |
> +				   (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
>  				   (msgs[i].len << GMBUS_BYTE_COUNT_SHIFT) |
>  				   (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
>  				   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
> @@ -317,9 +319,12 @@ clear_err:
>  	I915_WRITE(GMBUS1 + reg_offset, 0);
>  
>  done:
> -	/* Mark the GMBUS interface as disabled. We will re-enable it at the
> -	 * start of the next xfer, till then let it sleep.
> +	/* Mark the GMBUS interface as disabled after waiting for idle.
> +	 * We will re-enable it at the start of the next xfer,
> +	 * till then let it sleep.
>  	 */
> +	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
> +		DRM_INFO("GMBUS timed out waiting for idle\n");
>  	I915_WRITE(GMBUS0 + reg_offset, 0);
>  	return i;
>  
> -- 
> 1.7.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH] drm/i915: Fix single msg gmbus_xfers writes

[Daniel Kurtz]

On Feb 15, 2012 6:48 PM, "Daniel Vetter" <daniel@ffwll.ch> wrote:
>
> On Thu, Feb 09, 2012 at 12:03:17PM -0800, Benson Leung wrote:
> > gmbus_xfer with a single message (particularly a single message write) would
> > set Bus Cycle Select to 100b, the Gen Stop cycle, instead of 101b,
> > No Index, Stop cycle. This would not start single message i2c transactions.
> >
> > Also, gmbus_xfer done: will disable the interface without checking if
> > it is idle. In the case of writes, there will be no wait on status or delay
> > to ensure the write starts and completes before the interface is turned off.
> >
> > Fixed the former issue by using the same cycle selection as used in the
> > I2C_M_RD for the write case.
> > GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0)
> > Fixed the latter by waiting on GMBUS_ACTIVE to deassert before disable.
> >
> > Signed-off-by: Benson Leung <bleung@chromium.org>

Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>

>
> Can you clarify the commit message a bit and say that the first hunk is
> just for optics and the issue is only with the write path (because the
> read path is correct already). Silly me is just to easily confused ;-)
>
> Btw, I've reworked the gmbus -> gpio bit-banging fallback code a bit and
> if that passes review and all I'll reenable gmbus by default again. See
>
> http://cgit.freedesktop.org/~danvet/drm/log/?h=gmbus

If the write case is fixed by Benson's patch, is there any known use
case that still requires i2c bit banging on these pins?  It would be a
nice cleanup to remove it completely.

Also, I can think of at least two further potential performance
improvements that I was wondering if anybody has yet pursued:
  (1) Enabling the i915's gmbus interrupt.  This would eliminate the
need for the (relatively slow) wait_for polling loop.
  (2) Taking advantage of the i915's "INDEX" cycles to combine writing
a (1 or 2 byte) address & reading back an array of bytes into a single
transaction.

Best Regards,
-Daniel

>
> Yours, Daniel
>
> > ---
> >  drivers/gpu/drm/i915/intel_i2c.c |   13 +++++++++----
> >  1 files changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> > index d30cccc..64bb9cd 100644
> > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > @@ -249,7 +249,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
> >
> >               if (msgs[i].flags & I2C_M_RD) {
> >                       I915_WRITE(GMBUS1 + reg_offset,
> > -                                GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
> > +                                GMBUS_CYCLE_WAIT |
> > +                                (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
> >                                  (len << GMBUS_BYTE_COUNT_SHIFT) |
> >                                  (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
> >                                  GMBUS_SLAVE_READ | GMBUS_SW_RDY);
> > @@ -278,7 +279,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
> >
> >                       I915_WRITE(GMBUS3 + reg_offset, val);
> >                       I915_WRITE(GMBUS1 + reg_offset,
> > -                                (i + 1 == num ? GMBUS_CYCLE_STOP : GMBUS_CYCLE_WAIT) |
> > +                                GMBUS_CYCLE_WAIT |
> > +                                (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
> >                                  (msgs[i].len << GMBUS_BYTE_COUNT_SHIFT) |
> >                                  (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
> >                                  GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
> > @@ -317,9 +319,12 @@ clear_err:
> >       I915_WRITE(GMBUS1 + reg_offset, 0);
> >
> >  done:
> > -     /* Mark the GMBUS interface as disabled. We will re-enable it at the
> > -      * start of the next xfer, till then let it sleep.
> > +     /* Mark the GMBUS interface as disabled after waiting for idle.
> > +      * We will re-enable it at the start of the next xfer,
> > +      * till then let it sleep.
> >        */
> > +     if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
> > +             DRM_INFO("GMBUS timed out waiting for idle\n");
> >       I915_WRITE(GMBUS0 + reg_offset, 0);
> >       return i;
> >
> > --
> > 1.7.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: Fix single msg gmbus_xfers writes

[Daniel Vetter]

On Mon, Feb 20, 2012 at 07:22:00PM +0800, Daniel Kurtz wrote:
> On Feb 15, 2012 6:48 PM, "Daniel Vetter" <daniel@ffwll.ch> wrote:
> >
> > On Thu, Feb 09, 2012 at 12:03:17PM -0800, Benson Leung wrote:
> > > gmbus_xfer with a single message (particularly a single message write) would
> > > set Bus Cycle Select to 100b, the Gen Stop cycle, instead of 101b,
> > > No Index, Stop cycle. This would not start single message i2c transactions.
> > >
> > > Also, gmbus_xfer done: will disable the interface without checking if
> > > it is idle. In the case of writes, there will be no wait on status or delay
> > > to ensure the write starts and completes before the interface is turned off.
> > >
> > > Fixed the former issue by using the same cycle selection as used in the
> > > I2C_M_RD for the write case.
> > > GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0)
> > > Fixed the latter by waiting on GMBUS_ACTIVE to deassert before disable.
> > >
> > > Signed-off-by: Benson Leung <bleung@chromium.org>
> 
> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
> 
> >
> > Can you clarify the commit message a bit and say that the first hunk is
> > just for optics and the issue is only with the write path (because the
> > read path is correct already). Silly me is just to easily confused ;-)
> >
> > Btw, I've reworked the gmbus -> gpio bit-banging fallback code a bit and
> > if that passes review and all I'll reenable gmbus by default again. See
> >
> > http://cgit.freedesktop.org/~danvet/drm/log/?h=gmbus
> 
> If the write case is fixed by Benson's patch, is there any known use
> case that still requires i2c bit banging on these pins?  It would be a
> nice cleanup to remove it completely.

Let's first see how much things blow up when re-enabling gmbus again ;-)

> Also, I can think of at least two further potential performance
> improvements that I was wondering if anybody has yet pursued:
>   (1) Enabling the i915's gmbus interrupt.  This would eliminate the
> need for the (relatively slow) wait_for polling loop.
>   (2) Taking advantage of the i915's "INDEX" cycles to combine writing
> a (1 or 2 byte) address & reading back an array of bytes into a single
> transaction.

Afaik no one looked into this, but patches are highly welcome.

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

Re: [PATCH] drm/i915: Fix single msg gmbus_xfers writes

[Daniel Vetter]

On Mon, Feb 20, 2012 at 07:22:00PM +0800, Daniel Kurtz wrote:
> On Feb 15, 2012 6:48 PM, "Daniel Vetter" <daniel@ffwll.ch> wrote:
> >
> > On Thu, Feb 09, 2012 at 12:03:17PM -0800, Benson Leung wrote:
> > > gmbus_xfer with a single message (particularly a single message write) would
> > > set Bus Cycle Select to 100b, the Gen Stop cycle, instead of 101b,
> > > No Index, Stop cycle. This would not start single message i2c transactions.
> > >
> > > Also, gmbus_xfer done: will disable the interface without checking if
> > > it is idle. In the case of writes, there will be no wait on status or delay
> > > to ensure the write starts and completes before the interface is turned off.
> > >
> > > Fixed the former issue by using the same cycle selection as used in the
> > > I2C_M_RD for the write case.
> > > GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0)
> > > Fixed the latter by waiting on GMBUS_ACTIVE to deassert before disable.
> > >
> > > Signed-off-by: Benson Leung <bleung@chromium.org>
> 
> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>

Queued for -next (with a grumpy noted added to the commit message), thanks
for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48