On pe, 2016-10-07 at 15:03 +0530, akash.goel@xxxxxxxxx wrote:
> > From: Akash Goel <akash.goel@xxxxxxxxx>
>
> 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?
> @@ -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.
> 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;
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
