[PATCH v4 0/4] drm/panel: Panel Refcounting infrastructure

[PATCH v4 0/4] drm/panel: Panel Refcounting infrastructure

[Anusha Srivatsa]

This series adds the infrastructure needed for the refcounting
allocations for panels similar to Luca's efforts with bridges.
Underlying intention and idea is the same - avoid use-after-free
situations in panels. Get reference to panel when in use and put
the reference back (down) when not in use.
Once this gets approved, rest of the drivers will have to be
mass converted to use this API.  All the callers of of_drm_find_panel()
will have to be converted too.

Tried to split the patches as suggested in the RFC series[1].
Also fixed the connector used during panel_init to be the one
passed by driver.

Patch 4 was not suggested or part of my initial work. Added it
after looking at the comments Luca's v8 of the bridge series
received.[2]

[1] -> https://patchwork.freedesktop.org/series/146236/
[2] -> https://patchwork.freedesktop.org/series/146306/#rev2

Suggested-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>
---
Changes in v4:
- Move refcounting documentation from Patch 1 to Patch 2 
- Link to v3: https://lore.kernel.org/r/20250330-b4-panel-refcounting-v3-0-0e0d4e4641eb@redhat.com

Changes in v3:
- Move the include from patch 1 to patch 2 where it is actually used
- Move the refcounting documentation out from the returns section to the
  actual helper socumentation.
- Code style changes. Move the version changes after the s-o-b.
- Link to v2: https://lore.kernel.org/r/20250327-b4-panel-refcounting-v2-0-b5f5ca551f95@redhat.com

Changes in v2:
- Change drm_panel_put() to return void.
- Export drm_panel_get()/put()
- Code cleanups: add missing return documentation, improve documentation
  in commit logs. 
- Link to v1: https://lore.kernel.org/r/20250325-b4-panel-refcounting-v1-0-4e2bf5d19c5d@redhat.com

---
Anusha Srivatsa (4):
      drm/panel: Add new helpers for refcounted panel allocatons
      drm/panel: Add refcount support
      drm/panel: deprecate old-style panel allocation
      drm/panel/panel-simple: Use the new allocation in place of devm_kzalloc()

 drivers/gpu/drm/drm_panel.c          | 92 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/panel/panel-simple.c |  9 ++--
 include/drm/drm_panel.h              | 41 ++++++++++++++++
 3 files changed, 135 insertions(+), 7 deletions(-)
---
base-commit: 372a9ca3c1f2ea10dd05a5d5008d055bc9536ced
change-id: 20250324-b4-panel-refcounting-40ab56aa34f7

Best regards,
-- 
Anusha Srivatsa <asrivats@redhat.com>

[PATCH v4 1/4] drm/panel: Add new helpers for refcounted panel allocatons

[Anusha Srivatsa]

Introduce reference counted allocations for panels to avoid
use-after-free. The patch adds the macro devm_drm_bridge_alloc()
to allocate a new refcounted panel. Followed the documentation for
drmm_encoder_alloc() and devm_drm_dev_alloc and other similar
implementations for this purpose.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>

---
v4: Remove documentation about refcounting in this patch, it is
not used yet.

v3: Remove include. Fix typos. Code style corrections (Luca)
- Move the documentation to the main helper instead of in returns
  (Maxime)

v2: Better documentation for connector_type field - follow drm_panel_init
documentation. (Luca)
- Clarify the refcount initialisation in comments.(Maxime)
- Correct the documentation of the return type (Maxime)
---
 drivers/gpu/drm/drm_panel.c | 25 +++++++++++++++++++++++++
 include/drm/drm_panel.h     | 22 ++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index c627e42a7ce70459f50eb5095fffc806ca45dabf..bdeab5710ee324dc1742fbc77582250960556308 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -355,6 +355,31 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
 }
 EXPORT_SYMBOL(of_drm_find_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;
+
+	if (!funcs) {
+		dev_warn(dev, "Missing funcs pointer\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	container = devm_kzalloc(dev, size, GFP_KERNEL);
+	if (!container)
+		return ERR_PTR(-ENOMEM);
+
+	panel = container + offset;
+	panel->funcs = funcs;
+
+	drm_panel_init(panel, dev, funcs, connector_type);
+
+	return container;
+}
+EXPORT_SYMBOL(__devm_drm_panel_alloc);
+
 /**
  * of_drm_get_panel_orientation - look up the orientation of the panel through
  * the "rotation" binding from a device tree node
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index a9c042c8dea1a82ef979c7a68204e0b55483fc28..415e85e8b76a15679f59c944ea152367dc3e0488 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -268,6 +268,28 @@ struct drm_panel {
 	bool enabled;
 };
 
+void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
+			     const struct drm_panel_funcs *funcs,
+			     int connector_type);
+
+/**
+ * devm_drm_panel_alloc - Allocate and initialize a refcounted panel.
+ *
+ * @dev: struct device of the panel device
+ * @type: the type of the struct which contains struct &drm_panel
+ * @member: the name of the &drm_panel within @type
+ * @funcs: callbacks for this panel
+ * @connector_type: the connector type (DRM_MODE_CONNECTOR_*) corresponding to
+ * the panel interface
+ *
+ * Returns:
+ * Pointer to container structure embedding the panel, ERR_PTR on failure.
+ */
+#define devm_drm_panel_alloc(dev, type, member, funcs, connector_type) \
+	((type *)__devm_drm_panel_alloc(dev, sizeof(type), \
+					offsetof(type, member), funcs, \
+					connector_type))
+
 void drm_panel_init(struct drm_panel *panel, struct device *dev,
 		    const struct drm_panel_funcs *funcs,
 		    int connector_type);

-- 
2.48.1

[PATCH v4 2/4] drm/panel: Add refcount support

[Anusha Srivatsa]

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@bootlin.com>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Anusha Srivatsa <asrivats@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);
+
+	err = devm_add_action_or_reset(dev, drm_panel_put_void, panel);
+	if (err)
+		return ERR_PTR(err);
 
 	drm_panel_init(panel, dev, funcs, connector_type);
 
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 415e85e8b76a15679f59c944ea152367dc3e0488..31d84f901c514c93ab6cbc09f445853ef897bc95 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -28,6 +28,7 @@
 #include <linux/errno.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
+#include <linux/kref.h>
 
 struct backlight_device;
 struct dentry;
@@ -266,6 +267,17 @@ struct drm_panel {
 	 * If true then the panel has been enabled.
 	 */
 	bool enabled;
+
+	/**
+	 * @container: Pointer to the private driver struct embedding this
+	 * @struct drm_panel.
+	 */
+	void *container;
+
+	/**
+	 * @refcount: reference count of users referencing this panel.
+	 */
+	struct kref refcount;
 };
 
 void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
@@ -282,6 +294,10 @@ void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
  * @connector_type: the connector type (DRM_MODE_CONNECTOR_*) corresponding to
  * the panel interface
  *
+ * The reference count of the returned panel is initialized to 1. This
+ * reference will be automatically dropped via devm (by calling
+ * drm_panel_put()) when @dev is removed.
+ *
  * Returns:
  * Pointer to container structure embedding the panel, ERR_PTR on failure.
  */
@@ -294,6 +310,9 @@ void drm_panel_init(struct drm_panel *panel, struct device *dev,
 		    const struct drm_panel_funcs *funcs,
 		    int connector_type);
 
+struct drm_panel *drm_panel_get(struct drm_panel *panel);
+void drm_panel_put(struct drm_panel *panel);
+
 void drm_panel_add(struct drm_panel *panel);
 void drm_panel_remove(struct drm_panel *panel);
 

-- 
2.48.1

[PATCH v4 3/4] drm/panel: deprecate old-style panel allocation

[Anusha Srivatsa]

Start moving to the new refcounted allocations using
the new API devm_drm_panel_alloc(). Deprecate any other
allocation.

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>

---
v4: none.
v3: none.
v2: make the documentation changes in v1 more precise (Maxime)
---
 drivers/gpu/drm/drm_panel.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 7b17531d85a4dc3031709919564d2e4d8332f748..870bf8d471ee9c5fe65d88acc13661cacd883b9b 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -74,8 +74,9 @@ EXPORT_SYMBOL(drm_panel_init);
  * drm_panel_add - add a panel to the global registry
  * @panel: panel to add
  *
- * Add a panel to the global registry so that it can be looked up by display
- * drivers.
+ * Add a panel to the global registry so that it can be looked
+ * up by display drivers. The panel to be added must have been
+ * allocated by devm_drm_panel_alloc().
  */
 void drm_panel_add(struct drm_panel *panel)
 {

-- 
2.48.1

[PATCH v4 4/4] drm/panel/panel-simple: Use the new allocation in place of devm_kzalloc()

[Anusha Srivatsa]

Start using the new helper that does the refcounted
allocations.

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Anusha Srivatsa <asrivats@redhat.com>

---
v4: none.
v3: none.
v2: check error condition (Luca)
---
 drivers/gpu/drm/panel/panel-simple.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 6ba600f97aa4c8daae577823fcf17ef31b0eb46f..df718c4a86cb7dc0cd126e807d33306e5a21d8a0 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -579,9 +579,10 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 	u32 bus_flags;
 	int err;
 
-	panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
-	if (!panel)
-		return -ENOMEM;
+	panel = devm_drm_panel_alloc(dev, struct panel_simple, base,
+				     &panel_simple_funcs, desc->connector_type);
+	if (IS_ERR(panel))
+		return PTR_ERR(panel);
 
 	panel->desc = desc;
 
@@ -694,8 +695,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 	pm_runtime_set_autosuspend_delay(dev, 1000);
 	pm_runtime_use_autosuspend(dev);
 
-	drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
-
 	err = drm_panel_of_backlight(&panel->base);
 	if (err) {
 		dev_err_probe(dev, err, "Could not find backlight\n");

-- 
2.48.1

Re: [PATCH v4 0/4] drm/panel: Panel Refcounting infrastructure

[Maxime Ripard]

On Mon, 31 Mar 2025 11:15:24 -0400, Anusha Srivatsa wrote:
> This series adds the infrastructure needed for the refcounting
> allocations for panels similar to Luca's efforts with bridges.
> Underlying intention and idea is the same - avoid use-after-free
> situations in panels. Get reference to panel when in use and put
> the reference back (down) when not in use.
> Once this gets approved, rest of the drivers will have to be
> mass converted to use this API.  All the callers of of_drm_find_panel()
> will have to be converted too.
> 
> [...]

Applied to misc/kernel.git (drm-misc-next).

Thanks!
Maxime

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

On Mon, 31 Mar 2025, Anusha Srivatsa <asrivats@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.ceresoli@bootlin.com>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Anusha Srivatsa <asrivats@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. 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.


BR,
Jani.


> +
> +	err = devm_add_action_or_reset(dev, drm_panel_put_void, panel);
> +	if (err)
> +		return ERR_PTR(err);
>  
>  	drm_panel_init(panel, dev, funcs, connector_type);
>  
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 415e85e8b76a15679f59c944ea152367dc3e0488..31d84f901c514c93ab6cbc09f445853ef897bc95 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -28,6 +28,7 @@
>  #include <linux/errno.h>
>  #include <linux/list.h>
>  #include <linux/mutex.h>
> +#include <linux/kref.h>
>  
>  struct backlight_device;
>  struct dentry;
> @@ -266,6 +267,17 @@ struct drm_panel {
>  	 * If true then the panel has been enabled.
>  	 */
>  	bool enabled;
> +
> +	/**
> +	 * @container: Pointer to the private driver struct embedding this
> +	 * @struct drm_panel.
> +	 */
> +	void *container;
> +
> +	/**
> +	 * @refcount: reference count of users referencing this panel.
> +	 */
> +	struct kref refcount;
>  };
>  
>  void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
> @@ -282,6 +294,10 @@ void *__devm_drm_panel_alloc(struct device *dev, size_t size, size_t offset,
>   * @connector_type: the connector type (DRM_MODE_CONNECTOR_*) corresponding to
>   * the panel interface
>   *
> + * The reference count of the returned panel is initialized to 1. This
> + * reference will be automatically dropped via devm (by calling
> + * drm_panel_put()) when @dev is removed.
> + *
>   * Returns:
>   * Pointer to container structure embedding the panel, ERR_PTR on failure.
>   */
> @@ -294,6 +310,9 @@ void drm_panel_init(struct drm_panel *panel, struct device *dev,
>  		    const struct drm_panel_funcs *funcs,
>  		    int connector_type);
>  
> +struct drm_panel *drm_panel_get(struct drm_panel *panel);
> +void drm_panel_put(struct drm_panel *panel);
> +
>  void drm_panel_add(struct drm_panel *panel);
>  void drm_panel_remove(struct drm_panel *panel);

-- 
Jani Nikula, Intel

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4958 bytes --]

Hi Jani,

On Mon, Apr 28, 2025 at 07:31:50PM +0300, Jani Nikula wrote:
> On Mon, 31 Mar 2025, Anusha Srivatsa <asrivats@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.ceresoli@bootlin.com>
> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > Signed-off-by: Anusha Srivatsa <asrivats@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?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

On Tue, 29 Apr 2025, Maxime Ripard <mripard@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 <asrivats@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.ceresoli@bootlin.com>
>> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
>> > Signed-off-by: Anusha Srivatsa <asrivats@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...

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?

BR,
Jani.



-- 
Jani Nikula, Intel

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6654 bytes --]

Hi Jani,

On Tue, Apr 29, 2025 at 12:22:00PM +0300, Jani Nikula wrote:
> On Tue, 29 Apr 2025, Maxime Ripard <mripard@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 <asrivats@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.ceresoli@bootlin.com>
> >> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> >> > Signed-off-by: Anusha Srivatsa <asrivats@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.

> 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() :)

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Anusha Srivatsa]

[-- Attachment #1: Type: text/plain, Size: 7695 bytes --]

On Mon, May 5, 2025 at 2:54 AM Maxime Ripard <mripard@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 <mripard@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 <asrivats@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.ceresoli@bootlin.com>
> > >> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > >> > Signed-off-by: Anusha Srivatsa <asrivats@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.
>
> > 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() :)
>
>
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.


Anusha

> Maxime
>

[-- Attachment #2: Type: text/html, Size: 10929 bytes --]

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

On Mon, 05 May 2025, Anusha Srivatsa <asrivats@redhat.com> wrote:
> On Mon, May 5, 2025 at 2:54 AM Maxime Ripard <mripard@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 <mripard@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 <asrivats@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.ceresoli@bootlin.com>
>> > >> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
>> > >> > Signed-off-by: Anusha Srivatsa <asrivats@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.

>> > 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

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Anusha Srivatsa]

[-- Attachment #1: Type: text/plain, Size: 9640 bytes --]

On Thu, May 8, 2025 at 10:27 AM Jani Nikula <jani.nikula@linux.intel.com>
wrote:

> On Mon, 05 May 2025, Anusha Srivatsa <asrivats@redhat.com> wrote:
> > On Mon, May 5, 2025 at 2:54 AM Maxime Ripard <mripard@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 <mripard@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 <asrivats@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.ceresoli@bootlin.com>
> >> > >> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> >> > >> > Signed-off-by: Anusha Srivatsa <asrivats@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.
>
> >> > 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.
>
> Jani,
will intel_connector_init() be one of the code points where this will be
inflexible?

Anusha

> 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
>
>

[-- Attachment #2: Type: text/html, Size: 14415 bytes --]

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

On Thu, 08 May 2025, Anusha Srivatsa <asrivats@redhat.com> wrote:
> On Thu, May 8, 2025 at 10:27 AM Jani Nikula <jani.nikula@linux.intel.com>
> wrote:
>
>> On Mon, 05 May 2025, Anusha Srivatsa <asrivats@redhat.com> wrote:
>> > On Mon, May 5, 2025 at 2:54 AM Maxime Ripard <mripard@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 <mripard@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 <asrivats@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.ceresoli@bootlin.com>
>> >> > >> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
>> >> > >> > Signed-off-by: Anusha Srivatsa <asrivats@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.
>>
>> >> > 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.
>>
>> Jani,
> will intel_connector_init() be one of the code points where this will be
> inflexible?

Yes, imagine embedding drm_panel within intel_panel which is embedded
within intel_connector, which also embeds drm_connector...

Moreover, intel_connector is allocated using kzalloc(), and freed using
drm_connector_funcs ->destroy callback. I would also have an uneasy
feeling about having pointer members within intel_panel/intel_connector
that point to objects with a different lifetime (based on devm) than the
struct itself.

BR,
Jani.


>
> Anusha
>
>> 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
>>
>>

-- 
Jani Nikula, Intel

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 8324 bytes --]

On Thu, May 08, 2025 at 05:27:21PM +0300, Jani Nikula wrote:
> On Mon, 05 May 2025, Anusha Srivatsa <asrivats@redhat.com> wrote:
> > On Mon, May 5, 2025 at 2:54 AM Maxime Ripard <mripard@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 <mripard@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 <asrivats@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.ceresoli@bootlin.com>
> >> > >> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> >> > >> > Signed-off-by: Anusha Srivatsa <asrivats@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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

On Fri, 09 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> On Thu, May 08, 2025 at 05:27:21PM +0300, Jani Nikula wrote:
>> On Mon, 05 May 2025, Anusha Srivatsa <asrivats@redhat.com> wrote:
>> > On Mon, May 5, 2025 at 2:54 AM Maxime Ripard <mripard@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 <mripard@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 <asrivats@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.ceresoli@bootlin.com>
>> >> > >> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
>> >> > >> > Signed-off-by: Anusha Srivatsa <asrivats@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.


BR,
Jani.

-- 
Jani Nikula, Intel

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Anusha Srivatsa]

[-- Attachment #1: Type: text/plain, Size: 10877 bytes --]

On Fri, May 9, 2025 at 8:45 AM Jani Nikula <jani.nikula@linux.intel.com>
wrote:

> On Fri, 09 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> > On Thu, May 08, 2025 at 05:27:21PM +0300, Jani Nikula wrote:
> >> On Mon, 05 May 2025, Anusha Srivatsa <asrivats@redhat.com> wrote:
> >> > On Mon, May 5, 2025 at 2:54 AM Maxime Ripard <mripard@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 <mripard@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 <asrivats@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.ceresoli@bootlin.com>
> >> >> > >> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> >> >> > >> > Signed-off-by: Anusha Srivatsa <asrivats@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
>
>

[-- Attachment #2: Type: text/html, Size: 16333 bytes --]

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

On Mon, 12 May 2025, Anusha Srivatsa <asrivats@redhat.com> wrote:
> 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?

Touchscreen, it's stuff under investigation, WIP. Needs a bunch more
stuff, but just trying to ensure drm_panel doesn't become a blocker
right off the bat.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 11030 bytes --]

On Fri, May 09, 2025 at 03:45:36PM +0300, Jani Nikula wrote:
> On Fri, 09 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> > On Thu, May 08, 2025 at 05:27:21PM +0300, Jani Nikula wrote:
> >> On Mon, 05 May 2025, Anusha Srivatsa <asrivats@redhat.com> wrote:
> >> > On Mon, May 5, 2025 at 2:54 AM Maxime Ripard <mripard@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 <mripard@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 <asrivats@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.ceresoli@bootlin.com>
> >> >> > >> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> >> >> > >> > Signed-off-by: Anusha Srivatsa <asrivats@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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

On Tue, 13 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> 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?

The opposite is also true.

If you create a design that does not cleanly fit the model of the
biggest drivers in the subsystem, and expect massive refactors just for
the sake of conforming to the design to be able to use any of it, you'll
also get pushback.

> This one is usable, but you rule out the way you could use it.

I think you're off-hand and completely dismissing the amount of work it
would be. And still I'm not even ruling it out, but there has to be a
way to start off in small incremental steps, and use the parts that
work. And it's not like we're averse to refactoring in the least,
everyone knows that.

> 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.

I pointed out what I think is a bug in drm_panel, with nothing but good
intentions, and everything snowballed from there.

There has to be a middle ground instead of absolutes. Otherwise we'll
just end up in deeper silos. And more arguments.

BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Anusha Srivatsa]

[-- Attachment #1: Type: text/plain, Size: 1923 bytes --]

On Wed, May 14, 2025 at 5:22 AM Jani Nikula <jani.nikula@linux.intel.com>
wrote:

> On Tue, 13 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> > 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?
>
> The opposite is also true.
>
> If you create a design that does not cleanly fit the model of the
> biggest drivers in the subsystem, and expect massive refactors just for
> the sake of conforming to the design to be able to use any of it, you'll
> also get pushback.
>
> > This one is usable, but you rule out the way you could use it.
>
> I think you're off-hand and completely dismissing the amount of work it
> would be. And still I'm not even ruling it out, but there has to be a
> way to start off in small incremental steps, and use the parts that
> work. And it's not like we're averse to refactoring in the least,
> everyone knows that.
>
> > 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.
>
> I pointed out what I think is a bug in drm_panel, with nothing but good
> intentions, and everything snowballed from there.
>
> There has to be a middle ground instead of absolutes. Otherwise we'll
> just end up in deeper silos. And more arguments.
>
> BR,
> Jani.
>
>
Jani, Maxime,

Thinking out loud of different solutions we can have to make sure we take
this forward.

Is it possible to have a variant of drm_panel_follower for the non ARM
devices? That way if at any point in
the future, the drm_panel_follower infrastructure has to be used, the
refcounting allocation can be bypassed?

Adding Uma and VIlle to the thread here.

Thanks!
Anusha


> --
> Jani Nikula, Intel
>
>

[-- Attachment #2: Type: text/html, Size: 2726 bytes --]

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

On Fri, 16 May 2025, Anusha Srivatsa <asrivats@redhat.com> wrote:
> On Wed, May 14, 2025 at 5:22 AM Jani Nikula <jani.nikula@linux.intel.com>
> wrote:
>
>> On Tue, 13 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
>> > 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?
>>
>> The opposite is also true.
>>
>> If you create a design that does not cleanly fit the model of the
>> biggest drivers in the subsystem, and expect massive refactors just for
>> the sake of conforming to the design to be able to use any of it, you'll
>> also get pushback.
>>
>> > This one is usable, but you rule out the way you could use it.
>>
>> I think you're off-hand and completely dismissing the amount of work it
>> would be. And still I'm not even ruling it out, but there has to be a
>> way to start off in small incremental steps, and use the parts that
>> work. And it's not like we're averse to refactoring in the least,
>> everyone knows that.
>>
>> > 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.
>>
>> I pointed out what I think is a bug in drm_panel, with nothing but good
>> intentions, and everything snowballed from there.
>>
>> There has to be a middle ground instead of absolutes. Otherwise we'll
>> just end up in deeper silos. And more arguments.
>>
>> BR,
>> Jani.
>>
>>
> Jani, Maxime,
>
> Thinking out loud of different solutions we can have to make sure we take
> this forward.
>
> Is it possible to have a variant of drm_panel_follower for the non ARM
> devices? That way if at any point in
> the future, the drm_panel_follower infrastructure has to be used, the
> refcounting allocation can be bypassed?

Please let's not conflate two orthogonal matters. Refcounting or
allocation is not related to platforms in any way. I see no reason to
have that kind of dependency. It would just complicate matters more.


BR,
Jani.


>
> Adding Uma and VIlle to the thread here.
>
> Thanks!
> Anusha
>
>
>> --
>> Jani Nikula, Intel
>>
>>

-- 
Jani Nikula, Intel

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2923 bytes --]

On Wed, May 14, 2025 at 12:22:40PM +0300, Jani Nikula wrote:
> On Tue, 13 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> > 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?
> 
> The opposite is also true.
> 
> If you create a design that does not cleanly fit the model of the
> biggest drivers in the subsystem, and expect massive refactors just for
> the sake of conforming to the design to be able to use any of it, you'll
> also get pushback.

That's the thing though: i915 deviates pretty heavily from the helpers,
in general. If it wants to consume them, then it must also be ready to
make some adjustments. Or just roll its own thing like it has done in
the past.

Otherwise, where do we draw the line when anyone isn't happy with the
helpers in general and ask for an exception because reworking the driver
is too much work?

We did this on pretty much every ARM platform, we did this for amdgpu,
but somehow i915 should get a pass?

On what ground? What should we tell the next driver that uses the same
argument?

> > This one is usable, but you rule out the way you could use it.
> 
> I think you're off-hand and completely dismissing the amount of work it
> would be. And still I'm not even ruling it out, but there has to be a
> way to start off in small incremental steps, and use the parts that
> work. And it's not like we're averse to refactoring in the least,
> everyone knows that.

I'm not sure how pointing fingers at who has the right design,
overlooked some hypothetical usages 10y down the line, or is being
offhand helps the conversation in any way.

I've been asking questions to understand what you could work with, and
some are still left unanswered, and had to ask others multiple times to
get an answer.

And maybe I do indeed underestimate the amount of work it would take. I
don't believe so, and I still believe that it wouldn't be too hard. But
maybe you're right. You still haven't explained why it would take
anything more than registering a dumb device at probe time though.

> > 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.
> 
> I pointed out what I think is a bug in drm_panel, with nothing but good
> intentions, and everything snowballed from there.
> 
> There has to be a middle ground instead of absolutes. Otherwise we'll
> just end up in deeper silos. And more arguments.

I believe calling drm_panel_init would be a middle ground, if we add a
bunch of warnings, checks here and there and move things around a bit.
But to keep it, we do need a good reason for it, even more so if we
don't have any in-tree users anymore.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

Maxime -

I'm cutting a lot of context here. Not because I don't think it deserves
an answer, but because I seem to be failing at communication.

On Mon, 19 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> You still haven't explained why it would take anything more than
> registering a dumb device at probe time though.

With that, do you mean a dumb struct device, or any struct device with a
suitable lifetime, that we'd pass to devm_drm_panel_alloc()?

Is using devm_drm_panel_alloc() like that instead of our own allocation
with drm_panel_init() the main point of contention for you? If yes, we
can do that.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

On Tue, 20 May 2025, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> Maxime -
>
> I'm cutting a lot of context here. Not because I don't think it deserves
> an answer, but because I seem to be failing at communication.
>
> On Mon, 19 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
>> You still haven't explained why it would take anything more than
>> registering a dumb device at probe time though.
>
> With that, do you mean a dumb struct device, or any struct device with a
> suitable lifetime, that we'd pass to devm_drm_panel_alloc()?

I'm no expert in ACPI, but I think it needs to be a struct device
embedded inside struct acpi_device to implement the
drm_panel_add_follower() lookup for ACPI.

It would be natural to embed struct drm_panel inside struct intel_panel,
except we need struct intel_panel way before we have figured out the
acpi device. We need struct intel_panel at connector register time, acpi
devices currently get figured out after all connectors have been
registered. I'm trying to see if we can change that, but it doesn't look
easy. Separate allocation and initialization would cover that.

> Is using devm_drm_panel_alloc() like that instead of our own allocation
> with drm_panel_init() the main point of contention for you? If yes, we
> can do that.

As devm_drm_panel_alloc() forces embedding, and we can't easily embed
drm_panel inside intel_panel, even that would need a dummy wrapper
struct.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

On Tue, May 20, 2025 at 01:09:47PM +0300, Jani Nikula wrote:
> 
> Maxime -
> 
> I'm cutting a lot of context here. Not because I don't think it deserves
> an answer, but because I seem to be failing at communication.
> 
> On Mon, 19 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> > You still haven't explained why it would take anything more than
> > registering a dumb device at probe time though.
> 
> With that, do you mean a dumb struct device, or any struct device with a
> suitable lifetime, that we'd pass to devm_drm_panel_alloc()?
> 
> Is using devm_drm_panel_alloc() like that instead of our own allocation
> with drm_panel_init() the main point of contention for you? If yes, we
> can do that.

Yeah, I was thinking of something along the lines of:

const struct drm_panel_funcs dummy_funcs = {};

struct drm_panel *register_panel() {
    struct faux_device *faux;
    struct drm_panel *panel;
    int ret;

    faux = faux_device_create(...);
    if IS_ERR(faux)
       return ERR_CAST(faux);

    return __devm_drm_panel_alloc(&faux->dev, sizeof(*panel), 0, &dummy_funcs, $CONNECTOR_TYPE);
}

And you have a panel, under your control, with exactly the same
setup than anyone else.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]

Hi,

On Fri, May 23, 2025 at 02:34:05PM +0300, Jani Nikula wrote:
> On Tue, 20 May 2025, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > Maxime -
> >
> > I'm cutting a lot of context here. Not because I don't think it deserves
> > an answer, but because I seem to be failing at communication.
> >
> > On Mon, 19 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> >> You still haven't explained why it would take anything more than
> >> registering a dumb device at probe time though.
> >
> > With that, do you mean a dumb struct device, or any struct device with a
> > suitable lifetime, that we'd pass to devm_drm_panel_alloc()?
> 
> I'm no expert in ACPI, but I think it needs to be a struct device
> embedded inside struct acpi_device to implement the
> drm_panel_add_follower() lookup for ACPI.

What data do you have in the ACPI tables to associate the HID
touchscreen to the panel?

drm_panel_add_follower use the DT to lookup the panel, so the panel must
have a device->of_node pointer, so that obviously won't work, but it
might with ACPI, or we might need to split that function into several
parts to accomodate ACPI.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

On Tue, 27 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> Hi,
>
> On Fri, May 23, 2025 at 02:34:05PM +0300, Jani Nikula wrote:
>> On Tue, 20 May 2025, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> > Maxime -
>> >
>> > I'm cutting a lot of context here. Not because I don't think it deserves
>> > an answer, but because I seem to be failing at communication.
>> >
>> > On Mon, 19 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
>> >> You still haven't explained why it would take anything more than
>> >> registering a dumb device at probe time though.
>> >
>> > With that, do you mean a dumb struct device, or any struct device with a
>> > suitable lifetime, that we'd pass to devm_drm_panel_alloc()?
>> 
>> I'm no expert in ACPI, but I think it needs to be a struct device
>> embedded inside struct acpi_device to implement the
>> drm_panel_add_follower() lookup for ACPI.
>
> What data do you have in the ACPI tables to associate the HID
> touchscreen to the panel?
>
> drm_panel_add_follower use the DT to lookup the panel, so the panel must
> have a device->of_node pointer, so that obviously won't work, but it
> might with ACPI, or we might need to split that function into several
> parts to accomodate ACPI.

Personally, I'm pretty clueless about ACPI, but apparently it's possible
to add a _DSD to reference the panel.

And something like [1] could work to make drm_panel lookup agnostic to
DT/ACPI, but it's completely untested and unreviewed as of today. We'll
also need to find someone to help ensure that doesn't break DT systems.

Feedback is welcome, but please bear in mind I'm not even confident
enough to post it on dri-devel yet.


BR,
Jani.


[1] https://gitlab.freedesktop.org/jani/linux/-/commit/8ca2e1f25ca94c4ee094915a26781f9d9ace37af


-- 
Jani Nikula, Intel

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

On Tue, 27 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> On Tue, May 20, 2025 at 01:09:47PM +0300, Jani Nikula wrote:
>> 
>> Maxime -
>> 
>> I'm cutting a lot of context here. Not because I don't think it deserves
>> an answer, but because I seem to be failing at communication.
>> 
>> On Mon, 19 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
>> > You still haven't explained why it would take anything more than
>> > registering a dumb device at probe time though.
>> 
>> With that, do you mean a dumb struct device, or any struct device with a
>> suitable lifetime, that we'd pass to devm_drm_panel_alloc()?
>> 
>> Is using devm_drm_panel_alloc() like that instead of our own allocation
>> with drm_panel_init() the main point of contention for you? If yes, we
>> can do that.
>
> Yeah, I was thinking of something along the lines of:
>
> const struct drm_panel_funcs dummy_funcs = {};
>
> struct drm_panel *register_panel() {
>     struct faux_device *faux;
>     struct drm_panel *panel;
>     int ret;
>
>     faux = faux_device_create(...);
>     if IS_ERR(faux)
>        return ERR_CAST(faux);
>
>     return __devm_drm_panel_alloc(&faux->dev, sizeof(*panel), 0, &dummy_funcs, $CONNECTOR_TYPE);
> }
>
> And you have a panel, under your control, with exactly the same
> setup than anyone else.

This [1] is what I'm toying with now, but again, draft stuff. Using
__devm_drm_panel_alloc() directly like above does make it cleaner.

Long term it can be improved, but my first dab at refactoring to make
that happen is already like 15-20 patches, and it'll just have to wait
until after making stuff work at all first.

I'm not sure if the ACPI device I'm passing to devm_drm_panel_alloc() is
correct, but it'll have to be *some* ACPI device for the lookup to
work. I am blissfully ignorant about its lifetime, but as long as
drm_panel_add() and drm_panel_remove() remain as they are, I don't think
it leaks anything. Fingers crossed.


BR,
Jani.


[1] https://gitlab.freedesktop.org/jani/linux/-/commit/241f21487e5e9a8fa72e37a8eebcc36099e6a1ee

-- 
Jani Nikula, Intel

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2365 bytes --]

On Tue, May 27, 2025 at 10:40:49PM +0300, Jani Nikula wrote:
> On Tue, 27 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> > On Tue, May 20, 2025 at 01:09:47PM +0300, Jani Nikula wrote:
> >> 
> >> Maxime -
> >> 
> >> I'm cutting a lot of context here. Not because I don't think it deserves
> >> an answer, but because I seem to be failing at communication.
> >> 
> >> On Mon, 19 May 2025, Maxime Ripard <mripard@kernel.org> wrote:
> >> > You still haven't explained why it would take anything more than
> >> > registering a dumb device at probe time though.
> >> 
> >> With that, do you mean a dumb struct device, or any struct device with a
> >> suitable lifetime, that we'd pass to devm_drm_panel_alloc()?
> >> 
> >> Is using devm_drm_panel_alloc() like that instead of our own allocation
> >> with drm_panel_init() the main point of contention for you? If yes, we
> >> can do that.
> >
> > Yeah, I was thinking of something along the lines of:
> >
> > const struct drm_panel_funcs dummy_funcs = {};
> >
> > struct drm_panel *register_panel() {
> >     struct faux_device *faux;
> >     struct drm_panel *panel;
> >     int ret;
> >
> >     faux = faux_device_create(...);
> >     if IS_ERR(faux)
> >        return ERR_CAST(faux);
> >
> >     return __devm_drm_panel_alloc(&faux->dev, sizeof(*panel), 0, &dummy_funcs, $CONNECTOR_TYPE);
> > }
> >
> > And you have a panel, under your control, with exactly the same
> > setup than anyone else.
> 
> This [1] is what I'm toying with now, but again, draft stuff. Using
> __devm_drm_panel_alloc() directly like above does make it cleaner.
> 
> Long term it can be improved, but my first dab at refactoring to make
> that happen is already like 15-20 patches, and it'll just have to wait
> until after making stuff work at all first.
> 
> I'm not sure if the ACPI device I'm passing to devm_drm_panel_alloc() is
> correct, but it'll have to be *some* ACPI device for the lookup to
> work. I am blissfully ignorant about its lifetime, but as long as
> drm_panel_add() and drm_panel_remove() remain as they are, I don't think
> it leaks anything. Fingers crossed.

Thanks for working on that. Your two patches (the one you sent here, and
the one in the other subthread) look good to me. So if that works, it
looks like we have a way forward.

Thanks!
Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v4 2/4] drm/panel: Add refcount support

[Jani Nikula]

On Fri, 06 Jun 2025, Maxime Ripard <mripard@kernel.org> wrote:
> Thanks for working on that. Your two patches (the one you sent here, and
> the one in the other subthread) look good to me. So if that works, it
> looks like we have a way forward.

Coincidentally, I just posted the first non-draft patches [1]. Thanks
for your feedback, and for bearing with me.


BR,
Jani.


[1] https://lore.kernel.org/r/cover.1749199013.git.jani.nikula@intel.com


-- 
Jani Nikula, Intel