diff mbox series

[v4,01/16] ALSA: pcm: Introduce MSBITS subformat interface

Message ID 20231116112255.1584795-2-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 Nov. 16, 2023, 11:22 a.m. UTC 
From: Jaroslav Kysela <perex@perex.cz>

Improve granularity of format selection for S32/U32 formats by adding
constants representing 20, 24 and MAX most significant bits.

The MAX means the maximum number of significant bits which can
the physical format hold. For 32-bit formats, MAX is related
to 32 bits. For 8-bit formats, MAX is related to 8 bits etc.

The drivers may use snd_pcm_hw_constraint_subformats with
a simple format -> subformats table.

Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Jaroslav Kysela <perex@perex.cz>
Co-developed-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/sound/pcm.h               |  7 ++++
 include/uapi/sound/asound.h       |  7 ++--
 sound/core/pcm.c                  |  3 ++
 sound/core/pcm_native.c           | 55 +++++++++++++++++++++++++++++--
 tools/include/uapi/sound/asound.h |  7 ++--
 5 files changed, 73 insertions(+), 6 deletions(-)

Comments

Jaroslav Kysela Nov. 16, 2023, 3:10 p.m. UTC | #1 
On 16. 11. 23 12:22, Cezary Rojewski wrote:
> From: Jaroslav Kysela <perex@perex.cz>
> 
> Improve granularity of format selection for S32/U32 formats by adding
> constants representing 20, 24 and MAX most significant bit >
> The MAX means the maximum number of significant bits which can
> the physical format hold. For 32-bit formats, MAX is related
> to 32 bits. For 8-bit formats, MAX is related to 8 bits etc.
> 
> The drivers may use snd_pcm_hw_constraint_subformats with
> a simple format -> subformats table.

I am afraid, the above sentence is no more correct and the current code does 
not follow my original idea. It's a bit step back to the initial code. But I 
admit that from the API POV, it's workable now (with the added refine 
mechanism for one format).

As noted several times, this is not my preferred implementation (I would keep 
only the constraint function which will be called by drivers on demand in the 
ALSA PCM core code). The latest proposed simplification may be applied in the 
ASoC core (store S32_LE subformat mask in snd_soc_pcm_runtime and install this 
intersected constraint for FE PCM - user space). Something like ASoC core does 
for the msbits constraint.

If nobody else thinks that it's a good direction, please, add a note to the 
comment that this implementation (extend snd_pcm_hardware structure) is a 
compromise for the ASoC code with details.

> +		if (f == SNDRV_PCM_FORMAT_S32_LE)

Missing mask check: (f == SNDRV_PCM_FORMAT_S32_LE && *subformats)

Otherwise the MSBITS_MAX won't be set for S32_LE by default.

> +			m.bits[0] |= *subformats;

				Jaroslav
Cezary Rojewski Nov. 17, 2023, 11:45 a.m. UTC | #2 
On 2023-11-16 4:10 PM, Jaroslav Kysela wrote:
> On 16. 11. 23 12:22, Cezary Rojewski wrote:
>> From: Jaroslav Kysela <perex@perex.cz>
>>
>> Improve granularity of format selection for S32/U32 formats by adding
>> constants representing 20, 24 and MAX most significant bit >
>> The MAX means the maximum number of significant bits which can
>> the physical format hold. For 32-bit formats, MAX is related
>> to 32 bits. For 8-bit formats, MAX is related to 8 bits etc.
>>
>> The drivers may use snd_pcm_hw_constraint_subformats with
>> a simple format -> subformats table.
> 
> I am afraid, the above sentence is no more correct and the current code 
> does not follow my original idea. It's a bit step back to the initial 
> code. But I admit that from the API POV, it's workable now (with the 
> added refine mechanism for one format).

I'll update the commit message, indeed no longer up-to date. Will also 
try to explain the thought process which is currently missing, as you 
noted below.

> As noted several times, this is not my preferred implementation (I would 
> keep only the constraint function which will be called by drivers on 
> demand in the ALSA PCM core code). The latest proposed simplification 
> may be applied in the ASoC core (store S32_LE subformat mask in 
> snd_soc_pcm_runtime and install this intersected constraint for FE PCM - 
> user space). Something like ASoC core does for the msbits constraint.
> 
> If nobody else thinks that it's a good direction, please, add a note to 
> the comment that this implementation (extend snd_pcm_hardware structure) 
> is a compromise for the ASoC code with details.

I'd like to avoid complicating ASoC runtime structures as storing 
hw_params is not among their current task list, at least not currently.

But I'm open for any improvements in the future.

>> +        if (f == SNDRV_PCM_FORMAT_S32_LE)
> 
> Missing mask check: (f == SNDRV_PCM_FORMAT_S32_LE && *subformats)
> 
> Otherwise the MSBITS_MAX won't be set for S32_LE by default.

Ack.

>> +            m.bits[0] |= *subformats;
> 
>                  Jaroslav
>
diff mbox series

Patch

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 2a815373dac1..cc175c623dac 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -32,6 +32,7 @@ 
 struct snd_pcm_hardware {
 	unsigned int info;		/* SNDRV_PCM_INFO_* */
 	u64 formats;			/* SNDRV_PCM_FMTBIT_* */
+	u32 subformats;			/* for S32_LE, SNDRV_PCM_SUBFMTBIT_* */
 	unsigned int rates;		/* SNDRV_PCM_RATE_* */
 	unsigned int rate_min;		/* min rate */
 	unsigned int rate_max;		/* max rate */
@@ -217,6 +218,12 @@  struct snd_pcm_ops {
 #define SNDRV_PCM_FMTBIT_U20		SNDRV_PCM_FMTBIT_U20_BE
 #endif
 
+#define _SNDRV_PCM_SUBFMTBIT(fmt)	BIT((__force int)SNDRV_PCM_SUBFORMAT_##fmt)
+#define SNDRV_PCM_SUBFMTBIT_STD		_SNDRV_PCM_SUBFMTBIT(STD)
+#define SNDRV_PCM_SUBFMTBIT_MSBITS_MAX	_SNDRV_PCM_SUBFMTBIT(MSBITS_MAX)
+#define SNDRV_PCM_SUBFMTBIT_MSBITS_20	_SNDRV_PCM_SUBFMTBIT(MSBITS_20)
+#define SNDRV_PCM_SUBFMTBIT_MSBITS_24	_SNDRV_PCM_SUBFMTBIT(MSBITS_24)
+
 struct snd_pcm_file {
 	struct snd_pcm_substream *substream;
 	int no_compat_mmap;
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index f9939da41122..d5b9cfbd9cea 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -142,7 +142,7 @@  struct snd_hwdep_dsp_image {
  *                                                                           *
  *****************************************************************************/
 
-#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 15)
+#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 16)
 
 typedef unsigned long snd_pcm_uframes_t;
 typedef signed long snd_pcm_sframes_t;
@@ -267,7 +267,10 @@  typedef int __bitwise snd_pcm_format_t;
 
 typedef int __bitwise snd_pcm_subformat_t;
 #define	SNDRV_PCM_SUBFORMAT_STD		((__force snd_pcm_subformat_t) 0)
-#define	SNDRV_PCM_SUBFORMAT_LAST	SNDRV_PCM_SUBFORMAT_STD
+#define	SNDRV_PCM_SUBFORMAT_MSBITS_MAX	((__force snd_pcm_subformat_t) 1)
+#define	SNDRV_PCM_SUBFORMAT_MSBITS_20	((__force snd_pcm_subformat_t) 2)
+#define	SNDRV_PCM_SUBFORMAT_MSBITS_24	((__force snd_pcm_subformat_t) 3)
+#define	SNDRV_PCM_SUBFORMAT_LAST	SNDRV_PCM_SUBFORMAT_MSBITS_24
 
 #define SNDRV_PCM_INFO_MMAP		0x00000001	/* hardware supports mmap */
 #define SNDRV_PCM_INFO_MMAP_VALID	0x00000002	/* period data are valid during transfer */
diff --git a/sound/core/pcm.c b/sound/core/pcm.c
index 20bb2d7c8d4b..c4bc15f048b6 100644
--- a/sound/core/pcm.c
+++ b/sound/core/pcm.c
@@ -265,6 +265,9 @@  static const char * const snd_pcm_access_names[] = {
 
 static const char * const snd_pcm_subformat_names[] = {
 	SUBFORMAT(STD), 
+	SUBFORMAT(MSBITS_MAX),
+	SUBFORMAT(MSBITS_20),
+	SUBFORMAT(MSBITS_24),
 };
 
 static const char * const snd_pcm_tstamp_mode_names[] = {
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c
index f610b08f5a2b..5a9faa701059 100644
--- a/sound/core/pcm_native.c
+++ b/sound/core/pcm_native.c
@@ -479,6 +479,7 @@  static int fixup_unreferenced_params(struct snd_pcm_substream *substream,
 {
 	const struct snd_interval *i;
 	const struct snd_mask *m;
+	struct snd_mask *m_rw;
 	int err;
 
 	if (!params->msbits) {
@@ -487,6 +488,22 @@  static int fixup_unreferenced_params(struct snd_pcm_substream *substream,
 			params->msbits = snd_interval_value(i);
 	}
 
+	if (params->msbits) {
+		m = hw_param_mask_c(params, SNDRV_PCM_HW_PARAM_FORMAT);
+		if (snd_mask_single(m)) {
+			snd_pcm_format_t format = (__force snd_pcm_format_t)snd_mask_min(m);
+
+			if (snd_pcm_format_linear(format) &&
+			    snd_pcm_format_width(format) != params->msbits) {
+				m_rw = hw_param_mask(params, SNDRV_PCM_HW_PARAM_SUBFORMAT);
+				snd_mask_reset(m_rw,
+					       (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX);
+				if (snd_mask_empty(m_rw))
+					return -EINVAL;
+			}
+		}
+	}
+
 	if (!params->rate_den) {
 		i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_RATE);
 		if (snd_interval_single(i)) {
@@ -2483,6 +2500,41 @@  static int snd_pcm_hw_rule_buffer_bytes_max(struct snd_pcm_hw_params *params,
 	return snd_interval_refine(hw_param_interval(params, rule->var), &t);
 }		
 
+static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params,
+				      struct snd_pcm_hw_rule *rule)
+{
+	struct snd_mask *sfmask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_SUBFORMAT);
+	struct snd_mask *fmask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
+	u32 *subformats = rule->private;
+	snd_pcm_format_t f;
+	struct snd_mask m;
+
+	snd_mask_none(&m);
+	/* All PCMs support at least the default STD subformat. */
+	snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_STD);
+
+	pcm_for_each_format(f) {
+		if (!snd_mask_test(fmask, (__force unsigned)f))
+			continue;
+
+		if (f == SNDRV_PCM_FORMAT_S32_LE)
+			m.bits[0] |= *subformats;
+		else if (snd_pcm_format_linear(f))
+			snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX);
+	}
+
+	return snd_mask_refine(sfmask, &m);
+}
+
+static int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime,
+					   unsigned int cond, u32 *subformats)
+{
+	return snd_pcm_hw_rule_add(runtime, cond, -1,
+				   snd_pcm_hw_rule_subformats, (void *)subformats,
+				   SNDRV_PCM_HW_PARAM_SUBFORMAT,
+				   SNDRV_PCM_HW_PARAM_FORMAT, -1);
+}
+
 static int snd_pcm_hw_constraints_init(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
@@ -2634,8 +2686,7 @@  static int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream)
 	if (err < 0)
 		return err;
 
-	err = snd_pcm_hw_constraint_mask(runtime, SNDRV_PCM_HW_PARAM_SUBFORMAT,
-					 PARAM_MASK_BIT(SNDRV_PCM_SUBFORMAT_STD));
+	err = snd_pcm_hw_constraint_subformats(runtime, 0, &hw->subformats);
 	if (err < 0)
 		return err;
 
diff --git a/tools/include/uapi/sound/asound.h b/tools/include/uapi/sound/asound.h
index f9939da41122..d5b9cfbd9cea 100644
--- a/tools/include/uapi/sound/asound.h
+++ b/tools/include/uapi/sound/asound.h
@@ -142,7 +142,7 @@  struct snd_hwdep_dsp_image {
  *                                                                           *
  *****************************************************************************/
 
-#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 15)
+#define SNDRV_PCM_VERSION		SNDRV_PROTOCOL_VERSION(2, 0, 16)
 
 typedef unsigned long snd_pcm_uframes_t;
 typedef signed long snd_pcm_sframes_t;
@@ -267,7 +267,10 @@  typedef int __bitwise snd_pcm_format_t;
 
 typedef int __bitwise snd_pcm_subformat_t;
 #define	SNDRV_PCM_SUBFORMAT_STD		((__force snd_pcm_subformat_t) 0)
-#define	SNDRV_PCM_SUBFORMAT_LAST	SNDRV_PCM_SUBFORMAT_STD
+#define	SNDRV_PCM_SUBFORMAT_MSBITS_MAX	((__force snd_pcm_subformat_t) 1)
+#define	SNDRV_PCM_SUBFORMAT_MSBITS_20	((__force snd_pcm_subformat_t) 2)
+#define	SNDRV_PCM_SUBFORMAT_MSBITS_24	((__force snd_pcm_subformat_t) 3)
+#define	SNDRV_PCM_SUBFORMAT_LAST	SNDRV_PCM_SUBFORMAT_MSBITS_24
 
 #define SNDRV_PCM_INFO_MMAP		0x00000001	/* hardware supports mmap */
 #define SNDRV_PCM_INFO_MMAP_VALID	0x00000002	/* period data are valid during transfer */