Re: [Intel-gfx] [PATCH v2] drm/i915: Warn on if set frontbuffer return value is not NULL on release

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 28 Sep 2023, Jouni Högander <jouni.hogander@intel.com> wrote:
> i915_gem_object_set_frontbuffer returns set frontbuffer pointer.  When we
> are releasing frontbuffer we are clearing the pointer from the object. Warn
> on if return value is not null.
>
> v2: Instead of ignoring do drm_WARN_ON
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_frontbuffer.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> index d5540c739404..95319301cf2b 100644
> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> @@ -259,7 +259,8 @@ static void frontbuffer_release(struct kref *ref)
>  
>  	i915_ggtt_clear_scanout(obj);
>  
> -	i915_gem_object_set_frontbuffer(obj, NULL);
> +	drm_WARN_ON(&intel_bo_to_i915(obj)->drm,
> +		    i915_gem_object_set_frontbuffer(obj, NULL) != NULL);

I'm in the camp that says drm_WARN_ON() and friends should not contain
any functional actions, and it should just be for checks. I.e. if you
removed all the warns, things would still work, and lines that do stuff
stand out and aren't hidden in WARN parameters.

Like so:

	ret = i915_gem_object_set_frontbuffer(obj, NULL);
	drm_WARN_ON(&intel_bo_to_i915(obj)->drm, ret);

BR,
Jani.

>  	spin_unlock(&intel_bo_to_i915(obj)->display.fb_tracking.lock);
>  
>  	i915_active_fini(&front->write);

-- 
Jani Nikula, Intel