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

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

On 2023-09-12 6:30 PM, Jaroslav Kysela wrote:
> On 11. 09. 23 17:45, Cezary Rojewski wrote:
>> On 2023-09-11 10:43 AM, Cezary Rojewski wrote:

...

>>>
>>> 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.
> 
> This is not a correct implementation. Many things are missing including 
> the proper subformats filter. I sent my own version here for discussion:
> 
> https://lore.kernel.org/alsa-devel/20230912162526.7138-1-perex@perex.cz/

I do appreciate the input but I expected that, through guidance, this 
patch gets updated. Sending a separate patch, not connected to this 
patchset - not even a single reference within the commit message body - 
is not nice.

I'd rather send v2 with your patch incorporated as a part of the 
patchset. The subformat improvement alone, without showcasing the users 
is confusing to the reviewers.


Czarek