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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <048FF805-BF7E-4D1B-B522-E783415057BD@gmail.com>
Date: Fri, 10 Jan 2025 13:35:30 +0400
From: Christian Hewitt <christianshewitt@...il.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Neil Armstrong <neil.armstrong@...aro.org>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>,
 Kevin Hilman <khilman@...libre.com>,
 Jerome Brunet <jbrunet@...libre.com>,
 Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
 Jernej Skrabec <jernej.skrabec@...il.com>,
 dri-devel@...ts.freedesktop.org,
 linux-amlogic@...ts.infradead.org,
 linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] drm/meson: vclk: fix precision in vclk calculations

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