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

[Chris Wilson]

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.
 
> So this, why plain spin_lock? It can only be called from irq context
> now but the comment does not say that and there aren't any assert
> (if they are even possible nowadays).

Exactly.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre