On Thu, May 08, 2025 at 05:27:21PM +0300, Jani Nikula wrote:
> On Mon, 05 May 2025, Anusha Srivatsa <asriv...@redhat.com> wrote:
> > On Mon, May 5, 2025 at 2:54 AM Maxime Ripard <mrip...@kernel.org> wrote:
> >
> >> Hi Jani,
> >>
> >> On Tue, Apr 29, 2025 at 12:22:00PM +0300, Jani Nikula wrote:
> >> > On Tue, 29 Apr 2025, Maxime Ripard <mrip...@kernel.org> wrote:
> >> > > Hi Jani,
> >> > >
> >> > > On Mon, Apr 28, 2025 at 07:31:50PM +0300, Jani Nikula wrote:
> >> > >> On Mon, 31 Mar 2025, Anusha Srivatsa <asriv...@redhat.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.ceres...@bootlin.com>
> >> > >> > Reviewed-by: Maxime Ripard <mrip...@kernel.org>
> >> > >> > Signed-off-by: Anusha Srivatsa <asriv...@redhat.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? Maxime
signature.asc
Description: PGP signature
