Re: [PATCH v2 1/5] drm/i915/frontbuffer: Move bo refcounting intel_frontbuffer_{get, release}()

[Jani Nikula <jani.nikula@linux.intel.com>]

On Fri, 03 Oct 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> 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ä <ville.syrjala@linux.intel.com>

I'm a bit uneasy about all of this code, but the change looks all right,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>



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