| Message ID | 20181012063142.16080-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915: GEM_WARN_ON considered harmful | expand |
On 2018-10-12 08:31, 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. > > v2: > * Dropped BUG_ON hunk. I have to agree with the need to "fix" GEM_WARN_ON() - ran into not realizing the difference to WARN_ON() myself. And while nobody likes multiplying variants of macros, I can't disagree with the need of having DEBUG-only version as well. Reviewed-by: Tomasz Lis <tomasz.lis@intel.com> > 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> > Cc: Tomasz Lis <tomasz.lis@intel.com> > --- > 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 | 6 +++--- > drivers/gpu/drm/i915/intel_workarounds.c | 2 +- > 5 files changed, 15 insertions(+), 13 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 f27dbe26bcc1..78e42c0825d2 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]); > @@ -402,7 +402,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 ff0e2b36cb8b..22b57b8926fc 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1515,7 +1515,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)) { > @@ -1554,8 +1554,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; > } > diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c > index e4136590fed9..01b9b7591c5d 100644 > --- a/drivers/gpu/drm/i915/intel_workarounds.c > +++ b/drivers/gpu/drm/i915/intel_workarounds.c > @@ -948,7 +948,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 12/10/2018 08:16, Patchwork wrote: > == Series Details == > > Series: drm/i915: GEM_WARN_ON considered harmful (rev2) > URL : https://patchwork.freedesktop.org/series/49340/ > State : success > > == Summary == > > = CI Bug Log - changes from CI_DRM_4976 -> Patchwork_10434 = > > == Summary - SUCCESS == > > No regressions found. > > External URL: https://patchwork.freedesktop.org/api/1.0/series/49340/revisions/2/mbox/ > > == Known issues == > > Here are the changes found in Patchwork_10434 that come from known issues: > > === IGT changes === > > ==== Issues hit ==== > > igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a: > fi-byt-clapper: PASS -> FAIL (fdo#103191, fdo#107362) > fi-icl-u: PASS -> INCOMPLETE (fdo#107713) > > > ==== Possible fixes ==== > > igt@kms_flip@basic-flip-vs-modeset: > fi-glk-j4005: DMESG-WARN (fdo#106097) -> PASS > > igt@kms_frontbuffer_tracking@basic: > fi-byt-clapper: FAIL (fdo#103167) -> PASS > > igt@kms_pipe_crc_basic@read-crc-pipe-c: > fi-skl-6700k2: FAIL (fdo#103191, fdo#107362) -> PASS > > igt@pm_rpm@module-reload: > fi-skl-6600u: INCOMPLETE (fdo#107807) -> PASS > fi-glk-j4005: FAIL -> PASS > > > fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167 > fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191 > fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097 > fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362 > fdo#107713 https://bugs.freedesktop.org/show_bug.cgi?id=107713 > fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807 > > > == Participating hosts (45 -> 41) == > > Missing (4): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan > > > == Build changes == > > * Linux: CI_DRM_4976 -> Patchwork_10434 > > CI_DRM_4976: dec9886eff39d38332bb5ea438b9b053d6b2177c @ git://anongit.freedesktop.org/gfx-ci/linux > IGT_4674: 93871c6fb3c25e5d350c9faf36ded917174214de @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools > Patchwork_10434: a5c8a240cd324ed77a36dfced6ac50912c53cc04 @ git://anongit.freedesktop.org/gfx-ci/linux > > > == Linux commits == > > a5c8a240cd32 drm/i915: GEM_WARN_ON considered harmful Pushed, thanks for the comments and review! Regards, Tvrtko
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 f27dbe26bcc1..78e42c0825d2 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]); @@ -402,7 +402,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 ff0e2b36cb8b..22b57b8926fc 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1515,7 +1515,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)) { @@ -1554,8 +1554,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; } diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c index e4136590fed9..01b9b7591c5d 100644 --- a/drivers/gpu/drm/i915/intel_workarounds.c +++ b/drivers/gpu/drm/i915/intel_workarounds.c @@ -948,7 +948,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;