Chris Wilson <ch...@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 <ch...@chris-wilson.co.uk>
>> Cc: Matthew Auld <matthew.a...@intel.com>
>> Cc: Mika Kuoppala <mika.kuopp...@linux.intel.com>
>> Signed-off-by: Arkadiusz Hiler <arkadiusz.hi...@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 <ch...@chris-wilsono.c.uk>
> -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
