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

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

[Daniel Vetter]

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);
+}
+
 #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

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

[Damien Lespiau]

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

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

[Jani Nikula]

On Mon, 08 Dec 2014, Daniel Vetter <daniel.vetter@ffwll.ch> 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);

Won't this screw up the __FILE__ and __LINE__ in the backtrace?

BR,
Jani.

> +}
> +
>  #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

-- 
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: [PATCH] drm/i915: Check mask/bit helper functions

[Daniel Vetter]

On Mon, Dec 8, 2014 at 4:14 PM, Damien Lespiau <damien.lespiau@intel.com> wrote:
> 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 added the message for fighting slight discrepancies between a bug
reporters and developers source tree. If you build locally and
actually hit the BUILD_BUG gcc will dump you the entire
preprocessor/macro chain and so precisely tell you where you've
screwed up. So imo no need for this.

> - 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.

When compilation fails it doesn't really matter what we do - you'll
never see that WARN_ON getting executed. And we need to return
something to avoid pissing up gcc early (i.e. before it'll see that
it's dead code), so doing an else WARN(); doesn't actually save any
lines at all.
-Daniel
>
>>  #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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Mon, Dec 8, 2014 at 4:18 PM, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 08 Dec 2014, Daniel Vetter <daniel.vetter@ffwll.ch> 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);
>
> Won't this screw up the __FILE__ and __LINE__ in the backtrace

It will. Back to the drawing board :(
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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.

Spurred by Damien's bugfix which added _MASKED_FIELD.

v2: We use WARN_ON(1) a lot to catch default cases in switch blocks
which should always be extended. So this doesn't work really. Dunno
why gcc only started complaining when I've moved the WARN out of the
static inline helper to address a feedback from Jani.

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_irq.c | 6 ++++++
 1 file changed, 6 insertions(+)

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

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

[Jani Nikula]

On Mon, 08 Dec 2014, Daniel Vetter <daniel.vetter@ffwll.ch> 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.
>
> Spurred by Damien's bugfix which added _MASKED_FIELD.
>
> v2: We use WARN_ON(1) a lot to catch default cases in switch blocks
> which should always be extended. So this doesn't work really. Dunno
> why gcc only started complaining when I've moved the WARN out of the
> static inline helper to address a feedback from Jani.

Ah, that would be precisely because of the static inline helper. The
function parameter is never a builtin constant!

BR,
Jani.


>
> 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_irq.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> 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
>

-- 
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: [PATCH] drm/i915: Check mask/bit helper functions

[Jani Nikula]

On Mon, 08 Dec 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Mon, 08 Dec 2014, Daniel Vetter <daniel.vetter@ffwll.ch> 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.
>>
>> Spurred by Damien's bugfix which added _MASKED_FIELD.
>>
>> v2: We use WARN_ON(1) a lot to catch default cases in switch blocks
>> which should always be extended. So this doesn't work really. Dunno
>> why gcc only started complaining when I've moved the WARN out of the
>> static inline helper to address a feedback from Jani.
>
> Ah, that would be precisely because of the static inline helper. The
> function parameter is never a builtin constant!

What I learned today: This holds for -O0.

>
> BR,
> Jani.
>
>
>>
>> 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_irq.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> 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
>>
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
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: [PATCH] drm/i915: Check mask/bit helper functions

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +1                 364/366              365/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_kms_flip_wf_vblank-ts-check      DMESG_WARN(1, M26)PASS(4, M26M37)      PASS(1, M37)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx