| Message ID | 20230928065210.103376-1-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/i915: Warn on if set frontbuffer return value is not NULL on release | expand |
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);
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); >
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
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
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
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);
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(-)