Re: [PATCH v2] drm/i915: GEM_WARN_ON considered harmful

["Lis, Tomasz" <tomasz.lis@xxxxxxxxx>]

On 2018-10-12 08:31, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> 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@xxxxxxxxx>

> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
> Cc: Tomasz Lis <tomasz.lis@xxxxxxxxx>
> ---
>   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;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx