Hello Alexander,
On Fri, 01 Mar 2024 11:45:27 +0100
Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx> wrote:
> Hi Luca,
>
> Am Freitag, 1. März 2024, 11:10:59 CET schrieb Luca Ceresoli:
> > Hello Alexander,
> >
> > On Fri, 01 Mar 2024 10:57:37 +0100
> > Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx> wrote:
> >
> > > Hi Luca,
> > >
> > > Am Freitag, 1. März 2024, 10:44:49 CET schrieb Luca Ceresoli:
> > > > Hello Alexander,
> > > >
> > > > On Thu, 29 Feb 2024 12:11:23 +0100
> > > > Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx> wrote:
> > > >
> > > > > Hi Luca,
> > > > >
> > > > > Am Donnerstag, 29. Februar 2024, 10:47:23 CET schrieb Luca Ceresoli:
> > > > > > Hello Alexander,
> > > > > >
> > > > > > On Wed, 28 Feb 2024 09:15:46 +0100
> > > > > > Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx> wrote:
> > > > > >
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > Oh I mistook this DSI-LVDS bridge with the DSI-DP bridge on a different
> > > > > > > board, my bad. I hope I can provide some insights. My platform is
> > > > > > > imx8mm-tqma8mqml-mba8mx-lvds-tm070jvhg33.dtb.
> > > > > > > I can easily cause a PLL lock failure by reducing the delay for the
> > > > > > > enable-gpios 'gpio_delays'. This will result in a PLL lock faiure.
> > > > > > > On my platform the vcc-supply counters do look sane:
> > > > > > > > /sys/kernel/debug/regulator/SN65DSI83_1V8/open_count:1
> > > > > > > > /sys/kernel/debug/regulator/SN65DSI83_1V8/use_count:0
> > > > > >
> > > > > > Interesting. Thanks for taking time to report your initial issue!
> > > > > >
> > > > > > > Once I remove the ti_sn65dsi83 module, the open_count decrements to 0 as
> > > > > > > well. Looks sane to me.
> > > > > > >
> > > > > > > If I revert commit c81cd8f7c774 ("Revert "drm/bridge: ti-sn65dsi83:
> > > > > > > Fix enable error path""), vcc-supply counters are:
> > > > > > > > /sys/kernel/debug/regulator/SN65DSI83_1V8/open_count:1
> > > > > > > > /sys/kernel/debug/regulator/SN65DSI83_1V8/use_count:1
> > > > > > >
> > > > > > > So in my case the use_count does not decrease! If I remove the module
> > > > > > > ti_sn65dsi83, I get the WARN_ON (enable_count is still non-zero):
> > > > > > > > WARNING: CPU: 2 PID: 402 at drivers/regulator/core.c:2398 _regulator_put+0x15c/0x164
> > > > > > >
> > > > > > > This is on 6.8.0-rc6-next-20240228 with the following diff applied:
> > > > > > > --->8---
> > > > > > > diff --git a/arch/arm64/boot/dts/freescale/mba8mx.dtsi b/arch/arm64/boot/dts/freescale/mba8mx.dtsi
> > > > > > > index 427467df42bf..8461e1fd396f 100644
> > > > > > > --- a/arch/arm64/boot/dts/freescale/mba8mx.dtsi
> > > > > > > +++ b/arch/arm64/boot/dts/freescale/mba8mx.dtsi
> > > > > > > @@ -285,7 +285,7 @@ &i2c3 {
> > > > > > > dsi_lvds_bridge: bridge@2d {
> > > > > > > compatible = "ti,sn65dsi84";
> > > > > > > reg = <0x2d>;
> > > > > > > - enable-gpios = <&gpio_delays 0 130000 0>;
> > > > > > > + enable-gpios = <&gpio_delays 0 0 0>;
> > > > > > > vcc-supply = <®_sn65dsi83_1v8>;
> > > > > > > status = "disabled";
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > > > > > index 4814b7b6d1fd..57a7ed13f996 100644
> > > > > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > > > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
> > > > > > > @@ -478,7 +478,6 @@ static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge,
> > > > > > > dev_err(ctx->dev, "failed to lock PLL, ret=%i\n", ret);
> > > > > > > /* On failure, disable PLL again and exit. */
> > > > > > > regmap_write(ctx->regmap, REG_RC_PLL_EN, 0x00);
> > > > > > > - regulator_disable(ctx->vcc);
> > > > > > > return;
> > > > > > > }
> > > > > > > --->8---
> > > > > > >
> > > > > > > So my patch indeed did fix an actual problem. On the other hand it seems
> > > > > > > sn65dsi83_atomic_disable is not called in my case for some reason.
> > > > > >
> > > > > > So you remove the module and atomic_disable is not called, after
> > > > > > having called atomic_pre_enable?
> > > > >
> > > > > Yes, that's the case.
> > > >
> > > > Ah, it's quite obvious looking at the code: removing the module will
> > > > call sn65dsi83_remove()
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/ti-sn65dsi83.c#L729
> > > >
> > > > which does just call drm_bridge_remove()
> > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_bridge.c#L243
> > > >
> > > > which just removes the bridge from the list.
> > > >
> > > > So maybe sn65dsi83_remove() should call regulator_disable() as a last
> > > > resort, but I'm not sure this is the correct solution and it would
> > > > involve some housekeeping to not disable the regulator more times than
> > > > it has been enabled.
> > >
> > > Actually I think removing the module should be prohibited while the bridge
> > > is enabled in the first place.
> > >
> > > > What is the use case you have for removing the driver module?
> > >
> > > I was dealing the PLL lock failure myself, caused by some external delays.
> > > For easy testing I was loading/unloading the module.
> >
> > Ah, I see, so do you agree that we can say:
> > 1. there is no valid use case for rmmod while the pipeline is running
> > (I'm not counting debugging here)
> > 2. so the regulator_disable() in pre_enable is not needed
> > ?
>
> Yes, that seems reasonable. I'm wondering how 1. can actually prevented though.
I agree rmmod should be handled better. I also think not being able to
return an error from the atomic_[pre_]enable ops seems bad. Both these
aspects are way larger than the scope of this patch however...
So, back to this patch, I've sent a revert patch, trying to summarize
the history we discussed here, and I'd be glad to get your comments and
ACK/NACK on that patch:
https://lore.kernel.org/dri-devel/20240306-ti-sn65dsi83-regulator-imbalance-v1-1-a3cea5f3e5b3@xxxxxxxxxxx/
Thanks for the discussion!
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
