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