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

[Goel, Akash]

On 7/27/2016 3:23 PM, Chris Wilson wrote:
> 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.)
>
Sorry do you want to check the impact of these modifications on the
overall code size (i915.o)

Would there be additional load instructions now ?

Earlier from the base address of dev_priv structure, the address of 
intel_engine_cs fields can derived as they were at a fixed offset.
But now an additional load will be required.

Before,
1109804   24212     416 1134432  114f60 ./drivers/gpu/drm/i915/i915.o

After,
1108796   24212     416 1133424  114b70 ./drivers/gpu/drm/i915/i915.o

>> 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...)
>

Fine, instead of != relational operator, will use logical NOT operator.

>> +#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).
So Sorry, missed intel_engine_setup_common(), where the above 
initialization is repeated.

Thus no need to copy init_engine_lists() inside intel_engine_cs.c
file, can simply kill it.

>
>> -	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)

But this cleanup is to be done in future & not in this patch ?

Best regards
Akash
> -Chris
>