[Intel-gfx] [PATCH 06/50] drm/i915: s/intel_ring_buffer/intel_engine

[Mateo Lozano, Oscar]

---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47


> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Monday, May 19, 2014 4:50 PM
> To: Mateo Lozano, Oscar
> Cc: Daniel Vetter; Lespiau, Damien; intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 06/50] drm/i915:
> s/intel_ring_buffer/intel_engine
> 
> On Mon, May 19, 2014 at 03:26:09PM +0000, Mateo Lozano, Oscar wrote:
> > > I think special-casing the i915_gem_context_get function for the
> > > default context and using private_default_ctx a bit more sounds
> > > good. We need to adjust the idr allocator a bit though to reserve 0,
> > > and a bit of frobbing in the context create code.
> >
> > Ok, no problem. I´ll send an early patch that uses private_default_ctx
> > to do the special casing (no functionality change) together with the
> > other refactoring patches.
> >
> > > Wrt ctx abstraction I think separate functions for execlist/legacy
> > > contexts should be good enough. The lookup/create/destroy logic should
> carry over.
> >
> > Including the creation of I915_NUM_RINGS contexts per-filp? do you
> > want that to happen for the legacy case as well? This implies a number
> > of other changes, like struct intel_engine *last_ring not making sense
> > anymore and frobbing i915_gem_create_context so that we can create
> > more than one context with the same ppgtt...
> 
> Damn, I didn't think this through. ctx->hang_stats already tracks random stuff
> for logical contexts and we only have one of those for the implicit per-filp
> default context. Still, allocating backing storage for the big execlist contexts
> looks really wasteful since a lot of userspace will just use 1 ring (e.g. mesa or
> opencl).
> 
> Also we've decided to at least for now support vcs2 implicitly, so
> I915_NUM_RINGS is kinda already the wrong thing to use (presuming we can
> indeed switch hw contexts between vcs and vcs2).
> 
> Otoh adding fake contexts for the legacy case also feels ugly.
> 
> So maybe we should rename i915_hw_context to i915_context and figure out a
> sane design for the differences later on. Same option would be to embedded
> struct i915_context into a i915_legacy_rcs_context and i915_execlist_context
> structures, but meh.
> 
> So now I think we should:
> a) s/i915_hw_context/i915_context/ since its long past a 1:1 relationship with
> hw
> b) create a union in i915_context for legacy and execlists contexts.
> Legacy contexts would just have the single gem bo backing storage needed for
> rcs, execlists contexts would have an array of rings plus the backing storage
> (since the ring is just embedded in there).
> 
> How does this sound?

Sounds good: other than using a union, my latest patchset already looked like this :)
(and this means bringing back from life the deferred ctx creation idea...)

Summarizing the entire conversation, we have agreed to three refactoring patches:

A) s/intel_ring_buffer/intel_engine_cs/
B) Split the ringbuffers and the rings (the ringbuffer lives inside the ring thanks to a pointer, that will be NULL for the LRC case).
C) s/i915_hw_context/i915_context/

Plus an LRC-specific patch:

D) Introduce one context backing object per engine (using a union that leaves one backing object for the legacy case and an array of backing objects and ringbuffers for the LRC case).

BTW: do you want me to kill private_default_ctx as well? It doesn´t look very useful...