[PATCH V2] drm/imx: parallel-display: Remove bus flags check in imx_pd_bridge_atomic_check()

[PATCH V2] drm/imx: parallel-display: Remove bus flags check in imx_pd_bridge_atomic_check()

[Christoph Niedermaier]

If display timings were read from the devicetree using
of_get_display_timing() and pixelclk-active is defined
there, the flag DISPLAY_FLAGS_SYNC_POSEDGE/NEGEDGE is
automatically generated. Through the function
drm_bus_flags_from_videomode() e.g. called in the
panel-simple driver this flag got into the bus flags,
but then in imx_pd_bridge_atomic_check() the bus flag
check failed and will not initialize the display. The
original commit fe141cedc433 does not explain why this
check was introduced. So remove the bus flags check,
because it stops the initialization of the display with
valid bus flags.

Fixes: fe141cedc433 ("drm/imx: pd: Use bus format/flags provided by the bridge when available")
Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
Cc: Marek Vasut <marex@denx.de>
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: linux-arm-kernel@lists.infradead.org
To: dri-devel@lists.freedesktop.org
---
V2: - Add Boris to the Cc list
---
 drivers/gpu/drm/imx/parallel-display.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
index a8aba0141ce7..06cb1a59b9bc 100644
--- a/drivers/gpu/drm/imx/parallel-display.c
+++ b/drivers/gpu/drm/imx/parallel-display.c
@@ -217,14 +217,6 @@ static int imx_pd_bridge_atomic_check(struct drm_bridge *bridge,
 	if (!imx_pd_format_supported(bus_fmt))
 		return -EINVAL;
 
-	if (bus_flags &
-	    ~(DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_DE_HIGH |
-	      DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE |
-	      DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)) {
-		dev_warn(imxpd->dev, "invalid bus_flags (%x)\n", bus_flags);
-		return -EINVAL;
-	}
-
 	bridge_state->output_bus_cfg.flags = bus_flags;
 	bridge_state->input_bus_cfg.flags = bus_flags;
 	imx_crtc_state->bus_flags = bus_flags;
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] drm/imx: parallel-display: Remove bus flags check in imx_pd_bridge_atomic_check()

[Max Krummenacher]

Am Dienstag, den 01.02.2022, 12:36 +0100 schrieb Christoph Niedermaier:
> If display timings were read from the devicetree using
> of_get_display_timing() and pixelclk-active is defined
> there, the flag DISPLAY_FLAGS_SYNC_POSEDGE/NEGEDGE is
> automatically generated. Through the function
> drm_bus_flags_from_videomode() e.g. called in the
> panel-simple driver this flag got into the bus flags,
> but then in imx_pd_bridge_atomic_check() the bus flag
> check failed and will not initialize the display. The
> original commit fe141cedc433 does not explain why this
> check was introduced. So remove the bus flags check,
> because it stops the initialization of the display with
> valid bus flags.
> 
> Fixes: fe141cedc433 ("drm/imx: pd: Use bus format/flags provided by the bridge when available")
> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: linux-arm-kernel@lists.infradead.org
> To: dri-devel@lists.freedesktop.org
> ---
> V2: - Add Boris to the Cc list
> ---
>  drivers/gpu/drm/imx/parallel-display.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> index a8aba0141ce7..06cb1a59b9bc 100644
> --- a/drivers/gpu/drm/imx/parallel-display.c
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -217,14 +217,6 @@ static int imx_pd_bridge_atomic_check(struct drm_bridge *bridge,
>  	if (!imx_pd_format_supported(bus_fmt))
>  		return -EINVAL;
>  
> -	if (bus_flags &
> -	    ~(DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_DE_HIGH |
> -	      DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE |
> -	      DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)) {
> -		dev_warn(imxpd->dev, "invalid bus_flags (%x)\n", bus_flags);
> -		return -EINVAL;
> -	}
> -
>  	bridge_state->output_bus_cfg.flags = bus_flags;
>  	bridge_state->input_bus_cfg.flags = bus_flags;
>  	imx_crtc_state->bus_flags = bus_flags;

Tested on a Colibri iMX6DL with a panel-dpi based panel.

Tested-by: Max Krummenacher <max.krummenacher@toradex.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH V2] drm/imx: parallel-display: Remove bus flags check in imx_pd_bridge_atomic_check()

[Christoph Niedermaier]

From: Max Krummenacher [mailto:max.oss.09@gmail.com]
Sent: Wednesday, February 9, 2022 10:38 AM
>> If display timings were read from the devicetree using
>> of_get_display_timing() and pixelclk-active is defined
>> there, the flag DISPLAY_FLAGS_SYNC_POSEDGE/NEGEDGE is
>> automatically generated. Through the function
>> drm_bus_flags_from_videomode() e.g. called in the
>> panel-simple driver this flag got into the bus flags,
>> but then in imx_pd_bridge_atomic_check() the bus flag
>> check failed and will not initialize the display. The
>> original commit fe141cedc433 does not explain why this
>> check was introduced. So remove the bus flags check,
>> because it stops the initialization of the display with
>> valid bus flags.
>>
>> Fixes: fe141cedc433 ("drm/imx: pd: Use bus format/flags provided by the bridge when available")
>> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
>> Cc: Marek Vasut <marex@denx.de>
>> Cc: Boris Brezillon <boris.brezillon@collabora.com>
>> Cc: Philipp Zabel <p.zabel@pengutronix.de>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>> Cc: Fabio Estevam <festevam@gmail.com>
>> Cc: NXP Linux Team <linux-imx@nxp.com>
>> Cc: linux-arm-kernel@lists.infradead.org
>> To: dri-devel@lists.freedesktop.org
>> ---
>> V2: - Add Boris to the Cc list
>> ---
>>  drivers/gpu/drm/imx/parallel-display.c | 8 --------
>>  1 file changed, 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
>> index a8aba0141ce7..06cb1a59b9bc 100644
>> --- a/drivers/gpu/drm/imx/parallel-display.c
>> +++ b/drivers/gpu/drm/imx/parallel-display.c
>> @@ -217,14 +217,6 @@ static int imx_pd_bridge_atomic_check(struct drm_bridge *bridge,
>>       if (!imx_pd_format_supported(bus_fmt))
>>               return -EINVAL;
>>
>> -     if (bus_flags &
>> -         ~(DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_DE_HIGH |
>> -           DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE |
>> -           DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)) {
>> -             dev_warn(imxpd->dev, "invalid bus_flags (%x)\n", bus_flags);
>> -             return -EINVAL;
>> -     }
>> -
>>       bridge_state->output_bus_cfg.flags = bus_flags;
>>       bridge_state->input_bus_cfg.flags = bus_flags;
>>       imx_crtc_state->bus_flags = bus_flags;
> 
> Tested on a Colibri iMX6DL with a panel-dpi based panel.
> 
> Tested-by: Max Krummenacher <max.krummenacher@toradex.com>

I still ask myself why this bus flag check is in the code.
Is there a reason not to remove that?

Regards
Christoph
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] drm/imx: parallel-display: Remove bus flags check in imx_pd_bridge_atomic_check()

[Boris Brezillon]

Hello Christoph,

On Sat, 19 Feb 2022 09:28:44 +0000
Christoph Niedermaier <cniedermaier@dh-electronics.com> wrote:

> From: Max Krummenacher [mailto:max.oss.09@gmail.com]
> Sent: Wednesday, February 9, 2022 10:38 AM
> >> If display timings were read from the devicetree using
> >> of_get_display_timing() and pixelclk-active is defined
> >> there, the flag DISPLAY_FLAGS_SYNC_POSEDGE/NEGEDGE is
> >> automatically generated. Through the function
> >> drm_bus_flags_from_videomode() e.g. called in the
> >> panel-simple driver this flag got into the bus flags,
> >> but then in imx_pd_bridge_atomic_check() the bus flag
> >> check failed and will not initialize the display. The
> >> original commit fe141cedc433 does not explain why this
> >> check was introduced. So remove the bus flags check,
> >> because it stops the initialization of the display with
> >> valid bus flags.
> >>
> >> Fixes: fe141cedc433 ("drm/imx: pd: Use bus format/flags provided by the bridge when available")
> >> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> >> Cc: Marek Vasut <marex@denx.de>
> >> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> >> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> >> Cc: David Airlie <airlied@linux.ie>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> >> Cc: Shawn Guo <shawnguo@kernel.org>
> >> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> >> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> >> Cc: Fabio Estevam <festevam@gmail.com>
> >> Cc: NXP Linux Team <linux-imx@nxp.com>
> >> Cc: linux-arm-kernel@lists.infradead.org
> >> To: dri-devel@lists.freedesktop.org
> >> ---
> >> V2: - Add Boris to the Cc list
> >> ---
> >>  drivers/gpu/drm/imx/parallel-display.c | 8 --------
> >>  1 file changed, 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> >> index a8aba0141ce7..06cb1a59b9bc 100644
> >> --- a/drivers/gpu/drm/imx/parallel-display.c
> >> +++ b/drivers/gpu/drm/imx/parallel-display.c
> >> @@ -217,14 +217,6 @@ static int imx_pd_bridge_atomic_check(struct drm_bridge *bridge,
> >>       if (!imx_pd_format_supported(bus_fmt))
> >>               return -EINVAL;
> >>
> >> -     if (bus_flags &
> >> -         ~(DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_DE_HIGH |
> >> -           DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE |
> >> -           DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)) {
> >> -             dev_warn(imxpd->dev, "invalid bus_flags (%x)\n", bus_flags);
> >> -             return -EINVAL;
> >> -     }
> >> -
> >>       bridge_state->output_bus_cfg.flags = bus_flags;
> >>       bridge_state->input_bus_cfg.flags = bus_flags;
> >>       imx_crtc_state->bus_flags = bus_flags;  
> > 
> > Tested on a Colibri iMX6DL with a panel-dpi based panel.
> > 
> > Tested-by: Max Krummenacher <max.krummenacher@toradex.com>  
> 
> I still ask myself why this bus flag check is in the code.
> Is there a reason not to remove that?

The reasoning was that DE_{LOW,HIGH} and
FLAG_PIXDATA_DRIVE_{POS,NEG}EDGE were the only bus_flags taken into
account by the crtc logic, so anything else is simply ignored. This was
definitely wrong since the driver supports at least one of the VSYNC
polarity (perhaps both if there's a way to configure it that's not
hooked-up yet).

So I guess figuring out the default VSYNC polarity and accepting the
according DISPLAY_FLAGS_SYNC_XXXEDGE flag is what makes most sense here.

Regards,

Boris

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] drm/imx: parallel-display: Remove bus flags check in imx_pd_bridge_atomic_check()

[Max Krummenacher]

Am Montag, den 21.02.2022, 09:29 +0100 schrieb Boris Brezillon:
> Hello Christoph,
> 
> On Sat, 19 Feb 2022 09:28:44 +0000
> Christoph Niedermaier <cniedermaier@dh-electronics.com> wrote:
> 
> > From: Max Krummenacher [mailto:max.oss.09@gmail.com]
> > Sent: Wednesday, February 9, 2022 10:38 AM
> > > > If display timings were read from the devicetree using
> > > > of_get_display_timing() and pixelclk-active is defined
> > > > there, the flag DISPLAY_FLAGS_SYNC_POSEDGE/NEGEDGE is
> > > > automatically generated. Through the function
> > > > drm_bus_flags_from_videomode() e.g. called in the
> > > > panel-simple driver this flag got into the bus flags,
> > > > but then in imx_pd_bridge_atomic_check() the bus flag
> > > > check failed and will not initialize the display. The
> > > > original commit fe141cedc433 does not explain why this
> > > > check was introduced. So remove the bus flags check,
> > > > because it stops the initialization of the display with
> > > > valid bus flags.
> > > > 
> > > > Fixes: fe141cedc433 ("drm/imx: pd: Use bus format/flags provided by the bridge when
> > > > available")
> > > > Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
> > > > Cc: Marek Vasut <marex@denx.de>
> > > > Cc: Boris Brezillon <boris.brezillon@collabora.com>
> > > > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > > > Cc: David Airlie <airlied@linux.ie>
> > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > Cc: NXP Linux Team <linux-imx@nxp.com>
> > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > To: dri-devel@lists.freedesktop.org
> > > > ---
> > > > V2: - Add Boris to the Cc list
> > > > ---
> > > >  drivers/gpu/drm/imx/parallel-display.c | 8 --------
> > > >  1 file changed, 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c
> > > > index a8aba0141ce7..06cb1a59b9bc 100644
> > > > --- a/drivers/gpu/drm/imx/parallel-display.c
> > > > +++ b/drivers/gpu/drm/imx/parallel-display.c
> > > > @@ -217,14 +217,6 @@ static int imx_pd_bridge_atomic_check(struct drm_bridge *bridge,
> > > >       if (!imx_pd_format_supported(bus_fmt))
> > > >               return -EINVAL;
> > > > 
> > > > -     if (bus_flags &
> > > > -         ~(DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_DE_HIGH |
> > > > -           DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE |
> > > > -           DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)) {
> > > > -             dev_warn(imxpd->dev, "invalid bus_flags (%x)\n", bus_flags);
> > > > -             return -EINVAL;
> > > > -     }
> > > > -
> > > >       bridge_state->output_bus_cfg.flags = bus_flags;
> > > >       bridge_state->input_bus_cfg.flags = bus_flags;
> > > >       imx_crtc_state->bus_flags = bus_flags;  
> > > 
> > > Tested on a Colibri iMX6DL with a panel-dpi based panel.
> > > 
> > > Tested-by: Max Krummenacher <max.krummenacher@toradex.com>  
> > 
> > I still ask myself why this bus flag check is in the code.
> > Is there a reason not to remove that?
> 
> The reasoning was that DE_{LOW,HIGH} and
> FLAG_PIXDATA_DRIVE_{POS,NEG}EDGE were the only bus_flags taken into
> account by the crtc logic, so anything else is simply ignored. This was
> definitely wrong since the driver supports at least one of the VSYNC
> polarity (perhaps both if there's a way to configure it that's not
> hooked-up yet).
> 
> So I guess figuring out the default VSYNC polarity and accepting the
> according DISPLAY_FLAGS_SYNC_XXXEDGE flag is what makes most sense here.

Note that {HV}SYNC polarities are taken from the timings '.flag' field
The bus_flags do not carry
that information.
(drivers/gpu/ipu-v3/ipu-di.c:611:        if (sig->mode.flags &
DISPLAY_FLAGS_HSYNC_HIGH))

The new flags DRM_BUS_FLAG_SYNC_DRIVE_{POS,NEG}EDGE are siblings to
DRM_BUS_FLAG_PIXDATA_DRIVE_{POS,NEG}EDGE and would allow to specify
on which pixelclock edge the sync signals are to be driven.
Before that addition it was implicitly assumed that the sync signals
and data signals would be driven on the same clock edge.
The way I read the IPU driver it is not
foreseen that the data lines
and sync lines are driven by a different clock edge.

I personally would just drop the sanity test on
bus_flags. This is what
this patch proposes.

Regards,
Max
> 
> Regards,
> 
> Boris


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] drm/imx: parallel-display: Remove bus flags check in imx_pd_bridge_atomic_check()

[Marek Vasut]

On 2/21/22 13:31, Boris Brezillon wrote:
> On Mon, 21 Feb 2022 12:56:43 +0100
> Max Krummenacher <max.oss.09@gmail.com> wrote:
> 
>> Am Montag, den 21.02.2022, 09:29 +0100 schrieb Boris Brezillon:
>>> Hello Christoph,
>>>
>>> On Sat, 19 Feb 2022 09:28:44 +0000
>>> Christoph Niedermaier <cniedermaier@dh-electronics.com> wrote:
>>>    
>>>> From: Max Krummenacher [mailto:max.oss.09@gmail.com]
>>>> Sent: Wednesday, February 9, 2022 10:38 AM
>>>>>> If display timings were read from the devicetree using
>>>>>> of_get_display_timing() and pixelclk-active is defined
>>>>>> there, the flag DISPLAY_FLAGS_SYNC_POSEDGE/NEGEDGE is
>>>>>> automatically generated. Through the function
>>>>>> drm_bus_flags_from_videomode() e.g. called in the
>>>>>> panel-simple driver this flag got into the bus flags,
>>>>>> but then in imx_pd_bridge_atomic_check() the bus flag
>>>>>> check failed and will not initialize the display. The
>>>>>> original commit fe141cedc433 does not explain why this

Can you please update the commit message to fix the following warning:

Please use git commit description style 'commit <12+ chars of sha1> 
("<title line>")' - ie: 'commit fe141cedc433 ("drm/imx: pd: Use bus 
format/flags provided by the bridge when available")'

Also, collect the AB from Boris and TB from Max.

I can also fix it up for you while applying if that's OK with you.

[...]

> Acked-by: Boris Brezillon <boris.brezillon@collabora.com>

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH V2] drm/imx: parallel-display: Remove bus flags check in imx_pd_bridge_atomic_check()

[Marek Vasut]

On 2/28/22 02:04, Marek Vasut wrote:
> On 2/21/22 13:31, Boris Brezillon wrote:
>> On Mon, 21 Feb 2022 12:56:43 +0100
>> Max Krummenacher <max.oss.09@gmail.com> wrote:
>>
>>> Am Montag, den 21.02.2022, 09:29 +0100 schrieb Boris Brezillon:
>>>> Hello Christoph,
>>>>
>>>> On Sat, 19 Feb 2022 09:28:44 +0000
>>>> Christoph Niedermaier <cniedermaier@dh-electronics.com> wrote:
>>>>> From: Max Krummenacher [mailto:max.oss.09@gmail.com]
>>>>> Sent: Wednesday, February 9, 2022 10:38 AM
>>>>>>> If display timings were read from the devicetree using
>>>>>>> of_get_display_timing() and pixelclk-active is defined
>>>>>>> there, the flag DISPLAY_FLAGS_SYNC_POSEDGE/NEGEDGE is
>>>>>>> automatically generated. Through the function
>>>>>>> drm_bus_flags_from_videomode() e.g. called in the
>>>>>>> panel-simple driver this flag got into the bus flags,
>>>>>>> but then in imx_pd_bridge_atomic_check() the bus flag
>>>>>>> check failed and will not initialize the display. The
>>>>>>> original commit fe141cedc433 does not explain why this
> 
> Can you please update the commit message to fix the following warning:
> 
> Please use git commit description style 'commit <12+ chars of sha1> 
> ("<title line>")' - ie: 'commit fe141cedc433 ("drm/imx: pd: Use bus 
> format/flags provided by the bridge when available")'
> 
> Also, collect the AB from Boris and TB from Max.
> 
> I can also fix it up for you while applying if that's OK with you.

Nevermind, the patch got applied, it seems I'm not that proficient with 
dim yet ...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel