diff mbox series

[v2] drm: bridge: samsung-dsim: Fix init during host transfer

Message ID 20230724151640.555490-1-frieder@fris.de (mailing list archive)
State New, archived
Headers show
Series [v2] drm: bridge: samsung-dsim: Fix init during host transfer | expand

Commit Message

Frieder Schrempf July 24, 2023, 3:16 p.m. UTC 
From: Frieder Schrempf <frieder.schrempf@kontron.de>

In case the downstream bridge or panel uses DSI transfers before the
DSI host was actually initialized through samsung_dsim_atomic_enable()
which clears the stop state (LP11) mode, all transfers will fail.

This happens with downstream bridges that are controlled by DSI
commands such as the tc358762.

As documented in [1] DSI hosts are expected to allow transfers
outside the normal bridge enable/disable flow.

To fix this make sure that stop state is cleared in
samsung_dsim_host_transfer() which restores the previous
behavior.

We also factor out the common code to enable/disable stop state
to samsung_dsim_set_stop_state().

[1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec")
Reported-by: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
Changes for v2:
  * Fix reversed stop state enable/disable in samsung_dsim_set_stop_state()

Hi Tim, could you please give this patch a try and report back if
it fixes your problem? Thanks!
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

Comments

Tim Harvey Aug. 17, 2023, 8:53 p.m. UTC | #1 
On Mon, Jul 24, 2023 at 8:16 AM Frieder Schrempf <frieder@fris.de> wrote:
>
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> In case the downstream bridge or panel uses DSI transfers before the
> DSI host was actually initialized through samsung_dsim_atomic_enable()
> which clears the stop state (LP11) mode, all transfers will fail.
>
> This happens with downstream bridges that are controlled by DSI
> commands such as the tc358762.
>
> As documented in [1] DSI hosts are expected to allow transfers
> outside the normal bridge enable/disable flow.
>
> To fix this make sure that stop state is cleared in
> samsung_dsim_host_transfer() which restores the previous
> behavior.
>
> We also factor out the common code to enable/disable stop state
> to samsung_dsim_set_stop_state().
>
> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
>
> Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec")
> Reported-by: Tim Harvey <tharvey@gateworks.com>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> Changes for v2:
>   * Fix reversed stop state enable/disable in samsung_dsim_set_stop_state()
>
> Hi Tim, could you please give this patch a try and report back if
> it fixes your problem? Thanks!
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 043b8109e64a..73ec60757dbc 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1386,6 +1386,18 @@ static void samsung_dsim_disable_irq(struct samsung_dsim *dsi)
>         disable_irq(dsi->irq);
>  }
>
> +static void samsung_dsim_set_stop_state(struct samsung_dsim *dsi, bool enable)
> +{
> +       u32 reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +
> +       if (enable)
> +               reg |= DSIM_FORCE_STOP_STATE;
> +       else
> +               reg &= ~DSIM_FORCE_STOP_STATE;
> +
> +       samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +}
> +
>  static int samsung_dsim_init(struct samsung_dsim *dsi)
>  {
>         const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
> @@ -1445,15 +1457,12 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>                                        struct drm_bridge_state *old_bridge_state)
>  {
>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> -       u32 reg;
>
>         if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>                 samsung_dsim_set_display_mode(dsi);
>                 samsung_dsim_set_display_enable(dsi, true);
>         } else {
> -               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> -               reg &= ~DSIM_FORCE_STOP_STATE;
> -               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +               samsung_dsim_set_stop_state(dsi, false);
>         }
>
>         dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
> @@ -1463,16 +1472,12 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>                                         struct drm_bridge_state *old_bridge_state)
>  {
>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> -       u32 reg;
>
>         if (!(dsi->state & DSIM_STATE_ENABLED))
>                 return;
>
> -       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> -               reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> -               reg |= DSIM_FORCE_STOP_STATE;
> -               samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> -       }
> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> +               samsung_dsim_set_stop_state(dsi, true);
>
>         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>  }
> @@ -1775,6 +1780,8 @@ static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
>         if (ret)
>                 return ret;
>
> +       samsung_dsim_set_stop_state(dsi, false);
> +
>         ret = mipi_dsi_create_packet(&xfer.packet, msg);
>         if (ret < 0)
>                 return ret;
> --
> 2.41.0
>

Frieder,

Sorry for the delay. Yes this resolves the regression I ran into. I
tested it on top of v6.5-rc6 on a gw72xx-0x with a DFROBOT DRF0678 7in
800x480 (Raspberry Pi) display which has the Toshiba TC358762
compatible DSI to DBI bridge.

Let's please get this into v6.5 as soon as possible.

Best regards,

Tim
Fabio Estevam Aug. 17, 2023, 8:59 p.m. UTC | #2 
Hi Tim,

On Thu, Aug 17, 2023 at 5:53 PM Tim Harvey <tharvey@gateworks.com> wrote:

> Frieder,
>
> Sorry for the delay. Yes this resolves the regression I ran into. I
> tested it on top of v6.5-rc6 on a gw72xx-0x with a DFROBOT DRF0678 7in
> 800x480 (Raspberry Pi) display which has the Toshiba TC358762
> compatible DSI to DBI bridge.
>
> Let's please get this into v6.5 as soon as possible.

Care to provide your Tested-by tag?
Tim Harvey Aug. 17, 2023, 9 p.m. UTC | #3 
On Thu, Aug 17, 2023 at 1:59 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Tim,
>
> On Thu, Aug 17, 2023 at 5:53 PM Tim Harvey <tharvey@gateworks.com> wrote:
>
> > Frieder,
> >
> > Sorry for the delay. Yes this resolves the regression I ran into. I
> > tested it on top of v6.5-rc6 on a gw72xx-0x with a DFROBOT DRF0678 7in
> > 800x480 (Raspberry Pi) display which has the Toshiba TC358762
> > compatible DSI to DBI bridge.
> >
> > Let's please get this into v6.5 as soon as possible.
>
> Care to provide your Tested-by tag?

Fabio,

Yes, sorry:
Tested-by: Tim Harvey <tharvey@gateworks.com> #
imx8mm-venice-gw72xx-0x with toshiba tc358762 MIPI DSI bridge

best regards,

Tim
Neil Armstrong Aug. 18, 2023, 8:37 a.m. UTC | #4 
On 24/07/2023 17:16, Frieder Schrempf wrote:
> From: Frieder Schrempf <frieder.schrempf@kontron.de>
> 
> In case the downstream bridge or panel uses DSI transfers before the
> DSI host was actually initialized through samsung_dsim_atomic_enable()
> which clears the stop state (LP11) mode, all transfers will fail.
> 
> This happens with downstream bridges that are controlled by DSI
> commands such as the tc358762.
> 
> As documented in [1] DSI hosts are expected to allow transfers
> outside the normal bridge enable/disable flow.
> 
> To fix this make sure that stop state is cleared in
> samsung_dsim_host_transfer() which restores the previous
> behavior.
> 
> We also factor out the common code to enable/disable stop state
> to samsung_dsim_set_stop_state().
> 
> [1] https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation
> 
> Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec")
> Reported-by: Tim Harvey <tharvey@gateworks.com>
> Signed-off-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> Changes for v2:
>    * Fix reversed stop state enable/disable in samsung_dsim_set_stop_state()
> 
> Hi Tim, could you please give this patch a try and report back if
> it fixes your problem? Thanks!
> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 27 +++++++++++++++++----------
>   1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 043b8109e64a..73ec60757dbc 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1386,6 +1386,18 @@ static void samsung_dsim_disable_irq(struct samsung_dsim *dsi)
>   	disable_irq(dsi->irq);
>   }
>   
> +static void samsung_dsim_set_stop_state(struct samsung_dsim *dsi, bool enable)
> +{
> +	u32 reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> +
> +	if (enable)
> +		reg |= DSIM_FORCE_STOP_STATE;
> +	else
> +		reg &= ~DSIM_FORCE_STOP_STATE;
> +
> +	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +}
> +
>   static int samsung_dsim_init(struct samsung_dsim *dsi)
>   {
>   	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
> @@ -1445,15 +1457,12 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>   				       struct drm_bridge_state *old_bridge_state)
>   {
>   	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> -	u32 reg;
>   
>   	if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>   		samsung_dsim_set_display_mode(dsi);
>   		samsung_dsim_set_display_enable(dsi, true);
>   	} else {
> -		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> -		reg &= ~DSIM_FORCE_STOP_STATE;
> -		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> +		samsung_dsim_set_stop_state(dsi, false);
>   	}
>   
>   	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
> @@ -1463,16 +1472,12 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
>   					struct drm_bridge_state *old_bridge_state)
>   {
>   	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> -	u32 reg;
>   
>   	if (!(dsi->state & DSIM_STATE_ENABLED))
>   		return;
>   
> -	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
> -		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
> -		reg |= DSIM_FORCE_STOP_STATE;
> -		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> -	}
> +	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> +		samsung_dsim_set_stop_state(dsi, true);
>   
>   	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>   }
> @@ -1775,6 +1780,8 @@ static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
>   	if (ret)
>   		return ret;
>   
> +	samsung_dsim_set_stop_state(dsi, false);
> +
>   	ret = mipi_dsi_create_packet(&xfer.packet, msg);
>   	if (ret < 0)
>   		return ret;

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Neil Armstrong Aug. 18, 2023, 8:40 a.m. UTC | #5 
Hi,

On Mon, 24 Jul 2023 17:16:32 +0200, Frieder Schrempf wrote:
> In case the downstream bridge or panel uses DSI transfers before the
> DSI host was actually initialized through samsung_dsim_atomic_enable()
> which clears the stop state (LP11) mode, all transfers will fail.
> 
> This happens with downstream bridges that are controlled by DSI
> commands such as the tc358762.
> 
> [...]

Thanks, Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-fixes)

[1/1] drm: bridge: samsung-dsim: Fix init during host transfer
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=20c827683de05a6c7e7ae7fae586899690693251
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 043b8109e64a..73ec60757dbc 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1386,6 +1386,18 @@  static void samsung_dsim_disable_irq(struct samsung_dsim *dsi)
 	disable_irq(dsi->irq);
 }
 
+static void samsung_dsim_set_stop_state(struct samsung_dsim *dsi, bool enable)
+{
+	u32 reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
+
+	if (enable)
+		reg |= DSIM_FORCE_STOP_STATE;
+	else
+		reg &= ~DSIM_FORCE_STOP_STATE;
+
+	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+}
+
 static int samsung_dsim_init(struct samsung_dsim *dsi)
 {
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
@@ -1445,15 +1457,12 @@  static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
 				       struct drm_bridge_state *old_bridge_state)
 {
 	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
-	u32 reg;
 
 	if (samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
 		samsung_dsim_set_display_mode(dsi);
 		samsung_dsim_set_display_enable(dsi, true);
 	} else {
-		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
-		reg &= ~DSIM_FORCE_STOP_STATE;
-		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
+		samsung_dsim_set_stop_state(dsi, false);
 	}
 
 	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
@@ -1463,16 +1472,12 @@  static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
 					struct drm_bridge_state *old_bridge_state)
 {
 	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
-	u32 reg;
 
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return;
 
-	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
-		reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
-		reg |= DSIM_FORCE_STOP_STATE;
-		samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
-	}
+	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
+		samsung_dsim_set_stop_state(dsi, true);
 
 	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
 }
@@ -1775,6 +1780,8 @@  static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
 	if (ret)
 		return ret;
 
+	samsung_dsim_set_stop_state(dsi, false);
+
 	ret = mipi_dsi_create_packet(&xfer.packet, msg);
 	if (ret < 0)
 		return ret;