| 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
| ||
|
Message-ID: <bensvtxc67i566qqcjketdlffyrwxcnydwarqyjau6b7ibcq4b@d6d4sbm3rubf>
Date: Mon, 10 Mar 2025 22:14:52 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Maxime Ripard <mripard@...nel.org>
Cc: Dmitry Baryshkov <lumag@...nel.org>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Rob Clark <robdclark@...il.com>,
Abhinav Kumar <quic_abhinavk@...cinc.com>, Sean Paul <sean@...rly.run>,
Marijn Suijten <marijn.suijten@...ainline.org>, Dave Stevenson <dave.stevenson@...pberrypi.com>,
Maíra Canal <mcanal@...lia.com>, Raspberry Pi Kernel Maintenance <kernel-list@...pberrypi.com>,
Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong <neil.armstrong@...aro.org>,
Robert Foss <rfoss@...nel.org>, Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
Jonas Karlman <jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
freedreno@...ts.freedesktop.org
Subject: Re: [PATCH 1/4] drm/display: hdmi: provide central data authority
for ACR params
On Mon, Mar 10, 2025 at 03:46:33PM +0100, Maxime Ripard wrote:
> On Sun, Mar 09, 2025 at 10:13:56AM +0200, Dmitry Baryshkov wrote:
> > From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> >
> > HDMI standard defines recommended N and CTS values for Audio Clock
> > Regeneration. Currently each driver implements those, frequently in
> > somewhat unique way. Provide a generic helper for getting those values
> > to be used by the HDMI drivers.
> >
> > The helper is added to drm_hdmi_helper.c rather than drm_hdmi_audio.c
> > since HDMI drivers can be using this helper function even without
> > switching to DRM HDMI Audio helpers.
> >
> > Note: currently this only handles the values per HDMI 1.4b Section 7.2
> > and HDMI 2.0 Section 9.2.1. Later the table can be expanded to
> > accommodate for Deep Color TMDS char rates per HDMI 1.4 Appendix D
> > and/or HDMI 2.0 / 2.1 Appendix C).
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> > ---
> > drivers/gpu/drm/display/drm_hdmi_helper.c | 164 ++++++++++++++++++++++++++++++
> > include/drm/display/drm_hdmi_helper.h | 6 ++
> > 2 files changed, 170 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > index 74dd4d01dd9bb2c9e69ec1c60b0056bd69417e8a..89d25571bfd21c56c6835821d2272a12c816a76e 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > @@ -256,3 +256,167 @@ drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
> > return DIV_ROUND_CLOSEST_ULL(clock * bpc, 8);
> > }
> > EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
> > +
> > +struct drm_hdmi_acr_n_cts_entry {
> > + unsigned int n;
> > + unsigned int cts;
> > +};
> > +
> > +struct drm_hdmi_acr_data {
> > + unsigned long tmds_clock_khz;
> > + struct drm_hdmi_acr_n_cts_entry n_cts_32k,
> > + n_cts_44k1,
> > + n_cts_48k;
> > +};
> > +
> > +static const struct drm_hdmi_acr_data hdmi_acr_n_cts[] = {
> > + {
> > + /* "Other" entry */
> > + .n_cts_32k = { .n = 4096, },
> > + .n_cts_44k1 = { .n = 6272, },
> > + .n_cts_48k = { .n = 6144, },
> > + }, {
> > + .tmds_clock_khz = 25175,
> > + .n_cts_32k = { .n = 4576, .cts = 28125, },
> > + .n_cts_44k1 = { .n = 7007, .cts = 31250, },
> > + .n_cts_48k = { .n = 6864, .cts = 28125, },
> > + }, {
> > + .tmds_clock_khz = 25200,
> > + .n_cts_32k = { .n = 4096, .cts = 25200, },
> > + .n_cts_44k1 = { .n = 6272, .cts = 28000, },
> > + .n_cts_48k = { .n = 6144, .cts = 25200, },
> > + }, {
> > + .tmds_clock_khz = 27000,
> > + .n_cts_32k = { .n = 4096, .cts = 27000, },
> > + .n_cts_44k1 = { .n = 6272, .cts = 30000, },
> > + .n_cts_48k = { .n = 6144, .cts = 27000, },
> > + }, {
> > + .tmds_clock_khz = 27027,
> > + .n_cts_32k = { .n = 4096, .cts = 27027, },
> > + .n_cts_44k1 = { .n = 6272, .cts = 30030, },
> > + .n_cts_48k = { .n = 6144, .cts = 27027, },
> > + }, {
> > + .tmds_clock_khz = 54000,
> > + .n_cts_32k = { .n = 4096, .cts = 54000, },
> > + .n_cts_44k1 = { .n = 6272, .cts = 60000, },
> > + .n_cts_48k = { .n = 6144, .cts = 54000, },
> > + }, {
> > + .tmds_clock_khz = 54054,
> > + .n_cts_32k = { .n = 4096, .cts = 54054, },
> > + .n_cts_44k1 = { .n = 6272, .cts = 60060, },
> > + .n_cts_48k = { .n = 6144, .cts = 54054, },
> > + }, {
> > + .tmds_clock_khz = 74176,
> > + .n_cts_32k = { .n = 11648, .cts = 210937, }, /* and 210938 */
> > + .n_cts_44k1 = { .n = 17836, .cts = 234375, },
> > + .n_cts_48k = { .n = 11648, .cts = 140625, },
> > + }, {
> > + .tmds_clock_khz = 74250,
> > + .n_cts_32k = { .n = 4096, .cts = 74250, },
> > + .n_cts_44k1 = { .n = 6272, .cts = 82500, },
> > + .n_cts_48k = { .n = 6144, .cts = 74250, },
> > + }, {
> > + .tmds_clock_khz = 148352,
> > + .n_cts_32k = { .n = 11648, .cts = 421875, },
> > + .n_cts_44k1 = { .n = 8918, .cts = 234375, },
> > + .n_cts_48k = { .n = 5824, .cts = 140625, },
> > + }, {
> > + .tmds_clock_khz = 148500,
> > + .n_cts_32k = { .n = 4096, .cts = 148500, },
> > + .n_cts_44k1 = { .n = 6272, .cts = 165000, },
> > + .n_cts_48k = { .n = 6144, .cts = 148500, },
> > + }, {
> > + .tmds_clock_khz = 296703,
> > + .n_cts_32k = { .n = 5824, .cts = 421875, },
> > + .n_cts_44k1 = { .n = 4459, .cts = 234375, },
> > + .n_cts_48k = { .n = 5824, .cts = 281250, },
> > + }, {
> > + .tmds_clock_khz = 297000,
> > + .n_cts_32k = { .n = 3072, .cts = 222750, },
> > + .n_cts_44k1 = { .n = 4704, .cts = 247500, },
> > + .n_cts_48k = { .n = 5120, .cts = 247500, },
> > + }, {
> > + .tmds_clock_khz = 593407,
> > + .n_cts_32k = { .n = 5824, .cts = 843750, },
> > + .n_cts_44k1 = { .n = 8918, .cts = 937500, },
> > + .n_cts_48k = { .n = 5824, .cts = 562500, },
> > + }, {
> > + .tmds_clock_khz = 594000,
> > + .n_cts_32k = { .n = 3072, .cts = 445500, },
> > + .n_cts_44k1 = { .n = 9408, .cts = 990000, },
> > + .n_cts_48k = { .n = 6144, .cts = 594000, },
> > + },
> > +};
> > +
> > +static int drm_hdmi_acr_find_tmds_entry(unsigned long tmds_clock_khz)
> > +{
> > + int i;
> > +
> > + /* skip the "other" entry */
> > + for (i = 1; i < ARRAY_SIZE(hdmi_acr_n_cts); i++) {
> > + if (hdmi_acr_n_cts[i].tmds_clock_khz == tmds_clock_khz)
> > + return i;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * drm_hdmi_acr_get_n_cts() - get N and CTS values for Audio Clock Regeneration
> > + *
> > + * @tmds_char_rate: TMDS clock (char rate) as used by the HDMI connector
> > + * @sample_rate: audio sample rate
> > + * @out_n: a pointer to write the N value
> > + * @out_cts: a pointer to write the CTS value
> > + *
> > + * Get the N and CTS values (either by calculating them or by returning data
> > + * from the tables. This follows the HDMI 1.4b Section 7.2 "Audio Sample Clock
> > + * Capture and Regeneration".
> > + */
>
> I think we need to make it clear that it's for L-PCM only (I think?),
> either through a format parameter or through the documentation.
Ack
>
> > +void
> > +drm_hdmi_acr_get_n_cts(unsigned long long tmds_char_rate,
> > + unsigned int sample_rate,
> > + unsigned int *out_n,
> > + unsigned int *out_cts)
>
> And we should probably take the connector (or EDID) to make sure the
> monitor can support the format and sample rates.
Interesting perspective, I'll give it a thought. I was really just
trying to get rid of the duplication.
I think that 'supported' parts should be implemented in the hdmi-codec
instead, parsing the ELD and updating hw constraints. WDYT?
>
> > +{
> > + /* be a bit more tolerant, especially for the 1.001 entries */
> > + unsigned long tmds_clock_khz = DIV_ROUND_CLOSEST_ULL(tmds_char_rate, 1000);
> > + const struct drm_hdmi_acr_n_cts_entry *entry;
> > + unsigned int n, cts, mult;
> > + int tmds_idx;
> > +
> > + tmds_idx = drm_hdmi_acr_find_tmds_entry(tmds_clock_khz);
> > +
> > + /*
> > + * Don't change the order, 192 kHz is divisible by 48k and 32k, but it
> > + * should use 48k entry.
> > + */
> > + if (sample_rate % 48000 == 0) {
> > + entry = &hdmi_acr_n_cts[tmds_idx].n_cts_48k;
> > + mult = sample_rate / 48000;
> > + } else if (sample_rate % 44100 == 0) {
> > + entry = &hdmi_acr_n_cts[tmds_idx].n_cts_44k1;
> > + mult = sample_rate / 44100;
> > + } else if (sample_rate % 32000 == 0) {
> > + entry = &hdmi_acr_n_cts[tmds_idx].n_cts_32k;
> > + mult = sample_rate / 32000;
> > + } else {
> > + entry = NULL;
> > + }
> > +
> > + if (entry) {
> > + n = entry->n * mult;
> > + cts = entry->cts;
> > + } else {
> > + /* Recommended optimal value, HDMI 1.4b, Section 7.2.1 */
> > + n = 128 * sample_rate / 1000;
> > + cts = 0;
> > + }
> > +
> > + if (!cts)
> > + cts = DIV_ROUND_CLOSEST_ULL(tmds_char_rate * n,
> > + 128 * sample_rate);
> > +
> > + *out_n = n;
> > + *out_cts = cts;
> > +}
>
> EXPORT_SYMBOL?
Yes, I forgot it.
>
> Also, I'd really like to have some unit tests for this. Not for all the
> combinations, obviously, but testing that, say, 44.1kHz with a 148.5 MHz
> char rate works as expected, and then all the failure conditions
> depending on the monitor capabilities.
Ack for the tests. For the monitor capabilities, let's finish the
discussion above first.
--
With best wishes
Dmitry
Powered by blists - more mailing lists