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

[Goel, Akash]

On 8/4/2016 8:53 PM, Chris Wilson wrote:
> On Thu, Aug 04, 2016 at 08:55:18PM +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.
>>
>> 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.
>
> Already out of date, sorry.
>
>> @@ -394,10 +394,11 @@ static void print_batch_pool_stats(struct seq_file *m,
>>  	struct file_stats stats;
>>  	struct intel_engine_cs *engine;
>>  	int j;
>> +	u32 iter;
>
> Can you please use s/u32 iter/unsigned int iter/ ? I don't think there
> is a good reason to be specific about the integer size.
>
Or can 'enum intel_engine_id id' be used, like used for 
for_each_engine_id macro ?

 > And in doing so, keep the locals looking neat and not too much like a
 > disjointed stair case.

Sorry for that. Didn't realize that code will look ungainly by simply 
adding a new line, at the end of locals, for declaring the 'iter'.


>>  /* Iterator over subset of engines selected by mask */
>> -#define for_each_engine_masked(engine__, dev_priv__, mask__) \
>> -	for ((engine__) = &(dev_priv__)->engine[0]; \
>> -	     (engine__) < &(dev_priv__)->engine[I915_NUM_ENGINES]; \
>> -	     (engine__)++) \
>> -		for_each_if (((mask__) & intel_engine_flag(engine__)) && \
>> -			     intel_engine_initialized(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__)))
>
> Having restored the iterator, you can use mask__ & BIT(iter__) instead
> of intel_engine_flag()

Fine, that could also save a load/de-reference of engine.

Best regards
Akash
> -Chris
>