Re: [PATCH 31/49] drm/i915: Reduce locking in execlist command submission

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 03/27/2015 11:47 AM, 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".

I would like it to be explicit function takes the lock and not that the 
caller has to, for the duration of the critical section. Maybe it is my 
non-native English but from "take and hold the uncore lock for the 
duration.." I am not sure which one of the two it is.

>> 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.

OK, but comment needs to say that in my opinion.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx