| Message ID | 1448386585-4144-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 24 Nov 2015, Jani Nikula <jani.nikula@intel.com> wrote: > We have serious dangling else bugs waiting to happen in our for_each_ > style macros with ifs. Consider, for example, > > #define for_each_power_domain(domain, mask) \ > for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \ > for_each_if ((1 << (domain)) & (mask)) Blah, copy-paste from after the patch, should be s/for_each_if/if/ > > If this is used in context: > > if (condition) > for_each_power_domain(domain, mask); > else > foo(); > > foo() will be called for each domain *not* in mask, if condition holds, > and not at all if condition doesn't hold. > > Fix this by reversing the conditions in the macros, and adding an else > branch for the "for each" block, so that other if/else blocks can't > interfere. Provide a "for_each_if" helper macro to make it easier to get > this right. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 14 ++++++++------ > drivers/gpu/drm/i915/intel_display.c | 2 +- > drivers/gpu/drm/i915/intel_dsi.h | 2 +- > drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++-- > 4 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 3d8741eff7d3..f9c779fd84ad 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -263,6 +263,8 @@ struct i915_hotplug { > I915_GEM_DOMAIN_INSTRUCTION | \ > I915_GEM_DOMAIN_VERTEX) > > +#define for_each_if(condition) if (!(condition)); else > + > #define for_each_pipe(__dev_priv, __p) \ > for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++) > #define for_each_plane(__dev_priv, __pipe, __p) \ > @@ -286,7 +288,7 @@ struct i915_hotplug { > list_for_each_entry(intel_plane, \ > &(dev)->mode_config.plane_list, \ > base.head) \ > - if ((intel_plane)->pipe == (intel_crtc)->pipe) > + for_each_if ((intel_plane)->pipe == (intel_crtc)->pipe) > > #define for_each_intel_crtc(dev, intel_crtc) \ > list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) > @@ -303,15 +305,15 @@ struct i915_hotplug { > > #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \ > list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \ > - if ((intel_encoder)->base.crtc == (__crtc)) > + for_each_if ((intel_encoder)->base.crtc == (__crtc)) > > #define for_each_connector_on_encoder(dev, __encoder, intel_connector) \ > list_for_each_entry((intel_connector), &(dev)->mode_config.connector_list, base.head) \ > - if ((intel_connector)->base.encoder == (__encoder)) > + for_each_if ((intel_connector)->base.encoder == (__encoder)) > > #define for_each_power_domain(domain, mask) \ > for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \ > - if ((1 << (domain)) & (mask)) > + for_each_if ((1 << (domain)) & (mask)) > > struct drm_i915_private; > struct i915_mm_struct; > @@ -730,7 +732,7 @@ struct intel_uncore { > for ((i__) = 0, (domain__) = &(dev_priv__)->uncore.fw_domain[0]; \ > (i__) < FW_DOMAIN_ID_COUNT; \ > (i__)++, (domain__) = &(dev_priv__)->uncore.fw_domain[i__]) \ > - if (((mask__) & (dev_priv__)->uncore.fw_domains) & (1 << (i__))) > + for_each_if (((mask__) & (dev_priv__)->uncore.fw_domains) & (1 << (i__))) > > #define for_each_fw_domain(domain__, dev_priv__, i__) \ > for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__) > @@ -1969,7 +1971,7 @@ 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__)++) \ > - if (((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__))) > + for_each_if ((((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__)))) > > enum hdmi_force_audio { > HDMI_AUDIO_OFF_DVI = -2, /* no aux data for HDMI-DVI converter */ > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 75f03e5bee51..4b21d5e137dc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12417,7 +12417,7 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2) > list_for_each_entry((intel_crtc), \ > &(dev)->mode_config.crtc_list, \ > base.head) \ > - if (mask & (1 <<(intel_crtc)->pipe)) > + for_each_if (mask & (1 <<(intel_crtc)->pipe)) > > static bool > intel_compare_m_n(unsigned int m, unsigned int n, > diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h > index e6cb25239941..02551ff228c2 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.h > +++ b/drivers/gpu/drm/i915/intel_dsi.h > @@ -117,7 +117,7 @@ static inline struct intel_dsi_host *to_intel_dsi_host(struct mipi_dsi_host *h) > > #define for_each_dsi_port(__port, __ports_mask) \ > for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++) \ > - if ((__ports_mask) & (1 << (__port))) > + for_each_if ((__ports_mask) & (1 << (__port))) > > static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) > { > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 3fa43af94946..469927edf459 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -54,13 +54,13 @@ > i < (power_domains)->power_well_count && \ > ((power_well) = &(power_domains)->power_wells[i]); \ > i++) \ > - if ((power_well)->domains & (domain_mask)) > + for_each_if ((power_well)->domains & (domain_mask)) > > #define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \ > for (i = (power_domains)->power_well_count - 1; \ > i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\ > i--) \ > - if ((power_well)->domains & (domain_mask)) > + for_each_if ((power_well)->domains & (domain_mask)) > > bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, > int power_well_id);
On Tue, Nov 24, 2015 at 07:36:25PM +0200, Jani Nikula wrote: > We have serious dangling else bugs waiting to happen in our for_each_ > style macros with ifs. Consider, for example, > > #define for_each_power_domain(domain, mask) \ > for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \ > for_each_if ((1 << (domain)) & (mask)) > > If this is used in context: > > if (condition) > for_each_power_domain(domain, mask); > else > foo(); > > foo() will be called for each domain *not* in mask, if condition holds, > and not at all if condition doesn't hold. > > Fix this by reversing the conditions in the macros, and adding an else > branch for the "for each" block, so that other if/else blocks can't > interfere. Provide a "for_each_if" helper macro to make it easier to get > this right. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> There's a shocking amount of these in drm core too. Just looking through drm_crtc.h finds plenty. Care to respin with that included (and for_each_if move to drmP.h or something like that)? On the i915 parts meanwhile: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_drv.h | 14 ++++++++------ > drivers/gpu/drm/i915/intel_display.c | 2 +- > drivers/gpu/drm/i915/intel_dsi.h | 2 +- > drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++-- > 4 files changed, 12 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 3d8741eff7d3..f9c779fd84ad 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -263,6 +263,8 @@ struct i915_hotplug { > I915_GEM_DOMAIN_INSTRUCTION | \ > I915_GEM_DOMAIN_VERTEX) > > +#define for_each_if(condition) if (!(condition)); else > + > #define for_each_pipe(__dev_priv, __p) \ > for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++) > #define for_each_plane(__dev_priv, __pipe, __p) \ > @@ -286,7 +288,7 @@ struct i915_hotplug { > list_for_each_entry(intel_plane, \ > &(dev)->mode_config.plane_list, \ > base.head) \ > - if ((intel_plane)->pipe == (intel_crtc)->pipe) > + for_each_if ((intel_plane)->pipe == (intel_crtc)->pipe) > > #define for_each_intel_crtc(dev, intel_crtc) \ > list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) > @@ -303,15 +305,15 @@ struct i915_hotplug { > > #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \ > list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \ > - if ((intel_encoder)->base.crtc == (__crtc)) > + for_each_if ((intel_encoder)->base.crtc == (__crtc)) > > #define for_each_connector_on_encoder(dev, __encoder, intel_connector) \ > list_for_each_entry((intel_connector), &(dev)->mode_config.connector_list, base.head) \ > - if ((intel_connector)->base.encoder == (__encoder)) > + for_each_if ((intel_connector)->base.encoder == (__encoder)) > > #define for_each_power_domain(domain, mask) \ > for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \ > - if ((1 << (domain)) & (mask)) > + for_each_if ((1 << (domain)) & (mask)) > > struct drm_i915_private; > struct i915_mm_struct; > @@ -730,7 +732,7 @@ struct intel_uncore { > for ((i__) = 0, (domain__) = &(dev_priv__)->uncore.fw_domain[0]; \ > (i__) < FW_DOMAIN_ID_COUNT; \ > (i__)++, (domain__) = &(dev_priv__)->uncore.fw_domain[i__]) \ > - if (((mask__) & (dev_priv__)->uncore.fw_domains) & (1 << (i__))) > + for_each_if (((mask__) & (dev_priv__)->uncore.fw_domains) & (1 << (i__))) > > #define for_each_fw_domain(domain__, dev_priv__, i__) \ > for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__) > @@ -1969,7 +1971,7 @@ 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__)++) \ > - if (((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__))) > + for_each_if ((((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__)))) > > enum hdmi_force_audio { > HDMI_AUDIO_OFF_DVI = -2, /* no aux data for HDMI-DVI converter */ > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 75f03e5bee51..4b21d5e137dc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -12417,7 +12417,7 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2) > list_for_each_entry((intel_crtc), \ > &(dev)->mode_config.crtc_list, \ > base.head) \ > - if (mask & (1 <<(intel_crtc)->pipe)) > + for_each_if (mask & (1 <<(intel_crtc)->pipe)) > > static bool > intel_compare_m_n(unsigned int m, unsigned int n, > diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h > index e6cb25239941..02551ff228c2 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.h > +++ b/drivers/gpu/drm/i915/intel_dsi.h > @@ -117,7 +117,7 @@ static inline struct intel_dsi_host *to_intel_dsi_host(struct mipi_dsi_host *h) > > #define for_each_dsi_port(__port, __ports_mask) \ > for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++) \ > - if ((__ports_mask) & (1 << (__port))) > + for_each_if ((__ports_mask) & (1 << (__port))) > > static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) > { > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 3fa43af94946..469927edf459 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -54,13 +54,13 @@ > i < (power_domains)->power_well_count && \ > ((power_well) = &(power_domains)->power_wells[i]); \ > i++) \ > - if ((power_well)->domains & (domain_mask)) > + for_each_if ((power_well)->domains & (domain_mask)) > > #define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \ > for (i = (power_domains)->power_well_count - 1; \ > i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\ > i--) \ > - if ((power_well)->domains & (domain_mask)) > + for_each_if ((power_well)->domains & (domain_mask)) > > bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, > int power_well_id); > -- > 2.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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. -Chris
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. -Chris
On Wed, 25 Nov 2015, Chris Wilson <chris@chris-wilson.co.uk> 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 good, looks like you have the patch so I won't bother. v2 of my patch was merged to drm-misc now, so that complicates a bit. Perhaps the i__ to ring->id change could be a prep step. *shrug*. BR, Jani. > > Seems a reasonable shrinkage. > -Chris
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
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. .Dave.
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
On 02/12/15 13:46, Chris Wilson wrote: > 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(;;). Nothing wrong with another loop :) Although, to keep it tidy, it can be inside an inline helper function that skips over the unwanted items in the iteration. I think it's better to have the if-ready condition test inside the iterator (and therefore clearly bounded) than let it dangle at the end of the macro. > 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 Or, put the active ones on a linked list, or keep a bitmask of which ones have been initialised inside the dev_priv structure, so you don't have to even dereference the engine[] array to work out whether a particular engine is initialised. Apropos which, wouldn't it be much more efficient to do that, because intel_ring_initialized() is quite heavyweight and the results surely don't change often, if at all, during normal operation. So we should only evaluate it when something has changed, and cache the bool result for use in all those for_each() loops! .Dave.
On Wed, Dec 02, 2015 at 02:51:17PM +0000, Dave Gordon wrote: > Or, put the active ones on a linked list, or keep a bitmask of which > ones have been initialised inside the dev_priv structure, so you > don't have to even dereference the engine[] array to work out > whether a particular engine is initialised. Apropos which, wouldn't > it be much more efficient to do that, because > intel_ring_initialized() is quite heavyweight and the results surely > don't change often, if at all, during normal operation. So we should > only evaluate it when something has changed, and cache the bool > result for use in all those for_each() loops! commit b53cd50c19f9d3c6f3308165b3e26c47b19dd041 Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Tue Mar 31 00:25:20 2015 +0100 drm/i915: intel_ring_initialized() must be simple and inline Fixes regression from commit 48d823878d64f93163f5a949623346748bbce1b4 Author: Oscar Mateo <oscar.mateo@intel.com> Date: Thu Jul 24 17:04:23 2014 +0100 drm/i915/bdw: Generic logical ring init and cleanup Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> ... -bool intel_ring_initialized(struct intel_engine_cs *ring); +static inline bool +intel_ring_initialized(struct intel_engine_cs *ring) +{ + return ring->dev != NULL; +} Just waiting to clear a massive backlog. If we can use an array rather than a list (and a static assignment certainly qualifies), use an array. -Chris
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 Hi, I've done an implementation of for_each_engine(ring, dev_priv), and converted a few uses of for_each_ring(ring, dev_priv, unused) to get rid of the unused dummy variable. That works fine, so now I'm looking at for_each_ring() for the cases where the variable IS used. The comments above imply to me that the loop variable shouldn't really be the index in dev_priv->ring[i], but rather the value of engine->id. Is this correct? Presumably there is at present no difference, i.e. dev_priv->ring[i].id == i (at least if the ring has been initialised?). So is the reason that converting from index to id might give more flexibility in how to organise the ring structures? .Dave.
On Thu, Dec 10, 2015 at 12:32:03PM +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 > > Hi, > > I've done an implementation of for_each_engine(ring, dev_priv), and > converted a few uses of for_each_ring(ring, dev_priv, unused) to get > rid of the unused dummy variable. That works fine, so now I'm > looking at for_each_ring() for the cases where the variable IS used. > > The comments above imply to me that the loop variable shouldn't > really be the index in dev_priv->ring[i], but rather the value of > engine->id. Is this correct? Yes, that is more common and I think looks better for the slight abstraction. > Presumably there is at present no difference, i.e. > dev_priv->ring[i].id == i > (at least if the ring has been initialised?). So is the reason that > converting from index to id might give more flexibility in how to > organise the ring structures? It would. Primary motivation today is consistency. -Chris
Late last year, there were discussions on updating the for_each_ring() macro -- now renamed for_each_engine() -- to eliminate the third (index) parameter, which is mostly unused at the macro callsites. In particular, Chris Wilson and Daniel Vetter mentioned that we would like the flexibility to eventually move away from a linear indexed array-of-engines to some more sophisticated structure. So here's a patchset that does just that. First we locate all the places where the third argument to the macro *is* used, and change the macro to a new version "for_each_engine_id()", at the same time updating the third argument from an (int) index (usually 'i') to an intel_engine_id. This replacement makes no functional change in itself, because at present (dev_priv->engine[i].id == i for all i in 0..I915_NUM_ENGINES-1), but it opens the way to using something other than an embedded array in future. Then, we can replace all remaining instances with a simpler two-parameter version. Along the way, all the engine-iterator macros are updated to the form suggested by Chris Wilson, where the pointer into the array (rather than the index variable) is used to compute loop termination, and many redundant loop-counter variables are eliminated :) Dave Gordon (2): drm/i915: introduce for_each_engine_id() drm/i915: replace for_each_engine() drivers/gpu/drm/i915/i915_debugfs.c | 94 +++++++++++++++--------------- drivers/gpu/drm/i915/i915_drv.h | 28 ++++++--- drivers/gpu/drm/i915/i915_gem.c | 50 +++++++--------- drivers/gpu/drm/i915/i915_gem_context.c | 6 +- drivers/gpu/drm/i915/i915_gem_debug.c | 3 +- drivers/gpu/drm/i915/i915_gem_gtt.c | 9 +-- drivers/gpu/drm/i915/i915_gpu_error.c | 6 +- drivers/gpu/drm/i915/i915_guc_submission.c | 14 ++--- drivers/gpu/drm/i915/i915_irq.c | 24 ++++---- drivers/gpu/drm/i915/intel_guc_loader.c | 8 +-- drivers/gpu/drm/i915/intel_lrc.c | 3 +- drivers/gpu/drm/i915/intel_mocs.c | 6 +- drivers/gpu/drm/i915/intel_pm.c | 19 +++--- drivers/gpu/drm/i915/intel_ringbuffer.c | 21 ++++--- 14 files changed, 143 insertions(+), 148 deletions(-)
On 24/03/16 09:06, Patchwork wrote: > == Series Details == > > Series: drm/i915: fix potential dangling else problems in for_each_ macros (rev3) > URL : https://patchwork.freedesktop.org/series/1142/ > State : failure > > == Summary == > > Series 1142v3 drm/i915: fix potential dangling else problems in for_each_ macros > http://patchwork.freedesktop.org/api/1.0/series/1142/revisions/3/mbox/ > > Test kms_flip: > Subgroup basic-flip-vs-wf_vblank: > pass -> FAIL (snb-x220t) Already filed, https://bugs.freedesktop.org/show_bug.cgi?id=94294 > Test kms_pipe_crc_basic: > Subgroup suspend-read-crc-pipe-c: > dmesg-warn -> PASS (bsw-nuc-2) > Test pm_rpm: > Subgroup basic-pci-d3-state: > dmesg-warn -> PASS (bsw-nuc-2) > pass -> DMESG-WARN (byt-nuc) Ditto, https://bugs.freedesktop.org/show_bug.cgi?id=94164 > Subgroup basic-rte: > dmesg-warn -> PASS (byt-nuc) UNSTABLE > > bdw-nuci7 total:192 pass:180 dwarn:0 dfail:0 fail:0 skip:12 > bdw-ultra total:192 pass:171 dwarn:0 dfail:0 fail:0 skip:21 > bsw-nuc-2 total:192 pass:155 dwarn:0 dfail:0 fail:0 skip:37 > byt-nuc total:192 pass:156 dwarn:1 dfail:0 fail:0 skip:35 > hsw-brixbox total:192 pass:170 dwarn:0 dfail:0 fail:0 skip:22 > hsw-gt2 total:192 pass:175 dwarn:0 dfail:0 fail:0 skip:17 > ilk-hp8440p total:192 pass:129 dwarn:0 dfail:0 fail:0 skip:63 > ivb-t430s total:192 pass:167 dwarn:0 dfail:0 fail:0 skip:25 > skl-i7k-2 total:192 pass:169 dwarn:0 dfail:0 fail:0 skip:23 > skl-nuci5 total:192 pass:181 dwarn:0 dfail:0 fail:0 skip:11 > snb-dellxps total:192 pass:158 dwarn:0 dfail:0 fail:0 skip:34 > snb-x220t total:192 pass:157 dwarn:0 dfail:0 fail:2 skip:33 > > Results at /archive/results/CI_IGT_test/Patchwork_1699/ > > 8f2e9bbc15607f56261aefd7a1f8caf6909c6b9d drm-intel-nightly: 2016y-03m-23d-17h-03m-10s UTC integration manifest > c4613575f37039f81f8969a4e805a80ff0a64ef9 drm/i915: replace for_each_engine() > 31beb6168bc53ad8b3fe008dcaa2ae038743a252 drm/i915: introduce for_each_engine_id() >
On 24/03/16 12:03, Patchwork wrote: > == Series Details == > > Series: drm/i915: fix potential dangling else problems in for_each_ macros (rev4) > URL : https://patchwork.freedesktop.org/series/1142/ > State : success > > == Summary == > > Series 1142v4 drm/i915: fix potential dangling else problems in for_each_ macros > http://patchwork.freedesktop.org/api/1.0/series/1142/revisions/4/mbox/ > > Test gem_exec_suspend: > Subgroup basic-s3: > dmesg-warn -> PASS (bsw-nuc-2) > Test kms_pipe_crc_basic: > Subgroup suspend-read-crc-pipe-a: > pass -> SKIP (byt-nuc) > Subgroup suspend-read-crc-pipe-c: > incomplete -> PASS (hsw-gt2) > Test pm_rpm: > Subgroup basic-pci-d3-state: > dmesg-warn -> PASS (byt-nuc) > > bdw-nuci7 total:192 pass:180 dwarn:0 dfail:0 fail:0 skip:12 > bdw-ultra total:192 pass:171 dwarn:0 dfail:0 fail:0 skip:21 > bsw-nuc-2 total:192 pass:155 dwarn:0 dfail:0 fail:0 skip:37 > byt-nuc total:192 pass:156 dwarn:0 dfail:0 fail:0 skip:36 > hsw-brixbox total:192 pass:170 dwarn:0 dfail:0 fail:0 skip:22 > hsw-gt2 total:192 pass:175 dwarn:0 dfail:0 fail:0 skip:17 > ivb-t430s total:192 pass:167 dwarn:0 dfail:0 fail:0 skip:25 > skl-i7k-2 total:192 pass:169 dwarn:0 dfail:0 fail:0 skip:23 > skl-nuci5 total:192 pass:181 dwarn:0 dfail:0 fail:0 skip:11 > snb-dellxps total:192 pass:158 dwarn:0 dfail:0 fail:0 skip:34 > snb-x220t total:192 pass:158 dwarn:0 dfail:0 fail:1 skip:33 > > Results at /archive/results/CI_IGT_test/Patchwork_1704/ > > 79ee42317266a82b932a39e9567244ed91dd27d6 drm-intel-nightly: 2016y-03m-24d-10h-26m-54s UTC integration manifest > 0126b0b851d5abdfc08ccd48cf2babf2fdd550c0 drm/i915: replace for_each_engine() > 6fe2515ba81fbeaf484a388941b776350f910ba2 drm/i915: introduce for_each_engine_id() Merged, thanks for patches and review! Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3d8741eff7d3..f9c779fd84ad 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -263,6 +263,8 @@ struct i915_hotplug { I915_GEM_DOMAIN_INSTRUCTION | \ I915_GEM_DOMAIN_VERTEX) +#define for_each_if(condition) if (!(condition)); else + #define for_each_pipe(__dev_priv, __p) \ for ((__p) = 0; (__p) < INTEL_INFO(__dev_priv)->num_pipes; (__p)++) #define for_each_plane(__dev_priv, __pipe, __p) \ @@ -286,7 +288,7 @@ struct i915_hotplug { list_for_each_entry(intel_plane, \ &(dev)->mode_config.plane_list, \ base.head) \ - if ((intel_plane)->pipe == (intel_crtc)->pipe) + for_each_if ((intel_plane)->pipe == (intel_crtc)->pipe) #define for_each_intel_crtc(dev, intel_crtc) \ list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) @@ -303,15 +305,15 @@ struct i915_hotplug { #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \ list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \ - if ((intel_encoder)->base.crtc == (__crtc)) + for_each_if ((intel_encoder)->base.crtc == (__crtc)) #define for_each_connector_on_encoder(dev, __encoder, intel_connector) \ list_for_each_entry((intel_connector), &(dev)->mode_config.connector_list, base.head) \ - if ((intel_connector)->base.encoder == (__encoder)) + for_each_if ((intel_connector)->base.encoder == (__encoder)) #define for_each_power_domain(domain, mask) \ for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \ - if ((1 << (domain)) & (mask)) + for_each_if ((1 << (domain)) & (mask)) struct drm_i915_private; struct i915_mm_struct; @@ -730,7 +732,7 @@ struct intel_uncore { for ((i__) = 0, (domain__) = &(dev_priv__)->uncore.fw_domain[0]; \ (i__) < FW_DOMAIN_ID_COUNT; \ (i__)++, (domain__) = &(dev_priv__)->uncore.fw_domain[i__]) \ - if (((mask__) & (dev_priv__)->uncore.fw_domains) & (1 << (i__))) + for_each_if (((mask__) & (dev_priv__)->uncore.fw_domains) & (1 << (i__))) #define for_each_fw_domain(domain__, dev_priv__, i__) \ for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__) @@ -1969,7 +1971,7 @@ 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__)++) \ - if (((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__))) + for_each_if ((((ring__) = &(dev_priv__)->ring[(i__)]), intel_ring_initialized((ring__)))) enum hdmi_force_audio { HDMI_AUDIO_OFF_DVI = -2, /* no aux data for HDMI-DVI converter */ diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 75f03e5bee51..4b21d5e137dc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12417,7 +12417,7 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2) list_for_each_entry((intel_crtc), \ &(dev)->mode_config.crtc_list, \ base.head) \ - if (mask & (1 <<(intel_crtc)->pipe)) + for_each_if (mask & (1 <<(intel_crtc)->pipe)) static bool intel_compare_m_n(unsigned int m, unsigned int n, diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h index e6cb25239941..02551ff228c2 100644 --- a/drivers/gpu/drm/i915/intel_dsi.h +++ b/drivers/gpu/drm/i915/intel_dsi.h @@ -117,7 +117,7 @@ static inline struct intel_dsi_host *to_intel_dsi_host(struct mipi_dsi_host *h) #define for_each_dsi_port(__port, __ports_mask) \ for ((__port) = PORT_A; (__port) < I915_MAX_PORTS; (__port)++) \ - if ((__ports_mask) & (1 << (__port))) + for_each_if ((__ports_mask) & (1 << (__port))) static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) { diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 3fa43af94946..469927edf459 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -54,13 +54,13 @@ i < (power_domains)->power_well_count && \ ((power_well) = &(power_domains)->power_wells[i]); \ i++) \ - if ((power_well)->domains & (domain_mask)) + for_each_if ((power_well)->domains & (domain_mask)) #define for_each_power_well_rev(i, power_well, domain_mask, power_domains) \ for (i = (power_domains)->power_well_count - 1; \ i >= 0 && ((power_well) = &(power_domains)->power_wells[i]);\ i--) \ - if ((power_well)->domains & (domain_mask)) + for_each_if ((power_well)->domains & (domain_mask)) bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv, int power_well_id);
We have serious dangling else bugs waiting to happen in our for_each_ style macros with ifs. Consider, for example, #define for_each_power_domain(domain, mask) \ for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \ for_each_if ((1 << (domain)) & (mask)) If this is used in context: if (condition) for_each_power_domain(domain, mask); else foo(); foo() will be called for each domain *not* in mask, if condition holds, and not at all if condition doesn't hold. Fix this by reversing the conditions in the macros, and adding an else branch for the "for each" block, so that other if/else blocks can't interfere. Provide a "for_each_if" helper macro to make it easier to get this right. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 14 ++++++++------ drivers/gpu/drm/i915/intel_display.c | 2 +- drivers/gpu/drm/i915/intel_dsi.h | 2 +- drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++-- 4 files changed, 12 insertions(+), 10 deletions(-)