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