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

Chris Wilson chris at chris-wilson.co.uk
Fri Aug 5 07:16:03 UTC 2016


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.

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

?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre


More information about the Intel-gfx-trybot mailing list