Doing modeset in .atomic_pre_enable callback instead of dedicated .mode_set
callback does not seem right. Undo this change, which was added as part of
commit 05aa61334592 ("drm: bridge: dw-mipi-dsi: Fix enable/disable of DSI
controller") as it breaks STM32MP15xx LTDC scanout (DSI)->TC358762 DSI-to-DPI
bridge->PT800480 DPI panel pipeline. The original fix for HX8394 panel likely
requires HX8394 panel side fix instead.
Fixes: 05aa61334592 ("drm: bridge: dw-mipi-dsi: Fix enable/disable of DSI controller")
Signed-off-by: Marek Vasut <[email protected]>
---
Cc: Andrzej Hajda <[email protected]>
Cc: Biju Das <[email protected]>
Cc: Daniel Vetter <[email protected]>
Cc: David Airlie <[email protected]>
Cc: Douglas Anderson <[email protected]>
Cc: Jernej Skrabec <[email protected]>
Cc: Jonas Karlman <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: Liu Ying <[email protected]>
Cc: Maarten Lankhorst <[email protected]>
Cc: Maxime Ripard <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Ondrej Jirman <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Robert Foss <[email protected]>
Cc: Sam Ravnborg <[email protected]>
Cc: Thomas Zimmermann <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 18 +++---------------
1 file changed, 3 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 824fb3c65742e..ca5894393dba4 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -268,7 +268,6 @@ struct dw_mipi_dsi {
struct dw_mipi_dsi *master; /* dual-dsi master ptr */
struct dw_mipi_dsi *slave; /* dual-dsi slave ptr */
- struct drm_display_mode mode;
const struct dw_mipi_dsi_plat_data *plat_data;
};
@@ -1016,25 +1015,15 @@ static void dw_mipi_dsi_mode_set(struct dw_mipi_dsi *dsi,
phy_ops->power_on(dsi->plat_data->priv_data);
}
-static void dw_mipi_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
- struct drm_bridge_state *old_bridge_state)
-{
- struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
-
- /* Power up the dsi ctl into a command mode */
- dw_mipi_dsi_mode_set(dsi, &dsi->mode);
- if (dsi->slave)
- dw_mipi_dsi_mode_set(dsi->slave, &dsi->mode);
-}
-
static void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge,
const struct drm_display_mode *mode,
const struct drm_display_mode *adjusted_mode)
{
struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
- /* Store the display mode for later use in pre_enable callback */
- drm_mode_copy(&dsi->mode, adjusted_mode);
+ dw_mipi_dsi_mode_set(dsi, adjusted_mode);
+ if (dsi->slave)
+ dw_mipi_dsi_mode_set(dsi->slave, adjusted_mode);
}
static void dw_mipi_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
@@ -1090,7 +1079,6 @@ static const struct drm_bridge_funcs dw_mipi_dsi_bridge_funcs = {
.atomic_get_input_bus_fmts = dw_mipi_dsi_bridge_atomic_get_input_bus_fmts,
.atomic_check = dw_mipi_dsi_bridge_atomic_check,
.atomic_reset = drm_atomic_helper_bridge_reset,
- .atomic_pre_enable = dw_mipi_dsi_bridge_atomic_pre_enable,
.atomic_enable = dw_mipi_dsi_bridge_atomic_enable,
.atomic_post_disable = dw_mipi_dsi_bridge_post_atomic_disable,
.mode_set = dw_mipi_dsi_bridge_mode_set,
--
2.43.0
Hi,
On Sun, Apr 21, 2024 at 02:22:35AM GMT, Marek Vasut wrote:
> Doing modeset in .atomic_pre_enable callback instead of dedicated .mode_set
> callback does not seem right. Undo this change, which was added as part of
Actually no. If anything, mode_set callback should be dropped entirely:
See https://elixir.bootlin.com/linux/latest/source/include/drm/drm_bridge.h#L231
It's deprecated, and enable callback should just use adjusted_mode:
This is deprecated, do not use! New drivers shall set their mode in the
&drm_bridge_funcs.atomic_enable operation.
> commit 05aa61334592 ("drm: bridge: dw-mipi-dsi: Fix enable/disable of DSI
> controller") as it breaks STM32MP15xx LTDC scanout (DSI)->TC358762 DSI-to-DPI
> bridge->PT800480 DPI panel pipeline. The original fix for HX8394 panel likely
> requires HX8394 panel side fix instead.
There's nothing wrong with the panel driver. And that commit is not fixing issue
with the panel driver, just like the subject hints at. Look at the referenced
commit, at "Before:" sequence specifically.
dw_mipi_dsi_mode_set may be named *_mode_set or whatever, but it's basically
an enable function that turns on clocks, initalizes phy, etc. etc.
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L998
And if you check "Before:" sequence, you'll see that .mode_set callback is just
called once at boot and never again. And it's atomic(_pre)_enable/atomic(_post)_disable
callbacks that actually are called in ballanced way to enable/disable the
controller repeatedly ever after.
Function dw_mipi_dsi_bridge_post_atomic_disable is the inverse of
dw_mipi_dsi_mode_set, it undoes everything that dw_mipi_dsi_mode_set does.
You need to find root cause for your issue on STM32MP15xx instead of reverting
fixes for resource use bugs in this driver.
kind regards,
o.
> Fixes: 05aa61334592 ("drm: bridge: dw-mipi-dsi: Fix enable/disable of DSI controller")
> Signed-off-by: Marek Vasut <[email protected]>
> ---
> Cc: Andrzej Hajda <[email protected]>
> Cc: Biju Das <[email protected]>
> Cc: Daniel Vetter <[email protected]>
> Cc: David Airlie <[email protected]>
> Cc: Douglas Anderson <[email protected]>
> Cc: Jernej Skrabec <[email protected]>
> Cc: Jonas Karlman <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> Cc: Liu Ying <[email protected]>
> Cc: Maarten Lankhorst <[email protected]>
> Cc: Maxime Ripard <[email protected]>
> Cc: Neil Armstrong <[email protected]>
> Cc: Ondrej Jirman <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Robert Foss <[email protected]>
> Cc: Sam Ravnborg <[email protected]>
> Cc: Thomas Zimmermann <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> ---
> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 18 +++---------------
> 1 file changed, 3 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 824fb3c65742e..ca5894393dba4 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -268,7 +268,6 @@ struct dw_mipi_dsi {
> struct dw_mipi_dsi *master; /* dual-dsi master ptr */
> struct dw_mipi_dsi *slave; /* dual-dsi slave ptr */
>
> - struct drm_display_mode mode;
> const struct dw_mipi_dsi_plat_data *plat_data;
> };
>
> @@ -1016,25 +1015,15 @@ static void dw_mipi_dsi_mode_set(struct dw_mipi_dsi *dsi,
> phy_ops->power_on(dsi->plat_data->priv_data);
> }
>
> -static void dw_mipi_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> - struct drm_bridge_state *old_bridge_state)
> -{
> - struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
> -
> - /* Power up the dsi ctl into a command mode */
> - dw_mipi_dsi_mode_set(dsi, &dsi->mode);
> - if (dsi->slave)
> - dw_mipi_dsi_mode_set(dsi->slave, &dsi->mode);
> -}
> -
> static void dw_mipi_dsi_bridge_mode_set(struct drm_bridge *bridge,
> const struct drm_display_mode *mode,
> const struct drm_display_mode *adjusted_mode)
> {
> struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
>
> - /* Store the display mode for later use in pre_enable callback */
> - drm_mode_copy(&dsi->mode, adjusted_mode);
> + dw_mipi_dsi_mode_set(dsi, adjusted_mode);
> + if (dsi->slave)
> + dw_mipi_dsi_mode_set(dsi->slave, adjusted_mode);
> }
>
> static void dw_mipi_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
> @@ -1090,7 +1079,6 @@ static const struct drm_bridge_funcs dw_mipi_dsi_bridge_funcs = {
> .atomic_get_input_bus_fmts = dw_mipi_dsi_bridge_atomic_get_input_bus_fmts,
> .atomic_check = dw_mipi_dsi_bridge_atomic_check,
> .atomic_reset = drm_atomic_helper_bridge_reset,
> - .atomic_pre_enable = dw_mipi_dsi_bridge_atomic_pre_enable,
> .atomic_enable = dw_mipi_dsi_bridge_atomic_enable,
> .atomic_post_disable = dw_mipi_dsi_bridge_post_atomic_disable,
> .mode_set = dw_mipi_dsi_bridge_mode_set,
> --
> 2.43.0
>
On 4/21/24 1:09 PM, Ondřej Jirman wrote:
> Hi,
Hi,
> On Sun, Apr 21, 2024 at 02:22:35AM GMT, Marek Vasut wrote:
>> Doing modeset in .atomic_pre_enable callback instead of dedicated .mode_set
>> callback does not seem right. Undo this change, which was added as part of
>
> Actually no. If anything, mode_set callback should be dropped entirely:
>
> See https://elixir.bootlin.com/linux/latest/source/include/drm/drm_bridge.h#L231
>
> It's deprecated, and enable callback should just use adjusted_mode:
>
> This is deprecated, do not use! New drivers shall set their mode in the
> &drm_bridge_funcs.atomic_enable operation.
This mentions new drivers ?
>> commit 05aa61334592 ("drm: bridge: dw-mipi-dsi: Fix enable/disable of DSI
>> controller") as it breaks STM32MP15xx LTDC scanout (DSI)->TC358762 DSI-to-DPI
>> bridge->PT800480 DPI panel pipeline. The original fix for HX8394 panel likely
>> requires HX8394 panel side fix instead.
>
> There's nothing wrong with the panel driver. And that commit is not fixing issue
> with the panel driver, just like the subject hints at. Look at the referenced
> commit, at "Before:" sequence specifically.
>
> dw_mipi_dsi_mode_set may be named *_mode_set or whatever, but it's basically
> an enable function that turns on clocks, initalizes phy, etc. etc.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L998
>
> And if you check "Before:" sequence, you'll see that .mode_set callback is just
> called once at boot and never again. And it's atomic(_pre)_enable/atomic(_post)_disable
> callbacks that actually are called in ballanced way to enable/disable the
> controller repeatedly ever after.
>
> Function dw_mipi_dsi_bridge_post_atomic_disable is the inverse of
> dw_mipi_dsi_mode_set, it undoes everything that dw_mipi_dsi_mode_set does.
>
> You need to find root cause for your issue on STM32MP15xx instead of reverting
> fixes for resource use bugs in this driver.
Actually, reverting commit 05aa61334592 ("drm: bridge: dw-mipi-dsi: Fix
enable/disable of DSI controller") makes the STM32MP15xx work again like
it used to since Linux 5.10 or so, so that commit breaks existing
working use case.
It seems it is sufficient to revert only this part of the commit to make
the STM32MP15xx work as it used to, do you have any idea why ?
On Sun, Apr 21, 2024 at 04:25:34PM GMT, Marek Vasut wrote:
> On 4/21/24 1:09 PM, Ondřej Jirman wrote:
> > Hi,
>
> Hi,
>
> > On Sun, Apr 21, 2024 at 02:22:35AM GMT, Marek Vasut wrote:
> > > Doing modeset in .atomic_pre_enable callback instead of dedicated .mode_set
> > > callback does not seem right. Undo this change, which was added as part of
> >
> > Actually no. If anything, mode_set callback should be dropped entirely:
> >
> > See https://elixir.bootlin.com/linux/latest/source/include/drm/drm_bridge.h#L231
> >
> > It's deprecated, and enable callback should just use adjusted_mode:
> >
> > This is deprecated, do not use! New drivers shall set their mode in the
> > &drm_bridge_funcs.atomic_enable operation.
>
> This mentions new drivers ?
Yes.
> > > commit 05aa61334592 ("drm: bridge: dw-mipi-dsi: Fix enable/disable of DSI
> > > controller") as it breaks STM32MP15xx LTDC scanout (DSI)->TC358762 DSI-to-DPI
> > > bridge->PT800480 DPI panel pipeline. The original fix for HX8394 panel likely
> > > requires HX8394 panel side fix instead.
> >
> > There's nothing wrong with the panel driver. And that commit is not fixing issue
> > with the panel driver, just like the subject hints at. Look at the referenced
> > commit, at "Before:" sequence specifically.
> >
> > dw_mipi_dsi_mode_set may be named *_mode_set or whatever, but it's basically
> > an enable function that turns on clocks, initalizes phy, etc. etc.
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L998
> >
> > And if you check "Before:" sequence, you'll see that .mode_set callback is just
> > called once at boot and never again. And it's atomic(_pre)_enable/atomic(_post)_disable
> > callbacks that actually are called in ballanced way to enable/disable the
> > controller repeatedly ever after.
> >
> > Function dw_mipi_dsi_bridge_post_atomic_disable is the inverse of
> > dw_mipi_dsi_mode_set, it undoes everything that dw_mipi_dsi_mode_set does.
> >
> > You need to find root cause for your issue on STM32MP15xx instead of reverting
> > fixes for resource use bugs in this driver.
>
> Actually, reverting commit 05aa61334592 ("drm: bridge: dw-mipi-dsi: Fix
> enable/disable of DSI controller") makes the STM32MP15xx work again like it
> used to since Linux 5.10 or so, so that commit breaks existing working use
> case.
It may simply be revealing bug elsewhere in the STM display stack.
> It seems it is sufficient to revert only this part of the commit to make the
> STM32MP15xx work as it used to, do you have any idea why ?
No, I don't have any STM based board. This revert reintroduces the bug that
causes this driver resource use failures after a simple blanking/unblnaking
cycle done via sysfs when using Linux VT.
It's quite likely reproducible on your board, too. It's pretty clear from the code
why that happens and it will happen regardless of what panel you're using.
You can't expect to call pm_runtime_get once and pm_runtime_put 5 times and have
this driver work at all. The same thing for clk_enable_*/disable_* and so on.
You'll need to fix the issue you're seeing differently without the revert.
kind regards,
o.