On Tue, 29 Dec 2015, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxx>
>> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
>> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
