[Intel-gfx] [PATCH] drm/i915: fix comment referencing imaginary functions

[Chris Wilson]

On Fri, Oct 21, 2016 at 04:00:10PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris at 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 at chris-wilson.co.uk>
> >> > Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> >> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler at 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

-- 
Chris Wilson, Intel Open Source Technology Centre