Re: [PATCH v9 2/6] drm/i915: Convert requests to use struct fence

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

On 07/06/16 12:42, Maarten Lankhorst wrote:
> Op 02-06-16 om 13:07 schreef Tvrtko Ursulin:

[snip]

>>> +static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
>>> +                          bool lazy_coherency)
>>> +{
>>> +    return fence_is_signaled(&req->fence);
>>> +}
>>
>> I would squash the following patch into this one, it makes no sense to keep a function with an unused parameter. And fewer patches in the series makes it less scary to review. :) Of course if they are also not too big. :D
> It's easier to read with all the function parameter changes in a separate patch.

I do not think so, but it is not even near a blocking commit so OK.

>>>        u32 (*get_cmd_length_mask)(u32 cmd_header);
>>> +
>>> +    spinlock_t fence_lock;
>>
>> Why is this lock per-engine, and not for example per timeline? Aren't fencees living completely isolated in their per-context-per-engine domains? So presumably there is something somewhere which is shared outside that domain to need a lock at the engine level?
> All outstanding requests are added to engine->fence_signal_list in patch 4, which means a per engine lock is required.

Okay, a comment is required here to describe the lock then. All what it 
protects and how and when it needs to be taken. Both from the i915 point 
of view and from the fence API side.

Regards,

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