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

[Gustavo Sousa]

Quoting Jani Nikula (2023-10-02 04:08:40-03:00)
>On Fri, 29 Sep 2023, Gustavo Sousa <gustavo.sousa at intel.com> wrote:
>> Quoting Jani Nikula (2023-09-28 04:43:13-03:00)
>>>On Thu, 28 Sep 2023, Jouni Högander <jouni.hogander at 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 at intel.com>
>>>>
>>>> Signed-off-by: Jouni Högander <jouni.hogander at 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!

Yep... Not sure, but maybe this could be improved to somehow try to
capture only functions that have some side-effect (e.g., register writes
or modification to driver data).

--
Gustavo Sousa

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