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

[akash goel]

On Fri, Oct 7, 2016 at 4:19 PM, Joonas Lahtinen
<joonas.lahtinen at linux.intel.com> wrote:
> On pe, 2016-10-07 at 15:03 +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.
>>
>> 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?

Fine will then go with 'id' only.

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

Fine after using 'for_each_engine' here, the below Null pointer check
on 'engine' can be removed safely.
As you suggested, will not keep BUG_ON also.

>> 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;
>
Chris found an issue inside intel_engine_sync_index(), will add his
suggested fix also in the next version.


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

Thanks much for the review.

Best regards
Akash

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