On Fri, May 9, 2025 at 8:45 AM Jani Nikula <jani.nik...@linux.intel.com>
wrote:
> On Fri, 09 May 2025, Maxime Ripard <mrip...@kernel.org> wrote:
> > 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?
>
> 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. 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.
>
> 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.
>
> 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, instead of trying to force everyone into
> the same model, whether it makes sense or not.
>
> The point was to actually make it easier and less prone to bugs and not
the other way around.
Trying to understand the changes needed to make this work for i915 usecase.
I get that embedding drm_panel within intel_panel which is embedded
within intel_connector, which also embeds drm_connector and so on can
get confusing to use. But Jani,
about drm_panel_follower , which is the panel that is following the
intel_panel? Is it the associated
touchscreen if any or some other device?
Anusha
>
> BR,
> Jani.
>
> --
> Jani Nikula, Intel
>
>
