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

[Chris Wilson]

On Thu, Jun 23, 2016 at 11:26:27AM +0100, Tvrtko Ursulin wrote:
> 
> On 22/06/16 17:59, Chris Wilson wrote:
> >On Wed, Jun 22, 2016 at 05:35:48PM +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.
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> >
> >>  int intel_logical_rings_init(struct drm_device *dev)
> >>  {
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>+	struct intel_engine_cs *engine;
> >>+	unsigned int i;
> >>  	int ret;
> >>
> >>-	ret = logical_render_ring_init(dev);
> >>-	if (ret)
> >>-		return ret;
> >>-
> >>-	if (HAS_BSD(dev)) {
> >>-		ret = logical_bsd_ring_init(dev);
> >>-		if (ret)
> >>-			goto cleanup_render_ring;
> >>-	}
> >>-
> >>-	if (HAS_BLT(dev)) {
> >>-		ret = logical_blt_ring_init(dev);
> >>-		if (ret)
> >>-			goto cleanup_bsd_ring;
> >>-	}
> >>-
> >>-	if (HAS_VEBOX(dev)) {
> >>-		ret = logical_vebox_ring_init(dev);
> >>-		if (ret)
> >>-			goto cleanup_blt_ring;
> >>-	}
> >>-
> >>-	if (HAS_BSD2(dev)) {
> >>-		ret = logical_bsd2_ring_init(dev);
> >>-		if (ret)
> >>-			goto cleanup_vebox_ring;
> >>+	for (i = 0; i < I915_NUM_ENGINES; i++) {
> >
> >One more thing to consider is that logical_rings[] has an unspecified
> >size. Either we set NUM_ENGINES there or use ARRAY_SIZE here, either way
> >we risk not initialising all engines we expect.
> >
> >I think we need:
> >unsigned mask = 0;
> >
> >>+		if (HAS_ENGINE(dev_priv, i)) {
> >>+			engine = logical_ring_setup(dev_priv, i);
> >>+			ret = logical_rings[i].init(engine);
> >>+			if (ret)
> >>+				goto cleanup;
> >			mask |= intel_engine_flag(engine);
> >>+		}
> >>  	}
> >
> >if (WARN_ON(mask != dev_priv->info.rings_mask))
> >	mkwrite_intel_info(dev_priv)->rings_mask = mask;
> >
> >To catch any future forgotten additions without exploding.
> 
> Hm, if logical_rings does not contain all required engines it can
> either crash or, if somehow it magically manages to initialize it
> from random data, still pass your test.

I was expecting you to use if (!init()) continue or something to stop
the crash and so leave holes in the mask :)

> Perhaps we just need:
> 
> BUILD_BUG_ON(ARRAY_SIZE(logical_rings) == I915_NUM_ENGINES)
> 
> ?

Yeah, I can't see what more information we can provide other than
pointing out the piece of forgotten code. But what if we add new engines
to a future submission mechanism that we don't need in execlists?

When the build bug fires, would be the time to consider how to fix it.

> What is this mkwrite_intel_info thing? There is a facility nowadays
> to write to the rodata section? :)

It's not rodata, intel_info is copied into dev_priv, modified, then
treated as const.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre