Re: [PATCH v2] drm/i915: Small compaction of the engine init code

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@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@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Chris Wilson <chris@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.

Perhaps we just need:

BUILD_BUG_ON(ARRAY_SIZE(logical_rings) == I915_NUM_ENGINES)

?

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

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx