[PATCH v7 0/2] drm/bridge: add docs and kunit test for devm_drm_bridge_alloc()

[PATCH v7 0/2] drm/bridge: add docs and kunit test for devm_drm_bridge_alloc()

[Luca Ceresoli]

This small series adds documentation and a simple kunit test for the new
DRM bridge allocation flow, based on the recently introduced
devm_drm_bridge_alloc() [0].

This work was formerly a part of my v6 DRM bridge hotplug series [1], now
split as a standalone series, hence the "v7" version number.

The overall goal is supporting Linux devices with a DRM pipeline whose
final components can be hot-plugged and hot-unplugged, including one or
more bridges. For more details see the big picture [0].

Current plan and status of the DRM bridge refcounting work:

 A. ✔ add new alloc API and refcounting -> (now in drm-misc-next)
 B. convert all bridge drivers to new API (v1 under review [2])
 C. ➜ documentation, kunit tests (this series)
 D. after (B), add get/put to drm_bridge_add/remove() + attach/detech()
 E. after (B), convert accessors; this is a large work and can be done
    in chunks
 F. debugfs improvements

[0] https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/0cc6aadd7fc1e629b715ea3d1ba537ef2da95eec
[1] https://lore.kernel.org/dri-devel/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/
[2] https://lore.kernel.org/lkml/20250407-drm-bridge-convert-to-alloc-api-v1-0-42113ff8d9c0@bootlin.com/

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (2):
      drm/bridge: documentat bridge allocation and lifecycle
      drm/tests: bridge: add a KUnit test for devm_drm_bridge_alloc()

 Documentation/gpu/drm-kms-helpers.rst   |  6 +++
 drivers/gpu/drm/drm_bridge.c            | 73 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tests/drm_bridge_test.c | 60 +++++++++++++++++++++++++++
 3 files changed, 139 insertions(+)
---
base-commit: e8bf4a1bdaeadb28d13b9a2bcfd5910fda06eede
change-id: 20250408-drm-bridge-alloc-doc-test-267df0def880

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH v7 1/2] drm/bridge: documentat bridge allocation and lifecycle

[Luca Ceresoli]

Document in detail the DRM bridge allocation and refcounting process based
on the recently introduced devm_drm_bridge_alloc().

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changes in v7:
 - remove mention of "legacy mode", we now support only refcounted
   bridges
 - rename patch title from "drm/bridge: add documentation of refcounted
   bridges", we now support only refcounted bridges

Changes in v6:
 - update to the new devm_drm_bridge_alloc() API
 - rewrite and improve various sentences for clarity
 - fix typos (Randy Dunlap)

This patch was added in v5.
---
 Documentation/gpu/drm-kms-helpers.rst |  6 +++
 drivers/gpu/drm/drm_bridge.c          | 73 +++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 5139705089f200b189876a5a61bf2a935cec433a..393cd0e4cb5af3fe98674e7a96c853ffb2556c97 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -151,6 +151,12 @@ Overview
 .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
    :doc: overview
 
+Bridge allocation and lifecycle
+-------------------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
+   :doc: bridge lifecycle
+
 Display Driver Integration
 --------------------------
 
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index b4c89ec01998b849018ce031c7cd84614e65e710..b7e1ad761dad52bdb2ec09d425e69ee23a18fd36 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -61,6 +61,79 @@
  * encoder chain.
  */
 
+/**
+ * DOC: bridge lifecycle
+ *
+ * In some use cases such as hot-plugging a DRM bridge device can
+ * physically disappear and reappear at runtime. To handle such cases
+ * without destroying and recreating the entire DRM pipeline, DRM bridge
+ * lifetime is managed using reference counting:
+ *
+ * - each &struct drm_bridge is reference counted since its allocation
+ * - any code taking a pointer to a bridge has APIs to get a reference and
+ *   put it when done, to ensure the memory allocated for the bridge won't
+ *   be deallocated while there is still a reference to it
+ * - the driver implementing the bridge also holds a reference, but the
+ *   allocated struct can survive the driver in case other references still
+ *   exist
+ * - deallocation is done when the last put happens, dropping the refcount
+ *   to zero
+ *
+ * Usage of refcounted bridges happens in two sides: the bridge *provider*
+ * and the bridge *consumers*. The bridge provider is the driver
+ * implementing the bridge. The bridge consumers are all parts of the
+ * kernel taking a &struct drm_bridge pointer, including other bridges,
+ * encoders and the DRM core.
+ *
+ * For bridge **providers**, the bridge driver declares a driver-specific
+ * struct embedding a &struct drm_bridge. E.g.::
+ *
+ *   struct my_bridge {
+ *       ...
+ *       struct drm_bridge bridge;
+ *       ...
+ *   };
+ *
+ * The driver must allocate and initialize ``struct my_bridge`` using
+ * devm_drm_bridge_alloc(), as in this example::
+ *
+ *     static int my_bridge_probe(...)
+ *     {
+ *         struct device *dev = ...;
+ *         struct my_bridge *mybr;
+ *
+ *         mybr = devm_drm_bridge_alloc(dev, struct my_bridge, bridge, &my_bridge_funcs);
+ *         if (IS_ERR(mybr))
+ *             return PTR_ERR(mybr);
+ *
+ *         // Get resources, initialize my_bridge members...
+ *         drm_bridge_add(&mybr->bridge);
+ *         ...
+ *     }
+ *
+ *     static void my_bridge_remove(...)
+ *     {
+ *         struct my_bridge *mybr = ...;
+ *
+ *         drm_bridge_remove(&mybr->bridge);
+ *         // Free resources
+ *         // ... NO kfree here!
+ *     }
+ *
+ * Bridge **consumers** need to handle the case of a bridge being removed
+ * while they have a pointer to it. As this can happen at any time, such
+ * code can incur in use-after-free. To avoid that, consumers have to call
+ * drm_bridge_get() when taking a pointer and drm_bridge_put() after they
+ * are done using it. This will extend the allocation lifetime of the
+ * bridge struct until the last reference has been put, potentially a long
+ * time after the bridge device has been removed from the kernel.
+ *
+ * Functions that return a pointer to a bridge, such as
+ * of_drm_find_bridge(), internally call drm_bridge_get() on the bridge
+ * they are about to return, so users using such functions to get a bridge
+ * pointer only have to take care of calling drm_bridge_put().
+ */
+
 /**
  * DOC:	display driver integration
  *

-- 
2.49.0

Re: [PATCH v7 1/2] drm/bridge: documentat bridge allocation and lifecycle

[Maxime Ripard]

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

Hi,

On Wed, Apr 09, 2025 at 04:50:34PM +0200, Luca Ceresoli wrote:
> Document in detail the DRM bridge allocation and refcounting process based
> on the recently introduced devm_drm_bridge_alloc().
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

There's a typo in your commit title.

> ---
> 
> Changes in v7:
>  - remove mention of "legacy mode", we now support only refcounted
>    bridges
>  - rename patch title from "drm/bridge: add documentation of refcounted
>    bridges", we now support only refcounted bridges
> 
> Changes in v6:
>  - update to the new devm_drm_bridge_alloc() API
>  - rewrite and improve various sentences for clarity
>  - fix typos (Randy Dunlap)
> 
> This patch was added in v5.
> ---
>  Documentation/gpu/drm-kms-helpers.rst |  6 +++
>  drivers/gpu/drm/drm_bridge.c          | 73 +++++++++++++++++++++++++++++++++++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 5139705089f200b189876a5a61bf2a935cec433a..393cd0e4cb5af3fe98674e7a96c853ffb2556c97 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -151,6 +151,12 @@ Overview
>  .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
>     :doc: overview
>  
> +Bridge allocation and lifecycle
> +-------------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> +   :doc: bridge lifecycle
> +
>  Display Driver Integration
>  --------------------------
>  
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index b4c89ec01998b849018ce031c7cd84614e65e710..b7e1ad761dad52bdb2ec09d425e69ee23a18fd36 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -61,6 +61,79 @@
>   * encoder chain.
>   */
>  
> +/**
> + * DOC: bridge lifecycle
> + *
> + * In some use cases such as hot-plugging a DRM bridge device can
> + * physically disappear and reappear at runtime. To handle such cases
> + * without destroying and recreating the entire DRM pipeline, DRM bridge
> + * lifetime is managed using reference counting:

That case doesn't exist yet, so documenting it seems a source of confusion.

> + * - each &struct drm_bridge is reference counted since its allocation
> + * - any code taking a pointer to a bridge has APIs to get a reference and
> + *   put it when done, to ensure the memory allocated for the bridge won't
> + *   be deallocated while there is still a reference to it
> + * - the driver implementing the bridge also holds a reference, but the
> + *   allocated struct can survive the driver in case other references still
> + *   exist
> + * - deallocation is done when the last put happens, dropping the refcount
> + *   to zero
> + *
> + * Usage of refcounted bridges happens in two sides: the bridge *provider*
> + * and the bridge *consumers*. The bridge provider is the driver
> + * implementing the bridge. The bridge consumers are all parts of the
> + * kernel taking a &struct drm_bridge pointer, including other bridges,
> + * encoders and the DRM core.
> + *
> + * For bridge **providers**, the bridge driver declares a driver-specific
> + * struct embedding a &struct drm_bridge. E.g.::
> + *
> + *   struct my_bridge {
> + *       ...
> + *       struct drm_bridge bridge;
> + *       ...
> + *   };
> + *
> + * The driver must allocate and initialize ``struct my_bridge`` using
> + * devm_drm_bridge_alloc(), as in this example::
> + *
> + *     static int my_bridge_probe(...)
> + *     {
> + *         struct device *dev = ...;
> + *         struct my_bridge *mybr;
> + *
> + *         mybr = devm_drm_bridge_alloc(dev, struct my_bridge, bridge, &my_bridge_funcs);
> + *         if (IS_ERR(mybr))
> + *             return PTR_ERR(mybr);
> + *
> + *         // Get resources, initialize my_bridge members...
> + *         drm_bridge_add(&mybr->bridge);
> + *         ...
> + *     }
> + *
> + *     static void my_bridge_remove(...)
> + *     {
> + *         struct my_bridge *mybr = ...;
> + *
> + *         drm_bridge_remove(&mybr->bridge);
> + *         // Free resources
> + *         // ... NO kfree here!
> + *     }

This part is already documented by drm_bridge_add(), so it's not clear
what that section brings to the table either.

> + * Bridge **consumers** need to handle the case of a bridge being removed
> + * while they have a pointer to it. As this can happen at any time, such
> + * code can incur in use-after-free. To avoid that, consumers have to call
> + * drm_bridge_get() when taking a pointer and drm_bridge_put() after they
> + * are done using it. This will extend the allocation lifetime of the
> + * bridge struct until the last reference has been put, potentially a long
> + * time after the bridge device has been removed from the kernel.

And it's kind of the same thing here. You're saying here that every
consumer absolutely needs to call drm_bridge_get() and drm_bridge_put()
on their pointer ...

> + * Functions that return a pointer to a bridge, such as
> + * of_drm_find_bridge(), internally call drm_bridge_get() on the bridge
> + * they are about to return, so users using such functions to get a bridge
> + * pointer only have to take care of calling drm_bridge_put().
> + */

... but that every function that gives you a pointer will take care of
drm_bridge_get already and (will) document that you need to call
drm_bridge_put ?

I guess my larger question is kind of an editorial one. What do you want
people to learn here that isn't in some function documentation already?
At the moment, it looks like a doc that used to be useful but got kind
of deprecated by the documentation you created on all the functions we
merged so far, or a documentation that might be useful at some point but
not quite yet. Either way, it's confusing.

Maxime

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

Re: [PATCH v7 1/2] drm/bridge: documentat bridge allocation and lifecycle

[Luca Ceresoli]

On Mon, 14 Apr 2025 17:40:46 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> Hi,
> 
> On Wed, Apr 09, 2025 at 04:50:34PM +0200, Luca Ceresoli wrote:
> > Document in detail the DRM bridge allocation and refcounting process based
> > on the recently introduced devm_drm_bridge_alloc().
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>  
> 
> There's a typo in your commit title.
> 
> > ---
> > 
> > Changes in v7:
> >  - remove mention of "legacy mode", we now support only refcounted
> >    bridges
> >  - rename patch title from "drm/bridge: add documentation of refcounted
> >    bridges", we now support only refcounted bridges
> > 
> > Changes in v6:
> >  - update to the new devm_drm_bridge_alloc() API
> >  - rewrite and improve various sentences for clarity
> >  - fix typos (Randy Dunlap)
> > 
> > This patch was added in v5.
> > ---
> >  Documentation/gpu/drm-kms-helpers.rst |  6 +++
> >  drivers/gpu/drm/drm_bridge.c          | 73 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 79 insertions(+)
> > 
> > diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> > index 5139705089f200b189876a5a61bf2a935cec433a..393cd0e4cb5af3fe98674e7a96c853ffb2556c97 100644
> > --- a/Documentation/gpu/drm-kms-helpers.rst
> > +++ b/Documentation/gpu/drm-kms-helpers.rst
> > @@ -151,6 +151,12 @@ Overview
> >  .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> >     :doc: overview
> >  
> > +Bridge allocation and lifecycle
> > +-------------------------------
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> > +   :doc: bridge lifecycle
> > +
> >  Display Driver Integration
> >  --------------------------
> >  
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index b4c89ec01998b849018ce031c7cd84614e65e710..b7e1ad761dad52bdb2ec09d425e69ee23a18fd36 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -61,6 +61,79 @@
> >   * encoder chain.
> >   */
> >  
> > +/**
> > + * DOC: bridge lifecycle
> > + *
> > + * In some use cases such as hot-plugging a DRM bridge device can
> > + * physically disappear and reappear at runtime. To handle such cases
> > + * without destroying and recreating the entire DRM pipeline, DRM bridge
> > + * lifetime is managed using reference counting:  
> 
> That case doesn't exist yet, so documenting it seems a source of confusion.

OK, I'd replace it all with:

+ * DRM bridge lifetime is managed using reference counting:  

> > + * - each &struct drm_bridge is reference counted since its allocation
> > + * - any code taking a pointer to a bridge has APIs to get a reference and
> > + *   put it when done, to ensure the memory allocated for the bridge won't
> > + *   be deallocated while there is still a reference to it
> > + * - the driver implementing the bridge also holds a reference, but the
> > + *   allocated struct can survive the driver in case other references still
> > + *   exist
> > + * - deallocation is done when the last put happens, dropping the refcount
> > + *   to zero
> > + *
> > + * Usage of refcounted bridges happens in two sides: the bridge *provider*
> > + * and the bridge *consumers*. The bridge provider is the driver
> > + * implementing the bridge. The bridge consumers are all parts of the
> > + * kernel taking a &struct drm_bridge pointer, including other bridges,
> > + * encoders and the DRM core.
> > + *
> > + * For bridge **providers**, the bridge driver declares a driver-specific
> > + * struct embedding a &struct drm_bridge. E.g.::
> > + *
> > + *   struct my_bridge {
> > + *       ...
> > + *       struct drm_bridge bridge;
> > + *       ...
> > + *   };
> > + *
> > + * The driver must allocate and initialize ``struct my_bridge`` using
> > + * devm_drm_bridge_alloc(), as in this example::
> > + *
> > + *     static int my_bridge_probe(...)
> > + *     {
> > + *         struct device *dev = ...;
> > + *         struct my_bridge *mybr;
> > + *
> > + *         mybr = devm_drm_bridge_alloc(dev, struct my_bridge, bridge, &my_bridge_funcs);
> > + *         if (IS_ERR(mybr))
> > + *             return PTR_ERR(mybr);
> > + *
> > + *         // Get resources, initialize my_bridge members...
> > + *         drm_bridge_add(&mybr->bridge);
> > + *         ...
> > + *     }
> > + *
> > + *     static void my_bridge_remove(...)
> > + *     {
> > + *         struct my_bridge *mybr = ...;
> > + *
> > + *         drm_bridge_remove(&mybr->bridge);
> > + *         // Free resources
> > + *         // ... NO kfree here!
> > + *     }  
> 
> This part is already documented by drm_bridge_add(), so it's not clear
> what that section brings to the table either.
> 
> > + * Bridge **consumers** need to handle the case of a bridge being removed
> > + * while they have a pointer to it. As this can happen at any time, such
> > + * code can incur in use-after-free. To avoid that, consumers have to call
> > + * drm_bridge_get() when taking a pointer and drm_bridge_put() after they
> > + * are done using it. This will extend the allocation lifetime of the
> > + * bridge struct until the last reference has been put, potentially a long
> > + * time after the bridge device has been removed from the kernel.  
> 
> And it's kind of the same thing here. You're saying here that every
> consumer absolutely needs to call drm_bridge_get() and drm_bridge_put()
> on their pointer ...
> 
> > + * Functions that return a pointer to a bridge, such as
> > + * of_drm_find_bridge(), internally call drm_bridge_get() on the bridge
> > + * they are about to return, so users using such functions to get a bridge
> > + * pointer only have to take care of calling drm_bridge_put().
> > + */  
> 
> ... but that every function that gives you a pointer will take care of
> drm_bridge_get already and (will) document that you need to call
> drm_bridge_put ?
> 
> I guess my larger question is kind of an editorial one. What do you want
> people to learn here that isn't in some function documentation already?
> At the moment, it looks like a doc that used to be useful but got kind
> of deprecated by the documentation you created on all the functions we
> merged so far, or a documentation that might be useful at some point but
> not quite yet. Either way, it's confusing.

When I start looking into a kernel subsystem that is new to me, I am
very happy when there is high-level, "big picture" introductory
documentation like this. Otherwise I need to learn from existing code,
with the risk of learning from drivers that not following the best
practice. That's what I tried to do here.

Of course neither you or I will need this documentation. But I suspect
you consider this not useful in general.

Do you think this patch should be removed entirely?

(no offense taken if you do, just I won't invest more time in improving
this patch if it is not going to be taken)

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH v7 2/2] drm/tests: bridge: add a KUnit test for devm_drm_bridge_alloc()

[Luca Ceresoli]

Add a basic KUnit test for the newly introduced drm_bridge_alloc().

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changed in v7:
 - rebase on current drm-misc-next, which now has a drm_bridge_test.c file
 - cleanup commit message

Changed in v6:
 - update to new devm_drm_bridge_alloc() API
 - remove drm_test_drm_bridge_put test, not straightforward to write with
   the new API and the current notification mechanism
 - do not allocate a drm_device: a bridge is allocated without one
 - rename some identifiers for easier code reading

This patch was added in v5.
---
 drivers/gpu/drm/tests/drm_bridge_test.c | 60 +++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_bridge_test.c b/drivers/gpu/drm/tests/drm_bridge_test.c
index ff88ec2e911c9cc9a718483f09d4c764f45f991a..87fb64744b67f0780457a546aba77ba945a0ce67 100644
--- a/drivers/gpu/drm/tests/drm_bridge_test.c
+++ b/drivers/gpu/drm/tests/drm_bridge_test.c
@@ -8,6 +8,7 @@
 #include <drm/drm_bridge_helper.h>
 #include <drm/drm_kunit_helpers.h>
 
+#include <kunit/device.h>
 #include <kunit/test.h>
 
 struct drm_bridge_init_priv {
@@ -407,11 +408,70 @@ static struct kunit_suite drm_bridge_helper_reset_crtc_test_suite = {
 	.test_cases = drm_bridge_helper_reset_crtc_tests,
 };
 
+struct drm_bridge_alloc_test_ctx {
+	struct device *dev;
+};
+
+/*
+ * Mimick the typical struct defined by a bridge driver, which embeds a
+ * bridge plus other fields.
+ */
+struct dummy_drm_bridge {
+	int dummy; // ensure we test non-zero @bridge offset
+	struct drm_bridge bridge;
+};
+
+static const struct drm_bridge_funcs drm_bridge_dummy_funcs = {
+};
+
+static int drm_test_bridge_alloc_init(struct kunit *test)
+{
+	struct drm_bridge_alloc_test_ctx *ctx;
+
+	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+
+	ctx->dev = kunit_device_register(test, "drm-bridge-dev");
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->dev);
+
+	test->priv = ctx;
+
+	return 0;
+}
+
+/*
+ * Test that the allocation and initialization of a bridge works as
+ * expected and doesn't report any error.
+ */
+static void drm_test_drm_bridge_alloc(struct kunit *test)
+{
+	struct drm_bridge_alloc_test_ctx *ctx = test->priv;
+	struct dummy_drm_bridge *dummy;
+
+	dummy = devm_drm_bridge_alloc(ctx->dev, struct dummy_drm_bridge, bridge,
+				      &drm_bridge_dummy_funcs);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy);
+}
+
+static struct kunit_case drm_bridge_alloc_tests[] = {
+	KUNIT_CASE(drm_test_drm_bridge_alloc),
+	{ }
+};
+
+static struct kunit_suite drm_bridge_alloc_test_suite = {
+	.name = "drm_bridge_alloc",
+	.init = drm_test_bridge_alloc_init,
+	.test_cases = drm_bridge_alloc_tests,
+};
+
 kunit_test_suites(
 	&drm_bridge_get_current_state_test_suite,
 	&drm_bridge_helper_reset_crtc_test_suite,
+	&drm_bridge_alloc_test_suite,
 );
 
 MODULE_AUTHOR("Maxime Ripard <mripard@kernel.org>");
+MODULE_AUTHOR("Luca Ceresoli <luca.ceresoli@bootlin.com>");
+
 MODULE_DESCRIPTION("Kunit test for drm_bridge functions");
 MODULE_LICENSE("GPL");

-- 
2.49.0

Re: [PATCH v7 2/2] drm/tests: bridge: add a KUnit test for devm_drm_bridge_alloc()

[Maxime Ripard]

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

Hi,

On Wed, Apr 09, 2025 at 04:50:35PM +0200, Luca Ceresoli wrote:
> Add a basic KUnit test for the newly introduced drm_bridge_alloc().
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Changed in v7:
>  - rebase on current drm-misc-next, which now has a drm_bridge_test.c file
>  - cleanup commit message
> 
> Changed in v6:
>  - update to new devm_drm_bridge_alloc() API
>  - remove drm_test_drm_bridge_put test, not straightforward to write with
>    the new API and the current notification mechanism
>  - do not allocate a drm_device: a bridge is allocated without one
>  - rename some identifiers for easier code reading
> 
> This patch was added in v5.
> ---
>  drivers/gpu/drm/tests/drm_bridge_test.c | 60 +++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_bridge_test.c b/drivers/gpu/drm/tests/drm_bridge_test.c
> index ff88ec2e911c9cc9a718483f09d4c764f45f991a..87fb64744b67f0780457a546aba77ba945a0ce67 100644
> --- a/drivers/gpu/drm/tests/drm_bridge_test.c
> +++ b/drivers/gpu/drm/tests/drm_bridge_test.c
> @@ -8,6 +8,7 @@
>  #include <drm/drm_bridge_helper.h>
>  #include <drm/drm_kunit_helpers.h>
>  
> +#include <kunit/device.h>
>  #include <kunit/test.h>
>  
>  struct drm_bridge_init_priv {
> @@ -407,11 +408,70 @@ static struct kunit_suite drm_bridge_helper_reset_crtc_test_suite = {
>  	.test_cases = drm_bridge_helper_reset_crtc_tests,
>  };
>  
> +struct drm_bridge_alloc_test_ctx {
> +	struct device *dev;
> +};

You don't need a struct there then, you can just pass the device pointer.

> +/*
> + * Mimick the typical struct defined by a bridge driver, which embeds a
> + * bridge plus other fields.
> + */
> +struct dummy_drm_bridge {
> +	int dummy; // ensure we test non-zero @bridge offset
> +	struct drm_bridge bridge;
> +};

drm_bridge_init_priv gives you that already.

> +static const struct drm_bridge_funcs drm_bridge_dummy_funcs = {
> +};
> +
> +static int drm_test_bridge_alloc_init(struct kunit *test)
> +{
> +	struct drm_bridge_alloc_test_ctx *ctx;
> +
> +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +	ctx->dev = kunit_device_register(test, "drm-bridge-dev");
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->dev);
> +
> +	test->priv = ctx;
> +
> +	return 0;
> +}
> +
> +/*
> + * Test that the allocation and initialization of a bridge works as
> + * expected and doesn't report any error.
> + */
> +static void drm_test_drm_bridge_alloc(struct kunit *test)
> +{
> +	struct drm_bridge_alloc_test_ctx *ctx = test->priv;
> +	struct dummy_drm_bridge *dummy;
> +
> +	dummy = devm_drm_bridge_alloc(ctx->dev, struct dummy_drm_bridge, bridge,
> +				      &drm_bridge_dummy_funcs);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy);

Why did you need the dummy value in dummy_drm_bridge if you're not using
it?

We'd need a couple more tests, in particular some to make sure the
bridge pointer is properly cleaned up when the device goes away, but not
when we have called drm_bridge_get pointer on it, etc.

Maxime

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

Re: [PATCH v7 2/2] drm/tests: bridge: add a KUnit test for devm_drm_bridge_alloc()

[Luca Ceresoli]

Hi Maxime,

thanks for the careful review.

On Mon, 14 Apr 2025 17:49:16 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> Hi,
> 
> On Wed, Apr 09, 2025 at 04:50:35PM +0200, Luca Ceresoli wrote:
> > Add a basic KUnit test for the newly introduced drm_bridge_alloc().
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > 
> > ---
> > 
> > Changed in v7:
> >  - rebase on current drm-misc-next, which now has a drm_bridge_test.c file
> >  - cleanup commit message
> > 
> > Changed in v6:
> >  - update to new devm_drm_bridge_alloc() API
> >  - remove drm_test_drm_bridge_put test, not straightforward to write with
> >    the new API and the current notification mechanism
> >  - do not allocate a drm_device: a bridge is allocated without one
> >  - rename some identifiers for easier code reading
> > 
> > This patch was added in v5.
> > ---
> >  drivers/gpu/drm/tests/drm_bridge_test.c | 60 +++++++++++++++++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/tests/drm_bridge_test.c b/drivers/gpu/drm/tests/drm_bridge_test.c
> > index ff88ec2e911c9cc9a718483f09d4c764f45f991a..87fb64744b67f0780457a546aba77ba945a0ce67 100644
> > --- a/drivers/gpu/drm/tests/drm_bridge_test.c
> > +++ b/drivers/gpu/drm/tests/drm_bridge_test.c
> > @@ -8,6 +8,7 @@
> >  #include <drm/drm_bridge_helper.h>
> >  #include <drm/drm_kunit_helpers.h>
> >  
> > +#include <kunit/device.h>
> >  #include <kunit/test.h>
> >  
> >  struct drm_bridge_init_priv {
> > @@ -407,11 +408,70 @@ static struct kunit_suite drm_bridge_helper_reset_crtc_test_suite = {
> >  	.test_cases = drm_bridge_helper_reset_crtc_tests,
> >  };
> >  
> > +struct drm_bridge_alloc_test_ctx {
> > +	struct device *dev;
> > +};  
> 
> You don't need a struct there then, you can just pass the device pointer.

Indeed!

> > +/*
> > + * Mimick the typical struct defined by a bridge driver, which embeds a
> > + * bridge plus other fields.
> > + */
> > +struct dummy_drm_bridge {
> > +	int dummy; // ensure we test non-zero @bridge offset
> > +	struct drm_bridge bridge;
> > +};  
> 
> drm_bridge_init_priv gives you that already.

On one hand, that's true. On the other hand, looking at
drm_bridge_init_priv I noticed it is allocating a bridge without using
devm_drm_bridge_alloc(). This should be converted, like all bridge
alloctions.

So I think the we first need to update drm_bridge_test.c to allocate
the bridge using devm_drm_bridge_alloc(), along with the needed changes
to the kunit helpers.

One way would be allocating the entire drm_bridge_init_priv using
devm_drm_bridge_alloc(), but that does not look like a correct design
and after reading the helpers code I'm not even sure it would be doable.

Instead I think we need to change struct drm_bridge_init_priv
to embed a pointer to (a modified version of) struct dummy_drm_bridge:

 struct drm_bridge_init_priv {
         struct drm_device drm;
         struct drm_plane *plane;
         struct drm_crtc *crtc;
         struct drm_encoder encoder;
-        struct drm_bridge bridge;
+        struct dummy_drm_bridge *test_bridge;
         struct drm_connector *connector;
         unsigned int enable_count;
         unsigned int disable_count;
 };

So that devm_drm_bridge_alloc() can allocate the new test_bridge
dynamically:

 priv->test_bridge =
   devm_drm_bridge_alloc(..., struct dummy_drm_bridge, bridge, ...);

Do you think this would be the correct approach?

> > +static const struct drm_bridge_funcs drm_bridge_dummy_funcs = {
> > +};
> > +
> > +static int drm_test_bridge_alloc_init(struct kunit *test)
> > +{
> > +	struct drm_bridge_alloc_test_ctx *ctx;
> > +
> > +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> > +
> > +	ctx->dev = kunit_device_register(test, "drm-bridge-dev");
> > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->dev);
> > +
> > +	test->priv = ctx;
> > +
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Test that the allocation and initialization of a bridge works as
> > + * expected and doesn't report any error.
> > + */
> > +static void drm_test_drm_bridge_alloc(struct kunit *test)
> > +{
> > +	struct drm_bridge_alloc_test_ctx *ctx = test->priv;
> > +	struct dummy_drm_bridge *dummy;
> > +
> > +	dummy = devm_drm_bridge_alloc(ctx->dev, struct dummy_drm_bridge, bridge,
> > +				      &drm_bridge_dummy_funcs);
> > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy);  
> 
> Why did you need the dummy value in dummy_drm_bridge if you're not using
> it?

To ensure we test non-zero @bridge offset. Say there is a bug in the
pointer math, e.g. 'bridge = container - offset' instead of 'bridge =
container + offset'. That would not be caught if @bridge is the first
field in the struct.

Does this look like a good reason to keep it?

> We'd need a couple more tests, in particular some to make sure the
> bridge pointer is properly cleaned up when the device goes away, but not
> when we have called drm_bridge_get pointer on it, etc.

It would surely be useful, and there was one in the initial patch I
sent ([0], search for "destroyed"). Then I removed it because the code
changed, there is no callback anymore, so no place where this can be
tested.

I'd be glad to re-add such a check but I don't see how it could be
implemented in a clean, non-invasive way.

The only way that comes to mind is be that the kunit test does not call
drm_bridge_put() but rather a kunit-specific reimplementation that
passes a reimplementation of __drm_bridge_free() which does the
accounting. Quick draft (note the added "_test" infix):

struct dummy_drm_bridge {
        struct drm_bridge_init_priv *test_priv;
        struct drm_bridge bridge;
};

// reimplemented version of __drm_bridge_free
static void __drm_test_bridge_free(struct kref *kref)
{
        struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
	struct dummy_drm_bridge *dummy = bridge->container;
	
	dummy->text_priv->destroyed = true;
        kfree(bridge->container);
}

// reimplemented version of drm_bridge_put
void drm_test_bridge_put(struct drm_bridge *bridge)
{
        if (bridge)
                kref_put(&bridge->refcount, __drm_test_bridge_free);
}

My concern with this idea is that it is not testing the actual
drm_bridge.c code, but a different implementation. Even more, if the
functions in drm_bridge.c will change, the ones in drm_bridge_test.c
might be forgotten, thus we'd end up in testing code that is different
from the code actually used.

Another way would be adding an optional .destroy a callback in struct
drm_bridge_funcs that is called in __drm_bridge_free(), and only the
kunit test code implements it. Maybe looks cleaner, but it would be
invasive on code that all bridges use. We had discussed a different
idea of .destroy callback in the past, for different reasons, and it
was not solving the problem we had in that case. So kunit would be the
only user for the foreseeable future.

You opinion about these ideas? Can you suggest a better way to implement
such a test, that is clean and not needing to change drm_bridge_put()
and related functions?

Luca

[0] https://lore.kernel.org/all/20241231-hotplug-drm-bridge-v5-5-173065a1ece1@bootlin.com/

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v7 2/2] drm/tests: bridge: add a KUnit test for devm_drm_bridge_alloc()

[Maxime Ripard]

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

On Tue, Apr 15, 2025 at 01:22:14PM +0200, Luca Ceresoli wrote:
> > > +/*
> > > + * Mimick the typical struct defined by a bridge driver, which embeds a
> > > + * bridge plus other fields.
> > > + */
> > > +struct dummy_drm_bridge {
> > > +	int dummy; // ensure we test non-zero @bridge offset
> > > +	struct drm_bridge bridge;
> > > +};  
> > 
> > drm_bridge_init_priv gives you that already.
> 
> On one hand, that's true. On the other hand, looking at
> drm_bridge_init_priv I noticed it is allocating a bridge without using
> devm_drm_bridge_alloc(). This should be converted, like all bridge
> alloctions.
>
> So I think the we first need to update drm_bridge_test.c to allocate
> the bridge using devm_drm_bridge_alloc(), along with the needed changes
> to the kunit helpers.

Oh, yeah, absolutely.

> One way would be allocating the entire drm_bridge_init_priv using
> devm_drm_bridge_alloc(), but that does not look like a correct design
> and after reading the helpers code I'm not even sure it would be doable.
> 
> Instead I think we need to change struct drm_bridge_init_priv
> to embed a pointer to (a modified version of) struct dummy_drm_bridge:
> 
>  struct drm_bridge_init_priv {
>          struct drm_device drm;
>          struct drm_plane *plane;
>          struct drm_crtc *crtc;
>          struct drm_encoder encoder;
> -        struct drm_bridge bridge;
> +        struct dummy_drm_bridge *test_bridge;
>          struct drm_connector *connector;
>          unsigned int enable_count;
>          unsigned int disable_count;
>  };
> 
> So that devm_drm_bridge_alloc() can allocate the new test_bridge
> dynamically:
> 
>  priv->test_bridge =
>    devm_drm_bridge_alloc(..., struct dummy_drm_bridge, bridge, ...);
> 
> Do you think this would be the correct approach?

It's kind of correct, but you're also correct that it's probably too
much for those simple tests, so it might not be worth it in the end.

> > > +static const struct drm_bridge_funcs drm_bridge_dummy_funcs = {
> > > +};
> > > +
> > > +static int drm_test_bridge_alloc_init(struct kunit *test)
> > > +{
> > > +	struct drm_bridge_alloc_test_ctx *ctx;
> > > +
> > > +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> > > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> > > +
> > > +	ctx->dev = kunit_device_register(test, "drm-bridge-dev");
> > > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->dev);
> > > +
> > > +	test->priv = ctx;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/*
> > > + * Test that the allocation and initialization of a bridge works as
> > > + * expected and doesn't report any error.
> > > + */
> > > +static void drm_test_drm_bridge_alloc(struct kunit *test)
> > > +{
> > > +	struct drm_bridge_alloc_test_ctx *ctx = test->priv;
> > > +	struct dummy_drm_bridge *dummy;
> > > +
> > > +	dummy = devm_drm_bridge_alloc(ctx->dev, struct dummy_drm_bridge, bridge,
> > > +				      &drm_bridge_dummy_funcs);
> > > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy);  
> > 
> > Why did you need the dummy value in dummy_drm_bridge if you're not using
> > it?
> 
> To ensure we test non-zero @bridge offset. Say there is a bug in the
> pointer math, e.g. 'bridge = container - offset' instead of 'bridge =
> container + offset'. That would not be caught if @bridge is the first
> field in the struct.
> 
> Does this look like a good reason to keep it?

Ack, but please document it with a comment

> > We'd need a couple more tests, in particular some to make sure the
> > bridge pointer is properly cleaned up when the device goes away, but not
> > when we have called drm_bridge_get pointer on it, etc.
> 
> It would surely be useful, and there was one in the initial patch I
> sent ([0], search for "destroyed"). Then I removed it because the code
> changed, there is no callback anymore, so no place where this can be
> tested.

Why did we end up removing the destroy hook? It looks useful to me.

> I'd be glad to re-add such a check but I don't see how it could be
> implemented in a clean, non-invasive way.
> 
> The only way that comes to mind is be that the kunit test does not call
> drm_bridge_put() but rather a kunit-specific reimplementation that
> passes a reimplementation of __drm_bridge_free() which does the
> accounting. Quick draft (note the added "_test" infix):
> 
> struct dummy_drm_bridge {
>         struct drm_bridge_init_priv *test_priv;
>         struct drm_bridge bridge;
> };
> 
> // reimplemented version of __drm_bridge_free
> static void __drm_test_bridge_free(struct kref *kref)
> {
>         struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
> 	struct dummy_drm_bridge *dummy = bridge->container;
> 	
> 	dummy->text_priv->destroyed = true;
>         kfree(bridge->container);
> }
> 
> // reimplemented version of drm_bridge_put
> void drm_test_bridge_put(struct drm_bridge *bridge)
> {
>         if (bridge)
>                 kref_put(&bridge->refcount, __drm_test_bridge_free);
> }
> 
> My concern with this idea is that it is not testing the actual
> drm_bridge.c code, but a different implementation. Even more, if the
> functions in drm_bridge.c will change, the ones in drm_bridge_test.c
> might be forgotten, thus we'd end up in testing code that is different
> from the code actually used.

Yeah, I agree, it's not really useful if it's not testing the code we
would typically run.

> Another way would be adding an optional .destroy a callback in struct
> drm_bridge_funcs that is called in __drm_bridge_free(), and only the
> kunit test code implements it. Maybe looks cleaner, but it would be
> invasive on code that all bridges use. We had discussed a different
> idea of .destroy callback in the past, for different reasons, and it
> was not solving the problem we had in that case. So kunit would be the
> only user for the foreseeable future.

Sorry, we've had many conversations about all that work so I can't
recall (or find) what your objections or concerns (or mine, maybe?) were
about thing topic. It looks totally reasonable to me, and consistent
with all the other DRM entities.

I'm also not entirely sure how invasive it would be? Add that callback,
check if it's set and if it is, call it from __drm_bridge_free(), and
you're pretty much done, right?

Maxime

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

Re: [PATCH v7 2/2] drm/tests: bridge: add a KUnit test for devm_drm_bridge_alloc()

[Luca Ceresoli]

Hi Maxime,

On Thu, 15 May 2025 10:11:33 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Tue, Apr 15, 2025 at 01:22:14PM +0200, Luca Ceresoli wrote:
> > > > +/*
> > > > + * Mimick the typical struct defined by a bridge driver, which embeds a
> > > > + * bridge plus other fields.
> > > > + */
> > > > +struct dummy_drm_bridge {
> > > > +	int dummy; // ensure we test non-zero @bridge offset
> > > > +	struct drm_bridge bridge;
> > > > +};    
> > > 
> > > drm_bridge_init_priv gives you that already.  
> > 
> > On one hand, that's true. On the other hand, looking at
> > drm_bridge_init_priv I noticed it is allocating a bridge without using
> > devm_drm_bridge_alloc(). This should be converted, like all bridge
> > alloctions.
> >
> > So I think the we first need to update drm_bridge_test.c to allocate
> > the bridge using devm_drm_bridge_alloc(), along with the needed changes
> > to the kunit helpers.  
> 
> Oh, yeah, absolutely.
> 
> > One way would be allocating the entire drm_bridge_init_priv using
> > devm_drm_bridge_alloc(), but that does not look like a correct design
> > and after reading the helpers code I'm not even sure it would be doable.
> > 
> > Instead I think we need to change struct drm_bridge_init_priv
> > to embed a pointer to (a modified version of) struct dummy_drm_bridge:
> > 
> >  struct drm_bridge_init_priv {
> >          struct drm_device drm;
> >          struct drm_plane *plane;
> >          struct drm_crtc *crtc;
> >          struct drm_encoder encoder;
> > -        struct drm_bridge bridge;
> > +        struct dummy_drm_bridge *test_bridge;
> >          struct drm_connector *connector;
> >          unsigned int enable_count;
> >          unsigned int disable_count;
> >  };
> > 
> > So that devm_drm_bridge_alloc() can allocate the new test_bridge
> > dynamically:
> > 
> >  priv->test_bridge =
> >    devm_drm_bridge_alloc(..., struct dummy_drm_bridge, bridge, ...);
> > 
> > Do you think this would be the correct approach?  
> 
> It's kind of correct, but you're also correct that it's probably too
> much for those simple tests, so it might not be worth it in the end.

I haven't found any better ways, so I implemented the idea sketched
above. It will be in v8.

> > > > +static const struct drm_bridge_funcs drm_bridge_dummy_funcs = {
> > > > +};
> > > > +
> > > > +static int drm_test_bridge_alloc_init(struct kunit *test)
> > > > +{
> > > > +	struct drm_bridge_alloc_test_ctx *ctx;
> > > > +
> > > > +	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
> > > > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> > > > +
> > > > +	ctx->dev = kunit_device_register(test, "drm-bridge-dev");
> > > > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->dev);
> > > > +
> > > > +	test->priv = ctx;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Test that the allocation and initialization of a bridge works as
> > > > + * expected and doesn't report any error.
> > > > + */
> > > > +static void drm_test_drm_bridge_alloc(struct kunit *test)
> > > > +{
> > > > +	struct drm_bridge_alloc_test_ctx *ctx = test->priv;
> > > > +	struct dummy_drm_bridge *dummy;
> > > > +
> > > > +	dummy = devm_drm_bridge_alloc(ctx->dev, struct dummy_drm_bridge, bridge,
> > > > +				      &drm_bridge_dummy_funcs);
> > > > +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy);    
> > > 
> > > Why did you need the dummy value in dummy_drm_bridge if you're not using
> > > it?  
> > 
> > To ensure we test non-zero @bridge offset. Say there is a bug in the
> > pointer math, e.g. 'bridge = container - offset' instead of 'bridge =
> > container + offset'. That would not be caught if @bridge is the first
> > field in the struct.
> > 
> > Does this look like a good reason to keep it?  
> 
> Ack, but please document it with a comment

There is one already:

struct dummy_drm_bridge {
	int dummy; // ensure we test non-zero @bridge offset
	struct drm_bridge bridge;
};    

but the v8 code will be different because of the conversion to
devm_drm_bdirge_alloc(), and anyway I extended the comment.

> > Another way would be adding an optional .destroy a callback in struct
> > drm_bridge_funcs that is called in __drm_bridge_free(), and only the
> > kunit test code implements it. Maybe looks cleaner, but it would be
> > invasive on code that all bridges use. We had discussed a different
> > idea of .destroy callback in the past, for different reasons, and it
> > was not solving the problem we had in that case. So kunit would be the
> > only user for the foreseeable future.  
> 
> Sorry, we've had many conversations about all that work so I can't
> recall (or find) what your objections or concerns (or mine, maybe?) were
> about thing topic. It looks totally reasonable to me, and consistent
> with all the other DRM entities.

That was a long story and I also don't remember all the details,
however here's a summary of what I can recollect:

 1. initially I proposed a .destroy called in *drm_bridge_free(), i.e.
    upon the last put [1]
     * it was used to ask the bridge driver to kfree() the driver struct
       that embeds the drm_bridge; that was not a good design, putting
       deallocation duties on each driver's shoulders
     * it was made unnecessary by devm_drm_bridge_alloc(), which moved
       the entire kfree into __drm_bridge_free() itself, based on the 
       .container pointer
 2. we re-discussed it as a way to handle the panel_bridge, but in that
    case it would have been called by drm_bridge_remove() IIRC [2]
     * you said it was not a good solution (and I agree) and that a much
       wider rework would be needed for panels, eventually including the
       panel_bridge
     * then Anusha sent the patches to start the panel rework

So now we are discussing adding .destroy again, and in
__drm_bridge_free(), as it was at step 1, but for a different reason.

[1] https://lore.kernel.org/all/20241231-hotplug-drm-bridge-v5-3-173065a1ece1@bootlin.com/
[2] https://oftc.catirclogs.org/dri-devel/2025-02-14#

> I'm also not entirely sure how invasive it would be? Add that callback,
> check if it's set and if it is, call it from __drm_bridge_free(), and
> you're pretty much done, right?

No much added code indeed. My concern is about the fact that the
callback would be used only by kunit test and not "real code". It is
possibly worth doing anyway, so I wrote something for v8 and we'll see
how it looks.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v7 2/2] drm/tests: bridge: add a KUnit test for devm_drm_bridge_alloc()

[Maxime Ripard]

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

On Fri, May 16, 2025 at 05:38:28PM +0200, Luca Ceresoli wrote:
> > > Another way would be adding an optional .destroy a callback in struct
> > > drm_bridge_funcs that is called in __drm_bridge_free(), and only the
> > > kunit test code implements it. Maybe looks cleaner, but it would be
> > > invasive on code that all bridges use. We had discussed a different
> > > idea of .destroy callback in the past, for different reasons, and it
> > > was not solving the problem we had in that case. So kunit would be the
> > > only user for the foreseeable future.  
> > 
> > Sorry, we've had many conversations about all that work so I can't
> > recall (or find) what your objections or concerns (or mine, maybe?) were
> > about thing topic. It looks totally reasonable to me, and consistent
> > with all the other DRM entities.
> 
> That was a long story and I also don't remember all the details,
> however here's a summary of what I can recollect:
> 
>  1. initially I proposed a .destroy called in *drm_bridge_free(), i.e.
>     upon the last put [1]
>      * it was used to ask the bridge driver to kfree() the driver struct
>        that embeds the drm_bridge; that was not a good design, putting
>        deallocation duties on each driver's shoulders
>      * it was made unnecessary by devm_drm_bridge_alloc(), which moved
>        the entire kfree into __drm_bridge_free() itself, based on the 
>        .container pointer
>  2. we re-discussed it as a way to handle the panel_bridge, but in that
>     case it would have been called by drm_bridge_remove() IIRC [2]
>      * you said it was not a good solution (and I agree) and that a much
>        wider rework would be needed for panels, eventually including the
>        panel_bridge
>      * then Anusha sent the patches to start the panel rework

Thanks for the summary. I still agree with our discussions, however...

> So now we are discussing adding .destroy again, and in
> __drm_bridge_free(), as it was at step 1, but for a different reason.
> 
> [1] https://lore.kernel.org/all/20241231-hotplug-drm-bridge-v5-3-173065a1ece1@bootlin.com/
> [2] https://oftc.catirclogs.org/dri-devel/2025-02-14#
> 
> > I'm also not entirely sure how invasive it would be? Add that callback,
> > check if it's set and if it is, call it from __drm_bridge_free(), and
> > you're pretty much done, right?
> 
> No much added code indeed. My concern is about the fact that the
> callback would be used only by kunit test and not "real code". It is
> possibly worth doing anyway, so I wrote something for v8 and we'll see
> how it looks.

... It's still useful. With KMS drivers, you usually get two different
lifetimes: the hardware device and the DRM device. The DRM device will
outlive the hardware device. Thus, the driver instance will still be
live even after the device is gone away, and still need to somewhat
interact with the framework, even if in degraded mode.

All the resources tied to the device (memory mapping, clocks,
interrupts, etc.) are handled by devm. We don't have to take care of
them in destroy, they are long gone before we reach that point.

However, the bridge might still need to allocate memory, create objects,
 to interact with the framework. Things like a private object, or a
 scratch buffer, or... whatever.

But anything that the driver needs to implement what the framework
expects basically. *Those* are what need to be cleaned up in destroy.

Maxime

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