Re: [PATCH 06/50] drm/i915: s/intel_ring_buffer/intel_engine

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 19, 2014 at 04:12:26PM +0000, Mateo Lozano, Oscar wrote:
> 
> 
> ---------------------------------------------------------------------
> 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@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@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...)

Yeah, it took me a while to see the light here ;-) But if we split up the
sw ctx tracking structure a bit from the low-level stuff I think this
indeed makes sense. The lazy context allocation simply irked me while we
still called the structure a _hw_ context. Which at that point it just
isn't any more.
> 
> 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).

Yes, ack on this prep-work refactor plan. And in case I didn't mention it
yet ofc ack for shoveling all the new execlist stuff into intel_lrc.c.

Btw for intel_lrc.c can you please (at the end is probably best) add a
patch to add kerneldoc for non-static functions and pull them into the
i915 guide? Similar to how we've done it for Brad's cmd parser. I think we
need better docs, so best to get started with new features.

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

Yeah, might as well throw this on top.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch