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

[Maxime Ripard]

On Fri, May 16, 2025 at 05:38:28PM +0200, Luca Ceresoli wrote:
> > > 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

Thanks for the summary. I still agree with our discussions, however...

> 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-173065a1ece1@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.

... It's still useful. With KMS drivers, you usually get two different
lifetimes: the hardware device and the DRM device. The DRM device will
outlive the hardware device. Thus, the driver instance will still be
live even after the device is gone away, and still need to somewhat
interact with the framework, even if in degraded mode.

All the resources tied to the device (memory mapping, clocks,
interrupts, etc.) are handled by devm. We don't have to take care of
them in destroy, they are long gone before we reach that point.

However, the bridge might still need to allocate memory, create objects,
 to interact with the framework. Things like a private object, or a
 scratch buffer, or... whatever.

But anything that the driver needs to implement what the framework
expects basically. *Those* are what need to be cleaned up in destroy.

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/20250522/0e21e3cf/attachment.sig>