[Intel-gfx] [PATCH 21/29] drm/i915: Switch back to an array of logical per-engine HW contexts

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-04-11 14:05:19)
> 
> On 10/04/2019 17:18, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-04-10 16:32:18)
> >> Yep as mentioned somewhere above. I definitely think another helper
> >> would help code base redability. Even if called unimaginatively as
> >> __i915_request_add.
> > 
> > It is just these two (based on a quick grep).
> > 
> > Maybe intel_context_create_request() ?
> > 
> > Hmm, but isn't that what i915_request_create() is. Quiet!
> 
> Why not __i915_request_add? i915_request_add would then be just wedged 
> check calling __i915_request_add, no? (Going from memory.. might not be 
> reliable.)

At the moment, we have:

__i915_request_create(): no timeline locking, no runtime-pm
i915_request_create(): no pinning, does timeline + runtime-pm
intel_context_create_request(): pin + call i915_request_create
igt_request_alloc(): lookup intel_context + call intel_context_create_request

I suppose there is some room in bringing i915_request_alloc() back from
the dead, but it's not the simple allocator one would expect from the
name. (igt_request_alloc is just for ease of recognition.)

Outside of igt_request_alloc(), there are two users for
intel_context_create_request() in my tree. One in
__intel_engine_record_defaults() and the other in
context_barrier_task().

For the in-kernel blitter tasks, we'll either be using a pinned kernel
context, or be shoehorning it into a pinned user context. Both should be
using i915_request_create() (as currently planned). And things like the
heartbeat and idle barriers, use the pinned kernel context.

Just to say intel_context_create_request() is the odd one out, and
deserves the longest name :)

> >>> +struct i915_gem_engines {
> >>> +     struct rcu_work rcu;
> >>> +     struct drm_i915_private *i915;
> >>> +     unsigned long num_engines;
> >>
> >> unsigned int?
> > 
> > Pointer before, array of pointers after, left an unsigned long hole.
> > 
> > I was just filling the space.
> 
> Well long makes me think there's a reason int is not enough. Which in 
> this case there isn't.

And I thought you were planning for a busy future, full of little
engines :)

> So I would still go for an int regardless of the 
> hole or not. There is nothing to be gained by filling space. Could even 
> be worse if some instructions expand to longer opcodes. :)

Hmm, in the near future this becomes

struct i915_gem_engines {
        struct rcu_head rcu;
        unsigned long num_engines;
        struct intel_context *engines[];
};

struct rcu_head is a pair of pointers, so that's still a pointer sized
hole. I give in. We'll only support 4 billion engines.
-Chris