[v2,08/12] drm: bridge: samsung-dsim: Add atomic_check

Message ID 20220504114021.33265-9-jagan@amarulasolutions.com
State New
Headers show
Series
  • drm: bridge: Add Samsung MIPI DSIM bridge
Related show

Commit Message

Jagan Teki May 4, 2022, 11:40 a.m. UTC
Fixing up the mode flags is required in order to correlate the
correct sync flags of the surrounding components in the chain
to make sure the whole pipeline can work properly.

So, handle the mode flags via bridge, atomic_check.

v2:
* none

v1:
* fix mode flags in atomic_check instead of mode_fixup

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Marek Szyprowski May 10, 2022, 9:41 p.m. UTC | #1
On 04.05.2022 13:40, Jagan Teki wrote:
> Fixing up the mode flags is required in order to correlate the
> correct sync flags of the surrounding components in the chain
> to make sure the whole pipeline can work properly.
>
> So, handle the mode flags via bridge, atomic_check.
>
> v2:
> * none
>
> v1:
> * fix mode flags in atomic_check instead of mode_fixup
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index b618e52d0ee3..bd78cef890e4 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1340,6 +1340,19 @@ static void samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>   	pm_runtime_put_sync(dsi->dev);
>   }
>   
> +static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
> +				     struct drm_bridge_state *bridge_state,
> +				     struct drm_crtc_state *crtc_state,
> +				     struct drm_connector_state *conn_state)
> +{
> +	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> +
> +	adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);

Could you point why this change is needed? It breaks display on Samsung 
s6e8aa0 dsi panel (Trats and Trats2 boards) and tc358764 bridge (Arndale 
board). I have no detailed knowledge of the DSI protocol nor those 
panels/bridges. If there is something missing in the drivers for those 
panels/bridges, let me know.


> +	adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> +
> +	return 0;
> +}
> +
>   static void samsung_dsim_mode_set(struct drm_bridge *bridge,
>   				  const struct drm_display_mode *mode,
>   				  const struct drm_display_mode *adjusted_mode)
> @@ -1361,6 +1374,7 @@ static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
>   	.atomic_duplicate_state		= drm_atomic_helper_bridge_duplicate_state,
>   	.atomic_destroy_state		= drm_atomic_helper_bridge_destroy_state,
>   	.atomic_reset			= drm_atomic_helper_bridge_reset,
> +	.atomic_check			= samsung_dsim_atomic_check,
>   	.atomic_pre_enable		= samsung_dsim_atomic_pre_enable,
>   	.atomic_enable			= samsung_dsim_atomic_enable,
>   	.atomic_disable			= samsung_dsim_atomic_disable,

Best regards
Andrzej Hajda May 11, 2022, 10:31 a.m. UTC | #2
On 04.05.2022 13:40, Jagan Teki wrote:
> Fixing up the mode flags is required in order to correlate the
> correct sync flags of the surrounding components in the chain
> to make sure the whole pipeline can work properly.
> 
> So, handle the mode flags via bridge, atomic_check.
> 
> v2:
> * none
> 
> v1:
> * fix mode flags in atomic_check instead of mode_fixup
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index b618e52d0ee3..bd78cef890e4 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1340,6 +1340,19 @@ static void samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>   	pm_runtime_put_sync(dsi->dev);
>   }
>   
> +static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
> +				     struct drm_bridge_state *bridge_state,
> +				     struct drm_crtc_state *crtc_state,
> +				     struct drm_connector_state *conn_state)
> +{
> +	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> +
> +	adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> +	adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);


1. Shouldn't this be in mode_fixup callback?
2. Where this requirement comes from? As Marek says it breaks Samsung 
platforms and is against DSIM specification[1]:

"45.2.2.1.2 RGB Interface
Vsync, Hsync, and VDEN are active high signals"

[1]: 
https://chasinglulu.github.io/downloads/SEC_Exynos4412_Users%20Manual_Ver.1.00.00.pdf

Regards
Andrzej



> +
> +	return 0;
> +}
> +
>   static void samsung_dsim_mode_set(struct drm_bridge *bridge,
>   				  const struct drm_display_mode *mode,
>   				  const struct drm_display_mode *adjusted_mode)
> @@ -1361,6 +1374,7 @@ static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
>   	.atomic_duplicate_state		= drm_atomic_helper_bridge_duplicate_state,
>   	.atomic_destroy_state		= drm_atomic_helper_bridge_destroy_state,
>   	.atomic_reset			= drm_atomic_helper_bridge_reset,
> +	.atomic_check			= samsung_dsim_atomic_check,
>   	.atomic_pre_enable		= samsung_dsim_atomic_pre_enable,
>   	.atomic_enable			= samsung_dsim_atomic_enable,
>   	.atomic_disable			= samsung_dsim_atomic_disable,
Jagan Teki June 13, 2022, 11:17 a.m. UTC | #3
On Wed, May 11, 2022 at 4:01 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>
> On 04.05.2022 13:40, Jagan Teki wrote:
> > Fixing up the mode flags is required in order to correlate the
> > correct sync flags of the surrounding components in the chain
> > to make sure the whole pipeline can work properly.
> >
> > So, handle the mode flags via bridge, atomic_check.
> >
> > v2:
> > * none
> >
> > v1:
> > * fix mode flags in atomic_check instead of mode_fixup
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >   drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index b618e52d0ee3..bd78cef890e4 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -1340,6 +1340,19 @@ static void samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
> >       pm_runtime_put_sync(dsi->dev);
> >   }
> >
> > +static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
> > +                                  struct drm_bridge_state *bridge_state,
> > +                                  struct drm_crtc_state *crtc_state,
> > +                                  struct drm_connector_state *conn_state)
> > +{
> > +     struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> > +
> > +     adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> > +     adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>
>
> 1. Shouldn't this be in mode_fixup callback?

Possible to do it on mode_fixup, yes. if we want to do the same stuff
on atomic API then atomic_check is the proper helper.

> 2. Where this requirement comes from? As Marek says it breaks Samsung
> platforms and is against DSIM specification[1]:

At least the bridge chain starting from LCDIF+DSIM requires active high sync.

Jagan.
Marek Szyprowski June 13, 2022, 11:32 a.m. UTC | #4
On 13.06.2022 13:17, Jagan Teki wrote:
> On Wed, May 11, 2022 at 4:01 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>> On 04.05.2022 13:40, Jagan Teki wrote:
>>> Fixing up the mode flags is required in order to correlate the
>>> correct sync flags of the surrounding components in the chain
>>> to make sure the whole pipeline can work properly.
>>>
>>> So, handle the mode flags via bridge, atomic_check.
>>>
>>> v2:
>>> * none
>>>
>>> v1:
>>> * fix mode flags in atomic_check instead of mode_fixup
>>>
>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>> ---
>>>    drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> index b618e52d0ee3..bd78cef890e4 100644
>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>> @@ -1340,6 +1340,19 @@ static void samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>>>        pm_runtime_put_sync(dsi->dev);
>>>    }
>>>
>>> +static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
>>> +                                  struct drm_bridge_state *bridge_state,
>>> +                                  struct drm_crtc_state *crtc_state,
>>> +                                  struct drm_connector_state *conn_state)
>>> +{
>>> +     struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
>>> +
>>> +     adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
>>> +     adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>>
>> 1. Shouldn't this be in mode_fixup callback?
> Possible to do it on mode_fixup, yes. if we want to do the same stuff
> on atomic API then atomic_check is the proper helper.
>
>> 2. Where this requirement comes from? As Marek says it breaks Samsung
>> platforms and is against DSIM specification[1]:
> At least the bridge chain starting from LCDIF+DSIM requires active high sync.

Then please make this specific to the imx variant. The current version 
breaks DSI operation on all Exynos based boards.

Best regards
Jagan Teki June 13, 2022, 11:34 a.m. UTC | #5
On Mon, Jun 13, 2022 at 5:02 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> On 13.06.2022 13:17, Jagan Teki wrote:
> > On Wed, May 11, 2022 at 4:01 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
> >> On 04.05.2022 13:40, Jagan Teki wrote:
> >>> Fixing up the mode flags is required in order to correlate the
> >>> correct sync flags of the surrounding components in the chain
> >>> to make sure the whole pipeline can work properly.
> >>>
> >>> So, handle the mode flags via bridge, atomic_check.
> >>>
> >>> v2:
> >>> * none
> >>>
> >>> v1:
> >>> * fix mode flags in atomic_check instead of mode_fixup
> >>>
> >>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >>> ---
> >>>    drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++++++++
> >>>    1 file changed, 14 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>> index b618e52d0ee3..bd78cef890e4 100644
> >>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> >>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>> @@ -1340,6 +1340,19 @@ static void samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
> >>>        pm_runtime_put_sync(dsi->dev);
> >>>    }
> >>>
> >>> +static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
> >>> +                                  struct drm_bridge_state *bridge_state,
> >>> +                                  struct drm_crtc_state *crtc_state,
> >>> +                                  struct drm_connector_state *conn_state)
> >>> +{
> >>> +     struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> >>> +
> >>> +     adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> >>> +     adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> >>
> >> 1. Shouldn't this be in mode_fixup callback?
> > Possible to do it on mode_fixup, yes. if we want to do the same stuff
> > on atomic API then atomic_check is the proper helper.
> >
> >> 2. Where this requirement comes from? As Marek says it breaks Samsung
> >> platforms and is against DSIM specification[1]:
> > At least the bridge chain starting from LCDIF+DSIM requires active high sync.
>
> Then please make this specific to the imx variant. The current version
> breaks DSI operation on all Exynos based boards.

But exynos trm also says the same?

"45.2.2.1.2 RGB Interface
Vsync, Hsync, and VDEN are active high signals"

Jagan.
Marek Szyprowski June 13, 2022, 11:36 a.m. UTC | #6
On 13.06.2022 13:34, Jagan Teki wrote:
> On Mon, Jun 13, 2022 at 5:02 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 13.06.2022 13:17, Jagan Teki wrote:
>>> On Wed, May 11, 2022 at 4:01 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
>>>> On 04.05.2022 13:40, Jagan Teki wrote:
>>>>> Fixing up the mode flags is required in order to correlate the
>>>>> correct sync flags of the surrounding components in the chain
>>>>> to make sure the whole pipeline can work properly.
>>>>>
>>>>> So, handle the mode flags via bridge, atomic_check.
>>>>>
>>>>> v2:
>>>>> * none
>>>>>
>>>>> v1:
>>>>> * fix mode flags in atomic_check instead of mode_fixup
>>>>>
>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
>>>>> ---
>>>>>     drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++++++++
>>>>>     1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> index b618e52d0ee3..bd78cef890e4 100644
>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
>>>>> @@ -1340,6 +1340,19 @@ static void samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
>>>>>         pm_runtime_put_sync(dsi->dev);
>>>>>     }
>>>>>
>>>>> +static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
>>>>> +                                  struct drm_bridge_state *bridge_state,
>>>>> +                                  struct drm_crtc_state *crtc_state,
>>>>> +                                  struct drm_connector_state *conn_state)
>>>>> +{
>>>>> +     struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
>>>>> +
>>>>> +     adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
>>>>> +     adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>>>> 1. Shouldn't this be in mode_fixup callback?
>>> Possible to do it on mode_fixup, yes. if we want to do the same stuff
>>> on atomic API then atomic_check is the proper helper.
>>>
>>>> 2. Where this requirement comes from? As Marek says it breaks Samsung
>>>> platforms and is against DSIM specification[1]:
>>> At least the bridge chain starting from LCDIF+DSIM requires active high sync.
>> Then please make this specific to the imx variant. The current version
>> breaks DSI operation on all Exynos based boards.
> But exynos trm also says the same?
>
> "45.2.2.1.2 RGB Interface
> Vsync, Hsync, and VDEN are active high signals"

Right, but You are forcing the negative sync signals:

adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);

Best regards
Lucas Stach June 13, 2022, 12:15 p.m. UTC | #7
Am Montag, dem 13.06.2022 um 13:36 +0200 schrieb Marek Szyprowski:
> On 13.06.2022 13:34, Jagan Teki wrote:
> > On Mon, Jun 13, 2022 at 5:02 PM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> > > On 13.06.2022 13:17, Jagan Teki wrote:
> > > > On Wed, May 11, 2022 at 4:01 PM Andrzej Hajda <andrzej.hajda@intel.com> wrote:
> > > > > On 04.05.2022 13:40, Jagan Teki wrote:
> > > > > > Fixing up the mode flags is required in order to correlate the
> > > > > > correct sync flags of the surrounding components in the chain
> > > > > > to make sure the whole pipeline can work properly.
> > > > > > 
> > > > > > So, handle the mode flags via bridge, atomic_check.
> > > > > > 
> > > > > > v2:
> > > > > > * none
> > > > > > 
> > > > > > v1:
> > > > > > * fix mode flags in atomic_check instead of mode_fixup
> > > > > > 
> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > ---
> > > > > >     drivers/gpu/drm/bridge/samsung-dsim.c | 14 ++++++++++++++
> > > > > >     1 file changed, 14 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > > index b618e52d0ee3..bd78cef890e4 100644
> > > > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > > @@ -1340,6 +1340,19 @@ static void samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
> > > > > >         pm_runtime_put_sync(dsi->dev);
> > > > > >     }
> > > > > > 
> > > > > > +static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
> > > > > > +                                  struct drm_bridge_state *bridge_state,
> > > > > > +                                  struct drm_crtc_state *crtc_state,
> > > > > > +                                  struct drm_connector_state *conn_state)
> > > > > > +{
> > > > > > +     struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> > > > > > +
> > > > > > +     adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> > > > > > +     adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> > > > > 1. Shouldn't this be in mode_fixup callback?
> > > > Possible to do it on mode_fixup, yes. if we want to do the same stuff
> > > > on atomic API then atomic_check is the proper helper.
> > > > 
> > > > > 2. Where this requirement comes from? As Marek says it breaks Samsung
> > > > > platforms and is against DSIM specification[1]:
> > > > At least the bridge chain starting from LCDIF+DSIM requires active high sync.
> > > Then please make this specific to the imx variant. The current version
> > > breaks DSI operation on all Exynos based boards.
> > But exynos trm also says the same?
> > 
> > "45.2.2.1.2 RGB Interface
> > Vsync, Hsync, and VDEN are active high signals"
> 
> Right, but You are forcing the negative sync signals:
> 
> adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> 
And this is a quirk of the i.MX8MM (and possibly i.MX8MN) SoC
integration. The same Samsung DSI IP core on the i.MX8MP does require
active high sync signals, matching the documentation.

Regards,
Lucas

Patch

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index b618e52d0ee3..bd78cef890e4 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1340,6 +1340,19 @@  static void samsung_dsim_atomic_post_disable(struct drm_bridge *bridge,
 	pm_runtime_put_sync(dsi->dev);
 }
 
+static int samsung_dsim_atomic_check(struct drm_bridge *bridge,
+				     struct drm_bridge_state *bridge_state,
+				     struct drm_crtc_state *crtc_state,
+				     struct drm_connector_state *conn_state)
+{
+	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
+
+	adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
+	adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
+
+	return 0;
+}
+
 static void samsung_dsim_mode_set(struct drm_bridge *bridge,
 				  const struct drm_display_mode *mode,
 				  const struct drm_display_mode *adjusted_mode)
@@ -1361,6 +1374,7 @@  static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
 	.atomic_duplicate_state		= drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state		= drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset			= drm_atomic_helper_bridge_reset,
+	.atomic_check			= samsung_dsim_atomic_check,
 	.atomic_pre_enable		= samsung_dsim_atomic_pre_enable,
 	.atomic_enable			= samsung_dsim_atomic_enable,
 	.atomic_disable			= samsung_dsim_atomic_disable,