[v2,05/12] drm: bridge: samsung-dsim: Add DSI init in bridge pre_enable()

Message ID 20220504114021.33265-6-jagan@amarulasolutions.com
State New
Headers show
Series
  • drm: bridge: Add Samsung MIPI DSIM bridge
Related show

Commit Message

Jagan Teki May 4, 2022, 11:40 a.m. UTC
Host transfer() in DSI master will invoke only when the DSI commands
are sent from DSI devices like DSI Panel or DSI bridges and this
host transfer wouldn't invoke for I2C-based-DSI bridge drivers.

Handling DSI host initialization in transfer calls misses the
controller setup for I2C configured DSI bridges.

This patch adds the DSI initialization from transfer to bridge
pre_enable as the bridge pre_enable API is invoked by core as
it is common across all classes of DSI device drivers.

v2:
* check initialized state in samsung_dsim_init

v1:
* keep DSI init in host transfer

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Marek Szyprowski May 11, 2022, 3:02 p.m. UTC | #1
On 04.05.2022 13:40, Jagan Teki wrote:
> Host transfer() in DSI master will invoke only when the DSI commands
> are sent from DSI devices like DSI Panel or DSI bridges and this
> host transfer wouldn't invoke for I2C-based-DSI bridge drivers.
>
> Handling DSI host initialization in transfer calls misses the
> controller setup for I2C configured DSI bridges.
>
> This patch adds the DSI initialization from transfer to bridge
> pre_enable as the bridge pre_enable API is invoked by core as
> it is common across all classes of DSI device drivers.
>
> v2:
> * check initialized state in samsung_dsim_init
>
> v1:
> * keep DSI init in host transfer
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>

This doesn't work with Exynos DSI and DSI panels. Here is a bit more 
detailed explanation and my solution for this:

https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/


> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 60dc863113a0..b9361af5ef2d 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1259,6 +1259,9 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
>   {
>   	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
>   
> +	if (dsi->state & DSIM_STATE_INITIALIZED)
> +		return 0;
> +
>   	samsung_dsim_reset(dsi);
>   	samsung_dsim_enable_irq(dsi);
>   
> @@ -1271,6 +1274,8 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
>   	samsung_dsim_set_phy_ctrl(dsi);
>   	samsung_dsim_init_link(dsi);
>   
> +	dsi->state |= DSIM_STATE_INITIALIZED;
> +
>   	return 0;
>   }
>   
> @@ -1290,6 +1295,10 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>   	}
>   
>   	dsi->state |= DSIM_STATE_ENABLED;
> +
> +	ret = samsung_dsim_init(dsi);
> +	if (ret)
> +		return;
>   }
>   
>   static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
> @@ -1464,12 +1473,9 @@ static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
>   	if (!(dsi->state & DSIM_STATE_ENABLED))
>   		return -EINVAL;
>   
> -	if (!(dsi->state & DSIM_STATE_INITIALIZED)) {
> -		ret = samsung_dsim_init(dsi);
> -		if (ret)
> -			return ret;
> -		dsi->state |= DSIM_STATE_INITIALIZED;
> -	}
> +	ret = samsung_dsim_init(dsi);
> +	if (ret)
> +		return ret;
>   
>   	ret = mipi_dsi_create_packet(&xfer.packet, msg);
>   	if (ret < 0)

Best regards
Marek Szyprowski May 18, 2022, 2:15 p.m. UTC | #2
On 11.05.2022 17:02, Marek Szyprowski wrote:
> On 04.05.2022 13:40, Jagan Teki wrote:
>> Host transfer() in DSI master will invoke only when the DSI commands
>> are sent from DSI devices like DSI Panel or DSI bridges and this
>> host transfer wouldn't invoke for I2C-based-DSI bridge drivers.
>>
>> Handling DSI host initialization in transfer calls misses the
>> controller setup for I2C configured DSI bridges.
>>
>> This patch adds the DSI initialization from transfer to bridge
>> pre_enable as the bridge pre_enable API is invoked by core as
>> it is common across all classes of DSI device drivers.
>>
>> v2:
>> * check initialized state in samsung_dsim_init
>>
>> v1:
>> * keep DSI init in host transfer
>>
>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>
> This doesn't work with Exynos DSI and DSI panels. Here is a bit more 
> detailed explanation and my solution for this:
>
> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/ 
>


I wonder if your modification works on i.MX8MM with a pure DSI-based 
panel/bridge. In your tree I only see that you have tested it with the 
i2c-controlled DSI-to-LVDS converter.


After the comments from the above mentioned thread I've reworked the 
initialization again. It looks that the ultimate solution for both 
worlds is to call samsung_dsim_init() twice, once in pre_enable, then 
before the first host_transfer, see 
https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework-v2

This way at least it works fine on all my Exynos based test boards.

>> ---
>>   drivers/gpu/drm/bridge/samsung-dsim.c | 18 ++++++++++++------
>>   1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c 
>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>> index 60dc863113a0..b9361af5ef2d 100644
>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>> @@ -1259,6 +1259,9 @@ static int samsung_dsim_init(struct 
>> samsung_dsim *dsi)
>>   {
>>       const struct samsung_dsim_driver_data *driver_data = 
>> dsi->driver_data;
>>   +    if (dsi->state & DSIM_STATE_INITIALIZED)
>> +        return 0;
>> +
>>       samsung_dsim_reset(dsi);
>>       samsung_dsim_enable_irq(dsi);
>>   @@ -1271,6 +1274,8 @@ static int samsung_dsim_init(struct 
>> samsung_dsim *dsi)
>>       samsung_dsim_set_phy_ctrl(dsi);
>>       samsung_dsim_init_link(dsi);
>>   +    dsi->state |= DSIM_STATE_INITIALIZED;
>> +
>>       return 0;
>>   }
>>   @@ -1290,6 +1295,10 @@ static void 
>> samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>>       }
>>         dsi->state |= DSIM_STATE_ENABLED;
>> +
>> +    ret = samsung_dsim_init(dsi);
>> +    if (ret)
>> +        return;
>>   }
>>     static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
>> @@ -1464,12 +1473,9 @@ static ssize_t 
>> samsung_dsim_host_transfer(struct mipi_dsi_host *host,
>>       if (!(dsi->state & DSIM_STATE_ENABLED))
>>           return -EINVAL;
>>   -    if (!(dsi->state & DSIM_STATE_INITIALIZED)) {
>> -        ret = samsung_dsim_init(dsi);
>> -        if (ret)
>> -            return ret;
>> -        dsi->state |= DSIM_STATE_INITIALIZED;
>> -    }
>> +    ret = samsung_dsim_init(dsi);
>> +    if (ret)
>> +        return ret;
>>         ret = mipi_dsi_create_packet(&xfer.packet, msg);
>>       if (ret < 0)
>
> Best regards

Best regards

Patch

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 60dc863113a0..b9361af5ef2d 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1259,6 +1259,9 @@  static int samsung_dsim_init(struct samsung_dsim *dsi)
 {
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 
+	if (dsi->state & DSIM_STATE_INITIALIZED)
+		return 0;
+
 	samsung_dsim_reset(dsi);
 	samsung_dsim_enable_irq(dsi);
 
@@ -1271,6 +1274,8 @@  static int samsung_dsim_init(struct samsung_dsim *dsi)
 	samsung_dsim_set_phy_ctrl(dsi);
 	samsung_dsim_init_link(dsi);
 
+	dsi->state |= DSIM_STATE_INITIALIZED;
+
 	return 0;
 }
 
@@ -1290,6 +1295,10 @@  static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
 	}
 
 	dsi->state |= DSIM_STATE_ENABLED;
+
+	ret = samsung_dsim_init(dsi);
+	if (ret)
+		return;
 }
 
 static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
@@ -1464,12 +1473,9 @@  static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return -EINVAL;
 
-	if (!(dsi->state & DSIM_STATE_INITIALIZED)) {
-		ret = samsung_dsim_init(dsi);
-		if (ret)
-			return ret;
-		dsi->state |= DSIM_STATE_INITIALIZED;
-	}
+	ret = samsung_dsim_init(dsi);
+	if (ret)
+		return ret;
 
 	ret = mipi_dsi_create_packet(&xfer.packet, msg);
 	if (ret < 0)