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 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;

         /* 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;

         ret = mipi_dsi_create_packet(&xfer.packet, msg);
         if (ret < 0)


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland




[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