Re: [RFC PATCH] ALSA: pcm: Introduce MSBITS subformat API extension

[Cezary Rojewski <cezary.rojewski@intel.com>]

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);
>   int snd_pcm_hw_constraint_msbits(struct snd_pcm_runtime *runtime,
>   				 unsigned int cond,
>   				 unsigned int width,

...

> 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);