[Intel-gfx] [PATCH v3] drm/i915: Small compaction of the engine init code

[Chris Wilson]

On Thu, Jun 23, 2016 at 12:46:42PM +0100, Tvrtko Ursulin wrote:
> 
> On 23/06/16 12:25, Chris Wilson wrote:
> >On Thu, Jun 23, 2016 at 12:12:29PM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >>Effectively removes one layer of indirection between the mask of
> >>possible engines and the engine constructors. Instead of spelling
> >>out in code the mapping of HAS_<engine> to constructors, makes
> >>more use of the recently added data driven approach by putting
> >>engine constructor vfuncs into the table as well.
> >>
> >>Effect is fewer lines of source and smaller binary.
> >>
> >>At the same time simplify the error handling since engine
> >>destructors can run on unitialized engines anyway.
> >>
> >>Similar approach could be done for legacy submission is wanted.
> >>
> >>v2: Removed ugly BUILD_BUG_ONs in favour of newly introduced
> >>     ENGINE_MASK and HAS_ENGINE macros.
> >>     Also removed the forward declarations by shuffling functions
> >>     around.
> >>
> >>v3: Warn when logical_rings table does not contain enough data
> >>     and disable the engines which could not be initialized.
> >>     (Chris Wilson)
> >
> >I was happy with the BUILD_BUG suggestion :)
> 
> I've changed my mind later. :)
> 
> >>+	for (i = 0;
> >>+		i < I915_NUM_ENGINES && i < ARRAY_SIZE(logical_rings); i++) {
> >
> >HAS_ENGINE() == false if i >= I915_NUM_ENGINES
> 
> Don't follow. :) Why is v3 not good enough?

Both (all three) is overkill.

I feel like HAS_ENGINE() should encompass i < I915_NUM_ENGINES quite
succinctly. For belt and braces,

WARN_ON(dev_priv->intel_info.rings_mask & -(1 << I915_NUM_ENGINES)));
for (i = 0; i < ARRAY_SIZE(); i++) {
	if (!HAS_ENGINE(i))
		continue;
	
	if (!logical_rings[i].init)
		continue;
	
	ret = logical_rings[i].init(logical_rings_engine(i));
	if (ret)
		goto err;

	mask |= ENGINE_MASK(i);
}

WARN_ON(mask != dev_priv->intel_info.rings_mask) ...

?

-- 
Chris Wilson, Intel Open Source Technology Centre