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

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

Posted by Frieder Schrempf 2 years, 6 months ago

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

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

Posted by Neil Armstrong 2 years, 5 months ago

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

-- 
Neil

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

Posted by Neil Armstrong 2 years, 5 months ago

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>

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

Posted by Tim Harvey 2 years, 5 months ago

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

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

Posted by Fabio Estevam 2 years, 5 months ago

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?

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

Posted by Tim Harvey 2 years, 5 months ago

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