diff mbox series

[v2,02/17] ALSA: pcm: Honor subformat when configuring substream

Message ID 20230918133940.3676091-3-cezary.rojewski@intel.com
State New
Headers show
Series None | expand

Commit Message

Cezary Rojewski Sept. 18, 2023, 1:39 p.m. UTC 
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(-)

Comments

Jaroslav Kysela Sept. 21, 2023, 6:41 a.m. UTC | #1 
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/
Cezary Rojewski Sept. 21, 2023, 8:59 a.m. UTC | #2 
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/
>
kernel test robot Sept. 23, 2023, 10:55 a.m. UTC | #3 
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-Introduce-MSBITS-subformat-interface/20230918-214758
base:   564ee9ac788b680e4ec4a6cb3a4a953dc61d5da8
patch link:    https://lore.kernel.org/r/20230918133940.3676091-3-cezary.rojewski%40intel.com
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@intel.com/config)
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@intel.com/reproduce)

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
diff mbox series

Patch

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;