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 Fri, 29 Sep 2023, Gustavo Sousa <gustavo.sousa@intel.com> wrote:
> Quoting Jani Nikula (2023-09-28 04:43:13-03:00)
>>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.
>
> Good rationale.
>
> Here is a command for finding places where a fix might be applicable :-)
>
>     spatch --very-quiet --sp 'drm_WARN_ON(..., <+... E(...) ...+>)' drivers/gpu/drm/i915

Good hints, but also a *lot* of false positives!

BR,
Jani.

>
> --
> Gustavo Sousa
>
>>
>>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

-- 
Jani Nikula, Intel