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

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre