Re: [PATCH 2/2] drm/meson: vclk: fix precision in vclk calculations

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4289 bytes --]

Hi,

On Fri, Jan 10, 2025 at 07:44:58AM +0000, Christian Hewitt wrote:
> Playing YUV420 @ 59.94 media causes HDMI output to lose sync
> with a fatal error reported:
> 
> [   89.610280] Fatal Error, invalid HDMI vclk freq 593406
> 
> In meson_encoder_hdmi_set_vclk the initial vclk_freq value is
> 593407 but YUV420 modes halve the value to 296703.5 and this
> is stored as int which loses precision by rounding down to
> 296703. The rounded value is later doubled to 593406 and then
> meson_encoder_hdmi_set_vclk sets an invalid vclk_freq value
> and the error triggers during meson_vlkc_setup validation.
> 
> Fix precision in meson_encoder_hdmi_set_vclk by switching to
> unsigned long long KHz values instead of int MHz. As values
> for phy_freq are now more accurate we also need to handle an
> additional match scenario in meson_vclk_setup.
> 
> Fixes: e5fab2ec9ca4 ("drm/meson: vclk: add support for YUV420 setup")
> Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
> ---
>  drivers/gpu/drm/meson/meson_encoder_hdmi.c | 42 +++++++++++-----------
>  drivers/gpu/drm/meson/meson_vclk.c         |  3 +-
>  2 files changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_encoder_hdmi.c b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> index 0593a1cde906..fa37cf975992 100644
> --- a/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> +++ b/drivers/gpu/drm/meson/meson_encoder_hdmi.c
> @@ -70,12 +70,12 @@ static void meson_encoder_hdmi_set_vclk(struct meson_encoder_hdmi *encoder_hdmi,
>  {
>  	struct meson_drm *priv = encoder_hdmi->priv;
>  	int vic = drm_match_cea_mode(mode);
> -	unsigned int phy_freq;
> -	unsigned int vclk_freq;
> -	unsigned int venc_freq;
> -	unsigned int hdmi_freq;
> +	unsigned long long vclk_freq;
> +	unsigned long long phy_freq;
> +	unsigned long long venc_freq;
> +	unsigned long long hdmi_freq;
>  
> -	vclk_freq = mode->clock;
> +	vclk_freq = mode->clock * 1000ULL;

You should be using drm_hdmi_compute_mode_clock() here



>  	/* For 420, pixel clock is half unlike venc clock */
>  	if (encoder_hdmi->output_bus_fmt == MEDIA_BUS_FMT_UYYVYY8_0_5X24)
> @@ -85,8 +85,9 @@ static void meson_encoder_hdmi_set_vclk(struct meson_encoder_hdmi *encoder_hdmi,
>  	phy_freq = vclk_freq * 10;
>  
>  	if (!vic) {
> -		meson_vclk_setup(priv, MESON_VCLK_TARGET_DMT, phy_freq,
> -				 vclk_freq, vclk_freq, vclk_freq, false);
> +		meson_vclk_setup(priv, MESON_VCLK_TARGET_DMT, phy_freq / 1000ULL,
> +				 vclk_freq / 1000ULL, vclk_freq / 1000ULL,
> +				 vclk_freq / 1000ULL, false);
>  		return;
>  	}
>  
> @@ -107,12 +108,9 @@ static void meson_encoder_hdmi_set_vclk(struct meson_encoder_hdmi *encoder_hdmi,
>  	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
>  		venc_freq /= 2;
>  
> -	dev_dbg(priv->dev, "vclk:%d phy=%d venc=%d hdmi=%d enci=%d\n",
> -		phy_freq, vclk_freq, venc_freq, hdmi_freq,
> -		priv->venc.hdmi_use_enci);
> -
> -	meson_vclk_setup(priv, MESON_VCLK_TARGET_HDMI, phy_freq, vclk_freq,
> -			 venc_freq, hdmi_freq, priv->venc.hdmi_use_enci);
> +	meson_vclk_setup(priv, MESON_VCLK_TARGET_HDMI, phy_freq / 1000ULL,
> +			 vclk_freq / 1000ULL, venc_freq / 1000ULL, hdmi_freq / 1000ULL,
> +			 priv->venc.hdmi_use_enci);
>  }
>  
>  static enum drm_mode_status meson_encoder_hdmi_mode_valid(struct drm_bridge *bridge,
> @@ -122,10 +120,10 @@ static enum drm_mode_status meson_encoder_hdmi_mode_valid(struct drm_bridge *bri
>  	struct meson_encoder_hdmi *encoder_hdmi = bridge_to_meson_encoder_hdmi(bridge);
>  	struct meson_drm *priv = encoder_hdmi->priv;
>  	bool is_hdmi2_sink = display_info->hdmi.scdc.supported;
> -	unsigned int phy_freq;
> -	unsigned int vclk_freq;
> -	unsigned int venc_freq;
> -	unsigned int hdmi_freq;
> +	unsigned long long vclk_freq;
> +	unsigned long long phy_freq;
> +	unsigned long long venc_freq;
> +	unsigned long long hdmi_freq;
>  	int vic = drm_match_cea_mode(mode);
>  	enum drm_mode_status status;
>  
> @@ -149,7 +147,7 @@ static enum drm_mode_status meson_encoder_hdmi_mode_valid(struct drm_bridge *bri
>  	} else if (!meson_venc_hdmi_supported_vic(vic))
>  		return MODE_BAD;
>  
> -	vclk_freq = mode->clock;
> +	vclk_freq = mode->clock * 1000ULL;

And here too.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic