Re: [PATCH] drm: meson: venc: cvbs: fix CVBS mode matching

[Neil Armstrong]

Hi Martin,

On 30/11/2019 23:25, Martin Blumenstingl wrote:
> Drop the picture_aspect_ratio from the drm_display_modes which are valid
> for the Amlogic Meson CVBS encoder. meson_venc_cvbs_encoder_atomic_check
> and meson_venc_cvbs_encoder_mode_set only support two very specific
> drm_display_modes.
> 
> With commit 222ec1618c3ace ("drm: Add aspect ratio parsing in DRM
> layer") the drm core started honoring the picture_aspect_ratio field
> when comparing two drm_display_modes. Prior to that it was ignored.
> When the CVBS encoder driver was initially submitted there was no aspect
> ratio check.
> 
> This patch fixes "kmscube" and X.org output using the CVBS connector
> with the Amlogic Meson VPU driver. Prior to this patch kmscube reported:
>   failed to set mode: Invalid argument
> Additionally it makes the CVBS mode checking behave identical to the
> sun4i (drivers/gpu/drm/sun4i/sun4i_tv.c sun4i_tv_mode_to_drm_mode) and
> ZTE (drivers/gpu/drm/zte/zx_tvenc.c tvenc_mode_{pal,ntsc}) which are
> both not setting "picture_aspect_ratio" either.
> 
> Fixes: 222ec1618c3ace ("drm: Add aspect ratio parsing in DRM layer")
> Fixes: bbbe775ec5b5da ("drm: Add support for Amlogic Meson Graphic Controller")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/meson/meson_venc_cvbs.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_venc_cvbs.c b/drivers/gpu/drm/meson/meson_venc_cvbs.c
> index 9ab27aecfcf3..2ddcda8fa5b0 100644
> --- a/drivers/gpu/drm/meson/meson_venc_cvbs.c
> +++ b/drivers/gpu/drm/meson/meson_venc_cvbs.c
> @@ -49,7 +49,6 @@ struct meson_cvbs_mode meson_cvbs_modes[MESON_CVBS_MODES_COUNT] = {
>  				 720, 732, 795, 864, 0, 576, 580, 586, 625, 0,
>  				 DRM_MODE_FLAG_INTERLACE),
>  			.vrefresh = 50,
> -			.picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3,
>  		},
>  	},
>  	{ /* NTSC */
> @@ -59,7 +58,6 @@ struct meson_cvbs_mode meson_cvbs_modes[MESON_CVBS_MODES_COUNT] = {
>  				720, 739, 801, 858, 0, 480, 488, 494, 525, 0,
>  				DRM_MODE_FLAG_INTERLACE),
>  			.vrefresh = 60,
> -			.picture_aspect_ratio = HDMI_PICTURE_ASPECT_4_3,
>  		},
>  	},
>  };
> 

Thanks for finding this issue !!

I would rather prefer changing the drm_mode_equal to drm_mode_match without DRM_MODE_MATCH_ASPECT_RATIO.

Neil