> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@xxxxxxxx] On Behalf Of Daniel > Vetter > Sent: Wednesday, June 18, 2014 9:16 PM > To: Mateo Lozano, Oscar > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [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@xxxxxxxxx wrote: > > From: Oscar Mateo <oscar.mateo@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > --- > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
