diff mbox series

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

Message ID 20221209152343.180139-11-jagan@amarulasolutions.com
State New
Headers
Series drm: bridge: Add Samsung MIPI DSIM bridge |

Commit Message

Jagan Teki Dec. 9, 2022, 3:23 p.m. UTC 
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@samsung.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
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(-)

Comments

Marek Szyprowski Dec. 12, 2022, 8:26 a.m. UTC | #1 
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@samsung.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> 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);

         if (driver_data->reg_values[RESET_TYPE] == DSIM_FUNCRST)
                 samsung_dsim_enable_lane(dsi, BIT(dsi->lanes) - 1);


> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 2e15d753fdd0..ec3ab679afd9 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1254,6 +1254,19 @@ static int samsung_dsim_init(struct samsung_dsim *dsi, unsigned int flag)
>   {
>   	const struct samsung_dsim_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 ((dsi->plat_data->hw_type == SAMSUNG_DSIM_TYPE_IMX8MM) &&
> +	    dsi->state & DSIM_STATE_REINITIALIZED)
> +		return 0;
> +
>   	if (dsi->state & flag)
>   		return 0;
>   
> @@ -1467,7 +1480,7 @@ static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
>   	if (!(dsi->state & DSIM_STATE_ENABLED))
>   		return -EINVAL;
>   
> -	ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED);
> +	ret = samsung_dsim_init(dsi, DSIM_STATE_REINITIALIZED);
>   	if (ret)
>   		return ret;
>   
> diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
> index b8132bf8e36f..0c5a905f3de7 100644
> --- a/include/drm/bridge/samsung-dsim.h
> +++ b/include/drm/bridge/samsung-dsim.h
> @@ -17,8 +17,9 @@ struct samsung_dsim;
>   
>   #define DSIM_STATE_ENABLED		BIT(0)
>   #define DSIM_STATE_INITIALIZED		BIT(1)
> -#define DSIM_STATE_CMD_LPM		BIT(2)
> -#define DSIM_STATE_VIDOUT_AVAILABLE	BIT(3)
> +#define DSIM_STATE_REINITIALIZED	BIT(2)
> +#define DSIM_STATE_CMD_LPM		BIT(3)
> +#define DSIM_STATE_VIDOUT_AVAILABLE	BIT(4)
>   
>   enum samsung_dsim_type {
>   	SAMSUNG_DSIM_TYPE_EXYNOS3250,

Best regards
Jagan Teki Dec. 12, 2022, 8:32 a.m. UTC | #2 
On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski
<m.szyprowski@samsung.com> 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@samsung.com>
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> > 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?

Jagan.
Marek Szyprowski Dec. 12, 2022, 8:43 a.m. UTC | #3 
On 12.12.2022 09:32, Jagan Teki wrote:
> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski
> <m.szyprowski@samsung.com> 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@samsung.com>
>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>> ---
>>> 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.

Best regards
Marek Szyprowski Dec. 12, 2022, 3:21 p.m. UTC | #4 
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@samsung.com> 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@samsung.com>
>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>> ---
>>>> 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
Jagan Teki Dec. 12, 2022, 3:33 p.m. UTC | #5 
On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
<m.szyprowski@samsung.com> 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@samsung.com> 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@samsung.com>
> >>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >>>> ---
> >>>> 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.

Jagan.
Marek Szyprowski Dec. 13, 2022, 8:58 a.m. UTC | #6 
On 12.12.2022 16:33, Jagan Teki wrote:
> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> <m.szyprowski@samsung.com>  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@samsung.com>  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@samsung.com>
>>>>>> Signed-off-by: Jagan Teki<jagan@amarulasolutions.com>
>>>>>> ---
>>>>>> 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@samsung.com/

If you would like to see the exact proper moment of the dsi host 
initialization on the Exynos see the code here:

https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework  and patches addingmipi_dsi_host_init() to panel/bridge drivers.

Best regards
Jagan Teki Dec. 13, 2022, 10:40 a.m. UTC | #7 
Hi Marek,

On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 12.12.2022 16:33, Jagan Teki wrote:
>
> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> <m.szyprowski@samsung.com> 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@samsung.com> 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@samsung.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> 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@samsung.com/
>
> If you would like to see the exact proper moment of the dsi host initialization on the Exynos see the code here:
>
> https://github.com/mszyprow/linux/tree/v5.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

Thanks,
Jagan.
Fabio Estevam Dec. 13, 2022, 10:47 a.m. UTC | #8 
Hi Jagan,

On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote:

> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10

Please preserve the authorship of the patches.

This one is from Marek Vasut:
https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680

but in your tree, it appears as if you were the original author.

Please double-check globally.
Jagan Teki Dec. 13, 2022, 10:53 a.m. UTC | #9 
Hi Fabio,

On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> Hi Jagan,
>
> On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> > https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10
>
> Please preserve the authorship of the patches.
>
> This one is from Marek Vasut:
> https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680

The original patch was changed with respect to this one and that is
the reason I have to keep his signed-off-by.

Jagan.
Marek Szyprowski Dec. 13, 2022, 12:20 p.m. UTC | #10 
Hi,

On 13.12.2022 11:40, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 12.12.2022 16:33, Jagan Teki wrote:
>>
>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
>> <m.szyprowski@samsung.com> 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@samsung.com> 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@samsung.com>
>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>> ---
>> 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@samsung.com/
>>
>> 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...

Best regards
Marek Vasut Dec. 13, 2022, 1:14 p.m. UTC | #11 
On 12/13/22 11:53, Jagan Teki wrote:
> Hi Fabio,

Hi,

> On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote:
>>
>> Hi Jagan,
>>
>> On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>>
>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10
>>
>> Please preserve the authorship of the patches.
>>
>> This one is from Marek Vasut:
>> https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680
> 
> The original patch was changed with respect to this one and that is
> the reason I have to keep his signed-off-by.

You did change the authorship of the patch, not just a SoB line.
It seems that the only change is dropped comment, which was squashed 
into earlier patch in this series, see the original submission:

https://patchwork.freedesktop.org/patch/507166/

btw. it seems hunk 3 has disappeared, the samsung_dsim_attach() hw_type 
check.
Jagan Teki Dec. 13, 2022, 1:18 p.m. UTC | #12 
On Tue, Dec 13, 2022 at 6:44 PM Marek Vasut <marex@denx.de> wrote:
>
> On 12/13/22 11:53, Jagan Teki wrote:
> > Hi Fabio,
>
> Hi,
>
> > On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote:
> >>
> >> Hi Jagan,
> >>
> >> On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >>
> >>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10
> >>
> >> Please preserve the authorship of the patches.
> >>
> >> This one is from Marek Vasut:
> >> https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680
> >
> > The original patch was changed with respect to this one and that is
> > the reason I have to keep his signed-off-by.
>
> You did change the authorship of the patch, not just a SoB line.
> It seems that the only change is dropped comment, which was squashed
> into earlier patch in this series, see the original submission:

OKay. I will update it on V10 or if you want to send it from your side
then I will exclude it from the series. let me know.

>
> https://patchwork.freedesktop.org/patch/507166/
>
> btw. it seems hunk 3 has disappeared, the samsung_dsim_attach() hw_type
> check.

Do you mean previous = NULL; addition?

Jagan.
Marek Vasut Dec. 13, 2022, 1:21 p.m. UTC | #13 
On 12/13/22 14:18, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 6:44 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 12/13/22 11:53, Jagan Teki wrote:
>>> Hi Fabio,
>>
>> Hi,
>>
>>> On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote:
>>>>
>>>> Hi Jagan,
>>>>
>>>> On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>>>>
>>>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10
>>>>
>>>> Please preserve the authorship of the patches.
>>>>
>>>> This one is from Marek Vasut:
>>>> https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680
>>>
>>> The original patch was changed with respect to this one and that is
>>> the reason I have to keep his signed-off-by.
>>
>> You did change the authorship of the patch, not just a SoB line.
>> It seems that the only change is dropped comment, which was squashed
>> into earlier patch in this series, see the original submission:
> 
> OKay. I will update it on V10 or if you want to send it from your side
> then I will exclude it from the series. let me know.

Just keep the authorship intact, unless there is significant change to 
the patch.

>> https://patchwork.freedesktop.org/patch/507166/
>>
>> btw. it seems hunk 3 has disappeared, the samsung_dsim_attach() hw_type
>> check.
> 
> Do you mean previous = NULL; addition?

Yes, this hunk has been dropped.
Jagan Teki Dec. 13, 2022, 1:26 p.m. UTC | #14 
On Tue, Dec 13, 2022 at 6:51 PM Marek Vasut <marex@denx.de> wrote:
>
> On 12/13/22 14:18, Jagan Teki wrote:
> > On Tue, Dec 13, 2022 at 6:44 PM Marek Vasut <marex@denx.de> wrote:
> >>
> >> On 12/13/22 11:53, Jagan Teki wrote:
> >>> Hi Fabio,
> >>
> >> Hi,
> >>
> >>> On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote:
> >>>>
> >>>> Hi Jagan,
> >>>>
> >>>> On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >>>>
> >>>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10
> >>>>
> >>>> Please preserve the authorship of the patches.
> >>>>
> >>>> This one is from Marek Vasut:
> >>>> https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680
> >>>
> >>> The original patch was changed with respect to this one and that is
> >>> the reason I have to keep his signed-off-by.
> >>
> >> You did change the authorship of the patch, not just a SoB line.
> >> It seems that the only change is dropped comment, which was squashed
> >> into earlier patch in this series, see the original submission:
> >
> > OKay. I will update it on V10 or if you want to send it from your side
> > then I will exclude it from the series. let me know.
>
> Just keep the authorship intact, unless there is significant change to
> the patch.

Please confirm it.
https://gitlab.com/openedev/kernel/-/commit/8ce066d7fdf45e17cb1979376e70e6be353e001b

>
> >> https://patchwork.freedesktop.org/patch/507166/
> >>
> >> btw. it seems hunk 3 has disappeared, the samsung_dsim_attach() hw_type
> >> check.
> >
> > Do you mean previous = NULL; addition?
>
> Yes, this hunk has been dropped.

Yes this FIXME has dropped due to Dave's changes.

Jagan.
Marek Vasut Dec. 13, 2022, 1:29 p.m. UTC | #15 
On 12/13/22 14:26, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 6:51 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 12/13/22 14:18, Jagan Teki wrote:
>>> On Tue, Dec 13, 2022 at 6:44 PM Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 12/13/22 11:53, Jagan Teki wrote:
>>>>> Hi Fabio,
>>>>
>>>> Hi,
>>>>
>>>>> On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote:
>>>>>>
>>>>>> Hi Jagan,
>>>>>>
>>>>>> On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
>>>>>>
>>>>>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10
>>>>>>
>>>>>> Please preserve the authorship of the patches.
>>>>>>
>>>>>> This one is from Marek Vasut:
>>>>>> https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680
>>>>>
>>>>> The original patch was changed with respect to this one and that is
>>>>> the reason I have to keep his signed-off-by.
>>>>
>>>> You did change the authorship of the patch, not just a SoB line.
>>>> It seems that the only change is dropped comment, which was squashed
>>>> into earlier patch in this series, see the original submission:
>>>
>>> OKay. I will update it on V10 or if you want to send it from your side
>>> then I will exclude it from the series. let me know.
>>
>> Just keep the authorship intact, unless there is significant change to
>> the patch.
> 
> Please confirm it.
> https://gitlab.com/openedev/kernel/-/commit/8ce066d7fdf45e17cb1979376e70e6be353e001b

Seems OK. thanks

>>>> https://patchwork.freedesktop.org/patch/507166/
>>>>
>>>> btw. it seems hunk 3 has disappeared, the samsung_dsim_attach() hw_type
>>>> check.
>>>
>>> Do you mean previous = NULL; addition?
>>
>> Yes, this hunk has been dropped.
> 
> Yes this FIXME has dropped due to Dave's changes.

OK
Jagan Teki Dec. 13, 2022, 1:31 p.m. UTC | #16 
On Tue, Dec 13, 2022 at 5:50 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi,
>
> On 13.12.2022 11:40, Jagan Teki wrote:
> > On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 12.12.2022 16:33, Jagan Teki wrote:
> >>
> >> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> >> <m.szyprowski@samsung.com> 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@samsung.com> 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@samsung.com>
> >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >> ---
> >> 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@samsung.com/
> >>
> >> 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...

May be some mode_flags changes in the panel driver.

Jagan.
Marek Szyprowski Dec. 13, 2022, 2:01 p.m. UTC | #17 
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@samsung.com> wrote:
>>> On 12.12.2022 16:33, Jagan Teki wrote:
>>>
>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
>>> <m.szyprowski@samsung.com> 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@samsung.com> 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@samsung.com>
>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>> ---
>>> 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@samsung.com/ 
>>>
>>>
>>> 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
Jagan Teki Dec. 13, 2022, 2:18 p.m. UTC | #18 
On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
<m.szyprowski@samsung.com> 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@samsung.com> wrote:
> >>> On 12.12.2022 16:33, Jagan Teki wrote:
> >>>
> >>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> >>> <m.szyprowski@samsung.com> 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@samsung.com> 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@samsung.com>
> >>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >>> ---
> >>> 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@samsung.com/
> >>>
> >>>
> >>> 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

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

Jagan.
Marek Szyprowski Dec. 13, 2022, 2:54 p.m. UTC | #19 
On 13.12.2022 15:18, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
> <m.szyprowski@samsung.com> 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@samsung.com> wrote:
>>>>> On 12.12.2022 16:33, Jagan Teki wrote:
>>>>>
>>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
>>>>> <m.szyprowski@samsung.com> 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@samsung.com> 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@samsung.com>
>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>> ---
>>>>> 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@samsung.com/
>>>>>
>>>>>
>>>>> 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.


Best regards
Jagan Teki Dec. 13, 2022, 3:15 p.m. UTC | #20 
On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 13.12.2022 15:18, Jagan Teki wrote:
> > On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> 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@samsung.com> wrote:
> >>>>> On 12.12.2022 16:33, Jagan Teki wrote:
> >>>>>
> >>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> >>>>> <m.szyprowski@samsung.com> 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@samsung.com> 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@samsung.com>
> >>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >>>>> ---
> >>>>> 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@samsung.com/
> >>>>>
> >>>>>
> >>>>> 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

Jagan.
Marek Szyprowski Dec. 13, 2022, 3:41 p.m. UTC | #21 
On 13.12.2022 16:15, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 13.12.2022 15:18, Jagan Teki wrote:
>>> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
>>> <m.szyprowski@samsung.com> 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@samsung.com> wrote:
>>>>>>> On 12.12.2022 16:33, Jagan Teki wrote:
>>>>>>>
>>>>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
>>>>>>> <m.szyprowski@samsung.com> 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@samsung.com> 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@samsung.com>
>>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>>>> ---
>>>>>>> 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@samsung.com/
>>>>>>>
>>>>>>>
>>>>>>> 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
Jagan Teki Dec. 14, 2022, 5:33 a.m. UTC | #22 
On Tue, Dec 13, 2022 at 9:11 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 13.12.2022 16:15, Jagan Teki wrote:
> > On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 13.12.2022 15:18, Jagan Teki wrote:
> >>> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
> >>> <m.szyprowski@samsung.com> 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@samsung.com> wrote:
> >>>>>>> On 12.12.2022 16:33, Jagan Teki wrote:
> >>>>>>>
> >>>>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> >>>>>>> <m.szyprowski@samsung.com> 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@samsung.com> 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@samsung.com>
> >>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >>>>>>> ---
> >>>>>>> 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@samsung.com/
> >>>>>>>
> >>>>>>>
> >>>>>>> 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.

Thanks, I have included the changes and added your authorship as well.

Please test this final version and let me know if you have any comments.
https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10

Jagan.
Marek Szyprowski Dec. 14, 2022, 8:04 a.m. UTC | #23 
On 14.12.2022 06:33, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 9:11 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 13.12.2022 16:15, Jagan Teki wrote:
>>> On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski
>>> <m.szyprowski@samsung.com> wrote:
>>>> On 13.12.2022 15:18, Jagan Teki wrote:
>>>>> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
>>>>> <m.szyprowski@samsung.com> 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@samsung.com> wrote:
>>>>>>>>> On 12.12.2022 16:33, Jagan Teki wrote:
>>>>>>>>>
>>>>>>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
>>>>>>>>> <m.szyprowski@samsung.com> 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@samsung.com> 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@samsung.com>
>>>>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>>>>>> ---
>>>>>>>>> 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@samsung.com/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 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.
> Thanks, I have included the changes and added your authorship as well.
>
> Please test this final version and let me know if you have any comments.
> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10


Fine for me.


Best regards
Jagan Teki Dec. 14, 2022, 8:23 a.m. UTC | #24 
On Wed, Dec 14, 2022 at 1:34 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 14.12.2022 06:33, Jagan Teki wrote:
> > On Tue, Dec 13, 2022 at 9:11 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 13.12.2022 16:15, Jagan Teki wrote:
> >>> On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski
> >>> <m.szyprowski@samsung.com> wrote:
> >>>> On 13.12.2022 15:18, Jagan Teki wrote:
> >>>>> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
> >>>>> <m.szyprowski@samsung.com> 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@samsung.com> wrote:
> >>>>>>>>> On 12.12.2022 16:33, Jagan Teki wrote:
> >>>>>>>>>
> >>>>>>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> >>>>>>>>> <m.szyprowski@samsung.com> 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@samsung.com> 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@samsung.com>
> >>>>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >>>>>>>>> ---
> >>>>>>>>> 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@samsung.com/
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> 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.
> > Thanks, I have included the changes and added your authorship as well.
> >
> > Please test this final version and let me know if you have any comments.
> > https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10
>
>
> Fine for me.

Thanks, I will send V10.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 2e15d753fdd0..ec3ab679afd9 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1254,6 +1254,19 @@  static int samsung_dsim_init(struct samsung_dsim *dsi, unsigned int flag)
 {
 	const struct samsung_dsim_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 ((dsi->plat_data->hw_type == SAMSUNG_DSIM_TYPE_IMX8MM) &&
+	    dsi->state & DSIM_STATE_REINITIALIZED)
+		return 0;
+
 	if (dsi->state & flag)
 		return 0;
 
@@ -1467,7 +1480,7 @@  static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return -EINVAL;
 
-	ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED);
+	ret = samsung_dsim_init(dsi, DSIM_STATE_REINITIALIZED);
 	if (ret)
 		return ret;
 
diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h
index b8132bf8e36f..0c5a905f3de7 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -17,8 +17,9 @@  struct samsung_dsim;
 
 #define DSIM_STATE_ENABLED		BIT(0)
 #define DSIM_STATE_INITIALIZED		BIT(1)
-#define DSIM_STATE_CMD_LPM		BIT(2)
-#define DSIM_STATE_VIDOUT_AVAILABLE	BIT(3)
+#define DSIM_STATE_REINITIALIZED	BIT(2)
+#define DSIM_STATE_CMD_LPM		BIT(3)
+#define DSIM_STATE_VIDOUT_AVAILABLE	BIT(4)
 
 enum samsung_dsim_type {
 	SAMSUNG_DSIM_TYPE_EXYNOS3250,