[PATCH 0/2] iec958 plugin enhancements for HDMI

[PATCH 0/2] iec958 plugin enhancements for HDMI

[Matthias Reichl]

Testing with vc4-hdmi / Raspberry Pi 4 showed that the iec958 plugin
was missing two features required for HDMI compatibility:

HD audio bitstreams like DTS HD MA or Dolby True HD are usually packed
into 8-channel 192kHz 16-bit frames but are in fact a single IEC 61937
stream at 768kHz frame rate. So the data in the 8-channel frame has to
be formatted as 4 contiguous IEC 60958 frames.

If channel status bits weren't set, iec958 indicated a sample rate of
48kHz in the channel status bits which the HDMI analyzer complained about
and caused issues with some HDMI devices if it didn't match the real
rate. That was a long outstanding FIXME since the iec958 plugin was added.

With this series HD audio passthrough and PCM audio works fine with
the downstream 5.4 RPi kernel.

Matthias Reichl (2):
  pcm: iec958: implement HDMI HBR audio formatting
  pcm: iec958: set channel status bits according to rate and format

 src/pcm/pcm_iec958.c | 117 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 109 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH 1/2] pcm: iec958: implement HDMI HBR audio formatting

[Matthias Reichl]

High bitrate compressed audio data like DTS HD or MAT is usually
packed into 8-channel data. The HDMI specs state this has to be
formatted as a single IEC958 stream, compared to normal multi-
channel PCM data which has to be formatted as parallel IEC958 streams.

As this single-stream formatting mode may break existing setups that
expect non-PCM multichannel data to be formatted as parallel IEC958
streams it needs to be explicitly selected by setting the hdmi_mode
option to true.

The single-stream formatting implementation is prepared to cope with
arbitrary channel counts but only limited testing was done for channel
counts other than 8.

Signed-off-by: Matthias Reichl <hias@horus.com>
---
 src/pcm/pcm_iec958.c | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/src/pcm/pcm_iec958.c b/src/pcm/pcm_iec958.c
index 76d3ca7b..17ade957 100644
--- a/src/pcm/pcm_iec958.c
+++ b/src/pcm/pcm_iec958.c
@@ -63,6 +63,7 @@ struct snd_pcm_iec958 {
 	unsigned int byteswap;
 	unsigned char preamble[3];	/* B/M/W or Z/X/Y */
 	snd_pcm_fast_ops_t fops;
+	int hdmi_mode;
 };
 
 enum { PREAMBLE_Z, PREAMBLE_X, PREAMBLE_Y };
@@ -193,6 +194,10 @@ static void snd_pcm_iec958_encode(snd_pcm_iec958_t *iec,
 	unsigned int channel;
 	int32_t sample = 0;
 	int counter = iec->counter;
+	int single_stream = iec->hdmi_mode &&
+			    (iec->status[0] & IEC958_AES0_NONAUDIO) &&
+			    (channels == 8);
+	int counter_step = single_stream ? ((channels + 1) >> 1) : 1;
 	for (channel = 0; channel < channels; ++channel) {
 		const char *src;
 		uint32_t *dst;
@@ -205,7 +210,12 @@ static void snd_pcm_iec958_encode(snd_pcm_iec958_t *iec,
 		src_step = snd_pcm_channel_area_step(src_area);
 		dst_step = snd_pcm_channel_area_step(dst_area) / sizeof(uint32_t);
 		frames1 = frames;
-		iec->counter = counter;
+
+		if (single_stream)
+			iec->counter = (counter + (channel >> 1)) % 192;
+		else
+			iec->counter = counter;
+
 		while (frames1-- > 0) {
 			goto *get;
 #define GET32_END after
@@ -217,9 +227,11 @@ static void snd_pcm_iec958_encode(snd_pcm_iec958_t *iec,
 			*dst = sample;
 			src += src_step;
 			dst += dst_step;
-			iec->counter++;
+			iec->counter += counter_step;
 			iec->counter %= 192;
 		}
+		if (single_stream) /* set counter to ch0 value for next iteration */
+			iec->counter = (counter + frames * counter_step) % 192;
 	}
 }
 #endif /* DOC_HIDDEN */
@@ -473,6 +485,7 @@ static const snd_pcm_ops_t snd_pcm_iec958_ops = {
  * \param close_slave When set, the slave PCM handle is closed with copy PCM
  * \param status_bits The IEC958 status bits
  * \param preamble_vals The preamble byte values
+ * \param hdmi_mode When set, enable HDMI compliant formatting
  * \retval zero on success otherwise a negative error code
  * \warning Using of this function might be dangerous in the sense
  *          of compatibility reasons. The prototype might be freely
@@ -481,7 +494,8 @@ static const snd_pcm_ops_t snd_pcm_iec958_ops = {
 int snd_pcm_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sformat,
 			snd_pcm_t *slave, int close_slave,
 			const unsigned char *status_bits,
-			const unsigned char *preamble_vals)
+			const unsigned char *preamble_vals,
+		        int hdmi_mode)
 {
 	snd_pcm_t *pcm;
 	snd_pcm_iec958_t *iec;
@@ -519,6 +533,8 @@ int snd_pcm_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sfo
 
 	memcpy(iec->preamble, preamble_vals, 3);
 
+	iec->hdmi_mode = hdmi_mode;
+
 	err = snd_pcm_new(&pcm, SND_PCM_TYPE_IEC958, name, slave->stream, slave->mode);
 	if (err < 0) {
 		free(iec);
@@ -566,9 +582,14 @@ pcm.name {
 	[preamble.z or preamble.b val]
 	[preamble.x or preamble.m val]
 	[preamble.y or preamble.w val]
+	[hdmi_mode true]
 }
 \endcode
 
+When <code>hdmi_mode</code> is true, 8-channel compressed data is
+formatted as 4 contiguous frames of a single IEC958 stream as required
+by the HDMI HBR specification.
+
 \subsection pcm_plugins_iec958_funcref Function reference
 
 <UL>
@@ -605,6 +626,7 @@ int _snd_pcm_iec958_open(snd_pcm_t **pcmp, const char *name,
 	unsigned char preamble_vals[3] = {
 		0x08, 0x02, 0x04 /* Z, X, Y */
 	};
+	int hdmi_mode = 0;
 
 	snd_config_for_each(i, next, conf) {
 		snd_config_t *n = snd_config_iterator_entry(i);
@@ -633,6 +655,13 @@ int _snd_pcm_iec958_open(snd_pcm_t **pcmp, const char *name,
 			preamble = n;
 			continue;
 		}
+		if (strcmp(id, "hdmi_mode") == 0) {
+			err = snd_config_get_bool(n);
+			if (err < 0)
+				continue;
+			hdmi_mode = err;
+			continue;
+		}
 		SNDERR("Unknown field %s", id);
 		return -EINVAL;
 	}
@@ -707,7 +736,7 @@ int _snd_pcm_iec958_open(snd_pcm_t **pcmp, const char *name,
 		return err;
 	err = snd_pcm_iec958_open(pcmp, name, sformat, spcm, 1,
 				  status ? status_bits : NULL,
-				  preamble_vals);
+				  preamble_vals, hdmi_mode);
 	if (err < 0)
 		snd_pcm_close(spcm);
 	return err;
-- 
2.20.1

[PATCH 2/2] pcm: iec958: set channel status bits according to rate and format

[Matthias Reichl]

This mimics snd_pcm_create_iec958_consumer in the kernel.

The rate and wordlength bits will only be modified if they are
set to "not indicated", which is now the default if no status
option is used.

This allows applications to override parameters determined from
the stream or implement channel status bits extensions without
needing to change pcm_iec958 code.

Signed-off-by: Matthias Reichl <hias@horus.com>
---
 src/pcm/pcm_iec958.c | 80 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 76 insertions(+), 4 deletions(-)

diff --git a/src/pcm/pcm_iec958.c b/src/pcm/pcm_iec958.c
index 17ade957..a11a0439 100644
--- a/src/pcm/pcm_iec958.c
+++ b/src/pcm/pcm_iec958.c
@@ -365,9 +365,80 @@ static int snd_pcm_iec958_hw_params(snd_pcm_t *pcm, snd_pcm_hw_params_t * params
 			iec->byteswap = format != SND_PCM_FORMAT_IEC958_SUBFRAME;
 		}
 	}
-	/* FIXME: needs to adjust status_bits according to the format
-	 *        and sample rate
-	 */
+
+	if ((iec->status[0] & IEC958_AES0_PROFESSIONAL) == 0) {
+		if ((iec->status[3] & IEC958_AES3_CON_FS) == IEC958_AES3_CON_FS_NOTID) {
+			unsigned int rate = 0;
+			unsigned char fs;
+
+			err = INTERNAL(snd_pcm_hw_params_get_rate)(params, &rate, 0);
+			if (err < 0)
+				rate = 0;
+
+			switch (rate) {
+			case 22050:
+				fs = IEC958_AES3_CON_FS_22050;
+				break;
+			case 24000:
+				fs = IEC958_AES3_CON_FS_24000;
+				break;
+			case 32000:
+				fs = IEC958_AES3_CON_FS_32000;
+				break;
+			case 44100:
+				fs = IEC958_AES3_CON_FS_44100;
+				break;
+			case 48000:
+				fs = IEC958_AES3_CON_FS_48000;
+				break;
+			case 88200:
+				fs = IEC958_AES3_CON_FS_88200;
+				break;
+			case 96000:
+				fs = IEC958_AES3_CON_FS_96000;
+				break;
+			case 176400:
+				fs = IEC958_AES3_CON_FS_176400;
+				break;
+			case 192000:
+				fs = IEC958_AES3_CON_FS_192000;
+				break;
+			case 768000:
+				fs = IEC958_AES3_CON_FS_768000;
+				break;
+			default:
+				fs = IEC958_AES3_CON_FS_NOTID;
+				break;
+			}
+
+			iec->status[3] &= ~IEC958_AES3_CON_FS;
+			iec->status[3] |= fs;
+		}
+
+		if ((iec->status[4] & IEC958_AES4_CON_WORDLEN) == IEC958_AES4_CON_WORDLEN_NOTID) {
+			unsigned char ws;
+			switch (snd_pcm_format_width(format)) {
+			case 16:
+				ws = IEC958_AES4_CON_WORDLEN_20_16;
+				break;
+			case 18:
+				ws = IEC958_AES4_CON_WORDLEN_22_18;
+				break;
+			case 20:
+				ws = IEC958_AES4_CON_WORDLEN_20_16 | IEC958_AES4_CON_MAX_WORDLEN_24;
+				break;
+			case 24:
+			case 32: /* Assume 24-bit width for 32-bit samples. */
+				ws = IEC958_AES4_CON_WORDLEN_24_20 | IEC958_AES4_CON_MAX_WORDLEN_24;
+				break;
+			default:
+				ws = IEC958_AES4_CON_WORDLEN_NOTID;
+				break;
+			}
+			iec->status[4] &= ~(IEC958_AES4_CON_MAX_WORDLEN_24 | IEC958_AES4_CON_WORDLEN);
+			iec->status[4] |= ws;
+		}
+	}
 	return 0;
 }
 
@@ -504,7 +575,8 @@ int snd_pcm_iec958_open(snd_pcm_t **pcmp, const char *name, snd_pcm_format_t sfo
 		IEC958_AES0_CON_EMPHASIS_NONE,
 		IEC958_AES1_CON_ORIGINAL | IEC958_AES1_CON_PCM_CODER,
 		0,
-		IEC958_AES3_CON_FS_48000
+		IEC958_AES3_CON_FS_NOTID, /* will be set in hwparams */
+		IEC958_AES4_CON_WORDLEN_NOTID /* will be set in hwparams */
 	};
 
 	assert(pcmp && slave);
-- 
2.20.1

Re: [PATCH 0/2] iec958 plugin enhancements for HDMI

[Takashi Iwai]

On Mon, 13 Jul 2020 23:17:02 +0200,
Matthias Reichl wrote:
> 
> Testing with vc4-hdmi / Raspberry Pi 4 showed that the iec958 plugin
> was missing two features required for HDMI compatibility:
> 
> HD audio bitstreams like DTS HD MA or Dolby True HD are usually packed
> into 8-channel 192kHz 16-bit frames but are in fact a single IEC 61937
> stream at 768kHz frame rate. So the data in the 8-channel frame has to
> be formatted as 4 contiguous IEC 60958 frames.
> 
> If channel status bits weren't set, iec958 indicated a sample rate of
> 48kHz in the channel status bits which the HDMI analyzer complained about
> and caused issues with some HDMI devices if it didn't match the real
> rate. That was a long outstanding FIXME since the iec958 plugin was added.
> 
> With this series HD audio passthrough and PCM audio works fine with
> the downstream 5.4 RPi kernel.
> 
> Matthias Reichl (2):
>   pcm: iec958: implement HDMI HBR audio formatting
>   pcm: iec958: set channel status bits according to rate and format

Both patches look like a nice improvement.
Applied now.  Thanks!


Takashi