[PATCH 0/2] drm/meson: vclk: revert and re-fix vclk calculations

[PATCH 0/2] drm/meson: vclk: revert and re-fix vclk calculations

[Christian Hewitt]

Patch 1 reverts a previous fix for loss of HDMI sync when playing YUV420
@ 59.94 media. The patch does resolve a calculation issue. It also makes
all fractional rates invalid which is a bigger problem.

Patch 2 provides a proper fix after I figured out the actual root cause
of the original problem. This time the maths checks out.

Apologies to the stable team as the original patch has been applied to
a wide range of older and LTS kernels (as far as 5.10). Please let me
know if combining the two patches into a single 'fix the previous fix'
patch would be preferred?

Christian

Christian Hewitt (2):
  Revert "drm/meson: vclk: fix calculation of 59.94 fractional rates"
  drm/meson: vclk: fix precision in vclk calculations

 drivers/gpu/drm/meson/meson_encoder_hdmi.c | 42 +++++++++++-----------
 drivers/gpu/drm/meson/meson_vclk.c         |  7 ++--
 2 files changed, 25 insertions(+), 24 deletions(-)

-- 
2.34.1

[PATCH 1/2] Revert "drm/meson: vclk: fix calculation of 59.94 fractional rates"

[Christian Hewitt]

This reverts commit bfbc68e4d8695497f858a45a142665e22a512ea3.

The patch does permit the offending YUV420 @ 59.94 phy_freq and
vclk_freq mode to match in calculations. It also results in all
fractional rates being unavailable for use. This was unintended
and requires the patch to be reverted.

Cc: <stable@vger.kernel.org>
Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
---
 drivers/gpu/drm/meson/meson_vclk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/meson/meson_vclk.c b/drivers/gpu/drm/meson/meson_vclk.c
index 2a942dc6a6dc..2a82119eb58e 100644
--- a/drivers/gpu/drm/meson/meson_vclk.c
+++ b/drivers/gpu/drm/meson/meson_vclk.c
@@ -790,13 +790,13 @@ meson_vclk_vic_supported_freq(struct meson_drm *priv, unsigned int phy_freq,
 				 FREQ_1000_1001(params[i].pixel_freq));
 		DRM_DEBUG_DRIVER("i = %d phy_freq = %d alt = %d\n",
 				 i, params[i].phy_freq,
-				 FREQ_1000_1001(params[i].phy_freq/1000)*1000);
+				 FREQ_1000_1001(params[i].phy_freq/10)*10);
 		/* 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 == (FREQ_1000_1001(params[i].phy_freq/1000)*1000) &&
+		if (phy_freq == (FREQ_1000_1001(params[i].phy_freq/10)*10) &&
 		    vclk_freq == FREQ_1000_1001(params[i].vclk_freq))
 			return MODE_OK;
 	}
@@ -1070,7 +1070,7 @@ 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 == FREQ_1000_1001(params[freq].phy_freq/1000)*1000) &&
+		     phy_freq == FREQ_1000_1001(params[freq].phy_freq/10)*10) &&
 		    (vclk_freq == params[freq].vclk_freq ||
 		     vclk_freq == FREQ_1000_1001(params[freq].vclk_freq))) {
 			if (vclk_freq != params[freq].vclk_freq)
-- 
2.34.1

Re: [PATCH 1/2] Revert "drm/meson: vclk: fix calculation of 59.94 fractional rates"

[Neil Armstrong]

On 10/01/2025 08:44, Christian Hewitt wrote:
> This reverts commit bfbc68e4d8695497f858a45a142665e22a512ea3.
> 
> The patch does permit the offending YUV420 @ 59.94 phy_freq and
> vclk_freq mode to match in calculations. It also results in all
> fractional rates being unavailable for use. This was unintended
> and requires the patch to be reverted.
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>
> ---
>   drivers/gpu/drm/meson/meson_vclk.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/meson/meson_vclk.c b/drivers/gpu/drm/meson/meson_vclk.c
> index 2a942dc6a6dc..2a82119eb58e 100644
> --- a/drivers/gpu/drm/meson/meson_vclk.c
> +++ b/drivers/gpu/drm/meson/meson_vclk.c
> @@ -790,13 +790,13 @@ meson_vclk_vic_supported_freq(struct meson_drm *priv, unsigned int phy_freq,
>   				 FREQ_1000_1001(params[i].pixel_freq));
>   		DRM_DEBUG_DRIVER("i = %d phy_freq = %d alt = %d\n",
>   				 i, params[i].phy_freq,
> -				 FREQ_1000_1001(params[i].phy_freq/1000)*1000);
> +				 FREQ_1000_1001(params[i].phy_freq/10)*10);
>   		/* 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 == (FREQ_1000_1001(params[i].phy_freq/1000)*1000) &&
> +		if (phy_freq == (FREQ_1000_1001(params[i].phy_freq/10)*10) &&
>   		    vclk_freq == FREQ_1000_1001(params[i].vclk_freq))
>   			return MODE_OK;
>   	}
> @@ -1070,7 +1070,7 @@ 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 == FREQ_1000_1001(params[freq].phy_freq/1000)*1000) &&
> +		     phy_freq == FREQ_1000_1001(params[freq].phy_freq/10)*10) &&
>   		    (vclk_freq == params[freq].vclk_freq ||
>   		     vclk_freq == FREQ_1000_1001(params[freq].vclk_freq))) {
>   			if (vclk_freq != params[freq].vclk_freq)

I wonder if a Fixes is also required here

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

[Christian Hewitt]

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;
 
 	/* 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;
 
 	/* For 420, pixel clock is half unlike venc clock */
 	if (drm_mode_is_420_only(display_info, mode) ||
@@ -179,10 +177,12 @@ static enum drm_mode_status meson_encoder_hdmi_mode_valid(struct drm_bridge *bri
 	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
 		venc_freq /= 2;
 
-	dev_dbg(priv->dev, "%s: vclk:%d phy=%d venc=%d hdmi=%d\n",
-		__func__, phy_freq, vclk_freq, venc_freq, hdmi_freq);
+	dev_dbg(priv->dev, "%s: phy=%lld vclk=%lld venc=%lld hdmi=%lld\n",
+		__func__, phy_freq / 1000ULL, vclk_freq / 1000ULL,
+		venc_freq / 1000ULL, hdmi_freq / 1000ULL);
 
-	return meson_vclk_vic_supported_freq(priv, phy_freq, vclk_freq);
+	return meson_vclk_vic_supported_freq(priv, phy_freq / 1000ULL,
+					     vclk_freq / 1000ULL);
 }
 
 static void meson_encoder_hdmi_atomic_enable(struct drm_bridge *bridge,
diff --git a/drivers/gpu/drm/meson/meson_vclk.c b/drivers/gpu/drm/meson/meson_vclk.c
index 2a82119eb58e..a2e93b500ed6 100644
--- a/drivers/gpu/drm/meson/meson_vclk.c
+++ b/drivers/gpu/drm/meson/meson_vclk.c
@@ -1070,7 +1070,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 == FREQ_1000_1001(params[freq].phy_freq/10)*10) &&
+		     phy_freq == FREQ_1000_1001(params[freq].phy_freq/10)*10 ||
+		   ((phy_freq/10)*10) == FREQ_1000_1001(params[freq].phy_freq/10)*10) &&
 		    (vclk_freq == params[freq].vclk_freq ||
 		     vclk_freq == FREQ_1000_1001(params[freq].vclk_freq))) {
 			if (vclk_freq != params[freq].vclk_freq)
-- 
2.34.1

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

[Maxime Ripard]

[-- Attachment #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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Christian Hewitt]

> On 10 Jan 2025, at 12:36 pm, Maxime Ripard <mripard@kernel.org> wrote:
> 
> 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

Hello Maxime,

If we’re talking a one-two line change than I’d like pointers
and guidance on how to go about that (offline to avoid group
noise). I have only rudimentary c skills though, so anything
that looks like replumbing a driver around drm_connector is
way beyond my current drm code and general coding knowledge;
I can only offer a fix not a proper improvement :(

Christian

> /* 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

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

[Neil Armstrong]

On 10/01/2025 09:36, Maxime Ripard wrote:
> 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

Yes in a second time, this simply fixes the actual calculations so it can be
backported to stable kernel to make the feature work again.

And yes at some point we should switch to hdmi helpers in a separate forward
changeset.

Neil

> 
> 
> 
>>   	/* 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

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

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.13 next-20250120]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Hewitt/Revert-drm-meson-vclk-fix-calculation-of-59-94-fractional-rates/20250110-154701
base:   linus/master
patch link:    https://lore.kernel.org/r/20250110074458.3624094-3-christianshewitt%40gmail.com
patch subject: [PATCH 2/2] drm/meson: vclk: fix precision in vclk calculations
config: arm-randconfig-r072-20250118 (https://download.01.org/0day-ci/archive/20250121/202501210513.GCec6kts-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project c23f2417dc5f6dc371afb07af5627ec2a9d373a0)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250121/202501210513.GCec6kts-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501210513.GCec6kts-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

WARNING: modpost: missing MODULE_DESCRIPTION() in arch/arm/probes/kprobes/test-kprobes.o
WARNING: modpost: missing MODULE_DESCRIPTION() in lib/slub_kunit.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/gpib/fmh_gpib/fmh_gpib.o
>> ERROR: modpost: "__aeabi_uldivmod" [drivers/gpu/drm/meson/meson-drm.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Neil Armstrong]

Hi,

On 20/01/2025 22:56, kernel test robot wrote:
> Hi Christian,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on linus/master]
> [also build test ERROR on v6.13 next-20250120]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Hewitt/Revert-drm-meson-vclk-fix-calculation-of-59-94-fractional-rates/20250110-154701
> base:   linus/master
> patch link:    https://lore.kernel.org/r/20250110074458.3624094-3-christianshewitt%40gmail.com
> patch subject: [PATCH 2/2] drm/meson: vclk: fix precision in vclk calculations
> config: arm-randconfig-r072-20250118 (https://download.01.org/0day-ci/archive/20250121/202501210513.GCec6kts-lkp@intel.com/config)
> compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project c23f2417dc5f6dc371afb07af5627ec2a9d373a0)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250121/202501210513.GCec6kts-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202501210513.GCec6kts-lkp@intel.com/
> 
> All errors (new ones prefixed by >>, old ones prefixed by <<):
> 
> WARNING: modpost: missing MODULE_DESCRIPTION() in arch/arm/probes/kprobes/test-kprobes.o
> WARNING: modpost: missing MODULE_DESCRIPTION() in lib/slub_kunit.o
> WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/gpib/fmh_gpib/fmh_gpib.o
>>> ERROR: modpost: "__aeabi_uldivmod" [drivers/gpu/drm/meson/meson-drm.ko] undefined!
> 

Please use do_div() when dividing 64bit numbers to please 32-bit platforms.

Neil

Re: [PATCH 0/2] drm/meson: vclk: revert and re-fix vclk calculations

[Neil Armstrong]

On 10/01/2025 08:44, Christian Hewitt wrote:
> Patch 1 reverts a previous fix for loss of HDMI sync when playing YUV420
> @ 59.94 media. The patch does resolve a calculation issue. It also makes
> all fractional rates invalid which is a bigger problem.
> 
> Patch 2 provides a proper fix after I figured out the actual root cause
> of the original problem. This time the maths checks out.
> 
> Apologies to the stable team as the original patch has been applied to
> a wide range of older and LTS kernels (as far as 5.10). Please let me
> know if combining the two patches into a single 'fix the previous fix'
> patch would be preferred?
> 
> Christian
> 
> Christian Hewitt (2):
>    Revert "drm/meson: vclk: fix calculation of 59.94 fractional rates"
>    drm/meson: vclk: fix precision in vclk calculations
> 
>   drivers/gpu/drm/meson/meson_encoder_hdmi.c | 42 +++++++++++-----------
>   drivers/gpu/drm/meson/meson_vclk.c         |  7 ++--
>   2 files changed, 25 insertions(+), 24 deletions(-)
> 

LGTM

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