<div dir="ltr"><div dir="ltr"> </div> <div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Thu, May 8, 2025 at 10:27 AM Jani Nikula <<a href="mailto:jani.nikula@linux.intel.com">jani.nikula@linux.intel.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Mon, 05 May 2025, Anusha Srivatsa <<a href="mailto:asrivats@redhat.com" target="_blank">asrivats@redhat.com</a>> wrote: > On Mon, May 5, 2025 at 2:54 AM Maxime Ripard <<a href="mailto:mripard@kernel.org" target="_blank">mripard@kernel.org</a>> wrote: > >> Hi Jani, >> >> On Tue, Apr 29, 2025 at 12:22:00PM +0300, Jani Nikula wrote: >> > On Tue, 29 Apr 2025, Maxime Ripard <<a href="mailto:mripard@kernel.org" target="_blank">mripard@kernel.org</a>> wrote: >> > > Hi Jani, >> > > >> > > On Mon, Apr 28, 2025 at 07:31:50PM +0300, Jani Nikula wrote: >> > >> On Mon, 31 Mar 2025, Anusha Srivatsa <<a href="mailto:asrivats@redhat.com" target="_blank">asrivats@redhat.com</a>> 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 <<a href="mailto:luca.ceresoli@bootlin.com" target="_blank">luca.ceresoli@bootlin.com</a>> >> > >> > Reviewed-by: Maxime Ripard <<a href="mailto:mripard@kernel.org" target="_blank">mripard@kernel.org</a>> >> > >> > Signed-off-by: Anusha Srivatsa <<a href="mailto:asrivats@redhat.com" target="_blank">asrivats@redhat.com</a>> >> > >> > >> > >> > --- >> > >> > 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. </blockquote><div>Jani, </div><div>will intel_connector_init() be one of the code points where this will be inflexible?</div><div> </div><div>Anusha</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> I mean devm_drm_panel_alloc() is great, but please don't make its use mandatory! </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">> Jani, > the series that converts all drivers to use the new API: > <a href="https://patchwork.freedesktop.org/series/147082/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.org/series/147082/</a> > <a href="https://patchwork.freedesktop.org/series/147157/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.org/series/147157/</a> > <a href="https://patchwork.freedesktop.org/series/147246/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.org/series/147246/</a> > > 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 </blockquote></div></div>