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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre