| Message ID | 1352563101-1701-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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)
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(-)