[PATCH] drm/i915/i2c: Track users of GMBUS force-bit

[PATCH] drm/i915/i2c: Track users of GMBUS force-bit

[Chris Wilson]

This fixes a regression for SDVO from

commit fbfcc4f3a0cf8bbde87646b74241faa8e37426bf
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Mon Oct 22 16:12:18 2012 +0300

    drm/i915/sdvo: restore i2c adapter config on intel_sdvo_init() failures

As SDVOB and SDVOC are multiplexed on the same pin, if a chipset does
not have the second SDVO encoder, it will then remove the force-bit
setting on the common i2c adapter during teardown. All subsequent
attempts of trying to use GMBUS with SDVOB then fail.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |    2 +-
 drivers/gpu/drm/i915/intel_i2c.c |    9 ++++++---
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c2daad1..cf407b1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -394,7 +394,7 @@ struct intel_fbc_work;
 
 struct intel_gmbus {
 	struct i2c_adapter adapter;
-	bool force_bit;
+	u32 force_bit;
 	u32 reg0;
 	u32 gpio_reg;
 	struct i2c_algo_bit_data bit_algo;
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 84b68a6..f8ca0be 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -482,7 +482,7 @@ timeout:
 	I915_WRITE(GMBUS0 + reg_offset, 0);
 
 	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
-	bus->force_bit = true;
+	bus->force_bit = 1;
 	ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
 
 out:
@@ -542,7 +542,7 @@ int intel_setup_gmbus(struct drm_device *dev)
 
 		/* gmbus seems to be broken on i830 */
 		if (IS_I830(dev))
-			bus->force_bit = true;
+			bus->force_bit = 1;
 
 		intel_gpio_setup(bus, port);
 
@@ -583,7 +583,10 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
 {
 	struct intel_gmbus *bus = to_intel_gmbus(adapter);
 
-	bus->force_bit = force_bit;
+	bus->force_bit += force_bit ? 1 : -1;
+	DRM_DEBUG_KMS("%sabling bit-banging on %s. force bit now %d\n",
+		      force_bit ? "dis" : "en", adapter->name,
+		      bus->force_bit);
 }
 
 void intel_teardown_gmbus(struct drm_device *dev)
-- 
1.7.10.4

Re: [PATCH] drm/i915/i2c: Track users of GMBUS force-bit

[Daniel Vetter]

On Sat, Nov 10, 2012 at 03:58:21PM +0000, Chris Wilson wrote:
> This fixes a regression for SDVO from
> 
> commit fbfcc4f3a0cf8bbde87646b74241faa8e37426bf
> Author: Jani Nikula <jani.nikula@intel.com>
> Date:   Mon Oct 22 16:12:18 2012 +0300
> 
>     drm/i915/sdvo: restore i2c adapter config on intel_sdvo_init() failures
> 
> As SDVOB and SDVOC are multiplexed on the same pin, if a chipset does
> not have the second SDVO encoder, it will then remove the force-bit
> setting on the common i2c adapter during teardown. All subsequent
> attempts of trying to use GMBUS with SDVOB then fail.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>

Fixes annoyingly loud WARN spam on my i915G w/ sdvo. Oh, how wonderful it
would be when our QA would run more than 5 machines and be able to report
such regression timely and with a bisect result ...

/me keeps on dreaming

Thanks a lot for tracking this down, patch merged to dinq.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915/i2c: Track users of GMBUS force-bit

[Jani Nikula]

On Sat, 10 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> This fixes a regression for SDVO from
>
> commit fbfcc4f3a0cf8bbde87646b74241faa8e37426bf
> Author: Jani Nikula <jani.nikula@intel.com>
> Date:   Mon Oct 22 16:12:18 2012 +0300
>
>     drm/i915/sdvo: restore i2c adapter config on intel_sdvo_init() failures
>
> As SDVOB and SDVOC are multiplexed on the same pin, if a chipset does
> not have the second SDVO encoder, it will then remove the force-bit
> setting on the common i2c adapter during teardown. All subsequent
> attempts of trying to use GMBUS with SDVOB then fail.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |    2 +-
>  drivers/gpu/drm/i915/intel_i2c.c |    9 ++++++---
>  2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c2daad1..cf407b1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -394,7 +394,7 @@ struct intel_fbc_work;
>  
>  struct intel_gmbus {
>  	struct i2c_adapter adapter;
> -	bool force_bit;
> +	u32 force_bit;
>  	u32 reg0;
>  	u32 gpio_reg;
>  	struct i2c_algo_bit_data bit_algo;
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 84b68a6..f8ca0be 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -482,7 +482,7 @@ timeout:
>  	I915_WRITE(GMBUS0 + reg_offset, 0);
>  
>  	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
> -	bus->force_bit = true;
> +	bus->force_bit = 1;
>  	ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
>  
>  out:
> @@ -542,7 +542,7 @@ int intel_setup_gmbus(struct drm_device *dev)
>  
>  		/* gmbus seems to be broken on i830 */
>  		if (IS_I830(dev))
> -			bus->force_bit = true;
> +			bus->force_bit = 1;
>  
>  		intel_gpio_setup(bus, port);
>  
> @@ -583,7 +583,10 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
>  {
>  	struct intel_gmbus *bus = to_intel_gmbus(adapter);
>  
> -	bus->force_bit = force_bit;
> +	bus->force_bit += force_bit ? 1 : -1;
> +	DRM_DEBUG_KMS("%sabling bit-banging on %s. force bit now %d\n",
> +		      force_bit ? "dis" : "en", adapter->name,
> +		      bus->force_bit);

To be honest, I think this creates a mine for someone to step on.

bus->force_bit is checked using if (bus->force_bit). So in the default
state, doing either intel_gmbus_force_bit(true) *or*
intel_gmbus_force_bit(false) will enable force_bit.

bus->force_bit is implicitly set to 1 on timeout and IS_I830(), so you
also can't assume a neat pair of intel_gmbus_force_bit()s will always
work.

So, good piece of debugging there (and sorry about causing that in the
first place!) but I think we should be more careful with the
implementation. Maybe changing the ifs to (bus->force_bit > 0) and
always using intel_gmbus_force_bit() also internally (instead of setting
to true) are enough, but perhaps you have even better ideas.

BR,
Jani.



>  }
>  
>  void intel_teardown_gmbus(struct drm_device *dev)
> -- 
> 1.7.10.4

Re: [PATCH] drm/i915/i2c: Track users of GMBUS force-bit

[Jani Nikula]

On Sat, 10 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> @@ -583,7 +583,10 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
>  {
>  	struct intel_gmbus *bus = to_intel_gmbus(adapter);
>  
> -	bus->force_bit = force_bit;
> +	bus->force_bit += force_bit ? 1 : -1;
> +	DRM_DEBUG_KMS("%sabling bit-banging on %s. force bit now %d\n",
> +		      force_bit ? "dis" : "en", adapter->name,
> +		      bus->force_bit);

Reading a dmesg from Chris, I noticed force_bit ? "dis" : "en" is
backwards.

BR,
Jani.

Re: [PATCH] drm/i915/i2c: Track users of GMBUS force-bit

[Daniel Vetter]

On Tue, Nov 13, 2012 at 04:39:43PM +0200, Jani Nikula wrote:
> On Sat, 10 Nov 2012, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > @@ -583,7 +583,10 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
> >  {
> >  	struct intel_gmbus *bus = to_intel_gmbus(adapter);
> >  
> > -	bus->force_bit = force_bit;
> > +	bus->force_bit += force_bit ? 1 : -1;
> > +	DRM_DEBUG_KMS("%sabling bit-banging on %s. force bit now %d\n",
> > +		      force_bit ? "dis" : "en", adapter->name,
> > +		      bus->force_bit);
> 
> Reading a dmesg from Chris, I noticed force_bit ? "dis" : "en" is
> backwards.

Thanks for spotting this, rebase out, it never happened.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch