[PATCH] drm: rcar-du: Fix CRTC timings when CMM is used

[Kieran Bingham]

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 at 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 at ideasonboard.com>



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