[PATCH v2 1/2] drm/bridge/synopsys: dw-hdmi: Handle audio for more clock rates

[PATCH v2 1/2] drm/bridge/synopsys: dw-hdmi: Handle audio for more clock rates

[Douglas Anderson]

Let's add some better support for HDMI audio to dw_hdmi.
Specifically:

1. For 44.1 kHz audio the old code made the assumption that an N of
6272 was right most of the time.  That wasn't true and the new table
should pick a more ideal value.

2. The new table has values from the HDMI spec for 297 MHz and 594
MHz.

3. There is now code to try to come up with a more idea N/CTS for
clock rates that aren't in the table.  This code is a bit slow because
it iterates over every possible value of N and picks the best one, but
it should make a good fallback.

NOTES:
- The oddest part of this patch comes about because computing the
  ideal N/CTS means knowing the _exact_ clock rate, not a rounded
  version of it.  The drm framework makes this harder by rounding
  rates to kHz, but even if it didn't there might be cases where the
  ideal rate could only be calculated if we knew the real
  (non-integral) rate.  This means that in cases where we know (or
  believe) that the true rate is something other than the rate we are
  told by drm.
- This patch makes much less of a difference after the patch
  ("drm/bridge: dw-hdmi: Use automatic CTS generation mode when using
  non-AHB audio"), at least if you're using I2S audio.  The main goal
  of picking a good N is to make it possible to get a nice integral
  CTS value, but if CTS is automatic then that's much less critical.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Atop ("drm/bridge: dw-hdmi: Use automatic CTS generation mode when
  using non-AHB audio").
- Split out the ability of a platform to provide custom tables.

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 203 +++++++++++++++++-----
 1 file changed, 162 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index de4c3669c83f..7cdffebcc7cb 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -60,6 +60,82 @@ enum hdmi_datamap {
 	YCbCr422_12B = 0x12,
 };
 
+struct dw_hdmi_audio_tmds_n {
+	unsigned long tmds;
+	unsigned int n_32k;
+	unsigned int n_44k1;
+	unsigned int n_48k;
+};
+
+/*
+ * Unless otherwise noted, entries in this table are 100% optimization.
+ * Values can be obtained from hdmi_compute_n() but that function is
+ * slow so we pre-compute values we expect to see.
+ *
+ * All 32k and 48k values are expected to be the same (due to the way
+ * the math works) for any rate that's an exact kHz.
+ *
+ * If a particular platform knows that it makes a rate slightly
+ * differently then it should add a platform-specific match.
+ */
+static const struct dw_hdmi_audio_tmds_n common_tmds_n_table[] = {
+	/* Doesn't match computations, assumes real clock = 25.2 MHz / 1.001 */
+	{ .tmds = 25175000, .n_32k = 4576, .n_44k1 = 7007, .n_48k = 6864, },
+
+	{ .tmds = 25200000, .n_32k = 4096, .n_44k1 = 5656, .n_48k = 6144, },
+	{ .tmds = 27000000, .n_32k = 4096, .n_44k1 = 5488, .n_48k = 6144, },
+	{ .tmds = 27027000, .n_32k = 4096, .n_44k1 = 6272, .n_48k = 6144, },
+	{ .tmds = 28320000, .n_32k = 4096, .n_44k1 = 5586, .n_48k = 6144, },
+	{ .tmds = 30240000, .n_32k = 4096, .n_44k1 = 5642, .n_48k = 6144, },
+	{ .tmds = 31500000, .n_32k = 4096, .n_44k1 = 5600, .n_48k = 6144, },
+	{ .tmds = 32000000, .n_32k = 4096, .n_44k1 = 5733, .n_48k = 6144, },
+	{ .tmds = 33750000, .n_32k = 4096, .n_44k1 = 6272, .n_48k = 6144, },
+	{ .tmds = 36000000, .n_32k = 4096, .n_44k1 = 5684, .n_48k = 6144, },
+	{ .tmds = 40000000, .n_32k = 4096, .n_44k1 = 5733, .n_48k = 6144, },
+	{ .tmds = 49500000, .n_32k = 4096, .n_44k1 = 5488, .n_48k = 6144, },
+	{ .tmds = 50000000, .n_32k = 4096, .n_44k1 = 5292, .n_48k = 6144, },
+	{ .tmds = 54000000, .n_32k = 4096, .n_44k1 = 5684, .n_48k = 6144, },
+	{ .tmds = 65000000, .n_32k = 4096, .n_44k1 = 7056, .n_48k = 6144, },
+	{ .tmds = 68250000, .n_32k = 4096, .n_44k1 = 5376, .n_48k = 6144, },
+	{ .tmds = 71000000, .n_32k = 4096, .n_44k1 = 7056, .n_48k = 6144, },
+	{ .tmds = 72000000, .n_32k = 4096, .n_44k1 = 5635, .n_48k = 6144, },
+	{ .tmds = 73250000, .n_32k = 4096, .n_44k1 = 14112, .n_48k = 6144, },
+
+	/* Doesn't match computations, assumes real clock = 74.25 MHz / 1.001 */
+	{ .tmds = 74176000, .n_32k = 11648, .n_44k1 = 17836, .n_48k = 11648, },
+
+	{ .tmds = 74250000, .n_32k = 4096, .n_44k1 = 6272, .n_48k = 6144, },
+	{ .tmds = 75000000, .n_32k = 4096, .n_44k1 = 5880, .n_48k = 6144, },
+	{ .tmds = 78750000, .n_32k = 4096, .n_44k1 = 5600, .n_48k = 6144, },
+	{ .tmds = 78800000, .n_32k = 4096, .n_44k1 = 5292, .n_48k = 6144, },
+	{ .tmds = 79500000, .n_32k = 4096, .n_44k1 = 4704, .n_48k = 6144, },
+	{ .tmds = 83500000, .n_32k = 4096, .n_44k1 = 7056, .n_48k = 6144, },
+	{ .tmds = 85500000, .n_32k = 4096, .n_44k1 = 5488, .n_48k = 6144, },
+	{ .tmds = 88750000, .n_32k = 4096, .n_44k1 = 14112, .n_48k = 6144, },
+	{ .tmds = 97750000, .n_32k = 4096, .n_44k1 = 14112, .n_48k = 6144, },
+	{ .tmds = 101000000, .n_32k = 4096, .n_44k1 = 7056, .n_48k = 6144, },
+	{ .tmds = 106500000, .n_32k = 4096, .n_44k1 = 4704, .n_48k = 6144, },
+	{ .tmds = 108000000, .n_32k = 4096, .n_44k1 = 5684, .n_48k = 6144, },
+	{ .tmds = 115500000, .n_32k = 4096, .n_44k1 = 5712, .n_48k = 6144, },
+	{ .tmds = 119000000, .n_32k = 4096, .n_44k1 = 5544, .n_48k = 6144, },
+	{ .tmds = 135000000, .n_32k = 4096, .n_44k1 = 5488, .n_48k = 6144, },
+	{ .tmds = 146250000, .n_32k = 4096, .n_44k1 = 6272, .n_48k = 6144, },
+
+	/* Doesn't match computations, assumes real clock = 148.5 MHz / 1.001 */
+	{ .tmds = 148352000, .n_32k = 11648, .n_44k1 = 8918, .n_48k = 5824, },
+
+	{ .tmds = 148500000, .n_32k = 4096, .n_44k1 = 5488, .n_48k = 6144, },
+	{ .tmds = 154000000, .n_32k = 4096, .n_44k1 = 5544, .n_48k = 6144, },
+	{ .tmds = 162000000, .n_32k = 4096, .n_44k1 = 5684, .n_48k = 6144, },
+
+	/* For 297 MHz+ HDMI spec has some other rule for setting N */
+	{ .tmds = 297000000, .n_32k = 3073, .n_44k1 = 4704, .n_48k = 5120, },
+	{ .tmds = 594000000, .n_32k = 3073, .n_44k1 = 9408, .n_48k = 10240, },
+
+	/* End of table */
+	{ .tmds = 0,         .n_32k = 0,    .n_44k1 = 0,    .n_48k = 0, },
+};
+
 static const u16 csc_coeff_default[3][4] = {
 	{ 0x2000, 0x0000, 0x0000, 0x0000 },
 	{ 0x0000, 0x2000, 0x0000, 0x0000 },
@@ -524,60 +600,105 @@ static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts,
 	hdmi_writeb(hdmi, n & 0xff, HDMI_AUD_N1);
 }
 
-static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk)
+static int hdmi_match_tmds_n_table(struct dw_hdmi *hdmi, unsigned int freq,
+				   unsigned long pixel_clk)
 {
-	unsigned int n = (128 * freq) / 1000;
-	unsigned int mult = 1;
+	const struct dw_hdmi_audio_tmds_n *tmds_n = NULL;
+	int mult = 1;
+	int i;
 
 	while (freq > 48000) {
 		mult *= 2;
 		freq /= 2;
 	}
 
+	for (i = 0; common_tmds_n_table[i].tmds != 0; i++) {
+		if (pixel_clk == common_tmds_n_table[i].tmds) {
+			tmds_n = &common_tmds_n_table[i];
+			break;
+		}
+	}
+
+	if (tmds_n == NULL)
+		return -ENOENT;
+
 	switch (freq) {
 	case 32000:
-		if (pixel_clk == 25175000)
-			n = 4576;
-		else if (pixel_clk == 27027000)
-			n = 4096;
-		else if (pixel_clk == 74176000 || pixel_clk == 148352000)
-			n = 11648;
-		else
-			n = 4096;
-		n *= mult;
-		break;
-
+		return tmds_n->n_32k * mult;
 	case 44100:
-		if (pixel_clk == 25175000)
-			n = 7007;
-		else if (pixel_clk == 74176000)
-			n = 17836;
-		else if (pixel_clk == 148352000)
-			n = 8918;
-		else
-			n = 6272;
-		n *= mult;
-		break;
-
+		return tmds_n->n_44k1 * mult;
 	case 48000:
-		if (pixel_clk == 25175000)
-			n = 6864;
-		else if (pixel_clk == 27027000)
-			n = 6144;
-		else if (pixel_clk == 74176000)
-			n = 11648;
-		else if (pixel_clk == 148352000)
-			n = 5824;
-		else
-			n = 6144;
-		n *= mult;
-		break;
-
+		return tmds_n->n_48k * mult;
 	default:
-		break;
+		return -ENOENT;
+	}
+}
+
+static u64 hdmi_audio_math_diff(unsigned int freq, unsigned int n,
+				unsigned int pixel_clk)
+{
+	u64 final, diff;
+	u64 cts;
+
+	final = (u64)pixel_clk * n;
+
+	cts = final;
+	do_div(cts, 128 * freq);
+
+	diff = final - (u64)cts * (128 * freq);
+
+	return diff;
+}
+
+static unsigned int hdmi_compute_n(struct dw_hdmi *hdmi, unsigned int freq,
+				   unsigned long pixel_clk)
+{
+	unsigned int min_n = DIV_ROUND_UP((128 * freq), 1500);
+	unsigned int max_n = (128 * freq) / 300;
+	unsigned int ideal_n = (128 * freq) / 1000;
+	unsigned int best_n_distance = ideal_n;
+	unsigned int best_n = 0;
+	u64 best_diff = U64_MAX;
+	int n;
+
+	/* If the ideal N could satisfy the audio math, then just take it */
+	if (hdmi_audio_math_diff(freq, ideal_n, pixel_clk) == 0)
+		return ideal_n;
+
+	for (n = min_n; n <= max_n; n++) {
+		u64 diff = hdmi_audio_math_diff(freq, n, pixel_clk);
+
+		if (diff < best_diff || (diff == best_diff &&
+		    abs(n - ideal_n) < best_n_distance)) {
+			best_n = n;
+			best_diff = diff;
+			best_n_distance = abs(best_n - ideal_n);
+		}
+
+		/*
+		 * The best N already satisfy the audio math, and also be
+		 * the closest value to ideal N, so just cut the loop.
+		 */
+		if ((best_diff == 0) && (abs(n - ideal_n) > best_n_distance))
+			break;
 	}
 
-	return n;
+	return best_n;
+}
+
+static unsigned int hdmi_find_n(struct dw_hdmi *hdmi, unsigned int freq,
+				unsigned long pixel_clk)
+{
+	int n;
+
+	n = hdmi_match_tmds_n_table(hdmi, freq, pixel_clk);
+	if (n > 0)
+		return n;
+
+	dev_warn(hdmi->dev, "Rate %lu missing; compute N dynamically\n",
+		 pixel_clk);
+
+	return hdmi_compute_n(hdmi, freq, pixel_clk);
 }
 
 static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
@@ -588,7 +709,7 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
 	u8 config3;
 	u64 tmp;
 
-	n = hdmi_compute_n(sample_rate, pixel_clk);
+	n = hdmi_find_n(hdmi, sample_rate, pixel_clk);
 
 	config3 = hdmi_readb(hdmi, HDMI_CONFIG3_ID);
 
-- 
2.22.0.410.gd8fdbe21b5-goog

[PATCH v2 2/2] drm/bridge/synopsys: dw-hdmi: Allow platforms to provide custom audio tables

[Douglas Anderson]

On some platforms using dw_hdmi it may not be possible to make an HDMI
pixel clock exactly, but it may be possible to make a rate that is
close enough to be within spec.  For instance on rk3288 we can make
25,176,471 Hz instead of 25,174,825.1748... Hz (25.2 MHz / 1.001).  A
future patch to the rk3288 platform code could enable support for this
clock rate and specify the N/CTS that would be ideal.

NOTE: I haven't yet posted said patch due to complexities with knowing
whether dw_hdmi can control the dynamic PLL on rk3288.  Thus for now
there are no users of this feature yet.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Split out the ability of a platform to provide custom tables.

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 27 ++++++++++++++---------
 include/drm/bridge/dw_hdmi.h              |  8 +++++++
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 7cdffebcc7cb..b6027edf2942 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -60,13 +60,6 @@ enum hdmi_datamap {
 	YCbCr422_12B = 0x12,
 };
 
-struct dw_hdmi_audio_tmds_n {
-	unsigned long tmds;
-	unsigned int n_32k;
-	unsigned int n_44k1;
-	unsigned int n_48k;
-};
-
 /*
  * Unless otherwise noted, entries in this table are 100% optimization.
  * Values can be obtained from hdmi_compute_n() but that function is
@@ -603,6 +596,7 @@ static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts,
 static int hdmi_match_tmds_n_table(struct dw_hdmi *hdmi, unsigned int freq,
 				   unsigned long pixel_clk)
 {
+	const struct dw_hdmi_plat_data *plat_data = hdmi->plat_data;
 	const struct dw_hdmi_audio_tmds_n *tmds_n = NULL;
 	int mult = 1;
 	int i;
@@ -612,10 +606,21 @@ static int hdmi_match_tmds_n_table(struct dw_hdmi *hdmi, unsigned int freq,
 		freq /= 2;
 	}
 
-	for (i = 0; common_tmds_n_table[i].tmds != 0; i++) {
-		if (pixel_clk == common_tmds_n_table[i].tmds) {
-			tmds_n = &common_tmds_n_table[i];
-			break;
+	if (plat_data->tmds_n_table) {
+		for (i = 0; plat_data->tmds_n_table[i].tmds != 0; i++) {
+			if (pixel_clk == plat_data->tmds_n_table[i].tmds) {
+				tmds_n = &plat_data->tmds_n_table[i];
+				break;
+			}
+		}
+	}
+
+	if (tmds_n == NULL) {
+		for (i = 0; common_tmds_n_table[i].tmds != 0; i++) {
+			if (pixel_clk == common_tmds_n_table[i].tmds) {
+				tmds_n = &common_tmds_n_table[i];
+				break;
+			}
 		}
 	}
 
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index c402364aec0d..5ee6b0a127aa 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -90,6 +90,13 @@ enum dw_hdmi_phy_type {
 	DW_HDMI_PHY_VENDOR_PHY = 0xfe,
 };
 
+struct dw_hdmi_audio_tmds_n {
+	unsigned long tmds;
+	unsigned int n_32k;
+	unsigned int n_44k1;
+	unsigned int n_48k;
+};
+
 struct dw_hdmi_mpll_config {
 	unsigned long mpixelclock;
 	struct {
@@ -137,6 +144,7 @@ struct dw_hdmi_plat_data {
 	const struct dw_hdmi_mpll_config *mpll_cfg;
 	const struct dw_hdmi_curr_ctrl *cur_ctr;
 	const struct dw_hdmi_phy_config *phy_config;
+	const struct dw_hdmi_audio_tmds_n *tmds_n_table;
 	int (*configure_phy)(struct dw_hdmi *hdmi,
 			     const struct dw_hdmi_plat_data *pdata,
 			     unsigned long mpixelclock);
-- 
2.22.0.410.gd8fdbe21b5-goog

Re: [PATCH v2 1/2] drm/bridge/synopsys: dw-hdmi: Handle audio for more clock rates

[Andrzej Hajda]

On 19.06.2019 23:07, Douglas Anderson wrote:
> Let's add some better support for HDMI audio to dw_hdmi.
> Specifically:
>
> 1. For 44.1 kHz audio the old code made the assumption that an N of
> 6272 was right most of the time.  That wasn't true and the new table
> should pick a more ideal value.


Why? I ask because it is against recommendation from HDMI specs.



>
> 2. The new table has values from the HDMI spec for 297 MHz and 594
> MHz.
>
> 3. There is now code to try to come up with a more idea N/CTS for
> clock rates that aren't in the table.  This code is a bit slow because
> it iterates over every possible value of N and picks the best one, but
> it should make a good fallback.
>
> NOTES:
> - The oddest part of this patch comes about because computing the
>   ideal N/CTS means knowing the _exact_ clock rate, not a rounded
>   version of it.  The drm framework makes this harder by rounding
>   rates to kHz, but even if it didn't there might be cases where the
>   ideal rate could only be calculated if we knew the real
>   (non-integral) rate.  This means that in cases where we know (or
>   believe) that the true rate is something other than the rate we are
>   told by drm.
> - This patch makes much less of a difference after the patch
>   ("drm/bridge: dw-hdmi: Use automatic CTS generation mode when using
>   non-AHB audio"), at least if you're using I2S audio.  The main goal
>   of picking a good N is to make it possible to get a nice integral
>   CTS value, but if CTS is automatic then that's much less critical.


As I said above HDMI recommendations are different from those from your
patch. Please elaborate why?

Btw I've seen your old patches introducing recommended N/CTS calculation
helpers in HDMI framework, unfortunately abandoned due to lack of interest.

Maybe resurrecting them would be a good idea, with assumption there will
be users :)


Regards

Andrzej


>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
> Changes in v2:
> - Atop ("drm/bridge: dw-hdmi: Use automatic CTS generation mode when
>   using non-AHB audio").
> - Split out the ability of a platform to provide custom tables.
>
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 203 +++++++++++++++++-----
>  1 file changed, 162 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index de4c3669c83f..7cdffebcc7cb 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -60,6 +60,82 @@ enum hdmi_datamap {
>  	YCbCr422_12B = 0x12,
>  };
>  
> +struct dw_hdmi_audio_tmds_n {
> +	unsigned long tmds;
> +	unsigned int n_32k;
> +	unsigned int n_44k1;
> +	unsigned int n_48k;
> +};
> +
> +/*
> + * Unless otherwise noted, entries in this table are 100% optimization.
> + * Values can be obtained from hdmi_compute_n() but that function is
> + * slow so we pre-compute values we expect to see.
> + *
> + * All 32k and 48k values are expected to be the same (due to the way
> + * the math works) for any rate that's an exact kHz.
> + *
> + * If a particular platform knows that it makes a rate slightly
> + * differently then it should add a platform-specific match.
> + */
> +static const struct dw_hdmi_audio_tmds_n common_tmds_n_table[] = {
> +	/* Doesn't match computations, assumes real clock = 25.2 MHz / 1.001 */
> +	{ .tmds = 25175000, .n_32k = 4576, .n_44k1 = 7007, .n_48k = 6864, },
> +
> +	{ .tmds = 25200000, .n_32k = 4096, .n_44k1 = 5656, .n_48k = 6144, },
> +	{ .tmds = 27000000, .n_32k = 4096, .n_44k1 = 5488, .n_48k = 6144, },
> +	{ .tmds = 27027000, .n_32k = 4096, .n_44k1 = 6272, .n_48k = 6144, },
> +	{ .tmds = 28320000, .n_32k = 4096, .n_44k1 = 5586, .n_48k = 6144, },
> +	{ .tmds = 30240000, .n_32k = 4096, .n_44k1 = 5642, .n_48k = 6144, },
> +	{ .tmds = 31500000, .n_32k = 4096, .n_44k1 = 5600, .n_48k = 6144, },
> +	{ .tmds = 32000000, .n_32k = 4096, .n_44k1 = 5733, .n_48k = 6144, },
> +	{ .tmds = 33750000, .n_32k = 4096, .n_44k1 = 6272, .n_48k = 6144, },
> +	{ .tmds = 36000000, .n_32k = 4096, .n_44k1 = 5684, .n_48k = 6144, },
> +	{ .tmds = 40000000, .n_32k = 4096, .n_44k1 = 5733, .n_48k = 6144, },
> +	{ .tmds = 49500000, .n_32k = 4096, .n_44k1 = 5488, .n_48k = 6144, },
> +	{ .tmds = 50000000, .n_32k = 4096, .n_44k1 = 5292, .n_48k = 6144, },
> +	{ .tmds = 54000000, .n_32k = 4096, .n_44k1 = 5684, .n_48k = 6144, },
> +	{ .tmds = 65000000, .n_32k = 4096, .n_44k1 = 7056, .n_48k = 6144, },
> +	{ .tmds = 68250000, .n_32k = 4096, .n_44k1 = 5376, .n_48k = 6144, },
> +	{ .tmds = 71000000, .n_32k = 4096, .n_44k1 = 7056, .n_48k = 6144, },
> +	{ .tmds = 72000000, .n_32k = 4096, .n_44k1 = 5635, .n_48k = 6144, },
> +	{ .tmds = 73250000, .n_32k = 4096, .n_44k1 = 14112, .n_48k = 6144, },
> +
> +	/* Doesn't match computations, assumes real clock = 74.25 MHz / 1.001 */
> +	{ .tmds = 74176000, .n_32k = 11648, .n_44k1 = 17836, .n_48k = 11648, },
> +
> +	{ .tmds = 74250000, .n_32k = 4096, .n_44k1 = 6272, .n_48k = 6144, },
> +	{ .tmds = 75000000, .n_32k = 4096, .n_44k1 = 5880, .n_48k = 6144, },
> +	{ .tmds = 78750000, .n_32k = 4096, .n_44k1 = 5600, .n_48k = 6144, },
> +	{ .tmds = 78800000, .n_32k = 4096, .n_44k1 = 5292, .n_48k = 6144, },
> +	{ .tmds = 79500000, .n_32k = 4096, .n_44k1 = 4704, .n_48k = 6144, },
> +	{ .tmds = 83500000, .n_32k = 4096, .n_44k1 = 7056, .n_48k = 6144, },
> +	{ .tmds = 85500000, .n_32k = 4096, .n_44k1 = 5488, .n_48k = 6144, },
> +	{ .tmds = 88750000, .n_32k = 4096, .n_44k1 = 14112, .n_48k = 6144, },
> +	{ .tmds = 97750000, .n_32k = 4096, .n_44k1 = 14112, .n_48k = 6144, },
> +	{ .tmds = 101000000, .n_32k = 4096, .n_44k1 = 7056, .n_48k = 6144, },
> +	{ .tmds = 106500000, .n_32k = 4096, .n_44k1 = 4704, .n_48k = 6144, },
> +	{ .tmds = 108000000, .n_32k = 4096, .n_44k1 = 5684, .n_48k = 6144, },
> +	{ .tmds = 115500000, .n_32k = 4096, .n_44k1 = 5712, .n_48k = 6144, },
> +	{ .tmds = 119000000, .n_32k = 4096, .n_44k1 = 5544, .n_48k = 6144, },
> +	{ .tmds = 135000000, .n_32k = 4096, .n_44k1 = 5488, .n_48k = 6144, },
> +	{ .tmds = 146250000, .n_32k = 4096, .n_44k1 = 6272, .n_48k = 6144, },
> +
> +	/* Doesn't match computations, assumes real clock = 148.5 MHz / 1.001 */
> +	{ .tmds = 148352000, .n_32k = 11648, .n_44k1 = 8918, .n_48k = 5824, },
> +
> +	{ .tmds = 148500000, .n_32k = 4096, .n_44k1 = 5488, .n_48k = 6144, },
> +	{ .tmds = 154000000, .n_32k = 4096, .n_44k1 = 5544, .n_48k = 6144, },
> +	{ .tmds = 162000000, .n_32k = 4096, .n_44k1 = 5684, .n_48k = 6144, },
> +
> +	/* For 297 MHz+ HDMI spec has some other rule for setting N */
> +	{ .tmds = 297000000, .n_32k = 3073, .n_44k1 = 4704, .n_48k = 5120, },
> +	{ .tmds = 594000000, .n_32k = 3073, .n_44k1 = 9408, .n_48k = 10240, },
> +
> +	/* End of table */
> +	{ .tmds = 0,         .n_32k = 0,    .n_44k1 = 0,    .n_48k = 0, },
> +};
> +
>  static const u16 csc_coeff_default[3][4] = {
>  	{ 0x2000, 0x0000, 0x0000, 0x0000 },
>  	{ 0x0000, 0x2000, 0x0000, 0x0000 },
> @@ -524,60 +600,105 @@ static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts,
>  	hdmi_writeb(hdmi, n & 0xff, HDMI_AUD_N1);
>  }
>  
> -static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk)
> +static int hdmi_match_tmds_n_table(struct dw_hdmi *hdmi, unsigned int freq,
> +				   unsigned long pixel_clk)
>  {
> -	unsigned int n = (128 * freq) / 1000;
> -	unsigned int mult = 1;
> +	const struct dw_hdmi_audio_tmds_n *tmds_n = NULL;
> +	int mult = 1;
> +	int i;
>  
>  	while (freq > 48000) {
>  		mult *= 2;
>  		freq /= 2;
>  	}
>  
> +	for (i = 0; common_tmds_n_table[i].tmds != 0; i++) {
> +		if (pixel_clk == common_tmds_n_table[i].tmds) {
> +			tmds_n = &common_tmds_n_table[i];
> +			break;
> +		}
> +	}
> +
> +	if (tmds_n == NULL)
> +		return -ENOENT;
> +
>  	switch (freq) {
>  	case 32000:
> -		if (pixel_clk == 25175000)
> -			n = 4576;
> -		else if (pixel_clk == 27027000)
> -			n = 4096;
> -		else if (pixel_clk == 74176000 || pixel_clk == 148352000)
> -			n = 11648;
> -		else
> -			n = 4096;
> -		n *= mult;
> -		break;
> -
> +		return tmds_n->n_32k * mult;
>  	case 44100:
> -		if (pixel_clk == 25175000)
> -			n = 7007;
> -		else if (pixel_clk == 74176000)
> -			n = 17836;
> -		else if (pixel_clk == 148352000)
> -			n = 8918;
> -		else
> -			n = 6272;
> -		n *= mult;
> -		break;
> -
> +		return tmds_n->n_44k1 * mult;
>  	case 48000:
> -		if (pixel_clk == 25175000)
> -			n = 6864;
> -		else if (pixel_clk == 27027000)
> -			n = 6144;
> -		else if (pixel_clk == 74176000)
> -			n = 11648;
> -		else if (pixel_clk == 148352000)
> -			n = 5824;
> -		else
> -			n = 6144;
> -		n *= mult;
> -		break;
> -
> +		return tmds_n->n_48k * mult;
>  	default:
> -		break;
> +		return -ENOENT;
> +	}
> +}
> +
> +static u64 hdmi_audio_math_diff(unsigned int freq, unsigned int n,
> +				unsigned int pixel_clk)
> +{
> +	u64 final, diff;
> +	u64 cts;
> +
> +	final = (u64)pixel_clk * n;
> +
> +	cts = final;
> +	do_div(cts, 128 * freq);
> +
> +	diff = final - (u64)cts * (128 * freq);
> +
> +	return diff;
> +}
> +
> +static unsigned int hdmi_compute_n(struct dw_hdmi *hdmi, unsigned int freq,
> +				   unsigned long pixel_clk)
> +{
> +	unsigned int min_n = DIV_ROUND_UP((128 * freq), 1500);
> +	unsigned int max_n = (128 * freq) / 300;
> +	unsigned int ideal_n = (128 * freq) / 1000;
> +	unsigned int best_n_distance = ideal_n;
> +	unsigned int best_n = 0;
> +	u64 best_diff = U64_MAX;
> +	int n;
> +
> +	/* If the ideal N could satisfy the audio math, then just take it */
> +	if (hdmi_audio_math_diff(freq, ideal_n, pixel_clk) == 0)
> +		return ideal_n;
> +
> +	for (n = min_n; n <= max_n; n++) {
> +		u64 diff = hdmi_audio_math_diff(freq, n, pixel_clk);
> +
> +		if (diff < best_diff || (diff == best_diff &&
> +		    abs(n - ideal_n) < best_n_distance)) {
> +			best_n = n;
> +			best_diff = diff;
> +			best_n_distance = abs(best_n - ideal_n);
> +		}
> +
> +		/*
> +		 * The best N already satisfy the audio math, and also be
> +		 * the closest value to ideal N, so just cut the loop.
> +		 */
> +		if ((best_diff == 0) && (abs(n - ideal_n) > best_n_distance))
> +			break;
>  	}
>  
> -	return n;
> +	return best_n;
> +}
> +
> +static unsigned int hdmi_find_n(struct dw_hdmi *hdmi, unsigned int freq,
> +				unsigned long pixel_clk)
> +{
> +	int n;
> +
> +	n = hdmi_match_tmds_n_table(hdmi, freq, pixel_clk);
> +	if (n > 0)
> +		return n;
> +
> +	dev_warn(hdmi->dev, "Rate %lu missing; compute N dynamically\n",
> +		 pixel_clk);
> +
> +	return hdmi_compute_n(hdmi, freq, pixel_clk);
>  }
>  
>  static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
> @@ -588,7 +709,7 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
>  	u8 config3;
>  	u64 tmp;
>  
> -	n = hdmi_compute_n(sample_rate, pixel_clk);
> +	n = hdmi_find_n(hdmi, sample_rate, pixel_clk);
>  
>  	config3 = hdmi_readb(hdmi, HDMI_CONFIG3_ID);
>  

Re: [PATCH v2 1/2] drm/bridge/synopsys: dw-hdmi: Handle audio for more clock rates

[Doug Anderson]

Hi,


On Tue, Jun 25, 2019 at 9:07 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> On 19.06.2019 23:07, Douglas Anderson wrote:
> > Let's add some better support for HDMI audio to dw_hdmi.
> > Specifically:
> >
> > 1. For 44.1 kHz audio the old code made the assumption that an N of
> > 6272 was right most of the time.  That wasn't true and the new table
> > should pick a more ideal value.
>
>
> Why? I ask because it is against recommendation from HDMI specs.

The place where it does matter (and why I originally did this work) is
when you don't have auto-CTS.  In such a case you really need "N" and
"CTS" to make the math work and both be integral.  This makes sure
that you don't slowly accumulate offsets.  I'm hoping that this point
should be non-controversial so I won't argue it more.

I am an admitted non-expert, but I have a feeling that with Auto-CTS
either the old number or the new numbers would produce pretty much the
same experience.  AKA: anyone using auto-CTS won't notice any change
at all.  I guess the question is: with Auto-CTS should you pick the
"ideal" 6272 or a value that allows CTS to be the closest to integral
as possible.  By reading between the lines of the spec, I decided that
it was slightly more important to allow for an integral CTS.  If
achieving an integral CTS wasn't a goal then the spec wouldn't even
have listed special cases for any of the clock rates.  We would just
be using the ideal N and Auto-CTS and be done with it.  The whole
point of the tables they list is to make CTS integral.


> > 2. The new table has values from the HDMI spec for 297 MHz and 594
> > MHz.
> >
> > 3. There is now code to try to come up with a more idea N/CTS for
> > clock rates that aren't in the table.  This code is a bit slow because
> > it iterates over every possible value of N and picks the best one, but
> > it should make a good fallback.
> >
> > NOTES:
> > - The oddest part of this patch comes about because computing the
> >   ideal N/CTS means knowing the _exact_ clock rate, not a rounded
> >   version of it.  The drm framework makes this harder by rounding
> >   rates to kHz, but even if it didn't there might be cases where the
> >   ideal rate could only be calculated if we knew the real
> >   (non-integral) rate.  This means that in cases where we know (or
> >   believe) that the true rate is something other than the rate we are
> >   told by drm.
> > - This patch makes much less of a difference after the patch
> >   ("drm/bridge: dw-hdmi: Use automatic CTS generation mode when using
> >   non-AHB audio"), at least if you're using I2S audio.  The main goal
> >   of picking a good N is to make it possible to get a nice integral
> >   CTS value, but if CTS is automatic then that's much less critical.
>
>
> As I said above HDMI recommendations are different from those from your
> patch. Please elaborate why?
>
> Btw I've seen your old patches introducing recommended N/CTS calculation
> helpers in HDMI framework, unfortunately abandoned due to lack of interest.
>
> Maybe resurrecting them would be a good idea, with assumption there will
> be users :)

I have old patches introducing this into the HDMI framework?  I don't
remember them / can't find them.  Can you provide a pointer?

-Doug

Re: [PATCH v2 1/2] drm/bridge/synopsys: dw-hdmi: Handle audio for more clock rates

[Andrzej Hajda]

On 25.06.2019 18:26, Doug Anderson wrote:
> Hi,
>
>
> On Tue, Jun 25, 2019 at 9:07 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>> On 19.06.2019 23:07, Douglas Anderson wrote:
>>> Let's add some better support for HDMI audio to dw_hdmi.
>>> Specifically:
>>>
>>> 1. For 44.1 kHz audio the old code made the assumption that an N of
>>> 6272 was right most of the time.  That wasn't true and the new table
>>> should pick a more ideal value.
>>
>> Why? I ask because it is against recommendation from HDMI specs.
> The place where it does matter (and why I originally did this work) is
> when you don't have auto-CTS.  In such a case you really need "N" and
> "CTS" to make the math work and both be integral.  This makes sure
> that you don't slowly accumulate offsets.  I'm hoping that this point
> should be non-controversial so I won't argue it more.
>
> I am an admitted non-expert, but I have a feeling that with Auto-CTS
> either the old number or the new numbers would produce pretty much the
> same experience.


Because Auto-CTS mechanism will alternate between two or more CTS values
every frame, thus it will compensate non-rational clock relationship.


>   AKA: anyone using auto-CTS won't notice any change
> at all.  I guess the question is: with Auto-CTS should you pick the
> "ideal" 6272 or a value that allows CTS to be the closest to integral
> as possible.  By reading between the lines of the spec, I decided that
> it was slightly more important to allow for an integral CTS.  If
> achieving an integral CTS wasn't a goal then the spec wouldn't even
> have listed special cases for any of the clock rates.  We would just
> be using the ideal N and Auto-CTS and be done with it.  The whole
> point of the tables they list is to make CTS integral.


Specification recommends many contradictory things without explicit
prioritization, at least I have not found it.

So we should relay on our intuition.

I guess that with auto-cts N we should follow recommendation - I guess
most sinks have been better tested with recommended values.

So what with non-auto-cts case:

1. How many devices do not have auto-cts? how many alternative TMDS
clocks we have? Maybe it is theoretical problem.

2. Alternating CTS in software is possible, but quite
complicated/annoying, but at least it will follow recommendation :)


Regards

Andrzej


>
>
>>> 2. The new table has values from the HDMI spec for 297 MHz and 594
>>> MHz.
>>>
>>> 3. There is now code to try to come up with a more idea N/CTS for
>>> clock rates that aren't in the table.  This code is a bit slow because
>>> it iterates over every possible value of N and picks the best one, but
>>> it should make a good fallback.
>>>
>>> NOTES:
>>> - The oddest part of this patch comes about because computing the
>>>   ideal N/CTS means knowing the _exact_ clock rate, not a rounded
>>>   version of it.  The drm framework makes this harder by rounding
>>>   rates to kHz, but even if it didn't there might be cases where the
>>>   ideal rate could only be calculated if we knew the real
>>>   (non-integral) rate.  This means that in cases where we know (or
>>>   believe) that the true rate is something other than the rate we are
>>>   told by drm.
>>> - This patch makes much less of a difference after the patch
>>>   ("drm/bridge: dw-hdmi: Use automatic CTS generation mode when using
>>>   non-AHB audio"), at least if you're using I2S audio.  The main goal
>>>   of picking a good N is to make it possible to get a nice integral
>>>   CTS value, but if CTS is automatic then that's much less critical.
>>
>> As I said above HDMI recommendations are different from those from your
>> patch. Please elaborate why?
>>
>> Btw I've seen your old patches introducing recommended N/CTS calculation
>> helpers in HDMI framework, unfortunately abandoned due to lack of interest.
>>
>> Maybe resurrecting them would be a good idea, with assumption there will
>> be users :)
> I have old patches introducing this into the HDMI framework?  I don't
> remember them / can't find them.  Can you provide a pointer?
>
> -Doug
>
>

Re: [PATCH v2 1/2] drm/bridge/synopsys: dw-hdmi: Handle audio for more clock rates

[Andrzej Hajda]

On 26.06.2019 11:56, Andrzej Hajda wrote:
> On 25.06.2019 18:26, Doug Anderson wrote:
>> Hi,
>>
>>
>> On Tue, Jun 25, 2019 at 9:07 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>>> On 19.06.2019 23:07, Douglas Anderson wrote:
>>>> Let's add some better support for HDMI audio to dw_hdmi.
>>>> Specifically:
>>>>
>>>> 1. For 44.1 kHz audio the old code made the assumption that an N of
>>>> 6272 was right most of the time.  That wasn't true and the new table
>>>> should pick a more ideal value.
>>> Why? I ask because it is against recommendation from HDMI specs.
>> The place where it does matter (and why I originally did this work) is
>> when you don't have auto-CTS.  In such a case you really need "N" and
>> "CTS" to make the math work and both be integral.  This makes sure
>> that you don't slowly accumulate offsets.  I'm hoping that this point
>> should be non-controversial so I won't argue it more.
>>
>> I am an admitted non-expert, but I have a feeling that with Auto-CTS
>> either the old number or the new numbers would produce pretty much the
>> same experience.
>
> Because Auto-CTS mechanism will alternate between two or more CTS values
> every frame, thus it will compensate non-rational clock relationship.
>
>
>>   AKA: anyone using auto-CTS won't notice any change
>> at all.  I guess the question is: with Auto-CTS should you pick the
>> "ideal" 6272 or a value that allows CTS to be the closest to integral
>> as possible.  By reading between the lines of the spec, I decided that
>> it was slightly more important to allow for an integral CTS.  If
>> achieving an integral CTS wasn't a goal then the spec wouldn't even
>> have listed special cases for any of the clock rates.  We would just
>> be using the ideal N and Auto-CTS and be done with it.  The whole
>> point of the tables they list is to make CTS integral.
>
> Specification recommends many contradictory things without explicit
> prioritization, at least I have not found it.
>
> So we should relay on our intuition.
>
> I guess that with auto-cts N we should follow recommendation - I guess
> most sinks have been better tested with recommended values.
>
> So what with non-auto-cts case:
>
> 1. How many devices do not have auto-cts? how many alternative TMDS
> clocks we have? Maybe it is theoretical problem.
>
> 2. Alternating CTS in software is possible, but quite
> complicated/annoying, but at least it will follow recommendation :)
>
>
> Regards
>
> Andrzej
>
>
>>
>>>> 2. The new table has values from the HDMI spec for 297 MHz and 594
>>>> MHz.
>>>>
>>>> 3. There is now code to try to come up with a more idea N/CTS for
>>>> clock rates that aren't in the table.  This code is a bit slow because
>>>> it iterates over every possible value of N and picks the best one, but
>>>> it should make a good fallback.
>>>>
>>>> NOTES:
>>>> - The oddest part of this patch comes about because computing the
>>>>   ideal N/CTS means knowing the _exact_ clock rate, not a rounded
>>>>   version of it.  The drm framework makes this harder by rounding
>>>>   rates to kHz, but even if it didn't there might be cases where the
>>>>   ideal rate could only be calculated if we knew the real
>>>>   (non-integral) rate.  This means that in cases where we know (or
>>>>   believe) that the true rate is something other than the rate we are
>>>>   told by drm.
>>>> - This patch makes much less of a difference after the patch
>>>>   ("drm/bridge: dw-hdmi: Use automatic CTS generation mode when using
>>>>   non-AHB audio"), at least if you're using I2S audio.  The main goal
>>>>   of picking a good N is to make it possible to get a nice integral
>>>>   CTS value, but if CTS is automatic then that's much less critical.
>>> As I said above HDMI recommendations are different from those from your
>>> patch. Please elaborate why?
>>>
>>> Btw I've seen your old patches introducing recommended N/CTS calculation
>>> helpers in HDMI framework, unfortunately abandoned due to lack of interest.
>>>
>>> Maybe resurrecting them would be a good idea, with assumption there will
>>> be users :)
>> I have old patches introducing this into the HDMI framework?  I don't
>> remember them / can't find them.  Can you provide a pointer?


And forgot answer this:

My mistake the patches were by Arnaud Pouliquen[1].

[1]: https://patchwork.kernel.org/patch/8906791/


Regards

Andrzej



>>
>> -Doug
>>
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/2] drm/bridge/synopsys: dw-hdmi: Handle audio for more clock rates

[Doug Anderson]

Hi,

On Wed, Jun 26, 2019 at 2:56 AM Andrzej Hajda <a.hajda@samsung.com> wrote:
>
> >   AKA: anyone using auto-CTS won't notice any change
> > at all.  I guess the question is: with Auto-CTS should you pick the
> > "ideal" 6272 or a value that allows CTS to be the closest to integral
> > as possible.  By reading between the lines of the spec, I decided that
> > it was slightly more important to allow for an integral CTS.  If
> > achieving an integral CTS wasn't a goal then the spec wouldn't even
> > have listed special cases for any of the clock rates.  We would just
> > be using the ideal N and Auto-CTS and be done with it.  The whole
> > point of the tables they list is to make CTS integral.
>
>
> Specification recommends many contradictory things without explicit
> prioritization, at least I have not found it.
>
> So we should relay on our intuition.
>
> I guess that with auto-cts N we should follow recommendation - I guess
> most sinks have been better tested with recommended values.
>
> So what with non-auto-cts case:
>
> 1. How many devices do not have auto-cts? how many alternative TMDS
> clocks we have? Maybe it is theoretical problem.
>
> 2. Alternating CTS in software is possible, but quite
> complicated/annoying, but at least it will follow recommendation :)

It is OK w/ me if we want to drop my patch.  With the auto-CTS patch
it shouldn't matter anymore.  ...but I still wanted to post it to the
list for posterity in case it is ever useful for someone else.

-Doug