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

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

On 2023-09-21 8:25 AM, Jaroslav Kysela wrote:
> On 18. 09. 23 15:39, Cezary Rojewski wrote:
>> Improve granularity of format selection for S32/U32 formats by adding
>> constants representing 20, 24 and MAX most significant bits.
>>
>> To make it easy for drivers to utilize those constants, introduce
>> snd_pcm_subformat_width() and snd_pcm_hw_params_bps(). While the former
>> is self-explanatory, the latter returns the bit-per-sample value based
>> on format and subformat characteristics of the provided hw_params.
>> snd_pcm_hw_copy() helps with copying a hardware parameters space as with
>> introduction of subformats the operations becomes non-trivial.
>>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> 
> ...
> 
>>   struct snd_pcm_hardware {
>>       unsigned int info;        /* SNDRV_PCM_INFO_* */
>>       u64 formats;            /* SNDRV_PCM_FMTBIT_* */
>> +    struct snd_pcm_subformat *subformats;
> 
> I don't think that it's required to add subformats to the hardware 
> template. The new constraint can handle subformat refining without the 
> template modifications (pointer to map table is stored privately to this 
> constraint).

After iterating over the feedback, modified the field to u32 and all the 
handlers to acknowledge that it is S32_LE-specific. I agree with 
statement that we can expand on the API in the future if there's demand 
for it.

> Also, I miss the constraint handling here. Without the constraint, the 
> new API is not functional and it does not make sense to split the 
> constraint code to other patch.

Ack, merging the two.

>> +int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p);
> 
> This may be probably inline function. See bellow.
> 
>> +    kfree(runtime->hw.subformats);
> 
> Do we really need to do an assumption about allocations for this field? 
> The driver may use a static structure. No problem, when this is not 
> added to runtime->hw.

This code is gone in v3 since there is no need to kfree() a u32.

>> +int snd_pcm_hw_params_bps(const struct snd_pcm_hw_params *p)
>> +{
>> +    snd_pcm_subformat_t subformat = params_subformat(p);
>> +    snd_pcm_format_t format = params_format(p);
>> +    int width;
>> +
>> +    switch (format) {
>> +    case SNDRV_PCM_FORMAT_S32_LE:
>> +    case SNDRV_PCM_FORMAT_U32_LE:
>> +    case SNDRV_PCM_FORMAT_S32_BE:
>> +    case SNDRV_PCM_FORMAT_U32_BE:
>> +        width = snd_pcm_subformat_width(subformat);
> 
> This is not a correct implementation. The width should be returned for 
> MAX subformat (== snd_pcm_format_width value). See bellow.

snd_pcm_subformat_width() returns 0 if subformat==MAX and fallthrough 
ensures snd_pcm_format_width() is returned in such case.

>> +int snd_pcm_subformat_width(snd_pcm_subformat_t subformat)
> 
> This function should probably have two arguments - format + subformat to 
> return the correct information. The MAX subformat should return 
> snd_pcm_format_width value.

My intention is to have two granular functions. One for obtaining 
subformat-width explicitly and the other for calculating bits-per-sample.

>> +int snd_pcm_hw_copy(struct snd_pcm_hardware *hw, const struct 
>> snd_pcm_hardware *from)
> 
> This function is not required, if only the constraint function handles 
> the subformat refining.

Ack, removing in v3.