Message-ID: <874ixvtbxy.fsf@intel.com>
Date: Thu, 08 May 2025 17:27:21 +0300
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Anusha Srivatsa <asrivats@...hat.com>, Maxime Ripard <mripard@...nel.org>
Cc: 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 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.

>> > But basically in its current state, the refcounting would not be
>> > reliable for that use case. I guess with panel->container being NULL
>> > nothing happens, but the idea that the refcount drops back to 0 after a
>> > get/put is a bit scary.
>> >
>> > Anyway, I think there should be no harm in moving the kref init to
>> > drm_panel_init(), right?
>>
>> I mean, there is because the plan so far was to remove drm_panel_init() :)

The problem with that is that it forces a certain type of allocation of
drm_panel. devm_drm_panel_alloc() allows embedding drm_panel inside
another struct, but that's inflexible for our use case, where we'd
probably like to embed it inside something that's already allocated as
part of something else.

I mean devm_drm_panel_alloc() is great, but please don't make its use
mandatory!

> Jani,
> the series that converts all drivers to use the new API:
> https://patchwork.freedesktop.org/series/147082/
> https://patchwork.freedesktop.org/series/147157/
> https://patchwork.freedesktop.org/series/147246/
>
> not landed yet but these are WIP. Still trying to understand your point
> though... not sure what is broken.

Nothing upstream is broken per se, but if you allocated drm_panel
directly yourself, initialized it with drm_panel_init(), its refcount
would initially be 0, not 1, and each subsequent get/put on it would
call __drm_panel_free().

Even that doesn't break stuff, because by luck panel->container is also
NULL in this case.


BR,
Jani.

-- 
Jani Nikula, Intel

Powered by blists - more mailing lists