On Wed, Dec 02, 2015 at 01:29:21PM +0000, Dave Gordon wrote:
> On 25/11/15 09:23, Daniel Vetter wrote:
> >On Tue, Nov 24, 2015 at 11:47:26PM +0000, Chris Wilson wrote:
> >>On Tue, Nov 24, 2015 at 10:26:01PM +0000, Chris Wilson wrote:
> >>>On Tue, Nov 24, 2015 at 07:36:25PM +0200, Jani Nikula wrote:
> >>>> /* Iterate over initialised rings */
> >>>> #define for_each_ring(ring__, dev_priv__, i__) \
> >>>> for ((i__) = 0; (i__) < I915_NUM_RINGS; (i__)++) \
> >>>>- if (((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__)))
> >>>>+ for_each_if ((((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__))))
> >>>
> >>>Idly wondering if we would be happy with
> >>>
> >>>for_each_ring(ring__, dev_priv__)
> >>> for ((ring__) = &(dev_priv__)->ring[0];
> >>> (ring__) <= &(dev_priv__)->ring[I915_NUM_RINGS];
> >>> (ring__)++)
> >>> for_each_if(intel_ring_initialized(ring__))
> >>>
> >>>?
> >>>
> >>>The downside is that we have used i__ in several places rather than
> >>>ring->id.
> >>
> >>Fwiw, 13 files changed, 113 insertions(+), 140 deletions(-)
> >>
> >>Seems a reasonable shrinkage.
> >
> >Maybe for_each_engine even, and phase out for_each_ring completely?
> >-Daniel
> >
>
> Wouldn't it be nicer (and safer) not to build macros that fold the
> loop structure into the macro (in contravention of kernel
> programming guidelines).
>
> So how about NOT including the actual for() inside the macro, so
> that instead of writing
>
> for_each_engine(engine, dev_priv)
> do_stuff(engine)
>
> we would write it as
>
> for (EACH_ENGINE(engine, dev_priv))
> initialise(engine)
>
> for (EACH_ACTIVE_ENGINE(engine, dev_priv)) {
> service(engine)
> restart(engine)
> }
>
> etc. The for-loop is visible and the scope doesn't give you any surprises.
>
> [The EACH_ENGINE() macros expands to a semicolon-separated triplet
> of expressions; still a violation of the "don't use macros to
> redefine C syntax" guideline, but much less egregious than macros
> that contain embedded 'for's and 'if's.
for_each() is common practice in the kernel, so hiding the for() inside
the macro isn't that egregious. The problem is defining EACH_ACTIVE_ENGINE()
simply, preferrably without the use of another loop inside the for(;;).
One is to pack the i915->engines[] and have i915->num_engines and
intel_lookup_engine (or an i915->engine_for_id[]) for the occasional
case where we look up e.g. &i915->engines[BCS].
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
