On 2023-09-12 6:25 PM, Jaroslav Kysela wrote:
Improve granularity of format selection for linear formats by adding constants representing MAX, 20, 24 most significant bits.
The MAX means the maximum number of significant bits which can the physical format hold. For 32-bit formats, MAX is related to 32 bits. For 8-bit formats, MAX is related to 8 bits etc.
The drivers may use snd_pcm_hw_constraint_subformats with a simple format -> subformats table.
The code looks good overall. I have few comments and nitpicks regarding readability - comes from person who recently was digging through hw_rule and related code and found themselves lost. Examples such as this patch are good how-to references in hw_rule world.
-
The message lacks reference to the original patchset. I'd suggest to have it here. Either that or incorporate it directly into the patchset. And I must admit I'm a bit surprised by the lack of few CCs when compared to the original subject.
Cc: Cezary Rojewski cezary.rojewski@intel.com Signed-off-by: Jaroslav Kysela perex@perex.cz
include/sound/pcm.h | 17 +++++++++ include/uapi/sound/asound.h | 7 ++-- sound/core/pcm_lib.c | 59 +++++++++++++++++++++++++++++++ sound/core/pcm_native.c | 18 +++++++--- tools/include/uapi/sound/asound.h | 7 ++-- 5 files changed, 100 insertions(+), 8 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 2a815373dac1..59ad45b42e03 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -217,6 +217,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;
@@ -290,6 +296,13 @@ struct snd_pcm_hw_constraint_ranges { unsigned int mask; };
+#define SNDRV_PCM_FORMAT_CONSTRAINT_END (~0)
+struct snd_pcm_hw_constraint_subformat {
- snd_pcm_format_t format; /* SNDRV_PCM_FORMAT_* */
- u32 subformats; /* SNDRV_PCM_SUBFMTBIT_* */
From what I know, we are dealing with u64 masks here. Why u32 here?
+};
- /*
- userspace-provided audio timestamp config to kernel,
- structure is for internal use only and filled with dedicated unpack routine
@@ -375,6 +388,7 @@ struct snd_pcm_runtime { unsigned int rate_num; unsigned int rate_den; unsigned int no_period_wakeup: 1;
unsigned int subformat_constraint: 1;
/* -- SW params; see struct snd_pcm_sw_params for comments -- */ int tstamp_mode;
@@ -1068,6 +1082,9 @@ int snd_pcm_hw_constraint_ratdens(struct snd_pcm_runtime *runtime, unsigned int cond, snd_pcm_hw_param_t var, const struct snd_pcm_hw_constraint_ratdens *r); +int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime,
unsigned int cond,struct snd_pcm_hw_constraint_subformat *subformats);
...
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index a11cd7d6295f..f414f8fd217b 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1404,6 +1404,65 @@ int snd_pcm_hw_constraint_ratdens(struct snd_pcm_runtime *runtime, } EXPORT_SYMBOL(snd_pcm_hw_constraint_ratdens);
+static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params,
struct snd_pcm_hw_rule *rule)+{
- const struct snd_pcm_hw_constraint_subformat *sf;
What's 'sf'? I'd suggest to be more descriptive here.
- snd_pcm_format_t k;
Internally I was utilizing 'f' as that's what macro expects in its declaration. s/k/f/
- struct snd_mask m;
- struct snd_mask *fmask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT);
- struct snd_mask *mask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_SUBFORMAT);
So, the reason I opted for 'subformat_mask' and 'format_mask' is that otherwise reader is crowded with ambiguous 'mask' and its relatives. I'd avoid shortcuts when multiple variables touch the same subject.
s/fmask/format_mask/ s/mask/subformat_mask/
- snd_mask_none(&m);
- snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_STD);
- bool found;
Suggestion is to add newline before declaration and execution blocks. Also, why not reserve-christmass-tree model? There quite a few variables here.
- pcm_for_each_format(k) {
if (!snd_mask_test(fmask, k))continue;
Similarly here. A newline would effectively separate conditional for-loop from the actual execution block.
found = false;for (sf = rule->private; sf && sf->format != SNDRV_PCM_FORMAT_CONSTRAINT_END; sf++) {if (sf->format != k)continue;found = true;m.bits[0] |= sf->subformats;break;}if (!found && snd_pcm_format_linear(k))
For my own education, why checking if format is linear is essential here? Perhaps a comment?
snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX);- }
- return snd_mask_refine(mask, &m);
+}
+/**
- snd_pcm_hw_constraint_subformats - add a hw constraint subformats rule
- @runtime: PCM runtime instance
- @cond: condition bits
- @subformats: array with struct snd_pcm_subformat elements
- @nmemd: size of array with struct snd_pcm_subformat elements
- This constraint will set relation between format and subformats.
I do not believe 'This constaint' brings any value. Reader is already aware of it. Starting from the 'Set' part brings the same value with fewer words.
- The STD and MAX subformats are handled automatically. If the driver
- does not set this constraint, only STD and MAX subformats are handled.
- Return: Zero if successful, or a negative error code on failure.
- */
+int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime,
unsigned int cond,struct snd_pcm_hw_constraint_subformat *subformats)+{
- int ret;
- ret = snd_pcm_hw_rule_add(runtime, cond, -1,
snd_pcm_hw_rule_subformats,(void*) subformats,SNDRV_PCM_HW_PARAM_SUBFORMAT,SNDRV_PCM_HW_PARAM_FORMAT, -1);- if (ret < 0)
return ret;- runtime->subformat_constraint = 1;
- return 0;
+} +EXPORT_SYMBOL(snd_pcm_hw_constraint_subformats);
- static int snd_pcm_hw_rule_msbits(struct snd_pcm_hw_params *params, struct snd_pcm_hw_rule *rule) {
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index bd9ddf412b46..69609e6aa507 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -479,6 +479,7 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream, { const struct snd_interval *i; const struct snd_mask *m;
- struct snd_mask *m_rw;
Two masks named 'm' and 'm_rw' is confusing in my opinion. The 'm_rw' is used only in subformat case so the name could be more descriptive.
int err;
if (!params->msbits) { @@ -487,6 +488,14 @@ 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_width(snd_mask_min(m)) != params->msbits) {m_rw = hw_param_mask(params, SNDRV_PCM_HW_PARAM_SUBFORMAT);snd_mask_reset(m_rw, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX);}- }
- if (!params->rate_den) { i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_RATE); if (snd_interval_single(i)) {
@@ -2634,10 +2643,11 @@ 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));- if (err < 0)
return err;
- if (!runtime->subformat_constraint) {
I'd try to avoid another special-bit in the runtime space. But I might be wrong here and it's unavoidable. Let me ask though, why cannot we do the constraint unconditionally?
err = snd_pcm_hw_constraint_subformats(runtime, 0, NULL);if (err < 0)return err;}
err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_CHANNELS, hw->channels_min, hw->channels_max);