diff mbox series

[RFC,05/17] ALSA: hda: Upgrade stream-format infrastructure

Message ID 20230811164853.1797547-6-cezary.rojewski@intel.com (mailing list archive)
State Superseded
Headers show
Series ALSA/ASoC: hda: Address format selection limitations and ambiguity | expand

Commit Message

Cezary Rojewski Aug. 11, 2023, 4:48 p.m. UTC
Introduce a set of functions that facilite SDxFMT-related calculations
in atomic manner:

snd_hdac_format_normalize() - format converter. S20_LE, S24_LE and their
unsigned and BE friends are invalid from HDAudio perspective but still
can be specified as function argument due to compatibility reasons.

snd_hdac_stream_format_bps() - obtain just the bits-per-sample value.
Does not ignore subformat and msbits parameters.

snd_hdac_stream_format() and snd_hdac_spdif_stream_format() - obtain the
SDxFMT value given the audio format parameters. The former is stripped
away of spdif-related information. Useful for users that do not care
about them.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/sound/hdaudio.h |   5 ++
 sound/hda/hdac_device.c | 124 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+)

Comments

Takashi Iwai Aug. 14, 2023, 2:41 p.m. UTC | #1
On Fri, 11 Aug 2023 18:48:41 +0200,
Cezary Rojewski wrote:
> 
> Introduce a set of functions that facilite SDxFMT-related calculations
> in atomic manner:
> 
> snd_hdac_format_normalize() - format converter. S20_LE, S24_LE and their
> unsigned and BE friends are invalid from HDAudio perspective but still
> can be specified as function argument due to compatibility reasons.

Which compatibility reason?  Those formats should have been never used
for HD-audio codecs at the first place, so shouldn't it rather return
an error?


thanks,

Takashi
Cezary Rojewski Aug. 14, 2023, 2:47 p.m. UTC | #2
On 2023-08-14 4:41 PM, Takashi Iwai wrote:
> On Fri, 11 Aug 2023 18:48:41 +0200,
> Cezary Rojewski wrote:
>>
>> Introduce a set of functions that facilite SDxFMT-related calculations
>> in atomic manner:
>>
>> snd_hdac_format_normalize() - format converter. S20_LE, S24_LE and their
>> unsigned and BE friends are invalid from HDAudio perspective but still
>> can be specified as function argument due to compatibility reasons.
> 
> Which compatibility reason?  Those formats should have been never used
> for HD-audio codecs at the first place, so shouldn't it rather return
> an error?

In regard to avs-driver we can "force" the S24_LE out, no problem. 
However, I do not believe the same is true for the skylake-driver. I 
agree, S24_LE and its friends should not have been used, but they were.


Czarek
Takashi Iwai Aug. 14, 2023, 2:59 p.m. UTC | #3
On Mon, 14 Aug 2023 16:47:05 +0200,
Cezary Rojewski wrote:
> 
> On 2023-08-14 4:41 PM, Takashi Iwai wrote:
> > On Fri, 11 Aug 2023 18:48:41 +0200,
> > Cezary Rojewski wrote:
> >> 
> >> Introduce a set of functions that facilite SDxFMT-related calculations
> >> in atomic manner:
> >> 
> >> snd_hdac_format_normalize() - format converter. S20_LE, S24_LE and their
> >> unsigned and BE friends are invalid from HDAudio perspective but still
> >> can be specified as function argument due to compatibility reasons.
> > 
> > Which compatibility reason?  Those formats should have been never used
> > for HD-audio codecs at the first place, so shouldn't it rather return
> > an error?
> 
> In regard to avs-driver we can "force" the S24_LE out, no
> problem. However, I do not believe the same is true for the
> skylake-driver. I agree, S24_LE and its friends should not have been
> used, but they were.

Hm, if those formats are actually passed to the codec side, then it's
rather a bug, I suppose.  It can be used for the controller where DSP
converts the format, though.


Takashi
diff mbox series

Patch

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index a549956e102a..f6d76c23ffa4 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -139,6 +139,11 @@  int snd_hdac_get_connections(struct hdac_device *codec, hda_nid_t nid,
 			     hda_nid_t *conn_list, int max_conns);
 int snd_hdac_get_sub_nodes(struct hdac_device *codec, hda_nid_t nid,
 			   hda_nid_t *start_id);
+unsigned int snd_hdac_stream_format_bps(snd_pcm_format_t format, snd_pcm_subformat_t subformat,
+					unsigned int maxbps);
+unsigned int snd_hdac_stream_format(unsigned int channels, unsigned int bps, unsigned int rate);
+unsigned int snd_hdac_spdif_stream_format(unsigned int channels, unsigned int bps,
+					  unsigned int rate, unsigned short spdif_ctls);
 unsigned int snd_hdac_calc_stream_format(unsigned int rate,
 					 unsigned int channels,
 					 snd_pcm_format_t format,
diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c
index 5b01a2b9033d..e592469d63b9 100644
--- a/sound/hda/hdac_device.c
+++ b/sound/hda/hdac_device.c
@@ -13,6 +13,7 @@ 
 #include <sound/hdaudio.h>
 #include <sound/hda_regmap.h>
 #include <sound/pcm.h>
+#include <sound/pcm_params.h>
 #include "local.h"
 
 static void setup_fg_nodes(struct hdac_device *codec);
@@ -724,6 +725,129 @@  static const struct hda_rate_tbl rate_bits[] = {
 	{ 0 } /* terminator */
 };
 
+static snd_pcm_format_t snd_hdac_format_normalize(snd_pcm_format_t format)
+{
+	switch (format) {
+	case SNDRV_PCM_FORMAT_S20_LE:
+	case SNDRV_PCM_FORMAT_S24_LE:
+		return SNDRV_PCM_FORMAT_S32_LE;
+
+	case SNDRV_PCM_FORMAT_U20_LE:
+	case SNDRV_PCM_FORMAT_U24_LE:
+		return SNDRV_PCM_FORMAT_U32_LE;
+
+	case SNDRV_PCM_FORMAT_S20_BE:
+	case SNDRV_PCM_FORMAT_S24_BE:
+		return SNDRV_PCM_FORMAT_S32_BE;
+
+	case SNDRV_PCM_FORMAT_U20_BE:
+	case SNDRV_PCM_FORMAT_U24_BE:
+		return SNDRV_PCM_FORMAT_U32_BE;
+
+	default:
+		return format;
+	}
+}
+
+/**
+ * snd_hdac_stream_format_bps - obtain bits per sample value.
+ * @format:	the PCM format.
+ * @subformat:	the PCM subformat.
+ * @maxbps:	the maximum bits per sample.
+ *
+ * Return: The number of bits per sample.
+ */
+unsigned int snd_hdac_stream_format_bps(snd_pcm_format_t format, snd_pcm_subformat_t subformat,
+					unsigned int maxbps)
+{
+	struct snd_pcm_hw_params params;
+	unsigned int bps;
+
+	memset(&params, 0, sizeof(params));
+
+	params_set_format(&params, snd_hdac_format_normalize(format));
+	snd_mask_set(hw_param_mask(&params, SNDRV_PCM_HW_PARAM_SUBFORMAT),
+		     (__force unsigned int)subformat);
+
+	bps = params_bps(&params);
+	if (maxbps)
+		return min(bps, maxbps);
+	return bps;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_stream_format_bps);
+
+/**
+ * snd_hdac_stream_format - convert format parameters to SDxFMT value.
+ * @channels:	the number of channels.
+ * @bps:	bits per sample.
+ * @rate:	the sample rate.
+ *
+ * Return: The format bitset or zero if invalid.
+ */
+unsigned int snd_hdac_stream_format(unsigned int channels, unsigned int bps, unsigned int rate)
+{
+	unsigned int val = 0;
+	int i;
+
+	for (i = 0; rate_bits[i].hz; i++) {
+		if (rate_bits[i].hz == rate) {
+			val = rate_bits[i].hda_fmt;
+			break;
+		}
+	}
+
+	if (!rate_bits[i].hz)
+		return 0;
+
+	if (channels == 0 || channels > 8)
+		return 0;
+	val |= channels - 1;
+
+	switch (bps) {
+	case 8:
+		val |= AC_FMT_BITS_8;
+		break;
+	case 16:
+		val |= AC_FMT_BITS_16;
+		break;
+	case 20:
+		val |= AC_FMT_BITS_20;
+		break;
+	case 24:
+		val |= AC_FMT_BITS_24;
+		break;
+	case 32:
+		val |= AC_FMT_BITS_32;
+		break;
+	default:
+		return 0;
+	}
+
+	return val;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_stream_format);
+
+/**
+ * snd_hdac_spdif_stream_format - convert format parameters to SDxFMT value.
+ * @channels:	the number of channels.
+ * @bps:	bits per sample.
+ * @rate:	the sample rate.
+ * @spdif_ctls:	HD-audio SPDIF status bits (0 if irrelevant).
+ *
+ * Return: The format bitset or zero if invalid.
+ */
+unsigned int snd_hdac_spdif_stream_format(unsigned int channels, unsigned int bps,
+					  unsigned int rate, unsigned short spdif_ctls)
+{
+	unsigned int val = snd_hdac_stream_format(channels, bps, rate);
+
+	if (val && spdif_ctls & AC_DIG1_NONAUDIO)
+		val |= AC_FMT_TYPE_NON_PCM;
+
+	return val;
+}
+EXPORT_SYMBOL_GPL(snd_hdac_spdif_stream_format);
+
 /**
  * snd_hdac_calc_stream_format - calculate the format bitset
  * @rate: the sample rate