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

[Goel, Akash]

On 8/5/2016 12:46 PM, Chris Wilson wrote:
> On Fri, Aug 05, 2016 at 10:33:21AM +0530, Goel, Akash wrote:
>>
>>
>> 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 ?
>
> Hmm, yes. I think of iter as an untyped integer (strictly a long)
> because it is an index into the array. But enum intel_engine_id iter is
> preferrable to u32 iter.
>

Thanks will use 'enum intel_engine_id iter' then.

>>> 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.
>
> How about:
>
> for (iter = 0; iter < I915_NUM_ENGINES; iter++)
> 	if (mask & BIT(iter) && (engine = dev_priv->engine[iter]))
>
> ?
ohh, even better now.. Will do as per the above.
Thanks..

Best regards
Akash
> -Chris
>