Re: [RFC PATCH 01/17] ALSA: pcm: Introduce MSBITS subformat interface

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

On 2023-09-11 10:43 AM, Cezary Rojewski wrote:
> On 2023-09-11 9:35 AM, Jaroslav Kysela wrote:
>> On 08. 09. 23 16:36, Cezary Rojewski wrote:

...

>>> After reading all of this again, I'm fine with rewording MSBITS_32 to
>>> MSBITS_MAX.
>>>
>>> As I do not see any other points to address here and review of v1 has no
>>> points to address either, I'll send v2 with this single change. If I'd
>>> missed anything, let me know.
>>
>> The subformat bitmask should be also refined/updated depending on the 
>> selected format.
>>
>> https://lore.kernel.org/alsa-devel/f97bbbd5-1397-f5d3-5ccf-420ec813deac@perex.cz/
>>
>> It requires new code in pcm_lib.c and ASoC PCM core code.
> 
> Could you help me understand what new code is needed? The 
> get_subformat() example raised more questions than answers. The patchset 
> defines snd_pcm_subformat_width(), perhaps you meant that I should 
> update that function by adding paramter 'format' to its parameters list 
> and handle it accordingly?
> 
> Any guidance would be much appreciated.

What I come up with is a hw_rule for subformat that I add in 
snd_pcm_hw_constraints_init(). That piece, plus both STD and MSBITS_MAX 
ORed into hw->subformats in snd_pcm_hw_constraints_complete() make 
things spin.

static int snd_pcm_hw_rule_subformat(struct snd_pcm_hw_params *params,
				     struct snd_pcm_hw_rule *rule)
{
	struct snd_mask *subformat_mask = hw_param_mask(params, 
SNDRV_PCM_HW_PARAM_SUBFORMAT);
	struct snd_mask *format_mask = hw_param_mask(params, 
SNDRV_PCM_HW_PARAM_FORMAT);
	snd_pcm_format_t f;
	struct snd_mask m;
	int width;

	snd_mask_none(&m);
	snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_STD);
	snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_MAX);

	pcm_for_each_format(f) {
		if (!snd_mask_test_format(format_mask, f))
			continue;

		width = snd_pcm_format_width(f);
		switch (width) {
		case 32:
			snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_20);
			snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_24);
			break;
		default:
			break;
		}
	}

	return snd_mask_refine(subformat_mask, &m);
}


However, this means snd_hdac_query_supported_pcm() becomes confusing as 
you need to MSBITS_MAX regardless of what the codec supports.
After spending additional few hours on this, I'd say I preferred how 
things look with MSBITS_32 instead. STD and MSBITS_MAX existing 
simultaneously is confusing too.

Czarek