Re: [PATCH 6/9] drm/{i915,xe}/fbdev: add intel_fbdev_fb_bo_destroy()

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

On Wed, 17 Sep 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Wed, Sep 17, 2025 at 03:33:31PM +0300, Jani Nikula wrote:
>> On Thu, 04 Sep 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Wed, Sep 03, 2025 at 11:32:03PM +0300, Jani Nikula wrote:
>> >> xe does xe_bo_unpin_map_no_vm() on the failure path. Add a common helper
>> >> to enable further refactoring.
>> >> 
>> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/display/intel_fbdev_fb.c | 5 +++++
>> >>  drivers/gpu/drm/i915/display/intel_fbdev_fb.h | 1 +
>> >>  drivers/gpu/drm/xe/display/intel_fbdev_fb.c   | 7 ++++++-
>> >>  3 files changed, 12 insertions(+), 1 deletion(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
>> >> index 3837973b0d25..6b70823ce5ef 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.c
>> >> @@ -51,6 +51,11 @@ struct drm_gem_object *intel_fbdev_fb_bo_create(struct drm_device *drm, int size
>> >>  	return &obj->base;
>> >>  }
>> >>  
>> >> +void intel_fbdev_fb_bo_destroy(struct drm_gem_object *obj)
>> >> +{
>> >> +	/* nop? */
>> >
>> > gem_object_put() is what destroys the bo on i915, so I think you're
>> > introducing a leak in the next patch with this nop implementation.
>> >
>> > xe seems to be riddled with footguns here since it conflates
>> > creation+pinning+whatever in the same thing (and I guess it
>> > doesn't know how to clean all that up when the last reference
>> > to the object disappears?) and you have to use that horribly
>> > misnamed function instead...
>> 
>> Hmm, can we just slap i915_gem_object_put(obj) in there?
>
> That would at least match how it behaves currently. Though I
> didn't look too deeply whether that is 100% sufficient to clean
> up properly.

I'm doing just that in [1].

[1] https://lore.kernel.org/r/22bc3c3158f5a22ab258ada8684766fdf75fefec.1758184771.git.jani.nikula@intel.com

>> 
>> The i915 variant of intel_fbdev_fb_alloc() ignores errors from
>> intel_framebuffer_create() and just unconditionally does
>> i915_gem_object_put() afterwards:
>> 
>> 	fb = intel_framebuffer_create(intel_bo_to_drm_bo(obj),
>> 				      drm_get_format_info(display->drm,
>> 							  mode_cmd.pixel_format,
>> 							  mode_cmd.modifier[0]),
>> 				      &mode_cmd);
>> 	i915_gem_object_put(obj);
>> 
>> 	return to_intel_framebuffer(fb);
>> 
>> Presumably the refcounts are handled correctly either way.
>> 
>> It's just a bit fishy that the potential error pointer from
>> intel_framebuffer_create() goes through to_intel_framebuffer() to the
>> caller.
>
> Yeah, would be less confusing to just check for the error
> explicitly.

Added a new patch for that [2].

[2] https://lore.kernel.org/r/17631db227d527d6c67f5d6b67adec1ff8dc6f8d.1758184771.git.jani.nikula@intel.com

Together, I believe [1] and [2] keep the current behaviour
intact. Whether that's 100% correct or not, I couldn't say, but at least
I'm shouldn't be introducing any new issues.

BR,
Jani.


-- 
Jani Nikula, Intel