Re: [PATCH] drm/i915: fix comment referencing imaginary functions

[Arkadiusz Hiler <arkadiusz.hiler@intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx