| Message ID | 20250110074458.3624094-3-christianshewitt@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/meson: vclk: revert and re-fix vclk calculations | expand |
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
> 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
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
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)
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(-)