On Fri, 03 Oct 2025, Ville Syrjala <[email protected]> wrote: > From: Ville Syrjälä <[email protected]> > > Currently xe's intel_frontbuffer implementation forgets to > hold a reference on the bo. This makes the entire thing > extremely fragile as the cleanup order now depends on bo > references held by other things > (namely intel_fb_bo_framebuffer_fini()). > > Move the bo refcounting to intel_frontbuffer_{get,release}() > so that both i915 and xe do this the same way. > > I first tried to fix this by having xe do the refcounting > from its intel_bo_set_frontbuffer() implementation > (which is what i915 does currently), but turns out xe's > drm_gem_object_free() can sleep and thus drm_gem_object_put() > isn't safe to call while we hold fb_tracking.lock. > > Signed-off-by: Ville Syrjälä <[email protected]>
I'm a bit uneasy about all of this code, but the change looks all right, Reviewed-by: Jani Nikula <[email protected]> > --- > drivers/gpu/drm/i915/display/intel_frontbuffer.c | 10 +++++++++- > drivers/gpu/drm/i915/gem/i915_gem_object_frontbuffer.h | 2 -- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c > b/drivers/gpu/drm/i915/display/intel_frontbuffer.c > index 43be5377ddc1..73ed28ac9573 100644 > --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c > +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c > @@ -270,6 +270,8 @@ static void frontbuffer_release(struct kref *ref) > spin_unlock(&display->fb_tracking.lock); > > i915_active_fini(&front->write); > + > + drm_gem_object_put(obj); > kfree_rcu(front, rcu); > } > > @@ -287,6 +289,8 @@ intel_frontbuffer_get(struct drm_gem_object *obj) > if (!front) > return NULL; > > + drm_gem_object_get(obj); > + > front->obj = obj; > kref_init(&front->ref); > atomic_set(&front->bits, 0); > @@ -299,8 +303,12 @@ intel_frontbuffer_get(struct drm_gem_object *obj) > spin_lock(&display->fb_tracking.lock); > cur = intel_bo_set_frontbuffer(obj, front); > spin_unlock(&display->fb_tracking.lock); > - if (cur != front) > + > + if (cur != front) { > + drm_gem_object_put(obj); > kfree(front); > + } > + > return cur; > } > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_frontbuffer.h > b/drivers/gpu/drm/i915/gem/i915_gem_object_frontbuffer.h > index b6dc3d1b9bb1..b682969e3a29 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_frontbuffer.h > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_frontbuffer.h > @@ -89,12 +89,10 @@ i915_gem_object_set_frontbuffer(struct > drm_i915_gem_object *obj, > > if (!front) { > RCU_INIT_POINTER(obj->frontbuffer, NULL); > - drm_gem_object_put(intel_bo_to_drm_bo(obj)); > } else if (rcu_access_pointer(obj->frontbuffer)) { > cur = rcu_dereference_protected(obj->frontbuffer, true); > kref_get(&cur->ref); > } else { > - drm_gem_object_get(intel_bo_to_drm_bo(obj)); > rcu_assign_pointer(obj->frontbuffer, front); > } -- Jani Nikula, Intel
