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

[Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>]

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

  static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
diff --git a/include/drm/bridge/samsung-dsim.h 
b/include/drm/bridge/samsung-dsim.h
index b8132bf8e36f..b4e26de88b9e 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -30,6 +30,9 @@ enum samsung_dsim_type {
         SAMSUNG_DSIM_TYPE_COUNT,
  };

+#define samsung_dsim_hw_is_exynos(hw) ((hw) >= 
SAMSUNG_DSIM_TYPE_EXYNOS3250 && \
+       (hw) <= SAMSUNG_DSIM_TYPE_EXYNOS5433)
+
  struct samsung_dsim_transfer {
         struct list_head list;
         struct completion completed;



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