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@xxxxxxxxxxxxxxxxxx>
> Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> Signed-off-by: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxx>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
