Re: [PATCH v1] drm/meson: fix more rounding issues with 59.94Hz modes

[neil.armstrong@linaro.org]

On 09/06/2025 22:27, Martin Blumenstingl wrote:
> Commit 1017560164b6 ("drm/meson: use unsigned long long / Hz for
> frequency types") attempts to resolve video playback using 59.94Hz.
>   using YUV420 by changing the clock calculation to use
> Hz instead of kHz (thus yielding more precision).
> 
> The basic calculation itself is correct, however the comparisions in
> meson_vclk_vic_supported_freq() and meson_vclk_setup() don't work
> anymore for 59.94Hz modes (using the freq * 1000 / 1001 logic). For
> example, drm/edid specifies a 593407kHz clock for 3840x2160@59.94Hz.
> With the mentioend commit we convert this to Hz. Then meson_vclk
> tries to find a matchig "params" entry (as the clock setup code
> currently only supports specific frequencies) by taking the venc_freq
> from the params and calculating the "alt frequency" (used for the
> 59.94Hz modes) from it, which is:
>    (594000000Hz * 1000) / 1001 = 593406593Hz
> 
> Similar calculation is applied to the phy_freq (TMDS clock), which is 10
> times the pixel clock.
> 
> Implement a new meson_vclk_freqs_are_matching_param() function whose
> purpose is to compare if the requested and calculated frequencies. They
> may not match exactly (for the reasons mentioned above). Allow the
> clocks to deviate slightly to make the 59.94Hz modes again.
> 
> Fixes: 1017560164b6 ("drm/meson: use unsigned long long / Hz for frequency types")
> Reported-by: Christian Hewitt <christianshewitt@gmail.com>
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> Special thanks to Christian for testing (off-list) and managing so I
> can do better testing myself in the future!
> 
> This is meant to be applied on top of "drm/meson: use vclk_freq instead
> of pixel_freq in debug print" from [0]
> 
> 
> [0] https://lore.kernel.org/dri-devel/20250606221031.3419353-1-martin.blumenstingl@googlemail.com/
> 
> 
>   drivers/gpu/drm/meson/meson_vclk.c | 55 ++++++++++++++++++------------
>   1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_vclk.c b/drivers/gpu/drm/meson/meson_vclk.c
> index c4123bb958e4..dfe0c28a0f05 100644
> --- a/drivers/gpu/drm/meson/meson_vclk.c
> +++ b/drivers/gpu/drm/meson/meson_vclk.c
> @@ -110,10 +110,7 @@
>   #define HDMI_PLL_LOCK		BIT(31)
>   #define HDMI_PLL_LOCK_G12A	(3 << 30)
>   
> -#define PIXEL_FREQ_1000_1001(_freq)	\
> -	DIV_ROUND_CLOSEST_ULL((_freq) * 1000ULL, 1001ULL)
> -#define PHY_FREQ_1000_1001(_freq)	\
> -	(PIXEL_FREQ_1000_1001(DIV_ROUND_DOWN_ULL(_freq, 10ULL)) * 10)
> +#define FREQ_1000_1001(_freq)	DIV_ROUND_CLOSEST_ULL((_freq) * 1000ULL, 1001ULL)
>   
>   /* VID PLL Dividers */
>   enum {
> @@ -772,6 +769,36 @@ static void meson_hdmi_pll_generic_set(struct meson_drm *priv,
>   		  pll_freq);
>   }
>   
> +static bool meson_vclk_freqs_are_matching_param(unsigned int idx,
> +						unsigned long long phy_freq,
> +						unsigned long long vclk_freq)
> +{
> +	DRM_DEBUG_DRIVER("i = %d vclk_freq = %lluHz alt = %lluHz\n",
> +			 idx, params[idx].vclk_freq,
> +			 FREQ_1000_1001(params[idx].vclk_freq));
> +	DRM_DEBUG_DRIVER("i = %d phy_freq = %lluHz alt = %lluHz\n",
> +			 idx, params[idx].phy_freq,
> +			 FREQ_1000_1001(params[idx].phy_freq));
> +
> +	/* Match strict frequency */
> +	if (phy_freq == params[idx].phy_freq &&
> +	    vclk_freq == params[idx].vclk_freq)
> +		return true;
> +
> +	/* Match 1000/1001 variant: vclk deviation has to be less than 1kHz
> +	 * (drm EDID is defined in 1kHz steps, so everything smaller must be
> +	 * rounding error) and the PHY freq deviation has to be less than
> +	 * 10kHz (as the TMDS clock is 10 times the pixel clock, so anything
> +	 * smaller must be rounding error as well).
> +	 */
> +	if (abs(vclk_freq - FREQ_1000_1001(params[idx].vclk_freq)) < 1000 &&
> +	    abs(phy_freq - FREQ_1000_1001(params[idx].phy_freq)) < 10000)
> +		return true;
> +
> +	/* no match */
> +	return false;
> +}
> +
>   enum drm_mode_status
>   meson_vclk_vic_supported_freq(struct meson_drm *priv,
>   			      unsigned long long phy_freq,
> @@ -790,19 +817,7 @@ meson_vclk_vic_supported_freq(struct meson_drm *priv,
>   	}
>   
>   	for (i = 0 ; params[i].pixel_freq ; ++i) {
> -		DRM_DEBUG_DRIVER("i = %d vclk_freq = %lluHz alt = %lluHz\n",
> -				 i, params[i].vclk_freq,
> -				 PIXEL_FREQ_1000_1001(params[i].vclk_freq));
> -		DRM_DEBUG_DRIVER("i = %d phy_freq = %lluHz alt = %lluHz\n",
> -				 i, params[i].phy_freq,
> -				 PHY_FREQ_1000_1001(params[i].phy_freq));
> -		/* Match strict frequency */
> -		if (phy_freq == params[i].phy_freq &&
> -		    vclk_freq == params[i].vclk_freq)
> -			return MODE_OK;
> -		/* Match 1000/1001 variant */
> -		if (phy_freq == PHY_FREQ_1000_1001(params[i].phy_freq) &&
> -		    vclk_freq == PIXEL_FREQ_1000_1001(params[i].vclk_freq))
> +		if (meson_vclk_freqs_are_matching_param(i, phy_freq, vclk_freq))
>   			return MODE_OK;
>   	}
>   
> @@ -1075,10 +1090,8 @@ void meson_vclk_setup(struct meson_drm *priv, unsigned int target,
>   	}
>   
>   	for (freq = 0 ; params[freq].pixel_freq ; ++freq) {
> -		if ((phy_freq == params[freq].phy_freq ||
> -		     phy_freq == PHY_FREQ_1000_1001(params[freq].phy_freq)) &&
> -		    (vclk_freq == params[freq].vclk_freq ||
> -		     vclk_freq == PIXEL_FREQ_1000_1001(params[freq].vclk_freq))) {
> +		if (meson_vclk_freqs_are_matching_param(freq, phy_freq,
> +							vclk_freq)) {
>   			if (vclk_freq != params[freq].vclk_freq)
>   				vic_alternate_clock = true;
>   			else

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>