Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On Wed, Dec 14, 2022 at 1:34 PM Marek Szyprowski
wrote:
>
> On 14.12.2022 06:33, Jagan Teki wrote:
> > On Tue, Dec 13, 2022 at 9:11 PM Marek Szyprowski
> > wrote:
> >> On 13.12.2022 16:15, Jagan Teki wrote:
> >>> On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski
> >>> wrote:
> On 13.12.2022 15:18, Jagan Teki wrote:
> > On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
> > 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
> wrote:
> > On 12.12.2022 16:33, Jagan Teki wrote:
> >
> > On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> > 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
> > 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
> > Signed-off-by: Jagan Teki
> > ---
> > 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 ho
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On 14.12.2022 06:33, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 9:11 PM Marek Szyprowski
> wrote:
>> On 13.12.2022 16:15, Jagan Teki wrote:
>>> On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski
>>> wrote:
On 13.12.2022 15:18, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
> 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
wrote:
> On 12.12.2022 16:33, Jagan Teki wrote:
>
> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> 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
> 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
> Signed-off-by: Jagan Teki
> ---
> 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-9f9d179b5...@samsung.com/
>
>>
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On Tue, Dec 13, 2022 at 9:11 PM Marek Szyprowski
wrote:
>
> On 13.12.2022 16:15, Jagan Teki wrote:
> > On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski
> > wrote:
> >> On 13.12.2022 15:18, Jagan Teki wrote:
> >>> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
> >>> 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
> >> wrote:
> >>> On 12.12.2022 16:33, Jagan Teki wrote:
> >>>
> >>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> >>> 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
> >>> 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
> >>> Signed-off-by: Jagan Teki
> >>> ---
> >>> 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-9f9d179b5...@samsung.com/
> >>>
> >>>
> >>> If you would like to see the exact
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On 13.12.2022 16:15, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski
> wrote:
>> On 13.12.2022 15:18, Jagan Teki wrote:
>>> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
>>> 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
>> wrote:
>>> On 12.12.2022 16:33, Jagan Teki wrote:
>>>
>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
>>> 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
>>> 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
>>> Signed-off-by: Jagan Teki
>>> ---
>>> 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-9f9d179b5...@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-
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski
wrote:
>
> On 13.12.2022 15:18, Jagan Teki wrote:
> > On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
> > 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
> wrote:
> > On 12.12.2022 16:33, Jagan Teki wrote:
> >
> > On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> > 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
> > 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
> > Signed-off-by: Jagan Teki
> > ---
> > 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-9f9d179b5...@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 add
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On 13.12.2022 15:18, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
> 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
wrote:
> On 12.12.2022 16:33, Jagan Teki wrote:
>
> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> 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
> 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
> Signed-off-by: Jagan Teki
> ---
> 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-9f9d179b5...@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 h
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski
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
> >> wrote:
> >>> On 12.12.2022 16:33, Jagan Teki wrote:
> >>>
> >>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> >>> 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
> >>> 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
> >>> Signed-off-by: Jagan Teki
> >>> ---
> >>> 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-9f9d179b5...@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
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
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
>> wrote:
>>> On 12.12.2022 16:33, Jagan Teki wrote:
>>>
>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
>>> 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
>>> 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
>>> Signed-off-by: Jagan Teki
>>> ---
>>> 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-9f9d179b5...@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
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On Tue, Dec 13, 2022 at 5:50 PM Marek Szyprowski
wrote:
>
> Hi,
>
> On 13.12.2022 11:40, Jagan Teki wrote:
> > On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski
> > wrote:
> >> On 12.12.2022 16:33, Jagan Teki wrote:
> >>
> >> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> >> 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
> >> 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
> >> Signed-off-by: Jagan Teki
> >> ---
> >> 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-9f9d179b5...@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
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On 12/13/22 14:26, Jagan Teki wrote: On Tue, Dec 13, 2022 at 6:51 PM Marek Vasut wrote: On 12/13/22 14:18, Jagan Teki wrote: On Tue, Dec 13, 2022 at 6:44 PM Marek Vasut wrote: On 12/13/22 11:53, Jagan Teki wrote: Hi Fabio, Hi, On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam wrote: Hi Jagan, On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki 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
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On Tue, Dec 13, 2022 at 6:51 PM Marek Vasut wrote: > > On 12/13/22 14:18, Jagan Teki wrote: > > On Tue, Dec 13, 2022 at 6:44 PM Marek Vasut wrote: > >> > >> On 12/13/22 11:53, Jagan Teki wrote: > >>> Hi Fabio, > >> > >> Hi, > >> > >>> On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam wrote: > > Hi Jagan, > > On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki > 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.
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On 12/13/22 14:18, Jagan Teki wrote: On Tue, Dec 13, 2022 at 6:44 PM Marek Vasut wrote: On 12/13/22 11:53, Jagan Teki wrote: Hi Fabio, Hi, On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam wrote: Hi Jagan, On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki 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.
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On Tue, Dec 13, 2022 at 6:44 PM Marek Vasut wrote: > > On 12/13/22 11:53, Jagan Teki wrote: > > Hi Fabio, > > Hi, > > > On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam wrote: > >> > >> Hi Jagan, > >> > >> On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki > >> 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.
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On 12/13/22 11:53, Jagan Teki wrote: Hi Fabio, Hi, On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam wrote: Hi Jagan, On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki 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.
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
Hi,
On 13.12.2022 11:40, Jagan Teki wrote:
> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski
> wrote:
>> On 12.12.2022 16:33, Jagan Teki wrote:
>>
>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
>> 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
>> 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
>> Signed-off-by: Jagan Teki
>> ---
>> 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-9f9d179b5...@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 Szyprowski, PhD
Samsung R&D Institute Poland
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
Hi Fabio, On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam wrote: > > Hi Jagan, > > On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki 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.
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
Hi Jagan, On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki 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.
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
Hi Marek,
On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski
wrote:
>
> On 12.12.2022 16:33, Jagan Teki wrote:
>
> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> 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
> 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
> Signed-off-by: Jagan Teki
> ---
> 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-9f9d179b5...@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.
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On 12.12.2022 16:33, Jagan Teki wrote:
> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
> 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
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
>> Signed-off-by: Jagan Teki
>> ---
>> 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-9f9d179b5...@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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski
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
> >> 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
> Signed-off-by: Jagan Teki
> ---
> 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.
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
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
>> 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
Signed-off-by: Jagan Teki
---
Changes from v9:
- derived from v8
- added comments
drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++-
include/drm/bridge/samsung-dsim.h | 5 +++--
2 files changed, 17 insertions(+), 3 deletions(-)
>>> The following chunk is missing compared to v8:
>>>
>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> index 6e9ad955ebd3..6a9403cb92ae 100644
>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim
>>> *dsi, unsigned int flag)
>>> return 0;
>>>
>>> samsung_dsim_reset(dsi);
>>> - samsung_dsim_enable_irq(dsi);
>>> +
>>> + if (!(dsi->state & DSIM_STATE_INITIALIZED))
>>> + samsung_dsim_enable_irq(dsi);
>> Is this really required? does it make sure that the IRQ does not
>> enable twice?
>
> That's what that check does. Without the 'if (!(dsi->state &
> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first
> from pre_enable, then from the first transfer), what leads to a
> warning from irq core.
I've just noticed that we also would need to clear the
DSIM_STATE_REINITIALIZED flag in dsim_suspend.
However I've found that a bit simpler patch would keep the current code
flow for Exynos instead of this reinitialization hack. This can be
applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host
init in pre_enable" patch:
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
b/drivers/gpu/drm/bridge/samsung-dsim.c
index 0b2e52585485..acc95c61ae45 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1291,9 +1291,11 @@ static void samsung_dsim_atomic_pre_enable(struct
drm_bridge *bridge,
dsi->state |= DSIM_STATE_ENABLED;
- ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED);
- if (ret)
- return;
+ if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) {
+ ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED);
+ if (ret)
+ return;
+ }
}
static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
diff --git a/include/drm/bridge/samsung-dsim.h
b/include/drm/bridge/samsung-dsim.h
index b8132bf8e36f..b4e26de88b9e 100644
--- a/include/drm/bridge/samsung-dsim.h
+++ b/include/drm/bridge/samsung-dsim.h
@@ -30,6 +30,9 @@ enum samsung_dsim_type {
SAMSUNG_DSIM_TYPE_COUNT,
};
+#define samsung_dsim_hw_is_exynos(hw) ((hw) >=
SAMSUNG_DSIM_TYPE_EXYNOS3250 && \
+ (hw) <= SAMSUNG_DSIM_TYPE_EXYNOS5433)
+
struct samsung_dsim_transfer {
struct list_head list;
struct completion completed;
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On 12.12.2022 09:32, Jagan Teki wrote: > On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski > 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 >>> Signed-off-by: Jagan Teki >>> --- >>> 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, PhD Samsung R&D Institute Poland
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski 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 > > Signed-off-by: Jagan Teki > > --- > > 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.
Re: [PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
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
> Signed-off-by: Jagan Teki
> ---
> 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
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
[PATCH v9 10/18] drm: bridge: samsung-dsim: Init exynos host for first DSI transfer
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
Signed-off-by: Jagan Teki
---
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(-)
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_AVAILABLEBIT(3)
+#define DSIM_STATE_REINITIALIZED BIT(2)
+#define DSIM_STATE_CMD_LPM BIT(3)
+#define DSIM_STATE_VIDOUT_AVAILABLEBIT(4)
enum samsung_dsim_type {
SAMSUNG_DSIM_TYPE_EXYNOS3250,
--
2.25.1
