Re: [Intel-gfx] [PATCH] drm/i915: GMBUS don't need no forcewake
On Wed, Oct 12, 2016 at 03:39:47PM +0300, Ville Syrjälä wrote:
> On Wed, Oct 12, 2016 at 12:58:34PM +0100, Chris Wilson wrote:
> > On Wed, Oct 12, 2016 at 02:44:47PM +0300, ville.syrj...@linux.intel.com
> > wrote:
> > > From: Ville Syrjälä
> > >
> > > GMBUS is part of the display engine, and thus has no need for
> > > forcewake. Let's not bother trying to grab it then.
> > >
> > > I don't recall if the display engine suffers from system hangs
> > > due to multiple accesses to the same "cacheline" in mmio space.
> > > I hope not since we're no longer protected by the uncore lock
> > > since commit 4e6c2d58ba86 ("drm/i915: Take forcewake once for
> > > the entire GMBUS transaction")
> >
> > Only applies to concurrent access to the same cacheline, in this case
> > should be serialised by the mutex around the gmbus xfer.
>
> Hmm. Yeah, I suppose there shouldn't be unrelated stuff nearby. Haven't
> double checked though.
>
> >
> > > Cc: Chris Wilson
> > > Cc: David Weinehall
> > > Signed-off-by: Ville Syrjälä
> > > ---
> > > drivers/gpu/drm/i915/intel_i2c.c | 5 -
> > > 1 file changed, 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c
> > > b/drivers/gpu/drm/i915/intel_i2c.c
> > > index 79aab9ad6faa..49c7824a4c29 100644
> > > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > > @@ -468,13 +468,9 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct
> > > i2c_msg *msgs, int num)
> > > struct intel_gmbus,
> > > adapter);
> > > struct drm_i915_private *dev_priv = bus->dev_priv;
> > > - const unsigned int fw =
> > > - intel_uncore_forcewake_for_reg(dev_priv, GMBUS0,
> > > -FW_REG_READ | FW_REG_WRITE);
> > > int i = 0, inc, try = 0;
> > > int ret = 0;
> >
> > I915_WARN_ON(intel_uncore_forcewake_for_reg(dev_priv, GMBUS0,
> > FW_REG_READ |
> > FW_REG_WRITE));
> >
> > ? Would be good to test the fw handling as well.
>
> Not sure I'd want to sprinkle forcewake testing into modeset code.
You never use registers here? ;)
Reviewed-by: Chris Wilson
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: GMBUS don't need no forcewake
On Wed, Oct 12, 2016 at 12:58:34PM +0100, Chris Wilson wrote:
> On Wed, Oct 12, 2016 at 02:44:47PM +0300, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä
> >
> > GMBUS is part of the display engine, and thus has no need for
> > forcewake. Let's not bother trying to grab it then.
> >
> > I don't recall if the display engine suffers from system hangs
> > due to multiple accesses to the same "cacheline" in mmio space.
> > I hope not since we're no longer protected by the uncore lock
> > since commit 4e6c2d58ba86 ("drm/i915: Take forcewake once for
> > the entire GMBUS transaction")
>
> Only applies to concurrent access to the same cacheline, in this case
> should be serialised by the mutex around the gmbus xfer.
Hmm. Yeah, I suppose there shouldn't be unrelated stuff nearby. Haven't
double checked though.
>
> > Cc: Chris Wilson
> > Cc: David Weinehall
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/intel_i2c.c | 5 -
> > 1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_i2c.c
> > b/drivers/gpu/drm/i915/intel_i2c.c
> > index 79aab9ad6faa..49c7824a4c29 100644
> > --- a/drivers/gpu/drm/i915/intel_i2c.c
> > +++ b/drivers/gpu/drm/i915/intel_i2c.c
> > @@ -468,13 +468,9 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct
> > i2c_msg *msgs, int num)
> >struct intel_gmbus,
> >adapter);
> > struct drm_i915_private *dev_priv = bus->dev_priv;
> > - const unsigned int fw =
> > - intel_uncore_forcewake_for_reg(dev_priv, GMBUS0,
> > - FW_REG_READ | FW_REG_WRITE);
> > int i = 0, inc, try = 0;
> > int ret = 0;
>
> I915_WARN_ON(intel_uncore_forcewake_for_reg(dev_priv, GMBUS0,
> FW_REG_READ |
> FW_REG_WRITE));
>
> ? Would be good to test the fw handling as well.
Not sure I'd want to sprinkle forcewake testing into modeset code.
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: GMBUS don't need no forcewake
On Wed, Oct 12, 2016 at 02:44:47PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä
>
> GMBUS is part of the display engine, and thus has no need for
> forcewake. Let's not bother trying to grab it then.
>
> I don't recall if the display engine suffers from system hangs
> due to multiple accesses to the same "cacheline" in mmio space.
> I hope not since we're no longer protected by the uncore lock
> since commit 4e6c2d58ba86 ("drm/i915: Take forcewake once for
> the entire GMBUS transaction")
Only applies to concurrent access to the same cacheline, in this case
should be serialised by the mutex around the gmbus xfer.
> Cc: Chris Wilson
> Cc: David Weinehall
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/intel_i2c.c | 5 -
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c
> b/drivers/gpu/drm/i915/intel_i2c.c
> index 79aab9ad6faa..49c7824a4c29 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -468,13 +468,9 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct
> i2c_msg *msgs, int num)
> struct intel_gmbus,
> adapter);
> struct drm_i915_private *dev_priv = bus->dev_priv;
> - const unsigned int fw =
> - intel_uncore_forcewake_for_reg(dev_priv, GMBUS0,
> -FW_REG_READ | FW_REG_WRITE);
> int i = 0, inc, try = 0;
> int ret = 0;
I915_WARN_ON(intel_uncore_forcewake_for_reg(dev_priv, GMBUS0,
FW_REG_READ |
FW_REG_WRITE));
? Would be good to test the fw handling as well.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
