* [PATCH] drm/rcar-du: dsi: Handle both DRM_MODE_FLAG_N.SYNC and !DRM_MODE_FLAG_P.SYNC
@ 2025-11-07 23:04 Marek Vasut
2025-11-07 23:23 ` Laurent Pinchart
0 siblings, 1 reply; 5+ messages in thread
From: Marek Vasut @ 2025-11-07 23:04 UTC (permalink / raw)
To: dri-devel
Cc: Marek Vasut, David Airlie, Geert Uytterhoeven, Kieran Bingham,
Laurent Pinchart, Maarten Lankhorst, Magnus Damm, Maxime Ripard,
Simona Vetter, Thomas Zimmermann, Tomi Valkeinen, linux-kernel,
linux-renesas-soc
Since commit 94fe479fae96 ("drm/rcar-du: dsi: Clean up handling of DRM mode flags")
the driver does not set TXVMVPRMSET0R_VSPOL_LOW and TXVMVPRMSET0R_HSPOL_LOW
for modes which set neither DRM_MODE_FLAG_[PN].SYNC. The previous behavior
was to assume that neither flag means DRM_MODE_FLAG_N.SYNC . Restore the
previous behavior for maximum compatibility.
Fixes: 94fe479fae96 ("drm/rcar-du: dsi: Clean up handling of DRM mode flags")
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: David Airlie <airlied@gmail.com>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Magnus Damm <magnus.damm@gmail.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
---
drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
index 9413b76d0bfce..98bd7f40adbea 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
@@ -492,9 +492,11 @@ static void rcar_mipi_dsi_set_display_timing(struct rcar_mipi_dsi *dsi,
/* Configuration for Video Parameters, input is always RGB888 */
vprmset0r = TXVMVPRMSET0R_BPP_24;
- if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+ if ((mode->flags & DRM_MODE_FLAG_NVSYNC) ||
+ !(mode->flags & DRM_MODE_FLAG_PVSYNC))
vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
- if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+ if ((mode->flags & DRM_MODE_FLAG_NHSYNC) ||
+ !(mode->flags & DRM_MODE_FLAG_PHSYNC))
vprmset0r |= TXVMVPRMSET0R_HSPOL_LOW;
vprmset1r = TXVMVPRMSET1R_VACTIVE(mode->vdisplay)
--
2.51.0
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/rcar-du: dsi: Handle both DRM_MODE_FLAG_N.SYNC and !DRM_MODE_FLAG_P.SYNC
2025-11-07 23:04 [PATCH] drm/rcar-du: dsi: Handle both DRM_MODE_FLAG_N.SYNC and !DRM_MODE_FLAG_P.SYNC Marek Vasut
@ 2025-11-07 23:23 ` Laurent Pinchart
2025-11-25 20:13 ` Marek Vasut
0 siblings, 1 reply; 5+ messages in thread
From: Laurent Pinchart @ 2025-11-07 23:23 UTC (permalink / raw)
To: Marek Vasut
Cc: dri-devel, David Airlie, Geert Uytterhoeven, Kieran Bingham,
Maarten Lankhorst, Magnus Damm, Maxime Ripard, Simona Vetter,
Thomas Zimmermann, Tomi Valkeinen, linux-kernel,
linux-renesas-soc
On Sat, Nov 08, 2025 at 12:04:10AM +0100, Marek Vasut wrote:
> Since commit 94fe479fae96 ("drm/rcar-du: dsi: Clean up handling of DRM mode flags")
> the driver does not set TXVMVPRMSET0R_VSPOL_LOW and TXVMVPRMSET0R_HSPOL_LOW
> for modes which set neither DRM_MODE_FLAG_[PN].SYNC.
Could you please explain what broke ? What panel are you using ?
> The previous behavior
> was to assume that neither flag means DRM_MODE_FLAG_N.SYNC . Restore the
> previous behavior for maximum compatibility.
>
> Fixes: 94fe479fae96 ("drm/rcar-du: dsi: Clean up handling of DRM mode flags")
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: David Airlie <airlied@gmail.com>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Magnus Damm <magnus.damm@gmail.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
> index 9413b76d0bfce..98bd7f40adbea 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_mipi_dsi.c
> @@ -492,9 +492,11 @@ static void rcar_mipi_dsi_set_display_timing(struct rcar_mipi_dsi *dsi,
>
> /* Configuration for Video Parameters, input is always RGB888 */
> vprmset0r = TXVMVPRMSET0R_BPP_24;
> - if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> + if ((mode->flags & DRM_MODE_FLAG_NVSYNC) ||
> + !(mode->flags & DRM_MODE_FLAG_PVSYNC))
> vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
I don't think this restores the previous behaviour. You would need to
write
if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
> - if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> + if ((mode->flags & DRM_MODE_FLAG_NHSYNC) ||
> + !(mode->flags & DRM_MODE_FLAG_PHSYNC))
> vprmset0r |= TXVMVPRMSET0R_HSPOL_LOW;
Same here.
>
> vprmset1r = TXVMVPRMSET1R_VACTIVE(mode->vdisplay)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/rcar-du: dsi: Handle both DRM_MODE_FLAG_N.SYNC and !DRM_MODE_FLAG_P.SYNC
2025-11-07 23:23 ` Laurent Pinchart
@ 2025-11-25 20:13 ` Marek Vasut
2025-12-01 6:09 ` Laurent Pinchart
0 siblings, 1 reply; 5+ messages in thread
From: Marek Vasut @ 2025-11-25 20:13 UTC (permalink / raw)
To: Laurent Pinchart
Cc: dri-devel, David Airlie, Geert Uytterhoeven, Kieran Bingham,
Maarten Lankhorst, Magnus Damm, Maxime Ripard, Simona Vetter,
Thomas Zimmermann, Tomi Valkeinen, linux-kernel,
linux-renesas-soc
On 11/8/25 12:23 AM, Laurent Pinchart wrote:
> On Sat, Nov 08, 2025 at 12:04:10AM +0100, Marek Vasut wrote:
>> Since commit 94fe479fae96 ("drm/rcar-du: dsi: Clean up handling of DRM mode flags")
>> the driver does not set TXVMVPRMSET0R_VSPOL_LOW and TXVMVPRMSET0R_HSPOL_LOW
>> for modes which set neither DRM_MODE_FLAG_[PN].SYNC.
>
> Could you please explain what broke ?
Consider mode->flags, V-ones for simplicity:
Before 94fe479fae96 :
DRM_MODE_FLAG_PVSYNC => vprmset0r |= 0
DRM_MODE_FLAG_NVSYNC => vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW
Neither DRM_MODE_FLAG_[PN]VSYNC => vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW
After 94fe479fae96 :
DRM_MODE_FLAG_PVSYNC => vprmset0r |= 0
DRM_MODE_FLAG_NVSYNC => vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW
Neither DRM_MODE_FLAG_[PN]VSYNC => vprmset0r |= 0 <---------- This broke
The "Neither" case behavior is different. I did not realize that:
DRM_MODE_FLAG_N[HV]SYNC is not equivalent !DRM_MODE_FLAG_P[HV]SYNC
They really are not equivalent .
[...]
>> /* Configuration for Video Parameters, input is always RGB888 */
>> vprmset0r = TXVMVPRMSET0R_BPP_24;
>> - if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>> + if ((mode->flags & DRM_MODE_FLAG_NVSYNC) ||
>> + !(mode->flags & DRM_MODE_FLAG_PVSYNC))
>> vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
>
> I don't think this restores the previous behaviour. You would need to
> write
>
> if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
> vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
This patch covers both the N[HV]SYNC and !P[HV]SYNC , so that should
restore the behavior to "Before" and explicitly be clear that N[HV]SYNC
and !P[HV]SYNC are not the same thing.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/rcar-du: dsi: Handle both DRM_MODE_FLAG_N.SYNC and !DRM_MODE_FLAG_P.SYNC
2025-11-25 20:13 ` Marek Vasut
@ 2025-12-01 6:09 ` Laurent Pinchart
2025-12-02 18:18 ` Marek Vasut
0 siblings, 1 reply; 5+ messages in thread
From: Laurent Pinchart @ 2025-12-01 6:09 UTC (permalink / raw)
To: Marek Vasut
Cc: dri-devel, David Airlie, Geert Uytterhoeven, Kieran Bingham,
Maarten Lankhorst, Magnus Damm, Maxime Ripard, Simona Vetter,
Thomas Zimmermann, Tomi Valkeinen, linux-kernel,
linux-renesas-soc
Hi Marek,
On Tue, Nov 25, 2025 at 09:13:02PM +0100, Marek Vasut wrote:
> On 11/8/25 12:23 AM, Laurent Pinchart wrote:
> > On Sat, Nov 08, 2025 at 12:04:10AM +0100, Marek Vasut wrote:
> >> Since commit 94fe479fae96 ("drm/rcar-du: dsi: Clean up handling of DRM mode flags")
> >> the driver does not set TXVMVPRMSET0R_VSPOL_LOW and TXVMVPRMSET0R_HSPOL_LOW
> >> for modes which set neither DRM_MODE_FLAG_[PN].SYNC.
> >
> > Could you please explain what broke ?
Sorry, I wasn't clear. I meant could you summarize the explanation in
the commit message ?
> Consider mode->flags, V-ones for simplicity:
>
> Before 94fe479fae96 :
>
> DRM_MODE_FLAG_PVSYNC => vprmset0r |= 0
> DRM_MODE_FLAG_NVSYNC => vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW
> Neither DRM_MODE_FLAG_[PN]VSYNC => vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW
>
> After 94fe479fae96 :
>
> DRM_MODE_FLAG_PVSYNC => vprmset0r |= 0
> DRM_MODE_FLAG_NVSYNC => vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW
> Neither DRM_MODE_FLAG_[PN]VSYNC => vprmset0r |= 0 <---------- This broke
>
> The "Neither" case behavior is different. I did not realize that:
>
> DRM_MODE_FLAG_N[HV]SYNC is not equivalent !DRM_MODE_FLAG_P[HV]SYNC
>
> They really are not equivalent .
>
> [...]
>
> >> /* Configuration for Video Parameters, input is always RGB888 */
> >> vprmset0r = TXVMVPRMSET0R_BPP_24;
> >> - if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> >> + if ((mode->flags & DRM_MODE_FLAG_NVSYNC) ||
> >> + !(mode->flags & DRM_MODE_FLAG_PVSYNC))
> >> vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
> >
> > I don't think this restores the previous behaviour. You would need to
> > write
> >
> > if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
> > vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
>
> This patch covers both the N[HV]SYNC and !P[HV]SYNC , so that should
> restore the behavior to "Before" and explicitly be clear that N[HV]SYNC
> and !P[HV]SYNC are not the same thing.
Before commit 94fe479fae96 we had
vprmset0r = (mode->flags & DRM_MODE_FLAG_PVSYNC ?
TXVMVPRMSET0R_VSPOL_HIG : TXVMVPRMSET0R_VSPOL_LOW)
| (mode->flags & DRM_MODE_FLAG_PHSYNC ?
TXVMVPRMSET0R_HSPOL_HIG : TXVMVPRMSET0R_HSPOL_LOW)
| TXVMVPRMSET0R_CSPC_RGB | TXVMVPRMSET0R_BPP_24;
Considering the vertical sync for simplicity, this gives us
NVSYNC \ PVSYNC 0 1
0 VSPOL_LOW VSPOL_HIG
1 VSPOL_LOW VSPOL_HIG
With this patch, the code becomes
/* Configuration for Video Parameters, input is always RGB888 */
vprmset0r = TXVMVPRMSET0R_BPP_24;
if ((mode->flags & DRM_MODE_FLAG_NVSYNC) ||
!(mode->flags & DRM_MODE_FLAG_PVSYNC))
vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
if ((mode->flags & DRM_MODE_FLAG_NHSYNC) ||
!(mode->flags & DRM_MODE_FLAG_PHSYNC))
vprmset0r |= TXVMVPRMSET0R_HSPOL_LOW;
which gives us
NVSYNC \ PVSYNC 0 1
0 VSPOL_LOW VSPOL_HIG
1 VSPOL_LOW VSPOL_LOW
This is a different behaviour. Granted, we should never have both NVSYNC
and PVSYNC set together (unless I'm missing something), so the
difference in behaviour shouldn't matter. I'm fine with that if you
explain it in the commit message, however I think that writing
if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
vprmset0r |= TXVMVPRMSET0R_HSPOL_LOW;
would both restore the previous behaviour in all cases, and be simpler.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/rcar-du: dsi: Handle both DRM_MODE_FLAG_N.SYNC and !DRM_MODE_FLAG_P.SYNC
2025-12-01 6:09 ` Laurent Pinchart
@ 2025-12-02 18:18 ` Marek Vasut
0 siblings, 0 replies; 5+ messages in thread
From: Marek Vasut @ 2025-12-02 18:18 UTC (permalink / raw)
To: Laurent Pinchart
Cc: dri-devel, David Airlie, Geert Uytterhoeven, Kieran Bingham,
Maarten Lankhorst, Magnus Damm, Maxime Ripard, Simona Vetter,
Thomas Zimmermann, Tomi Valkeinen, linux-kernel,
linux-renesas-soc
On 12/1/25 7:09 AM, Laurent Pinchart wrote:
Hello Laurent,
> On Tue, Nov 25, 2025 at 09:13:02PM +0100, Marek Vasut wrote:
>> On 11/8/25 12:23 AM, Laurent Pinchart wrote:
>>> On Sat, Nov 08, 2025 at 12:04:10AM +0100, Marek Vasut wrote:
>>>> Since commit 94fe479fae96 ("drm/rcar-du: dsi: Clean up handling of DRM mode flags")
>>>> the driver does not set TXVMVPRMSET0R_VSPOL_LOW and TXVMVPRMSET0R_HSPOL_LOW
>>>> for modes which set neither DRM_MODE_FLAG_[PN].SYNC.
>>>
>>> Could you please explain what broke ?
>
> Sorry, I wasn't clear. I meant could you summarize the explanation in
> the commit message ?
>
>> Consider mode->flags, V-ones for simplicity:
>>
>> Before 94fe479fae96 :
>>
>> DRM_MODE_FLAG_PVSYNC => vprmset0r |= 0
>> DRM_MODE_FLAG_NVSYNC => vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW
>> Neither DRM_MODE_FLAG_[PN]VSYNC => vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW
>>
>> After 94fe479fae96 :
>>
>> DRM_MODE_FLAG_PVSYNC => vprmset0r |= 0
>> DRM_MODE_FLAG_NVSYNC => vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW
>> Neither DRM_MODE_FLAG_[PN]VSYNC => vprmset0r |= 0 <---------- This broke
>>
>> The "Neither" case behavior is different. I did not realize that:
>>
>> DRM_MODE_FLAG_N[HV]SYNC is not equivalent !DRM_MODE_FLAG_P[HV]SYNC
>>
>> They really are not equivalent .
>>
>> [...]
>>
>>>> /* Configuration for Video Parameters, input is always RGB888 */
>>>> vprmset0r = TXVMVPRMSET0R_BPP_24;
>>>> - if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>>>> + if ((mode->flags & DRM_MODE_FLAG_NVSYNC) ||
>>>> + !(mode->flags & DRM_MODE_FLAG_PVSYNC))
>>>> vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
>>>
>>> I don't think this restores the previous behaviour. You would need to
>>> write
>>>
>>> if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
>>> vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
>>
>> This patch covers both the N[HV]SYNC and !P[HV]SYNC , so that should
>> restore the behavior to "Before" and explicitly be clear that N[HV]SYNC
>> and !P[HV]SYNC are not the same thing.
>
> Before commit 94fe479fae96 we had
>
> vprmset0r = (mode->flags & DRM_MODE_FLAG_PVSYNC ?
> TXVMVPRMSET0R_VSPOL_HIG : TXVMVPRMSET0R_VSPOL_LOW)
> | (mode->flags & DRM_MODE_FLAG_PHSYNC ?
> TXVMVPRMSET0R_HSPOL_HIG : TXVMVPRMSET0R_HSPOL_LOW)
> | TXVMVPRMSET0R_CSPC_RGB | TXVMVPRMSET0R_BPP_24;
>
> Considering the vertical sync for simplicity, this gives us
>
> NVSYNC \ PVSYNC 0 1
> 0 VSPOL_LOW VSPOL_HIG
> 1 VSPOL_LOW VSPOL_HIG
>
> With this patch, the code becomes
>
> /* Configuration for Video Parameters, input is always RGB888 */
> vprmset0r = TXVMVPRMSET0R_BPP_24;
> if ((mode->flags & DRM_MODE_FLAG_NVSYNC) ||
> !(mode->flags & DRM_MODE_FLAG_PVSYNC))
> vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
> if ((mode->flags & DRM_MODE_FLAG_NHSYNC) ||
> !(mode->flags & DRM_MODE_FLAG_PHSYNC))
> vprmset0r |= TXVMVPRMSET0R_HSPOL_LOW;
>
> which gives us
>
> NVSYNC \ PVSYNC 0 1
> 0 VSPOL_LOW VSPOL_HIG
> 1 VSPOL_LOW VSPOL_LOW
>
> This is a different behaviour. Granted, we should never have both NVSYNC
> and PVSYNC set together (unless I'm missing something), so the
> difference in behaviour shouldn't matter. I'm fine with that if you
> explain it in the commit message, however I think that writing
>
> if (!(mode->flags & DRM_MODE_FLAG_PVSYNC))
> vprmset0r |= TXVMVPRMSET0R_VSPOL_LOW;
> if (!(mode->flags & DRM_MODE_FLAG_PHSYNC))
> vprmset0r |= TXVMVPRMSET0R_HSPOL_LOW;
>
> would both restore the previous behaviour in all cases, and be simpler.
I sent a V2 which addresses both, the commit message update and this
comment.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-12-02 18:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-11-07 23:04 [PATCH] drm/rcar-du: dsi: Handle both DRM_MODE_FLAG_N.SYNC and !DRM_MODE_FLAG_P.SYNC Marek Vasut
2025-11-07 23:23 ` Laurent Pinchart
2025-11-25 20:13 ` Marek Vasut
2025-12-01 6:09 ` Laurent Pinchart
2025-12-02 18:18 ` Marek Vasut
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®