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@xxxxxxxxxx> 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@xxxxxxxxxxx>
> > 
> > ---
> > 
> > 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@xxxxxxxxxxx/

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