| Message ID | 1472054591-29619-1-git-send-email-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 24, 2016 at 05:03:11PM +0100, Matthew Auld wrote: > The comment which documents the proper usage of the *_FW family of macros makes > reference to intel_uncore_forcewake_irq{unlock, lock}, which is just > confusing, seeing as such a set of functions don't even exist and never have > for that matter(according to git). Let's fix that by replacing them with > intel_uncore_forcewake_{get, put}. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ff96b7a..c285d61 100644 > --- a/drivers/gpu/drm/i916/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3792,8 +3792,8 @@ __raw_write(64, q) > * critical sections inside IRQ handlers where forcewake is explicitly > * controlled. > * Think twice, and think again, before using these. > - * Note: Should only be used between intel_uncore_forcewake_irqlock() and > - * intel_uncore_forcewake_irqunlock(). > + * Note: Should only be used between intel_uncore_forcewake_get and > + * intel_uncore_forcewake_put. > */ > #define I915_READ_FW(reg__) __raw_i915_read32(dev_priv, (reg__)) > #define I915_WRITE_FW(reg__, val__) __raw_i915_write32(dev_priv, (reg__), (val__)) > -- > 2.7.4
On Fri, Oct 21, 2016 at 02:16:46PM +0200, Arkadiusz Hiler wrote: > On Wed, Aug 24, 2016 at 05:03:11PM +0100, Matthew Auld wrote: > > The comment which documents the proper usage of the *_FW family of macros makes > > reference to intel_uncore_forcewake_irq{unlock, lock}, which is just > > confusing, seeing as such a set of functions don't even exist and never have > > for that matter(according to git). Let's fix that by replacing them with > > intel_uncore_forcewake_{get, put}. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> The downside is that this now doesn't mention the locking required to prevent machine hangs on some platforms. -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > On Fri, Oct 21, 2016 at 02:16:46PM +0200, Arkadiusz Hiler wrote: >> On Wed, Aug 24, 2016 at 05:03:11PM +0100, Matthew Auld wrote: >> > The comment which documents the proper usage of the *_FW family of macros makes >> > reference to intel_uncore_forcewake_irq{unlock, lock}, which is just >> > confusing, seeing as such a set of functions don't even exist and never have >> > for that matter(according to git). Let's fix that by replacing them with >> > intel_uncore_forcewake_{get, put}. >> > >> > Cc: Chris Wilson <chris@chris-wilson.co.uk> >> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > The downside is that this now doesn't mention the locking required to > prevent machine hangs on some platforms. "intel_uncore_forcewake_get will acquire forcewake reference and also take a uncore.lock to guarantee explicit access by one thread only. As some registers don't need forcewake held, intel_uncore_forcewake_{get,put} can be omitted. If you do so, be warned that on some gens (gen7), concurrent access to the same cacheline by multiple cpu threads with the gpu can risk a system hang. You need to grab uncore spinlock explicitly to guard against this." Would that be accurate addition? -Mika > -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
On Fri, Oct 21, 2016 at 04:00:10PM +0300, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > On Fri, Oct 21, 2016 at 02:16:46PM +0200, Arkadiusz Hiler wrote: > >> On Wed, Aug 24, 2016 at 05:03:11PM +0100, Matthew Auld wrote: > >> > The comment which documents the proper usage of the *_FW family of macros makes > >> > reference to intel_uncore_forcewake_irq{unlock, lock}, which is just > >> > confusing, seeing as such a set of functions don't even exist and never have > >> > for that matter(according to git). Let's fix that by replacing them with > >> > intel_uncore_forcewake_{get, put}. > >> > > >> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > >> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > > > The downside is that this now doesn't mention the locking required to > > prevent machine hangs on some platforms. > > "intel_uncore_forcewake_get will acquire forcewake reference and also > take a uncore.lock to guarantee explicit access by one thread only. As > some registers don't need forcewake held, intel_uncore_forcewake_{get,put} > can be omitted. If you do so, be warned that on some gens (gen7), > concurrent access to the same cacheline by multiple cpu threads with the gpu > can risk a system hang. You need to grab uncore spinlock explicitly to > guard against this." > > Would that be accurate addition? intel_uncore_forcewake_get() doesn't acquire the spinlock for you, just for itself. The full sequence would be spin_lock_irq(&dev_priv->uncore.lock); intel_uncore_forcewake_get__locked() ... intel_uncore_forcewake_put__locked() spin_unlock_irq(&dev_priv->uncore.lock); We very rarely do that either (a) presuming that we are serialised by some other lock, (b) don't care because it is safe or (c) completely forgotten about the risks. -Chris
On Fri, Oct 21, 2016 at 02:57:28PM +0100, Chris Wilson wrote: > On Fri, Oct 21, 2016 at 04:00:10PM +0300, Mika Kuoppala wrote: > > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > > > On Fri, Oct 21, 2016 at 02:16:46PM +0200, Arkadiusz Hiler wrote: > > >> On Wed, Aug 24, 2016 at 05:03:11PM +0100, Matthew Auld wrote: > > >> > The comment which documents the proper usage of the *_FW family of macros makes > > >> > reference to intel_uncore_forcewake_irq{unlock, lock}, which is just > > >> > confusing, seeing as such a set of functions don't even exist and never have > > >> > for that matter(according to git). Let's fix that by replacing them with > > >> > intel_uncore_forcewake_{get, put}. > > >> > > > >> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > >> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > >> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > > > > > The downside is that this now doesn't mention the locking required to > > > prevent machine hangs on some platforms. Previous version neither mentioned that clearly. Imaginary functions with irq in name is more confusing than helpful in my opinion. The assumption that those were mistaken for {get,put} is easy enough to make. > > > > "intel_uncore_forcewake_get will acquire forcewake reference and also > > take a uncore.lock to guarantee explicit access by one thread only. As > > some registers don't need forcewake held, intel_uncore_forcewake_{get,put} > > can be omitted. If you do so, be warned that on some gens (gen7), > > concurrent access to the same cacheline by multiple cpu threads with the gpu > > can risk a system hang. You need to grab uncore spinlock explicitly to > > guard against this." > > > > Would that be accurate addition? > > intel_uncore_forcewake_get() doesn't acquire the spinlock for you, just > for itself. > > The full sequence would be > > spin_lock_irq(&dev_priv->uncore.lock); > intel_uncore_forcewake_get() > ... > intel_uncore_forcewake_put() > spin_unlock_irq(&dev_priv->uncore.lock); > > We very rarely do that either (a) presuming that we are serialised by > some other lock, (b) don't care because it is safe or (c) completely > forgotten about the risks. > -Chris Then all that should be mentioned? My take on it: These are untraced mmio-accessors that are only valid to be used inside critical sections inside IRQ handlers where forcewake is explicitly controlled. Think twice, and think again, before using these. Those possibly should be used between: spin_lock_irq(&dev_priv->uncore.lock); intel_uncore_forcewake_get(); and intel_uncore_forcewake_put(); spin_unlock_irq(&dev_priv->uncore.lock); Note: some registers may not need forcewake held, so intel_uncore_forcewake_{get,put} can be omitted. Code may be serialised by different lock, so immediate spin_{lock,unlock}_irq() may not be necessary. -- Cheers, Arek
> These are untraced mmio-accessors that are only valid to be used inside > critical sections inside IRQ handlers where forcewake is explicitly > controlled. > > Think twice, and think again, before using these. > > Those possibly should be used between: > > spin_lock_irq(&dev_priv->uncore.lock); > intel_uncore_forcewake_get(); > > and > > intel_uncore_forcewake_put(); > spin_unlock_irq(&dev_priv->uncore.lock); > > > Note: some registers may not need forcewake held, so > intel_uncore_forcewake_{get,put} can be omitted. > > Code may be serialised by different lock, so immediate > spin_{lock,unlock}_irq() may not be necessary. Maybe roll that up into a new patch? Assuming Chris is happy...
On Tue, Oct 25, 2016 at 12:29:41PM +0100, Matthew Auld wrote: > > These are untraced mmio-accessors that are only valid to be used inside > > critical sections inside IRQ handlers where forcewake is explicitly > > controlled. > > > > Think twice, and think again, before using these. > > > > Those possibly should be used between: > > > > spin_lock_irq(&dev_priv->uncore.lock); > > intel_uncore_forcewake_get(); > > > > and > > > > intel_uncore_forcewake_put(); > > spin_unlock_irq(&dev_priv->uncore.lock); > > > > > > Note: some registers may not need forcewake held, so > > intel_uncore_forcewake_{get,put} can be omitted. > > > > Code may be serialised by different lock, so immediate > > spin_{lock,unlock}_irq() may not be necessary. > Maybe roll that up into a new patch? Assuming Chris is happy... > s/inside IRQ handlers/, such as inside IRQ handlers,/ As an example, these accessors can possibly be used between: can be omitted, see intel_uncore_forcewake_for_reg(). Certain architectures will die if the same cacheline is concurrently accessed by different clients (e.g. Ivybridge). Access to registers should therefore generally be serialised, by either the dev_priv->uncore.lock or a more localised lock guarding all access to that bank of registers.
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ff96b7a..c285d61 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3792,8 +3792,8 @@ __raw_write(64, q) * critical sections inside IRQ handlers where forcewake is explicitly * controlled. * Think twice, and think again, before using these. - * Note: Should only be used between intel_uncore_forcewake_irqlock() and - * intel_uncore_forcewake_irqunlock(). + * Note: Should only be used between intel_uncore_forcewake_get and + * intel_uncore_forcewake_put. */ #define I915_READ_FW(reg__) __raw_i915_read32(dev_priv, (reg__)) #define I915_WRITE_FW(reg__, val__) __raw_i915_write32(dev_priv, (reg__), (val__))
The comment which documents the proper usage of the *_FW family of macros makes reference to intel_uncore_forcewake_irq{unlock, lock}, which is just confusing, seeing as such a set of functions don't even exist and never have for that matter(according to git). Let's fix that by replacing them with intel_uncore_forcewake_{get, put}. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)