Re: [PATCH 6/8] drm/{i915,xe}/panic: move framebuffer allocation where it belongs

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 01, 2025 at 08:28:53PM +0300, Jani Nikula wrote:
> On Wed, 01 Oct 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Wed, Oct 01, 2025 at 06:04:58PM +0300, Jani Nikula wrote:
> >> On Tue, 02 Sep 2025, Jani Nikula <jani.nikula@intel.com> wrote:
> >> > The struct intel_framebuffer allocation naturally belongs in intel_fb.c,
> >> > not hidden inside panic implementation. Separate the panic
> >> > allocation. Drop the unnecessary struct i915_framebuffer and struct
> >> > xe_framebuffer types.
> >> >
> >> > Cc: Jocelyn Falempe <jfalempe@redhat.com>
> >> > Cc: Maarten Lankhorst <dev@lankhorst.se>
> >> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/display/intel_fb.c    | 17 ++++++++++++++++-
> >> >  drivers/gpu/drm/i915/display/intel_panic.c |  4 ++--
> >> >  drivers/gpu/drm/i915/display/intel_panic.h |  3 ++-
> >> >  drivers/gpu/drm/i915/gem/i915_gem_object.h |  5 +++--
> >> >  drivers/gpu/drm/i915/gem/i915_gem_pages.c  | 17 ++++-------------
> >> >  drivers/gpu/drm/xe/display/xe_panic.c      | 17 ++++-------------
> >> >  6 files changed, 31 insertions(+), 32 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> >> > index 2a2ed0f0461f..22a4a1575d22 100644
> >> > --- a/drivers/gpu/drm/i915/display/intel_fb.c
> >> > +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> >> > @@ -2346,7 +2346,22 @@ intel_user_framebuffer_create(struct drm_device *dev,
> >> >  
> >> >  struct intel_framebuffer *intel_framebuffer_alloc(void)
> >> >  {
> >> > -	return intel_bo_alloc_framebuffer();
> >> > +	struct intel_framebuffer *intel_fb;
> >> > +	struct intel_panic *panic;
> >> > +
> >> > +	intel_fb = kzalloc(sizeof(*intel_fb), GFP_KERNEL);
> >> > +	if (!intel_fb)
> >> > +		return NULL;
> >> > +
> >> > +	panic = intel_panic_alloc();
> >> > +	if (!panic) {
> >> > +		kfree(intel_fb);
> >> > +		return NULL;
> >> > +	}
> >> > +
> >> > +	intel_fb->panic = panic;
> >> 
> >> So I screwed up here. There's no deallocation of fb->panic, and this
> >> leaks. I don't know what I was thinking.
> >> 
> >> To make matters worse, struct intel_framebuffer is deallocated via
> >> drm_framebuffer_put() i.e. there's no obvious place to plug the free in.
> >
> > intel_user_framebuffer_destroy()
> 
> D'oh! I still wasn't thinking it appears. Thanks.
> 
> Still not straightforward with the alloc and init split in
> i9xx_get_initial_plane_config()/skl_get_initial_plane_config() and the
> intel_alloc_initial_plane_obj() implementations.
> 
> I think the framebuffer leaked on the error paths already before my
> change, so I guess I could just plug what I caused.

Maybe just move the panic alloc into intel_framebuffer_init()?

Hmm, looks like someone opened the small race there again with the
intel_frontbuffer_get() vs. intel_fb_bo_framebuffer_init() reodering.
I'll need to fix that again...

-- 
Ville Syrjälä
Intel