On Wed, Sep 10, 2025 at 06:47:52PM +0200, Luca Ceresoli wrote: > On Wed, 10 Sep 2025 12:59:12 +0200 > Maxime Ripard <[email protected]> wrote: > > > On Mon, Sep 08, 2025 at 03:49:06PM +0200, Luca Ceresoli wrote: > > > Hi Maxime, > > > > > > On Wed, 27 Aug 2025 09:46:03 +0200 > > > Maxime Ripard <[email protected]> wrote: > > > > > > > On Wed, Aug 20, 2025 at 01:13:02PM +0200, Luca Ceresoli wrote: > > > > > Hello Maxime, > > > > > > > > > > On Tue, 19 Aug 2025 14:29:32 +0200 > > > > > Maxime Ripard <[email protected]> wrote: > > > > > > > > > > > > @@ -1005,7 +1041,24 @@ static void sn65dsi83_remove(struct > > > > > > > i2c_client *client) > > > > > > > { > > > > > > > struct sn65dsi83 *ctx = i2c_get_clientdata(client); > > > > > > > > > > > > > > + drm_bridge_unplug(&ctx->bridge); > > > > > > > drm_bridge_remove(&ctx->bridge); > > > > > > > > > > > > Shouldn't we merge drm_bridge_unplug with the release part of > > > > > > devm_drm_bridge_alloc? > > > > > > > > > > I'm not sure I got what you are suggesting here, sorry. > > > > > > > > > > Do you mean that __devm_drm_bridge_alloc() should add a devres action > > > > > to call drm_bridge_unplug(), so the unplug is called implicitly and > > > > > does not need to be called explicitly by all drivers? > > > > > > > > Yes > > > > > > > > > If that's what you mean, I don't think that would work. Unless I'm > > > > > missing something, devres actions are always invoked just after the > > > > > driver .remove callback. > > > > > > > > Yes, they are called in reverse order of registration, after remove. > > > > > > > > > But we need to call drm_bridge_unplug() at the beginning (or just > > > > > before) .remove, at least for drivers that need to do something in > > > > > .remove that cannot be done by devm. > > > > > > > > > > In pseudocode: > > > > > > > > > > mybridge_remove() > > > > > { > > > > > drm_bridge_unplug(); <-- explicit call as in my patch > > > > > xyz_disable(); > > > > > drm_bridge_unplug(); <-- implicitly done by devres > > > > > } > > > > > > > > > > We want xyz_disable() to be done after drm_bridge_unplug(), so other > > > > > code paths using drm_bridge_enter/exit() won't mess with xyz. > > > > > > > > It's not clear to me why doing it before xyz_disable() is important > > > > here? If anything, it would prevent from disabling the hardware for > > > > example, even though you still have your memory mapping, clocks, power > > > > domains, regulators, etc. to properly disable it. > > > > > > > > You're still correct that it's a bad idea though because we want to do > > > > it before we start freeing all those, so it needs to execute as the > > > > before the devm actions ... > > > > > > > > > devres actions cannot be added to be executed _before_ .remove, > > > > > AFAIK. > > > > > > > > ... and we can't do that either. > > > > > > I understand your words as "the drm_bridge_unplug() is OK where it is, > > > your patch is OK in this respect". Correct? > > > > > > So if this is correct, and my reply on the devres cleanups is also > > > correct (other reply in this thread), that means the whole patch is OK. > > > > I'm still confused why it's so important than in your example > > xyz_disable must be called after drm_bridge_unplug. > > Let me clarify with an example. > > As I wrote in another reply, I have moved from a flag > (disable_resources_needed) to a devres action as you had suggested, but > the example here is based on the old flag because it is more explicit, > code would be executed in the same order anyway, and, well, because I > had written the example before the devres action conversion. > > Take these two functions (stripped versions of the actual ones): > > /* Same as proposed, but with _unplug moved at the end */ > static void sn65dsi83_remove() > { > struct sn65dsi83 *ctx = i2c_get_clientdata(client); > > drm_bridge_remove(&ctx->bridge); > > /* > * I moved the following code to a devm action, but keeping it > * explicit here for the discussion > */ > if (ctx->disable_resources_needed) { > sn65dsi83_monitor_stop(ctx); > regulator_disable(ctx->vcc); > } > > drm_bridge_unplug(&ctx->bridge); // At the end! > }
First off, why do we need to have drm_bridge_unplug and
drm_bridge_remove separate?
If we were to mirror drm_dev_enter and drm_dev_unplug, drm_dev_unplug
calls drm_dev_unregister itself, and I can't find a reason where we
might want to split the two.
> static void sn65dsi83_atomic_disable()
> {
> if (!drm_bridge_enter(bridge, &idx))
> return;
>
> /* These 3 lines will be replaced by devm_release_action() */
> ctx->disable_resources_needed = false;
> sn65dsi83_monitor_stop(ctx);
> regulator_disable(ctx->vcc);
>
> drm_bridge_exit(idx);
> }
>
> Here the xyz_disable() in my pseudocode is the sn65dsi83_monitor_stop()
> + regulator_disable().
>
> If sn65dsi83_remove() and sn65dsi83_atomic_disable() were to happen
> concurrently, this sequence of events could happen:
>
> 1. atomic_disable: drm_bridge_enter() -> OK, can go
> 2. remove: drm_bridge_remove()
> 3. remove: sn65dsi83_monitor_stop()
> 4. remove: regulator_disable()
> 5. remove: drm_bridge_unplug() -- too late to stop atomic_disable
drm_dev_unplug would also get delayed until drm_dev_exit is called,
mitigating your issue here.
> 6. atomic_disable: ctx->disable_resources_needed = false -- too late to stop
> .remove
> 7. atomic_disable: sn65dsi83_monitor_stop() -- twice, maybe no problem
> 8. atomic_disable: regulator_disable() -- Twice, en/disable imbalance!
>
> So there is an excess regulator disable, which is an error. I don't see
> how this can be avoided if the drm_bridge_unplug() is called after the
> regulator_disable().
>
> Let me know whether this clarifies the need to _unplug at the beginning
> of the .remove function.
Another thing that just crossed my mind is why we don't call
atomic_disable when we're tearing down the bridge too. We're doing it
for the main DRM devices, it would make sense to me to disable the
encoder -> bridge -> connector (and possibly CRTC) chain if we remove a
bridge automatically.
Maxime
signature.asc
Description: PGP signature
