Re: [PATCH v9] drm/i915: Allocate intel_engine_cs structure only for the enabled engines

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On pe, 2016-10-07 at 15:03 +0530, akash.goel@intel.com wrote:
> > From: Akash Goel <akash.goel@intel.com>
> 
> With the possibility of addition of many more number of rings in future,
> the drm_i915_private structure could bloat as an array, of type
> intel_engine_cs, is embedded inside it.
> 	struct intel_engine_cs engine[I915_NUM_ENGINES];
> Though this is still fine as generally there is only a single instance of
> drm_i915_private structure used, but not all of the possible rings would be
> enabled or active on most of the platforms. Some memory can be saved by
> allocating intel_engine_cs structure only for the enabled/active engines.
> Currently the engine/ring ID is kept static and dev_priv->engine[] is simply
> indexed using the enums defined in intel_engine_id.
> To save memory and continue using the static engine/ring IDs, 'engine' is
> defined as an array of pointers.
> 	struct intel_engine_cs *engine[I915_NUM_ENGINES];
> dev_priv->engine[engine_ID] will be NULL for disabled engine instances.
> 
> v2:
> - Remove the engine iterator field added in drm_i915_private structure,
>   instead pass a local iterator variable to the for_each_engine**
>   macros. (Chris)
> - Do away with intel_engine_initialized() and instead directly use the
>   NULL pointer check on engine pointer. (Chris)
> 
> v3:
> - Remove for_each_engine_id() macro, as the updated macro for_each_engine()
>   can be used in place of it. (Chris)
> - Protect the access to Render engine Fault register with a NULL check, as
>   engine specific init is done later in Driver load sequence.
> 
> v4:
> - Use !!dev_priv->engine[VCS] style for the engine check in getparam. (Chris)
> - Kill the superfluous init_engine_lists().
> 
> v5:
> - Cleanup the intel_engines_init() & intel_engines_setup(), with respect to
>   allocation of intel_engine_cs structure. (Chris)
> 
> v6:
> - Rebase.
> 
> v7:
> - Optimize the for_each_engine_masked() macro. (Chris)
> - Change the type of 'iter' local variable to enum intel_engine_id. (Chris)
> - Rebase.
> 

Would not it be consistent to go with 'id' everywhere rather than
'iter'. Consistency is good, and my vote for 'id' as it's more
descriptive?
 
> @@ -153,9 +163,9 @@ int intel_engines_init(struct drm_device *dev)
>  cleanup:
>  	for (i = 0; i < I915_NUM_ENGINES; i++) {

Use for_each_engine here too.

> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 936f6f6..08303e3 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1645,7 +1645,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine)
>  {
>  	struct drm_i915_private *dev_priv;
>  
> -	if (!intel_engine_initialized(engine))
> +	if (!engine)
>  		return;

Remove this check or make it GEM_BUG_ON(!engine); but I don't think we
need that much paranoia.

> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c

<SNIP>

> @@ -2091,7 +2092,7 @@ void intel_engine_cleanup(struct intel_engine_cs *engine)
>  {
> >  	struct drm_i915_private *dev_priv;
>  
> -	if (!intel_engine_initialized(engine))
> +	if (!engine)
>  		return;

Same as above.

With those points fixed;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx