Re: [PATCH 1/1] drm/bridge: ti-sn65dsi83: Fix enable error path

[Alexander Stein <alexander.stein@ew.tq-group.com>]

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@ew.tq-group.com> 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 = <&reg_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.

> > I'm very possibly missing something, but this looks like a bug in the
> > DRM bridge code at first sight.

Yes, that seemed fishy for me as well. I should not be able to remove the
bridge driver while the pipeline is running. Weston is still running, but
nevertheless I can remove the bridge driver.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/