Re: [PATCH 1/2] drm/i915: introduce for_each_engine(), rework for_each_ring()

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 29 Dec 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Dec 29, 2015 at 06:59:57PM +0000, Dave Gordon wrote:
>> In the discussions around commit 373701b1 (Jani Nikula)
>>     drm: fix potential dangling else problems in for_each_ macros
>> Daniel Vetter mooted the idea of a for_each_engine(ring, dev_priv)
>> macro that didn't use or rely on having an integer index variable.
>> 
>> So here it is; and, for backwards compatibility, an updated version
>> of for_each_ring() implemented using the same internals. As an example
>> of the use of this new macro, the functions in intel_uncore.c have
>> been converted to use it in preference to for_each_ring() wherever
>> possible. A subsequent patch will convert many of the uses in other
>> source files.
>> 
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h     | 32 ++++++++++++++++++++++++++++----
>>  drivers/gpu/drm/i915/intel_uncore.c |  5 ++---
>>  2 files changed, 30 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index cf7e0fc..a6457ee 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1968,10 +1968,34 @@ static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
>>  	return container_of(guc, struct drm_i915_private, guc);
>>  }
>>  
>> -/* Iterate over initialised rings */
>> -#define for_each_ring(ring__, dev_priv__, i__) \
>> -	for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
>> -		for_each_if ((((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__))))
>> +/* Helpers for the for_each_* macros below */
>> +#define	__INIT_ENGINE_PTR(engine, dev_priv)				\
>> +	({								\
>> +	 	struct intel_engine_cs *ep = (dev_priv)->ring;		\
>> +		(engine) = --ep;					\
>> +	})
>> +#define	__NEXT_ACTIVE_ENGINE(engine, dev_priv, nr)			\
>> +	({								\
>> +		struct intel_engine_cs *end = &(dev_priv)->ring[nr];	\
>> +		struct intel_engine_cs *ep = (engine);			\
>> +		bool in_range;						\
>> +		do {							\
>> +			in_range = ++(ep) < end;			\
>> +		} while (in_range && !intel_ring_initialized(ep));	\
>> +	 	(engine) = ep;						\
>> +		in_range;						\
>> +	})
>> +
>> +/* Iterate over initialised engines */
>> +#define for_each_engine(engine, dev_priv)				\
>> +	for (__INIT_ENGINE_PTR(engine, dev_priv);			\
>> +	     __NEXT_ACTIVE_ENGINE(engine, dev_priv, I915_NUM_RINGS); )
>> +
>> +/* Backwards compatibility: iterate over initialised "rings" */
>> +#define for_each_ring(engine, dev_priv, ring_id)			\
>> +	for (__INIT_ENGINE_PTR(engine, dev_priv);			\
>> +	     __NEXT_ACTIVE_ENGINE(engine, dev_priv, I915_NUM_RINGS) &&	\
>> +	     ((ring_id) = (engine)->id, true); )
>
> No, that is hard to read and bloats the kernel for no benefit. Even more
> so compared against just

Considering that I couldn't convince myself in a few minutes that the
above is correct, I'm with Chris here. The below is much more readable.

BR,
Jani.


>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 578f954dba1d..739569458bb7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1967,8 +1967,8 @@ static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
>  
>  /* Iterate over initialised rings */
>  #define for_each_ring(ring__, dev_priv__, i__) \
> -       for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
> -               for_each_if ((((ring__) = &(dev_priv__)->ring[(i__)]), intel_engine_initialized((ring__))))
> +       for ((ring__) = &(dev_priv__)->ring[0]; (ring__) < &(dev_priv__)->ring[I915_NUM_RINGS]; (ring__)++) \
> +               for_each_if (intel_engine_initialized((ring__)))
>  
>  enum hdmi_force_audio {
>         HDMI_AUDIO_OFF_DVI = -2,        /* no aux data for HDMI-DVI converter */
>
> (which is a net win over the current code, presumably solely due to
> eliminating dead locals)
> -Chris

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx