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

[Daniel Vetter]

On Mon, Dec 8, 2014 at 4:14 PM, Damien Lespiau <damien.lespiau at 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 at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Signed-off-by: Daniel Vetter <daniel.vetter at 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 at 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