Re: [PATCH] drm/i915: Check mask/bit helper functions

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

On Mon, Dec 08, 2014 at 04:00:29PM +0100, Daniel Vetter wrote:
> After a bit of irc discussion we've concluded that it would be prudent
> to check that callers use the mask/enable paramters correctly. So add
> a WARN_ON.
> 
> Now most callers have static parameters, so even better would be if we
> could bug at compile-time. Hence improve the i915 WARN_ON to
> BUILD_BUG_ON if the condition can be statically determined. Thanks to
> Chris for this suggestion.
> 
> All this spurred by Damien's bugfix which added _MASKED_FIELD.
> 
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 9 ++++++++-
>  drivers/gpu/drm/i915/i915_irq.c | 6 ++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 95dfa2dd35b9..e5d9d6642b09 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -57,8 +57,15 @@
>  #define DRIVER_DESC		"Intel Graphics"
>  #define DRIVER_DATE		"20141205"
>  
> +static inline bool __i915_warn_on(bool cond, const char *str)
> +{
> +	if (__builtin_constant_p(cond))
> +		BUILD_BUG_ON(cond);
> +	return WARN(cond, str);
> +}

- Can we have BUILD_BUG_ON_MSG()?

- We could avoid emitting the WARN() part if __builtin_constant_p() is
  true, we don't really need to run-time code in there in that case.

>  #undef WARN_ON
> -#define WARN_ON(x)		WARN(x, "WARN_ON(" #x ")")
> +#define WARN_ON(x) __i915_warn_on((x), "WARN_ON(" #x ")")
>  
>  enum pipe {
>  	INVALID_PIPE = -1,
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 08a5a4ba52ac..e6a1db36928e 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -183,6 +183,8 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
>  {
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
> +	WARN_ON(enabled_irq_mask & ~interrupt_mask);
> +
>  	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
>  		return;
>  
> @@ -229,6 +231,8 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>  {
>  	uint32_t new_val;
>  
> +	WARN_ON(enabled_irq_mask & ~interrupt_mask);
> +
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
>  	new_val = dev_priv->pm_irq_mask;
> @@ -328,6 +332,8 @@ void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
>  	sdeimr &= ~interrupt_mask;
>  	sdeimr |= (~enabled_irq_mask & interrupt_mask);
>  
> +	WARN_ON(enabled_irq_mask & ~interrupt_mask);
> +
>  	assert_spin_locked(&dev_priv->irq_lock);
>  
>  	if (WARN_ON(!intel_irqs_enabled(dev_priv)))
> -- 
> 2.1.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx