[PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Michael Walle]

The FORCE_STOP_STATE bit is unsuitable to force the DSI link into LP-11
mode. It seems the bridge internally queues DSI packets and when the
FORCE_STOP_STATE bit is cleared, they are sent in close succession
without any useful timing (this also means that the DSI lanes won't go
into LP-11 mode). The length of this gibberish varies between 1ms and
5ms. This sometimes breaks an attached bridge (TI SN65DSI84 in this
case). In our case, the bridge will fail in about 1 per 500 reboots.

The FORCE_STOP_STATE handling was introduced to have the DSI lanes in
LP-11 state during the .pre_enable phase. But as it turns out, none of
this is needed at all. Between samsung_dsim_init() and
samsung_dsim_set_display_enable() the lanes are already in LP-11 mode.
The code as it was before commit 20c827683de0 ("drm: bridge:
samsung-dsim: Fix init during host transfer") and 0c14d3130654 ("drm:
bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec") was correct
in this regard.

This patch basically reverts both commits. It was tested on an i.MX8M
SoC with an SN65DSI84 bridge. The signals were probed and the DSI
packets were decoded during initialization and link start-up. After this
patch the first DSI packet on the link is a VSYNC packet and the timing
is correct.

Command mode between .pre_enable and .enable was also briefly tested by
a quick hack. There was no DSI link partner which would have responded,
but it was made sure the DSI packet was send on the link. As a side
note, the command mode seems to just work in HS mode. I couldn't find
that the bridge will handle commands in LP mode.

Fixes: 20c827683de0 ("drm: bridge: samsung-dsim: Fix init during host transfer")
Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec")
Signed-off-by: Michael Walle <mwalle@kernel.org>
---
Let me know wether this should be two commits each reverting one, but both
commits appeared first in kernel 6.5.

 drivers/gpu/drm/bridge/samsung-dsim.c | 32 ++-------------------------
 1 file changed, 2 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index cf777bdb25d2..4233a50baac7 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -939,10 +939,6 @@ static int samsung_dsim_init_link(struct samsung_dsim *dsi)
 	reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
 	reg &= ~DSIM_STOP_STATE_CNT_MASK;
 	reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
-
-	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
-		reg |= DSIM_FORCE_STOP_STATE;
-
 	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
 
 	reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
@@ -1387,18 +1383,6 @@ 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;
@@ -1448,9 +1432,6 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
 		ret = samsung_dsim_init(dsi);
 		if (ret)
 			return;
-
-		samsung_dsim_set_display_mode(dsi);
-		samsung_dsim_set_display_enable(dsi, true);
 	}
 }
 
@@ -1459,12 +1440,8 @@ static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
 {
 	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
 
-	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 {
-		samsung_dsim_set_stop_state(dsi, false);
-	}
+	samsung_dsim_set_display_mode(dsi);
+	samsung_dsim_set_display_enable(dsi, true);
 
 	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
 }
@@ -1477,9 +1454,6 @@ static void samsung_dsim_atomic_disable(struct drm_bridge *bridge,
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return;
 
-	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
-		samsung_dsim_set_stop_state(dsi, true);
-
 	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
 }
 
@@ -1781,8 +1755,6 @@ 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.39.2

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Alexander Stein]

Hi Michael,

Am Montag, 13. November 2023, 17:43:44 CET schrieb Michael Walle:
> The FORCE_STOP_STATE bit is unsuitable to force the DSI link into LP-11
> mode. It seems the bridge internally queues DSI packets and when the
> FORCE_STOP_STATE bit is cleared, they are sent in close succession
> without any useful timing (this also means that the DSI lanes won't go
> into LP-11 mode). The length of this gibberish varies between 1ms and
> 5ms. This sometimes breaks an attached bridge (TI SN65DSI84 in this
> case). In our case, the bridge will fail in about 1 per 500 reboots.
> 
> The FORCE_STOP_STATE handling was introduced to have the DSI lanes in
> LP-11 state during the .pre_enable phase. But as it turns out, none of
> this is needed at all. Between samsung_dsim_init() and
> samsung_dsim_set_display_enable() the lanes are already in LP-11 mode.

Apparently LP-11 is actually entered with the call to 
samsung_dsim_enable_lane(), but I don't know about other requisites on that 
matter. Unfortunately documentation lacks a lot in that regard.

> The code as it was before commit 20c827683de0 ("drm: bridge:
> samsung-dsim: Fix init during host transfer") and 0c14d3130654 ("drm:
> bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec") was correct
> in this regard.
> 
> This patch basically reverts both commits. It was tested on an i.MX8M
> SoC with an SN65DSI84 bridge. The signals were probed and the DSI
> packets were decoded during initialization and link start-up. After this
> patch the first DSI packet on the link is a VSYNC packet and the timing
> is correct.

At which point does SN65DSI84 require LP-11?
You have access to a DSI/D-PHY analyzer?

> Command mode between .pre_enable and .enable was also briefly tested by
> a quick hack. There was no DSI link partner which would have responded,
> but it was made sure the DSI packet was send on the link. As a side
> note, the command mode seems to just work in HS mode. I couldn't find
> that the bridge will handle commands in LP mode.

AFAIK ti-sn65dsi83.c only uses I2C for communication. Did you send DSI read/
writes instead?

best regards,
Alexander

> Fixes: 20c827683de0 ("drm: bridge: samsung-dsim: Fix init during host
> transfer") Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M
> enable flow to meet spec") Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
> Let me know wether this should be two commits each reverting one, but both
> commits appeared first in kernel 6.5.
> 
>  drivers/gpu/drm/bridge/samsung-dsim.c | 32 ++-------------------------
>  1 file changed, 2 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c index cf777bdb25d2..4233a50baac7
> 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -939,10 +939,6 @@ static int samsung_dsim_init_link(struct samsung_dsim
> *dsi) reg = samsung_dsim_read(dsi, DSIM_ESCMODE_REG);
>  	reg &= ~DSIM_STOP_STATE_CNT_MASK;
>  	reg |= DSIM_STOP_STATE_CNT(driver_data->reg_values[STOP_STATE_CNT]);
> -
> -	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> -		reg |= DSIM_FORCE_STOP_STATE;
> -
>  	samsung_dsim_write(dsi, DSIM_ESCMODE_REG, reg);
> 
>  	reg = DSIM_BTA_TIMEOUT(0xff) | DSIM_LPDR_TIMEOUT(0xffff);
> @@ -1387,18 +1383,6 @@ 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;
> @@ -1448,9 +1432,6 @@ static void samsung_dsim_atomic_pre_enable(struct
> drm_bridge *bridge, ret = samsung_dsim_init(dsi);
>  		if (ret)
>  			return;
> -
> -		samsung_dsim_set_display_mode(dsi);
> -		samsung_dsim_set_display_enable(dsi, true);
>  	}
>  }
> 
> @@ -1459,12 +1440,8 @@ static void samsung_dsim_atomic_enable(struct
> drm_bridge *bridge, {
>  	struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> 
> -	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 {
> -		samsung_dsim_set_stop_state(dsi, false);
> -	}
> +	samsung_dsim_set_display_mode(dsi);
> +	samsung_dsim_set_display_enable(dsi, true);
> 
>  	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
>  }
> @@ -1477,9 +1454,6 @@ static void samsung_dsim_atomic_disable(struct
> drm_bridge *bridge, if (!(dsi->state & DSIM_STATE_ENABLED))
>  		return;
> 
> -	if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> -		samsung_dsim_set_stop_state(dsi, true);
> -
>  	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>  }
> 
> @@ -1781,8 +1755,6 @@ 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;


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Michael Walle]

Hi Alexander,

>> The FORCE_STOP_STATE bit is unsuitable to force the DSI link into 
>> LP-11
>> mode. It seems the bridge internally queues DSI packets and when the
>> FORCE_STOP_STATE bit is cleared, they are sent in close succession
>> without any useful timing (this also means that the DSI lanes won't go
>> into LP-11 mode). The length of this gibberish varies between 1ms and
>> 5ms. This sometimes breaks an attached bridge (TI SN65DSI84 in this
>> case). In our case, the bridge will fail in about 1 per 500 reboots.
>> 
>> The FORCE_STOP_STATE handling was introduced to have the DSI lanes in
>> LP-11 state during the .pre_enable phase. But as it turns out, none of
>> this is needed at all. Between samsung_dsim_init() and
>> samsung_dsim_set_display_enable() the lanes are already in LP-11 mode.
> 
> Apparently LP-11 is actually entered with the call to
> samsung_dsim_enable_lane(), but I don't know about other requisites on 
> that
> matter. Unfortunately documentation lacks a lot in that regard.

Which is called during samsung_dsim_atomic_pre_enable(). So I'm not sure
why the FORCE_STOP_STATE was needed at all. Lanes will be in LP-11 mode
if the video stream isn't enabled.

>> The code as it was before commit 20c827683de0 ("drm: bridge:
>> samsung-dsim: Fix init during host transfer") and 0c14d3130654 ("drm:
>> bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec") was 
>> correct
>> in this regard.
>> 
>> This patch basically reverts both commits. It was tested on an i.MX8M
>> SoC with an SN65DSI84 bridge. The signals were probed and the DSI
>> packets were decoded during initialization and link start-up. After 
>> this
>> patch the first DSI packet on the link is a VSYNC packet and the 
>> timing
>> is correct.
> 
> At which point does SN65DSI84 require LP-11?

I guess I have the same requirement as Frieder (we use the same bridge).
According to the datasheet, the DSI data must be in LP-11 when releasing
EN. According to the init sequence:
  - asserting EN
  - configure CSRs
  - enable video stream

Although not, stated explicitly, LP-11 should also be active during CSR
writes.

But after all, the DSIM driver should adhere to the linux requirement,
which Frieder cited [1].

> You have access to a DSI/D-PHY analyzer?

A pretty fast oscilloscope with an DSI decoder. So we can analyze one 
DSI
lane.

>> Command mode between .pre_enable and .enable was also briefly tested 
>> by
>> a quick hack. There was no DSI link partner which would have 
>> responded,
>> but it was made sure the DSI packet was send on the link. As a side
>> note, the command mode seems to just work in HS mode. I couldn't find
>> that the bridge will handle commands in LP mode.
> 
> AFAIK ti-sn65dsi83.c only uses I2C for communication. Did you send DSI 
> read/
> writes instead?

Yeah the bridge only supports I2C. I just wanted to try that a DSI 
command
will still work after this patch. As a quick hack, I just added an
mipi_dsi_generic_write() to sn65dsi83_atomic_pre_enable() and made sure
there is a DSI write packet on the actual link.

-michael

> 
> best regards,
> Alexander
> 
>> Fixes: 20c827683de0 ("drm: bridge: samsung-dsim: Fix init during host
>> transfer") Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M
>> enable flow to meet spec") Signed-off-by: Michael Walle 
>> <mwalle@kernel.org>
>> ---
>> Let me know wether this should be two commits each reverting one, but 
>> both
>> commits appeared first in kernel 6.5.
>> 

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

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Frieder Schrempf]

Hi Michael,

On 13.11.23 17:43, Michael Walle wrote:
> The FORCE_STOP_STATE bit is unsuitable to force the DSI link into LP-11
> mode. It seems the bridge internally queues DSI packets and when the
> FORCE_STOP_STATE bit is cleared, they are sent in close succession
> without any useful timing (this also means that the DSI lanes won't go
> into LP-11 mode). The length of this gibberish varies between 1ms and
> 5ms. This sometimes breaks an attached bridge (TI SN65DSI84 in this
> case). In our case, the bridge will fail in about 1 per 500 reboots.
> 
> The FORCE_STOP_STATE handling was introduced to have the DSI lanes in
> LP-11 state during the .pre_enable phase. But as it turns out, none of
> this is needed at all. Between samsung_dsim_init() and
> samsung_dsim_set_display_enable() the lanes are already in LP-11 mode.
> The code as it was before commit 20c827683de0 ("drm: bridge:
> samsung-dsim: Fix init during host transfer") and 0c14d3130654 ("drm:
> bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec") was correct
> in this regard.
> 
> This patch basically reverts both commits. It was tested on an i.MX8M
> SoC with an SN65DSI84 bridge. The signals were probed and the DSI
> packets were decoded during initialization and link start-up. After this
> patch the first DSI packet on the link is a VSYNC packet and the timing
> is correct.
> 
> Command mode between .pre_enable and .enable was also briefly tested by
> a quick hack. There was no DSI link partner which would have responded,
> but it was made sure the DSI packet was send on the link. As a side
> note, the command mode seems to just work in HS mode. I couldn't find
> that the bridge will handle commands in LP mode.
> 
> Fixes: 20c827683de0 ("drm: bridge: samsung-dsim: Fix init during host transfer")
> Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec")
> Signed-off-by: Michael Walle <mwalle@kernel.org>

Thanks for the fix. Your explanation sounds convincing.

Unfortunately I'm currently not able to understand why I had to
introduce these changes in the first place. What I tried to fix was
exactly this kind of issue where the display stays black every few
hundred boot cycles.

My current guess would be that the issue I was seeing was already fixed
with dd9e329af723 ("drm/bridge: ti-sn65dsi83: Fix enable/disable flow to
meet spec") and I didn't properly test both changes separately.

My cheap scope is not able to capture the DSI signals and I admit that
we didn't use our more expensive equipment to verify the changes back then.

Instead, we had an automated test setup to do cyclic on/off switching
for the display and check for a black screen using a sensor. It is quite
a hassle to set up and I'm currently not planning to spend that much
effort to verify this change again.

Anyway, I currently don't see any reasons to not revert my changes. Your
revert looks correct and seems to work fine as far as I can tell.

Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Thanks
Frieder

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Michael Walle]

Hi,

> My current guess would be that the issue I was seeing was already fixed
> with dd9e329af723 ("drm/bridge: ti-sn65dsi83: Fix enable/disable flow 
> to
> meet spec") and I didn't properly test both changes separately.

I had the exact same thought, as I've found your second patch.

> My cheap scope is not able to capture the DSI signals and I admit that
> we didn't use our more expensive equipment to verify the changes back 
> then.
> 
> Instead, we had an automated test setup to do cyclic on/off switching
> for the display and check for a black screen using a sensor. It is 
> quite
> a hassle to set up and I'm currently not planning to spend that much
> effort to verify this change again.

That is actually, what we are also doing right now and how the issue was
found in the first place.

> Anyway, I currently don't see any reasons to not revert my changes. 
> Your
> revert looks correct and seems to work fine as far as I can tell.
> 
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Thanks!

-michael

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Michael Walle]

> The FORCE_STOP_STATE bit is unsuitable to force the DSI link into LP-11
> mode. It seems the bridge internally queues DSI packets and when the
> FORCE_STOP_STATE bit is cleared, they are sent in close succession
> without any useful timing (this also means that the DSI lanes won't go
> into LP-11 mode). The length of this gibberish varies between 1ms and
> 5ms. This sometimes breaks an attached bridge (TI SN65DSI84 in this
> case). In our case, the bridge will fail in about 1 per 500 reboots.
> 
> The FORCE_STOP_STATE handling was introduced to have the DSI lanes in
> LP-11 state during the .pre_enable phase. But as it turns out, none of
> this is needed at all. Between samsung_dsim_init() and
> samsung_dsim_set_display_enable() the lanes are already in LP-11 mode.
> The code as it was before commit 20c827683de0 ("drm: bridge:
> samsung-dsim: Fix init during host transfer") and 0c14d3130654 ("drm:
> bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec") was correct
> in this regard.
> 
> This patch basically reverts both commits. It was tested on an i.MX8M
> SoC with an SN65DSI84 bridge. The signals were probed and the DSI
> packets were decoded during initialization and link start-up. After 
> this
> patch the first DSI packet on the link is a VSYNC packet and the timing
> is correct.
> 
> Command mode between .pre_enable and .enable was also briefly tested by
> a quick hack. There was no DSI link partner which would have responded,
> but it was made sure the DSI packet was send on the link. As a side
> note, the command mode seems to just work in HS mode. I couldn't find
> that the bridge will handle commands in LP mode.
> 
> Fixes: 20c827683de0 ("drm: bridge: samsung-dsim: Fix init during host 
> transfer")
> Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow 
> to meet spec")
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
> Let me know wether this should be two commits each reverting one, but 
> both
> commits appeared first in kernel 6.5.

Are there any news?

-michael

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Frieder Schrempf]

On 01.12.23 10:04, Michael Walle wrote:
>> The FORCE_STOP_STATE bit is unsuitable to force the DSI link into LP-11
>> mode. It seems the bridge internally queues DSI packets and when the
>> FORCE_STOP_STATE bit is cleared, they are sent in close succession
>> without any useful timing (this also means that the DSI lanes won't go
>> into LP-11 mode). The length of this gibberish varies between 1ms and
>> 5ms. This sometimes breaks an attached bridge (TI SN65DSI84 in this
>> case). In our case, the bridge will fail in about 1 per 500 reboots.
>>
>> The FORCE_STOP_STATE handling was introduced to have the DSI lanes in
>> LP-11 state during the .pre_enable phase. But as it turns out, none of
>> this is needed at all. Between samsung_dsim_init() and
>> samsung_dsim_set_display_enable() the lanes are already in LP-11 mode.
>> The code as it was before commit 20c827683de0 ("drm: bridge:
>> samsung-dsim: Fix init during host transfer") and 0c14d3130654 ("drm:
>> bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec") was correct
>> in this regard.
>>
>> This patch basically reverts both commits. It was tested on an i.MX8M
>> SoC with an SN65DSI84 bridge. The signals were probed and the DSI
>> packets were decoded during initialization and link start-up. After this
>> patch the first DSI packet on the link is a VSYNC packet and the timing
>> is correct.
>>
>> Command mode between .pre_enable and .enable was also briefly tested by
>> a quick hack. There was no DSI link partner which would have responded,
>> but it was made sure the DSI packet was send on the link. As a side
>> note, the command mode seems to just work in HS mode. I couldn't find
>> that the bridge will handle commands in LP mode.
>>
>> Fixes: 20c827683de0 ("drm: bridge: samsung-dsim: Fix init during host
>> transfer")
>> Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable
>> flow to meet spec")
>> Signed-off-by: Michael Walle <mwalle@kernel.org>
>> ---
>> Let me know wether this should be two commits each reverting one, but
>> both
>> commits appeared first in kernel 6.5.
> 
> Are there any news?

Inki, are you picking this up? Or if not, who will?

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Inki Dae]

[-- Attachment #1: Type: text/plain, Size: 2588 bytes --]

2023년 12월 19일 (화) 오전 11:11, Frieder Schrempf <frieder.schrempf@kontron.de>님이
작성:

> On 01.12.23 10:04, Michael Walle wrote:
> >> The FORCE_STOP_STATE bit is unsuitable to force the DSI link into LP-11
> >> mode. It seems the bridge internally queues DSI packets and when the
> >> FORCE_STOP_STATE bit is cleared, they are sent in close succession
> >> without any useful timing (this also means that the DSI lanes won't go
> >> into LP-11 mode). The length of this gibberish varies between 1ms and
> >> 5ms. This sometimes breaks an attached bridge (TI SN65DSI84 in this
> >> case). In our case, the bridge will fail in about 1 per 500 reboots.
> >>
> >> The FORCE_STOP_STATE handling was introduced to have the DSI lanes in
> >> LP-11 state during the .pre_enable phase. But as it turns out, none of
> >> this is needed at all. Between samsung_dsim_init() and
> >> samsung_dsim_set_display_enable() the lanes are already in LP-11 mode.
> >> The code as it was before commit 20c827683de0 ("drm: bridge:
> >> samsung-dsim: Fix init during host transfer") and 0c14d3130654 ("drm:
> >> bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec") was correct
> >> in this regard.
> >>
> >> This patch basically reverts both commits. It was tested on an i.MX8M
> >> SoC with an SN65DSI84 bridge. The signals were probed and the DSI
> >> packets were decoded during initialization and link start-up. After this
> >> patch the first DSI packet on the link is a VSYNC packet and the timing
> >> is correct.
> >>
> >> Command mode between .pre_enable and .enable was also briefly tested by
> >> a quick hack. There was no DSI link partner which would have responded,
> >> but it was made sure the DSI packet was send on the link. As a side
> >> note, the command mode seems to just work in HS mode. I couldn't find
> >> that the bridge will handle commands in LP mode.
> >>
> >> Fixes: 20c827683de0 ("drm: bridge: samsung-dsim: Fix init during host
> >> transfer")
> >> Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable
> >> flow to meet spec")
> >> Signed-off-by: Michael Walle <mwalle@kernel.org>
> >> ---
> >> Let me know wether this should be two commits each reverting one, but
> >> both
> >> commits appeared first in kernel 6.5.
> >
> > Are there any news?
>
> Inki, are you picking this up? Or if not, who will?
>

I can pick it up but it would be better to go to the drm-misc tree. If
nobody cares about it then I will pick it up. :)

acked-by : Inki Dae <inki.dae@samsung.com>

Thanks,
Inki Dae

[-- Attachment #2: Type: text/html, Size: 3709 bytes --]

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Michael Walle]

Hi,

>> Inki, are you picking this up? Or if not, who will?
> 
> I can pick it up but it would be better to go to the drm-misc tree. If
> nobody cares about it then I will pick it up. :)
> 
> acked-by : Inki Dae <inki.dae@samsung.com>

Who is going to pick this up? Who has access to the drm-misc tree?

-michael

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Daniel Vetter]

On Tue, Jan 09, 2024 at 09:47:20AM +0100, Michael Walle wrote:
> Hi,
> 
> > > Inki, are you picking this up? Or if not, who will?
> > 
> > I can pick it up but it would be better to go to the drm-misc tree. If
> > nobody cares about it then I will pick it up. :)
> > 
> > acked-by : Inki Dae <inki.dae@samsung.com>
> 
> Who is going to pick this up? Who has access to the drm-misc tree?

Inki has, so I'm assuming since he acked he'll also merge.
-Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Inki Dae]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]

Really sorry for late. Will pick it up.

Thanks,
Inki Dae

2024년 1월 9일 (화) 오후 9:50, Daniel Vetter <daniel@ffwll.ch>님이 작성:

> On Tue, Jan 09, 2024 at 09:47:20AM +0100, Michael Walle wrote:
> > Hi,
> >
> > > > Inki, are you picking this up? Or if not, who will?
> > >
> > > I can pick it up but it would be better to go to the drm-misc tree. If
> > > nobody cares about it then I will pick it up. :)
> > >
> > > acked-by : Inki Dae <inki.dae@samsung.com>
> >
> > Who is going to pick this up? Who has access to the drm-misc tree?
>
> Inki has, so I'm assuming since he acked he'll also merge.
> -Sima
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>

[-- Attachment #2: Type: text/html, Size: 1305 bytes --]

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Dave Airlie]

Just FYI this conflictted pretty heavily with drm-misc-next changes in
the same area, someone should check drm-tip has the correct
resolution, I'm not really sure what is definitely should be.

Dave.

On Fri, 19 Jan 2024 at 16:37, Inki Dae <daeinki@gmail.com> wrote:
>
> Really sorry for late. Will pick it up.
>
> Thanks,
> Inki Dae
>
> 2024년 1월 9일 (화) 오후 9:50, Daniel Vetter <daniel@ffwll.ch>님이 작성:
>>
>> On Tue, Jan 09, 2024 at 09:47:20AM +0100, Michael Walle wrote:
>> > Hi,
>> >
>> > > > Inki, are you picking this up? Or if not, who will?
>> > >
>> > > I can pick it up but it would be better to go to the drm-misc tree. If
>> > > nobody cares about it then I will pick it up. :)
>> > >
>> > > acked-by : Inki Dae <inki.dae@samsung.com>
>> >
>> > Who is going to pick this up? Who has access to the drm-misc tree?
>>
>> Inki has, so I'm assuming since he acked he'll also merge.
>> -Sima
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Frieder Schrempf]

On 26.01.24 19:28, Dave Airlie wrote:
> Just FYI this conflictted pretty heavily with drm-misc-next changes in
> the same area, someone should check drm-tip has the correct
> resolution, I'm not really sure what is definitely should be.
> 
> Dave.

Thanks! I took a quick look at what is now in Linus' tree and it looks
correct to me. The only thing I'm missing is my Reviewed-by tag which
got lost somewhere, but I can get over that.

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Michael Walle]

> Just FYI this conflictted pretty heavily with drm-misc-next changes in
> the same area, someone should check drm-tip has the correct
> resolution, I'm not really sure what is definitely should be.

FWIW, this looks rather messy now. The drm-tip doesn't build.

There was a new call to samsung_dsim_set_stop_state() introduced
in commit b2fe2292624ac (drm: bridge: samsung-dsim: enter display
mode in the enable() callback).

Also merge commit 663a907e199b (Merge remote-tracking branch
'drm-misc/drm-misc-next' into drm-tip) is broken because it
completely removes samsung_dsim_atomic_disable(). Dunno whats
going on there.

I'm just about to look at getting it to compile again and
I'm trying to retest it.

-michael

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Michael Walle]

> Also merge commit 663a907e199b (Merge remote-tracking branch
> 'drm-misc/drm-misc-next' into drm-tip) is broken because it
> completely removes samsung_dsim_atomic_disable(). Dunno whats
> going on there.

Actually, that merge commit looks even worse. It somehow folds
the original samsung_dsim_atomic_disable() into
samsung_dsim_atomic_enable(). Therefore, now the enable op
will clear the DSIM_STATE_VIDOUT_AVAILABLE flag?! It will also
never be set. Dunno how to proceed here.

-michael

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Michael Walle]

>> Just FYI this conflictted pretty heavily with drm-misc-next changes in
>> the same area, someone should check drm-tip has the correct
>> resolution, I'm not really sure what is definitely should be.
> 
> FWIW, this looks rather messy now. The drm-tip doesn't build.
> 
> There was a new call to samsung_dsim_set_stop_state() introduced
> in commit b2fe2292624ac (drm: bridge: samsung-dsim: enter display
> mode in the enable() callback).

I had a closer look at the latest linux-next (where somehow my patch
made it into) and tried to apply commit b2fe2292624ac (drm: bridge:
samsung-dsim: enter display mode in the enable() callback). It looks
like only the following hunk is still needed from that patch. Everything
else is covered by this fixes patch.

Dario, could you rebase your commit onto this patch? I had a quick test
with this change and it seems to work fine for our case.

--snip--
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 63a1a0c88be4..92755c90e7d2 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1498,6 +1498,8 @@ static void samsung_dsim_atomic_disable(struct 
drm_bridge *bridge,
         if (!(dsi->state & DSIM_STATE_ENABLED))
                 return;

+       samsung_dsim_set_display_enable(dsi, false);
+
         dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
  }

@@ -1506,8 +1508,6 @@ static void 
samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
  {
         struct samsung_dsim *dsi = bridge_to_dsi(bridge);

-       samsung_dsim_set_display_enable(dsi, false);
-
         dsi->state &= ~DSIM_STATE_ENABLED;
         pm_runtime_put_sync(dsi->dev);
  }
--snip--

-michael

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Frieder Schrempf]

On 29.01.24 10:20, Frieder Schrempf wrote:
> On 26.01.24 19:28, Dave Airlie wrote:
>> Just FYI this conflictted pretty heavily with drm-misc-next changes in
>> the same area, someone should check drm-tip has the correct
>> resolution, I'm not really sure what is definitely should be.
>>
>> Dave.
> 
> Thanks! I took a quick look at what is now in Linus' tree and it looks
> correct to me. The only thing I'm missing is my Reviewed-by tag which
> got lost somewhere, but I can get over that.

Apparently I missed the point here. I was looking at the wrong trees
(drm-next and master instead of drm-misc-next and drm-tip). Sorry for
the noise. Michael already pointed out the correct details.

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Dario Binacchi]

Hi Michael,

On Mon, Jan 29, 2024 at 5:06 PM Michael Walle <mwalle@kernel.org> wrote:
>
> >> Just FYI this conflictted pretty heavily with drm-misc-next changes in
> >> the same area, someone should check drm-tip has the correct
> >> resolution, I'm not really sure what is definitely should be.
> >
> > FWIW, this looks rather messy now. The drm-tip doesn't build.
> >
> > There was a new call to samsung_dsim_set_stop_state() introduced
> > in commit b2fe2292624ac (drm: bridge: samsung-dsim: enter display
> > mode in the enable() callback).
>
> I had a closer look at the latest linux-next (where somehow my patch
> made it into) and tried to apply commit b2fe2292624ac (drm: bridge:
> samsung-dsim: enter display mode in the enable() callback). It looks
> like only the following hunk is still needed from that patch. Everything
> else is covered by this fixes patch.
>
> Dario, could you rebase your commit onto this patch? I had a quick test
> with this change and it seems to work fine for our case.
>
> --snip--
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 63a1a0c88be4..92755c90e7d2 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1498,6 +1498,8 @@ static void samsung_dsim_atomic_disable(struct
> drm_bridge *bridge,
>          if (!(dsi->state & DSIM_STATE_ENABLED))
>                  return;
>
> +       samsung_dsim_set_display_enable(dsi, false);
> +
>          dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>   }
>
> @@ -1506,8 +1508,6 @@ static void
> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>   {
>          struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>
> -       samsung_dsim_set_display_enable(dsi, false);
> -
>          dsi->state &= ~DSIM_STATE_ENABLED;
>          pm_runtime_put_sync(dsi->dev);
>   }
> --snip--
>
> -michael

I'm sorry, but I didn't understand well what I have to do.
This is what I have done:

git clone https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next.git
cd linux-next
# add your changes, the ones of the emails
git am --reject 0001-drm-bridge-samsung-dsim-enter-display-mode-in-the-en.patch

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 92755c90e7d2..b47929072583 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1508,6 +1508,9 @@ static void
samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
 {
        struct samsung_dsim *dsi = bridge_to_dsi(bridge);

+       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
+               samsung_dsim_set_stop_state(dsi, true);
+
        dsi->state &= ~DSIM_STATE_ENABLED;
        pm_runtime_put_sync(dsi->dev);
 }

And then test the driver for my use case.

Is everything I wrote correct, or am I making a mistake?

Thanks and regards,
Dario

-- 

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

www.amarulasolutions.com

Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

[Michael Walle]

Hi Dario,

>> >> Just FYI this conflictted pretty heavily with drm-misc-next changes in
>> >> the same area, someone should check drm-tip has the correct
>> >> resolution, I'm not really sure what is definitely should be.
>> >
>> > FWIW, this looks rather messy now. The drm-tip doesn't build.
>> >
>> > There was a new call to samsung_dsim_set_stop_state() introduced
>> > in commit b2fe2292624ac (drm: bridge: samsung-dsim: enter display
>> > mode in the enable() callback).
>> 
>> I had a closer look at the latest linux-next (where somehow my patch
>> made it into) and tried to apply commit b2fe2292624ac (drm: bridge:
>> samsung-dsim: enter display mode in the enable() callback). It looks
>> like only the following hunk is still needed from that patch. 
>> Everything
>> else is covered by this fixes patch.
>> 
>> Dario, could you rebase your commit onto this patch? I had a quick 
>> test
>> with this change and it seems to work fine for our case.
>> 
>> --snip--
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index 63a1a0c88be4..92755c90e7d2 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -1498,6 +1498,8 @@ static void samsung_dsim_atomic_disable(struct
>> drm_bridge *bridge,
>>          if (!(dsi->state & DSIM_STATE_ENABLED))
>>                  return;
>> 
>> +       samsung_dsim_set_display_enable(dsi, false);
>> +
>>          dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
>>   }
>> 
>> @@ -1506,8 +1508,6 @@ static void
>> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>>   {
>>          struct samsung_dsim *dsi = bridge_to_dsi(bridge);
>> 
>> -       samsung_dsim_set_display_enable(dsi, false);
>> -
>>          dsi->state &= ~DSIM_STATE_ENABLED;
>>          pm_runtime_put_sync(dsi->dev);
>>   }
>> --snip--
>> 
>> -michael
> 
> I'm sorry, but I didn't understand well what I have to do.

Basically, just rebase your patch (drm: bridge: samsung-dsim:
enter display mode in the enable() callback) on top of
linux-next.

> This is what I have done:
> 
> git clone 
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/next/linux-next.git
> cd linux-next
> # add your changes, the ones of the emails
> git am --reject 
> 0001-drm-bridge-samsung-dsim-enter-display-mode-in-the-en.patch
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 92755c90e7d2..b47929072583 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1508,6 +1508,9 @@ static void
> samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>  {
>         struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> 
> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type))
> +               samsung_dsim_set_stop_state(dsi, true);
> +

This one should be removed. There is no stop state anymore.
With that hunk, it doesn't compile anyway.

>         dsi->state &= ~DSIM_STATE_ENABLED;
>         pm_runtime_put_sync(dsi->dev);
>  }
> 
> And then test the driver for my use case.

Yes. The hunk I've posted above, should be all what's left
of your patch, because as far as I see it, most of your changes
are already contained in my fixes patch. What's left is that
you disable the video mode in .disable() and not in
.post_disable().

-michael