[Intel-gfx] [PATCH 073/190] drm/i915: Introduce i915_gem_active for request tracking

[Tvrtko Ursulin]

On 11/01/16 22:49, Chris Wilson wrote:
> On Mon, Jan 11, 2016 at 05:32:27PM +0000, Tvrtko Ursulin wrote:
>>> +struct i915_gem_active {
>>> +	struct drm_i915_gem_request *request;
>>> +};
>>> +
>>> +static inline void
>>> +i915_gem_request_mark_active(struct drm_i915_gem_request *request,
>>> +			     struct i915_gem_active *active)
>>> +{
>>> +	i915_gem_request_assign(&active->request, request);
>>> +}
>>
>> This function name bothers me since I think the name is misleading
>> and unintuitive. It is not marking a request as active but
>> associating it with the second data structure.
>>
>> Maybe i915_gem_request_move_to_active to keep the mental association
>> with the well established vma_move_to_active ?
>
> That's backwards imo, since it is the i915_gem_active that gets added to
> the request. (Or at least will be.)
>
>> Maybe struct i915_gem_active could also be better called
>> i915_gem_active_list ?
>
> It's not a list but a node. I started with drm_i915_gem_request_node,
> but that's too unwieldy and I felt even more confusing.
>
>> It is not ideal because of the future little reversal of who is in
>> who's list, so maybe there is something even better. But I think an
>> intuitive name is really important for code clarity and
>> maintainability.
>
> In userspace, I have the request (which is actually a userspace fence
> itself) containing a list of fences (that are identical to i915_gem_active,
> they track which request contains the reference and a callback for
> signalling) and those fences have a direct correspondence to,
> ARB_sync_objects, for example. But we already have plenty of conflict
> regarding the term fence, so that's no go.
>
> i915_gem_active, for me, made the association with the active-reference
> tracking that is ingrained into the objects and beyond. I quite like the
> connection with GPU activity
>
> i915_gem_retire_notifier? Hmm, I still like how
> i915_gem_active.request != NULL is quite self-descriptive.

Yes the last bit is neat.

Perhaps then leave the structure name as is and just rename the function 
to i915_gem_request_assign_active? I think that describes better what is 
actually happening.

Regards,

Tvrtko