diff mbox series

[v2,01/17] ALSA: pcm: Introduce MSBITS subformat interface

Message ID 20230918133940.3676091-2-cezary.rojewski@intel.com (mailing list archive)
State New, archived
Headers show
Series ALSA/ASoC: hda: Address format selection limitations and ambiguity | expand

Commit Message

Cezary Rojewski Sept. 18, 2023, 1:39 p.m. UTC
Improve granularity of format selection for S32/U32 formats by adding
constants representing 20, 24 and MAX most significant bits.

To make it easy for drivers to utilize those constants, introduce
snd_pcm_subformat_width() and snd_pcm_hw_params_bps(). While the former
is self-explanatory, the latter returns the bit-per-sample value based
on format and subformat characteristics of the provided hw_params.
snd_pcm_hw_copy() helps with copying a hardware parameters space as with
introduction of subformats the operations becomes non-trivial.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 include/sound/pcm.h               | 14 +++++++++
 include/sound/pcm_params.h        |  2 ++
 include/uapi/sound/asound.h       |  7 +++--
 sound/core/pcm.c                  |  4 +++
 sound/core/pcm_lib.c              | 28 +++++++++++++++++
 sound/core/pcm_misc.c             | 51 +++++++++++++++++++++++++++++++
 tools/include/uapi/sound/asound.h |  7 +++--
 7 files changed, 109 insertions(+), 4 deletions(-)

Comments

Jaroslav Kysela Sept. 21, 2023, 6:25 a.m. UTC | #1
On 18. 09. 23 15:39, Cezary Rojewski wrote:
> Improve granularity of format selection for S32/U32 formats by adding
> constants representing 20, 24 and MAX most significant bits.
> 
> To make it easy for drivers to utilize those constants, introduce
> snd_pcm_subformat_width() and snd_pcm_hw_params_bps(). While the former
> is self-explanatory, the latter returns the bit-per-sample value based
> on format and subformat characteristics of the provided hw_params.
> snd_pcm_hw_copy() helps with copying a hardware parameters space as with
> introduction of subformats the operations becomes non-trivial.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>

...

>   struct snd_pcm_hardware {
>   	unsigned int info;		/* SNDRV_PCM_INFO_* */
>   	u64 formats;			/* SNDRV_PCM_FMTBIT_* */
> +	struct snd_pcm_subformat *subformats;

I don't think that it's required to add subformats to the hardware template. 
The new constraint can handle subformat refining without the template 
modifications (pointer to map table is stored privately to this constraint).

Also, I miss the constraint handling here. Without the constraint, the new API 
is not functional and it does not make sense to split the constraint code to 
other patch.

> +int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p);

This may be probably inline function. See bellow.

> +	kfree(runtime->hw.subformats);

Do we really need to do an assumption about allocations for this field? The 
driver may use a static structure. No problem, when this is not added to 
runtime->hw.

> +int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p)
> +{
> +	snd_pcm_subformat_t subformat = params_subformat(p);
> +	snd_pcm_format_t format = params_format(p);
> +	int width;
> +
> +	switch (format) {
> +	case SNDRV_PCM_FORMAT_S32_LE:
> +	case SNDRV_PCM_FORMAT_U32_LE:
> +	case SNDRV_PCM_FORMAT_S32_BE:
> +	case SNDRV_PCM_FORMAT_U32_BE:
> +		width = snd_pcm_subformat_width(subformat);

This is not a correct implementation. The width should be returned for MAX 
subformat (== snd_pcm_format_width value). See bellow.

> +int snd_pcm_subformat_width(snd_pcm_subformat_t subformat)

This function should probably have two arguments - format + subformat to 
return the correct information. The MAX subformat should return 
snd_pcm_format_width value.

> +int snd_pcm_hw_copy(struct snd_pcm_hardware *hw, const struct snd_pcm_hardware *from)

This function is not required, if only the constraint function handles the 
subformat refining.

						Jaroslav
Cezary Rojewski Nov. 14, 2023, 8:09 p.m. UTC | #2
On 2023-09-21 8:25 AM, Jaroslav Kysela wrote:
> On 18. 09. 23 15:39, Cezary Rojewski wrote:
>> Improve granularity of format selection for S32/U32 formats by adding
>> constants representing 20, 24 and MAX most significant bits.
>>
>> To make it easy for drivers to utilize those constants, introduce
>> snd_pcm_subformat_width() and snd_pcm_hw_params_bps(). While the former
>> is self-explanatory, the latter returns the bit-per-sample value based
>> on format and subformat characteristics of the provided hw_params.
>> snd_pcm_hw_copy() helps with copying a hardware parameters space as with
>> introduction of subformats the operations becomes non-trivial.
>>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> 
> ...
> 
>>   struct snd_pcm_hardware {
>>       unsigned int info;        /* SNDRV_PCM_INFO_* */
>>       u64 formats;            /* SNDRV_PCM_FMTBIT_* */
>> +    struct snd_pcm_subformat *subformats;
> 
> I don't think that it's required to add subformats to the hardware 
> template. The new constraint can handle subformat refining without the 
> template modifications (pointer to map table is stored privately to this 
> constraint).

After iterating over the feedback, modified the field to u32 and all the 
handlers to acknowledge that it is S32_LE-specific. I agree with 
statement that we can expand on the API in the future if there's demand 
for it.

> Also, I miss the constraint handling here. Without the constraint, the 
> new API is not functional and it does not make sense to split the 
> constraint code to other patch.

Ack, merging the two.

>> +int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p);
> 
> This may be probably inline function. See bellow.
> 
>> +    kfree(runtime->hw.subformats);
> 
> Do we really need to do an assumption about allocations for this field? 
> The driver may use a static structure. No problem, when this is not 
> added to runtime->hw.

This code is gone in v3 since there is no need to kfree() a u32.

>> +int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p)
>> +{
>> +    snd_pcm_subformat_t subformat = params_subformat(p);
>> +    snd_pcm_format_t format = params_format(p);
>> +    int width;
>> +
>> +    switch (format) {
>> +    case SNDRV_PCM_FORMAT_S32_LE:
>> +    case SNDRV_PCM_FORMAT_U32_LE:
>> +    case SNDRV_PCM_FORMAT_S32_BE:
>> +    case SNDRV_PCM_FORMAT_U32_BE:
>> +        width = snd_pcm_subformat_width(subformat);
> 
> This is not a correct implementation. The width should be returned for 
> MAX subformat (== snd_pcm_format_width value). See bellow.

snd_pcm_subformat_width() returns 0 if subformat==MAX and fallthrough 
ensures snd_pcm_format_width() is returned in such case.

>> +int snd_pcm_subformat_width(snd_pcm_subformat_t subformat)
> 
> This function should probably have two arguments - format + subformat to 
> return the correct information. The MAX subformat should return 
> snd_pcm_format_width value.

My intention is to have two granular functions. One for obtaining 
subformat-width explicitly and the other for calculating bits-per-sample.

>> +int snd_pcm_hw_copy(struct snd_pcm_hardware *hw, const struct 
>> snd_pcm_hardware *from)
> 
> This function is not required, if only the constraint function handles 
> the subformat refining.

Ack, removing in v3.
diff mbox series

Patch

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 2a815373dac1..ddacfedba3ab 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -29,9 +29,15 @@ 
  *  Hardware (lowlevel) section
  */
 
+struct snd_pcm_subformat {
+	snd_pcm_format_t format;	/* SNDRV_PCM_FORMAT_* */
+	u32 mask;			/* Mask of SNDRV_PCM_SUBFMTBIT_* */
+};
+
 struct snd_pcm_hardware {
 	unsigned int info;		/* SNDRV_PCM_INFO_* */
 	u64 formats;			/* SNDRV_PCM_FMTBIT_* */
+	struct snd_pcm_subformat *subformats;
 	unsigned int rates;		/* SNDRV_PCM_RATE_* */
 	unsigned int rate_min;		/* min rate */
 	unsigned int rate_max;		/* max rate */
@@ -217,6 +223,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;
@@ -1128,6 +1140,7 @@  int snd_pcm_format_physical_width(snd_pcm_format_t format);		/* in bits */
 ssize_t snd_pcm_format_size(snd_pcm_format_t format, size_t samples);
 const unsigned char *snd_pcm_format_silence_64(snd_pcm_format_t format);
 int snd_pcm_format_set_silence(snd_pcm_format_t format, void *buf, unsigned int frames);
+int snd_pcm_subformat_width(snd_pcm_subformat_t subformat);
 
 void snd_pcm_set_ops(struct snd_pcm * pcm, int direction,
 		     const struct snd_pcm_ops *ops);
@@ -1196,6 +1209,7 @@  snd_pcm_kernel_readv(struct snd_pcm_substream *substream,
 	return __snd_pcm_lib_xfer(substream, bufs, false, frames, true);
 }
 
+int snd_pcm_hw_copy(struct snd_pcm_hardware *hw, const struct snd_pcm_hardware *from);
 int snd_pcm_hw_limit_rates(struct snd_pcm_hardware *hw);
 
 static inline int
diff --git a/include/sound/pcm_params.h b/include/sound/pcm_params.h
index ba184f49f7e1..df17c7d3e853 100644
--- a/include/sound/pcm_params.h
+++ b/include/sound/pcm_params.h
@@ -362,6 +362,8 @@  static inline int params_physical_width(const struct snd_pcm_hw_params *p)
 	return snd_pcm_format_physical_width(params_format(p));
 }
 
+int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p);
+
 static inline void
 params_set_format(struct snd_pcm_hw_params *p, snd_pcm_format_t fmt)
 {
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..74d7f244e81c 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[] = {
@@ -999,6 +1002,7 @@  void snd_pcm_detach_substream(struct snd_pcm_substream *substream)
 	free_pages_exact(runtime->control,
 		       PAGE_ALIGN(sizeof(struct snd_pcm_mmap_control)));
 	kfree(runtime->hw_constraints.rules);
+	kfree(runtime->hw.subformats);
 	/* Avoid concurrent access to runtime via PCM timer interface */
 	if (substream->timer) {
 		spin_lock_irq(&substream->timer->lock);
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index a11cd7d6295f..05f649b0bf00 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1706,6 +1706,34 @@  int snd_pcm_hw_param_last(struct snd_pcm_substream *pcm,
 }
 EXPORT_SYMBOL(snd_pcm_hw_param_last);
 
+/**
+ * snd_pcm_hw_params_bps - Get the number of bits per the sample.
+ * @p: hardware parameters
+ *
+ * Return: The number of bits per sample based on the format,
+ * subformat and msbits the specified hw params has.
+ */
+int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p)
+{
+	snd_pcm_subformat_t subformat = params_subformat(p);
+	snd_pcm_format_t format = params_format(p);
+	int width;
+
+	switch (format) {
+	case SNDRV_PCM_FORMAT_S32_LE:
+	case SNDRV_PCM_FORMAT_U32_LE:
+	case SNDRV_PCM_FORMAT_S32_BE:
+	case SNDRV_PCM_FORMAT_U32_BE:
+		width = snd_pcm_subformat_width(subformat);
+		if (width)
+			return width;
+		fallthrough;
+	default:
+		return snd_pcm_format_width(format);
+	}
+}
+EXPORT_SYMBOL(snd_pcm_hw_params_bps);
+
 static int snd_pcm_lib_ioctl_reset(struct snd_pcm_substream *substream,
 				   void *arg)
 {
diff --git a/sound/core/pcm_misc.c b/sound/core/pcm_misc.c
index 5588b6a1ee8b..e468b9b82d0c 100644
--- a/sound/core/pcm_misc.c
+++ b/sound/core/pcm_misc.c
@@ -482,6 +482,57 @@  int snd_pcm_format_set_silence(snd_pcm_format_t format, void *data, unsigned int
 }
 EXPORT_SYMBOL(snd_pcm_format_set_silence);
 
+/**
+ * snd_pcm_subformat_width - return the bit-width of the subformat
+ * @subformat: the subformat to check
+ *
+ * Return: The bit-width of the subformat, or 0 if result is dependent
+ * on other parameters in the configuration space.
+ */
+int snd_pcm_subformat_width(snd_pcm_subformat_t subformat)
+{
+	switch (subformat) {
+	case SNDRV_PCM_SUBFORMAT_MSBITS_20:
+		return 20;
+	case SNDRV_PCM_SUBFORMAT_MSBITS_24:
+		return 24;
+	case SNDRV_PCM_SUBFORMAT_MSBITS_MAX:
+	case SNDRV_PCM_SUBFORMAT_STD:
+	default:
+		return 0;
+	}
+}
+EXPORT_SYMBOL(snd_pcm_subformat_width);
+
+/**
+ * snd_pcm_hw_copy - Copy information of one hardware parameters space into another.
+ * @hw: the space to copy the information into.
+ * @from: the space to copy the information from.
+ *
+ * Return: Zero on success, negative error code otherwise.
+ */
+int snd_pcm_hw_copy(struct snd_pcm_hardware *hw, const struct snd_pcm_hardware *from)
+{
+	struct snd_pcm_subformat *sf = NULL;
+	struct snd_pcm_subformat *pos;
+	u32 count = 1; /* At least a sentinel. */
+
+	if (from->subformats) {
+		for (pos = from->subformats; pos->mask; pos++)
+			count++;
+
+		sf = kmemdup(from->subformats, count * sizeof(*from->subformats), GFP_KERNEL);
+		if (!sf)
+			return -ENOMEM;
+		kfree(hw->subformats);
+	}
+
+	*hw = *from;
+	hw->subformats = sf;
+	return 0;
+}
+EXPORT_SYMBOL(snd_pcm_hw_copy);
+
 /**
  * snd_pcm_hw_limit_rates - determine rate_min/rate_max fields
  * @hw: the pcm hw instance
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 */