On Wed, Sep 17, 2025 at 03:33:31PM +0300, Jani Nikula wrote:
> On Thu, 04 Sep 2025, Ville Syrjälä <ville.syrj...@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.nik...@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.
>
> 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.
--
Ville Syrjälä
Intel
