[Intel-gfx] [PATCH 06/53] drm/i915/bdw: Introduce one context backing object per engine

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, June 18, 2014 9:16 PM
> To: Mateo Lozano, Oscar
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 06/53] drm/i915/bdw: Introduce one context
> backing object per engine
> 
> On Fri, Jun 13, 2014 at 04:37:24PM +0100, oscar.mateo at intel.com wrote:
> > From: Oscar Mateo <oscar.mateo at intel.com>
> >
> > A context backing object only makes sense for a given engine (because
> > it holds state data specific to that engine).
> >
> > In legacy ringbuffer sumission mode, the only MI_SET_CONTEXT we really
> > perform is for the render engine, so one backing object is all we needed.
> >
> > With Execlists, however, we need backing objects for every engine, as
> > contexts become the only way to submit workloads to the GPU. To tackle
> > this problem, we multiplex the context struct to contain
> > <no-of-engines> objects.
> >
> > Originally, I colored this code by instantiating one new context for
> > every engine I wanted to use, but this change suggested by Brad Volkin
> > makes it more elegant.
> >
> > v2: Leave the old backing object pointer behind. Daniel Vetter
> > suggested using a union, but it makes more sense to keep render_obj as
> > a NULL pointer behind, to make sure no one uses it incorrectly when
> > Execlists are enabled, similar to what we are doing with ring->buffer
> > (Rusty's API level 5).
> >
> > Signed-off-by: Oscar Mateo <oscar.mateo at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index a15370c..ccc1ba6 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -593,7 +593,14 @@ struct intel_context {
> >  	uint8_t remap_slice;
> >  	struct drm_i915_file_private *file_priv;
> >  	struct intel_engine_cs *last_ring;
> > +
> > +	/* Legacy ring buffer submission */
> >  	struct drm_i915_gem_object *render_obj;
> 
> Per my previous request, is_initialized should also be nearby, maybe wrapped
> in a struct. So union {
> 	struct {
> 		struct gem_bo *obj;
> 		bool is_iniatlized;
> 
> 	} render_ctx;
> 	struct {
> 		...
> 	} lrc[I915_NUM_RINGS];
> }
> 
> Or some other means to make it clearer which fields are for legacy render
> ctx objects and which for lrc contexts. I also wonder whether we should
> shovel all the hw specific stuff at the end to have a clearer separation
> between the sw-side field members associated with the software context
> object and the stuff for the hw thing.
> 
> Just ideas to pick&choose really, we can cocci-polish this once it's all settled
> easily (i.e. afterwards).
> -Daniel

Hmmmm... in the Execlists code I reused is_initialized for the render null-context (same as in the legacy context: I don´t want to do it more than once, e.g. when we come out of reset). Renaming it to render_is_initialized works for me, because null-context is only for the RCS in any case. But I wouldn´t mark that field as "legacy submission only"...

Also, I didn´t follow you instructions about the union for a reason:

> > v2: Leave the old backing object pointer behind. Daniel Vetter
> > suggested using a union, but it makes more sense to keep render_obj as
> > a NULL pointer behind, to make sure no one uses it incorrectly when
> > Execlists are enabled, similar to what we are doing with ring->buffer
> > (Rusty's API level 5).

Not sure if you agree with this or you still prefer the union?

-- Oscar