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

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

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

*sigh*

I really wish we had someone actually well versed in i915/gem and xe/ttm
and all that figuring out the necessary building blocks of the common
API. I think as it is these are too high level, necessarily putting too
much display code in i915 and xe cores. We desperately need better
abstractions between the two drivers.

Yeah, I'm not that person, but here I am doing it anyway.


BR,
Jani.



>
>> +}
>> +
>>  struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_device *drm,
>>  					       struct drm_mode_fb_cmd2 *mode_cmd)
>>  {
>> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev_fb.h b/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
>> index b10c4635bf46..6d6f316834df 100644
>> --- a/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
>> +++ b/drivers/gpu/drm/i915/display/intel_fbdev_fb.h
>> @@ -17,6 +17,7 @@ struct intel_display;
>>  
>>  u32 intel_fbdev_fb_pitch_align(u32 stride);
>>  struct drm_gem_object *intel_fbdev_fb_bo_create(struct drm_device *drm, int size);
>> +void intel_fbdev_fb_bo_destroy(struct drm_gem_object *obj);
>>  struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_device *drm,
>>  					       struct drm_mode_fb_cmd2 *mode_cmd);
>>  int intel_fbdev_fb_fill_info(struct intel_display *display, struct fb_info *info,
>> diff --git a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
>> index 9a5d14d5781a..9a5cf50ea7de 100644
>> --- a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
>> +++ b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
>> @@ -54,6 +54,11 @@ struct drm_gem_object *intel_fbdev_fb_bo_create(struct drm_device *drm, int size
>>  	return &obj->ttm.base;
>>  }
>>  
>> +void intel_fbdev_fb_bo_destroy(struct drm_gem_object *obj)
>> +{
>> +	xe_bo_unpin_map_no_vm(gem_to_xe_bo(obj));
>> +}
>> +
>>  struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_device *drm,
>>  					       struct drm_mode_fb_cmd2 *mode_cmd)
>>  {
>> @@ -76,7 +81,7 @@ struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_device *drm,
>>  							  mode_cmd->modifier[0]),
>>  				      mode_cmd);
>>  	if (IS_ERR(fb)) {
>> -		xe_bo_unpin_map_no_vm(gem_to_xe_bo(obj));
>> +		intel_fbdev_fb_bo_destroy(obj);
>>  		goto err;
>>  	}
>>  
>> -- 
>> 2.47.2

-- 
Jani Nikula, Intel