[RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Jani Nikula]

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(-)

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

Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Jani Nikula]

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);

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

Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Jani Nikula]

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

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

Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Dave Gordon]

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Chris Wilson]

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Dave Gordon]

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH] drm/i915: fix potential dangling else problems in for_each_ macros

[Dave Gordon]

[-- Attachment #1: Type: text/plain, Size: 1714 bytes --]

On 02/12/15 14:58, Chris Wilson wrote:
> 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

Aha! That looks useful, but it didn't apply cleanly, so I've reworked it 
somewhat. Here's the patch (UNTESTED) for your comments ...

.Dave.

[-- Attachment #2: 0001-drm-i915-intel_ring_initialized-must-be-simple-and-i.patch --]
[-- Type: text/x-patch, Size: 6019 bytes --]

>From e2cc1e8f65c9b4bc465a3e1097de91d4bb6c13cd Mon Sep 17 00:00:00 2001
From: Dave Gordon <david.s.gordon@intel.com>
Date: Wed, 2 Dec 2015 17:58:33 +0000
Subject: [RFC] drm/i915: intel_ring_initialized() must be simple and inline
Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ

Based on Chris Wilson's patch from 6 months ago, rebased and adapted.

The idea is to use ring->dev as an indicator showing which engines have
been initialised and are therefore to be included in iterations that use
for_each_ring(). This allows us to avoid multiple memory references and
a (non-inlined) function call on each iteration of each such loop.

This version differs from Chris' primarily in the error cleanup paths,
where initialisation has failed and we therefore want to mark an engine
as NOT initialised. I have made the ring_cleanup() functions callable from
the failure path of the ring_init() code, rather than duplicating all the
steps to tear down a partially-constructed state. This also increases
symmetry; ring->dev is set at the start of ring_init, and cleared at the
end of ring_cleanup, in both the normal and error cases.

	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>
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 17 +++++++++-----
 drivers/gpu/drm/i915/intel_ringbuffer.c | 39 +++++++++++----------------------
 drivers/gpu/drm/i915/intel_ringbuffer.h |  6 ++++-
 3 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 4ebafab..7644c48 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1894,8 +1894,10 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 
 	dev_priv = ring->dev->dev_private;
 
-	intel_logical_ring_stop(ring);
-	WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0);
+	if (ring->buffer) {
+		intel_logical_ring_stop(ring);
+		WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0);
+	}
 
 	if (ring->cleanup)
 		ring->cleanup(ring);
@@ -1909,6 +1911,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *ring)
 	}
 
 	lrc_destroy_wa_ctx_obj(ring);
+	ring->dev = NULL;
 }
 
 static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *ring)
@@ -1931,11 +1934,11 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
 
 	ret = i915_cmd_parser_init_ring(ring);
 	if (ret)
-		return ret;
+		goto error;
 
 	ret = intel_lr_context_deferred_alloc(ring->default_context, ring);
 	if (ret)
-		return ret;
+		goto error;
 
 	/* As this is the default context, always pin it */
 	ret = intel_lr_context_do_pin(
@@ -1946,9 +1949,13 @@ static int logical_ring_init(struct drm_device *dev, struct intel_engine_cs *rin
 		DRM_ERROR(
 			"Failed to pin and map ringbuffer %s: %d\n",
 			ring->name, ret);
-		return ret;
+		goto error;
 	}
 
+	return 0;
+
+error:
+	intel_logical_ring_cleanup(ring);
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 57d78f2..921c8a6 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -33,23 +33,6 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 
-bool
-intel_ring_initialized(struct intel_engine_cs *ring)
-{
-	struct drm_device *dev = ring->dev;
-
-	if (!dev)
-		return false;
-
-	if (i915.enable_execlists) {
-		struct intel_context *dctx = ring->default_context;
-		struct intel_ringbuffer *ringbuf = dctx->engine[ring->id].ringbuf;
-
-		return ringbuf->obj;
-	} else
-		return ring->buffer && ring->buffer->obj;
-}
-
 int __intel_ring_space(int head, int tail, int size)
 {
 	int space = head - tail;
@@ -2167,8 +2150,10 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	init_waitqueue_head(&ring->irq_queue);
 
 	ringbuf = intel_engine_create_ringbuffer(ring, 32 * PAGE_SIZE);
-	if (IS_ERR(ringbuf))
-		return PTR_ERR(ringbuf);
+	if (IS_ERR(ringbuf)) {
+		ret = PTR_ERR(ringbuf);
+		goto error;
+	}
 	ring->buffer = ringbuf;
 
 	if (I915_NEED_GFX_HWS(dev)) {
@@ -2197,8 +2182,7 @@ static int intel_init_ring_buffer(struct drm_device *dev,
 	return 0;
 
 error:
-	intel_ringbuffer_free(ringbuf);
-	ring->buffer = NULL;
+	intel_cleanup_ring_buffer(ring);
 	return ret;
 }
 
@@ -2211,12 +2195,14 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
 
 	dev_priv = to_i915(ring->dev);
 
-	intel_stop_ring_buffer(ring);
-	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
+	if (ring->buffer) {
+		intel_stop_ring_buffer(ring);
+		WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
 
-	intel_unpin_ringbuffer_obj(ring->buffer);
-	intel_ringbuffer_free(ring->buffer);
-	ring->buffer = NULL;
+		intel_unpin_ringbuffer_obj(ring->buffer);
+		intel_ringbuffer_free(ring->buffer);
+		ring->buffer = NULL;
+	}
 
 	if (ring->cleanup)
 		ring->cleanup(ring);
@@ -2225,6 +2211,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
 
 	i915_cmd_parser_fini_ring(ring);
 	i915_gem_batch_pool_fini(&ring->batch_pool);
+	ring->dev = NULL;
 }
 
 static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 5d1eb20..49574ff 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -350,7 +350,11 @@ struct  intel_engine_cs {
 	u32 (*get_cmd_length_mask)(u32 cmd_header);
 };
 
-bool intel_ring_initialized(struct intel_engine_cs *ring);
+static inline bool
+intel_ring_initialized(struct intel_engine_cs *ring)
+{
+	return ring->dev != NULL;
+}
 
 static inline unsigned
 intel_ring_flag(struct intel_engine_cs *ring)
-- 
1.9.1


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC] drm/i915: for_each_engine()

[Dave Gordon]

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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC] drm/i915: for_each_engine()

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 0/2] Updating and simplifying for_each_engine()

[Dave Gordon]

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(-)

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/i915: introduce for_each_engine_id()

[Dave Gordon]

Equivalent to the existing for_each_engine() macro, this will replace
the latter wherever the third argument *is* actually wanted (in most
places, it is not used). The third argument is renamed to emphasise
that it is an engine id (type enum intel_engine_id). All the callers of
the macro that actually need the third argument are updated to use this
version, and the argument (generally 'i') is also updated to be 'id'.
Other callers (where the third argument is unused) are untouched for
now; they will be updated in the next patch.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 48 ++++++++++++++++--------------
 drivers/gpu/drm/i915/i915_drv.h            |  9 ++++++
 drivers/gpu/drm/i915/i915_gpu_error.c      |  6 ++--
 drivers/gpu/drm/i915/i915_guc_submission.c |  8 ++---
 drivers/gpu/drm/i915/i915_irq.c            | 10 +++----
 drivers/gpu/drm/i915/intel_mocs.c          |  6 ++--
 drivers/gpu/drm/i915/intel_ringbuffer.c    | 21 +++++++------
 7 files changed, 62 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index e0ba3e3..77dce52 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -132,7 +132,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
 	struct intel_engine_cs *engine;
 	struct i915_vma *vma;
 	int pin_count = 0;
-	int i;
+	enum intel_engine_id id;
 
 	seq_printf(m, "%pK: %s%s%s%s %8zdKiB %02x %02x [ ",
 		   &obj->base,
@@ -143,9 +143,9 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
 		   obj->base.size / 1024,
 		   obj->base.read_domains,
 		   obj->base.write_domain);
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine_id(engine, dev_priv, id)
 		seq_printf(m, "%x ",
-				i915_gem_request_get_seqno(obj->last_read_req[i]));
+				i915_gem_request_get_seqno(obj->last_read_req[id]));
 	seq_printf(m, "] %x %x%s%s%s",
 		   i915_gem_request_get_seqno(obj->last_write_req),
 		   i915_gem_request_get_seqno(obj->last_fenced_req),
@@ -1334,7 +1334,8 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
 	u64 acthd[I915_NUM_ENGINES];
 	u32 seqno[I915_NUM_ENGINES];
 	u32 instdone[I915_NUM_INSTDONE_REG];
-	int i, j;
+	enum intel_engine_id id;
+	int j;
 
 	if (!i915.enable_hangcheck) {
 		seq_printf(m, "Hangcheck disabled\n");
@@ -1343,9 +1344,9 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
 
 	intel_runtime_pm_get(dev_priv);
 
-	for_each_engine(engine, dev_priv, i) {
-		seqno[i] = engine->get_seqno(engine, false);
-		acthd[i] = intel_ring_get_active_head(engine);
+	for_each_engine_id(engine, dev_priv, id) {
+		seqno[id] = engine->get_seqno(engine, false);
+		acthd[id] = intel_ring_get_active_head(engine);
 	}
 
 	i915_get_extra_instdone(dev, instdone);
@@ -1359,13 +1360,13 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
 	} else
 		seq_printf(m, "Hangcheck inactive\n");
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine_id(engine, dev_priv, id) {
 		seq_printf(m, "%s:\n", engine->name);
 		seq_printf(m, "\tseqno = %x [current %x]\n",
-			   engine->hangcheck.seqno, seqno[i]);
+			   engine->hangcheck.seqno, seqno[id]);
 		seq_printf(m, "\tACTHD = 0x%08llx [current 0x%08llx]\n",
 			   (long long)engine->hangcheck.acthd,
-			   (long long)acthd[i]);
+			   (long long)acthd[id]);
 		seq_printf(m, "\tscore = %d\n", engine->hangcheck.score);
 		seq_printf(m, "\taction = %d\n", engine->hangcheck.action);
 
@@ -1947,7 +1948,8 @@ static int i915_context_status(struct seq_file *m, void *unused)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	struct intel_context *ctx;
-	int ret, i;
+	enum intel_engine_id id;
+	int ret;
 
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
@@ -1965,11 +1967,11 @@ static int i915_context_status(struct seq_file *m, void *unused)
 
 		if (i915.enable_execlists) {
 			seq_putc(m, '\n');
-			for_each_engine(engine, dev_priv, i) {
+			for_each_engine_id(engine, dev_priv, id) {
 				struct drm_i915_gem_object *ctx_obj =
-					ctx->engine[i].state;
+					ctx->engine[id].state;
 				struct intel_ringbuffer *ringbuf =
-					ctx->engine[i].ringbuf;
+					ctx->engine[id].ringbuf;
 
 				seq_printf(m, "%s: ", engine->name);
 				if (ctx_obj)
@@ -3134,7 +3136,8 @@ static int i915_semaphore_status(struct seq_file *m, void *unused)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	int num_rings = hweight32(INTEL_INFO(dev)->ring_mask);
-	int i, j, ret;
+	enum intel_engine_id id;
+	int j, ret;
 
 	if (!i915_semaphore_is_enabled(dev)) {
 		seq_puts(m, "Semaphores are disabled\n");
@@ -3153,14 +3156,14 @@ static int i915_semaphore_status(struct seq_file *m, void *unused)
 		page = i915_gem_object_get_page(dev_priv->semaphore_obj, 0);
 
 		seqno = (uint64_t *)kmap_atomic(page);
-		for_each_engine(engine, dev_priv, i) {
+		for_each_engine_id(engine, dev_priv, id) {
 			uint64_t offset;
 
 			seq_printf(m, "%s\n", engine->name);
 
 			seq_puts(m, "  Last signal:");
 			for (j = 0; j < num_rings; j++) {
-				offset = i * I915_NUM_ENGINES + j;
+				offset = id * I915_NUM_ENGINES + j;
 				seq_printf(m, "0x%08llx (0x%02llx) ",
 					   seqno[offset], offset * 8);
 			}
@@ -3168,7 +3171,7 @@ static int i915_semaphore_status(struct seq_file *m, void *unused)
 
 			seq_puts(m, "  Last wait:  ");
 			for (j = 0; j < num_rings; j++) {
-				offset = i + (j * I915_NUM_ENGINES);
+				offset = id + (j * I915_NUM_ENGINES);
 				seq_printf(m, "0x%08llx (0x%02llx) ",
 					   seqno[offset], offset * 8);
 			}
@@ -3178,7 +3181,7 @@ static int i915_semaphore_status(struct seq_file *m, void *unused)
 		kunmap_atomic(seqno);
 	} else {
 		seq_puts(m, "  Last signal:");
-		for_each_engine(engine, dev_priv, i)
+		for_each_engine(engine, dev_priv, id)
 			for (j = 0; j < num_rings; j++)
 				seq_printf(m, "0x%08x\n",
 					   I915_READ(engine->semaphore.mbox.signal[j]));
@@ -3186,7 +3189,7 @@ static int i915_semaphore_status(struct seq_file *m, void *unused)
 	}
 
 	seq_puts(m, "\nSync seqno:\n");
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv, id) {
 		for (j = 0; j < num_rings; j++) {
 			seq_printf(m, "  0x%08x ",
 				   engine->semaphore.sync_seqno[j]);
@@ -3236,6 +3239,7 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_workarounds *workarounds = &dev_priv->workarounds;
+	enum intel_engine_id id;
 
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
@@ -3244,9 +3248,9 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
 	intel_runtime_pm_get(dev_priv);
 
 	seq_printf(m, "Workarounds applied: %d\n", workarounds->count);
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine_id(engine, dev_priv, id)
 		seq_printf(m, "HW whitelist count for %s: %d\n",
-			   engine->name, workarounds->hw_whitelist_count[i]);
+			   engine->name, workarounds->hw_whitelist_count[id]);
 	for (i = 0; i < workarounds->count; ++i) {
 		i915_reg_t addr;
 		u32 mask, value, read;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9d29ab0..62ee86c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2013,6 +2013,15 @@ static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
 	for ((i__) = 0; (i__) < I915_NUM_ENGINES; (i__)++) \
 		for_each_if ((((ring__) = &(dev_priv__)->engine[(i__)]), intel_engine_initialized((ring__))))
 
+/* Iterator with engine_id */
+#define for_each_engine_id(engine__, dev_priv__, id__) \
+	for ((engine__) = &(dev_priv__)->engine[0], (id__) = 0; \
+	     (engine__) < &(dev_priv__)->engine[I915_NUM_ENGINES]; \
+	     (engine__)++) \
+		for_each_if (((id__) = (engine__)->id, \
+			      intel_engine_initialized(engine__)))
+
+/* Iterator over subset of engines selected by mask */
 #define for_each_engine_masked(engine__, dev_priv__, mask__) \
 	for ((engine__) = &dev_priv->engine[0]; (engine__) < &dev_priv->engine[I915_NUM_ENGINES]; (engine__)++) \
 		for_each_if (intel_engine_flag((engine__)) & (mask__) && intel_engine_initialized((engine__)))
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 1f8ff06..54c2086 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -846,7 +846,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
 					struct drm_i915_error_ring *ering)
 {
 	struct intel_engine_cs *to;
-	int i;
+	enum intel_engine_id id;
 
 	if (!i915_semaphore_is_enabled(dev_priv->dev))
 		return;
@@ -856,7 +856,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
 			i915_error_ggtt_object_create(dev_priv,
 						      dev_priv->semaphore_obj);
 
-	for_each_engine(to, dev_priv, i) {
+	for_each_engine_id(to, dev_priv, id) {
 		int idx;
 		u16 signal_offset;
 		u32 *tmp;
@@ -864,7 +864,7 @@ static void gen8_record_semaphore_state(struct drm_i915_private *dev_priv,
 		if (engine == to)
 			continue;
 
-		signal_offset = (GEN8_SIGNAL_OFFSET(engine, i) & (PAGE_SIZE - 1))
+		signal_offset = (GEN8_SIGNAL_OFFSET(engine, id) & (PAGE_SIZE - 1))
 				/ 4;
 		tmp = error->semaphore_obj->pages[0];
 		idx = intel_ring_sync_index(engine, to);
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index ae1f58d..0611bdc 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -381,7 +381,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 	struct intel_context *ctx = client->owner;
 	struct guc_context_desc desc;
 	struct sg_table *sg;
-	int i;
+	enum intel_engine_id id;
 
 	memset(&desc, 0, sizeof(desc));
 
@@ -390,7 +390,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 	desc.priority = client->priority;
 	desc.db_id = client->doorbell_id;
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine_id(engine, dev_priv, id) {
 		struct guc_execlist_context *lrc = &desc.lrc[engine->guc_id];
 		struct drm_i915_gem_object *obj;
 		uint64_t ctx_desc;
@@ -402,7 +402,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 		 * for now who owns a GuC client. But for future owner of GuC
 		 * client, need to make sure lrc is pinned prior to enter here.
 		 */
-		obj = ctx->engine[i].state;
+		obj = ctx->engine[id].state;
 		if (!obj)
 			break;	/* XXX: continue? */
 
@@ -415,7 +415,7 @@ static void guc_init_ctx_desc(struct intel_guc *guc,
 		lrc->context_id = (client->ctx_index << GUC_ELC_CTXID_OFFSET) |
 				(engine->guc_id << GUC_ELC_ENGINE_OFFSET);
 
-		obj = ctx->engine[i].ringbuf->obj;
+		obj = ctx->engine[id].ringbuf->obj;
 
 		lrc->ring_begin = i915_gem_obj_ggtt_offset(obj);
 		lrc->ring_end = lrc->ring_begin + obj->base.size - 1;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a55a7cc..14a23b3 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3073,7 +3073,7 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 			     gpu_error.hangcheck_work.work);
 	struct drm_device *dev = dev_priv->dev;
 	struct intel_engine_cs *engine;
-	int i;
+	enum intel_engine_id id;
 	int busy_count = 0, rings_hung = 0;
 	bool stuck[I915_NUM_ENGINES] = { 0 };
 #define BUSY 1
@@ -3097,7 +3097,7 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 	 */
 	intel_uncore_arm_unclaimed_mmio_detection(dev_priv);
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine_id(engine, dev_priv, id) {
 		u64 acthd;
 		u32 seqno;
 		bool busy = true;
@@ -3157,7 +3157,7 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 					break;
 				case HANGCHECK_HUNG:
 					engine->hangcheck.score += HUNG;
-					stuck[i] = true;
+					stuck[id] = true;
 					break;
 				}
 			}
@@ -3184,10 +3184,10 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
 		busy_count += busy;
 	}
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine_id(engine, dev_priv, id) {
 		if (engine->hangcheck.score >= HANGCHECK_SCORE_RING_HUNG) {
 			DRM_INFO("%s on %s\n",
-				 stuck[i] ? "stuck" : "no progress",
+				 stuck[id] ? "stuck" : "no progress",
 				 engine->name);
 			rings_hung |= intel_engine_flag(engine);
 		}
diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
index 3c725dd..7c7ac0a 100644
--- a/drivers/gpu/drm/i915/intel_mocs.c
+++ b/drivers/gpu/drm/i915/intel_mocs.c
@@ -325,11 +325,11 @@ int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req)
 	if (get_mocs_settings(req->engine->dev, &t)) {
 		struct drm_i915_private *dev_priv = req->i915;
 		struct intel_engine_cs *engine;
-		enum intel_engine_id ring_id;
+		enum intel_engine_id id;
 
 		/* Program the control registers */
-		for_each_engine(engine, dev_priv, ring_id) {
-			ret = emit_mocs_control_table(req, &t, ring_id);
+		for_each_engine_id(engine, dev_priv, id) {
+			ret = emit_mocs_control_table(req, &t, id);
 			if (ret)
 				return ret;
 		}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ce59850..a492bca 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1280,7 +1280,8 @@ static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req,
 	struct drm_device *dev = signaller->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *waiter;
-	int i, ret, num_rings;
+	enum intel_engine_id id;
+	int ret, num_rings;
 
 	num_rings = hweight32(INTEL_INFO(dev)->ring_mask);
 	num_dwords += (num_rings-1) * MBOX_UPDATE_DWORDS;
@@ -1290,9 +1291,9 @@ static int gen8_rcs_signal(struct drm_i915_gem_request *signaller_req,
 	if (ret)
 		return ret;
 
-	for_each_engine(waiter, dev_priv, i) {
+	for_each_engine_id(waiter, dev_priv, id) {
 		u32 seqno;
-		u64 gtt_offset = signaller->semaphore.signal_ggtt[i];
+		u64 gtt_offset = signaller->semaphore.signal_ggtt[id];
 		if (gtt_offset == MI_SEMAPHORE_SYNC_INVALID)
 			continue;
 
@@ -1321,7 +1322,8 @@ static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req,
 	struct drm_device *dev = signaller->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *waiter;
-	int i, ret, num_rings;
+	enum intel_engine_id id;
+	int ret, num_rings;
 
 	num_rings = hweight32(INTEL_INFO(dev)->ring_mask);
 	num_dwords += (num_rings-1) * MBOX_UPDATE_DWORDS;
@@ -1331,9 +1333,9 @@ static int gen8_xcs_signal(struct drm_i915_gem_request *signaller_req,
 	if (ret)
 		return ret;
 
-	for_each_engine(waiter, dev_priv, i) {
+	for_each_engine_id(waiter, dev_priv, id) {
 		u32 seqno;
-		u64 gtt_offset = signaller->semaphore.signal_ggtt[i];
+		u64 gtt_offset = signaller->semaphore.signal_ggtt[id];
 		if (gtt_offset == MI_SEMAPHORE_SYNC_INVALID)
 			continue;
 
@@ -1359,7 +1361,8 @@ static int gen6_signal(struct drm_i915_gem_request *signaller_req,
 	struct drm_device *dev = signaller->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *useless;
-	int i, ret, num_rings;
+	enum intel_engine_id id;
+	int ret, num_rings;
 
 #define MBOX_UPDATE_DWORDS 3
 	num_rings = hweight32(INTEL_INFO(dev)->ring_mask);
@@ -1370,8 +1373,8 @@ static int gen6_signal(struct drm_i915_gem_request *signaller_req,
 	if (ret)
 		return ret;
 
-	for_each_engine(useless, dev_priv, i) {
-		i915_reg_t mbox_reg = signaller->semaphore.mbox.signal[i];
+	for_each_engine_id(useless, dev_priv, id) {
+		i915_reg_t mbox_reg = signaller->semaphore.mbox.signal[id];
 
 		if (i915_mmio_reg_valid(mbox_reg)) {
 			u32 seqno = i915_gem_request_get_seqno(signaller_req);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: replace for_each_engine()

[Dave Gordon]

Having provided for_each_engine_id() for cases where the third (id)
argument is useful, we can now replace all the remaining instances with
a simpler version that takes only two parameters. In many cases, this
also allows the elimination of the local variable used in the iterator
(usually 'i').

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 50 +++++++++++++-----------------
 drivers/gpu/drm/i915/i915_drv.h            | 17 ++++++----
 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_guc_submission.c |  6 ++--
 drivers/gpu/drm/i915/i915_irq.c            | 14 +++------
 drivers/gpu/drm/i915/intel_guc_loader.c    |  8 ++---
 drivers/gpu/drm/i915/intel_lrc.c           |  3 +-
 drivers/gpu/drm/i915/intel_pm.c            | 19 +++++-------
 11 files changed, 82 insertions(+), 103 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 77dce52..d02f8ce 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -398,11 +398,11 @@ static void print_batch_pool_stats(struct seq_file *m,
 	struct drm_i915_gem_object *obj;
 	struct file_stats stats;
 	struct intel_engine_cs *engine;
-	int i, j;
+	int j;
 
 	memset(&stats, 0, sizeof(stats));
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		for (j = 0; j < ARRAY_SIZE(engine->batch_pool.cache_list); j++) {
 			list_for_each_entry(obj,
 					    &engine->batch_pool.cache_list[j],
@@ -638,13 +638,13 @@ static int i915_gem_batch_pool_info(struct seq_file *m, void *data)
 	struct drm_i915_gem_object *obj;
 	struct intel_engine_cs *engine;
 	int total = 0;
-	int ret, i, j;
+	int ret, j;
 
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		return ret;
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		for (j = 0; j < ARRAY_SIZE(engine->batch_pool.cache_list); j++) {
 			int count;
 
@@ -682,14 +682,14 @@ static int i915_gem_request_info(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	struct drm_i915_gem_request *req;
-	int ret, any, i;
+	int ret, any;
 
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		return ret;
 
 	any = 0;
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		int count;
 
 		count = 0;
@@ -739,14 +739,14 @@ static int i915_gem_seqno_info(struct seq_file *m, void *data)
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int ret, i;
+	int ret;
 
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		return ret;
 	intel_runtime_pm_get(dev_priv);
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		i915_ring_seqno_info(m, engine);
 
 	intel_runtime_pm_put(dev_priv);
@@ -933,7 +933,7 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
 		seq_printf(m, "Graphics Interrupt mask:		%08x\n",
 			   I915_READ(GTIMR));
 	}
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		if (INTEL_INFO(dev)->gen >= 6) {
 			seq_printf(m,
 				   "Graphics Interrupt mask (%s):	%08x\n",
@@ -2044,7 +2044,7 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	struct intel_context *ctx;
-	int ret, i;
+	int ret;
 
 	if (!i915.enable_execlists) {
 		seq_printf(m, "Logical Ring Contexts are disabled\n");
@@ -2057,7 +2057,7 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
 
 	list_for_each_entry(ctx, &dev_priv->context_list, link)
 		if (ctx != dev_priv->kernel_context)
-			for_each_engine(engine, dev_priv, i)
+			for_each_engine(engine, dev_priv)
 				i915_dump_lrc_obj(m, ctx, engine);
 
 	mutex_unlock(&dev->struct_mutex);
@@ -2077,8 +2077,7 @@ static int i915_execlists(struct seq_file *m, void *data)
 	u32 status;
 	u32 ctx_id;
 	struct list_head *cursor;
-	int ring_id, i;
-	int ret;
+	int i, ret;
 
 	if (!i915.enable_execlists) {
 		seq_puts(m, "Logical Ring Contexts are disabled\n");
@@ -2091,7 +2090,7 @@ static int i915_execlists(struct seq_file *m, void *data)
 
 	intel_runtime_pm_get(dev_priv);
 
-	for_each_engine(engine, dev_priv, ring_id) {
+	for_each_engine(engine, dev_priv) {
 		struct drm_i915_gem_request *head_req = NULL;
 		int count = 0;
 		unsigned long flags;
@@ -2250,12 +2249,12 @@ static void gen8_ppgtt_info(struct seq_file *m, struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
-	int unused, i;
+	int i;
 
 	if (!ppgtt)
 		return;
 
-	for_each_engine(engine, dev_priv, unused) {
+	for_each_engine(engine, dev_priv) {
 		seq_printf(m, "%s\n", engine->name);
 		for (i = 0; i < 4; i++) {
 			u64 pdp = I915_READ(GEN8_RING_PDP_UDW(engine, i));
@@ -2270,12 +2269,11 @@ static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int i;
 
 	if (INTEL_INFO(dev)->gen == 6)
 		seq_printf(m, "GFX_MODE: 0x%08x\n", I915_READ(GFX_MODE));
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		seq_printf(m, "%s\n", engine->name);
 		if (INTEL_INFO(dev)->gen == 7)
 			seq_printf(m, "GFX_MODE: 0x%08x\n",
@@ -2342,9 +2340,8 @@ static int count_irq_waiters(struct drm_i915_private *i915)
 {
 	struct intel_engine_cs *engine;
 	int count = 0;
-	int i;
 
-	for_each_engine(engine, i915, i)
+	for_each_engine(engine, i915)
 		count += engine->irq_refcount;
 
 	return count;
@@ -2455,7 +2452,6 @@ static void i915_guc_client_info(struct seq_file *m,
 {
 	struct intel_engine_cs *engine;
 	uint64_t tot = 0;
-	uint32_t i;
 
 	seq_printf(m, "\tPriority %d, GuC ctx index: %u, PD offset 0x%x\n",
 		client->priority, client->ctx_index, client->proc_desc_offset);
@@ -2468,7 +2464,7 @@ static void i915_guc_client_info(struct seq_file *m,
 	seq_printf(m, "\tFailed doorbell: %u\n", client->b_fail);
 	seq_printf(m, "\tLast submission result: %d\n", client->retcode);
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		seq_printf(m, "\tSubmissions: %llu %s\n",
 				client->submissions[engine->guc_id],
 				engine->name);
@@ -2485,7 +2481,6 @@ static int i915_guc_info(struct seq_file *m, void *data)
 	struct intel_guc guc;
 	struct i915_guc_client client = {};
 	struct intel_engine_cs *engine;
-	enum intel_engine_id i;
 	u64 total = 0;
 
 	if (!HAS_GUC_SCHED(dev_priv->dev))
@@ -2508,7 +2503,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
 	seq_printf(m, "GuC last action error code: %d\n", guc.action_err);
 
 	seq_printf(m, "\nGuC submissions:\n");
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		seq_printf(m, "\t%-24s: %10llu, last seqno 0x%08x\n",
 			engine->name, guc.submissions[engine->guc_id],
 			guc.last_seqno[engine->guc_id]);
@@ -3181,7 +3176,7 @@ static int i915_semaphore_status(struct seq_file *m, void *unused)
 		kunmap_atomic(seqno);
 	} else {
 		seq_puts(m, "  Last signal:");
-		for_each_engine(engine, dev_priv, id)
+		for_each_engine(engine, dev_priv)
 			for (j = 0; j < num_rings; j++)
 				seq_printf(m, "0x%08x\n",
 					   I915_READ(engine->semaphore.mbox.signal[j]));
@@ -3189,11 +3184,10 @@ static int i915_semaphore_status(struct seq_file *m, void *unused)
 	}
 
 	seq_puts(m, "\nSync seqno:\n");
-	for_each_engine(engine, dev_priv, id) {
-		for (j = 0; j < num_rings; j++) {
+	for_each_engine(engine, dev_priv) {
+		for (j = 0; j < num_rings; j++)
 			seq_printf(m, "  0x%08x ",
 				   engine->semaphore.sync_seqno[j]);
-		}
 		seq_putc(m, '\n');
 	}
 	seq_putc(m, '\n');
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 62ee86c..e462683 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2008,10 +2008,12 @@ 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_engine(ring__, dev_priv__, i__) \
-	for ((i__) = 0; (i__) < I915_NUM_ENGINES; (i__)++) \
-		for_each_if ((((ring__) = &(dev_priv__)->engine[(i__)]), intel_engine_initialized((ring__))))
+/* Simple iterator over all initialised engines */
+#define for_each_engine(engine__, dev_priv__) \
+	for ((engine__) = &(dev_priv__)->engine[0]; \
+	     (engine__) < &(dev_priv__)->engine[I915_NUM_ENGINES]; \
+	     (engine__)++) \
+		for_each_if (intel_engine_initialized(engine__))
 
 /* Iterator with engine_id */
 #define for_each_engine_id(engine__, dev_priv__, id__) \
@@ -2023,8 +2025,11 @@ static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
 
 /* Iterator over subset of engines selected by mask */
 #define for_each_engine_masked(engine__, dev_priv__, mask__) \
-	for ((engine__) = &dev_priv->engine[0]; (engine__) < &dev_priv->engine[I915_NUM_ENGINES]; (engine__)++) \
-		for_each_if (intel_engine_flag((engine__)) & (mask__) && intel_engine_initialized((engine__)))
+	for ((engine__) = &dev_priv->engine[0]; \
+	     (engine__) < &dev_priv->engine[I915_NUM_ENGINES]; \
+	     (engine__)++) \
+		for_each_if (((mask__) & intel_engine_flag(engine__)) && \
+			     intel_engine_initialized(engine__))
 
 enum hdmi_force_audio {
 	HDMI_AUDIO_OFF_DVI = -2,	/* no aux data for HDMI-DVI converter */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8588c83..c7a997a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2466,10 +2466,10 @@ void i915_vma_move_to_active(struct i915_vma *vma,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int ret, i, j;
+	int ret, j;
 
 	/* Carefully retire all requests without writing to the rings */
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		ret = intel_engine_idle(engine);
 		if (ret)
 			return ret;
@@ -2477,7 +2477,7 @@ void i915_vma_move_to_active(struct i915_vma *vma,
 	i915_gem_retire_requests(dev);
 
 	/* Finally reset hw state */
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		intel_ring_init_seqno(engine, seqno);
 
 		for (j = 0; j < ARRAY_SIZE(engine->semaphore.sync_seqno); j++)
@@ -2884,17 +2884,16 @@ void i915_gem_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int i;
 
 	/*
 	 * Before we free the objects from the requests, we need to inspect
 	 * them for finding the guilty party. As the requests only borrow
 	 * their reference to the objects, the inspection must be done first.
 	 */
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		i915_gem_reset_engine_status(dev_priv, engine);
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		i915_gem_reset_engine_cleanup(dev_priv, engine);
 
 	i915_gem_context_reset(dev);
@@ -2962,9 +2961,8 @@ void i915_gem_reset(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	bool idle = true;
-	int i;
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		i915_gem_retire_requests_ring(engine);
 		idle &= list_empty(&engine->request_list);
 		if (i915.enable_execlists) {
@@ -3009,24 +3007,20 @@ void i915_gem_reset(struct drm_device *dev)
 	struct drm_i915_private *dev_priv =
 		container_of(work, typeof(*dev_priv), mm.idle_work.work);
 	struct drm_device *dev = dev_priv->dev;
-	struct intel_engine_cs *ring;
-	int i;
+	struct intel_engine_cs *engine;
 
-	for_each_engine(ring, dev_priv, i)
-		if (!list_empty(&ring->request_list))
+	for_each_engine(engine, dev_priv)
+		if (!list_empty(&engine->request_list))
 			return;
 
 	/* we probably should sync with hangcheck here, using cancel_work_sync.
-	 * Also locking seems to be fubar here, ring->request_list is protected
+	 * Also locking seems to be fubar here, engine->request_list is protected
 	 * by dev->struct_mutex. */
 
 	intel_mark_idle(dev);
 
 	if (mutex_trylock(&dev->struct_mutex)) {
-		struct intel_engine_cs *engine;
-		int i;
-
-		for_each_engine(engine, dev_priv, i)
+		for_each_engine(engine, dev_priv)
 			i915_gem_batch_pool_fini(&engine->batch_pool);
 
 		mutex_unlock(&dev->struct_mutex);
@@ -3390,10 +3384,10 @@ int i915_gpu_idle(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int ret, i;
+	int ret;
 
 	/* Flush everything onto the inactive list. */
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		if (!i915.enable_execlists) {
 			struct drm_i915_gem_request *req;
 
@@ -4655,9 +4649,8 @@ void i915_gem_vma_destroy(struct i915_vma *vma)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int i;
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		dev_priv->gt.stop_engine(engine);
 }
 
@@ -4828,7 +4821,7 @@ int i915_gem_init_engines(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int ret, i, j;
+	int ret, j;
 
 	if (INTEL_INFO(dev)->gen < 6 && !intel_enable_gtt())
 		return -EIO;
@@ -4874,7 +4867,7 @@ int i915_gem_init_engines(struct drm_device *dev)
 	}
 
 	/* Need to do basic initialisation of all rings first: */
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		ret = engine->init_hw(engine);
 		if (ret)
 			goto out;
@@ -4899,7 +4892,7 @@ int i915_gem_init_engines(struct drm_device *dev)
 		goto out;
 
 	/* Now it is safe to go back round and do everything else: */
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		struct drm_i915_gem_request *req;
 
 		req = i915_gem_request_alloc(engine, NULL);
@@ -4916,7 +4909,8 @@ int i915_gem_init_engines(struct drm_device *dev)
 
 		ret = i915_ppgtt_init_ring(req);
 		if (ret && ret != -EIO) {
-			DRM_ERROR("PPGTT enable ring #%d failed %d\n", i, ret);
+			DRM_ERROR("PPGTT enable %s failed %d\n",
+				  engine->name, ret);
 			i915_gem_request_cancel(req);
 			i915_gem_cleanup_engines(dev);
 			goto out;
@@ -4924,7 +4918,8 @@ int i915_gem_init_engines(struct drm_device *dev)
 
 		ret = i915_gem_context_enable(req);
 		if (ret && ret != -EIO) {
-			DRM_ERROR("Context enable ring #%d failed %d\n", i, ret);
+			DRM_ERROR("Context enable %s failed %d\n",
+				  engine->name, ret);
 			i915_gem_request_cancel(req);
 			i915_gem_cleanup_engines(dev);
 			goto out;
@@ -5005,9 +5000,8 @@ int i915_gem_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int i;
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		dev_priv->gt.cleanup_engine(engine);
 
 	if (i915.enable_execlists)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 394e525..fe580cb 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -517,7 +517,7 @@ struct intel_context *
 		i915_semaphore_is_enabled(engine->dev) ?
 		hweight32(INTEL_INFO(engine->dev)->ring_mask) - 1 :
 		0;
-	int len, i, ret;
+	int len, ret;
 
 	/* w/a: If Flush TLB Invalidation Mode is enabled, driver must do a TLB
 	 * invalidation prior to MI_SET_CONTEXT. On GEN6 we don't set the value
@@ -553,7 +553,7 @@ struct intel_context *
 
 			intel_ring_emit(engine,
 					MI_LOAD_REGISTER_IMM(num_rings));
-			for_each_engine(signaller, to_i915(engine->dev), i) {
+			for_each_engine(signaller, to_i915(engine->dev)) {
 				if (signaller == engine)
 					continue;
 
@@ -582,7 +582,7 @@ struct intel_context *
 
 			intel_ring_emit(engine,
 					MI_LOAD_REGISTER_IMM(num_rings));
-			for_each_engine(signaller, to_i915(engine->dev), i) {
+			for_each_engine(signaller, to_i915(engine->dev)) {
 				if (signaller == engine)
 					continue;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_debug.c b/drivers/gpu/drm/i915/i915_gem_debug.c
index ef9cd70..a565164 100644
--- a/drivers/gpu/drm/i915/i915_gem_debug.c
+++ b/drivers/gpu/drm/i915/i915_gem_debug.c
@@ -38,12 +38,11 @@
 	struct drm_i915_gem_object *obj;
 	struct intel_engine_cs *engine;
 	int err = 0;
-	int i;
 
 	if (warned)
 		return 0;
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		list_for_each_entry(obj, &engine->active_list,
 				    engine_list[engine->id]) {
 			if (obj->base.dev != dev ||
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0715bb7..59e1821 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1737,9 +1737,8 @@ static void gen8_ppgtt_enable(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int j;
 
-	for_each_engine(engine, dev_priv, j) {
+	for_each_engine(engine, dev_priv) {
 		u32 four_level = USES_FULL_48BIT_PPGTT(dev) ? GEN8_GFX_PPGTT_48B : 0;
 		I915_WRITE(RING_MODE_GEN7(engine),
 			   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE | four_level));
@@ -1751,7 +1750,6 @@ static void gen7_ppgtt_enable(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	uint32_t ecochk, ecobits;
-	int i;
 
 	ecobits = I915_READ(GAC_ECO_BITS);
 	I915_WRITE(GAC_ECO_BITS, ecobits | ECOBITS_PPGTT_CACHE64B);
@@ -1765,7 +1763,7 @@ static void gen7_ppgtt_enable(struct drm_device *dev)
 	}
 	I915_WRITE(GAM_ECOCHK, ecochk);
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		/* GFX_MODE is per-ring on gen7+ */
 		I915_WRITE(RING_MODE_GEN7(engine),
 			   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
@@ -2287,12 +2285,11 @@ void i915_check_and_clear_faults(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int i;
 
 	if (INTEL_INFO(dev)->gen < 6)
 		return;
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		u32 fault_reg;
 		fault_reg = I915_READ(RING_FAULT_REG(engine));
 		if (fault_reg & RING_FAULT_VALID) {
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 0611bdc..da86bdb 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -842,7 +842,7 @@ static void guc_create_ads(struct intel_guc *guc)
 	struct guc_mmio_reg_state *reg_state;
 	struct intel_engine_cs *engine;
 	struct page *page;
-	u32 size, i;
+	u32 size;
 
 	/* The ads obj includes the struct itself and buffers passed to GuC */
 	size = sizeof(struct guc_ads) + sizeof(struct guc_policies) +
@@ -871,7 +871,7 @@ static void guc_create_ads(struct intel_guc *guc)
 	engine = &dev_priv->engine[RCS];
 	ads->golden_context_lrca = engine->status_page.gfx_addr;
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		ads->eng_state_size[engine->guc_id] = intel_lr_context_size(engine);
 
 	/* GuC scheduling policies */
@@ -884,7 +884,7 @@ static void guc_create_ads(struct intel_guc *guc)
 	/* MMIO reg state */
 	reg_state = (void *)policies + sizeof(struct guc_policies);
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		reg_state->mmio_white_list[engine->guc_id].mmio_start =
 			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 14a23b3..5aa4239 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1080,9 +1080,8 @@ static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
 static bool any_waiters(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
-	int i;
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		if (engine->irq_refcount)
 			return true;
 
@@ -2450,7 +2449,6 @@ static void i915_error_wake_up(struct drm_i915_private *dev_priv,
 			       bool reset_completed)
 {
 	struct intel_engine_cs *engine;
-	int i;
 
 	/*
 	 * Notify all waiters for GPU completion events that reset state has
@@ -2460,7 +2458,7 @@ static void i915_error_wake_up(struct drm_i915_private *dev_priv,
 	 */
 
 	/* Wake up __wait_seqno, potentially holding dev->struct_mutex. */
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		wake_up_all(&engine->irq_queue);
 
 	/* Wake up intel_crtc_wait_for_pending_flips, holding crtc->mutex. */
@@ -2829,10 +2827,9 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = engine->dev->dev_private;
 	struct intel_engine_cs *signaller;
-	int i;
 
 	if (INTEL_INFO(dev_priv->dev)->gen >= 8) {
-		for_each_engine(signaller, dev_priv, i) {
+		for_each_engine(signaller, dev_priv) {
 			if (engine == signaller)
 				continue;
 
@@ -2842,7 +2839,7 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
 	} else {
 		u32 sync_bits = ipehr & MI_SEMAPHORE_SYNC_MASK;
 
-		for_each_engine(signaller, dev_priv, i) {
+		for_each_engine(signaller, dev_priv) {
 			if(engine == signaller)
 				continue;
 
@@ -2958,9 +2955,8 @@ static int semaphore_passed(struct intel_engine_cs *engine)
 static void semaphore_clear_deadlocks(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
-	int i;
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		engine->hangcheck.deadlock = 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index e1aff62..b4976f9 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -82,12 +82,12 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
 static void direct_interrupts_to_host(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
-	int i, irqs;
+	int irqs;
 
 	/* tell all command streamers NOT to forward interrupts and vblank to GuC */
 	irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK, GFX_FORWARD_VBLANK_NEVER);
 	irqs |= _MASKED_BIT_DISABLE(GFX_INTERRUPT_STEERING);
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MODE_GEN7(engine), irqs);
 
 	/* route all GT interrupts to the host */
@@ -99,12 +99,12 @@ static void direct_interrupts_to_host(struct drm_i915_private *dev_priv)
 static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
-	int i, irqs;
+	int irqs;
 
 	/* tell all command streamers to forward interrupts and vblank to GuC */
 	irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK, GFX_FORWARD_VBLANK_ALWAYS);
 	irqs |= _MASKED_BIT_ENABLE(GFX_INTERRUPT_STEERING);
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MODE_GEN7(engine), irqs);
 
 	/* route USER_INTERRUPT to Host, all others are sent to GuC. */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 40ef4ea..5d4ca3b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2673,9 +2673,8 @@ void intel_lr_context_reset(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int i;
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		struct drm_i915_gem_object *ctx_obj =
 				ctx->engine[engine->id].state;
 		struct intel_ringbuffer *ringbuf =
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 521cf45..6a04761 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4817,7 +4817,6 @@ static void gen9_enable_rc6(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	uint32_t rc6_mask = 0;
-	int unused;
 
 	/* 1a: Software RC state - RC0 */
 	I915_WRITE(GEN6_RC_STATE, 0);
@@ -4838,7 +4837,7 @@ static void gen9_enable_rc6(struct drm_device *dev)
 		I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 54 << 16);
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
 	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */
-	for_each_engine(engine, dev_priv, unused)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 
 	if (HAS_GUC_UCODE(dev))
@@ -4887,7 +4886,6 @@ static void gen8_enable_rps(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	uint32_t rc6_mask = 0;
-	int unused;
 
 	/* 1a: Software RC state - RC0 */
 	I915_WRITE(GEN6_RC_STATE, 0);
@@ -4906,7 +4904,7 @@ static void gen8_enable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
 	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */
-	for_each_engine(engine, dev_priv, unused)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 	I915_WRITE(GEN6_RC_SLEEP, 0);
 	if (IS_BROADWELL(dev))
@@ -4971,7 +4969,7 @@ static void gen6_enable_rps(struct drm_device *dev)
 	u32 rc6vids, pcu_mbox = 0, rc6_mask = 0;
 	u32 gtfifodbg;
 	int rc6_mode;
-	int i, ret;
+	int ret;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
@@ -5003,7 +5001,7 @@ static void gen6_enable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
 	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25);
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 
 	I915_WRITE(GEN6_RC_SLEEP, 0);
@@ -5497,7 +5495,6 @@ static void cherryview_enable_rps(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	u32 gtfifodbg, val, rc6_mode = 0, pcbr;
-	int i;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
@@ -5522,7 +5519,7 @@ static void cherryview_enable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
 	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 	I915_WRITE(GEN6_RC_SLEEP, 0);
 
@@ -5595,7 +5592,6 @@ static void valleyview_enable_rps(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	u32 gtfifodbg, val, rc6_mode = 0;
-	int i;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
@@ -5633,7 +5629,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
 	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25);
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 
 	I915_WRITE(GEN6_RC6_THRESHOLD, 0x557);
@@ -6012,14 +6008,13 @@ bool i915_gpu_busy(void)
 	struct drm_i915_private *dev_priv;
 	struct intel_engine_cs *engine;
 	bool ret = false;
-	int i;
 
 	spin_lock_irq(&mchdev_lock);
 	if (!i915_mch_dev)
 		goto out_unlock;
 	dev_priv = i915_mch_dev;
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		ret |= !list_empty(&engine->request_list);
 
 out_unlock:
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: replace for_each_engine()

[Chris Wilson]

On Wed, Mar 23, 2016 at 06:19:54PM +0000, Dave Gordon wrote:
> @@ -2023,8 +2025,11 @@ static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
>  
>  /* Iterator over subset of engines selected by mask */
>  #define for_each_engine_masked(engine__, dev_priv__, mask__) \
> -	for ((engine__) = &dev_priv->engine[0]; (engine__) < &dev_priv->engine[I915_NUM_ENGINES]; (engine__)++) \
> -		for_each_if (intel_engine_flag((engine__)) & (mask__) && intel_engine_initialized((engine__)))
> +	for ((engine__) = &dev_priv->engine[0]; \
> +	     (engine__) < &dev_priv->engine[I915_NUM_ENGINES]; \
> +	     (engine__)++) \
> +		for_each_if (((mask__) & intel_engine_flag(engine__)) && \
> +			     intel_engine_initialized(engine__))

You've touched it, might as well fix it!
	s/dev_priv/(dev_priv__)/

With the macro fixed, both patches
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: fix potential dangling else problems in for_each_ macros (rev3)

[Patchwork]

== 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)
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)
        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()

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2 v2] drm/i915: replace for_each_engine()

[Dave Gordon]

Having provided for_each_engine_id() for cases where the third (id)
argument is useful, we can now replace all the remaining instances with
a simpler version that takes only two parameters. In many cases, this
also allows the elimination of the local variable used in the iterator
(usually 'i').

v2:
    s/dev_priv/(dev_priv__)/ in body of for_each_engine_masked() [Chris Wilson]

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 50 +++++++++++++-----------------
 drivers/gpu/drm/i915/i915_drv.h            | 17 ++++++----
 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_guc_submission.c |  6 ++--
 drivers/gpu/drm/i915/i915_irq.c            | 14 +++------
 drivers/gpu/drm/i915/intel_guc_loader.c    |  8 ++---
 drivers/gpu/drm/i915/intel_lrc.c           |  3 +-
 drivers/gpu/drm/i915/intel_pm.c            | 19 +++++-------
 11 files changed, 82 insertions(+), 103 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 77dce52..d02f8ce 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -398,11 +398,11 @@ static void print_batch_pool_stats(struct seq_file *m,
 	struct drm_i915_gem_object *obj;
 	struct file_stats stats;
 	struct intel_engine_cs *engine;
-	int i, j;
+	int j;
 
 	memset(&stats, 0, sizeof(stats));
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		for (j = 0; j < ARRAY_SIZE(engine->batch_pool.cache_list); j++) {
 			list_for_each_entry(obj,
 					    &engine->batch_pool.cache_list[j],
@@ -638,13 +638,13 @@ static int i915_gem_batch_pool_info(struct seq_file *m, void *data)
 	struct drm_i915_gem_object *obj;
 	struct intel_engine_cs *engine;
 	int total = 0;
-	int ret, i, j;
+	int ret, j;
 
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		return ret;
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		for (j = 0; j < ARRAY_SIZE(engine->batch_pool.cache_list); j++) {
 			int count;
 
@@ -682,14 +682,14 @@ static int i915_gem_request_info(struct seq_file *m, void *data)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	struct drm_i915_gem_request *req;
-	int ret, any, i;
+	int ret, any;
 
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		return ret;
 
 	any = 0;
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		int count;
 
 		count = 0;
@@ -739,14 +739,14 @@ static int i915_gem_seqno_info(struct seq_file *m, void *data)
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int ret, i;
+	int ret;
 
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		return ret;
 	intel_runtime_pm_get(dev_priv);
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		i915_ring_seqno_info(m, engine);
 
 	intel_runtime_pm_put(dev_priv);
@@ -933,7 +933,7 @@ static int i915_interrupt_info(struct seq_file *m, void *data)
 		seq_printf(m, "Graphics Interrupt mask:		%08x\n",
 			   I915_READ(GTIMR));
 	}
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		if (INTEL_INFO(dev)->gen >= 6) {
 			seq_printf(m,
 				   "Graphics Interrupt mask (%s):	%08x\n",
@@ -2044,7 +2044,7 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	struct intel_context *ctx;
-	int ret, i;
+	int ret;
 
 	if (!i915.enable_execlists) {
 		seq_printf(m, "Logical Ring Contexts are disabled\n");
@@ -2057,7 +2057,7 @@ static int i915_dump_lrc(struct seq_file *m, void *unused)
 
 	list_for_each_entry(ctx, &dev_priv->context_list, link)
 		if (ctx != dev_priv->kernel_context)
-			for_each_engine(engine, dev_priv, i)
+			for_each_engine(engine, dev_priv)
 				i915_dump_lrc_obj(m, ctx, engine);
 
 	mutex_unlock(&dev->struct_mutex);
@@ -2077,8 +2077,7 @@ static int i915_execlists(struct seq_file *m, void *data)
 	u32 status;
 	u32 ctx_id;
 	struct list_head *cursor;
-	int ring_id, i;
-	int ret;
+	int i, ret;
 
 	if (!i915.enable_execlists) {
 		seq_puts(m, "Logical Ring Contexts are disabled\n");
@@ -2091,7 +2090,7 @@ static int i915_execlists(struct seq_file *m, void *data)
 
 	intel_runtime_pm_get(dev_priv);
 
-	for_each_engine(engine, dev_priv, ring_id) {
+	for_each_engine(engine, dev_priv) {
 		struct drm_i915_gem_request *head_req = NULL;
 		int count = 0;
 		unsigned long flags;
@@ -2250,12 +2249,12 @@ static void gen8_ppgtt_info(struct seq_file *m, struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	struct i915_hw_ppgtt *ppgtt = dev_priv->mm.aliasing_ppgtt;
-	int unused, i;
+	int i;
 
 	if (!ppgtt)
 		return;
 
-	for_each_engine(engine, dev_priv, unused) {
+	for_each_engine(engine, dev_priv) {
 		seq_printf(m, "%s\n", engine->name);
 		for (i = 0; i < 4; i++) {
 			u64 pdp = I915_READ(GEN8_RING_PDP_UDW(engine, i));
@@ -2270,12 +2269,11 @@ static void gen6_ppgtt_info(struct seq_file *m, struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int i;
 
 	if (INTEL_INFO(dev)->gen == 6)
 		seq_printf(m, "GFX_MODE: 0x%08x\n", I915_READ(GFX_MODE));
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		seq_printf(m, "%s\n", engine->name);
 		if (INTEL_INFO(dev)->gen == 7)
 			seq_printf(m, "GFX_MODE: 0x%08x\n",
@@ -2342,9 +2340,8 @@ static int count_irq_waiters(struct drm_i915_private *i915)
 {
 	struct intel_engine_cs *engine;
 	int count = 0;
-	int i;
 
-	for_each_engine(engine, i915, i)
+	for_each_engine(engine, i915)
 		count += engine->irq_refcount;
 
 	return count;
@@ -2455,7 +2452,6 @@ static void i915_guc_client_info(struct seq_file *m,
 {
 	struct intel_engine_cs *engine;
 	uint64_t tot = 0;
-	uint32_t i;
 
 	seq_printf(m, "\tPriority %d, GuC ctx index: %u, PD offset 0x%x\n",
 		client->priority, client->ctx_index, client->proc_desc_offset);
@@ -2468,7 +2464,7 @@ static void i915_guc_client_info(struct seq_file *m,
 	seq_printf(m, "\tFailed doorbell: %u\n", client->b_fail);
 	seq_printf(m, "\tLast submission result: %d\n", client->retcode);
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		seq_printf(m, "\tSubmissions: %llu %s\n",
 				client->submissions[engine->guc_id],
 				engine->name);
@@ -2485,7 +2481,6 @@ static int i915_guc_info(struct seq_file *m, void *data)
 	struct intel_guc guc;
 	struct i915_guc_client client = {};
 	struct intel_engine_cs *engine;
-	enum intel_engine_id i;
 	u64 total = 0;
 
 	if (!HAS_GUC_SCHED(dev_priv->dev))
@@ -2508,7 +2503,7 @@ static int i915_guc_info(struct seq_file *m, void *data)
 	seq_printf(m, "GuC last action error code: %d\n", guc.action_err);
 
 	seq_printf(m, "\nGuC submissions:\n");
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		seq_printf(m, "\t%-24s: %10llu, last seqno 0x%08x\n",
 			engine->name, guc.submissions[engine->guc_id],
 			guc.last_seqno[engine->guc_id]);
@@ -3181,7 +3176,7 @@ static int i915_semaphore_status(struct seq_file *m, void *unused)
 		kunmap_atomic(seqno);
 	} else {
 		seq_puts(m, "  Last signal:");
-		for_each_engine(engine, dev_priv, id)
+		for_each_engine(engine, dev_priv)
 			for (j = 0; j < num_rings; j++)
 				seq_printf(m, "0x%08x\n",
 					   I915_READ(engine->semaphore.mbox.signal[j]));
@@ -3189,11 +3184,10 @@ static int i915_semaphore_status(struct seq_file *m, void *unused)
 	}
 
 	seq_puts(m, "\nSync seqno:\n");
-	for_each_engine(engine, dev_priv, id) {
-		for (j = 0; j < num_rings; j++) {
+	for_each_engine(engine, dev_priv) {
+		for (j = 0; j < num_rings; j++)
 			seq_printf(m, "  0x%08x ",
 				   engine->semaphore.sync_seqno[j]);
-		}
 		seq_putc(m, '\n');
 	}
 	seq_putc(m, '\n');
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 62ee86c..e462683 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2008,10 +2008,12 @@ 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_engine(ring__, dev_priv__, i__) \
-	for ((i__) = 0; (i__) < I915_NUM_ENGINES; (i__)++) \
-		for_each_if ((((ring__) = &(dev_priv__)->engine[(i__)]), intel_engine_initialized((ring__))))
+/* Simple iterator over all initialised engines */
+#define for_each_engine(engine__, dev_priv__) \
+	for ((engine__) = &(dev_priv__)->engine[0]; \
+	     (engine__) < &(dev_priv__)->engine[I915_NUM_ENGINES]; \
+	     (engine__)++) \
+		for_each_if (intel_engine_initialized(engine__))
 
 /* Iterator with engine_id */
 #define for_each_engine_id(engine__, dev_priv__, id__) \
@@ -2023,8 +2025,11 @@ static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
 
 /* Iterator over subset of engines selected by mask */
 #define for_each_engine_masked(engine__, dev_priv__, mask__) \
-	for ((engine__) = &dev_priv->engine[0]; (engine__) < &dev_priv->engine[I915_NUM_ENGINES]; (engine__)++) \
-		for_each_if (intel_engine_flag((engine__)) & (mask__) && intel_engine_initialized((engine__)))
+	for ((engine__) = &(dev_priv__)->engine[0]; \
+	     (engine__) < &(dev_priv__)->engine[I915_NUM_ENGINES]; \
+	     (engine__)++) \
+		for_each_if (((mask__) & intel_engine_flag(engine__)) && \
+			     intel_engine_initialized(engine__))
 
 enum hdmi_force_audio {
 	HDMI_AUDIO_OFF_DVI = -2,	/* no aux data for HDMI-DVI converter */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8588c83..c7a997a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2466,10 +2466,10 @@ void i915_vma_move_to_active(struct i915_vma *vma,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int ret, i, j;
+	int ret, j;
 
 	/* Carefully retire all requests without writing to the rings */
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		ret = intel_engine_idle(engine);
 		if (ret)
 			return ret;
@@ -2477,7 +2477,7 @@ void i915_vma_move_to_active(struct i915_vma *vma,
 	i915_gem_retire_requests(dev);
 
 	/* Finally reset hw state */
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		intel_ring_init_seqno(engine, seqno);
 
 		for (j = 0; j < ARRAY_SIZE(engine->semaphore.sync_seqno); j++)
@@ -2884,17 +2884,16 @@ void i915_gem_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int i;
 
 	/*
 	 * Before we free the objects from the requests, we need to inspect
 	 * them for finding the guilty party. As the requests only borrow
 	 * their reference to the objects, the inspection must be done first.
 	 */
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		i915_gem_reset_engine_status(dev_priv, engine);
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		i915_gem_reset_engine_cleanup(dev_priv, engine);
 
 	i915_gem_context_reset(dev);
@@ -2962,9 +2961,8 @@ void i915_gem_reset(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	bool idle = true;
-	int i;
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		i915_gem_retire_requests_ring(engine);
 		idle &= list_empty(&engine->request_list);
 		if (i915.enable_execlists) {
@@ -3009,24 +3007,20 @@ void i915_gem_reset(struct drm_device *dev)
 	struct drm_i915_private *dev_priv =
 		container_of(work, typeof(*dev_priv), mm.idle_work.work);
 	struct drm_device *dev = dev_priv->dev;
-	struct intel_engine_cs *ring;
-	int i;
+	struct intel_engine_cs *engine;
 
-	for_each_engine(ring, dev_priv, i)
-		if (!list_empty(&ring->request_list))
+	for_each_engine(engine, dev_priv)
+		if (!list_empty(&engine->request_list))
 			return;
 
 	/* we probably should sync with hangcheck here, using cancel_work_sync.
-	 * Also locking seems to be fubar here, ring->request_list is protected
+	 * Also locking seems to be fubar here, engine->request_list is protected
 	 * by dev->struct_mutex. */
 
 	intel_mark_idle(dev);
 
 	if (mutex_trylock(&dev->struct_mutex)) {
-		struct intel_engine_cs *engine;
-		int i;
-
-		for_each_engine(engine, dev_priv, i)
+		for_each_engine(engine, dev_priv)
 			i915_gem_batch_pool_fini(&engine->batch_pool);
 
 		mutex_unlock(&dev->struct_mutex);
@@ -3390,10 +3384,10 @@ int i915_gpu_idle(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int ret, i;
+	int ret;
 
 	/* Flush everything onto the inactive list. */
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		if (!i915.enable_execlists) {
 			struct drm_i915_gem_request *req;
 
@@ -4655,9 +4649,8 @@ void i915_gem_vma_destroy(struct i915_vma *vma)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int i;
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		dev_priv->gt.stop_engine(engine);
 }
 
@@ -4828,7 +4821,7 @@ int i915_gem_init_engines(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int ret, i, j;
+	int ret, j;
 
 	if (INTEL_INFO(dev)->gen < 6 && !intel_enable_gtt())
 		return -EIO;
@@ -4874,7 +4867,7 @@ int i915_gem_init_engines(struct drm_device *dev)
 	}
 
 	/* Need to do basic initialisation of all rings first: */
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		ret = engine->init_hw(engine);
 		if (ret)
 			goto out;
@@ -4899,7 +4892,7 @@ int i915_gem_init_engines(struct drm_device *dev)
 		goto out;
 
 	/* Now it is safe to go back round and do everything else: */
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		struct drm_i915_gem_request *req;
 
 		req = i915_gem_request_alloc(engine, NULL);
@@ -4916,7 +4909,8 @@ int i915_gem_init_engines(struct drm_device *dev)
 
 		ret = i915_ppgtt_init_ring(req);
 		if (ret && ret != -EIO) {
-			DRM_ERROR("PPGTT enable ring #%d failed %d\n", i, ret);
+			DRM_ERROR("PPGTT enable %s failed %d\n",
+				  engine->name, ret);
 			i915_gem_request_cancel(req);
 			i915_gem_cleanup_engines(dev);
 			goto out;
@@ -4924,7 +4918,8 @@ int i915_gem_init_engines(struct drm_device *dev)
 
 		ret = i915_gem_context_enable(req);
 		if (ret && ret != -EIO) {
-			DRM_ERROR("Context enable ring #%d failed %d\n", i, ret);
+			DRM_ERROR("Context enable %s failed %d\n",
+				  engine->name, ret);
 			i915_gem_request_cancel(req);
 			i915_gem_cleanup_engines(dev);
 			goto out;
@@ -5005,9 +5000,8 @@ int i915_gem_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int i;
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		dev_priv->gt.cleanup_engine(engine);
 
 	if (i915.enable_execlists)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 394e525..fe580cb 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -517,7 +517,7 @@ struct intel_context *
 		i915_semaphore_is_enabled(engine->dev) ?
 		hweight32(INTEL_INFO(engine->dev)->ring_mask) - 1 :
 		0;
-	int len, i, ret;
+	int len, ret;
 
 	/* w/a: If Flush TLB Invalidation Mode is enabled, driver must do a TLB
 	 * invalidation prior to MI_SET_CONTEXT. On GEN6 we don't set the value
@@ -553,7 +553,7 @@ struct intel_context *
 
 			intel_ring_emit(engine,
 					MI_LOAD_REGISTER_IMM(num_rings));
-			for_each_engine(signaller, to_i915(engine->dev), i) {
+			for_each_engine(signaller, to_i915(engine->dev)) {
 				if (signaller == engine)
 					continue;
 
@@ -582,7 +582,7 @@ struct intel_context *
 
 			intel_ring_emit(engine,
 					MI_LOAD_REGISTER_IMM(num_rings));
-			for_each_engine(signaller, to_i915(engine->dev), i) {
+			for_each_engine(signaller, to_i915(engine->dev)) {
 				if (signaller == engine)
 					continue;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_debug.c b/drivers/gpu/drm/i915/i915_gem_debug.c
index ef9cd70..a565164 100644
--- a/drivers/gpu/drm/i915/i915_gem_debug.c
+++ b/drivers/gpu/drm/i915/i915_gem_debug.c
@@ -38,12 +38,11 @@
 	struct drm_i915_gem_object *obj;
 	struct intel_engine_cs *engine;
 	int err = 0;
-	int i;
 
 	if (warned)
 		return 0;
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		list_for_each_entry(obj, &engine->active_list,
 				    engine_list[engine->id]) {
 			if (obj->base.dev != dev ||
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0715bb7..59e1821 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1737,9 +1737,8 @@ static void gen8_ppgtt_enable(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int j;
 
-	for_each_engine(engine, dev_priv, j) {
+	for_each_engine(engine, dev_priv) {
 		u32 four_level = USES_FULL_48BIT_PPGTT(dev) ? GEN8_GFX_PPGTT_48B : 0;
 		I915_WRITE(RING_MODE_GEN7(engine),
 			   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE | four_level));
@@ -1751,7 +1750,6 @@ static void gen7_ppgtt_enable(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	uint32_t ecochk, ecobits;
-	int i;
 
 	ecobits = I915_READ(GAC_ECO_BITS);
 	I915_WRITE(GAC_ECO_BITS, ecobits | ECOBITS_PPGTT_CACHE64B);
@@ -1765,7 +1763,7 @@ static void gen7_ppgtt_enable(struct drm_device *dev)
 	}
 	I915_WRITE(GAM_ECOCHK, ecochk);
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		/* GFX_MODE is per-ring on gen7+ */
 		I915_WRITE(RING_MODE_GEN7(engine),
 			   _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE));
@@ -2287,12 +2285,11 @@ void i915_check_and_clear_faults(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int i;
 
 	if (INTEL_INFO(dev)->gen < 6)
 		return;
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		u32 fault_reg;
 		fault_reg = I915_READ(RING_FAULT_REG(engine));
 		if (fault_reg & RING_FAULT_VALID) {
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 0611bdc..da86bdb 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -842,7 +842,7 @@ static void guc_create_ads(struct intel_guc *guc)
 	struct guc_mmio_reg_state *reg_state;
 	struct intel_engine_cs *engine;
 	struct page *page;
-	u32 size, i;
+	u32 size;
 
 	/* The ads obj includes the struct itself and buffers passed to GuC */
 	size = sizeof(struct guc_ads) + sizeof(struct guc_policies) +
@@ -871,7 +871,7 @@ static void guc_create_ads(struct intel_guc *guc)
 	engine = &dev_priv->engine[RCS];
 	ads->golden_context_lrca = engine->status_page.gfx_addr;
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		ads->eng_state_size[engine->guc_id] = intel_lr_context_size(engine);
 
 	/* GuC scheduling policies */
@@ -884,7 +884,7 @@ static void guc_create_ads(struct intel_guc *guc)
 	/* MMIO reg state */
 	reg_state = (void *)policies + sizeof(struct guc_policies);
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		reg_state->mmio_white_list[engine->guc_id].mmio_start =
 			engine->mmio_base + GUC_MMIO_WHITE_LIST_START;
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 14a23b3..5aa4239 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1080,9 +1080,8 @@ static u32 vlv_wa_c0_ei(struct drm_i915_private *dev_priv, u32 pm_iir)
 static bool any_waiters(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
-	int i;
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		if (engine->irq_refcount)
 			return true;
 
@@ -2450,7 +2449,6 @@ static void i915_error_wake_up(struct drm_i915_private *dev_priv,
 			       bool reset_completed)
 {
 	struct intel_engine_cs *engine;
-	int i;
 
 	/*
 	 * Notify all waiters for GPU completion events that reset state has
@@ -2460,7 +2458,7 @@ static void i915_error_wake_up(struct drm_i915_private *dev_priv,
 	 */
 
 	/* Wake up __wait_seqno, potentially holding dev->struct_mutex. */
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		wake_up_all(&engine->irq_queue);
 
 	/* Wake up intel_crtc_wait_for_pending_flips, holding crtc->mutex. */
@@ -2829,10 +2827,9 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = engine->dev->dev_private;
 	struct intel_engine_cs *signaller;
-	int i;
 
 	if (INTEL_INFO(dev_priv->dev)->gen >= 8) {
-		for_each_engine(signaller, dev_priv, i) {
+		for_each_engine(signaller, dev_priv) {
 			if (engine == signaller)
 				continue;
 
@@ -2842,7 +2839,7 @@ static void gen8_disable_vblank(struct drm_device *dev, unsigned int pipe)
 	} else {
 		u32 sync_bits = ipehr & MI_SEMAPHORE_SYNC_MASK;
 
-		for_each_engine(signaller, dev_priv, i) {
+		for_each_engine(signaller, dev_priv) {
 			if(engine == signaller)
 				continue;
 
@@ -2958,9 +2955,8 @@ static int semaphore_passed(struct intel_engine_cs *engine)
 static void semaphore_clear_deadlocks(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
-	int i;
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		engine->hangcheck.deadlock = 0;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index e1aff62..b4976f9 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -82,12 +82,12 @@ const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status)
 static void direct_interrupts_to_host(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
-	int i, irqs;
+	int irqs;
 
 	/* tell all command streamers NOT to forward interrupts and vblank to GuC */
 	irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK, GFX_FORWARD_VBLANK_NEVER);
 	irqs |= _MASKED_BIT_DISABLE(GFX_INTERRUPT_STEERING);
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MODE_GEN7(engine), irqs);
 
 	/* route all GT interrupts to the host */
@@ -99,12 +99,12 @@ static void direct_interrupts_to_host(struct drm_i915_private *dev_priv)
 static void direct_interrupts_to_guc(struct drm_i915_private *dev_priv)
 {
 	struct intel_engine_cs *engine;
-	int i, irqs;
+	int irqs;
 
 	/* tell all command streamers to forward interrupts and vblank to GuC */
 	irqs = _MASKED_FIELD(GFX_FORWARD_VBLANK_MASK, GFX_FORWARD_VBLANK_ALWAYS);
 	irqs |= _MASKED_BIT_ENABLE(GFX_INTERRUPT_STEERING);
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MODE_GEN7(engine), irqs);
 
 	/* route USER_INTERRUPT to Host, all others are sent to GuC. */
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 40ef4ea..5d4ca3b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2673,9 +2673,8 @@ void intel_lr_context_reset(struct drm_device *dev,
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
-	int i;
 
-	for_each_engine(engine, dev_priv, i) {
+	for_each_engine(engine, dev_priv) {
 		struct drm_i915_gem_object *ctx_obj =
 				ctx->engine[engine->id].state;
 		struct intel_ringbuffer *ringbuf =
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 521cf45..6a04761 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4817,7 +4817,6 @@ static void gen9_enable_rc6(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	uint32_t rc6_mask = 0;
-	int unused;
 
 	/* 1a: Software RC state - RC0 */
 	I915_WRITE(GEN6_RC_STATE, 0);
@@ -4838,7 +4837,7 @@ static void gen9_enable_rc6(struct drm_device *dev)
 		I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 54 << 16);
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
 	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */
-	for_each_engine(engine, dev_priv, unused)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 
 	if (HAS_GUC_UCODE(dev))
@@ -4887,7 +4886,6 @@ static void gen8_enable_rps(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	uint32_t rc6_mask = 0;
-	int unused;
 
 	/* 1a: Software RC state - RC0 */
 	I915_WRITE(GEN6_RC_STATE, 0);
@@ -4906,7 +4904,7 @@ static void gen8_enable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
 	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */
-	for_each_engine(engine, dev_priv, unused)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 	I915_WRITE(GEN6_RC_SLEEP, 0);
 	if (IS_BROADWELL(dev))
@@ -4971,7 +4969,7 @@ static void gen6_enable_rps(struct drm_device *dev)
 	u32 rc6vids, pcu_mbox = 0, rc6_mask = 0;
 	u32 gtfifodbg;
 	int rc6_mode;
-	int i, ret;
+	int ret;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
@@ -5003,7 +5001,7 @@ static void gen6_enable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
 	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25);
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 
 	I915_WRITE(GEN6_RC_SLEEP, 0);
@@ -5497,7 +5495,6 @@ static void cherryview_enable_rps(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	u32 gtfifodbg, val, rc6_mode = 0, pcbr;
-	int i;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
@@ -5522,7 +5519,7 @@ static void cherryview_enable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
 	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25); /* 25 * 1280ns */
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 	I915_WRITE(GEN6_RC_SLEEP, 0);
 
@@ -5595,7 +5592,6 @@ static void valleyview_enable_rps(struct drm_device *dev)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_engine_cs *engine;
 	u32 gtfifodbg, val, rc6_mode = 0;
-	int i;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
@@ -5633,7 +5629,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
 	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000);
 	I915_WRITE(GEN6_RC_IDLE_HYSTERSIS, 25);
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		I915_WRITE(RING_MAX_IDLE(engine->mmio_base), 10);
 
 	I915_WRITE(GEN6_RC6_THRESHOLD, 0x557);
@@ -6012,14 +6008,13 @@ bool i915_gpu_busy(void)
 	struct drm_i915_private *dev_priv;
 	struct intel_engine_cs *engine;
 	bool ret = false;
-	int i;
 
 	spin_lock_irq(&mchdev_lock);
 	if (!i915_mch_dev)
 		goto out_unlock;
 	dev_priv = i915_mch_dev;
 
-	for_each_engine(engine, dev_priv, i)
+	for_each_engine(engine, dev_priv)
 		ret |= !list_empty(&engine->request_list);
 
 out_unlock:
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for drm/i915: fix potential dangling else problems in for_each_ macros (rev3)

[Dave Gordon]

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()
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: fix potential dangling else problems in for_each_ macros (rev4)

[Patchwork]

== 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()

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for drm/i915: fix potential dangling else problems in for_each_ macros (rev4)

[Tvrtko Ursulin]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx