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
