| Message ID | 20230504065316.2640739-1-alexander.stein@ew.tq-group.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/1] drm/bridge: ti-sn65dsi83: Fix enable error path | expand |
Hi Alexander, Thank you for the patch. On Thu, May 04, 2023 at 08:53:16AM +0200, Alexander Stein wrote: > If PLL locking failed, the regulator needs to be disabled again. > > Fixes: 5664e3c907e2 ("drm/bridge: ti-sn65dsi83: Add vcc supply regulator support") > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > index 75286c9afbb9..1f5c07989e2b 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > @@ -478,6 +478,7 @@ static void sn65dsi83_atomic_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; > } >
From: Robert Foss <rfoss@kernel.org> On Thu, 4 May 2023 08:53:16 +0200, Alexander Stein wrote: > If PLL locking failed, the regulator needs to be disabled again. > > Applied, thanks! [1/1] drm/bridge: ti-sn65dsi83: Fix enable error path https://cgit.freedesktop.org/drm/drm-misc/commit/?id=8a91b29f1f50 Rob
Hello Alexander, On Thu, 4 May 2023 08:53:16 +0200 Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > If PLL locking failed, the regulator needs to be disabled again. > > Fixes: 5664e3c907e2 ("drm/bridge: ti-sn65dsi83: Add vcc supply regulator support") > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > --- > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > index 75286c9afbb9..1f5c07989e2b 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > @@ -478,6 +478,7 @@ static void sn65dsi83_atomic_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; > } I'm reviving this thread as I've been investigating a bug that appears related to this patch. Symptom: with a v6.8-rc5 kernel, if PLL fails locking, later on during atomic disable I get: [ 41.065198] ------------[ cut here ]------------ [ 41.069823] unbalanced disables for DOCK_SYS_1V8 [ 41.074482] WARNING: CPU: 0 PID: 58 at drivers/regulator/core.c:2999 _regulator_disable+0xf8/0x1d8 [ 41.083457] Modules linked in: smsc smsc95xx usbnet mii imx_cpufreq_dt exc3000 imx8mm_thermal snd_soc_tlv320aic3x_spi snd_soc_tlv320aic3x_i2c snd_soc_tlv320aic3x tmp103 snd_soc_simple_card snd_soc_simple_card_utils fsl_ldb rtc_snvs snvs_pwrkey snd_soc_fsl_sai imx8mp_interconnect snd_soc_fsl_utils imx_interconnect imx_pcm_dma rtc_rs5c372 ti_sn65dsi83 pwm_imx27 st_pressure_spi st_sensors_spi st_pressure_i2c st_pressure st_sensors_i2c industrialio_triggered_buffer lm75 kfifo_buf st_sensors opt3001 panel_simple etnaviv gpu_sched iio_hwmon governor_userspace imx_bus imx8mp_hdmi_tx dw_hdmi drm_display_helper samsung_dsim imx_sdma imx_lcdif drm_dma_helper imx8mp_hdmi_pvi drm_kms_helper drm drm_panel_orientation_quirks fsl_imx8_ddr_perf caam error sbs_battery pwm_bl backlight ltc2497 ltc2497_core crct10dif_ce [ 41.157281] CPU: 0 PID: 58 Comm: kworker/0:2 Not tainted 6.8.0-rc5+ #7 [ 41.170339] Workqueue: events drm_mode_rmfb_work_fn [drm] [ 41.175798] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) [ 41.182762] pc : _regulator_disable+0xf8/0x1d8 [ 41.187209] lr : _regulator_disable+0xf8/0x1d8 [ 41.191654] sp : ffff800081aaba90 [ 41.194967] x29: ffff800081aaba90 x28: 0000000000000000 x27: ffff000002647e80 [ 41.202109] x26: ffff000002d7a180 x25: ffff0000037858a0 x24: ffff800079748ac8 [ 41.209250] x23: ffff000002647ed8 x22: ffff00000263f800 x21: ffff00000373d000 [ 41.216392] x20: ffff00000373d000 x19: ffff000001de6480 x18: 0000000000000006 [ 41.223533] x17: 0000000000000000 x16: 1fffe000003423e1 x15: ffff800081aab520 [ 41.230674] x14: 0000000000000000 x13: 3856315f5359535f x12: 4b434f4420726f66 [ 41.237815] x11: 2073656c62617369 x10: ffff8000814647a0 x9 : ffff8000801b10e0 [ 41.244957] x8 : ffff8000814bc7a0 x7 : 0000000000017fe8 x6 : ffff8000814bc7a0 [ 41.252098] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000 [ 41.259239] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000011b6600 [ 41.266380] Call trace: [ 41.268826] _regulator_disable+0xf8/0x1d8 [ 41.272925] regulator_disable+0x4c/0x98 [ 41.276850] sn65dsi83_atomic_disable+0x70/0xc0 [ti_sn65dsi83] [ 41.282692] drm_atomic_bridge_chain_disable+0x78/0x110 [drm] [ 41.288481] disable_outputs+0x100/0x350 [drm_kms_helper] [ 41.293902] drm_atomic_helper_commit_tail_rpm+0x2c/0xb0 [drm_kms_helper] [ 41.300705] commit_tail+0xac/0x1a0 [drm_kms_helper] [ 41.305685] drm_atomic_helper_commit+0x16c/0x188 [drm_kms_helper] [ 41.311881] drm_atomic_commit+0xac/0xf0 [drm] [ 41.316365] drm_framebuffer_remove+0x464/0x550 [drm] [ 41.321458] drm_mode_rmfb_work_fn+0x84/0xb0 [drm] [ 41.326291] process_one_work+0x148/0x3b8 [ 41.330309] worker_thread+0x32c/0x450 [ 41.334061] kthread+0x11c/0x128 [ 41.337292] ret_from_fork+0x10/0x20 [ 41.340873] ---[ end trace 0000000000000000 ]--- The reason is clear from the code flow, which looks like this (after removing unrelated code): static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { regulator_enable(ctx->vcc); if (PLL failed locking) { regulator_disable(ctx->vcc); return; } } static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { regulator_disable(ctx->vcc); } So when the PLL fails locking, the vcc regulator is disable twice, leading to "unbalanced disables". I initially removed the regulator_disable() line in sn65dsi83_atomic_pre_enable() locally and it worked fine. Then I did some git log and found you added this line on purpose (even though it was in sn65dsi83_atomic_enable() initially), so my question is whether you can explain exactly what was wrong before your patch. I have been working for a few weeks with the regulator_disable() line removed and found no issue. Best regards, Luca
Hi Luca, Am Donnerstag, 22. Februar 2024, 16:36:37 CET schrieb Luca Ceresoli: > Hello Alexander, > > On Thu, 4 May 2023 08:53:16 +0200 > Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > > If PLL locking failed, the regulator needs to be disabled again. > > > > Fixes: 5664e3c907e2 ("drm/bridge: ti-sn65dsi83: Add vcc supply regulator support") > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > index 75286c9afbb9..1f5c07989e2b 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > @@ -478,6 +478,7 @@ static void sn65dsi83_atomic_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; > > } > > I'm reviving this thread as I've been investigating a bug that appears > related to this patch. > > Symptom: with a v6.8-rc5 kernel, if PLL fails locking, later on during > atomic disable I get: > > [ 41.065198] ------------[ cut here ]------------ > [ 41.069823] unbalanced disables for DOCK_SYS_1V8 > [ 41.074482] WARNING: CPU: 0 PID: 58 at drivers/regulator/core.c:2999 _regulator_disable+0xf8/0x1d8 > [ 41.083457] Modules linked in: smsc smsc95xx usbnet mii imx_cpufreq_dt exc3000 imx8mm_thermal snd_soc_tlv320aic3x_spi snd_soc_tlv320aic3x_i2c snd_soc_tlv320aic3x tmp103 snd_soc_simple_card snd_soc_simple_card_utils fsl_ldb rtc_snvs snvs_pwrkey snd_soc_fsl_sai imx8mp_interconnect snd_soc_fsl_utils imx_interconnect imx_pcm_dma rtc_rs5c372 ti_sn65dsi83 pwm_imx27 st_pressure_spi st_sensors_spi st_pressure_i2c st_pressure st_sensors_i2c industrialio_triggered_buffer lm75 kfifo_buf st_sensors opt3001 panel_simple etnaviv gpu_sched iio_hwmon governor_userspace imx_bus imx8mp_hdmi_tx dw_hdmi drm_display_helper samsung_dsim imx_sdma imx_lcdif drm_dma_helper imx8mp_hdmi_pvi drm_kms_helper drm drm_panel_orientation_quirks fsl_imx8_ddr_perf caam error sbs_battery pwm_bl backlight ltc2497 ltc2497_core crct10dif_ce > [ 41.157281] CPU: 0 PID: 58 Comm: kworker/0:2 Not tainted 6.8.0-rc5+ #7 > [ 41.170339] Workqueue: events drm_mode_rmfb_work_fn [drm] > [ 41.175798] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > [ 41.182762] pc : _regulator_disable+0xf8/0x1d8 > [ 41.187209] lr : _regulator_disable+0xf8/0x1d8 > [ 41.191654] sp : ffff800081aaba90 > [ 41.194967] x29: ffff800081aaba90 x28: 0000000000000000 x27: ffff000002647e80 > [ 41.202109] x26: ffff000002d7a180 x25: ffff0000037858a0 x24: ffff800079748ac8 > [ 41.209250] x23: ffff000002647ed8 x22: ffff00000263f800 x21: ffff00000373d000 > [ 41.216392] x20: ffff00000373d000 x19: ffff000001de6480 x18: 0000000000000006 > [ 41.223533] x17: 0000000000000000 x16: 1fffe000003423e1 x15: ffff800081aab520 > [ 41.230674] x14: 0000000000000000 x13: 3856315f5359535f x12: 4b434f4420726f66 > [ 41.237815] x11: 2073656c62617369 x10: ffff8000814647a0 x9 : ffff8000801b10e0 > [ 41.244957] x8 : ffff8000814bc7a0 x7 : 0000000000017fe8 x6 : ffff8000814bc7a0 > [ 41.252098] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000 > [ 41.259239] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000011b6600 > [ 41.266380] Call trace: > [ 41.268826] _regulator_disable+0xf8/0x1d8 > [ 41.272925] regulator_disable+0x4c/0x98 > [ 41.276850] sn65dsi83_atomic_disable+0x70/0xc0 [ti_sn65dsi83] > [ 41.282692] drm_atomic_bridge_chain_disable+0x78/0x110 [drm] > [ 41.288481] disable_outputs+0x100/0x350 [drm_kms_helper] > [ 41.293902] drm_atomic_helper_commit_tail_rpm+0x2c/0xb0 [drm_kms_helper] > [ 41.300705] commit_tail+0xac/0x1a0 [drm_kms_helper] > [ 41.305685] drm_atomic_helper_commit+0x16c/0x188 [drm_kms_helper] > [ 41.311881] drm_atomic_commit+0xac/0xf0 [drm] > [ 41.316365] drm_framebuffer_remove+0x464/0x550 [drm] > [ 41.321458] drm_mode_rmfb_work_fn+0x84/0xb0 [drm] > [ 41.326291] process_one_work+0x148/0x3b8 > [ 41.330309] worker_thread+0x32c/0x450 > [ 41.334061] kthread+0x11c/0x128 > [ 41.337292] ret_from_fork+0x10/0x20 > [ 41.340873] ---[ end trace 0000000000000000 ]--- > > The reason is clear from the code flow, which looks like this (after > removing unrelated code): > > static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, > struct drm_bridge_state *old_bridge_state) > { > regulator_enable(ctx->vcc); > > if (PLL failed locking) { > regulator_disable(ctx->vcc); > return; > } > } > > static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, > struct drm_bridge_state *old_bridge_state) > { > regulator_disable(ctx->vcc); > } > > So when the PLL fails locking, the vcc regulator is disable twice, > leading to "unbalanced disables". > > I initially removed the regulator_disable() line in sn65dsi83_atomic_pre_enable() > locally and it worked fine. Then I did some git log and found you added this line on > purpose (even though it was in sn65dsi83_atomic_enable() initially), so my question > is whether you can explain exactly what was wrong before your patch. I have been > working for a few weeks with the regulator_disable() line removed and found no issue. Unfortunately I' cant tell the details anymore, but I do remember hitting some bug regarding failed PLL lock. I do remember having a lock failure from time to time as well. I wont be able to test this bridge at the moment, but you seem to be right. On a general side, IMHO enabling the PLL in atomic_pre_enable is a bit late anyway, because you can't bail out if enabling fails. Best regards, Alexander > Best regards, > Luca > >
Hi Alexander, thanks for your feedback! On Tue, 27 Feb 2024 13:05:46 +0100 Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > Hi Luca, > > Am Donnerstag, 22. Februar 2024, 16:36:37 CET schrieb Luca Ceresoli: > > Hello Alexander, > > > > On Thu, 4 May 2023 08:53:16 +0200 > > Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > > > > If PLL locking failed, the regulator needs to be disabled again. > > > > > > Fixes: 5664e3c907e2 ("drm/bridge: ti-sn65dsi83: Add vcc supply regulator support") > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > > --- > > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > index 75286c9afbb9..1f5c07989e2b 100644 > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > @@ -478,6 +478,7 @@ static void sn65dsi83_atomic_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; > > > } > > > > I'm reviving this thread as I've been investigating a bug that appears > > related to this patch. > > > > Symptom: with a v6.8-rc5 kernel, if PLL fails locking, later on during > > atomic disable I get: > > > > [ 41.065198] ------------[ cut here ]------------ > > [ 41.069823] unbalanced disables for DOCK_SYS_1V8 > > [ 41.074482] WARNING: CPU: 0 PID: 58 at drivers/regulator/core.c:2999 _regulator_disable+0xf8/0x1d8 > > [ 41.083457] Modules linked in: smsc smsc95xx usbnet mii imx_cpufreq_dt exc3000 imx8mm_thermal snd_soc_tlv320aic3x_spi snd_soc_tlv320aic3x_i2c snd_soc_tlv320aic3x tmp103 snd_soc_simple_card snd_soc_simple_card_utils fsl_ldb rtc_snvs snvs_pwrkey snd_soc_fsl_sai imx8mp_interconnect snd_soc_fsl_utils imx_interconnect imx_pcm_dma rtc_rs5c372 ti_sn65dsi83 pwm_imx27 st_pressure_spi st_sensors_spi st_pressure_i2c st_pressure st_sensors_i2c industrialio_triggered_buffer lm75 kfifo_buf st_sensors opt3001 panel_simple etnaviv gpu_sched iio_hwmon governor_userspace imx_bus imx8mp_hdmi_tx dw_hdmi drm_display_helper samsung_dsim imx_sdma imx_lcdif drm_dma_helper imx8mp_hdmi_pvi drm_kms_helper drm drm_panel_orientation_quirks fsl_imx8_ddr_perf caam error sbs_battery pwm_bl backlight ltc2497 ltc2497_core crct10dif_ce > > [ 41.157281] CPU: 0 PID: 58 Comm: kworker/0:2 Not tainted 6.8.0-rc5+ #7 > > [ 41.170339] Workqueue: events drm_mode_rmfb_work_fn [drm] > > [ 41.175798] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > > [ 41.182762] pc : _regulator_disable+0xf8/0x1d8 > > [ 41.187209] lr : _regulator_disable+0xf8/0x1d8 > > [ 41.191654] sp : ffff800081aaba90 > > [ 41.194967] x29: ffff800081aaba90 x28: 0000000000000000 x27: ffff000002647e80 > > [ 41.202109] x26: ffff000002d7a180 x25: ffff0000037858a0 x24: ffff800079748ac8 > > [ 41.209250] x23: ffff000002647ed8 x22: ffff00000263f800 x21: ffff00000373d000 > > [ 41.216392] x20: ffff00000373d000 x19: ffff000001de6480 x18: 0000000000000006 > > [ 41.223533] x17: 0000000000000000 x16: 1fffe000003423e1 x15: ffff800081aab520 > > [ 41.230674] x14: 0000000000000000 x13: 3856315f5359535f x12: 4b434f4420726f66 > > [ 41.237815] x11: 2073656c62617369 x10: ffff8000814647a0 x9 : ffff8000801b10e0 > > [ 41.244957] x8 : ffff8000814bc7a0 x7 : 0000000000017fe8 x6 : ffff8000814bc7a0 > > [ 41.252098] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000 > > [ 41.259239] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000011b6600 > > [ 41.266380] Call trace: > > [ 41.268826] _regulator_disable+0xf8/0x1d8 > > [ 41.272925] regulator_disable+0x4c/0x98 > > [ 41.276850] sn65dsi83_atomic_disable+0x70/0xc0 [ti_sn65dsi83] > > [ 41.282692] drm_atomic_bridge_chain_disable+0x78/0x110 [drm] > > [ 41.288481] disable_outputs+0x100/0x350 [drm_kms_helper] > > [ 41.293902] drm_atomic_helper_commit_tail_rpm+0x2c/0xb0 [drm_kms_helper] > > [ 41.300705] commit_tail+0xac/0x1a0 [drm_kms_helper] > > [ 41.305685] drm_atomic_helper_commit+0x16c/0x188 [drm_kms_helper] > > [ 41.311881] drm_atomic_commit+0xac/0xf0 [drm] > > [ 41.316365] drm_framebuffer_remove+0x464/0x550 [drm] > > [ 41.321458] drm_mode_rmfb_work_fn+0x84/0xb0 [drm] > > [ 41.326291] process_one_work+0x148/0x3b8 > > [ 41.330309] worker_thread+0x32c/0x450 > > [ 41.334061] kthread+0x11c/0x128 > > [ 41.337292] ret_from_fork+0x10/0x20 > > [ 41.340873] ---[ end trace 0000000000000000 ]--- > > > > The reason is clear from the code flow, which looks like this (after > > removing unrelated code): > > > > static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, > > struct drm_bridge_state *old_bridge_state) > > { > > regulator_enable(ctx->vcc); > > > > if (PLL failed locking) { > > regulator_disable(ctx->vcc); > > return; > > } > > } > > > > static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, > > struct drm_bridge_state *old_bridge_state) > > { > > regulator_disable(ctx->vcc); > > } > > > > So when the PLL fails locking, the vcc regulator is disable twice, > > leading to "unbalanced disables". > > > > I initially removed the regulator_disable() line in sn65dsi83_atomic_pre_enable() > > locally and it worked fine. Then I did some git log and found you added this line on > > purpose (even though it was in sn65dsi83_atomic_enable() initially), so my question > > is whether you can explain exactly what was wrong before your patch. I have been > > working for a few weeks with the regulator_disable() line removed and found no issue. > > Unfortunately I' cant tell the details anymore, but I do remember hitting > some bug regarding failed PLL lock. I do remember having a lock failure > from time to time as well. Too bad, and unfortunately the commit message is not providing an example. However... > I wont be able to test this bridge at the moment, but you seem to be right. ...if you could test it soonish and report back that would be great. Otherwise to move forward from the current situation I see two options: * remove the regulator_disable() in the PLL failure case, de facto reverting commit 8a91b29f1f50 ("drm/bridge: ti-sn65dsi83: Fix enable error path"), and see if any problem happens again * add a flag to take not of whether we enabled the regulator or not, and in sn65dsi83_atomic_disable() call regulator_disable() conditionally based on that The first approach is simpler. It also means that in the window between atomic_pre_enable and atomic_disable the regulator would be enabled without need. I don't think this is a relevant problem as the video output is not working without a PLL, so people will fix that soon I guess. The second approach means introducing a little more complexity and we are not sure whether it is needed or not. So I have some preference for the first proposal unless there is a valid example where the added regulator_disable() is surely needed. This is what is running here singe several weeks, and it didn't show other issues. > On a general side, IMHO enabling the PLL in atomic_pre_enable is a bit late > anyway, because you can't bail out if enabling fails. True. However I don't see what we can do about that without changes to the DRM core, which would not be quick to do. So in the short term we need a fix in this driver. Best regards, Luca
Hi Luca, Am Dienstag, 27. Februar 2024, 18:41:44 CET schrieb Luca Ceresoli: > Hi Alexander, > > thanks for your feedback! > > On Tue, 27 Feb 2024 13:05:46 +0100 > Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > > Hi Luca, > > > > Am Donnerstag, 22. Februar 2024, 16:36:37 CET schrieb Luca Ceresoli: > > > Hello Alexander, > > > > > > On Thu, 4 May 2023 08:53:16 +0200 > > > Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > > > > > > If PLL locking failed, the regulator needs to be disabled again. > > > > > > > > Fixes: 5664e3c907e2 ("drm/bridge: ti-sn65dsi83: Add vcc supply regulator support") > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > > > --- > > > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > index 75286c9afbb9..1f5c07989e2b 100644 > > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > @@ -478,6 +478,7 @@ static void sn65dsi83_atomic_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; > > > > } > > > > > > I'm reviving this thread as I've been investigating a bug that appears > > > related to this patch. > > > > > > Symptom: with a v6.8-rc5 kernel, if PLL fails locking, later on during > > > atomic disable I get: > > > > > > [ 41.065198] ------------[ cut here ]------------ > > > [ 41.069823] unbalanced disables for DOCK_SYS_1V8 > > > [ 41.074482] WARNING: CPU: 0 PID: 58 at drivers/regulator/core.c:2999 _regulator_disable+0xf8/0x1d8 > > > [ 41.083457] Modules linked in: smsc smsc95xx usbnet mii imx_cpufreq_dt exc3000 imx8mm_thermal snd_soc_tlv320aic3x_spi snd_soc_tlv320aic3x_i2c snd_soc_tlv320aic3x tmp103 snd_soc_simple_card snd_soc_simple_card_utils fsl_ldb rtc_snvs snvs_pwrkey snd_soc_fsl_sai imx8mp_interconnect snd_soc_fsl_utils imx_interconnect imx_pcm_dma rtc_rs5c372 ti_sn65dsi83 pwm_imx27 st_pressure_spi st_sensors_spi st_pressure_i2c st_pressure st_sensors_i2c industrialio_triggered_buffer lm75 kfifo_buf st_sensors opt3001 panel_simple etnaviv gpu_sched iio_hwmon governor_userspace imx_bus imx8mp_hdmi_tx dw_hdmi drm_display_helper samsung_dsim imx_sdma imx_lcdif drm_dma_helper imx8mp_hdmi_pvi drm_kms_helper drm drm_panel_orientation_quirks fsl_imx8_ddr_perf caam error sbs_battery pwm_bl backlight ltc2497 ltc2497_core crct10dif_ce > > > [ 41.157281] CPU: 0 PID: 58 Comm: kworker/0:2 Not tainted 6.8.0-rc5+ #7 > > > [ 41.170339] Workqueue: events drm_mode_rmfb_work_fn [drm] > > > [ 41.175798] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > > > [ 41.182762] pc : _regulator_disable+0xf8/0x1d8 > > > [ 41.187209] lr : _regulator_disable+0xf8/0x1d8 > > > [ 41.191654] sp : ffff800081aaba90 > > > [ 41.194967] x29: ffff800081aaba90 x28: 0000000000000000 x27: ffff000002647e80 > > > [ 41.202109] x26: ffff000002d7a180 x25: ffff0000037858a0 x24: ffff800079748ac8 > > > [ 41.209250] x23: ffff000002647ed8 x22: ffff00000263f800 x21: ffff00000373d000 > > > [ 41.216392] x20: ffff00000373d000 x19: ffff000001de6480 x18: 0000000000000006 > > > [ 41.223533] x17: 0000000000000000 x16: 1fffe000003423e1 x15: ffff800081aab520 > > > [ 41.230674] x14: 0000000000000000 x13: 3856315f5359535f x12: 4b434f4420726f66 > > > [ 41.237815] x11: 2073656c62617369 x10: ffff8000814647a0 x9 : ffff8000801b10e0 > > > [ 41.244957] x8 : ffff8000814bc7a0 x7 : 0000000000017fe8 x6 : ffff8000814bc7a0 > > > [ 41.252098] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000 > > > [ 41.259239] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000011b6600 > > > [ 41.266380] Call trace: > > > [ 41.268826] _regulator_disable+0xf8/0x1d8 > > > [ 41.272925] regulator_disable+0x4c/0x98 > > > [ 41.276850] sn65dsi83_atomic_disable+0x70/0xc0 [ti_sn65dsi83] > > > [ 41.282692] drm_atomic_bridge_chain_disable+0x78/0x110 [drm] > > > [ 41.288481] disable_outputs+0x100/0x350 [drm_kms_helper] > > > [ 41.293902] drm_atomic_helper_commit_tail_rpm+0x2c/0xb0 [drm_kms_helper] > > > [ 41.300705] commit_tail+0xac/0x1a0 [drm_kms_helper] > > > [ 41.305685] drm_atomic_helper_commit+0x16c/0x188 [drm_kms_helper] > > > [ 41.311881] drm_atomic_commit+0xac/0xf0 [drm] > > > [ 41.316365] drm_framebuffer_remove+0x464/0x550 [drm] > > > [ 41.321458] drm_mode_rmfb_work_fn+0x84/0xb0 [drm] > > > [ 41.326291] process_one_work+0x148/0x3b8 > > > [ 41.330309] worker_thread+0x32c/0x450 > > > [ 41.334061] kthread+0x11c/0x128 > > > [ 41.337292] ret_from_fork+0x10/0x20 > > > [ 41.340873] ---[ end trace 0000000000000000 ]--- > > > > > > The reason is clear from the code flow, which looks like this (after > > > removing unrelated code): > > > > > > static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, > > > struct drm_bridge_state *old_bridge_state) > > > { > > > regulator_enable(ctx->vcc); > > > > > > if (PLL failed locking) { > > > regulator_disable(ctx->vcc); > > > return; > > > } > > > } > > > > > > static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, > > > struct drm_bridge_state *old_bridge_state) > > > { > > > regulator_disable(ctx->vcc); > > > } > > > > > > So when the PLL fails locking, the vcc regulator is disable twice, > > > leading to "unbalanced disables". > > > > > > I initially removed the regulator_disable() line in sn65dsi83_atomic_pre_enable() > > > locally and it worked fine. Then I did some git log and found you added this line on > > > purpose (even though it was in sn65dsi83_atomic_enable() initially), so my question > > > is whether you can explain exactly what was wrong before your patch. I have been > > > working for a few weeks with the regulator_disable() line removed and found no issue. > > > > Unfortunately I' cant tell the details anymore, but I do remember hitting > > some bug regarding failed PLL lock. I do remember having a lock failure > > from time to time as well. > > Too bad, and unfortunately the commit message is not providing an > example. However... > > > I wont be able to test this bridge at the moment, but you seem to be right. > > ...if you could test it soonish and report back that would be great. > Otherwise to move forward from the current situation I see two options: > > * remove the regulator_disable() in the PLL failure case, de facto > reverting commit 8a91b29f1f50 ("drm/bridge: ti-sn65dsi83: Fix enable > error path"), and see if any problem happens again > * add a flag to take not of whether we enabled the regulator or not, > and in sn65dsi83_atomic_disable() call regulator_disable() > conditionally based on that > > The first approach is simpler. It also means that in the window between > atomic_pre_enable and atomic_disable the regulator would be enabled > without need. I don't think this is a relevant problem as the video > output is not working without a PLL, so people will fix that soon I > guess. > > The second approach means introducing a little more complexity and we > are not sure whether it is needed or not. > > So I have some preference for the first proposal unless there is a > valid example where the added regulator_disable() is surely needed. > This is what is running here singe several weeks, and it didn't show > other issues. Feel free to go the first version, my platform still has DSI problems so there is no board which might break right now. > > On a general side, IMHO enabling the PLL in atomic_pre_enable is a bit late > > anyway, because you can't bail out if enabling fails. > > True. However I don't see what we can do about that without changes to > the DRM core, which would not be quick to do. So in the short term we > need a fix in this driver. Indeed, I was just mentioning, no need to address this right now. Best regards, Alexander
Hi Luca, Am Dienstag, 27. Februar 2024, 18:41:44 CET schrieb Luca Ceresoli: > Hi Alexander, > > thanks for your feedback! > > On Tue, 27 Feb 2024 13:05:46 +0100 > Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > > Hi Luca, > > > > Am Donnerstag, 22. Februar 2024, 16:36:37 CET schrieb Luca Ceresoli: > > > Hello Alexander, > > > > > > On Thu, 4 May 2023 08:53:16 +0200 > > > Alexander Stein <alexander.stein@ew.tq-group.com> wrote: > > > > > > > If PLL locking failed, the regulator needs to be disabled again. > > > > > > > > Fixes: 5664e3c907e2 ("drm/bridge: ti-sn65dsi83: Add vcc supply regulator support") > > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> > > > > --- > > > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 1 + > > > > 1 file changed, 1 insertion(+) > > > > > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > index 75286c9afbb9..1f5c07989e2b 100644 > > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > > > @@ -478,6 +478,7 @@ static void sn65dsi83_atomic_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; > > > > } > > > > > > I'm reviving this thread as I've been investigating a bug that appears > > > related to this patch. > > > > > > Symptom: with a v6.8-rc5 kernel, if PLL fails locking, later on during > > > atomic disable I get: > > > > > > [ 41.065198] ------------[ cut here ]------------ > > > [ 41.069823] unbalanced disables for DOCK_SYS_1V8 > > > [ 41.074482] WARNING: CPU: 0 PID: 58 at drivers/regulator/core.c:2999 _regulator_disable+0xf8/0x1d8 > > > [ 41.083457] Modules linked in: smsc smsc95xx usbnet mii imx_cpufreq_dt exc3000 imx8mm_thermal snd_soc_tlv320aic3x_spi snd_soc_tlv320aic3x_i2c snd_soc_tlv320aic3x tmp103 snd_soc_simple_card snd_soc_simple_card_utils fsl_ldb rtc_snvs snvs_pwrkey snd_soc_fsl_sai imx8mp_interconnect snd_soc_fsl_utils imx_interconnect imx_pcm_dma rtc_rs5c372 ti_sn65dsi83 pwm_imx27 st_pressure_spi st_sensors_spi st_pressure_i2c st_pressure st_sensors_i2c industrialio_triggered_buffer lm75 kfifo_buf st_sensors opt3001 panel_simple etnaviv gpu_sched iio_hwmon governor_userspace imx_bus imx8mp_hdmi_tx dw_hdmi drm_display_helper samsung_dsim imx_sdma imx_lcdif drm_dma_helper imx8mp_hdmi_pvi drm_kms_helper drm drm_panel_orientation_quirks fsl_imx8_ddr_perf caam error sbs_battery pwm_bl backlight ltc2497 ltc2497_core crct10dif_ce > > > [ 41.157281] CPU: 0 PID: 58 Comm: kworker/0:2 Not tainted 6.8.0-rc5+ #7 > > > [ 41.170339] Workqueue: events drm_mode_rmfb_work_fn [drm] > > > [ 41.175798] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > > > [ 41.182762] pc : _regulator_disable+0xf8/0x1d8 > > > [ 41.187209] lr : _regulator_disable+0xf8/0x1d8 > > > [ 41.191654] sp : ffff800081aaba90 > > > [ 41.194967] x29: ffff800081aaba90 x28: 0000000000000000 x27: ffff000002647e80 > > > [ 41.202109] x26: ffff000002d7a180 x25: ffff0000037858a0 x24: ffff800079748ac8 > > > [ 41.209250] x23: ffff000002647ed8 x22: ffff00000263f800 x21: ffff00000373d000 > > > [ 41.216392] x20: ffff00000373d000 x19: ffff000001de6480 x18: 0000000000000006 > > > [ 41.223533] x17: 0000000000000000 x16: 1fffe000003423e1 x15: ffff800081aab520 > > > [ 41.230674] x14: 0000000000000000 x13: 3856315f5359535f x12: 4b434f4420726f66 > > > [ 41.237815] x11: 2073656c62617369 x10: ffff8000814647a0 x9 : ffff8000801b10e0 > > > [ 41.244957] x8 : ffff8000814bc7a0 x7 : 0000000000017fe8 x6 : ffff8000814bc7a0 > > > [ 41.252098] x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000 > > > [ 41.259239] x2 : 0000000000000000 x1 : 0000000000000000 x0 : ffff0000011b6600 > > > [ 41.266380] Call trace: > > > [ 41.268826] _regulator_disable+0xf8/0x1d8 > > > [ 41.272925] regulator_disable+0x4c/0x98 > > > [ 41.276850] sn65dsi83_atomic_disable+0x70/0xc0 [ti_sn65dsi83] > > > [ 41.282692] drm_atomic_bridge_chain_disable+0x78/0x110 [drm] > > > [ 41.288481] disable_outputs+0x100/0x350 [drm_kms_helper] > > > [ 41.293902] drm_atomic_helper_commit_tail_rpm+0x2c/0xb0 [drm_kms_helper] > > > [ 41.300705] commit_tail+0xac/0x1a0 [drm_kms_helper] > > > [ 41.305685] drm_atomic_helper_commit+0x16c/0x188 [drm_kms_helper] > > > [ 41.311881] drm_atomic_commit+0xac/0xf0 [drm] > > > [ 41.316365] drm_framebuffer_remove+0x464/0x550 [drm] > > > [ 41.321458] drm_mode_rmfb_work_fn+0x84/0xb0 [drm] > > > [ 41.326291] process_one_work+0x148/0x3b8 > > > [ 41.330309] worker_thread+0x32c/0x450 > > > [ 41.334061] kthread+0x11c/0x128 > > > [ 41.337292] ret_from_fork+0x10/0x20 > > > [ 41.340873] ---[ end trace 0000000000000000 ]--- > > > > > > The reason is clear from the code flow, which looks like this (after > > > removing unrelated code): > > > > > > static void sn65dsi83_atomic_pre_enable(struct drm_bridge *bridge, > > > struct drm_bridge_state *old_bridge_state) > > > { > > > regulator_enable(ctx->vcc); > > > > > > if (PLL failed locking) { > > > regulator_disable(ctx->vcc); > > > return; > > > } > > > } > > > > > > static void sn65dsi83_atomic_disable(struct drm_bridge *bridge, > > > struct drm_bridge_state *old_bridge_state) > > > { > > > regulator_disable(ctx->vcc); > > > } > > > > > > So when the PLL fails locking, the vcc regulator is disable twice, > > > leading to "unbalanced disables". > > > > > > I initially removed the regulator_disable() line in sn65dsi83_atomic_pre_enable() > > > locally and it worked fine. Then I did some git log and found you added this line on > > > purpose (even though it was in sn65dsi83_atomic_enable() initially), so my question > > > is whether you can explain exactly what was wrong before your patch. I have been > > > working for a few weeks with the regulator_disable() line removed and found no issue. > > > > Unfortunately I' cant tell the details anymore, but I do remember hitting > > some bug regarding failed PLL lock. I do remember having a lock failure > > from time to time as well. > > Too bad, and unfortunately the commit message is not providing an > example. However... 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 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. Best regards, Alexander > > I wont be able to test this bridge at the moment, but you seem to be right. > > ...if you could test it soonish and report back that would be great. > Otherwise to move forward from the current situation I see two options: > > * remove the regulator_disable() in the PLL failure case, de facto > reverting commit 8a91b29f1f50 ("drm/bridge: ti-sn65dsi83: Fix enable > error path"), and see if any problem happens again > * add a flag to take not of whether we enabled the regulator or not, > and in sn65dsi83_atomic_disable() call regulator_disable() > conditionally based on that > > The first approach is simpler. It also means that in the window between > atomic_pre_enable and atomic_disable the regulator would be enabled > without need. I don't think this is a relevant problem as the video > output is not working without a PLL, so people will fix that soon I > guess. > > The second approach means introducing a little more complexity and we > are not sure whether it is needed or not. > > So I have some preference for the first proposal unless there is a > valid example where the added regulator_disable() is surely needed. > This is what is running here singe several weeks, and it didn't show > other issues. > > > On a general side, IMHO enabling the PLL in atomic_pre_enable is a bit late > > anyway, because you can't bail out if enabling fails. > > True. However I don't see what we can do about that without changes to > the DRM core, which would not be quick to do. So in the short term we > need a fix in this driver. > > Best regards, > Luca > >
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 = <®_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. Luca
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 = <®_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. [1] https://patchwork.freedesktop.org/patch/529288/ [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=4fb912e5e19075874379cfcf074d90bd51ebf8ea
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 = <®_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
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 = <®_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
Hello Alexander, On Thu, 29 Feb 2024 12:11:23 +0100 Alexander Stein <alexander.stein@ew.tq-group.com> 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@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 = <®_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. What is the use case you have for removing the driver module? I'm not implying removing the modules is wrong, but it definitely looks like not supported / not working. I'm just trying to understand the big picture. Best regards, Luca
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@ew.tq-group.com> 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@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 = <®_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. > I'm not implying removing the modules is wrong, but it definitely looks > like not supported / not working. I'm just trying to understand the big > picture. Unloading should be possible, but not if the bridge is currently enabled. Thanks for looking into this. Best regards, Alexander
Hello Alexander, On Fri, 01 Mar 2024 10:57:37 +0100 Alexander Stein <alexander.stein@ew.tq-group.com> 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@ew.tq-group.com> 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@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 = <®_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 ? Luca
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@ew.tq-group.com> 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@ew.tq-group.com> 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@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 = <®_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. Best regards, Alexander
Hello Alexander, On Fri, 01 Mar 2024 11:45:27 +0100 Alexander Stein <alexander.stein@ew.tq-group.com> 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@ew.tq-group.com> 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@ew.tq-group.com> 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@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 = <®_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@bootlin.com/ Thanks for the discussion! Luca
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 75286c9afbb9..1f5c07989e2b 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -478,6 +478,7 @@ static void sn65dsi83_atomic_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; }
If PLL locking failed, the regulator needs to be disabled again. Fixes: 5664e3c907e2 ("drm/bridge: ti-sn65dsi83: Add vcc supply regulator support") Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com> --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 1 + 1 file changed, 1 insertion(+)