Hi Maxime,
On Thu, 15 May 2025 10:11:33 +0200
Maxime Ripard <mrip...@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-173065a1e...@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
