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

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

On Thu, 29 Feb 2024 11:48:27 +0100
Frieder Schrempf <frieder.schrempf@kontron.de> wrote:

> On 29.02.24 10:47, Luca Ceresoli wrote:
> > 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?
> > 
> > I'm very possibly missing something, but this looks like a bug in the
> > DRM bridge code at first sight.  
> 
> I'm just guessing, but could it be that this patch [1] would fix it?
> 
> It looks like nobody cared to pick this up, although there are several
> reports for defects caused by [2] and this patch is supposed to fix them.

It looks like [1] (or the other patches mentioned by Michael in the same
thread?) should be applied indeed. I'm going to test those patches
ASAP, perhaps next week.

However I'm not sure this is related to the problem being discussed
here: the patches about pre_enable_prev_first are touching
atomic_pre_enable and atomic_pre_disable. Here Alexander reported when
removing the bridge atomic_disable is not called, and atomic_disable
is not affected by pre_enable_prev_first.

> [1] https://patchwork.freedesktop.org/patch/529288/
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=4fb912e5e19075874379cfcf074d90bd51ebf8ea

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com