Quoting Jani Nikula (2023-09-28 04:43:13-03:00)
>On Thu, 28 Sep 2023, Jouni Högander <jouni.hogander@xxxxxxxxx> 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@xxxxxxxxx>
>>
>> Signed-off-by: Jouni Högander <jouni.hogander@xxxxxxxxx>
>> ---
>> 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
--
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
