On Thu, 04 Sep 2025, Ville Syrjälä <[email protected]> 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 <[email protected]>
>> ---
>> 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?
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.
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
