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

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 8/23/2023 12:00 PM, Jaroslav Kysela wrote:
> On 23. 08. 23 11:53, Takashi Iwai wrote:
>> On Wed, 23 Aug 2023 11:10:38 +0200,
>> Jaroslav Kysela wrote:
>>>
>>> On 23. 08. 23 10:11, Cezary Rojewski wrote:
>>>> On 2023-08-22 9:03 PM, Jaroslav Kysela wrote:
>>>>> On 22. 08. 23 17:38, Takashi Iwai wrote:
>>>>>> On Tue, 22 Aug 2023 17:29:47 +0200,
>>>>>> Jaroslav Kysela wrote:
>>>>>>>
>>>>>>> On 22. 08. 23 17:07, Takashi Iwai wrote:
>>>>>>>> On Tue, 22 Aug 2023 17:03:02 +0200,
>>>>>>>> Jaroslav Kysela wrote:
>>>>>>>>>
>>>>>>>>> On 11. 08. 23 18:48, Cezary Rojewski wrote:
>>>>>>>>>
>>>>>>>>>> +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32
>>>>>>>>>> _SNDRV_PCM_SUBFMTBIT(MSBITS_32)
>>>>>>>>>
>>>>>>>>> What was reason to add 32/32 format ? Subformat STD + msbits == 32
>>>>>>>>> should already handle the maximal resolution. Until we do not 
>>>>>>>>> have 64
>>>>>>>>> bit formats, it seems like an useless extension.
>>>>>>>>
>>>>>>>> My understanding is to distinguish the cases "we do fully support
>>>>>>>> 32bit" and "we don't care".  But, the end effect is same for both,
>>>>>>>> user-space would handle 32bit in both cases, so this difference 
>>>>>>>> won't
>>>>>>>> help much, indeed.
>>>>>>>
>>>>>>> I don't think that we have a "do not care" situation. The 
>>>>>>> applications
>>>>>>> currently expects to use the maximal msbits for STD subformat. The
>>>>>>> subformat should be used only to refine (downgrade) the 
>>>>>>> resolution on
>>>>>>> the driver / hw side on demand. I would just add only necessary API
>>>>>>> extensions and save one bit for now.
>>>>>>
>>>>>> Well, the current behavior (with STD) is to choose whatever 32bit
>>>>>> format the driver supports, and the driver may set a different value
>>>>>> of hw_params.msbits at hw_params.  The explicit MSBITS_32 would
>>>>>> enforce the hw_params.msbits to be 32, otherwise hw_refine would
>>>>>> fail.  So I see a potential difference.
>>>>>
>>>>> I see. But if our target is to create a complete query/set msbits API,
>>>>> we should cover all cases also for other formats.
>>>>>
>>>>> I vote to replace SUBFMTBIT_MSBITS_32 to SUBFMTBIT_MSBITS_MAX as the
>>>>> second bit (right after STD). The format hw parameter already defines
>>>>> the maximal width. We can add SUBFMTBIT_MSBITS_32 when it's really
>>>>> required. Note that MAX should be handled for all cases (not only for
>>>>> S32_LE or so).
>>>>
>>>> In my opinion STD already states "max". The word is not explicit either
>>>> - max in the eyes of whom? The driver'? Then the driver may reply: max
>>>> allowed e.g.: 24/32. And that translates to: fallback to STD.
>>>
>>> Max in the contents of the physical sample format (S32 = 32 bits, S24
>>> = 24 bits, S8 = 8 bits etc). It would mean, if the driver supports S32
>>> but only with 24-bit resolution, this bit should not be
>>> set/allowed. We can also use word full or something other. If we like
>>> to extend the API in this way (force the specific msbits with the
>>> error handling), all formats should be covered. For STD - see
>>> Takashi's reply.
>>
>> I think MAX can be problematic when the device supports multiple
>> formats, say, 16bit and 32bit.  Then it's not clear which MAX points
>> to: is 16bit max or 32bit max.
> 
> I don't take this point. The subformat depends on the format, thus if 
> one format support max, it should be set for queries.
> 
> Theoretically, this problem is in this API extension proposal too. 
> Imagine that driver/hw support S24 and S32 formats and 20-bit msbits for 
> one of them. How do you handle this? The subformat depends on format and 
> should be refined when the format is known (single choice).
> 
>> I find the subformat extension OK, as this doesn't need much change in
>> API.  OTOH, if we want to be more consistent way, we may extend
>> hw_params for a new interval, e.g. SNDRV_PCM_HW_PARAM_MSBITS, and let
>> the driver choosing it.  This will need more hw_params rules and
>> become more complex, but it allows drivers really exotic setups (like
>> 19bit PCM :)  But my gut feeling is that the subformat extension
>> should suffice.
> 
> I'm not ok with 32 == 32. We should handle this case universally or 
> discard.
> 
>                      Jaroslav
> 

The reason for MSBITS32 is that, when only MSBITS_20 and MSBITS_24 are 
defined, when userspace (in this case aplay) asks for usable formats and 
subformat it gets something like:

--------------------
ACCESS: MMAP_INTERLEAVED RW_INTERLEAVED
FORMAT: S16_LE S32_LE
SUBFORMAT: STD MSBITS_20 MSBITS_24
SAMPLE_BITS: [16 32]
FRAME_BITS: [32 64]
CHANNELS: 2
RATE: 48000

when MSBITS_32 is not defined it is not clear if HW supports 24 or 32 
bits as maximum possible value of msbits. However when MSBITS_32 is 
defined it is clear - in the above case maximum possible bps then is 24, 
because MSBITS_32 is missing in output.

STD behaves as before and takes maximum possible value - in above case 
it is 24.

Amadeusz