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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 13, 2014 at 04:37:24PM +0100, oscar.mateo@intel.com wrote:
> From: Oscar Mateo <oscar.mateo@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@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


> +	/* 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
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch