[Intel-gfx] [PATCH 31/49] drm/i915: Reduce locking in execlist command submission

[Daniel Vetter]

On Fri, Mar 27, 2015 at 11:47:12AM +0000, Chris Wilson wrote:
> On Fri, Mar 27, 2015 at 11:40:12AM +0000, Tvrtko Ursulin wrote:
> > 
> > >+/* Like above but take and hold the uncore lock for the duration.
> > >+ * Must be used with I915_READ_FW and friends.
> > >+ */
> > >+void intel_uncore_forcewake_irqlock(struct drm_i915_private *dev_priv,
> > >+				enum forcewake_domains domains);
> > >+void intel_uncore_forcewake_irqunlock(struct drm_i915_private *dev_priv,
> > >+				   enum forcewake_domains domains);
> > 
> > Oh well I don't like your colour. :)
> > 
> > I would make the comment clearer in saying the function itself will
> > take the lock and not release it since "take and hold the uncore
> > lock for the duration" to me reads ambiguous.
> 
> "duration of the critical section".
> 
> > Also, not sure about the _irqlock suffix. It is well established in
> > spinlocks and the functions even does the opposite from that!
> > 
> > Maybe _get_and_lock / _put_and_unlock, or other way round?
> 
> How about _irq_get for the reasons that I don't this to be widely used
> elsewhere. We are trading off debugging for performance, that's only
> really justifiable inside irqs or busy-waits (and for busy-waits we
> already have the notrace variant).
> 
> Actually _get_irq/_put_irq.

If we bikeshed this, what about forcewake_get/put_locked and making the
lock acquisition explicit in the callers? spin_lock_irq is already a big
red flag asking for close scrutinity, not hiding would be a feature.
Especially if we're concerned with usage creep of these optimized
functions.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch