| Message ID | 1477399682-3133-1-git-send-email-arkadiusz.hiler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25 October 2016 at 13:48, Arkadiusz Hiler <arkadiusz.hiler@intel.com> wrote: > Comment mentioned use of intel_uncore_forcewake_irq{unlock, lock} > functions which are nonexistent (and never were). > > The description was also incomplete and could cause confusion. Updated > comment is more elaborate on usage and caveats. > > v2: mention __locked variant of intel_uncore_forcewake_{get,put} instead > of plain ones > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
On Tue, Oct 25, 2016 at 02:48:02PM +0200, Arkadiusz Hiler wrote: > Comment mentioned use of intel_uncore_forcewake_irq{unlock, lock} > functions which are nonexistent (and never were). > > The description was also incomplete and could cause confusion. Updated > comment is more elaborate on usage and caveats. > > v2: mention __locked variant of intel_uncore_forcewake_{get,put} instead > of plain ones > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 28 +++++++++++++++++++++++++--- > 1 file changed, 25 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index b4cb1f0..e0f3fa4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3840,11 +3840,33 @@ __raw_write(64, q) > #undef __raw_write > > /* These are untraced mmio-accessors that are only valid to be used inside > - * critical sections inside IRQ handlers where forcewake is explicitly > + * critical sections, such as 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(). > + * > + * As an example, these accessors can possibly be used between: > + * > + * spin_lock_irq(&dev_priv->uncore.lock); > + * intel_uncore_forcewake_get__locked(); > + * > + * and > + * > + * intel_uncore_forcewake_put__locked(); > + * spin_unlock_irq(&dev_priv->uncore.lock); > + * > + * > + * Note: some registers may not need forcewake held, so > + * intel_uncore_forcewake_{get,put} 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 e.g. on Ivybridge > + * generally be serialised, by either the dev_priv->uncore.lock or a more > + * localised lock guarding all access to that bank of registers. > + * > + * Code may be serialised by different lock, so immediate > + * spin_{lock,unlock}_irq() may not be necessary. This last sentence is redundant since the reason why we need some lock somewhere is given above. With that, Reviewed-by: Chris Wilson <chris@chris-wilsono.c.uk> -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > On Tue, Oct 25, 2016 at 02:48:02PM +0200, Arkadiusz Hiler wrote: >> Comment mentioned use of intel_uncore_forcewake_irq{unlock, lock} >> functions which are nonexistent (and never were). >> >> The description was also incomplete and could cause confusion. Updated >> comment is more elaborate on usage and caveats. >> >> v2: mention __locked variant of intel_uncore_forcewake_{get,put} instead >> of plain ones >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Matthew Auld <matthew.auld@intel.com> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 28 +++++++++++++++++++++++++--- >> 1 file changed, 25 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index b4cb1f0..e0f3fa4 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -3840,11 +3840,33 @@ __raw_write(64, q) >> #undef __raw_write >> >> /* These are untraced mmio-accessors that are only valid to be used inside >> - * critical sections inside IRQ handlers where forcewake is explicitly >> + * critical sections, such as 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(). >> + * >> + * As an example, these accessors can possibly be used between: >> + * >> + * spin_lock_irq(&dev_priv->uncore.lock); >> + * intel_uncore_forcewake_get__locked(); >> + * >> + * and >> + * >> + * intel_uncore_forcewake_put__locked(); >> + * spin_unlock_irq(&dev_priv->uncore.lock); >> + * >> + * >> + * Note: some registers may not need forcewake held, so >> + * intel_uncore_forcewake_{get,put} 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 > > e.g. on Ivybridge > >> + * generally be serialised, by either the dev_priv->uncore.lock or a more >> + * localised lock guarding all access to that bank of registers. >> + * >> + * Code may be serialised by different lock, so immediate >> + * spin_{lock,unlock}_irq() may not be necessary. > > This last sentence is redundant since the reason why we need some lock > somewhere is given above. > Squashed those out and pushed, thanks you for patch and review. -Mika > With that, > > Reviewed-by: Chris Wilson <chris@chris-wilsono.c.uk> > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b4cb1f0..e0f3fa4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3840,11 +3840,33 @@ __raw_write(64, q) #undef __raw_write /* These are untraced mmio-accessors that are only valid to be used inside - * critical sections inside IRQ handlers where forcewake is explicitly + * critical sections, such as 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(). + * + * As an example, these accessors can possibly be used between: + * + * spin_lock_irq(&dev_priv->uncore.lock); + * intel_uncore_forcewake_get__locked(); + * + * and + * + * intel_uncore_forcewake_put__locked(); + * spin_unlock_irq(&dev_priv->uncore.lock); + * + * + * Note: some registers may not need forcewake held, so + * intel_uncore_forcewake_{get,put} 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. + * + * Code may be serialised by different lock, so immediate + * spin_{lock,unlock}_irq() may not be necessary. */ #define I915_READ_FW(reg__) __raw_i915_read32(dev_priv, (reg__)) #define I915_WRITE_FW(reg__, val__) __raw_i915_write32(dev_priv, (reg__), (val__))
Comment mentioned use of intel_uncore_forcewake_irq{unlock, lock} functions which are nonexistent (and never were). The description was also incomplete and could cause confusion. Updated comment is more elaborate on usage and caveats. v2: mention __locked variant of intel_uncore_forcewake_{get,put} instead of plain ones Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Matthew Auld <matthew.auld@intel.com> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-)