[PATCH v2 00/17] ALSA/ASoC: hda: Address format selection limitations and ambiguity
Patchset aims to address format selection restrictions present currently in the HDAudio library. Formats which we are concerned about are 20 and 24 valid bits per sample within 32 bit depth container. One may identify them as S20_LE and S24_LE except that those, according to comments found in include/uapi/sound/asound.h, are for LSB-aligned scenarios. HDAudio streams expect MSB-aligned data, no matter if we are speaking of HOST (SDxFMT) or LINK (PPLCxFMT) side - chapter 4.5.1 of the public HDAudio specification. In short, S20_LE and S24_LE are invalid options.
Right now, given the implementation of snd_hdac_query_supported_pcm() within sound/hda/hdac_device.c, even if a codec responds with: "I support all possible formats specified within HDAudio specification", there will be no option to open a 20/32 or 24/32 stream. The kernel will force the stream to be opened using the highest available bit depth.
After discussing subject initially with Jaroslav and Takashi, suggestion was made to utilize 'subformat' option to address the problem. The eye-opening discussion begun much earlier though, in 2019 [1].
Paired with PRs for alsa-utils [2] and alsa-lib [3].
Flow of changes:
The very first patch adds MSBITS subformat options to allow for granular 20/32, 24/32 and 32/32 format selection. The next three make sure subformat is actually honored during runtime. Most of that code is based on format-related API.
Follow up is upgrade to the hda stream-format interface - several functions are added to make the granular format selection simple in the HDAudio world. Core of the implementation is based on the existing snd_hdac_calc_stream_format(). The next ten patches are straightforward switch from one interface to another with cleanup of now-unsed function as a finishing touch.
Last but not least - the avs-driver, on which the problem analyzed and debugged, is updated to no longer acknowledge S24_LE as a valid format option.
Results with skylake-driver and snd_hda_intel show status quo on our RVPs. PR filed on SOF github shows promising results too [4].
Changes in v2: - patch 01/17, introduced struct snd_pcm_subformat which task is to represent subformat-mask on per format basis. Expectation is that manipulated arrays of subformats always end with a sentinel entry - patch 01/17, added snd_pcm_hw_copy() as the copying snd_pcm_hardware becomes non-trivial - patch 02/17, added hw_rule that produces final subformat mask based on provided formats as suggested by Jaroslav - patch 04/17, soc_pcm_hw_update_subformat() refactored as the subformat intersection becomes non-trivial - relevant functions releasing resources occupied by hda_pcm and snd_pcm_runtime updated to also kfree() subformats - except for 16/17, no changes to patches past 04/17, retaining acks for these
Changes in v1: - fixed UBSAN due to missing snd_pcm_subformat_names[] entries for new subformats - as HDMI stream capabilities are assigned on PCM open, patch 16/17 has been updated to ignore such codecs for now. A separate patchset will take care of this case - params_bps() reworded to snd_pcm_hw_params_bps() - fixed compilation issues in sof-driver, patch 13/17
[1]: https://lore.kernel.org/alsa-devel/20190905053302.9262-1-pawel.harlozinski@l... [2]: https://github.com/alsa-project/alsa-utils/pull/228 [3]: https://github.com/alsa-project/alsa-lib/pull/342 [4]: https://github.com/thesofproject/linux/pull/4539
Cezary Rojewski (17): ALSA: pcm: Introduce MSBITS subformat interface ALSA: pcm: Honor subformat when configuring substream ALSA: hda: Honor subformat when querying PCMs ASoC: pcm: Honor subformat when configuring runtime ALSA: hda: Upgrade stream-format infrastructure ALSA: hda: Switch to new stream-format interface ALSA: hda/hdmi: Switch to new stream-format interface ALSA: hda/ca0132: Switch to new stream-format interface ASoC: codecs: hda: Switch to new stream-format interface ASoC: codecs: hdac_hda: Switch to new stream-format interface ASoC: codecs: hdac_hdmi: Switch to new stream-format interface ASoC: Intel Skylake: Switch to new stream-format interface ASoC: SOF: Intel: Switch to new stream-format interface ASoC: Intel: avs: Switch to new stream-format interface ALSA: hda: Drop snd_hdac_calc_stream_format() ASoC: Intel: avs: Kill S24_LE in HDAudio streaming ASoC: Intel: avs: Unhardcode HDAudio BE DAI drivers description
include/sound/hda_codec.h | 5 +- include/sound/hdaudio.h | 14 +-- include/sound/pcm.h | 14 +++ include/sound/pcm_params.h | 2 + include/sound/soc.h | 1 + include/uapi/sound/asound.h | 7 +- sound/core/pcm.c | 4 + sound/core/pcm_lib.c | 28 +++++ sound/core/pcm_misc.c | 51 ++++++++++ sound/core/pcm_native.c | 67 +++++++++++- sound/hda/hdac_device.c | 163 ++++++++++++++++++++++-------- sound/pci/hda/hda_codec.c | 5 + sound/pci/hda/hda_controller.c | 10 +- sound/pci/hda/patch_ca0132.c | 3 +- sound/pci/hda/patch_hdmi.c | 6 +- sound/soc/codecs/hda-dai.c | 6 +- sound/soc/codecs/hda.c | 2 + sound/soc/codecs/hdac_hda.c | 8 +- sound/soc/codecs/hdac_hdmi.c | 10 +- sound/soc/intel/avs/loader.c | 4 +- sound/soc/intel/avs/path.c | 2 +- sound/soc/intel/avs/pcm.c | 54 ++++++++-- sound/soc/intel/avs/probes.c | 3 +- sound/soc/intel/avs/topology.c | 19 +++- sound/soc/intel/skylake/skl-pcm.c | 11 +- sound/soc/soc-pcm.c | 35 ++++++- sound/soc/sof/intel/hda-dai-ops.c | 5 +- tools/include/uapi/sound/asound.h | 7 +- 28 files changed, 443 insertions(+), 103 deletions(-)
base-commit: 564ee9ac788b680e4ec4a6cb3a4a953dc61d5da8
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(-)
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 */
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
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.
Substream value is currently hardcoded to SNDRV_PCM_SUBFORMAT_STD. Update the constraint procedure so that subformat selection is not ignored. Case STD is always supported as most PCMs do not care about subformat.
Suggested-by: Jaroslav Kysela perex@perex.cz Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/core/pcm_native.c | 67 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 65 insertions(+), 2 deletions(-)
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index bd9ddf412b46..2ea856e95fb2 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 *sfmask; int err;
if (!params->msbits) { @@ -487,6 +488,23 @@ 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) { + sfmask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_SUBFORMAT); + snd_mask_reset(sfmask, + (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX); + if (snd_mask_empty(sfmask)) + return -EINVAL; + } + } + } + if (!params->rate_den) { i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_RATE); if (snd_interval_single(i)) { @@ -2483,6 +2501,52 @@ 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); + struct snd_pcm_hardware *hw = rule->private; + struct snd_pcm_subformat *sf; + snd_pcm_format_t f; + struct snd_mask m; + bool found; + + snd_mask_none(&m); + /* All PCMs support at least the default STD subformat. */ + snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_STD); + + if (!hw->subformats) + goto exit; + + pcm_for_each_format(f) { + if (!snd_mask_test(fmask, f)) + continue; + + found = false; + for (sf = hw->subformats; sf->mask && !found; sf++) { + if (sf->format != f) + continue; + m.bits[0] |= sf->mask; + found = true; + } + if (!found && snd_pcm_format_linear(f)) + snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX); + } +exit: + return snd_mask_refine(sfmask, &m); +} + +static int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime, + unsigned int cond, + struct snd_pcm_hardware *hw) +{ + return snd_pcm_hw_rule_add(runtime, cond, -1, + snd_pcm_hw_rule_subformats, (void *)hw, + 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 +2698,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); if (err < 0) return err;
On 18. 09. 23 15:39, Cezary Rojewski wrote:
Substream value is currently hardcoded to SNDRV_PCM_SUBFORMAT_STD. Update the constraint procedure so that subformat selection is not ignored. Case STD is always supported as most PCMs do not care about subformat.
Suggested-by: Jaroslav Kysela perex@perex.cz
Better Co-developed mark. Also I would move whole code to the first patch. It does not make sense to split the mandatory code.
Another option is to increase the protocol version to the separate patch where all necessary code changes are applied (for MSBITS_MAX). But it may break backports, so the change should be aligned with the SUBFMT defines.
- struct snd_mask *sfmask;
m_rw -> sfmask renaming. I prefer universal name to allow easy reuse in future.
for (sf = hw->subformats; sf->mask && !found; sf++) {
My proposal [1] defined SNDRV_PCM_FORMAT_CONSTRAINT_END value not relying to zero format (which is U8) and zero subformat to skip the MSBIT_MAX setting bellow. After some thought, if the driver sets SNDRV_PCM_SUBFORMAT_MSBITS_STD, the result will be similar, thus the mask can be zero and the code may be reduced. So no objection for this change.
if (!found && snd_pcm_format_linear(f))snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX);- }
+exit:
- return snd_mask_refine(sfmask, &m);
+}
+static int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime,
unsigned int cond,struct snd_pcm_hardware *hw)+{
Because your change does not assume that this constraint is called from the drivers, the comments and EXPORT_SYMBOL() lines were removed from the original proposal [1]. I believe that the standalone constraint is better at this time - reduce code, the use of the subformat extension is not mandatory.
Jaroslav
[1] https://lore.kernel.org/alsa-devel/20230912162526.7138-1-perex@perex.cz/ https://lore.kernel.org/alsa-devel/20230913103716.67315-1-perex@perex.cz/
On 2023-09-21 8:41 AM, Jaroslav Kysela wrote:
On 18. 09. 23 15:39, Cezary Rojewski wrote:
Substream value is currently hardcoded to SNDRV_PCM_SUBFORMAT_STD. Update the constraint procedure so that subformat selection is not ignored. Case STD is always supported as most PCMs do not care about subformat.
Suggested-by: Jaroslav Kysela perex@perex.cz
Better Co-developed mark. Also I would move whole code to the first patch. It does not make sense to split the mandatory code.
Another option is to increase the protocol version to the separate patch where all necessary code changes are applied (for MSBITS_MAX). But it may break backports, so the change should be aligned with the SUBFMT defines.
While most of my feedback is below, if we decide that "subformat as first class citizen" approach is good-to-go, this patch gets re-authorized in v3 as the input on the constraint part from your end is major.
+ struct snd_mask *sfmask;
m_rw -> sfmask renaming. I prefer universal name to allow easy reuse in future.
Ack.
+ for (sf = hw->subformats; sf->mask && !found; sf++) {
My proposal [1] defined SNDRV_PCM_FORMAT_CONSTRAINT_END value not relying to zero format (which is U8) and zero subformat to skip the MSBIT_MAX setting bellow. After some thought, if the driver sets SNDRV_PCM_SUBFORMAT_MSBITS_STD, the result will be similar, thus the mask can be zero and the code may be reduced. So no objection for this change.
+ if (!found && snd_pcm_format_linear(f)) + snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX); + } +exit: + return snd_mask_refine(sfmask, &m); +}
+static int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime, + unsigned int cond, + struct snd_pcm_hardware *hw) +{
Because your change does not assume that this constraint is called from the drivers, the comments and EXPORT_SYMBOL() lines were removed from the original proposal [1]. I believe that the standalone constraint is better at this time - reduce code, the use of the subformat extension is not mandatory.
Thank you for thorough feedback, Jaroslav. This is much appreciated. Before I comment on the rest of the comments, let me provide a summary: I believe that most of the subject comes down to: subformat as mainstream API -or- subformat as niche API.
If the general opinion of the developers is: let's go for the latter until we have more users, I have no problem with merging the patches 1 & 2 and addressing most of the review in 1:1 fashion as indeed many parts of the proposed API lose their purpose.
My view is different as I'd like subformat to become mainstream. To be honest, the object that allowed it to happen has been suggested by you Jaroslav - the struct made of { format; mask; }. It allows to describe what subformat _is_ in explicit fashion and by being exposed in snd_pcm_hardware becomes standard part of the API. As previously suggested, I believe it could replace ->msbits in future too.
Question to the fellow developers: What's your take on the subject?
Kind regards, Czarek
[1] https://lore.kernel.org/alsa-devel/20230912162526.7138-1-perex@perex.cz/
https://lore.kernel.org/alsa-devel/20230913103716.67315-1-perex@perex.cz/
Hi Cezary,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 564ee9ac788b680e4ec4a6cb3a4a953dc61d5da8]
url: https://github.com/intel-lab-lkp/linux/commits/Cezary-Rojewski/ALSA-pcm-Intr... base: 564ee9ac788b680e4ec4a6cb3a4a953dc61d5da8 patch link: https://lore.kernel.org/r/20230918133940.3676091-3-cezary.rojewski%40intel.c... patch subject: [PATCH v2 02/17] ALSA: pcm: Honor subformat when configuring substream config: x86_64-randconfig-121-20230923 (https://download.01.org/0day-ci/archive/20230923/202309231825.17qi62Yr-lkp@i...) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230923/202309231825.17qi62Yr-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202309231825.17qi62Yr-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
sound/core/pcm_native.c:2523:43: sparse: sparse: incorrect type in argument 2 (different base types) @@ expected unsigned int val @@ got restricted snd_pcm_format_t [assigned] [usertype] f @@
sound/core/pcm_native.c:2523:43: sparse: expected unsigned int val sound/core/pcm_native.c:2523:43: sparse: got restricted snd_pcm_format_t [assigned] [usertype] f sound/core/pcm_native.c:95:1: sparse: sparse: context imbalance in 'snd_pcm_group_lock' - different lock contexts for basic block sound/core/pcm_native.c:96:1: sparse: sparse: context imbalance in 'snd_pcm_group_unlock' - unexpected unlock sound/core/pcm_native.c:97:1: sparse: sparse: context imbalance in 'snd_pcm_group_lock_irq' - different lock contexts for basic block sound/core/pcm_native.c:98:1: sparse: sparse: context imbalance in 'snd_pcm_group_unlock_irq' - unexpected unlock sound/core/pcm_native.c:145:9: sparse: sparse: context imbalance in 'snd_pcm_stream_lock_nested' - different lock contexts for basic block sound/core/pcm_native.c:171:9: sparse: sparse: context imbalance in '_snd_pcm_stream_lock_irqsave' - different lock contexts for basic block sound/core/pcm_native.c:184:9: sparse: sparse: context imbalance in '_snd_pcm_stream_lock_irqsave_nested' - different lock contexts for basic block sound/core/pcm_native.c:201:39: sparse: sparse: context imbalance in 'snd_pcm_stream_unlock_irqrestore' - unexpected unlock sound/core/pcm_native.c:1279:44: sparse: sparse: context imbalance in 'snd_pcm_action_group' - unexpected unlock sound/core/pcm_native.c:1349:37: sparse: sparse: context imbalance in 'snd_pcm_stream_group_ref' - different lock contexts for basic block sound/core/pcm_native.c: note: in included file (through arch/x86/include/asm/uaccess.h, include/linux/uaccess.h, include/linux/sched/task.h, ...): arch/x86/include/asm/uaccess_64.h:88:24: sparse: sparse: cast removes address space '__user' of expression
vim +2523 sound/core/pcm_native.c
2503 2504 static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params, 2505 struct snd_pcm_hw_rule *rule) 2506 { 2507 struct snd_mask *sfmask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_SUBFORMAT); 2508 struct snd_mask *fmask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); 2509 struct snd_pcm_hardware *hw = rule->private; 2510 struct snd_pcm_subformat *sf; 2511 snd_pcm_format_t f; 2512 struct snd_mask m; 2513 bool found; 2514 2515 snd_mask_none(&m); 2516 /* All PCMs support at least the default STD subformat. */ 2517 snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_STD); 2518 2519 if (!hw->subformats) 2520 goto exit; 2521 2522 pcm_for_each_format(f) {
2523 if (!snd_mask_test(fmask, f))
2524 continue; 2525 2526 found = false; 2527 for (sf = hw->subformats; sf->mask && !found; sf++) { 2528 if (sf->format != f) 2529 continue; 2530 m.bits[0] |= sf->mask; 2531 found = true; 2532 } 2533 if (!found && snd_pcm_format_linear(f)) 2534 snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX); 2535 } 2536 exit: 2537 return snd_mask_refine(sfmask, &m); 2538 } 2539
Update mechanism for querying supported PCMs to allow for granular format selection when container size is 32 bits. Currently always the highest bit depth is selected, regardless of how many actual formats codec in question supports.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/hda_codec.h | 5 ++-- include/sound/hdaudio.h | 4 ++- sound/hda/hdac_device.c | 52 ++++++++++++++++++++++-------------- sound/pci/hda/hda_codec.c | 5 ++++ sound/pci/hda/patch_hdmi.c | 1 + sound/soc/codecs/hdac_hdmi.c | 3 ++- 6 files changed, 46 insertions(+), 24 deletions(-)
diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h index 5497dc9c396a..6db063d5a7fe 100644 --- a/include/sound/hda_codec.h +++ b/include/sound/hda_codec.h @@ -141,6 +141,7 @@ struct hda_pcm_stream { hda_nid_t nid; /* default NID to query rates/formats/bps, or set up */ u32 rates; /* supported rates */ u64 formats; /* supported formats (SNDRV_PCM_FMTBIT_) */ + const struct snd_pcm_subformat *subformats; unsigned int maxbps; /* supported max. bit per sample */ const struct snd_pcm_chmap_elem *chmap; /* chmap to override */ struct hda_pcm_ops ops; @@ -448,8 +449,8 @@ void __snd_hda_codec_cleanup_stream(struct hda_codec *codec, hda_nid_t nid, #define snd_hda_codec_cleanup_stream(codec, nid) \ __snd_hda_codec_cleanup_stream(codec, nid, 0)
-#define snd_hda_query_supported_pcm(codec, nid, ratesp, fmtsp, bpsp) \ - snd_hdac_query_supported_pcm(&(codec)->core, nid, ratesp, fmtsp, bpsp) +#define snd_hda_query_supported_pcm(codec, nid, ratesp, fmtsp, subfmtp, bpsp) \ + snd_hdac_query_supported_pcm(&(codec)->core, nid, ratesp, fmtsp, subfmtp, bpsp) #define snd_hda_is_supported_format(codec, nid, fmt) \ snd_hdac_is_supported_format(&(codec)->core, nid, fmt)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 32c59053b48e..a2f10f7785bb 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -146,7 +146,9 @@ unsigned int snd_hdac_calc_stream_format(unsigned int rate, unsigned int maxbps, unsigned short spdif_ctls); int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid, - u32 *ratesp, u64 *formatsp, unsigned int *bpsp); + u32 *ratesp, u64 *formatsp, + const struct snd_pcm_subformat **subformatsp, + unsigned int *bpsp); bool snd_hdac_is_supported_format(struct hdac_device *codec, hda_nid_t nid, unsigned int format);
diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c index bbf7bcdb449a..a2ce4ea809b3 100644 --- a/sound/hda/hdac_device.c +++ b/sound/hda/hdac_device.c @@ -817,15 +817,18 @@ static unsigned int query_stream_param(struct hdac_device *codec, hda_nid_t nid) * @nid: NID to query * @ratesp: the pointer to store the detected rate bitflags * @formatsp: the pointer to store the detected formats + * @subformatsp: the pointer to store the detected subformats * @bpsp: the pointer to store the detected format widths * - * Queries the supported PCM rates and formats. The NULL @ratesp, @formatsp - * or @bsps argument is ignored. + * Queries the supported PCM rates and formats. The NULL @ratesp, @formatsp, + * @subformatsp or @bpsp argument is ignored. * * Returns 0 if successful, otherwise a negative error code. */ int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid, - u32 *ratesp, u64 *formatsp, unsigned int *bpsp) + u32 *ratesp, u64 *formatsp, + const struct snd_pcm_subformat **subformatsp, + unsigned int *bpsp) { unsigned int i, val, wcaps;
@@ -848,9 +851,13 @@ int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid, *ratesp = rates; }
- if (formatsp || bpsp) { - u64 formats = 0; + if (formatsp || subformatsp || bpsp) { + struct snd_pcm_subformat subformats[] = { + { .format = SNDRV_PCM_FORMAT_S32_LE, }, + {} + }; unsigned int streams, bps; + u64 formats = 0;
streams = query_stream_param(codec, nid); if (!streams) @@ -866,24 +873,24 @@ int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid, formats |= SNDRV_PCM_FMTBIT_S16_LE; bps = 16; } - if (wcaps & AC_WCAP_DIGITAL) { - if (val & AC_SUPPCM_BITS_32) + if (val & AC_SUPPCM_BITS_20) { + formats |= SNDRV_PCM_FMTBIT_S32_LE; + subformats[0].mask |= SNDRV_PCM_SUBFMTBIT_MSBITS_20; + bps = 20; + } + if (val & AC_SUPPCM_BITS_24) { + formats |= SNDRV_PCM_FMTBIT_S32_LE; + subformats[0].mask |= SNDRV_PCM_SUBFMTBIT_MSBITS_24; + bps = 24; + } + if (val & AC_SUPPCM_BITS_32) { + if (wcaps & AC_WCAP_DIGITAL) { formats |= SNDRV_PCM_FMTBIT_IEC958_SUBFRAME_LE; - if (val & (AC_SUPPCM_BITS_20|AC_SUPPCM_BITS_24)) + } else { formats |= SNDRV_PCM_FMTBIT_S32_LE; - if (val & AC_SUPPCM_BITS_24) - bps = 24; - else if (val & AC_SUPPCM_BITS_20) - bps = 20; - } else if (val & (AC_SUPPCM_BITS_20|AC_SUPPCM_BITS_24| - AC_SUPPCM_BITS_32)) { - formats |= SNDRV_PCM_FMTBIT_S32_LE; - if (val & AC_SUPPCM_BITS_32) + subformats[0].mask |= SNDRV_PCM_SUBFMTBIT_MSBITS_MAX; bps = 32; - else if (val & AC_SUPPCM_BITS_24) - bps = 24; - else if (val & AC_SUPPCM_BITS_20) - bps = 20; + } } } #if 0 /* FIXME: CS4206 doesn't work, which is the only codec supporting float */ @@ -911,6 +918,11 @@ int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid, } if (formatsp) *formatsp = formats; + if (subformatsp) { + *subformatsp = kmemdup(subformats, sizeof(subformats), GFP_KERNEL); + if (!*subformatsp) + return -ENOMEM; + } if (bpsp) *bpsp = bps; } diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 33af707a65ab..8cd2db528bbd 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -755,12 +755,15 @@ void snd_hda_codec_disconnect_pcms(struct hda_codec *codec) static void codec_release_pcms(struct hda_codec *codec) { struct hda_pcm *pcm, *n; + int dir;
list_for_each_entry_safe(pcm, n, &codec->pcm_list_head, list) { list_del(&pcm->list); if (pcm->pcm) snd_device_free(pcm->codec->card, pcm->pcm); clear_bit(pcm->device, pcm->codec->bus->pcm_dev_bits); + for_each_pcm_streams(dir) + kfree(pcm->stream[dir].subformats); kfree(pcm->name); kfree(pcm); } @@ -3163,6 +3166,7 @@ static int set_pcm_default_values(struct hda_codec *codec, err = snd_hda_query_supported_pcm(codec, info->nid, info->rates ? NULL : &info->rates, info->formats ? NULL : &info->formats, + info->subformats ? NULL : &info->subformats, info->maxbps ? NULL : &info->maxbps); if (err < 0) return err; @@ -3757,6 +3761,7 @@ int snd_hda_multi_out_analog_open(struct hda_codec *codec, snd_hda_query_supported_pcm(codec, mout->dig_out_nid, &mout->spdif_rates, &mout->spdif_formats, + NULL, &mout->spdif_maxbps); } mutex_lock(&codec->spdif_mutex); diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 1cde2a69bdb4..687b8b8fd7ac 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1977,6 +1977,7 @@ static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t cvt_nid) err = snd_hda_query_supported_pcm(codec, cvt_nid, &per_cvt->rates, &per_cvt->formats, + NULL, &per_cvt->maxbps); if (err < 0) return err; diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index b9c5ffbfb5ba..3e4f632d8665 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -670,6 +670,7 @@ hdac_hdmi_query_cvt_params(struct hdac_device *hdev, struct hdac_hdmi_cvt *cvt) err = snd_hdac_query_supported_pcm(hdev, cvt->nid, &cvt->params.rates, &cvt->params.formats, + NULL, &cvt->params.maxbps); if (err < 0) dev_err(&hdev->dev, @@ -1577,7 +1578,7 @@ static int hdac_hdmi_create_dais(struct hdac_device *hdev,
list_for_each_entry(cvt, &hdmi->cvt_list, head) { ret = snd_hdac_query_supported_pcm(hdev, cvt->nid, - &rates, &formats, &bps); + &rates, &formats, NULL, &bps); if (ret) return ret;
Subformat options are ignored when setting up hardware parameters and assigning PCM stream capabilities. Account for them to allow for granular format selection.
With introduction of subformats, copying hardware parameters becomes a non-trivial operation. Implement a helper function to make things simple.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/soc.h | 1 + sound/soc/soc-pcm.c | 35 ++++++++++++++++++++++++++++++++--- 2 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 81ed08c5c67d..0d3158a7715e 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -620,6 +620,7 @@ enum snd_soc_trigger_order { struct snd_soc_pcm_stream { const char *stream_name; u64 formats; /* SNDRV_PCM_FMTBIT_* */ + const struct snd_pcm_subformat *subformats; unsigned int rates; /* SNDRV_PCM_RATE_* */ unsigned int rate_min; /* min rate */ unsigned int rate_max; /* max rate */ diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 54704250c0a2..b57795feb5f8 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -336,9 +336,7 @@ bool snd_soc_runtime_ignore_pmdown_time(struct snd_soc_pcm_runtime *rtd) int snd_soc_set_runtime_hwparams(struct snd_pcm_substream *substream, const struct snd_pcm_hardware *hw) { - substream->runtime->hw = *hw; - - return 0; + return snd_pcm_hw_copy(&substream->runtime->hw, hw); } EXPORT_SYMBOL_GPL(snd_soc_set_runtime_hwparams);
@@ -554,6 +552,33 @@ static void soc_pcm_hw_update_format(struct snd_pcm_hardware *hw, hw->formats &= p->formats; }
+static void soc_pcm_hw_update_subformat(struct snd_pcm_hardware *hw, + struct snd_soc_pcm_stream *p) +{ + const struct snd_pcm_subformat *sf2; + struct snd_pcm_subformat *sf1; + u32 last = 0; + + if (!hw->subformats || !p->subformats) + return; + + for (sf1 = hw->subformats; sf1->mask; sf1++) + last++; + + for (sf1 = hw->subformats; sf1->mask; sf1++) { + for (sf2 = p->subformats; sf2->mask; sf2++) { + if (sf1->format != sf2->format) + continue; + + sf1->mask &= sf2->mask; + if (!sf1->mask) { + swap(*sf1, hw->subformats[last]); + last--; + } + } + } +} + /** * snd_soc_runtime_calc_hw() - Calculate hw limits for a PCM stream * @rtd: ASoC PCM runtime @@ -592,6 +617,7 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd, soc_pcm_hw_update_chan(hw, cpu_stream); soc_pcm_hw_update_rate(hw, cpu_stream); soc_pcm_hw_update_format(hw, cpu_stream); + soc_pcm_hw_update_subformat(hw, cpu_stream); } cpu_chan_min = hw->channels_min; cpu_chan_max = hw->channels_max; @@ -613,6 +639,7 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd, soc_pcm_hw_update_chan(hw, codec_stream); soc_pcm_hw_update_rate(hw, codec_stream); soc_pcm_hw_update_format(hw, codec_stream); + soc_pcm_hw_update_subformat(hw, codec_stream); }
/* Verify both a valid CPU DAI and a valid CODEC DAI were found */ @@ -1698,6 +1725,7 @@ static void dpcm_runtime_setup_fe(struct snd_pcm_substream *substream) soc_pcm_hw_update_rate(hw, cpu_stream); soc_pcm_hw_update_chan(hw, cpu_stream); soc_pcm_hw_update_format(hw, cpu_stream); + soc_pcm_hw_update_subformat(hw, cpu_stream); }
} @@ -1735,6 +1763,7 @@ static void dpcm_runtime_setup_be_format(struct snd_pcm_substream *substream) codec_stream = snd_soc_dai_get_pcm_stream(dai, stream);
soc_pcm_hw_update_format(hw, codec_stream); + soc_pcm_hw_update_subformat(hw, codec_stream); } } }
On Mon, Sep 18, 2023 at 03:39:27PM +0200, Cezary Rojewski wrote:
Subformat options are ignored when setting up hardware parameters and assigning PCM stream capabilities. Account for them to allow for granular format selection.
Acked-by: Mark Brown broonie@kernel.org
On 18. 09. 23 15:39, Cezary Rojewski wrote:
Subformat options are ignored when setting up hardware parameters and assigning PCM stream capabilities. Account for them to allow for granular format selection.
With introduction of subformats, copying hardware parameters becomes a non-trivial operation. Implement a helper function to make things simple.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com
struct snd_soc_pcm_stream { const char *stream_name; u64 formats; /* SNDRV_PCM_FMTBIT_* */
- const struct snd_pcm_subformat *subformats;
I don't think that this extension is mandatory. The whole patch can be skipped if the driver installs the subformat runtime constraint.
Jaroslav
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(+)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index a2f10f7785bb..589812c6f6ce 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -140,6 +140,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 a2ce4ea809b3..ff5f45b95be4 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); @@ -725,6 +726,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(¶ms, 0, sizeof(params)); + + params_set_format(¶ms, snd_hdac_format_normalize(format)); + snd_mask_set(hw_param_mask(¶ms, SNDRV_PCM_HW_PARAM_SUBFORMAT), + (__force unsigned int)subformat); + + bps = snd_pcm_hw_params_bps(¶ms); + 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
To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/pci/hda/hda_controller.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/sound/pci/hda/hda_controller.c b/sound/pci/hda/hda_controller.c index 406779625fb5..0646ef0afd49 100644 --- a/sound/pci/hda/hda_controller.c +++ b/sound/pci/hda/hda_controller.c @@ -151,7 +151,7 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream) struct azx_dev *azx_dev = get_azx_dev(substream); struct hda_pcm_stream *hinfo = to_hda_pcm_stream(substream); struct snd_pcm_runtime *runtime = substream->runtime; - unsigned int format_val, stream_tag; + unsigned int format_val, stream_tag, bps; int err; struct hda_spdif_out *spdif = snd_hda_spdif_out_of_nid(apcm->codec, hinfo->nid); @@ -165,11 +165,9 @@ static int azx_pcm_prepare(struct snd_pcm_substream *substream) }
snd_hdac_stream_reset(azx_stream(azx_dev)); - format_val = snd_hdac_calc_stream_format(runtime->rate, - runtime->channels, - runtime->format, - hinfo->maxbps, - ctls); + bps = snd_hdac_stream_format_bps(runtime->format, SNDRV_PCM_SUBFORMAT_STD, hinfo->maxbps); + + format_val = snd_hdac_spdif_stream_format(runtime->channels, bps, runtime->rate, ctls); if (!format_val) { dev_err(chip->card->dev, "invalid format_val, rate=%d, ch=%d, format=%d\n",
To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/pci/hda/patch_hdmi.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 687b8b8fd7ac..dff2d7221982 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -1655,7 +1655,6 @@ static void hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
#define I915_SILENT_RATE 48000 #define I915_SILENT_CHANNELS 2 -#define I915_SILENT_FORMAT SNDRV_PCM_FORMAT_S16_LE #define I915_SILENT_FORMAT_BITS 16 #define I915_SILENT_FMT_MASK 0xf
@@ -1668,8 +1667,8 @@ static void silent_stream_enable_i915(struct hda_codec *codec, per_pin->dev_id, I915_SILENT_RATE);
/* trigger silent stream generation in hw */ - format = snd_hdac_calc_stream_format(I915_SILENT_RATE, I915_SILENT_CHANNELS, - I915_SILENT_FORMAT, I915_SILENT_FORMAT_BITS, 0); + format = snd_hdac_stream_format(I915_SILENT_CHANNELS, I915_SILENT_FORMAT_BITS, + I915_SILENT_RATE); snd_hda_codec_setup_stream(codec, per_pin->cvt_nid, I915_SILENT_FMT_MASK, I915_SILENT_FMT_MASK, format); usleep_range(100, 200);
To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/pci/hda/patch_ca0132.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/pci/hda/patch_ca0132.c b/sound/pci/hda/patch_ca0132.c index 748a3c40966e..aa312441604f 100644 --- a/sound/pci/hda/patch_ca0132.c +++ b/sound/pci/hda/patch_ca0132.c @@ -3022,8 +3022,7 @@ static int dma_convert_to_hda_format(struct hda_codec *codec, { unsigned int format_val;
- format_val = snd_hdac_calc_stream_format(sample_rate, - channels, SNDRV_PCM_FORMAT_S32_LE, 32, 0); + format_val = snd_hdac_stream_format(channels, 32, sample_rate);
if (hda_format) *hda_format = (unsigned short)format_val;
To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism.
While at it, complete PCM stream initialization with subformat options.
Acked-by: Mark Brown broonie@kernel.org Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/codecs/hda-dai.c | 6 ++++-- sound/soc/codecs/hda.c | 2 ++ 2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/hda-dai.c b/sound/soc/codecs/hda-dai.c index 5371ff086261..20b070c66c6f 100644 --- a/sound/soc/codecs/hda-dai.c +++ b/sound/soc/codecs/hda-dai.c @@ -76,13 +76,15 @@ static int hda_codec_dai_prepare(struct snd_pcm_substream *substream, struct snd struct hdac_stream *stream; struct hda_codec *codec; unsigned int format; + unsigned int bps; int ret;
codec = dev_to_hda_codec(dai->dev); stream = substream->runtime->private_data; stream_info = snd_soc_dai_get_dma_data(dai, substream); - format = snd_hdac_calc_stream_format(runtime->rate, runtime->channels, runtime->format, - runtime->sample_bits, 0); + + bps = snd_hdac_stream_format_bps(runtime->format, runtime->subformat, stream_info->maxbps); + format = snd_hdac_stream_format(runtime->channels, bps, runtime->rate);
ret = snd_hda_codec_prepare(codec, stream_info, stream->stream_tag, format, substream); if (ret < 0) { diff --git a/sound/soc/codecs/hda.c b/sound/soc/codecs/hda.c index d57b043d6bfe..d2117e36ddd1 100644 --- a/sound/soc/codecs/hda.c +++ b/sound/soc/codecs/hda.c @@ -52,6 +52,7 @@ static int hda_codec_create_dais(struct hda_codec *codec, int pcm_count, stream->channels_max = pcm->stream[dir].channels_max; stream->rates = pcm->stream[dir].rates; stream->formats = pcm->stream[dir].formats; + stream->subformats = pcm->stream[dir].subformats; stream->sig_bits = pcm->stream[dir].maxbps;
capture_dais: @@ -71,6 +72,7 @@ static int hda_codec_create_dais(struct hda_codec *codec, int pcm_count, stream->channels_max = pcm->stream[dir].channels_max; stream->rates = pcm->stream[dir].rates; stream->formats = pcm->stream[dir].formats; + stream->subformats = pcm->stream[dir].subformats; stream->sig_bits = pcm->stream[dir].maxbps; }
To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism.
Acked-by: Mark Brown broonie@kernel.org Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/codecs/hdac_hda.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/sound/soc/codecs/hdac_hda.c b/sound/soc/codecs/hdac_hda.c index be66853afbe2..7e308dac07d4 100644 --- a/sound/soc/codecs/hdac_hda.c +++ b/sound/soc/codecs/hdac_hda.c @@ -207,18 +207,16 @@ static int hdac_hda_dai_hw_params(struct snd_pcm_substream *substream, struct hdac_hda_priv *hda_pvt; unsigned int format_val; unsigned int maxbps; + unsigned int bps;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) maxbps = dai->driver->playback.sig_bits; else maxbps = dai->driver->capture.sig_bits; + bps = snd_hdac_stream_format_bps(params_format(params), SNDRV_PCM_SUBFORMAT_STD, maxbps);
hda_pvt = snd_soc_component_get_drvdata(component); - format_val = snd_hdac_calc_stream_format(params_rate(params), - params_channels(params), - params_format(params), - maxbps, - 0); + format_val = snd_hdac_stream_format(params_channels(params), bps, params_rate(params)); if (!format_val) { dev_err(dai->dev, "invalid format_val, rate=%d, ch=%d, format=%d, maxbps=%d\n",
To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism.
Acked-by: Mark Brown broonie@kernel.org Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/codecs/hdac_hdmi.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 3e4f632d8665..824fb4862f22 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -468,13 +468,14 @@ static int hdac_hdmi_set_hw_params(struct snd_pcm_substream *substream, struct hdac_hdmi_priv *hdmi = snd_soc_dai_get_drvdata(dai); struct hdac_hdmi_dai_port_map *dai_map; struct hdac_hdmi_pcm *pcm; + unsigned int bps; int format;
dai_map = &hdmi->dai_map[dai->id];
- format = snd_hdac_calc_stream_format(params_rate(hparams), - params_channels(hparams), params_format(hparams), - dai->driver->playback.sig_bits, 0); + bps = snd_hdac_stream_format_bps(params_format(hparams), SNDRV_PCM_SUBFORMAT_STD, + dai->driver->playback.sig_bits); + format = snd_hdac_stream_format(params_channels(hparams), bps, params_rate(hparams));
pcm = hdac_hdmi_get_pcm_from_cvt(hdmi, dai_map->cvt); if (!pcm)
To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism.
Acked-by: Mark Brown broonie@kernel.org Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/skylake/skl-pcm.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index ac3dc8c63c26..4613a1335819 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -128,6 +128,7 @@ int skl_pcm_host_dma_prepare(struct device *dev, struct skl_pipe_params *params) unsigned int format_val; struct hdac_stream *hstream; struct hdac_ext_stream *stream; + unsigned int bps; int err;
hstream = snd_hdac_get_stream(bus, params->stream, @@ -138,8 +139,8 @@ int skl_pcm_host_dma_prepare(struct device *dev, struct skl_pipe_params *params) stream = stream_to_hdac_ext_stream(hstream); snd_hdac_ext_stream_decouple(bus, stream, true);
- format_val = snd_hdac_calc_stream_format(params->s_freq, - params->ch, params->format, params->host_bps, 0); + bps = snd_hdac_stream_format_bps(params->format, SNDRV_PCM_SUBFORMAT_STD, params->host_bps); + format_val = snd_hdac_stream_format(params->ch, bps, params->s_freq);
dev_dbg(dev, "format_val=%d, rate=%d, ch=%d, format=%d\n", format_val, params->s_freq, params->ch, params->format); @@ -177,6 +178,7 @@ int skl_pcm_link_dma_prepare(struct device *dev, struct skl_pipe_params *params) struct hdac_ext_stream *stream; struct hdac_ext_link *link; unsigned char stream_tag; + unsigned int bps;
hstream = snd_hdac_get_stream(bus, params->stream, params->link_dma_id + 1); @@ -185,8 +187,9 @@ int skl_pcm_link_dma_prepare(struct device *dev, struct skl_pipe_params *params)
stream = stream_to_hdac_ext_stream(hstream); snd_hdac_ext_stream_decouple(bus, stream, true); - format_val = snd_hdac_calc_stream_format(params->s_freq, params->ch, - params->format, params->link_bps, 0); + + bps = snd_hdac_stream_format_bps(params->format, SNDRV_PCM_SUBFORMAT_STD, params->link_bps); + format_val = snd_hdac_stream_format(params->ch, bps, params->s_freq);
dev_dbg(dev, "format_val=%d, rate=%d, ch=%d, format=%d\n", format_val, params->s_freq, params->ch, params->format);
To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism.
Acked-by: Mark Brown broonie@kernel.org Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/sof/intel/hda-dai-ops.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/sound/soc/sof/intel/hda-dai-ops.c b/sound/soc/sof/intel/hda-dai-ops.c index 494ced2b746e..fb5989888ff4 100644 --- a/sound/soc/sof/intel/hda-dai-ops.c +++ b/sound/soc/sof/intel/hda-dai-ops.c @@ -208,14 +208,15 @@ static unsigned int hda_calc_stream_format(struct snd_sof_dev *sdev, struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0); unsigned int link_bps; unsigned int format_val; + unsigned int bps;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) link_bps = codec_dai->driver->playback.sig_bits; else link_bps = codec_dai->driver->capture.sig_bits; + bps = snd_hdac_stream_format_bps(params_format(params), SNDRV_PCM_SUBFORMAT_STD, link_bps);
- format_val = snd_hdac_calc_stream_format(params_rate(params), params_channels(params), - params_format(params), link_bps, 0); + format_val = snd_hdac_stream_format(params_channels(params), bps, params_rate(params));
dev_dbg(sdev->dev, "format_val=%#x, rate=%d, ch=%d, format=%d\n", format_val, params_rate(params), params_channels(params), params_format(params));
To provide option for selecting different bit-per-sample than just the maximum one, use the new format calculation mechanism.
Acked-by: Mark Brown broonie@kernel.org Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/loader.c | 4 ++-- sound/soc/intel/avs/path.c | 2 +- sound/soc/intel/avs/pcm.c | 19 ++++++++++++++----- sound/soc/intel/avs/probes.c | 3 +-- 4 files changed, 18 insertions(+), 10 deletions(-)
diff --git a/sound/soc/intel/avs/loader.c b/sound/soc/intel/avs/loader.c index 56bb0a59249d..2e3dd3d86b16 100644 --- a/sound/soc/intel/avs/loader.c +++ b/sound/soc/intel/avs/loader.c @@ -371,7 +371,7 @@ int avs_hda_load_basefw(struct avs_dev *adev, struct firmware *fw) hstream = hdac_stream(estream);
/* code loading performed with default format */ - sdfmt = snd_hdac_calc_stream_format(48000, 1, SNDRV_PCM_FORMAT_S32_LE, 32, 0); + sdfmt = snd_hdac_stream_format(1, 32, 48000); ret = snd_hdac_dsp_prepare(hstream, sdfmt, fw->size, &dmab); if (ret < 0) goto release_stream; @@ -438,7 +438,7 @@ int avs_hda_load_library(struct avs_dev *adev, struct firmware *lib, u32 id) stream = hdac_stream(estream);
/* code loading performed with default format */ - sdfmt = snd_hdac_calc_stream_format(48000, 1, SNDRV_PCM_FORMAT_S32_LE, 32, 0); + sdfmt = snd_hdac_stream_format(1, 32, 48000); ret = snd_hdac_dsp_prepare(stream, sdfmt, lib->size, &dmab); if (ret < 0) goto release_stream; diff --git a/sound/soc/intel/avs/path.c b/sound/soc/intel/avs/path.c index adbe23a47847..be9e455ee0a2 100644 --- a/sound/soc/intel/avs/path.c +++ b/sound/soc/intel/avs/path.c @@ -87,7 +87,7 @@ static bool avs_test_hw_params(struct snd_pcm_hw_params *params, return (params_rate(params) == fmt->sampling_freq && params_channels(params) == fmt->num_channels && params_physical_width(params) == fmt->bit_depth && - params_width(params) == fmt->valid_bit_depth); + snd_pcm_hw_params_bps(params) == fmt->valid_bit_depth); }
static struct avs_tplg_path * diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c index 8565a530706d..2a8aa4e6ce67 100644 --- a/sound/soc/intel/avs/pcm.c +++ b/sound/soc/intel/avs/pcm.c @@ -335,20 +335,25 @@ static int avs_dai_hda_be_prepare(struct snd_pcm_substream *substream, struct sn { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); struct snd_pcm_runtime *runtime = substream->runtime; - struct hdac_ext_stream *link_stream = runtime->private_data; + struct snd_soc_pcm_stream *stream_info; + struct hdac_ext_stream *link_stream; struct hdac_ext_link *link; struct hda_codec *codec; struct hdac_bus *bus; unsigned int format_val; + unsigned int bps; int ret;
+ link_stream = runtime->private_data; if (link_stream->link_prepared) return 0;
codec = dev_to_hda_codec(asoc_rtd_to_codec(rtd, 0)->dev); bus = &codec->bus->core; - format_val = snd_hdac_calc_stream_format(runtime->rate, runtime->channels, runtime->format, - runtime->sample_bits, 0); + stream_info = snd_soc_dai_get_pcm_stream(dai, substream->stream); + bps = snd_hdac_stream_format_bps(runtime->format, runtime->subformat, + stream_info->sig_bits); + format_val = snd_hdac_stream_format(runtime->channels, bps, runtime->rate);
snd_hdac_ext_stream_decouple(bus, link_stream, true); snd_hdac_ext_stream_reset(link_stream); @@ -601,11 +606,13 @@ static int avs_dai_fe_hw_free(struct snd_pcm_substream *substream, struct snd_so static int avs_dai_fe_prepare(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct snd_pcm_runtime *runtime = substream->runtime; + struct snd_soc_pcm_stream *stream_info; struct avs_dma_data *data; struct avs_dev *adev = to_avs_dev(dai->dev); struct hdac_ext_stream *host_stream; struct hdac_bus *bus; unsigned int format_val; + unsigned int bps; int ret;
data = snd_soc_dai_get_dma_data(dai, substream); @@ -618,8 +625,10 @@ static int avs_dai_fe_prepare(struct snd_pcm_substream *substream, struct snd_so snd_hdac_ext_stream_decouple(bus, data->host_stream, true); snd_hdac_stream_reset(hdac_stream(host_stream));
- format_val = snd_hdac_calc_stream_format(runtime->rate, runtime->channels, runtime->format, - runtime->sample_bits, 0); + stream_info = snd_soc_dai_get_pcm_stream(dai, substream->stream); + bps = snd_hdac_stream_format_bps(runtime->format, runtime->subformat, + stream_info->sig_bits); + format_val = snd_hdac_stream_format(runtime->channels, bps, runtime->rate);
ret = snd_hdac_stream_set_params(hdac_stream(host_stream), format_val); if (ret < 0) diff --git a/sound/soc/intel/avs/probes.c b/sound/soc/intel/avs/probes.c index 4cab8c6c4576..7d0aab3f2ada 100644 --- a/sound/soc/intel/avs/probes.c +++ b/sound/soc/intel/avs/probes.c @@ -140,8 +140,7 @@ static int avs_probe_compr_set_params(struct snd_compr_stream *cstream, bps = snd_pcm_format_physical_width(format); if (bps < 0) return bps; - format_val = snd_hdac_calc_stream_format(params->codec.sample_rate, params->codec.ch_out, - format, bps, 0); + format_val = snd_hdac_stream_format(params->codec.ch_out, bps, params->codec.sample_rate); ret = snd_hdac_stream_set_params(hdac_stream(host_stream), format_val); if (ret < 0) return ret;
There are no users of the function.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/hdaudio.h | 5 ---- sound/hda/hdac_device.c | 61 ----------------------------------------- 2 files changed, 66 deletions(-)
diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h index 589812c6f6ce..b6e4529279f9 100644 --- a/include/sound/hdaudio.h +++ b/include/sound/hdaudio.h @@ -145,11 +145,6 @@ unsigned int snd_hdac_stream_format_bps(snd_pcm_format_t format, snd_pcm_subform 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, - unsigned int maxbps, - unsigned short spdif_ctls); int snd_hdac_query_supported_pcm(struct hdac_device *codec, hda_nid_t nid, u32 *ratesp, u64 *formatsp, const struct snd_pcm_subformat **subformatsp, diff --git a/sound/hda/hdac_device.c b/sound/hda/hdac_device.c index ff5f45b95be4..c1243315338e 100644 --- a/sound/hda/hdac_device.c +++ b/sound/hda/hdac_device.c @@ -849,67 +849,6 @@ unsigned int snd_hdac_spdif_stream_format(unsigned int channels, unsigned int bp } EXPORT_SYMBOL_GPL(snd_hdac_spdif_stream_format);
-/** - * snd_hdac_calc_stream_format - calculate the format bitset - * @rate: the sample rate - * @channels: the number of channels - * @format: the PCM format (SNDRV_PCM_FORMAT_XXX) - * @maxbps: the max. bps - * @spdif_ctls: HD-audio SPDIF status bits (0 if irrelevant) - * - * Calculate the format bitset from the given rate, channels and th PCM format. - * - * Return zero if invalid. - */ -unsigned int snd_hdac_calc_stream_format(unsigned int rate, - unsigned int channels, - snd_pcm_format_t format, - unsigned int maxbps, - unsigned short spdif_ctls) -{ - int i; - unsigned int val = 0; - - 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 (snd_pcm_format_width(format)) { - case 8: - val |= AC_FMT_BITS_8; - break; - case 16: - val |= AC_FMT_BITS_16; - break; - case 20: - case 24: - case 32: - if (maxbps >= 32 || format == SNDRV_PCM_FORMAT_FLOAT_LE) - val |= AC_FMT_BITS_32; - else if (maxbps >= 24) - val |= AC_FMT_BITS_24; - else - val |= AC_FMT_BITS_20; - break; - default: - return 0; - } - - if (spdif_ctls & AC_DIG1_NONAUDIO) - val |= AC_FMT_TYPE_NON_PCM; - - return val; -} -EXPORT_SYMBOL_GPL(snd_hdac_calc_stream_format); - static unsigned int query_pcm_param(struct hdac_device *codec, hda_nid_t nid) { unsigned int val = 0;
Hi Cezary,
kernel test robot noticed the following build errors:
[auto build test ERROR on 564ee9ac788b680e4ec4a6cb3a4a953dc61d5da8]
url: https://github.com/intel-lab-lkp/linux/commits/Cezary-Rojewski/ALSA-pcm-Intr... base: 564ee9ac788b680e4ec4a6cb3a4a953dc61d5da8 patch link: https://lore.kernel.org/r/20230918133940.3676091-16-cezary.rojewski%40intel.... patch subject: [PATCH v2 15/17] ALSA: hda: Drop snd_hdac_calc_stream_format() config: arc-allmodconfig (https://download.01.org/0day-ci/archive/20230918/202309182301.K9lVpMbl-lkp@i...) compiler: arceb-elf-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230918/202309182301.K9lVpMbl-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202309182301.K9lVpMbl-lkp@intel.com/
All errors (new ones prefixed by >>):
sound/soc/sof/intel/hda-dai-ops.c: In function 'generic_calc_stream_format':
sound/soc/sof/intel/hda-dai-ops.c:243:22: error: implicit declaration of function 'snd_hdac_calc_stream_format'; did you mean 'hda_calc_stream_format'? [-Werror=implicit-function-declaration]
243 | format_val = snd_hdac_calc_stream_format(params_rate(params), params_channels(params), | ^~~~~~~~~~~~~~~~~~~~~~~~~~~ | hda_calc_stream_format cc1: some warnings being treated as errors
vim +243 sound/soc/sof/intel/hda-dai-ops.c
d1bf58474d17a7 Pierre-Louis Bossart 2023-06-02 236 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 237 static unsigned int generic_calc_stream_format(struct snd_sof_dev *sdev, 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 238 struct snd_pcm_substream *substream, 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 239 struct snd_pcm_hw_params *params) 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 240 { 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 241 unsigned int format_val; 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 242 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 @243 format_val = snd_hdac_calc_stream_format(params_rate(params), params_channels(params), 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 244 params_format(params), 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 245 params_physical_width(params), 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 246 0); 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 247 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 248 dev_dbg(sdev->dev, "format_val=%#x, rate=%d, ch=%d, format=%d\n", format_val, 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 249 params_rate(params), params_channels(params), params_format(params)); 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 250 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 251 return format_val; 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 252 } 12547730e5b7c4 Pierre-Louis Bossart 2023-08-07 253
Eliminate all occurrences of S24_LE within the HDAudio related pcm code, both HOST and LINK side. Replace those with MSBITS subformats to allow for granular selection when S32_LE is the format of choice.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/pcm.c | 16 +++++++++++++--- sound/soc/intel/avs/topology.c | 19 ++++++++++++++++++- 2 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c index 2a8aa4e6ce67..08855d4e0a37 100644 --- a/sound/soc/intel/avs/pcm.c +++ b/sound/soc/intel/avs/pcm.c @@ -1069,6 +1069,16 @@ static int avs_component_resume(struct snd_soc_component *component) return avs_component_resume_prepare(component, false); }
+static struct snd_pcm_subformat avs_pcm_subformats[] = { + { + .format = SNDRV_PCM_FORMAT_S32_LE, + .mask = SNDRV_PCM_SUBFMTBIT_MSBITS_20 | + SNDRV_PCM_SUBFMTBIT_MSBITS_24 | + SNDRV_PCM_SUBFMTBIT_MSBITS_MAX, + }, + {} +}; + static const struct snd_pcm_hardware avs_pcm_hardware = { .info = SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID | @@ -1077,8 +1087,8 @@ static const struct snd_pcm_hardware avs_pcm_hardware = { SNDRV_PCM_INFO_RESUME | SNDRV_PCM_INFO_NO_PERIOD_WAKEUP, .formats = SNDRV_PCM_FMTBIT_S16_LE | - SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE, + .subformats = avs_pcm_subformats, .buffer_bytes_max = AZX_MAX_BUF_SIZE, .period_bytes_min = 128, .period_bytes_max = AZX_MAX_BUF_SIZE / 2, @@ -1308,16 +1318,16 @@ static const struct snd_soc_dai_driver hda_cpu_dai = { .channels_max = 8, .rates = SNDRV_PCM_RATE_8000_192000, .formats = SNDRV_PCM_FMTBIT_S16_LE | - SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE, + .subformats = avs_pcm_subformats, }, .capture = { .channels_min = 1, .channels_max = 8, .rates = SNDRV_PCM_RATE_8000_192000, .formats = SNDRV_PCM_FMTBIT_S16_LE | - SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S32_LE, + .subformats = avs_pcm_subformats, }, };
diff --git a/sound/soc/intel/avs/topology.c b/sound/soc/intel/avs/topology.c index 45d0eb2a8e71..f2545dcbf86e 100644 --- a/sound/soc/intel/avs/topology.c +++ b/sound/soc/intel/avs/topology.c @@ -1456,8 +1456,22 @@ static int avs_dai_load(struct snd_soc_component *comp, int index, struct snd_soc_dai_driver *dai_drv, struct snd_soc_tplg_pcm *pcm, struct snd_soc_dai *dai) { - if (pcm) + static struct snd_pcm_subformat fe_subformats[] = { + { + .format = SNDRV_PCM_FORMAT_S32_LE, + .mask = SNDRV_PCM_SUBFMTBIT_MSBITS_20 | + SNDRV_PCM_SUBFMTBIT_MSBITS_24 | + SNDRV_PCM_SUBFMTBIT_MSBITS_MAX, + }, + {} + }; + + if (pcm) { dai_drv->ops = &avs_dai_fe_ops; + dai_drv->capture.subformats = fe_subformats; + dai_drv->playback.subformats = fe_subformats; + } + return 0; }
@@ -1476,6 +1490,9 @@ static int avs_link_load(struct snd_soc_component *comp, int index, struct snd_s /* Open LINK (BE) pipes last and close them first to prevent xruns. */ link->trigger[0] = SND_SOC_DPCM_TRIGGER_PRE; link->trigger[1] = SND_SOC_DPCM_TRIGGER_PRE; + } else { + /* Do not ignore codec capabilities. */ + link->dpcm_merged_format = 1; }
return 0;
To not expose more than in fact is supported by the codec, update CPU DAI initialization procedure to rely on codec capabilities instead of hardcoding them. This includes subformat which is currently ignored.
As capabilities for HDMI streams are initialized on PCM open, leave it as is for now.
Acked-by: Mark Brown broonie@kernel.org Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/avs/pcm.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)
diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c index 08855d4e0a37..cca1a5b7764a 100644 --- a/sound/soc/intel/avs/pcm.c +++ b/sound/soc/intel/avs/pcm.c @@ -17,6 +17,7 @@ #include "avs.h" #include "path.h" #include "topology.h" +#include "../../codecs/hda.h"
struct avs_dma_data { struct avs_tplg_path_template *template; @@ -1404,6 +1405,15 @@ static int avs_component_hda_probe(struct snd_soc_component *component) ret = -ENOMEM; goto exit; } + + if (!hda_codec_is_display(codec)) { + dais[i].playback.formats = pcm->stream[0].formats; + dais[i].playback.subformats = pcm->stream[0].subformats; + dais[i].playback.rates = pcm->stream[0].rates; + dais[i].playback.channels_min = pcm->stream[0].channels_min; + dais[i].playback.channels_max = pcm->stream[0].channels_max; + dais[i].playback.sig_bits = pcm->stream[0].maxbps; + } }
if (pcm->stream[1].substreams) { @@ -1414,6 +1424,15 @@ static int avs_component_hda_probe(struct snd_soc_component *component) ret = -ENOMEM; goto exit; } + + if (!hda_codec_is_display(codec)) { + dais[i].capture.formats = pcm->stream[1].formats; + dais[i].capture.subformats = pcm->stream[1].subformats; + dais[i].capture.rates = pcm->stream[1].rates; + dais[i].capture.channels_min = pcm->stream[1].channels_min; + dais[i].capture.channels_max = pcm->stream[1].channels_max; + dais[i].capture.sig_bits = pcm->stream[1].maxbps; + } }
dai = snd_soc_register_dai(component, &dais[i], false);
participants (4)
-
Cezary Rojewski -
Jaroslav Kysela -
kernel test robot -
Mark Brown