On Wed, Oct 01, 2025 at 06:04:58PM +0300, Jani Nikula wrote:
> On Tue, 02 Sep 2025, Jani Nikula <[email protected]> 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 <[email protected]>
> > Cc: Maarten Lankhorst <[email protected]>
> > Signed-off-by: Jani Nikula <[email protected]>
> > ---
> > 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()
And someone should probably s/_user// on all those fb func to
make things less confusing.
>
> Any ideas before I start looking at reverting the changes, and get back
> to the drawing board with abstracting the code between i915 and xe?
>
> BR,
> Jani.
>
> > +
> > + return intel_fb;
> > }
> >
> > struct drm_framebuffer *
> > diff --git a/drivers/gpu/drm/i915/display/intel_panic.c
> > b/drivers/gpu/drm/i915/display/intel_panic.c
> > index 20eecb0f168f..5431bd4d3a7d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_panic.c
> > +++ b/drivers/gpu/drm/i915/display/intel_panic.c
> > @@ -4,9 +4,9 @@
> > #include "gem/i915_gem_object.h"
> > #include "intel_panic.h"
> >
> > -struct intel_framebuffer *intel_bo_alloc_framebuffer(void)
> > +struct intel_panic *intel_panic_alloc(void)
> > {
> > - return i915_gem_object_alloc_framebuffer();
> > + return i915_gem_object_alloc_panic();
> > }
> >
> > int intel_panic_setup(struct drm_scanout_buffer *sb)
> > diff --git a/drivers/gpu/drm/i915/display/intel_panic.h
> > b/drivers/gpu/drm/i915/display/intel_panic.h
> > index 67ce253fcdf5..45ce6104e6fb 100644
> > --- a/drivers/gpu/drm/i915/display/intel_panic.h
> > +++ b/drivers/gpu/drm/i915/display/intel_panic.h
> > @@ -6,8 +6,9 @@
> >
> > struct drm_scanout_buffer;
> > struct intel_framebuffer;
> > +struct intel_panic;
> >
> > -struct intel_framebuffer *intel_bo_alloc_framebuffer(void);
> > +struct intel_panic *intel_panic_alloc(void);
> > int intel_panic_setup(struct drm_scanout_buffer *sb);
> > void intel_panic_finish(struct intel_framebuffer *fb);
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > index 565f8fa330db..9b3f25cb48db 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h
> > @@ -16,9 +16,10 @@
> > #include "i915_gem_ww.h"
> > #include "i915_vma_types.h"
> >
> > -struct drm_scanout_buffer;
> > enum intel_region_id;
> > +struct drm_scanout_buffer;
> > struct intel_framebuffer;
> > +struct intel_panic;
> >
> > #define obj_to_i915(obj__) to_i915((obj__)->base.dev)
> >
> > @@ -693,7 +694,7 @@ i915_gem_object_unpin_pages(struct drm_i915_gem_object
> > *obj)
> > int __i915_gem_object_put_pages(struct drm_i915_gem_object *obj);
> > int i915_gem_object_truncate(struct drm_i915_gem_object *obj);
> >
> > -struct intel_framebuffer *i915_gem_object_alloc_framebuffer(void);
> > +struct intel_panic *i915_gem_object_alloc_panic(void);
> > int i915_gem_object_panic_setup(struct drm_scanout_buffer *sb);
> > void i915_gem_object_panic_finish(struct intel_framebuffer *fb);
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > index e36d60b785b1..b219474aecc7 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > @@ -363,11 +363,6 @@ struct intel_panic {
> > void *vaddr;
> > };
> >
> > -struct i915_framebuffer {
> > - struct intel_framebuffer base;
> > - struct intel_panic panic;
> > -};
> > -
> > static void i915_panic_kunmap(struct intel_panic *panic)
> > {
> > if (panic->vaddr) {
> > @@ -436,17 +431,13 @@ static void
> > i915_gem_object_panic_page_set_pixel(struct drm_scanout_buffer *sb,
> > }
> > }
> >
> > -struct intel_framebuffer *i915_gem_object_alloc_framebuffer(void)
> > +struct intel_panic *i915_gem_object_alloc_panic(void)
> > {
> > - struct i915_framebuffer *i915_fb;
> > -
> > - i915_fb = kzalloc(sizeof(*i915_fb), GFP_KERNEL);
> > - if (!i915_fb)
> > - return NULL;
> > + struct intel_panic *panic;
> >
> > - i915_fb->base.panic = &i915_fb->panic;
> > + panic = kzalloc(sizeof(*panic), GFP_KERNEL);
> >
> > - return &i915_fb->base;
> > + return panic;
> > }
> >
> > /*
> > diff --git a/drivers/gpu/drm/xe/display/xe_panic.c
> > b/drivers/gpu/drm/xe/display/xe_panic.c
> > index 3ef23a6795b3..b5a7615708a1 100644
> > --- a/drivers/gpu/drm/xe/display/xe_panic.c
> > +++ b/drivers/gpu/drm/xe/display/xe_panic.c
> > @@ -15,11 +15,6 @@ struct intel_panic {
> > void *vaddr;
> > };
> >
> > -struct xe_framebuffer {
> > - struct intel_framebuffer base;
> > - struct intel_panic panic;
> > -};
> > -
> > static void xe_panic_kunmap(struct intel_panic *panic)
> > {
> > if (panic->vaddr) {
> > @@ -62,17 +57,13 @@ static void xe_panic_page_set_pixel(struct
> > drm_scanout_buffer *sb, unsigned int
> > }
> > }
> >
> > -struct intel_framebuffer *intel_bo_alloc_framebuffer(void)
> > +struct intel_panic *intel_panic_alloc(void)
> > {
> > - struct xe_framebuffer *xe_fb;
> > -
> > - xe_fb = kzalloc(sizeof(*xe_fb), GFP_KERNEL);
> > - if (!xe_fb)
> > - return NULL;
> > + struct intel_panic *panic;
> >
> > - xe_fb->base.panic = &xe_fb->panic;
> > + panic = kzalloc(sizeof(*panic), GFP_KERNEL);
> >
> > - return &xe_fb->base;
> > + return panic;
> > }
> >
> > int intel_panic_setup(struct drm_scanout_buffer *sb)
>
> --
> Jani Nikula, Intel
--
Ville Syrjälä
Intel
