Re: [PATCH] drm/tegra: hdmi: Fix audio to work with any pixel clock rate

From: Thierry Reding
Date: Fri Jan 04 2019 - 08:14:03 EST

On Wed, Dec 14, 2016 at 06:20:39PM +0100, Alban Bedel wrote:
> The audio setting implementation was limited to a few specific pixel
> clocks. This prevented HDMI audio from working on several test devices
> as they need a pixel clock that is not supported by this implementation.
>
> Fix this by implementing the algorithm provided in the TRM using fixed
> point arithmetic. This allows the driver to cope with any sane pixel
> clock rate.
>
> Signed-off-by: Alban Bedel <alban.bedel@xxxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/tegra/hdmi.c | 163 ++++++++++++++-----------------------------
> 1 file changed, 54 insertions(+), 109 deletions(-)

I had completely forgotten about this one, but then ran into this issue
myself yesterday and only then I started remembering this patch. It did
apply almost cleanly and still works perfectly. I made a couple of minor
modifications (see below) and applied this for v4.22.

> diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
> index cda0491ed6bf..b6078d6604b1 100644
> --- a/drivers/gpu/drm/tegra/hdmi.c
> +++ b/drivers/gpu/drm/tegra/hdmi.c
> @@ -14,6 +14,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
> #include <linux/reset.h>
> +#include <asm/div64.h>

Turned this into #include <linux/math64.h> which I think is the more
canonical way to get at do_div() nowadays.

>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_crtc.h>
> @@ -112,68 +113,11 @@ static inline void tegra_hdmi_writel(struct tegra_hdmi *hdmi, u32 value,
> }
>
> struct tegra_hdmi_audio_config {
> - unsigned int pclk;
> unsigned int n;
> unsigned int cts;
> unsigned int aval;
> };
>
> -static const struct tegra_hdmi_audio_config tegra_hdmi_audio_32k[] = {
> - { 25200000, 4096, 25200, 24000 },
> - { 27000000, 4096, 27000, 24000 },
> - { 74250000, 4096, 74250, 24000 },
> - { 148500000, 4096, 148500, 24000 },
> - { 0, 0, 0, 0 },
> -};
> -
> -static const struct tegra_hdmi_audio_config tegra_hdmi_audio_44_1k[] = {
> - { 25200000, 5880, 26250, 25000 },
> - { 27000000, 5880, 28125, 25000 },
> - { 74250000, 4704, 61875, 20000 },
> - { 148500000, 4704, 123750, 20000 },
> - { 0, 0, 0, 0 },
> -};
> -
> -static const struct tegra_hdmi_audio_config tegra_hdmi_audio_48k[] = {
> - { 25200000, 6144, 25200, 24000 },
> - { 27000000, 6144, 27000, 24000 },
> - { 74250000, 6144, 74250, 24000 },
> - { 148500000, 6144, 148500, 24000 },
> - { 0, 0, 0, 0 },
> -};
> -
> -static const struct tegra_hdmi_audio_config tegra_hdmi_audio_88_2k[] = {
> - { 25200000, 11760, 26250, 25000 },
> - { 27000000, 11760, 28125, 25000 },
> - { 74250000, 9408, 61875, 20000 },
> - { 148500000, 9408, 123750, 20000 },
> - { 0, 0, 0, 0 },
> -};
> -
> -static const struct tegra_hdmi_audio_config tegra_hdmi_audio_96k[] = {
> - { 25200000, 12288, 25200, 24000 },
> - { 27000000, 12288, 27000, 24000 },
> - { 74250000, 12288, 74250, 24000 },
> - { 148500000, 12288, 148500, 24000 },
> - { 0, 0, 0, 0 },
> -};
> -
> -static const struct tegra_hdmi_audio_config tegra_hdmi_audio_176_4k[] = {
> - { 25200000, 23520, 26250, 25000 },
> - { 27000000, 23520, 28125, 25000 },
> - { 74250000, 18816, 61875, 20000 },
> - { 148500000, 18816, 123750, 20000 },
> - { 0, 0, 0, 0 },
> -};
> -
> -static const struct tegra_hdmi_audio_config tegra_hdmi_audio_192k[] = {
> - { 25200000, 24576, 25200, 24000 },
> - { 27000000, 24576, 27000, 24000 },
> - { 74250000, 24576, 74250, 24000 },
> - { 148500000, 24576, 148500, 24000 },
> - { 0, 0, 0, 0 },
> -};
> -
> static const struct tmds_config tegra20_tmds_config[] = {
> { /* slow pixel clock modes */
> .pclk = 27000000,
> @@ -411,52 +355,49 @@ static const struct tmds_config tegra124_tmds_config[] = {
> },
> };
>
> -static const struct tegra_hdmi_audio_config *
> -tegra_hdmi_get_audio_config(unsigned int sample_rate, unsigned int pclk)
> +static int
> +tegra_hdmi_get_audio_config(unsigned int audio_freq, unsigned int pix_clock,
> + struct tegra_hdmi_audio_config *config)
> {
> - const struct tegra_hdmi_audio_config *table;
> -
> - switch (sample_rate) {
> - case 32000:
> - table = tegra_hdmi_audio_32k;
> - break;
> -
> - case 44100:
> - table = tegra_hdmi_audio_44_1k;
> - break;
> -
> - case 48000:
> - table = tegra_hdmi_audio_48k;
> - break;
> -
> - case 88200:
> - table = tegra_hdmi_audio_88_2k;
> - break;
> -
> - case 96000:
> - table = tegra_hdmi_audio_96k;
> - break;
> -
> - case 176400:
> - table = tegra_hdmi_audio_176_4k;
> - break;
> -
> - case 192000:
> - table = tegra_hdmi_audio_192k;
> - break;
> -
> - default:
> - return NULL;
> - }
> -
> - while (table->pclk) {
> - if (table->pclk == pclk)
> - return table;
> -
> - table++;
> + const int afreq = 128 * audio_freq;
> + const int min_n = afreq / 1500;
> + const int max_n = afreq / 300;
> + const int ideal_n = afreq / 1000;

Made all of these unsigned and added a new min_delta variable to track
abs(config->n - ideal_n). This is both to avoid recomputing it and also
...

> + int64_t min_err = (uint64_t)-1 >> 1;
> + int n;
> +
> + config->n = -1;
> +
> + for (n = min_n; n <= max_n; n++) {
> + uint64_t cts_f, aval_f;
> + int64_t cts, err;
> +
> + /* compute aval in 48.16 fixed point */
> + aval_f = ((int64_t)24000000 << 16) * n;
> + do_div(aval_f, afreq);
> + /* It should round without any rest */
> + if (aval_f & 0xFFFF)
> + continue;
> +
> + /* Compute cts in 48.16 fixed point */
> + cts_f = ((int64_t)pix_clock << 16) * n;
> + do_div(cts_f, afreq);
> + /* Round it to the nearest integer */
> + cts = (cts_f & ~0xFFFF) + ((cts_f & BIT(15)) << 1);
> +
> + /* Compute the absolute error */
> + err = abs((int64_t)cts_f - cts);
> + if (err < min_err ||
> + (err == min_err &&
> + abs(n - ideal_n) < abs(config->n - ideal_n))) {

... make this conditional slightly more readable.

Thanks,
Thierry

Attachment: signature.asc
Description: PGP signature