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

[Chris Wilson]

On Wed, Jul 27, 2016 at 03:27:35PM +0530, akash.goel at intel.com wrote:
> From: Akash Goel <akash.goel at 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.

What's the impact upon code size? (Always nice to know as the oppposite
change was intended to reduce code size.)
 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index fc84037..0f14bf5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -264,16 +264,16 @@ static int i915_getparam(struct drm_device *dev, void *data,
>  		value = 1;
>  		break;
>  	case I915_PARAM_HAS_BSD:
> -		value = intel_engine_initialized(&dev_priv->engine[VCS]);
> +		value = (dev_priv->engine[VCS] != NULL);

value = !!dev_priv->engine[VCS];

(no stray brackets, and if I'm not allowed to compare against NULL...)

> +#define for_each_engine(engine__, dev_priv__, iter__) \
> +	for ((iter__) = 0; \
> +	     ((iter__) < I915_NUM_ENGINES) && \
> +	     ((engine__) = (dev_priv__)->engine[(iter__)], true); \
> +	     (iter__)++) \
> +		for_each_if ((engine__))

> +#define for_each_engine_masked(engine__, dev_priv__, mask__, iter__) \
> +	for ((iter__) = 0; \
> +	     ((iter__) < I915_NUM_ENGINES) && \
> +	     ((engine__) = (dev_priv__)->engine[(iter__)], true); \
> +	     (iter__)++) \
> +		for_each_if ((engine__) && ((mask__) & intel_engine_flag(engine__)))

Looks good. One more pair of brackets than strictly required but meh.
  
> -static void
> -init_engine_lists(struct intel_engine_cs *engine)
> -{
> -	INIT_LIST_HEAD(&engine->active_list);
> -	INIT_LIST_HEAD(&engine->request_list);
> -}

The good news is that these should not be required (at least in the near
future).

> -	if (engine == &dev_priv->engine[RCS] &&
> +	if (engine == dev_priv->engine[RCS] &&
>  	    instp_mode != dev_priv->relative_constants_mode) {
>  		ret = intel_ring_begin(params->request, 4);
>  		if (ret)
> @@ -1663,7 +1663,7 @@ static int gen8_emit_flush(struct drm_i915_gem_request *request,
>  
>  	if (invalidate_domains & I915_GEM_GPU_DOMAINS) {
>  		cmd |= MI_INVALIDATE_TLB;
> -		if (engine == &dev_priv->engine[VCS])
> +		if (engine == dev_priv->engine[VCS])
>  			cmd |= MI_INVALIDATE_BSD;
>  	}

This style just needs to die. if (engine->id == VCS)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre