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

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

[Jouni Högander]

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);
 	spin_unlock(&intel_bo_to_i915(obj)->display.fb_tracking.lock);
 
 	i915_active_fini(&front->write);
-- 
2.34.1

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Warn on if set frontbuffer return value is not NULL on release

[Patchwork]

== Series Details ==

Series: drm/i915: Warn on if set frontbuffer return value is not NULL on release
URL   : https://patchwork.freedesktop.org/series/124375/
State : warning

== Summary ==

Error: dim checkpatch failed
01ae0b3d36e2 drm/i915: Warn on if set frontbuffer return value is not NULL on release
-:30: CHECK:COMPARISON_TO_NULL: Comparison to NULL could be written "i915_gem_object_set_frontbuffer"
#30: FILE: drivers/gpu/drm/i915/display/intel_frontbuffer.c:263:
+		    i915_gem_object_set_frontbuffer(obj, NULL) != NULL);

total: 0 errors, 0 warnings, 1 checks, 9 lines checked

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

[Jani Nikula]

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

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Warn on if set frontbuffer return value is not NULL on release

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 7912 bytes --]

== Series Details ==

Series: drm/i915: Warn on if set frontbuffer return value is not NULL on release
URL   : https://patchwork.freedesktop.org/series/124375/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13687 -> Patchwork_124375v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/index.html

Participating hosts (38 -> 38)
------------------------------

  Additional (2): bat-dg2-8 fi-hsw-4770 
  Missing    (2): fi-snb-2520m fi-pnv-d510 

Known issues
------------

  Here are the changes found in Patchwork_124375v1 that come from known issues:

### CI changes ###

#### Issues hit ####

  * boot:
    - fi-hsw-4770:        NOTRUN -> [FAIL][1] ([i915#8293])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/fi-hsw-4770/boot.html

  

### IGT changes ###

#### Issues hit ####

  * igt@gem_mmap@basic:
    - bat-dg2-8:          NOTRUN -> [SKIP][2] ([i915#4083])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@gem_mmap@basic.html

  * igt@gem_mmap_gtt@basic:
    - bat-dg2-8:          NOTRUN -> [SKIP][3] ([i915#4077]) +2 other tests skip
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@gem_mmap_gtt@basic.html

  * igt@gem_tiled_pread_basic:
    - bat-dg2-8:          NOTRUN -> [SKIP][4] ([i915#4079]) +1 other test skip
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_rps@basic-api:
    - bat-dg2-8:          NOTRUN -> [SKIP][5] ([i915#6621])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@requests:
    - bat-mtlp-8:         [PASS][6] -> [ABORT][7] ([i915#9414])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13687/bat-mtlp-8/igt@i915_selftest@live@requests.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-mtlp-8/igt@i915_selftest@live@requests.html

  * igt@i915_suspend@basic-s3-without-i915:
    - bat-dg2-8:          NOTRUN -> [SKIP][8] ([i915#6645])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - bat-dg2-8:          NOTRUN -> [SKIP][9] ([i915#5190])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_addfb_basic@basic-y-tiled-legacy:
    - bat-dg2-8:          NOTRUN -> [SKIP][10] ([i915#4215] / [i915#5190])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@kms_addfb_basic@basic-y-tiled-legacy.html

  * igt@kms_addfb_basic@framebuffer-vs-set-tiling:
    - bat-dg2-8:          NOTRUN -> [SKIP][11] ([i915#4212]) +6 other tests skip
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@kms_addfb_basic@framebuffer-vs-set-tiling.html

  * igt@kms_addfb_basic@tile-pitch-mismatch:
    - bat-dg2-8:          NOTRUN -> [SKIP][12] ([i915#4212] / [i915#5608])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@kms_addfb_basic@tile-pitch-mismatch.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - bat-dg2-8:          NOTRUN -> [SKIP][13] ([i915#4103] / [i915#4213] / [i915#5608]) +1 other test skip
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_force_connector_basic@force-load-detect:
    - bat-dg2-8:          NOTRUN -> [SKIP][14] ([fdo#109285])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_force_connector_basic@prune-stale-modes:
    - bat-dg2-8:          NOTRUN -> [SKIP][15] ([i915#5274])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@kms_force_connector_basic@prune-stale-modes.html

  * igt@kms_psr@cursor_plane_move:
    - bat-dg2-8:          NOTRUN -> [SKIP][16] ([i915#1072]) +3 other tests skip
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@kms_psr@cursor_plane_move.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-dg2-8:          NOTRUN -> [SKIP][17] ([i915#3555])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-fence-flip:
    - bat-dg2-8:          NOTRUN -> [SKIP][18] ([i915#3708])
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@prime_vgem@basic-fence-flip.html

  * igt@prime_vgem@basic-fence-mmap:
    - bat-dg2-8:          NOTRUN -> [SKIP][19] ([i915#3708] / [i915#4077]) +1 other test skip
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@prime_vgem@basic-fence-mmap.html

  * igt@prime_vgem@basic-write:
    - bat-dg2-8:          NOTRUN -> [SKIP][20] ([i915#3291] / [i915#3708]) +2 other tests skip
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-8/igt@prime_vgem@basic-write.html

  
#### Possible fixes ####

  * igt@kms_chamelium_edid@hdmi-edid-read:
    - {bat-dg2-13}:       [DMESG-WARN][21] ([i915#7952]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13687/bat-dg2-13/igt@kms_chamelium_edid@hdmi-edid-read.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/bat-dg2-13/igt@kms_chamelium_edid@hdmi-edid-read.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212
  [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213
  [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5274]: https://gitlab.freedesktop.org/drm/intel/issues/5274
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5608]: https://gitlab.freedesktop.org/drm/intel/issues/5608
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
  [i915#7952]: https://gitlab.freedesktop.org/drm/intel/issues/7952
  [i915#8293]: https://gitlab.freedesktop.org/drm/intel/issues/8293
  [i915#9414]: https://gitlab.freedesktop.org/drm/intel/issues/9414


Build changes
-------------

  * Linux: CI_DRM_13687 -> Patchwork_124375v1

  CI-20190529: 20190529
  CI_DRM_13687: e2d29b46ca6d480bc3bc328a7775c3028bc1e5c8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7506: 4fdf544bd0a38c5a100ef43c30171827e1c8c442 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_124375v1: e2d29b46ca6d480bc3bc328a7775c3028bc1e5c8 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

2142de6a8ec0 drm/i915: Warn on if set frontbuffer return value is not NULL on release

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_124375v1/index.html

[-- Attachment #2: Type: text/html, Size: 9353 bytes --]

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

[Hogander, Jouni]

On Thu, 2023-09-28 at 10:43 +0300, Jani Nikula wrote:
> 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);

Thank you Jani for bringing up this aspect. Just sent a new version of
this patch. Please check.

BR,

Jouni Högander

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

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

[Gustavo Sousa]

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

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

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

[Jani Nikula]

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

Re: [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@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!

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