Message-ID: <molexnyjkiryvhetfdc66gmzecrf6f7kxl656qn46djdkixrkb@fdgnp5hispbf>
Date: Tue, 13 May 2025 15:06:41 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Jani Nikula <jani.nikula@...ux.intel.com>
Cc: Anusha Srivatsa <asrivats@...hat.com>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Jessica Zhang <quic_jesszhan@...cinc.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann <tzimmermann@...e.de>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, dri-devel@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org, Luca Ceresoli <luca.ceresoli@...tlin.com>
Subject: Re: [PATCH v4 2/4] drm/panel: Add refcount support

On Fri, May 09, 2025 at 03:45:36PM +0300, Jani Nikula wrote:
> On Fri, 09 May 2025, Maxime Ripard <mripard@...nel.org> wrote:
> > On Thu, May 08, 2025 at 05:27:21PM +0300, Jani Nikula wrote:
> >> On Mon, 05 May 2025, Anusha Srivatsa <asrivats@...hat.com> wrote:
> >> > On Mon, May 5, 2025 at 2:54 AM Maxime Ripard <mripard@...nel.org> wrote:
> >> >
> >> >> Hi Jani,
> >> >>
> >> >> On Tue, Apr 29, 2025 at 12:22:00PM +0300, Jani Nikula wrote:
> >> >> > On Tue, 29 Apr 2025, Maxime Ripard <mripard@...nel.org> wrote:
> >> >> > > Hi Jani,
> >> >> > >
> >> >> > > On Mon, Apr 28, 2025 at 07:31:50PM +0300, Jani Nikula wrote:
> >> >> > >> On Mon, 31 Mar 2025, Anusha Srivatsa <asrivats@...hat.com> wrote:
> >> >> > >> > Allocate panel via reference counting. Add _get() and _put() helper
> >> >> > >> > functions to ensure panel allocations are refcounted. Avoid use
> >> >> after
> >> >> > >> > free by ensuring panel pointer is valid and can be usable till the
> >> >> last
> >> >> > >> > reference is put.
> >> >> > >> >
> >> >> > >> > Reviewed-by: Luca Ceresoli <luca.ceresoli@...tlin.com>
> >> >> > >> > Reviewed-by: Maxime Ripard <mripard@...nel.org>
> >> >> > >> > Signed-off-by: Anusha Srivatsa <asrivats@...hat.com>
> >> >> > >> >
> >> >> > >> > ---
> >> >> > >> > v4: Add refcounting documentation in this patch (Maxime)
> >> >> > >> >
> >> >> > >> > v3: Add include in this patch (Luca)
> >> >> > >> >
> >> >> > >> > v2: Export drm_panel_put/get() (Maxime)
> >> >> > >> > - Change commit log with better workding (Luca, Maxime)
> >> >> > >> > - Change drm_panel_put() to return void (Luca)
> >> >> > >> > - Code Cleanups - add return in documentation, replace bridge to
> >> >> > >> > panel (Luca)
> >> >> > >> > ---
> >> >> > >> >  drivers/gpu/drm/drm_panel.c | 64
> >> >> ++++++++++++++++++++++++++++++++++++++++++++-
> >> >> > >> >  include/drm/drm_panel.h     | 19 ++++++++++++++
> >> >> > >> >  2 files changed, 82 insertions(+), 1 deletion(-)
> >> >> > >> >
> >> >> > >> > diff --git a/drivers/gpu/drm/drm_panel.c
> >> >> b/drivers/gpu/drm/drm_panel.c
> >> >> > >> > index
> >> >> bdeab5710ee324dc1742fbc77582250960556308..7b17531d85a4dc3031709919564d2e4d8332f748
> >> >> 100644
> >> >> > >> > --- a/drivers/gpu/drm/drm_panel.c
> >> >> > >> > +++ b/drivers/gpu/drm/drm_panel.c
> >> >> > >> > @@ -355,24 +355,86 @@ struct drm_panel *of_drm_find_panel(const
> >> >> struct device_node *np)
> >> >> > >> >  }
> >> >> > >> >  EXPORT_SYMBOL(of_drm_find_panel);
> >> >> > >> >
> >> >> > >> > +static void __drm_panel_free(struct kref *kref)
> >> >> > >> > +{
> >> >> > >> > +        struct drm_panel *panel = container_of(kref, struct
> >> >> drm_panel, refcount);
> >> >> > >> > +
> >> >> > >> > +        kfree(panel->container);
> >> >> > >> > +}
> >> >> > >> > +
> >> >> > >> > +/**
> >> >> > >> > + * drm_panel_get - Acquire a panel reference
> >> >> > >> > + * @panel: DRM panel
> >> >> > >> > + *
> >> >> > >> > + * This function increments the panel's refcount.
> >> >> > >> > + * Returns:
> >> >> > >> > + * Pointer to @panel
> >> >> > >> > + */
> >> >> > >> > +struct drm_panel *drm_panel_get(struct drm_panel *panel)
> >> >> > >> > +{
> >> >> > >> > +        if (!panel)
> >> >> > >> > +                return panel;
> >> >> > >> > +
> >> >> > >> > +        kref_get(&panel->refcount);
> >> >> > >> > +
> >> >> > >> > +        return panel;
> >> >> > >> > +}
> >> >> > >> > +EXPORT_SYMBOL(drm_panel_get);
> >> >> > >> > +
> >> >> > >> > +/**
> >> >> > >> > + * drm_panel_put - Release a panel reference
> >> >> > >> > + * @panel: DRM panel
> >> >> > >> > + *
> >> >> > >> > + * This function decrements the panel's reference count and frees
> >> >> the
> >> >> > >> > + * object if the reference count drops to zero.
> >> >> > >> > + */
> >> >> > >> > +void drm_panel_put(struct drm_panel *panel)
> >> >> > >> > +{
> >> >> > >> > +        if (panel)
> >> >> > >> > +                kref_put(&panel->refcount, __drm_panel_free);
> >> >> > >> > +}
> >> >> > >> > +EXPORT_SYMBOL(drm_panel_put);
> >> >> > >> > +
> >> >> > >> > +/**
> >> >> > >> > + * drm_panel_put_void - wrapper to drm_panel_put() taking a void
> >> >> pointer
> >> >> > >> > + *
> >> >> > >> > + * @data: pointer to @struct drm_panel, cast to a void pointer
> >> >> > >> > + *
> >> >> > >> > + * Wrapper of drm_panel_put() to be used when a function taking a
> >> >> void
> >> >> > >> > + * pointer is needed, for example as a devm action.
> >> >> > >> > + */
> >> >> > >> > +static void drm_panel_put_void(void *data)
> >> >> > >> > +{
> >> >> > >> > +        struct drm_panel *panel = (struct drm_panel *)data;
> >> >> > >> > +
> >> >> > >> > +        drm_panel_put(panel);
> >> >> > >> > +}
> >> >> > >> > +
> >> >> > >> >  void *__devm_drm_panel_alloc(struct device *dev, size_t size,
> >> >> size_t offset,
> >> >> > >> >                               const struct drm_panel_funcs *funcs,
> >> >> > >> >                               int connector_type)
> >> >> > >> >  {
> >> >> > >> >          void *container;
> >> >> > >> >          struct drm_panel *panel;
> >> >> > >> > +        int err;
> >> >> > >> >
> >> >> > >> >          if (!funcs) {
> >> >> > >> >                  dev_warn(dev, "Missing funcs pointer\n");
> >> >> > >> >                  return ERR_PTR(-EINVAL);
> >> >> > >> >          }
> >> >> > >> >
> >> >> > >> > -        container = devm_kzalloc(dev, size, GFP_KERNEL);
> >> >> > >> > +        container = kzalloc(size, GFP_KERNEL);
> >> >> > >> >          if (!container)
> >> >> > >> >                  return ERR_PTR(-ENOMEM);
> >> >> > >> >
> >> >> > >> >          panel = container + offset;
> >> >> > >> > +        panel->container = container;
> >> >> > >> >          panel->funcs = funcs;
> >> >> > >> > +        kref_init(&panel->refcount);
> >> >> > >>
> >> >> > >> Hi Anusha, this should be done in drm_panel_init() instead.
> >> >> > >>
> >> >> > >> There are many users of drm_panel that don't use
> >> >> devm_drm_panel_alloc()
> >> >> > >> but allocate separately, and call drm_panel_init() only.
> >> >> > >
> >> >> > > That wouldn't really work, because then drivers would have allocated
> >> >> the
> >> >> > > panel with devm_kzalloc and thus the structure would be freed when the
> >> >> > > device is removed, no matter the reference counting state.
> >> >> > >
> >> >> > >> They'll all have refcount set to 0 instead of 1 like kref_init() does.
> >> >> > >>
> >> >> > >> This means all subsequent get/put pairs on such panels will lead to
> >> >> > >> __drm_panel_free() being called! But through a lucky coincidence, that
> >> >> > >> will be a nop because panel->container is also not initialized...
> >> >> > >>
> >> >> > >> I'm sorry to say, the drm refcounting interface is quite broken for
> >> >> such
> >> >> > >> use cases.
> >> >> > >
> >> >> > > The plan is to convert all panel drivers to that function, and Anusha
> >> >> > > already sent series to do. It still needs a bit of work, but it should
> >> >> > > land soon-ish.
> >> >> > >
> >> >> > > For the transitional period though, it's not clear to me what you think
> >> >> > > is broken at the moment, and / or what should be fixed.
> >> >> > >
> >> >> > > Would you prefer an explicit check on container not being 0, with a
> >> >> > > comment?
> >> >> >
> >> >> > I'm looking at what it would take to add drm_panel support to i915 so
> >> >> > that you could have drm_panel_followers on it. There are gaps of course,
> >> >> > but initially it would mean allocating and freeing drm_panel ourselves,
> >> >> > not via devm_drm_panel_alloc() nor devm_kzalloc(), because none of the
> >> >> > other stuff is allocated that way. drm_panel would just sit as a
> >> >> > sub-struct inside struct intel_panel, which is a sub-struct of struct
> >> >> > intel_connector, which has its own allocation...
> >> >>
> >> >> I'm not entirely sure why you would need to allocate it from i915? The
> >> >> drm_panel structure is only meant to be allocated by panel drivers, and
> >> >> afaik no panel interface controller is allocating it.
> >> 
> >> I'm looking into a use case involving drm_panel_follower, which requires
> >> a drm_panel. I don't really need any of the other stuff in drm_panel.
> >> 
> >> And basically you'd have one drm_panel per connector that is connected
> >> to a panel, within the same driver.
> >
> > That answers why you need a drm_panel pointer, but not really why the
> > i915 needs to allocate it itself. The whole point of panel drivers it to
> > decouple panel drivers from the connector driver (and everything
> > upstream).
> >
> > drm_panel is always allocated by the panel driver itself. I don't really
> > see a good reason to change that.
> >
> > If you don't have a panel descriptor in the ACPI tables, then you can
> > always allocate a panel-edp driver or whatever from i915 and getting its
> > drm_panel?
> 
> The thing is, absolutely none of our hardware/firmware/software stack
> was designed in a way that would fit the drm_panel model. (Or, arguably,
> drm_panel wasn't designed in a way that would fit our stack, in the
> chronology of things.)
>
> It's all one PCI device.

You access the panel itself through PCI? Not i2c, not CSI, not eDP, but
PCI?

> All in the same MMIO space. The VBT (Video BIOS Tables) contain all
> the information for the panels, as well as for absolutely everything
> else about our display hardware. It's not separate in any meaningful
> way.

It's a pretty common setup on ARM, there's nothing special about it
except (maybe) its scale. It's exactly what the MFD framework was
designed for, and several other similar mechanisms.

> Having a separate panel driver would get in the way. Having panel-edp
> would get in the way. That's why there isn't a single x86 user for
> drm_panel, AFAICT.

At the end of the day, it's also about interacting with the larger
framework. You're effectively asking a common part of the framework that
works with dozens of drivers to compromise its design for one.

Is it really surprising you get some pushback when you are using a
design that is the complete opposite to what every user of it for the
last decade has been doing?

> Yes, we only really need the drm_panel handle, to play ball with the
> things that were built around it, specifically drm_panel_follower.
> 
> And we do need to allocate drm_panel ourselves. We're both the host and
> the panel driver at the same time, and there's no benefit in trying to
> articially change that. DRM infrastructure should provide frameworks
> that are usable for everyone,

This one is usable, but you rule out the way you could use it. I guess
it's clear now that you won't consider anything else. I wonder why you
started that discussion in the first place if you already have a clear
mind on how to get things moving forward.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists