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

Commit Message

[e-mail] June 13, 2014, 3:37 p.m. UTC

From: Oscar Mateo <[e-mail]>

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 <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.h | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Daniel Vetter June 18, 2014, 8:16 p.m. UTC | #1

On Fri, Jun 13, 2014 at 04:37:24PM +0100, [e-mail] wrote:
> From: Oscar Mateo <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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


> +	/* Execlists */
> +	struct {
> +		struct drm_i915_gem_object *obj;
> +	} engine[I915_NUM_RINGS];
> +
>  	struct i915_ctx_hang_stats hang_stats;
>  	struct i915_address_space *vm;
>  
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[e-mail] June 19, 2014, 8:52 a.m. UTC | #2

> -----Original Message-----
> From: Daniel Vetter [mailto:[e-mail]] On Behalf Of Daniel
> Vetter
> Sent: Wednesday, June 18, 2014 9:16 PM
> To: Mateo Lozano, Oscar
> Cc: [e-mail]
> 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, [e-mail] wrote:
> > From: Oscar Mateo <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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

Daniel Vetter June 19, 2014, 10:57 a.m. UTC | #3

On Thu, Jun 19, 2014 at 10:52 AM, Mateo Lozano, Oscar
<[e-mail]> wrote:
>> > 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?

Well the union has the same idea but using less space. Not really
worth here though at all, so I'm ok with your approach. In any case
subclassing is usually the better approach than having a union.
-Daniel

Patch

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;
+	/* Execlists */
+	struct {
+		struct drm_i915_gem_object *obj;
+	} engine[I915_NUM_RINGS];
+
 	struct i915_ctx_hang_stats hang_stats;
 	struct i915_address_space *vm;