Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 13.12.2022 16:15, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski
> <m.szyprowski@xxxxxxxxxxx> wrote:
>> On 13.12.2022 15:18, Jagan Teki wrote:
>>> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
>>> <m.szyprowski@xxxxxxxxxxx> wrote:
>>>> On 13.12.2022 13:20, Marek Szyprowski wrote:
>>>>> On 13.12.2022 11:40, Jagan Teki wrote:
>>>>>> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski
>>>>>> <m.szyprowski@xxxxxxxxxxx> wrote:
>>>>>>> On 12.12.2022 16:33, Jagan Teki wrote:
>>>>>>>
>>>>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
>>>>>>> <m.szyprowski@xxxxxxxxxxx> wrote:
>>>>>>>
>>>>>>> On 12.12.2022 09:43, Marek Szyprowski wrote:
>>>>>>>
>>>>>>> On 12.12.2022 09:32, Jagan Teki wrote:
>>>>>>>
>>>>>>> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski
>>>>>>> <m.szyprowski@xxxxxxxxxxx> wrote:
>>>>>>>
>>>>>>> Hi Jagan,
>>>>>>>
>>>>>>> On 09.12.2022 16:23, Jagan Teki wrote:
>>>>>>>
>>>>>>> The existing drm panels and bridges in Exynos required host
>>>>>>> initialization during the first DSI command transfer even though
>>>>>>> the initialization was done before.
>>>>>>>
>>>>>>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED
>>>>>>> flag and triggers from host transfer.
>>>>>>>
>>>>>>> Do this exclusively for Exynos.
>>>>>>>
>>>>>>> Initial logic is derived from Marek Szyprowski changes.
>>>>>>>
>>>>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
>>>>>>> Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
>>>>>>> ---
>>>>>>> Changes from v9:
>>>>>>> - derived from v8
>>>>>>> - added comments
>>>>>>>
>>>>>>>       drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++-
>>>>>>>       include/drm/bridge/samsung-dsim.h     |  5 +++--
>>>>>>>       2 files changed, 17 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> The following chunk is missing compared to v8:
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> index 6e9ad955ebd3..6a9403cb92ae 100644
>>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim
>>>>>>> *dsi, unsigned int flag)
>>>>>>>                      return 0;
>>>>>>>
>>>>>>>              samsung_dsim_reset(dsi);
>>>>>>> -       samsung_dsim_enable_irq(dsi);
>>>>>>> +
>>>>>>> +       if (!(dsi->state & DSIM_STATE_INITIALIZED))
>>>>>>> +               samsung_dsim_enable_irq(dsi);
>>>>>>>
>>>>>>> Is this really required? does it make sure that the IRQ does not
>>>>>>> enable twice?
>>>>>>>
>>>>>>> That's what that check does. Without the 'if (!(dsi->state &
>>>>>>> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first
>>>>>>> from pre_enable, then from the first transfer), what leads to a
>>>>>>> warning from irq core.
>>>>>>>
>>>>>>> I've just noticed that we also would need to clear the
>>>>>>> DSIM_STATE_REINITIALIZED flag in dsim_suspend.
>>>>>>>
>>>>>>> However I've found that a bit simpler patch would keep the current code
>>>>>>> flow for Exynos instead of this reinitialization hack. This can be
>>>>>>> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host
>>>>>>> init in pre_enable" patch:
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> index 0b2e52585485..acc95c61ae45 100644
>>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>>>> @@ -1291,9 +1291,11 @@ static void
>>>>>>> samsung_dsim_atomic_pre_enable(struct
>>>>>>> drm_bridge *bridge,
>>>>>>>
>>>>>>>             dsi->state |= DSIM_STATE_ENABLED;
>>>>>>>
>>>>>>> -       ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED);
>>>>>>> -       if (ret)
>>>>>>> -               return;
>>>>>>> +       if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
>>>>>>> +               ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED);
>>>>>>> +               if (ret)
>>>>>>> +                       return;
>>>>>>> +       }
>>>>>>>
>>>>>>> Sorry, I don't understand this. Does it mean Exynos doesn't need to
>>>>>>> init host in pre_enable? If I remember correctly even though the host
>>>>>>> is initialized it has to reinitialize during the first transfer - This
>>>>>>> is what the Exynos requirement is. Please correct or explain here.
>>>>>>>
>>>>>>> This is a matter of enabling power regulator(s) in the right order
>>>>>>> and doing the host initialization in the right moment. It was never
>>>>>>> a matter of re-initialization. See the current code for the
>>>>>>> reference (it uses the same approach as my above change). I've
>>>>>>> already explained that here:
>>>>>>>
>>>>>>> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@xxxxxxxxxxx/
>>>>>>>
>>>>>>>
>>>>>>> If you would like to see the exact proper moment of the dsi host
>>>>>>> initialization on the Exynos see the code here:
>>>>>>>
>>>>>>> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework
>>>>>>> and patches adding mipi_dsi_host_init() to panel/bridge drivers.
>>>>>> As I said before, the downstream bridge needs an explicit call to host
>>>>>> init via mipi_dsi_host_init - this is indeed not a usual use-case
>>>>>> scenario. Let's handle this with a REINIT fix and see if we can update
>>>>>> this later to handle both scenarios.
>>>>>>
>>>>>> Would you please test this repo, I have included all.
>>>>>>
>>>>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10
>>>>> This doesn't work on TM2e board. Give me some time to find why...
>>>>>
>>>> The following change is missing in "drm: bridge: Generalize Exynos-DSI
>>>> driver into a Samsung DSIM bridge" patch:
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>> index 1dbff2bee93f..81828b5ee0ac 100644
>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>> @@ -1745,6 +1745,7 @@ int samsung_dsim_probe(struct platform_device *pdev)
>>>>            dsi->bridge.funcs = &samsung_dsim_bridge_funcs;
>>>>            dsi->bridge.of_node = dev->of_node;
>>>>            dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
>>>> +       dsi->bridge.pre_enable_prev_first = true;
>>> Can you check this and confirm, I keep this in exynos side.
>>> https://gitlab.com/openedev/kernel/-/commit/ccb02df7a313fdf91d8e116b0ec3d6c945fbb6fd#c93f0ce4d81b854fbde970e341fb307f1be78c16_1865_189
>> This one is fine!
>>
>>>>            /* DE_LOW: i.MX8M Mini/Nano LCDIF-DSIM glue logic inverts
>>>> HS/VS/DE */
>>>>            if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM)
>>>>
>>>>
>>>> After adding the above, all my test platform works fine.
>>>>
>>>> BTW, I still think that it is worth replacing the "drm: exynos: dsi: Add
>>>> host initialization in pre_enable" patch with the following simple
>>>> change and propagate it to bridge/samsung-dsim.c:
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> index fdaf514b39f2..071b74d60dcb 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
>>>> @@ -254,6 +254,9 @@ struct exynos_dsi_transfer {
>>>>     #define DSIM_STATE_CMD_LPM             BIT(2)
>>>>     #define DSIM_STATE_VIDOUT_AVAILABLE    BIT(3)
>>>>
>>>> +#define exynos_dsi_hw_is_exynos(hw) \
>>>> +       ((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433)
>>>> +
>>>>     enum exynos_dsi_type {
>>>>            DSIM_TYPE_EXYNOS3250,
>>>>            DSIM_TYPE_EXYNOS4210,
>>>> @@ -1344,6 +1347,9 @@ static int exynos_dsi_init(struct exynos_dsi *dsi)
>>>>     {
>>>>            const struct exynos_dsi_driver_data *driver_data =
>>>> dsi->driver_data;
>>>>
>>>> +       if (dsi->state & DSIM_STATE_INITIALIZED)
>>>> +               return 0;
>>>> +
>>>>            exynos_dsi_reset(dsi);
>>>>            exynos_dsi_enable_irq(dsi);
>>>>
>>>> @@ -1356,6 +1362,8 @@ static int exynos_dsi_init(struct exynos_dsi *dsi)
>>>>            exynos_dsi_set_phy_ctrl(dsi);
>>>>            exynos_dsi_init_link(dsi);
>>>>
>>>> +       dsi->state |= DSIM_STATE_INITIALIZED;
>>>> +
>>>>            return 0;
>>>>     }
>>>>
>>>> @@ -1411,6 +1419,12 @@ static void exynos_dsi_atomic_pre_enable(struct
>>>> drm_bridge *bridge,
>>>>            }
>>>>
>>>>            dsi->state |= DSIM_STATE_ENABLED;
>>>> +
>>>> +       if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type)) {
>>>> +               ret = exynos_dsi_init(dsi);
>>>> +               if (ret)
>>>> +                       return;
>>>> +       }
>>>>     }
>>>>
>>>>     static void exynos_dsi_atomic_enable(struct drm_bridge *bridge,
>>>> @@ -1557,12 +1571,9 @@ static ssize_t exynos_dsi_host_transfer(struct
>>>> mipi_dsi_host *host,
>>>>            if (!(dsi->state & DSIM_STATE_ENABLED))
>>>>                    return -EINVAL;
>>>>
>>>> -       if (!(dsi->state & DSIM_STATE_INITIALIZED)) {
>>>> -               ret = exynos_dsi_init(dsi);
>>>> -               if (ret)
>>>> -                       return ret;
>>>> -               dsi->state |= DSIM_STATE_INITIALIZED;
>>>> -       }
>>>> +       ret = exynos_dsi_init(dsi);
>>>> +       if (ret)
>>>> +               return ret;
>>> Below patch handling similar behavior by checking exynos hw_type at
>>> exynos_dsi_init, isn't it? Please check and let me know if I missing
>>> anything.
>>>
>>> https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5
>> You don't miss anything. Your version also works, but I just proposed a
>> bit simpler code.
> Do your changes don't need a DSIM_STATE_REINITIALIZED flag? would you
> please share the change on top of this commit?
> https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5

It doesn't need the DSIM_STATE_REINITIALIZED flag because the 
initialization is done only once - in pre_enable for non-Exynos case and 
on the first transfer for the Exynos case. In both cases the same flag 
(DSIM_STATE_INITIALIZED) is used.

See the attached patch.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland
From ba5fe3c792a2b0c232bc38f52a0b61a89512460f Mon Sep 17 00:00:00 2001
From: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
Date: Tue, 13 Dec 2022 16:40:13 +0100
Subject: [PATCH] fixup! drm: exynos: dsi: Handle proper host initialization

Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 39 ++++++++-----------------
 1 file changed, 12 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
index 0657f6655bc6..071b74d60dcb 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
@@ -251,9 +251,8 @@ struct exynos_dsi_transfer {
 
 #define DSIM_STATE_ENABLED		BIT(0)
 #define DSIM_STATE_INITIALIZED		BIT(1)
-#define DSIM_STATE_REINITIALIZED	BIT(2)
-#define DSIM_STATE_CMD_LPM		BIT(3)
-#define DSIM_STATE_VIDOUT_AVAILABLE	BIT(4)
+#define DSIM_STATE_CMD_LPM		BIT(2)
+#define DSIM_STATE_VIDOUT_AVAILABLE	BIT(3)
 
 #define exynos_dsi_hw_is_exynos(hw) \
 	((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433)
@@ -1344,30 +1343,15 @@ static void exynos_dsi_disable_irq(struct exynos_dsi *dsi)
 	disable_irq(dsi->irq);
 }
 
-static int exynos_dsi_init(struct exynos_dsi *dsi, unsigned int flag)
+static int exynos_dsi_init(struct exynos_dsi *dsi)
 {
 	const struct exynos_dsi_driver_data *driver_data = dsi->driver_data;
 
-	/*
-	 * FIXME:
-	 * The existing drm panels and bridges in Exynos required host
-	 * initialization during the first DSI command transfer even though
-	 * the initialization was done before.
-	 *
-	 * This host reinitialization is handled via DSIM_STATE_REINITIALIZED
-	 * flag and triggers from host transfer. Do this exclusively for Exynos.
-	 */
-	if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type) &&
-	    dsi->state & DSIM_STATE_REINITIALIZED)
-		return 0;
-
-	if (dsi->state & flag)
+	if (dsi->state & DSIM_STATE_INITIALIZED)
 		return 0;
 
 	exynos_dsi_reset(dsi);
-
-	if (!(dsi->state & DSIM_STATE_INITIALIZED))
-		exynos_dsi_enable_irq(dsi);
+	exynos_dsi_enable_irq(dsi);
 
 	if (driver_data->reg_values[RESET_TYPE] == DSIM_FUNCRST)
 		exynos_dsi_enable_lane(dsi, BIT(dsi->lanes) - 1);
@@ -1378,7 +1362,7 @@ static int exynos_dsi_init(struct exynos_dsi *dsi, unsigned int flag)
 	exynos_dsi_set_phy_ctrl(dsi);
 	exynos_dsi_init_link(dsi);
 
-	dsi->state |= flag;
+	dsi->state |= DSIM_STATE_INITIALIZED;
 
 	return 0;
 }
@@ -1436,9 +1420,11 @@ static void exynos_dsi_atomic_pre_enable(struct drm_bridge *bridge,
 
 	dsi->state |= DSIM_STATE_ENABLED;
 
-	ret = exynos_dsi_init(dsi, DSIM_STATE_INITIALIZED);
-	if (ret)
-		return;
+	if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type)) {
+		ret = exynos_dsi_init(dsi);
+		if (ret)
+			return;
+	}
 }
 
 static void exynos_dsi_atomic_enable(struct drm_bridge *bridge,
@@ -1585,7 +1571,7 @@ static ssize_t exynos_dsi_host_transfer(struct mipi_dsi_host *host,
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return -EINVAL;
 
-	ret = exynos_dsi_init(dsi, DSIM_STATE_REINITIALIZED);
+	ret = exynos_dsi_init(dsi);
 	if (ret)
 		return ret;
 
@@ -1791,7 +1777,6 @@ static int __maybe_unused exynos_dsi_suspend(struct device *dev)
 
 	if (dsi->state & DSIM_STATE_INITIALIZED) {
 		dsi->state &= ~DSIM_STATE_INITIALIZED;
-		dsi->state &= ~DSIM_STATE_REINITIALIZED;
 
 		exynos_dsi_disable_clock(dsi);
 
-- 
2.38.1


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux