Re: [PATCH] drm/i915: Use BUILD_BUG if possible in the i915 WARN_ON

[Damien Lespiau <damien.lespiau@intel.com>]

On Mon, Dec 08, 2014 at 05:06:34PM +0100, Daniel Vetter wrote:
> Faster feedback to errors is always better. This is inspired by the
> addition to WARN_ONs to mask/enable helpers for registers to make sure
> callers have the arguments ordered correctly: Pretty much always the
> arguments are static.
> 
> We use WARN_ON(1) a lot in default switch statements though where we
> should always handle all cases. So add a new macro specifically for
> that.
> 
> The idea to use __builtin_constant_p is from Chris Wilson.
> 
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h      | 10 +++++++++-
>  drivers/gpu/drm/i915/i915_gem_gtt.c  |  6 +++---
>  drivers/gpu/drm/i915/intel_display.c |  4 ++--
>  drivers/gpu/drm/i915/intel_uncore.c  |  4 ++--
>  5 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index d0e445eca9ce..f44a844a48db 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2347,7 +2347,7 @@ static const char *power_domain_str(enum intel_display_power_domain domain)
>  	case POWER_DOMAIN_INIT:
>  		return "INIT";
>  	default:
> -		WARN_ON(1);
> +		MISSING_CASE();
>  		return "?";
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 95dfa2dd35b9..b2ddc121654d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -57,8 +57,16 @@
>  #define DRIVER_DESC		"Intel Graphics"
>  #define DRIVER_DATE		"20141205"
>  
> +static inline void __i915_warn_on(bool cond)
> +{
> +	if (__builtin_constant_p(cond))
> +		BUILD_BUG_ON(cond);
> +}
> +
>  #undef WARN_ON
> -#define WARN_ON(x)		WARN(x, "WARN_ON(" #x ")")
> +#define WARN_ON(x) (__i915_warn_on((x)), WARN((x), "WARN_ON(" #x ")"))
> +

We're now evaluating x twice.

> +#define MISSING_CASE() WARN(1, "Missing switch case in %s\n", __func__);
>  
>  enum pipe {
>  	INVALID_PIPE = -1,
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index ac03a382000b..3b5807c11427 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -132,7 +132,7 @@ static gen6_gtt_pte_t snb_pte_encode(dma_addr_t addr,
>  		pte |= GEN6_PTE_UNCACHED;
>  		break;
>  	default:
> -		WARN_ON(1);
> +		MISSING_CASE();
>  	}
>  
>  	return pte;
> @@ -156,7 +156,7 @@ static gen6_gtt_pte_t ivb_pte_encode(dma_addr_t addr,
>  		pte |= GEN6_PTE_UNCACHED;
>  		break;
>  	default:
> -		WARN_ON(1);
> +		MISSING_CASE();
>  	}
>  
>  	return pte;
> @@ -1146,7 +1146,7 @@ int i915_ppgtt_init_hw(struct drm_device *dev)
>  	else if (INTEL_INFO(dev)->gen >= 8)
>  		gen8_ppgtt_enable(dev);
>  	else
> -		WARN_ON(1);
> +		MISSING_CASE();
>  
>  	if (ppgtt) {
>  		for_each_ring(ring, dev_priv, i) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d5153a4f90fe..b7155d5efc10 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4847,7 +4847,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
>  		cmd = 0;
>  		break;
>  	default:
> -		WARN_ON(1);
> +		MISSING_CASE();
>  		return;
>  	}
>  
> @@ -8224,7 +8224,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, u32 base)
>  				cntl |= CURSOR_MODE_256_ARGB_AX;
>  				break;
>  			default:
> -				WARN_ON(1);
> +				MISSING_CASE();
>  				return;
>  		}
>  		cntl |= pipe << 28; /* Connect to correct pipe */
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 46de8d75b4bf..83ab530fee06 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -1202,7 +1202,7 @@ void intel_uncore_init(struct drm_device *dev)
>  
>  	switch (INTEL_INFO(dev)->gen) {
>  	default:
> -		WARN_ON(1);
> +		MISSING_CASE();
>  		return;
>  	case 9:
>  		ASSIGN_WRITE_MMIO_VFUNCS(gen9);
> @@ -1300,7 +1300,7 @@ int i915_reg_read_ioctl(struct drm_device *dev,
>  		reg->val = I915_READ8(reg->offset);
>  		break;
>  	default:
> -		WARN_ON(1);
> +		MISSING_CASE();
>  		ret = -EINVAL;
>  		goto out;
>  	}
> -- 
> 2.1.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx