| Message ID | 20180907115351.31644-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/i915: GEM_WARN_ON considered harmful | expand |
Ping! Any comments here? Main goal was to allow GEM_WARN_ON as a statement, plus also protect uses in if statements, which there are some who I think don't expect the branch to completely disappear. Regards, Tvrtko On 07/09/2018 12:53, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > GEM_WARN_ON currently has dangerous semantics where it is completely > compiled out on !GEM_DEBUG builds. This can leave users who expect it to > be more like a WARN_ON, just without a warning in non-debug builds, in > complete ignorance. > > Another gotcha with it is that it cannot be used as a statement. Which is > again different from a standard kernel WARN_ON. > > This patch fixes both problems by making it behave as one would expect. > > It can now be used both as an expression and as statement, and also the > condition evaluates properly in all builds - code under the conditional > will therefore not unexpectedly disappear. > > To satisfy call sites which really want the code under the conditional to > completely disappear, we add GEM_DEBUG_WARN_ON and convert some of the > callers to it. This one can also be used as both expression and statement. > > From the above it follows GEM_DEBUG_WARN_ON should be used in situations > where we are certain the condition will be hit during development, but at > a place in code where error can be handled to the benefit of not crashing > the machine. > > GEM_WARN_ON on the other hand should be used where condition may happen in > production and we just want to distinguish the level of debugging output > emitted between the production and debug build. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > --- > Quickly put together and compile tested only! > --- > drivers/gpu/drm/i915/i915_gem.h | 4 +++- > drivers/gpu/drm/i915/i915_vma.c | 8 ++++---- > drivers/gpu/drm/i915/intel_engine_cs.c | 8 ++++---- > drivers/gpu/drm/i915/intel_lrc.c | 8 ++++---- > drivers/gpu/drm/i915/intel_workarounds.c | 2 +- > 5 files changed, 16 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h > index 599c4f6eb1ea..b0e4b976880c 100644 > --- a/drivers/gpu/drm/i915/i915_gem.h > +++ b/drivers/gpu/drm/i915/i915_gem.h > @@ -47,17 +47,19 @@ struct drm_i915_private; > #define GEM_DEBUG_DECL(var) var > #define GEM_DEBUG_EXEC(expr) expr > #define GEM_DEBUG_BUG_ON(expr) GEM_BUG_ON(expr) > +#define GEM_DEBUG_WARN_ON(expr) GEM_WARN_ON(expr) > > #else > > #define GEM_SHOW_DEBUG() (0) > > #define GEM_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr) > -#define GEM_WARN_ON(expr) (BUILD_BUG_ON_INVALID(expr), 0) > +#define GEM_WARN_ON(expr) ({ unlikely(!!(expr)); }) > > #define GEM_DEBUG_DECL(var) > #define GEM_DEBUG_EXEC(expr) do { } while (0) > #define GEM_DEBUG_BUG_ON(expr) > +#define GEM_DEBUG_WARN_ON(expr) ({ BUILD_BUG_ON_INVALID(expr); 0; }) > #endif > > #if IS_ENABLED(CONFIG_DRM_I915_TRACE_GEM) > diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c > index 31efc971a3a8..82652c3d1bed 100644 > --- a/drivers/gpu/drm/i915/i915_vma.c > +++ b/drivers/gpu/drm/i915/i915_vma.c > @@ -305,12 +305,12 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, > GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); > GEM_BUG_ON(vma->size > vma->node.size); > > - if (GEM_WARN_ON(range_overflows(vma->node.start, > - vma->node.size, > - vma->vm->total))) > + if (GEM_DEBUG_WARN_ON(range_overflows(vma->node.start, > + vma->node.size, > + vma->vm->total))) > return -ENODEV; > > - if (GEM_WARN_ON(!flags)) > + if (GEM_DEBUG_WARN_ON(!flags)) > return -EINVAL; > > bind_flags = 0; > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index 10cd051ba29e..8dbdb18b2668 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -273,13 +273,13 @@ intel_engine_setup(struct drm_i915_private *dev_priv, > BUILD_BUG_ON(MAX_ENGINE_CLASS >= BIT(GEN11_ENGINE_CLASS_WIDTH)); > BUILD_BUG_ON(MAX_ENGINE_INSTANCE >= BIT(GEN11_ENGINE_INSTANCE_WIDTH)); > > - if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS)) > + if (GEM_DEBUG_WARN_ON(info->class > MAX_ENGINE_CLASS)) > return -EINVAL; > > - if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) > + if (GEM_DEBUG_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) > return -EINVAL; > > - if (GEM_WARN_ON(dev_priv->engine_class[info->class][info->instance])) > + if (GEM_DEBUG_WARN_ON(dev_priv->engine_class[info->class][info->instance])) > return -EINVAL; > > GEM_BUG_ON(dev_priv->engine[id]); > @@ -399,7 +399,7 @@ int intel_engines_init(struct drm_i915_private *dev_priv) > err = -EINVAL; > err_id = id; > > - if (GEM_WARN_ON(!init)) > + if (GEM_DEBUG_WARN_ON(!init)) > goto cleanup; > > err = init(engine); > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 19c9c46308e5..a274cc695fa2 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1681,7 +1681,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) > unsigned int i; > int ret; > > - if (GEM_WARN_ON(engine->id != RCS)) > + if (GEM_DEBUG_WARN_ON(engine->id != RCS)) > return -EINVAL; > > switch (INTEL_GEN(engine->i915)) { > @@ -1720,8 +1720,8 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) > */ > for (i = 0; i < ARRAY_SIZE(wa_bb_fn); i++) { > wa_bb[i]->offset = batch_ptr - batch; > - if (GEM_WARN_ON(!IS_ALIGNED(wa_bb[i]->offset, > - CACHELINE_BYTES))) { > + if (GEM_DEBUG_WARN_ON(!IS_ALIGNED(wa_bb[i]->offset, > + CACHELINE_BYTES))) { > ret = -EINVAL; > break; > } > @@ -1730,7 +1730,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) > wa_bb[i]->size = batch_ptr - (batch + wa_bb[i]->offset); > } > > - BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE); > + GEM_BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE); > > kunmap_atomic(batch); > if (ret) > diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c > index 4bcdeaf8d98f..fb746c4b7c38 100644 > --- a/drivers/gpu/drm/i915/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/intel_workarounds.c > @@ -941,7 +941,7 @@ struct whitelist { > > static void whitelist_reg(struct whitelist *w, i915_reg_t reg) > { > - if (GEM_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS)) > + if (GEM_DEBUG_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS)) > return; > > w->reg[w->count++] = reg; >
On Thu, 20 Sep 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > Ping! > > Any comments here? > > Main goal was to allow GEM_WARN_ON as a statement, plus also protect > uses in if statements, which there are some who I think don't expect the > branch to completely disappear. I've said before I don't like the conditional early returns vanishing depending on config options, but I've been shot down. I think this patch is an improvement. BR, Jani. > > Regards, > > Tvrtko > > On 07/09/2018 12:53, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> GEM_WARN_ON currently has dangerous semantics where it is completely >> compiled out on !GEM_DEBUG builds. This can leave users who expect it to >> be more like a WARN_ON, just without a warning in non-debug builds, in >> complete ignorance. >> >> Another gotcha with it is that it cannot be used as a statement. Which is >> again different from a standard kernel WARN_ON. >> >> This patch fixes both problems by making it behave as one would expect. >> >> It can now be used both as an expression and as statement, and also the >> condition evaluates properly in all builds - code under the conditional >> will therefore not unexpectedly disappear. >> >> To satisfy call sites which really want the code under the conditional to >> completely disappear, we add GEM_DEBUG_WARN_ON and convert some of the >> callers to it. This one can also be used as both expression and statement. >> >> From the above it follows GEM_DEBUG_WARN_ON should be used in situations >> where we are certain the condition will be hit during development, but at >> a place in code where error can be handled to the benefit of not crashing >> the machine. >> >> GEM_WARN_ON on the other hand should be used where condition may happen in >> production and we just want to distinguish the level of debugging output >> emitted between the production and debug build. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Matthew Auld <matthew.auld@intel.com> >> Cc: Mika Kuoppala <mika.kuoppala@intel.com> >> --- >> Quickly put together and compile tested only! >> --- >> drivers/gpu/drm/i915/i915_gem.h | 4 +++- >> drivers/gpu/drm/i915/i915_vma.c | 8 ++++---- >> drivers/gpu/drm/i915/intel_engine_cs.c | 8 ++++---- >> drivers/gpu/drm/i915/intel_lrc.c | 8 ++++---- >> drivers/gpu/drm/i915/intel_workarounds.c | 2 +- >> 5 files changed, 16 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h >> index 599c4f6eb1ea..b0e4b976880c 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.h >> +++ b/drivers/gpu/drm/i915/i915_gem.h >> @@ -47,17 +47,19 @@ struct drm_i915_private; >> #define GEM_DEBUG_DECL(var) var >> #define GEM_DEBUG_EXEC(expr) expr >> #define GEM_DEBUG_BUG_ON(expr) GEM_BUG_ON(expr) >> +#define GEM_DEBUG_WARN_ON(expr) GEM_WARN_ON(expr) >> >> #else >> >> #define GEM_SHOW_DEBUG() (0) >> >> #define GEM_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr) >> -#define GEM_WARN_ON(expr) (BUILD_BUG_ON_INVALID(expr), 0) >> +#define GEM_WARN_ON(expr) ({ unlikely(!!(expr)); }) >> >> #define GEM_DEBUG_DECL(var) >> #define GEM_DEBUG_EXEC(expr) do { } while (0) >> #define GEM_DEBUG_BUG_ON(expr) >> +#define GEM_DEBUG_WARN_ON(expr) ({ BUILD_BUG_ON_INVALID(expr); 0; }) >> #endif >> >> #if IS_ENABLED(CONFIG_DRM_I915_TRACE_GEM) >> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c >> index 31efc971a3a8..82652c3d1bed 100644 >> --- a/drivers/gpu/drm/i915/i915_vma.c >> +++ b/drivers/gpu/drm/i915/i915_vma.c >> @@ -305,12 +305,12 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, >> GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); >> GEM_BUG_ON(vma->size > vma->node.size); >> >> - if (GEM_WARN_ON(range_overflows(vma->node.start, >> - vma->node.size, >> - vma->vm->total))) >> + if (GEM_DEBUG_WARN_ON(range_overflows(vma->node.start, >> + vma->node.size, >> + vma->vm->total))) >> return -ENODEV; >> >> - if (GEM_WARN_ON(!flags)) >> + if (GEM_DEBUG_WARN_ON(!flags)) >> return -EINVAL; >> >> bind_flags = 0; >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c >> index 10cd051ba29e..8dbdb18b2668 100644 >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >> @@ -273,13 +273,13 @@ intel_engine_setup(struct drm_i915_private *dev_priv, >> BUILD_BUG_ON(MAX_ENGINE_CLASS >= BIT(GEN11_ENGINE_CLASS_WIDTH)); >> BUILD_BUG_ON(MAX_ENGINE_INSTANCE >= BIT(GEN11_ENGINE_INSTANCE_WIDTH)); >> >> - if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS)) >> + if (GEM_DEBUG_WARN_ON(info->class > MAX_ENGINE_CLASS)) >> return -EINVAL; >> >> - if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) >> + if (GEM_DEBUG_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) >> return -EINVAL; >> >> - if (GEM_WARN_ON(dev_priv->engine_class[info->class][info->instance])) >> + if (GEM_DEBUG_WARN_ON(dev_priv->engine_class[info->class][info->instance])) >> return -EINVAL; >> >> GEM_BUG_ON(dev_priv->engine[id]); >> @@ -399,7 +399,7 @@ int intel_engines_init(struct drm_i915_private *dev_priv) >> err = -EINVAL; >> err_id = id; >> >> - if (GEM_WARN_ON(!init)) >> + if (GEM_DEBUG_WARN_ON(!init)) >> goto cleanup; >> >> err = init(engine); >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >> index 19c9c46308e5..a274cc695fa2 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -1681,7 +1681,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) >> unsigned int i; >> int ret; >> >> - if (GEM_WARN_ON(engine->id != RCS)) >> + if (GEM_DEBUG_WARN_ON(engine->id != RCS)) >> return -EINVAL; >> >> switch (INTEL_GEN(engine->i915)) { >> @@ -1720,8 +1720,8 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) >> */ >> for (i = 0; i < ARRAY_SIZE(wa_bb_fn); i++) { >> wa_bb[i]->offset = batch_ptr - batch; >> - if (GEM_WARN_ON(!IS_ALIGNED(wa_bb[i]->offset, >> - CACHELINE_BYTES))) { >> + if (GEM_DEBUG_WARN_ON(!IS_ALIGNED(wa_bb[i]->offset, >> + CACHELINE_BYTES))) { >> ret = -EINVAL; >> break; >> } >> @@ -1730,7 +1730,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) >> wa_bb[i]->size = batch_ptr - (batch + wa_bb[i]->offset); >> } >> >> - BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE); >> + GEM_BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE); >> >> kunmap_atomic(batch); >> if (ret) >> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c >> index 4bcdeaf8d98f..fb746c4b7c38 100644 >> --- a/drivers/gpu/drm/i915/intel_workarounds.c >> +++ b/drivers/gpu/drm/i915/intel_workarounds.c >> @@ -941,7 +941,7 @@ struct whitelist { >> >> static void whitelist_reg(struct whitelist *w, i915_reg_t reg) >> { >> - if (GEM_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS)) >> + if (GEM_DEBUG_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS)) >> return; >> >> w->reg[w->count++] = reg; >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
So I understand we agree on the change, just waiting for non-RFC version? -Tomasz On 2018-09-24 11:34, Jani Nikula wrote: > On Thu, 20 Sep 2018, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >> Ping! >> >> Any comments here? >> >> Main goal was to allow GEM_WARN_ON as a statement, plus also protect >> uses in if statements, which there are some who I think don't expect the >> branch to completely disappear. > I've said before I don't like the conditional early returns vanishing > depending on config options, but I've been shot down. I think this patch > is an improvement. > > > BR, > Jani. > > >> Regards, >> >> Tvrtko >> >> On 07/09/2018 12:53, Tvrtko Ursulin wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> GEM_WARN_ON currently has dangerous semantics where it is completely >>> compiled out on !GEM_DEBUG builds. This can leave users who expect it to >>> be more like a WARN_ON, just without a warning in non-debug builds, in >>> complete ignorance. >>> >>> Another gotcha with it is that it cannot be used as a statement. Which is >>> again different from a standard kernel WARN_ON. >>> >>> This patch fixes both problems by making it behave as one would expect. >>> >>> It can now be used both as an expression and as statement, and also the >>> condition evaluates properly in all builds - code under the conditional >>> will therefore not unexpectedly disappear. >>> >>> To satisfy call sites which really want the code under the conditional to >>> completely disappear, we add GEM_DEBUG_WARN_ON and convert some of the >>> callers to it. This one can also be used as both expression and statement. >>> >>> From the above it follows GEM_DEBUG_WARN_ON should be used in situations >>> where we are certain the condition will be hit during development, but at >>> a place in code where error can be handled to the benefit of not crashing >>> the machine. >>> >>> GEM_WARN_ON on the other hand should be used where condition may happen in >>> production and we just want to distinguish the level of debugging output >>> emitted between the production and debug build. >>> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >>> Cc: Matthew Auld <matthew.auld@intel.com> >>> Cc: Mika Kuoppala <mika.kuoppala@intel.com> >>> --- >>> Quickly put together and compile tested only! >>> --- >>> drivers/gpu/drm/i915/i915_gem.h | 4 +++- >>> drivers/gpu/drm/i915/i915_vma.c | 8 ++++---- >>> drivers/gpu/drm/i915/intel_engine_cs.c | 8 ++++---- >>> drivers/gpu/drm/i915/intel_lrc.c | 8 ++++---- >>> drivers/gpu/drm/i915/intel_workarounds.c | 2 +- >>> 5 files changed, 16 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h >>> index 599c4f6eb1ea..b0e4b976880c 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem.h >>> +++ b/drivers/gpu/drm/i915/i915_gem.h >>> @@ -47,17 +47,19 @@ struct drm_i915_private; >>> #define GEM_DEBUG_DECL(var) var >>> #define GEM_DEBUG_EXEC(expr) expr >>> #define GEM_DEBUG_BUG_ON(expr) GEM_BUG_ON(expr) >>> +#define GEM_DEBUG_WARN_ON(expr) GEM_WARN_ON(expr) >>> >>> #else >>> >>> #define GEM_SHOW_DEBUG() (0) >>> >>> #define GEM_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr) >>> -#define GEM_WARN_ON(expr) (BUILD_BUG_ON_INVALID(expr), 0) >>> +#define GEM_WARN_ON(expr) ({ unlikely(!!(expr)); }) >>> >>> #define GEM_DEBUG_DECL(var) >>> #define GEM_DEBUG_EXEC(expr) do { } while (0) >>> #define GEM_DEBUG_BUG_ON(expr) >>> +#define GEM_DEBUG_WARN_ON(expr) ({ BUILD_BUG_ON_INVALID(expr); 0; }) >>> #endif >>> >>> #if IS_ENABLED(CONFIG_DRM_I915_TRACE_GEM) >>> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c >>> index 31efc971a3a8..82652c3d1bed 100644 >>> --- a/drivers/gpu/drm/i915/i915_vma.c >>> +++ b/drivers/gpu/drm/i915/i915_vma.c >>> @@ -305,12 +305,12 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, >>> GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); >>> GEM_BUG_ON(vma->size > vma->node.size); >>> >>> - if (GEM_WARN_ON(range_overflows(vma->node.start, >>> - vma->node.size, >>> - vma->vm->total))) >>> + if (GEM_DEBUG_WARN_ON(range_overflows(vma->node.start, >>> + vma->node.size, >>> + vma->vm->total))) >>> return -ENODEV; >>> >>> - if (GEM_WARN_ON(!flags)) >>> + if (GEM_DEBUG_WARN_ON(!flags)) >>> return -EINVAL; >>> >>> bind_flags = 0; >>> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c >>> index 10cd051ba29e..8dbdb18b2668 100644 >>> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >>> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >>> @@ -273,13 +273,13 @@ intel_engine_setup(struct drm_i915_private *dev_priv, >>> BUILD_BUG_ON(MAX_ENGINE_CLASS >= BIT(GEN11_ENGINE_CLASS_WIDTH)); >>> BUILD_BUG_ON(MAX_ENGINE_INSTANCE >= BIT(GEN11_ENGINE_INSTANCE_WIDTH)); >>> >>> - if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS)) >>> + if (GEM_DEBUG_WARN_ON(info->class > MAX_ENGINE_CLASS)) >>> return -EINVAL; >>> >>> - if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) >>> + if (GEM_DEBUG_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) >>> return -EINVAL; >>> >>> - if (GEM_WARN_ON(dev_priv->engine_class[info->class][info->instance])) >>> + if (GEM_DEBUG_WARN_ON(dev_priv->engine_class[info->class][info->instance])) >>> return -EINVAL; >>> >>> GEM_BUG_ON(dev_priv->engine[id]); >>> @@ -399,7 +399,7 @@ int intel_engines_init(struct drm_i915_private *dev_priv) >>> err = -EINVAL; >>> err_id = id; >>> >>> - if (GEM_WARN_ON(!init)) >>> + if (GEM_DEBUG_WARN_ON(!init)) >>> goto cleanup; >>> >>> err = init(engine); >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >>> index 19c9c46308e5..a274cc695fa2 100644 >>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>> @@ -1681,7 +1681,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) >>> unsigned int i; >>> int ret; >>> >>> - if (GEM_WARN_ON(engine->id != RCS)) >>> + if (GEM_DEBUG_WARN_ON(engine->id != RCS)) >>> return -EINVAL; >>> >>> switch (INTEL_GEN(engine->i915)) { >>> @@ -1720,8 +1720,8 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) >>> */ >>> for (i = 0; i < ARRAY_SIZE(wa_bb_fn); i++) { >>> wa_bb[i]->offset = batch_ptr - batch; >>> - if (GEM_WARN_ON(!IS_ALIGNED(wa_bb[i]->offset, >>> - CACHELINE_BYTES))) { >>> + if (GEM_DEBUG_WARN_ON(!IS_ALIGNED(wa_bb[i]->offset, >>> + CACHELINE_BYTES))) { >>> ret = -EINVAL; >>> break; >>> } >>> @@ -1730,7 +1730,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) >>> wa_bb[i]->size = batch_ptr - (batch + wa_bb[i]->offset); >>> } >>> >>> - BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE); >>> + GEM_BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE); >>> >>> kunmap_atomic(batch); >>> if (ret) >>> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c >>> index 4bcdeaf8d98f..fb746c4b7c38 100644 >>> --- a/drivers/gpu/drm/i915/intel_workarounds.c >>> +++ b/drivers/gpu/drm/i915/intel_workarounds.c >>> @@ -941,7 +941,7 @@ struct whitelist { >>> >>> static void whitelist_reg(struct whitelist *w, i915_reg_t reg) >>> { >>> - if (GEM_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS)) >>> + if (GEM_DEBUG_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS)) >>> return; >>> >>> w->reg[w->count++] = reg; >>> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h index 599c4f6eb1ea..b0e4b976880c 100644 --- a/drivers/gpu/drm/i915/i915_gem.h +++ b/drivers/gpu/drm/i915/i915_gem.h @@ -47,17 +47,19 @@ struct drm_i915_private; #define GEM_DEBUG_DECL(var) var #define GEM_DEBUG_EXEC(expr) expr #define GEM_DEBUG_BUG_ON(expr) GEM_BUG_ON(expr) +#define GEM_DEBUG_WARN_ON(expr) GEM_WARN_ON(expr) #else #define GEM_SHOW_DEBUG() (0) #define GEM_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr) -#define GEM_WARN_ON(expr) (BUILD_BUG_ON_INVALID(expr), 0) +#define GEM_WARN_ON(expr) ({ unlikely(!!(expr)); }) #define GEM_DEBUG_DECL(var) #define GEM_DEBUG_EXEC(expr) do { } while (0) #define GEM_DEBUG_BUG_ON(expr) +#define GEM_DEBUG_WARN_ON(expr) ({ BUILD_BUG_ON_INVALID(expr); 0; }) #endif #if IS_ENABLED(CONFIG_DRM_I915_TRACE_GEM) diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index 31efc971a3a8..82652c3d1bed 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -305,12 +305,12 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, GEM_BUG_ON(!drm_mm_node_allocated(&vma->node)); GEM_BUG_ON(vma->size > vma->node.size); - if (GEM_WARN_ON(range_overflows(vma->node.start, - vma->node.size, - vma->vm->total))) + if (GEM_DEBUG_WARN_ON(range_overflows(vma->node.start, + vma->node.size, + vma->vm->total))) return -ENODEV; - if (GEM_WARN_ON(!flags)) + if (GEM_DEBUG_WARN_ON(!flags)) return -EINVAL; bind_flags = 0; diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 10cd051ba29e..8dbdb18b2668 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -273,13 +273,13 @@ intel_engine_setup(struct drm_i915_private *dev_priv, BUILD_BUG_ON(MAX_ENGINE_CLASS >= BIT(GEN11_ENGINE_CLASS_WIDTH)); BUILD_BUG_ON(MAX_ENGINE_INSTANCE >= BIT(GEN11_ENGINE_INSTANCE_WIDTH)); - if (GEM_WARN_ON(info->class > MAX_ENGINE_CLASS)) + if (GEM_DEBUG_WARN_ON(info->class > MAX_ENGINE_CLASS)) return -EINVAL; - if (GEM_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) + if (GEM_DEBUG_WARN_ON(info->instance > MAX_ENGINE_INSTANCE)) return -EINVAL; - if (GEM_WARN_ON(dev_priv->engine_class[info->class][info->instance])) + if (GEM_DEBUG_WARN_ON(dev_priv->engine_class[info->class][info->instance])) return -EINVAL; GEM_BUG_ON(dev_priv->engine[id]); @@ -399,7 +399,7 @@ int intel_engines_init(struct drm_i915_private *dev_priv) err = -EINVAL; err_id = id; - if (GEM_WARN_ON(!init)) + if (GEM_DEBUG_WARN_ON(!init)) goto cleanup; err = init(engine); diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 19c9c46308e5..a274cc695fa2 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1681,7 +1681,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) unsigned int i; int ret; - if (GEM_WARN_ON(engine->id != RCS)) + if (GEM_DEBUG_WARN_ON(engine->id != RCS)) return -EINVAL; switch (INTEL_GEN(engine->i915)) { @@ -1720,8 +1720,8 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) */ for (i = 0; i < ARRAY_SIZE(wa_bb_fn); i++) { wa_bb[i]->offset = batch_ptr - batch; - if (GEM_WARN_ON(!IS_ALIGNED(wa_bb[i]->offset, - CACHELINE_BYTES))) { + if (GEM_DEBUG_WARN_ON(!IS_ALIGNED(wa_bb[i]->offset, + CACHELINE_BYTES))) { ret = -EINVAL; break; } @@ -1730,7 +1730,7 @@ static int intel_init_workaround_bb(struct intel_engine_cs *engine) wa_bb[i]->size = batch_ptr - (batch + wa_bb[i]->offset); } - BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE); + GEM_BUG_ON(batch_ptr - batch > CTX_WA_BB_OBJ_SIZE); kunmap_atomic(batch); if (ret) diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c index 4bcdeaf8d98f..fb746c4b7c38 100644 --- a/drivers/gpu/drm/i915/intel_workarounds.c +++ b/drivers/gpu/drm/i915/intel_workarounds.c @@ -941,7 +941,7 @@ struct whitelist { static void whitelist_reg(struct whitelist *w, i915_reg_t reg) { - if (GEM_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS)) + if (GEM_DEBUG_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS)) return; w->reg[w->count++] = reg;