All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm: rcar-du: Fix CRTC timings when CMM is used
@ 2021-11-29 22:28 Laurent Pinchart
  2022-02-22 12:58 ` Kieran Bingham
  0 siblings, 1 reply; 4+ messages in thread
From: Laurent Pinchart @ 2021-11-29 22:28 UTC (permalink / raw)
  To: dri-devel; +Cc: linux-renesas-soc, Kieran Bingham

When the CMM is enabled, an offset of 25 pixels must be subtracted from
the HDS (horizontal display start) and HDE (horizontal display end)
registers. Fix the timings calculation, and take this into account in
the mode validation.

This fixes a visible horizontal offset in the image with VGA monitors.
HDMI monitors seem to be generally more tolerant to incorrect timings,
but may be affected too.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 5672830ca184..ee6ba74627a2 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -215,6 +215,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 	const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
 	struct rcar_du_device *rcdu = rcrtc->dev;
 	unsigned long mode_clock = mode->clock * 1000;
+	unsigned int hdse_offset;
 	u32 dsmr;
 	u32 escr;
 
@@ -298,10 +299,15 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
 	     | DSMR_DIPM_DISP | DSMR_CSPM;
 	rcar_du_crtc_write(rcrtc, DSMR, dsmr);
 
+	hdse_offset = 19;
+	if (rcrtc->group->cmms_mask & BIT(rcrtc->index % 2))
+		hdse_offset += 25;
+
 	/* Display timings */
-	rcar_du_crtc_write(rcrtc, HDSR, mode->htotal - mode->hsync_start - 19);
+	rcar_du_crtc_write(rcrtc, HDSR, mode->htotal - mode->hsync_start -
+					hdse_offset);
 	rcar_du_crtc_write(rcrtc, HDER, mode->htotal - mode->hsync_start +
-					mode->hdisplay - 19);
+					mode->hdisplay - hdse_offset);
 	rcar_du_crtc_write(rcrtc, HSWR, mode->hsync_end -
 					mode->hsync_start - 1);
 	rcar_du_crtc_write(rcrtc, HCR,  mode->htotal - 1);
@@ -836,6 +842,7 @@ rcar_du_crtc_mode_valid(struct drm_crtc *crtc,
 	struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
 	struct rcar_du_device *rcdu = rcrtc->dev;
 	bool interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
+	unsigned int min_sync_porch;
 	unsigned int vbp;
 
 	if (interlaced && !rcar_du_has(rcdu, RCAR_DU_FEATURE_INTERLACED))
@@ -843,9 +850,14 @@ rcar_du_crtc_mode_valid(struct drm_crtc *crtc,
 
 	/*
 	 * The hardware requires a minimum combined horizontal sync and back
-	 * porch of 20 pixels and a minimum vertical back porch of 3 lines.
+	 * porch of 20 pixels (when CMM isn't used) or 45 pixels (when CMM is
+	 * used), and a minimum vertical back porch of 3 lines.
 	 */
-	if (mode->htotal - mode->hsync_start < 20)
+	min_sync_porch = 20;
+	if (rcrtc->group->cmms_mask & BIT(rcrtc->index % 2))
+		min_sync_porch += 25;
+
+	if (mode->htotal - mode->hsync_start < min_sync_porch)
 		return MODE_HBLANK_NARROW;
 
 	vbp = (mode->vtotal - mode->vsync_end) / (interlaced ? 2 : 1);

base-commit: c18c8891111bb5e014e144716044991112f16833
prerequisite-patch-id: dc9121a1b85ea05bf3eae2b0ac2168d47101ee87
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix CRTC timings when CMM is used
  2021-11-29 22:28 [PATCH] drm: rcar-du: Fix CRTC timings when CMM is used Laurent Pinchart
@ 2022-02-22 12:58 ` Kieran Bingham
  2022-02-22 15:05     ` Laurent Pinchart
  0 siblings, 1 reply; 4+ messages in thread
From: Kieran Bingham @ 2022-02-22 12:58 UTC (permalink / raw)
  To: Laurent Pinchart, dri-devel; +Cc: linux-renesas-soc

Quoting Laurent Pinchart (2021-11-29 22:28:13)
> When the CMM is enabled, an offset of 25 pixels must be subtracted from
> the HDS (horizontal display start) and HDE (horizontal display end)
> registers. Fix the timings calculation, and take this into account in
> the mode validation.
> 
> This fixes a visible horizontal offset in the image with VGA monitors.
> HDMI monitors seem to be generally more tolerant to incorrect timings,
> but may be affected too.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 5672830ca184..ee6ba74627a2 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -215,6 +215,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>         const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
>         struct rcar_du_device *rcdu = rcrtc->dev;
>         unsigned long mode_clock = mode->clock * 1000;
> +       unsigned int hdse_offset;
>         u32 dsmr;
>         u32 escr;
>  
> @@ -298,10 +299,15 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
>              | DSMR_DIPM_DISP | DSMR_CSPM;
>         rcar_du_crtc_write(rcrtc, DSMR, dsmr);
>  

This looks like the kind of place that could do with a comment
explaining what is going on.

> +       hdse_offset = 19;
> +       if (rcrtc->group->cmms_mask & BIT(rcrtc->index % 2))
> +               hdse_offset += 25;
> +
>         /* Display timings */
> -       rcar_du_crtc_write(rcrtc, HDSR, mode->htotal - mode->hsync_start - 19);
> +       rcar_du_crtc_write(rcrtc, HDSR, mode->htotal - mode->hsync_start -
> +                                       hdse_offset);
>         rcar_du_crtc_write(rcrtc, HDER, mode->htotal - mode->hsync_start +
> -                                       mode->hdisplay - 19);
> +                                       mode->hdisplay - hdse_offset);
>         rcar_du_crtc_write(rcrtc, HSWR, mode->hsync_end -
>                                         mode->hsync_start - 1);
>         rcar_du_crtc_write(rcrtc, HCR,  mode->htotal - 1);
> @@ -836,6 +842,7 @@ rcar_du_crtc_mode_valid(struct drm_crtc *crtc,
>         struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
>         struct rcar_du_device *rcdu = rcrtc->dev;
>         bool interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
> +       unsigned int min_sync_porch;
>         unsigned int vbp;
>  
>         if (interlaced && !rcar_du_has(rcdu, RCAR_DU_FEATURE_INTERLACED))
> @@ -843,9 +850,14 @@ rcar_du_crtc_mode_valid(struct drm_crtc *crtc,
>  
>         /*
>          * The hardware requires a minimum combined horizontal sync and back
> -        * porch of 20 pixels and a minimum vertical back porch of 3 lines.
> +        * porch of 20 pixels (when CMM isn't used) or 45 pixels (when CMM is
> +        * used), and a minimum vertical back porch of 3 lines.
>          */
> -       if (mode->htotal - mode->hsync_start < 20)
> +       min_sync_porch = 20;
> +       if (rcrtc->group->cmms_mask & BIT(rcrtc->index % 2))
> +               min_sync_porch += 25;
> +
> +       if (mode->htotal - mode->hsync_start < min_sync_porch)
>                 return MODE_HBLANK_NARROW;

Is the '19' in the hdse offset, this min_sync_port - 1 for position
correction? It looks something like that. And the rest seems ok.

With or without the additional optional comment suggestion above:

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>



>  
>         vbp = (mode->vtotal - mode->vsync_end) / (interlaced ? 2 : 1);
> 
> base-commit: c18c8891111bb5e014e144716044991112f16833
> prerequisite-patch-id: dc9121a1b85ea05bf3eae2b0ac2168d47101ee87
> -- 
> Regards,
> 
> Laurent Pinchart
>

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix CRTC timings when CMM is used
  2022-02-22 12:58 ` Kieran Bingham
@ 2022-02-22 15:05     ` Laurent Pinchart
  0 siblings, 0 replies; 4+ messages in thread
From: Laurent Pinchart @ 2022-02-22 15:05 UTC (permalink / raw)
  To: Kieran Bingham; +Cc: dri-devel, linux-renesas-soc

Hi Kieran,

On Tue, Feb 22, 2022 at 12:58:25PM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2021-11-29 22:28:13)
> > When the CMM is enabled, an offset of 25 pixels must be subtracted from
> > the HDS (horizontal display start) and HDE (horizontal display end)
> > registers. Fix the timings calculation, and take this into account in
> > the mode validation.
> > 
> > This fixes a visible horizontal offset in the image with VGA monitors.
> > HDMI monitors seem to be generally more tolerant to incorrect timings,
> > but may be affected too.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > index 5672830ca184..ee6ba74627a2 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -215,6 +215,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
> >         const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
> >         struct rcar_du_device *rcdu = rcrtc->dev;
> >         unsigned long mode_clock = mode->clock * 1000;
> > +       unsigned int hdse_offset;
> >         u32 dsmr;
> >         u32 escr;
> >  
> > @@ -298,10 +299,15 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
> >              | DSMR_DIPM_DISP | DSMR_CSPM;
> >         rcar_du_crtc_write(rcrtc, DSMR, dsmr);
> >  
> 
> This looks like the kind of place that could do with a comment
> explaining what is going on.

How does this sound ?

	/*
	 * When the CMM is enabled, an additional offset of 25 pixels must be
	 * subtracted from the HDS (horizontal display start) and HDE
	 * (horizontal display end) registers.
	 */

> > +       hdse_offset = 19;
> > +       if (rcrtc->group->cmms_mask & BIT(rcrtc->index % 2))
> > +               hdse_offset += 25;
> > +
> >         /* Display timings */
> > -       rcar_du_crtc_write(rcrtc, HDSR, mode->htotal - mode->hsync_start - 19);
> > +       rcar_du_crtc_write(rcrtc, HDSR, mode->htotal - mode->hsync_start -
> > +                                       hdse_offset);
> >         rcar_du_crtc_write(rcrtc, HDER, mode->htotal - mode->hsync_start +
> > -                                       mode->hdisplay - 19);
> > +                                       mode->hdisplay - hdse_offset);
> >         rcar_du_crtc_write(rcrtc, HSWR, mode->hsync_end -
> >                                         mode->hsync_start - 1);
> >         rcar_du_crtc_write(rcrtc, HCR,  mode->htotal - 1);
> > @@ -836,6 +842,7 @@ rcar_du_crtc_mode_valid(struct drm_crtc *crtc,
> >         struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> >         struct rcar_du_device *rcdu = rcrtc->dev;
> >         bool interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
> > +       unsigned int min_sync_porch;
> >         unsigned int vbp;
> >  
> >         if (interlaced && !rcar_du_has(rcdu, RCAR_DU_FEATURE_INTERLACED))
> > @@ -843,9 +850,14 @@ rcar_du_crtc_mode_valid(struct drm_crtc *crtc,
> >  
> >         /*
> >          * The hardware requires a minimum combined horizontal sync and back
> > -        * porch of 20 pixels and a minimum vertical back porch of 3 lines.
> > +        * porch of 20 pixels (when CMM isn't used) or 45 pixels (when CMM is
> > +        * used), and a minimum vertical back porch of 3 lines.
> >          */
> > -       if (mode->htotal - mode->hsync_start < 20)
> > +       min_sync_porch = 20;
> > +       if (rcrtc->group->cmms_mask & BIT(rcrtc->index % 2))
> > +               min_sync_porch += 25;
> > +
> > +       if (mode->htotal - mode->hsync_start < min_sync_porch)
> >                 return MODE_HBLANK_NARROW;
> 
> Is the '19' in the hdse offset, this min_sync_port - 1 for position
> correction? It looks something like that. And the rest seems ok.

See Table 35.31 ("Correspondence Table of Settings of Display Timing
Generation Registers"):

Horizontal display start register n (HDSRn) - hsw + xs - 19

with the following footnote:

HDS should be set to 1 or greater

HDS = hsw + xs - 19 >= 1
hsw + xs >= 20

> With or without the additional optional comment suggestion above:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> >         vbp = (mode->vtotal - mode->vsync_end) / (interlaced ? 2 : 1);
> > 
> > base-commit: c18c8891111bb5e014e144716044991112f16833
> > prerequisite-patch-id: dc9121a1b85ea05bf3eae2b0ac2168d47101ee87

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm: rcar-du: Fix CRTC timings when CMM is used
@ 2022-02-22 15:05     ` Laurent Pinchart
  0 siblings, 0 replies; 4+ messages in thread
From: Laurent Pinchart @ 2022-02-22 15:05 UTC (permalink / raw)
  To: Kieran Bingham; +Cc: linux-renesas-soc, dri-devel

Hi Kieran,

On Tue, Feb 22, 2022 at 12:58:25PM +0000, Kieran Bingham wrote:
> Quoting Laurent Pinchart (2021-11-29 22:28:13)
> > When the CMM is enabled, an offset of 25 pixels must be subtracted from
> > the HDS (horizontal display start) and HDE (horizontal display end)
> > registers. Fix the timings calculation, and take this into account in
> > the mode validation.
> > 
> > This fixes a visible horizontal offset in the image with VGA monitors.
> > HDMI monitors seem to be generally more tolerant to incorrect timings,
> > but may be affected too.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > index 5672830ca184..ee6ba74627a2 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> > @@ -215,6 +215,7 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
> >         const struct drm_display_mode *mode = &rcrtc->crtc.state->adjusted_mode;
> >         struct rcar_du_device *rcdu = rcrtc->dev;
> >         unsigned long mode_clock = mode->clock * 1000;
> > +       unsigned int hdse_offset;
> >         u32 dsmr;
> >         u32 escr;
> >  
> > @@ -298,10 +299,15 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc)
> >              | DSMR_DIPM_DISP | DSMR_CSPM;
> >         rcar_du_crtc_write(rcrtc, DSMR, dsmr);
> >  
> 
> This looks like the kind of place that could do with a comment
> explaining what is going on.

How does this sound ?

	/*
	 * When the CMM is enabled, an additional offset of 25 pixels must be
	 * subtracted from the HDS (horizontal display start) and HDE
	 * (horizontal display end) registers.
	 */

> > +       hdse_offset = 19;
> > +       if (rcrtc->group->cmms_mask & BIT(rcrtc->index % 2))
> > +               hdse_offset += 25;
> > +
> >         /* Display timings */
> > -       rcar_du_crtc_write(rcrtc, HDSR, mode->htotal - mode->hsync_start - 19);
> > +       rcar_du_crtc_write(rcrtc, HDSR, mode->htotal - mode->hsync_start -
> > +                                       hdse_offset);
> >         rcar_du_crtc_write(rcrtc, HDER, mode->htotal - mode->hsync_start +
> > -                                       mode->hdisplay - 19);
> > +                                       mode->hdisplay - hdse_offset);
> >         rcar_du_crtc_write(rcrtc, HSWR, mode->hsync_end -
> >                                         mode->hsync_start - 1);
> >         rcar_du_crtc_write(rcrtc, HCR,  mode->htotal - 1);
> > @@ -836,6 +842,7 @@ rcar_du_crtc_mode_valid(struct drm_crtc *crtc,
> >         struct rcar_du_crtc *rcrtc = to_rcar_crtc(crtc);
> >         struct rcar_du_device *rcdu = rcrtc->dev;
> >         bool interlaced = mode->flags & DRM_MODE_FLAG_INTERLACE;
> > +       unsigned int min_sync_porch;
> >         unsigned int vbp;
> >  
> >         if (interlaced && !rcar_du_has(rcdu, RCAR_DU_FEATURE_INTERLACED))
> > @@ -843,9 +850,14 @@ rcar_du_crtc_mode_valid(struct drm_crtc *crtc,
> >  
> >         /*
> >          * The hardware requires a minimum combined horizontal sync and back
> > -        * porch of 20 pixels and a minimum vertical back porch of 3 lines.
> > +        * porch of 20 pixels (when CMM isn't used) or 45 pixels (when CMM is
> > +        * used), and a minimum vertical back porch of 3 lines.
> >          */
> > -       if (mode->htotal - mode->hsync_start < 20)
> > +       min_sync_porch = 20;
> > +       if (rcrtc->group->cmms_mask & BIT(rcrtc->index % 2))
> > +               min_sync_porch += 25;
> > +
> > +       if (mode->htotal - mode->hsync_start < min_sync_porch)
> >                 return MODE_HBLANK_NARROW;
> 
> Is the '19' in the hdse offset, this min_sync_port - 1 for position
> correction? It looks something like that. And the rest seems ok.

See Table 35.31 ("Correspondence Table of Settings of Display Timing
Generation Registers"):

Horizontal display start register n (HDSRn) - hsw + xs - 19

with the following footnote:

HDS should be set to 1 or greater

HDS = hsw + xs - 19 >= 1
hsw + xs >= 20

> With or without the additional optional comment suggestion above:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> >         vbp = (mode->vtotal - mode->vsync_end) / (interlaced ? 2 : 1);
> > 
> > base-commit: c18c8891111bb5e014e144716044991112f16833
> > prerequisite-patch-id: dc9121a1b85ea05bf3eae2b0ac2168d47101ee87

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-02-22 15:05 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-29 22:28 [PATCH] drm: rcar-du: Fix CRTC timings when CMM is used Laurent Pinchart
2022-02-22 12:58 ` Kieran Bingham
2022-02-22 15:05   ` Laurent Pinchart
2022-02-22 15:05     ` Laurent Pinchart

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.