Re: [RFC] drm/i915: GEM_WARN_ON considered harmful

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

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

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