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

[Maxime Ripard]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20250515/b8bcac7e/attachment.sig>