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

[Chris Wilson]

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 at ffwll.ch>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Signed-off-by: Dave Gordon <david.s.gordon at 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

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

-- 
Chris Wilson, Intel Open Source Technology Centre