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@xxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx>
>
> > vbp = (mode->vtotal - mode->vsync_end) / (interlaced ? 2 : 1);
> >
> > base-commit: c18c8891111bb5e014e144716044991112f16833
> > prerequisite-patch-id: dc9121a1b85ea05bf3eae2b0ac2168d47101ee87
--
Regards,
Laurent Pinchart
