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

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

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