[PATCH v8 0/3] drm/bridge: add kunit tests for devm_drm_bridge_alloc()

[PATCH v8 0/3] drm/bridge: add kunit tests for devm_drm_bridge_alloc()

[Luca Ceresoli]

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

It is part of the work towards removal of bridges from a still existing DRM
pipeline without use-after-free.

The steps in the grand plan [1] are:

 1. ➜ add refcounting to DRM bridges (struct drm_bridge)
 2. handle gracefully atomic updates during bridge removal
 3. avoid DSI host drivers to have dangling pointers to DSI devices
 4. finish the hotplug bridge work, removing the "always-disconnected"
    connector, moving code to the core and potentially removing the
    hotplug-bridge itself (this needs to be clarified as points 1-3 are
    developed)

This series is part of step 1 of the grand plan.

Current tasks in step 1 of the grand plan:

 A. ✔ add new alloc API and refcounting -> (now in drm-misc-next)
 B. … convert all bridge drivers to new API (in progress)
 C. ➜ 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/lkml/20250206-hotplug-drm-bridge-v6-0-9d6f2c9c3058@bootlin.com/t/#u

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Changes in v8:
- Remove documentation patch
- Add patch to convert existing kunit tests to use devm_drm_bridge_alloc()
- Add tests for bridge deallocation (based on adding a .destroy callback)
- Link to v7: https://lore.kernel.org/r/20250409-drm-bridge-alloc-doc-test-v7-0-a3ca4b97597f@bootlin.com

---
Luca Ceresoli (3):
      drm/tests: bridge: convert to devm_drm_bridge_alloc() API
      dmr/bridge: add a .destroy func
      drm/tests: bridge: add KUnit tests for devm_drm_bridge_alloc()

 drivers/gpu/drm/drm_bridge.c            |   2 +
 drivers/gpu/drm/tests/drm_bridge_test.c | 179 +++++++++++++++++++++++++-------
 include/drm/drm_bridge.h                |  10 ++
 3 files changed, 151 insertions(+), 40 deletions(-)
---
base-commit: aec8a40228acb385d60feec59b54573d307e60f3
change-id: 20250408-drm-bridge-alloc-doc-test-267df0def880

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

[PATCH v8 1/3] drm/tests: bridge: convert to devm_drm_bridge_alloc() API

[Luca Ceresoli]

Use the new DRM bridge allocation API, which is the only supported now, for
the kunit tests.

This change is more massive than for the typical DRM bridge driver because
struct drm_bridge_init_priv currently embeds a struct drm_bridge, which is
not supported anymore. We new have to use devm_drm_bridge_alloc() to
dynamically allocate a "private driver struct", which is a bit awkward here
because there is no real bridge driver. Thus let's add a "dummy" DRM bridge
struct to represent it.

As a nice cleanup we can now move the enable_count and disable_count
members, which are counting bridge-specific events, into the new "private
driver struct" (and avoid adding new unnecessary indirections).

Also add a trivial bridge_to_dummy_bridge() just like many drivers do.

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

---

This patch was added in v8.
---
 drivers/gpu/drm/tests/drm_bridge_test.c | 95 +++++++++++++++++++--------------
 1 file changed, 55 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_bridge_test.c b/drivers/gpu/drm/tests/drm_bridge_test.c
index ff88ec2e911c9cc9a718483f09d4c764f45f991a..f3a625c536f610dc8560b56531056df7c613f564 100644
--- a/drivers/gpu/drm/tests/drm_bridge_test.c
+++ b/drivers/gpu/drm/tests/drm_bridge_test.c
@@ -10,31 +10,45 @@
 
 #include <kunit/test.h>
 
+/*
+ * Mimick the typical struct defined by a bridge driver, which embeds a
+ * bridge plus other fields.
+ *
+ * Having at least one member before @bridge ensures we test non-zero
+ * @bridge offset.
+ */
+struct dummy_drm_bridge {
+	unsigned int enable_count;
+	unsigned int disable_count;
+	struct drm_bridge 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;
 };
 
+static struct dummy_drm_bridge *bridge_to_dummy_bridge(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct dummy_drm_bridge, bridge);
+}
+
 static void drm_test_bridge_enable(struct drm_bridge *bridge)
 {
-	struct drm_bridge_init_priv *priv =
-		container_of(bridge, struct drm_bridge_init_priv, bridge);
+	struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
 
-	priv->enable_count++;
+	dummy_br->enable_count++;
 }
 
 static void drm_test_bridge_disable(struct drm_bridge *bridge)
 {
-	struct drm_bridge_init_priv *priv =
-		container_of(bridge, struct drm_bridge_init_priv, bridge);
+	struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
 
-	priv->disable_count++;
+	dummy_br->disable_count++;
 }
 
 static const struct drm_bridge_funcs drm_test_bridge_legacy_funcs = {
@@ -45,19 +59,17 @@ static const struct drm_bridge_funcs drm_test_bridge_legacy_funcs = {
 static void drm_test_bridge_atomic_enable(struct drm_bridge *bridge,
 					  struct drm_atomic_state *state)
 {
-	struct drm_bridge_init_priv *priv =
-		container_of(bridge, struct drm_bridge_init_priv, bridge);
+	struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
 
-	priv->enable_count++;
+	dummy_br->enable_count++;
 }
 
 static void drm_test_bridge_atomic_disable(struct drm_bridge *bridge,
 					   struct drm_atomic_state *state)
 {
-	struct drm_bridge_init_priv *priv =
-		container_of(bridge, struct drm_bridge_init_priv, bridge);
+	struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
 
-	priv->disable_count++;
+	dummy_br->disable_count++;
 }
 
 static const struct drm_bridge_funcs drm_test_bridge_atomic_funcs = {
@@ -102,6 +114,10 @@ drm_test_bridge_init(struct kunit *test, const struct drm_bridge_funcs *funcs)
 	if (IS_ERR(priv))
 		return ERR_CAST(priv);
 
+	priv->test_bridge = devm_drm_bridge_alloc(dev, struct dummy_drm_bridge, bridge, funcs);
+	if (IS_ERR(priv->test_bridge))
+		return ERR_CAST(priv->test_bridge);
+
 	drm = &priv->drm;
 	priv->plane = drm_kunit_helper_create_primary_plane(test, drm,
 							    NULL,
@@ -125,9 +141,8 @@ drm_test_bridge_init(struct kunit *test, const struct drm_bridge_funcs *funcs)
 
 	enc->possible_crtcs = drm_crtc_mask(priv->crtc);
 
-	bridge = &priv->bridge;
+	bridge = &priv->test_bridge->bridge;
 	bridge->type = DRM_MODE_CONNECTOR_VIRTUAL;
-	bridge->funcs = funcs;
 
 	ret = drm_kunit_bridge_add(test, bridge);
 	if (ret)
@@ -173,7 +188,7 @@ static void drm_test_drm_bridge_get_current_state_atomic(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
 
 retry_commit:
-	bridge = &priv->bridge;
+	bridge = &priv->test_bridge->bridge;
 	bridge_state = drm_atomic_get_bridge_state(state, bridge);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, bridge_state);
 
@@ -228,7 +243,7 @@ static void drm_test_drm_bridge_get_current_state_legacy(struct kunit *test)
 	 * locking. The function would return NULL in all cases anyway,
 	 * so we don't really have any concurrency to worry about.
 	 */
-	bridge = &priv->bridge;
+	bridge = &priv->test_bridge->bridge;
 	KUNIT_EXPECT_NULL(test, drm_bridge_get_current_state(bridge));
 }
 
@@ -253,7 +268,7 @@ static void drm_test_drm_bridge_helper_reset_crtc_atomic(struct kunit *test)
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_bridge_init_priv *priv;
 	struct drm_display_mode *mode;
-	struct drm_bridge *bridge;
+	struct dummy_drm_bridge *dummy_br;
 	int ret;
 
 	priv = drm_test_bridge_init(test, &drm_test_bridge_atomic_funcs);
@@ -279,14 +294,14 @@ static void drm_test_drm_bridge_helper_reset_crtc_atomic(struct kunit *test)
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
-	bridge = &priv->bridge;
-	KUNIT_ASSERT_EQ(test, priv->enable_count, 1);
-	KUNIT_ASSERT_EQ(test, priv->disable_count, 0);
+	dummy_br = priv->test_bridge;
+	KUNIT_ASSERT_EQ(test, dummy_br->enable_count, 1);
+	KUNIT_ASSERT_EQ(test, dummy_br->disable_count, 0);
 
 	drm_modeset_acquire_init(&ctx, 0);
 
 retry_reset:
-	ret = drm_bridge_helper_reset_crtc(bridge, &ctx);
+	ret = drm_bridge_helper_reset_crtc(&dummy_br->bridge, &ctx);
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(&ctx);
 		goto retry_reset;
@@ -296,8 +311,8 @@ static void drm_test_drm_bridge_helper_reset_crtc_atomic(struct kunit *test)
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
-	KUNIT_EXPECT_EQ(test, priv->enable_count, 2);
-	KUNIT_EXPECT_EQ(test, priv->disable_count, 1);
+	KUNIT_EXPECT_EQ(test, dummy_br->enable_count, 2);
+	KUNIT_EXPECT_EQ(test, dummy_br->disable_count, 1);
 }
 
 /*
@@ -309,7 +324,7 @@ static void drm_test_drm_bridge_helper_reset_crtc_atomic_disabled(struct kunit *
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_bridge_init_priv *priv;
 	struct drm_display_mode *mode;
-	struct drm_bridge *bridge;
+	struct dummy_drm_bridge *dummy_br;
 	int ret;
 
 	priv = drm_test_bridge_init(test, &drm_test_bridge_atomic_funcs);
@@ -318,14 +333,14 @@ static void drm_test_drm_bridge_helper_reset_crtc_atomic_disabled(struct kunit *
 	mode = drm_kunit_display_mode_from_cea_vic(test, &priv->drm, 16);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mode);
 
-	bridge = &priv->bridge;
-	KUNIT_ASSERT_EQ(test, priv->enable_count, 0);
-	KUNIT_ASSERT_EQ(test, priv->disable_count, 0);
+	dummy_br = priv->test_bridge;
+	KUNIT_ASSERT_EQ(test, dummy_br->enable_count, 0);
+	KUNIT_ASSERT_EQ(test, dummy_br->disable_count, 0);
 
 	drm_modeset_acquire_init(&ctx, 0);
 
 retry_reset:
-	ret = drm_bridge_helper_reset_crtc(bridge, &ctx);
+	ret = drm_bridge_helper_reset_crtc(&dummy_br->bridge, &ctx);
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(&ctx);
 		goto retry_reset;
@@ -335,8 +350,8 @@ static void drm_test_drm_bridge_helper_reset_crtc_atomic_disabled(struct kunit *
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
-	KUNIT_EXPECT_EQ(test, priv->enable_count, 0);
-	KUNIT_EXPECT_EQ(test, priv->disable_count, 0);
+	KUNIT_EXPECT_EQ(test, dummy_br->enable_count, 0);
+	KUNIT_EXPECT_EQ(test, dummy_br->disable_count, 0);
 }
 
 /*
@@ -348,7 +363,7 @@ static void drm_test_drm_bridge_helper_reset_crtc_legacy(struct kunit *test)
 	struct drm_modeset_acquire_ctx ctx;
 	struct drm_bridge_init_priv *priv;
 	struct drm_display_mode *mode;
-	struct drm_bridge *bridge;
+	struct dummy_drm_bridge *dummy_br;
 	int ret;
 
 	priv = drm_test_bridge_init(test, &drm_test_bridge_legacy_funcs);
@@ -374,14 +389,14 @@ static void drm_test_drm_bridge_helper_reset_crtc_legacy(struct kunit *test)
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
-	bridge = &priv->bridge;
-	KUNIT_ASSERT_EQ(test, priv->enable_count, 1);
-	KUNIT_ASSERT_EQ(test, priv->disable_count, 0);
+	dummy_br = priv->test_bridge;
+	KUNIT_ASSERT_EQ(test, dummy_br->enable_count, 1);
+	KUNIT_ASSERT_EQ(test, dummy_br->disable_count, 0);
 
 	drm_modeset_acquire_init(&ctx, 0);
 
 retry_reset:
-	ret = drm_bridge_helper_reset_crtc(bridge, &ctx);
+	ret = drm_bridge_helper_reset_crtc(&dummy_br->bridge, &ctx);
 	if (ret == -EDEADLK) {
 		drm_modeset_backoff(&ctx);
 		goto retry_reset;
@@ -391,8 +406,8 @@ static void drm_test_drm_bridge_helper_reset_crtc_legacy(struct kunit *test)
 	drm_modeset_drop_locks(&ctx);
 	drm_modeset_acquire_fini(&ctx);
 
-	KUNIT_EXPECT_EQ(test, priv->enable_count, 2);
-	KUNIT_EXPECT_EQ(test, priv->disable_count, 1);
+	KUNIT_EXPECT_EQ(test, dummy_br->enable_count, 2);
+	KUNIT_EXPECT_EQ(test, dummy_br->disable_count, 1);
 }
 
 static struct kunit_case drm_bridge_helper_reset_crtc_tests[] = {

-- 
2.49.0

[PATCH v8 2/3] dmr/bridge: add a .destroy func

[Luca Ceresoli]

Some users of DRM bridges may need to execute specific code just before
deallocation.

As of now the only known user would be KUnit tests.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

This patch is new in v8. The .destroy callback had appeared in v5 as well
[5], but as part of a larger patch and for different reason that do not
apply anymore.

[5] https://lore.kernel.org/all/20241231-hotplug-drm-bridge-v5-3-173065a1ece1@bootlin.com/#t
---
 drivers/gpu/drm/drm_bridge.c |  2 ++
 include/drm/drm_bridge.h     | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index b4c89ec01998b849018ce031c7cd84614e65e710..6185cb29fe3162264f0912c09c205fb467975dee 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -203,6 +203,8 @@ static void __drm_bridge_free(struct kref *kref)
 {
 	struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
 
+	if (bridge->funcs->destroy)
+		bridge->funcs->destroy(bridge);
 	kfree(bridge->container);
 }
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4e418a29a9ff9d014d6ac0910a5d9bcf7118195e..3ccd493faa580845c2ed1166f398eca27b464261 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -86,6 +86,16 @@ struct drm_bridge_funcs {
 	 */
 	void (*detach)(struct drm_bridge *bridge);
 
+	/**
+	 * @destroy:
+	 *
+	 * This callback is invoked when the bridge is about to be
+	 * deallocated.
+	 *
+	 * The @destroy callback is optional.
+	 */
+	void (*destroy)(struct drm_bridge *bridge);
+
 	/**
 	 * @mode_valid:
 	 *

-- 
2.49.0

[PATCH v8 3/3] drm/tests: bridge: add KUnit tests for devm_drm_bridge_alloc()

[Luca Ceresoli]

Add KUnit tests for the newly introduced devm_drm_bridge_alloc().

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

---

Changed in v8:
 - rebase on new patch converting drm_bridge_test.c to
   devm_drm_bridge_alloc()
 - add check that bridge is removed (thanks to the .destroy callback)
 - add a check with get/put

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 | 84 +++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_bridge_test.c b/drivers/gpu/drm/tests/drm_bridge_test.c
index f3a625c536f610dc8560b56531056df7c613f564..32db3a82fe6d14a3e9d6536bcf4b19f1bc65969a 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>
 
 /*
@@ -21,6 +22,7 @@ struct dummy_drm_bridge {
 	unsigned int enable_count;
 	unsigned int disable_count;
 	struct drm_bridge bridge;
+	void *data;
 };
 
 struct drm_bridge_init_priv {
@@ -422,11 +424,93 @@ 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;
+	struct dummy_drm_bridge *dummy_br;
+	bool destroyed;
+};
+
+static void dummy_drm_bridge_destroy(struct drm_bridge *bridge)
+{
+	struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
+	struct drm_bridge_alloc_test_ctx *ctx = (struct drm_bridge_alloc_test_ctx *)dummy_br->data;
+
+	ctx->destroyed = true;
+}
+
+static const struct drm_bridge_funcs drm_bridge_dummy_funcs = {
+	.destroy = dummy_drm_bridge_destroy,
+};
+
+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;
+
+	ctx->dummy_br = devm_drm_bridge_alloc(ctx->dev, struct dummy_drm_bridge, bridge,
+					      &drm_bridge_dummy_funcs);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->dummy_br);
+
+	ctx->dummy_br->data = ctx;
+
+	KUNIT_ASSERT_FALSE(test, ctx->destroyed);
+
+	return 0;
+}
+
+static void drm_test_drm_bridge_alloc_basic(struct kunit *test)
+{
+	struct drm_bridge_alloc_test_ctx *ctx = test->priv;
+
+	KUNIT_ASSERT_FALSE(test, ctx->destroyed);
+
+	kunit_device_unregister(test, ctx->dev);
+	KUNIT_ASSERT_TRUE(test, ctx->destroyed);
+}
+
+static void drm_test_drm_bridge_alloc_get_put(struct kunit *test)
+{
+	struct drm_bridge_alloc_test_ctx *ctx = test->priv;
+
+	KUNIT_ASSERT_FALSE(test, ctx->destroyed);
+
+	drm_bridge_get(&ctx->dummy_br->bridge);
+	KUNIT_ASSERT_FALSE(test, ctx->destroyed);
+
+	kunit_device_unregister(test, ctx->dev);
+	KUNIT_ASSERT_FALSE(test, ctx->destroyed);
+
+	drm_bridge_put(&ctx->dummy_br->bridge);
+	KUNIT_ASSERT_TRUE(test, ctx->destroyed);
+}
+
+static struct kunit_case drm_bridge_alloc_tests[] = {
+	KUNIT_CASE(drm_test_drm_bridge_alloc_basic),
+	KUNIT_CASE(drm_test_drm_bridge_alloc_get_put),
+	{ }
+};
+
+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 v8 2/3] dmr/bridge: add a .destroy func

[Maxime Ripard]

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

On Fri, May 16, 2025 at 06:48:38PM +0200, Luca Ceresoli wrote:
> Some users of DRM bridges may need to execute specific code just before
> deallocation.
> 
> As of now the only known user would be KUnit tests.
> 
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> This patch is new in v8. The .destroy callback had appeared in v5 as well
> [5], but as part of a larger patch and for different reason that do not
> apply anymore.
> 
> [5] https://lore.kernel.org/all/20241231-hotplug-drm-bridge-v5-3-173065a1ece1@bootlin.com/#t
> ---
>  drivers/gpu/drm/drm_bridge.c |  2 ++
>  include/drm/drm_bridge.h     | 10 ++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index b4c89ec01998b849018ce031c7cd84614e65e710..6185cb29fe3162264f0912c09c205fb467975dee 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -203,6 +203,8 @@ static void __drm_bridge_free(struct kref *kref)
>  {
>  	struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
>  
> +	if (bridge->funcs->destroy)
> +		bridge->funcs->destroy(bridge);
>  	kfree(bridge->container);
>  }
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 4e418a29a9ff9d014d6ac0910a5d9bcf7118195e..3ccd493faa580845c2ed1166f398eca27b464261 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -86,6 +86,16 @@ struct drm_bridge_funcs {
>  	 */
>  	void (*detach)(struct drm_bridge *bridge);
>  
> +	/**
> +	 * @destroy:
> +	 *
> +	 * This callback is invoked when the bridge is about to be
> +	 * deallocated.
> +	 *
> +	 * The @destroy callback is optional.
> +	 */
> +	void (*destroy)(struct drm_bridge *bridge);
> +

destroy is before detach in alphabetical order, but otherwise it looks
good to me.

Once fixed,
Acked-by: Maxime Ripard <mripard@kernel.org>

Maxime

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

Re: [PATCH v8 2/3] dmr/bridge: add a .destroy func

[Luca Ceresoli]

Hi Maxime,

On Thu, 22 May 2025 17:43:37 +0200
Maxime Ripard <mripard@kernel.org> wrote:

> On Fri, May 16, 2025 at 06:48:38PM +0200, Luca Ceresoli wrote:
> > Some users of DRM bridges may need to execute specific code just before
> > deallocation.
> > 
> > As of now the only known user would be KUnit tests.
> > 
> > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > 
> > ---
> > 
> > This patch is new in v8. The .destroy callback had appeared in v5 as well
> > [5], but as part of a larger patch and for different reason that do not
> > apply anymore.
> > 
> > [5] https://lore.kernel.org/all/20241231-hotplug-drm-bridge-v5-3-173065a1ece1@bootlin.com/#t
> > ---
> >  drivers/gpu/drm/drm_bridge.c |  2 ++
> >  include/drm/drm_bridge.h     | 10 ++++++++++
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index b4c89ec01998b849018ce031c7cd84614e65e710..6185cb29fe3162264f0912c09c205fb467975dee 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -203,6 +203,8 @@ static void __drm_bridge_free(struct kref *kref)
> >  {
> >  	struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
> >  
> > +	if (bridge->funcs->destroy)
> > +		bridge->funcs->destroy(bridge);
> >  	kfree(bridge->container);
> >  }
> >  
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 4e418a29a9ff9d014d6ac0910a5d9bcf7118195e..3ccd493faa580845c2ed1166f398eca27b464261 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -86,6 +86,16 @@ struct drm_bridge_funcs {
> >  	 */
> >  	void (*detach)(struct drm_bridge *bridge);
> >  
> > +	/**
> > +	 * @destroy:
> > +	 *
> > +	 * This callback is invoked when the bridge is about to be
> > +	 * deallocated.
> > +	 *
> > +	 * The @destroy callback is optional.
> > +	 */
> > +	void (*destroy)(struct drm_bridge *bridge);
> > +  
> 
> destroy is before detach in alphabetical order, but otherwise it looks
> good to me.

I saw the struct is not alpha-ordered right now, so I did not get it
should be, and it looked like keeping .attach and .detach nearby would
be good.

> Once fixed,
> Acked-by: Maxime Ripard <mripard@kernel.org>

OK, will send new iteration with .destroy before .detach, thanks for
the review.

Luca

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

Re: [PATCH v8 1/3] drm/tests: bridge: convert to devm_drm_bridge_alloc() API

[Maxime Ripard]

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

On Fri, May 16, 2025 at 06:48:37PM +0200, Luca Ceresoli wrote:
> Use the new DRM bridge allocation API, which is the only supported now, for
> the kunit tests.
> 
> This change is more massive than for the typical DRM bridge driver because
> struct drm_bridge_init_priv currently embeds a struct drm_bridge, which is
> not supported anymore. We new have to use devm_drm_bridge_alloc() to
> dynamically allocate a "private driver struct", which is a bit awkward here
> because there is no real bridge driver. Thus let's add a "dummy" DRM bridge
> struct to represent it.
> 
> As a nice cleanup we can now move the enable_count and disable_count
> members, which are counting bridge-specific events, into the new "private
> driver struct" (and avoid adding new unnecessary indirections).
> 
> Also add a trivial bridge_to_dummy_bridge() just like many drivers do.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> This patch was added in v8.
> ---
>  drivers/gpu/drm/tests/drm_bridge_test.c | 95 +++++++++++++++++++--------------
>  1 file changed, 55 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tests/drm_bridge_test.c b/drivers/gpu/drm/tests/drm_bridge_test.c
> index ff88ec2e911c9cc9a718483f09d4c764f45f991a..f3a625c536f610dc8560b56531056df7c613f564 100644
> --- a/drivers/gpu/drm/tests/drm_bridge_test.c
> +++ b/drivers/gpu/drm/tests/drm_bridge_test.c
> @@ -10,31 +10,45 @@
>  
>  #include <kunit/test.h>
>  
> +/*
> + * Mimick the typical struct defined by a bridge driver, which embeds a
> + * bridge plus other fields.
> + *
> + * Having at least one member before @bridge ensures we test non-zero
> + * @bridge offset.
> + */
> +struct dummy_drm_bridge {
> +	unsigned int enable_count;
> +	unsigned int disable_count;
> +	struct drm_bridge bridge;
> +};
> +

If we want to remain consistent with the rest of the names, I guess
drm_bridge_priv would be a better choice.

>  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;
>  };
>  
> +static struct dummy_drm_bridge *bridge_to_dummy_bridge(struct drm_bridge *bridge)

bridge_to_priv

> +{
> +	return container_of(bridge, struct dummy_drm_bridge, bridge);
> +}
> +
>  static void drm_test_bridge_enable(struct drm_bridge *bridge)
>  {
> -	struct drm_bridge_init_priv *priv =
> -		container_of(bridge, struct drm_bridge_init_priv, bridge);
> +	struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);

and priv for the variable name is enough here too.

>  
> -	priv->enable_count++;
> +	dummy_br->enable_count++;
>  }
>  
>  static void drm_test_bridge_disable(struct drm_bridge *bridge)
>  {
> -	struct drm_bridge_init_priv *priv =
> -		container_of(bridge, struct drm_bridge_init_priv, bridge);
> +	struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
>  
> -	priv->disable_count++;
> +	dummy_br->disable_count++;
>  }
>  
>  static const struct drm_bridge_funcs drm_test_bridge_legacy_funcs = {
> @@ -45,19 +59,17 @@ static const struct drm_bridge_funcs drm_test_bridge_legacy_funcs = {
>  static void drm_test_bridge_atomic_enable(struct drm_bridge *bridge,
>  					  struct drm_atomic_state *state)
>  {
> -	struct drm_bridge_init_priv *priv =
> -		container_of(bridge, struct drm_bridge_init_priv, bridge);
> +	struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
>  
> -	priv->enable_count++;
> +	dummy_br->enable_count++;
>  }
>  
>  static void drm_test_bridge_atomic_disable(struct drm_bridge *bridge,
>  					   struct drm_atomic_state *state)
>  {
> -	struct drm_bridge_init_priv *priv =
> -		container_of(bridge, struct drm_bridge_init_priv, bridge);
> +	struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
>  
> -	priv->disable_count++;
> +	dummy_br->disable_count++;
>  }
>  
>  static const struct drm_bridge_funcs drm_test_bridge_atomic_funcs = {
> @@ -102,6 +114,10 @@ drm_test_bridge_init(struct kunit *test, const struct drm_bridge_funcs *funcs)
>  	if (IS_ERR(priv))
>  		return ERR_CAST(priv);
>  
> +	priv->test_bridge = devm_drm_bridge_alloc(dev, struct dummy_drm_bridge, bridge, funcs);
> +	if (IS_ERR(priv->test_bridge))
> +		return ERR_CAST(priv->test_bridge);
> +
>  	drm = &priv->drm;
>  	priv->plane = drm_kunit_helper_create_primary_plane(test, drm,
>  							    NULL,
> @@ -125,9 +141,8 @@ drm_test_bridge_init(struct kunit *test, const struct drm_bridge_funcs *funcs)
>  
>  	enc->possible_crtcs = drm_crtc_mask(priv->crtc);
>  
> -	bridge = &priv->bridge;
> +	bridge = &priv->test_bridge->bridge;
>  	bridge->type = DRM_MODE_CONNECTOR_VIRTUAL;
> -	bridge->funcs = funcs;
>  
>  	ret = drm_kunit_bridge_add(test, bridge);
>  	if (ret)
> @@ -173,7 +188,7 @@ static void drm_test_drm_bridge_get_current_state_atomic(struct kunit *test)
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>  
>  retry_commit:
> -	bridge = &priv->bridge;
> +	bridge = &priv->test_bridge->bridge;
>  	bridge_state = drm_atomic_get_bridge_state(state, bridge);
>  	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, bridge_state);
>  
> @@ -228,7 +243,7 @@ static void drm_test_drm_bridge_get_current_state_legacy(struct kunit *test)
>  	 * locking. The function would return NULL in all cases anyway,
>  	 * so we don't really have any concurrency to worry about.
>  	 */
> -	bridge = &priv->bridge;
> +	bridge = &priv->test_bridge->bridge;
>  	KUNIT_EXPECT_NULL(test, drm_bridge_get_current_state(bridge));
>  }
>  
> @@ -253,7 +268,7 @@ static void drm_test_drm_bridge_helper_reset_crtc_atomic(struct kunit *test)
>  	struct drm_modeset_acquire_ctx ctx;
>  	struct drm_bridge_init_priv *priv;
>  	struct drm_display_mode *mode;
> -	struct drm_bridge *bridge;
> +	struct dummy_drm_bridge *dummy_br;

and bridge_priv here.

The rest looks good
Maxime

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

Re: [PATCH v8 3/3] drm/tests: bridge: add KUnit tests for devm_drm_bridge_alloc()

[Maxime Ripard]

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

On Fri, May 16, 2025 at 06:48:39PM +0200, Luca Ceresoli wrote:
> Add KUnit tests for the newly introduced devm_drm_bridge_alloc().
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Changed in v8:
>  - rebase on new patch converting drm_bridge_test.c to
>    devm_drm_bridge_alloc()
>  - add check that bridge is removed (thanks to the .destroy callback)
>  - add a check with get/put
> 
> 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 | 84 +++++++++++++++++++++++++++++++++
>  1 file changed, 84 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_bridge_test.c b/drivers/gpu/drm/tests/drm_bridge_test.c
> index f3a625c536f610dc8560b56531056df7c613f564..32db3a82fe6d14a3e9d6536bcf4b19f1bc65969a 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>
>  
>  /*
> @@ -21,6 +22,7 @@ struct dummy_drm_bridge {
>  	unsigned int enable_count;
>  	unsigned int disable_count;
>  	struct drm_bridge bridge;
> +	void *data;
>  };
>  
>  struct drm_bridge_init_priv {
> @@ -422,11 +424,93 @@ 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 {

drm_bridge_alloc_priv

> +	struct device *dev;
> +	struct dummy_drm_bridge *dummy_br;
> +	bool destroyed;

This can be in drm_bridge_priv

> +};
> +
> +static void dummy_drm_bridge_destroy(struct drm_bridge *bridge)
> +{
> +	struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
> +	struct drm_bridge_alloc_test_ctx *ctx = (struct drm_bridge_alloc_test_ctx *)dummy_br->data;
> +
> +	ctx->destroyed = true;
> +}
> +
> +static const struct drm_bridge_funcs drm_bridge_dummy_funcs = {
> +	.destroy = dummy_drm_bridge_destroy,
> +};

And same here, you don't need to create yet another function set, just
add it to the existing ones.

> +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;
> +
> +	ctx->dummy_br = devm_drm_bridge_alloc(ctx->dev, struct dummy_drm_bridge, bridge,
> +					      &drm_bridge_dummy_funcs);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx->dummy_br);
> +
> +	ctx->dummy_br->data = ctx;
> +
> +	KUNIT_ASSERT_FALSE(test, ctx->destroyed);
> +
> +	return 0;
> +}
> +
> +static void drm_test_drm_bridge_alloc_basic(struct kunit *test)

You need a comment explaining what this test is about

> +{
> +	struct drm_bridge_alloc_test_ctx *ctx = test->priv;
> +
> +	KUNIT_ASSERT_FALSE(test, ctx->destroyed);
> +
> +	kunit_device_unregister(test, ctx->dev);
> +	KUNIT_ASSERT_TRUE(test, ctx->destroyed);

EXPECT

> +}
> +
> +static void drm_test_drm_bridge_alloc_get_put(struct kunit *test)

Comment here

> +{
> +	struct drm_bridge_alloc_test_ctx *ctx = test->priv;
> +
> +	KUNIT_ASSERT_FALSE(test, ctx->destroyed);
> +
> +	drm_bridge_get(&ctx->dummy_br->bridge);
> +	KUNIT_ASSERT_FALSE(test, ctx->destroyed);

EXPECT

> +	kunit_device_unregister(test, ctx->dev);
> +	KUNIT_ASSERT_FALSE(test, ctx->destroyed);

Ditto

> +	drm_bridge_put(&ctx->dummy_br->bridge);
> +	KUNIT_ASSERT_TRUE(test, ctx->destroyed);

Ditto

> +}
> +
> +static struct kunit_case drm_bridge_alloc_tests[] = {
> +	KUNIT_CASE(drm_test_drm_bridge_alloc_basic),
> +	KUNIT_CASE(drm_test_drm_bridge_alloc_get_put),
> +	{ }
> +};
> +
> +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");

Looks good otherwise, thanks
Maxime

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

Re: [PATCH v8 1/3] drm/tests: bridge: convert to devm_drm_bridge_alloc() API

[Anusha Srivatsa]

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

On Fri, May 16, 2025 at 12:48 PM Luca Ceresoli <luca.ceresoli@bootlin.com>
wrote:

> Use the new DRM bridge allocation API, which is the only supported now, for
> the kunit tests.
>
> This change is more massive than for the typical DRM bridge driver because
> struct drm_bridge_init_priv currently embeds a struct drm_bridge, which is
> not supported anymore. We new have to use devm_drm_bridge_alloc() to
>
                                            typo ^^^s/new/now.

Thanks,
Anusha

dynamically allocate a "private driver struct", which is a bit awkward here
> because there is no real bridge driver. Thus let's add a "dummy" DRM bridge
> struct to represent it.
>
> As a nice cleanup we can now move the enable_count and disable_count
> members, which are counting bridge-specific events, into the new "private
> driver struct" (and avoid adding new unnecessary indirections).
>
> Also add a trivial bridge_to_dummy_bridge() just like many drivers do.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>
> ---
>
> This patch was added in v8.
> ---
>  drivers/gpu/drm/tests/drm_bridge_test.c | 95
> +++++++++++++++++++--------------
>  1 file changed, 55 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_bridge_test.c
> b/drivers/gpu/drm/tests/drm_bridge_test.c
> index
> ff88ec2e911c9cc9a718483f09d4c764f45f991a..f3a625c536f610dc8560b56531056df7c613f564
> 100644
> --- a/drivers/gpu/drm/tests/drm_bridge_test.c
> +++ b/drivers/gpu/drm/tests/drm_bridge_test.c
> @@ -10,31 +10,45 @@
>
>  #include <kunit/test.h>
>
> +/*
> + * Mimick the typical struct defined by a bridge driver, which embeds a
> + * bridge plus other fields.
> + *
> + * Having at least one member before @bridge ensures we test non-zero
> + * @bridge offset.
> + */
> +struct dummy_drm_bridge {
> +       unsigned int enable_count;
> +       unsigned int disable_count;
> +       struct drm_bridge 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;
>  };
>
> +static struct dummy_drm_bridge *bridge_to_dummy_bridge(struct drm_bridge
> *bridge)
> +{
> +       return container_of(bridge, struct dummy_drm_bridge, bridge);
> +}
> +
>  static void drm_test_bridge_enable(struct drm_bridge *bridge)
>  {
> -       struct drm_bridge_init_priv *priv =
> -               container_of(bridge, struct drm_bridge_init_priv, bridge);
> +       struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
>
> -       priv->enable_count++;
> +       dummy_br->enable_count++;
>  }
>
>  static void drm_test_bridge_disable(struct drm_bridge *bridge)
>  {
> -       struct drm_bridge_init_priv *priv =
> -               container_of(bridge, struct drm_bridge_init_priv, bridge);
> +       struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
>
> -       priv->disable_count++;
> +       dummy_br->disable_count++;
>  }
>
>  static const struct drm_bridge_funcs drm_test_bridge_legacy_funcs = {
> @@ -45,19 +59,17 @@ static const struct drm_bridge_funcs
> drm_test_bridge_legacy_funcs = {
>  static void drm_test_bridge_atomic_enable(struct drm_bridge *bridge,
>                                           struct drm_atomic_state *state)
>  {
> -       struct drm_bridge_init_priv *priv =
> -               container_of(bridge, struct drm_bridge_init_priv, bridge);
> +       struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
>
> -       priv->enable_count++;
> +       dummy_br->enable_count++;
>  }
>
>  static void drm_test_bridge_atomic_disable(struct drm_bridge *bridge,
>                                            struct drm_atomic_state *state)
>  {
> -       struct drm_bridge_init_priv *priv =
> -               container_of(bridge, struct drm_bridge_init_priv, bridge);
> +       struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
>
> -       priv->disable_count++;
> +       dummy_br->disable_count++;
>  }
>
>  static const struct drm_bridge_funcs drm_test_bridge_atomic_funcs = {
> @@ -102,6 +114,10 @@ drm_test_bridge_init(struct kunit *test, const struct
> drm_bridge_funcs *funcs)
>         if (IS_ERR(priv))
>                 return ERR_CAST(priv);
>
> +       priv->test_bridge = devm_drm_bridge_alloc(dev, struct
> dummy_drm_bridge, bridge, funcs);
> +       if (IS_ERR(priv->test_bridge))
> +               return ERR_CAST(priv->test_bridge);
> +
>         drm = &priv->drm;
>         priv->plane = drm_kunit_helper_create_primary_plane(test, drm,
>                                                             NULL,
> @@ -125,9 +141,8 @@ drm_test_bridge_init(struct kunit *test, const struct
> drm_bridge_funcs *funcs)
>
>         enc->possible_crtcs = drm_crtc_mask(priv->crtc);
>
> -       bridge = &priv->bridge;
> +       bridge = &priv->test_bridge->bridge;
>         bridge->type = DRM_MODE_CONNECTOR_VIRTUAL;
> -       bridge->funcs = funcs;
>
>         ret = drm_kunit_bridge_add(test, bridge);
>         if (ret)
> @@ -173,7 +188,7 @@ static void
> drm_test_drm_bridge_get_current_state_atomic(struct kunit *test)
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>
>  retry_commit:
> -       bridge = &priv->bridge;
> +       bridge = &priv->test_bridge->bridge;
>         bridge_state = drm_atomic_get_bridge_state(state, bridge);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, bridge_state);
>
> @@ -228,7 +243,7 @@ static void
> drm_test_drm_bridge_get_current_state_legacy(struct kunit *test)
>          * locking. The function would return NULL in all cases anyway,
>          * so we don't really have any concurrency to worry about.
>          */
> -       bridge = &priv->bridge;
> +       bridge = &priv->test_bridge->bridge;
>         KUNIT_EXPECT_NULL(test, drm_bridge_get_current_state(bridge));
>  }
>
> @@ -253,7 +268,7 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_atomic(struct kunit *test)
>         struct drm_modeset_acquire_ctx ctx;
>         struct drm_bridge_init_priv *priv;
>         struct drm_display_mode *mode;
> -       struct drm_bridge *bridge;
> +       struct dummy_drm_bridge *dummy_br;
>         int ret;
>
>         priv = drm_test_bridge_init(test, &drm_test_bridge_atomic_funcs);
> @@ -279,14 +294,14 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_atomic(struct kunit *test)
>         drm_modeset_drop_locks(&ctx);
>         drm_modeset_acquire_fini(&ctx);
>
> -       bridge = &priv->bridge;
> -       KUNIT_ASSERT_EQ(test, priv->enable_count, 1);
> -       KUNIT_ASSERT_EQ(test, priv->disable_count, 0);
> +       dummy_br = priv->test_bridge;
> +       KUNIT_ASSERT_EQ(test, dummy_br->enable_count, 1);
> +       KUNIT_ASSERT_EQ(test, dummy_br->disable_count, 0);
>
>         drm_modeset_acquire_init(&ctx, 0);
>
>  retry_reset:
> -       ret = drm_bridge_helper_reset_crtc(bridge, &ctx);
> +       ret = drm_bridge_helper_reset_crtc(&dummy_br->bridge, &ctx);
>         if (ret == -EDEADLK) {
>                 drm_modeset_backoff(&ctx);
>                 goto retry_reset;
> @@ -296,8 +311,8 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_atomic(struct kunit *test)
>         drm_modeset_drop_locks(&ctx);
>         drm_modeset_acquire_fini(&ctx);
>
> -       KUNIT_EXPECT_EQ(test, priv->enable_count, 2);
> -       KUNIT_EXPECT_EQ(test, priv->disable_count, 1);
> +       KUNIT_EXPECT_EQ(test, dummy_br->enable_count, 2);
> +       KUNIT_EXPECT_EQ(test, dummy_br->disable_count, 1);
>  }
>
>  /*
> @@ -309,7 +324,7 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_atomic_disabled(struct kunit *
>         struct drm_modeset_acquire_ctx ctx;
>         struct drm_bridge_init_priv *priv;
>         struct drm_display_mode *mode;
> -       struct drm_bridge *bridge;
> +       struct dummy_drm_bridge *dummy_br;
>         int ret;
>
>         priv = drm_test_bridge_init(test, &drm_test_bridge_atomic_funcs);
> @@ -318,14 +333,14 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_atomic_disabled(struct kunit *
>         mode = drm_kunit_display_mode_from_cea_vic(test, &priv->drm, 16);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mode);
>
> -       bridge = &priv->bridge;
> -       KUNIT_ASSERT_EQ(test, priv->enable_count, 0);
> -       KUNIT_ASSERT_EQ(test, priv->disable_count, 0);
> +       dummy_br = priv->test_bridge;
> +       KUNIT_ASSERT_EQ(test, dummy_br->enable_count, 0);
> +       KUNIT_ASSERT_EQ(test, dummy_br->disable_count, 0);
>
>         drm_modeset_acquire_init(&ctx, 0);
>
>  retry_reset:
> -       ret = drm_bridge_helper_reset_crtc(bridge, &ctx);
> +       ret = drm_bridge_helper_reset_crtc(&dummy_br->bridge, &ctx);
>         if (ret == -EDEADLK) {
>                 drm_modeset_backoff(&ctx);
>                 goto retry_reset;
> @@ -335,8 +350,8 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_atomic_disabled(struct kunit *
>         drm_modeset_drop_locks(&ctx);
>         drm_modeset_acquire_fini(&ctx);
>
> -       KUNIT_EXPECT_EQ(test, priv->enable_count, 0);
> -       KUNIT_EXPECT_EQ(test, priv->disable_count, 0);
> +       KUNIT_EXPECT_EQ(test, dummy_br->enable_count, 0);
> +       KUNIT_EXPECT_EQ(test, dummy_br->disable_count, 0);
>  }
>
>  /*
> @@ -348,7 +363,7 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_legacy(struct kunit *test)
>         struct drm_modeset_acquire_ctx ctx;
>         struct drm_bridge_init_priv *priv;
>         struct drm_display_mode *mode;
> -       struct drm_bridge *bridge;
> +       struct dummy_drm_bridge *dummy_br;
>         int ret;
>
>         priv = drm_test_bridge_init(test, &drm_test_bridge_legacy_funcs);
> @@ -374,14 +389,14 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_legacy(struct kunit *test)
>         drm_modeset_drop_locks(&ctx);
>         drm_modeset_acquire_fini(&ctx);
>
> -       bridge = &priv->bridge;
> -       KUNIT_ASSERT_EQ(test, priv->enable_count, 1);
> -       KUNIT_ASSERT_EQ(test, priv->disable_count, 0);
> +       dummy_br = priv->test_bridge;
> +       KUNIT_ASSERT_EQ(test, dummy_br->enable_count, 1);
> +       KUNIT_ASSERT_EQ(test, dummy_br->disable_count, 0);
>
>         drm_modeset_acquire_init(&ctx, 0);
>
>  retry_reset:
> -       ret = drm_bridge_helper_reset_crtc(bridge, &ctx);
> +       ret = drm_bridge_helper_reset_crtc(&dummy_br->bridge, &ctx);
>         if (ret == -EDEADLK) {
>                 drm_modeset_backoff(&ctx);
>                 goto retry_reset;
> @@ -391,8 +406,8 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_legacy(struct kunit *test)
>         drm_modeset_drop_locks(&ctx);
>         drm_modeset_acquire_fini(&ctx);
>
> -       KUNIT_EXPECT_EQ(test, priv->enable_count, 2);
> -       KUNIT_EXPECT_EQ(test, priv->disable_count, 1);
> +       KUNIT_EXPECT_EQ(test, dummy_br->enable_count, 2);
> +       KUNIT_EXPECT_EQ(test, dummy_br->disable_count, 1);
>  }
>
>  static struct kunit_case drm_bridge_helper_reset_crtc_tests[] = {
>
> --
> 2.49.0
>
>

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

Re: [PATCH v8 2/3] dmr/bridge: add a .destroy func

[Anusha Srivatsa]

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

Luca, while you are sending a new revision, the patch subject says dmr
instead of drm. Kindly make that change.

Thanks,
Anusha


On Mon, May 26, 2025 at 7:51 AM Luca Ceresoli <luca.ceresoli@bootlin.com>
wrote:

> Hi Maxime,
>
> On Thu, 22 May 2025 17:43:37 +0200
> Maxime Ripard <mripard@kernel.org> wrote:
>
> > On Fri, May 16, 2025 at 06:48:38PM +0200, Luca Ceresoli wrote:
> > > Some users of DRM bridges may need to execute specific code just before
> > > deallocation.
> > >
> > > As of now the only known user would be KUnit tests.
> > >
> > > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > >
> > > ---
> > >
> > > This patch is new in v8. The .destroy callback had appeared in v5 as
> well
> > > [5], but as part of a larger patch and for different reason that do not
> > > apply anymore.
> > >
> > > [5]
> https://lore.kernel.org/all/20241231-hotplug-drm-bridge-v5-3-173065a1ece1@bootlin.com/#t
> > > ---
> > >  drivers/gpu/drm/drm_bridge.c |  2 ++
> > >  include/drm/drm_bridge.h     | 10 ++++++++++
> > >  2 files changed, 12 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_bridge.c
> b/drivers/gpu/drm/drm_bridge.c
> > > index
> b4c89ec01998b849018ce031c7cd84614e65e710..6185cb29fe3162264f0912c09c205fb467975dee
> 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -203,6 +203,8 @@ static void __drm_bridge_free(struct kref *kref)
> > >  {
> > >     struct drm_bridge *bridge = container_of(kref, struct drm_bridge,
> refcount);
> > >
> > > +   if (bridge->funcs->destroy)
> > > +           bridge->funcs->destroy(bridge);
> > >     kfree(bridge->container);
> > >  }
> > >
> > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > > index
> 4e418a29a9ff9d014d6ac0910a5d9bcf7118195e..3ccd493faa580845c2ed1166f398eca27b464261
> 100644
> > > --- a/include/drm/drm_bridge.h
> > > +++ b/include/drm/drm_bridge.h
> > > @@ -86,6 +86,16 @@ struct drm_bridge_funcs {
> > >      */
> > >     void (*detach)(struct drm_bridge *bridge);
> > >
> > > +   /**
> > > +    * @destroy:
> > > +    *
> > > +    * This callback is invoked when the bridge is about to be
> > > +    * deallocated.
> > > +    *
> > > +    * The @destroy callback is optional.
> > > +    */
> > > +   void (*destroy)(struct drm_bridge *bridge);
> > > +
> >
> > destroy is before detach in alphabetical order, but otherwise it looks
> > good to me.
>
> I saw the struct is not alpha-ordered right now, so I did not get it
> should be, and it looked like keeping .attach and .detach nearby would
> be good.
>
> > Once fixed,
> > Acked-by: Maxime Ripard <mripard@kernel.org>
>
> OK, will send new iteration with .destroy before .detach, thanks for
> the review.
>
> Luca
>
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
>

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

Re: [PATCH v8 3/3] drm/tests: bridge: add KUnit tests for devm_drm_bridge_alloc()

[Luca Ceresoli]

Hello Maxime,

thanks for reviewing this series.

On Tue, 27 May 2025 18:10:31 +0200
Maxime Ripard <mripard@kernel.org> wrote:

[...]

> > @@ -422,11 +424,93 @@ 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 {  
> 
> drm_bridge_alloc_priv
> 
> > +	struct device *dev;
> > +	struct dummy_drm_bridge *dummy_br;
> > +	bool destroyed;  
> 
> This can be in drm_bridge_priv

Not really, because drm_bridge_priv will be freed just after calling
.destroy, and we need .destroyed after the free happened.

[...]

> > +static const struct drm_bridge_funcs drm_bridge_dummy_funcs = {
> > +	.destroy = dummy_drm_bridge_destroy,
> > +};  
> 
> And same here, you don't need to create yet another function set, just
> add it to the existing ones.

OK, but it implies further changes.

In this version of this patch, the alloc tests being introduced use
drm_bridge_alloc_priv, while the other tests using the existing
function sets rely on drm_bridge_init_priv which has different fields.
So if all tests will call .destroy, we always need a valid struct
pointer for drm_bridge_priv.data.

Based on this, I think the only solution is to not introduce
drm_bridge_alloc_priv, and instead put its members (struct device *dev and bool
destroyed) to drm_bridge_init_priv, and then use drm_bridge_init_priv
for all tests.

The change is not very invasive, and perhaps even a cleanup, thus I'm
going to send as above in v9.


I'm OK with all the other changes you proposed. All queued for v9.

Luca

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