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

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

On 8/23/2023 3:42 PM, Jaroslav Kysela wrote:
> On 23. 08. 23 13:08, Takashi Iwai wrote:
>> On Wed, 23 Aug 2023 12:47:33 +0200,
>> Jaroslav Kysela wrote:
>>>
>>> On 23. 08. 23 12:20, Amadeusz Sławiński wrote:
>>>> 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.
>>>
>>> This example is nice for S32_LE but not S16_LE. With the max/full bit,
>>> we can already cover also S16_LE (so that the application can ask for
>>> the maximal msbits which fits to the physical format for S16_LE). It
>>> would be also a preparation for future, when we need to deal with more
>>> msbits combos (like 14bit or 15bit samples stored in the bigger
>>> physical words) etc.
>>>
>>> So application can set those parameters for in your case:
>>>
>>> S16_LE + STD         (maximum bits handled by driver - 16 in this case)
>>> S16_LE + MSBITS_MAX  (maximum physical bits for the format - 16)
>>> S32_LE + STD         (maximum bits handled by driver - 24 in this case)
>>> S32_LE + MSBITS_MAX  (maximum physical bits for the format - 32)
>>> S32_LE + MSBITS_24
>>> S32_LE + MSBITS_20
>>>
>>> Dtto for other format like S8, S24 etc. Another way is to define
>>> MSBITS_8, MSBITS_16 etc. But I'd prefer to save subformat bits. The
>>> MSBITS_MAX would cover almost all cases for now.
>>
>> It becomes a bit tricky if we have a device that has 24bit bps on
>> 32bit format and 16bit bps / 16bit format.  Both formats and
>> subformats are bitmaps in hw_params, and initially formats bitmap is
>> 16|24 and subformats bitmap is MAX|24.
>>
>> Now, suppose that app determines to use 16bit.  Then we need to we
>> need to update subformats bitmap to MAX by dropping 24.  OTOH, if app
>> chooses 32bit format, subformats will be 24 by dropping MAX, as we
>> don't support 32 bps.  And, it's not so trivial to achieve this
>> commonly only with the single subformats bitmap of snd_pcm_hardware,
>> as the meaning of MAX depends on the chosen format.
> 
> It's easy to implement using a code which will go through all set format 
> bits and do bitwise OR for all corresponding subformat bits. We can use 
> a callback which the driver may override.
> 
> Something like (for the above HDA example): - SUBFMTBIT_STD is required 
> so it would be handled at the upper level):
> 
> snd_pcm_subformat_t (*get_subformat)(struct snd_pcm_substream 
> *substream, snd_pcm_format_t format)
> {
>          switch (format) {
>      case SNDRV_PCM_FMTBIT_S16_LE:
>          return SNDRV_PCM_SUBFMTBIT_MSBITS_MAX;
>          case SNDRV_PCM_FMTBIT_S32_LE:
>                  return SNDRV_PCM_SUBFMTBIT_MSBITS_24 |
>                         SNDRV_PCM_SUBFMTBIT_MSBITS_20;
>      default:
>          return 0;
>      }
> }
> 
> SUBFMTBIT_STD is required so it would be handled at the upper level.
> 
>> Meanwhile, if the subformats bitmap is with explicit bit flags,
>> i.e. 24|16, we can reduce the bitmap easily depending on the format.
> 
> I don't think that the current proposed code does any reduction. It will 
> return MSBITS_24 when only S16_LE format is selected, too. The refining 
> mechanism is ignored. We need to handle subformat bits differently than 
> format bits because the dependency.
> 
>                      Jaroslav
> 

Problem with MSBITS_MAX is that if kernel reports something like this:

FORMAT: S16_LE S32_LE
SUBFORMAT: STD MSBITS_20 MSBITS_MAX

to userspace, is that userspace can't really tell if you should only 
apply it to S16_LE or to S32_LE, or both. On the other hand if at some 
point someone adds S64_LE format, something like:

FORMAT: S16_LE S32_LE S64_LE
SUBFORMAT: STD MSBITS_20 MSBITS_32

will be also problematic as, you can't be sure is MSBITS subformats are 
meant for S32_LE or S64_LE format.

The easiest way would of course be to add specific formats like:
S20_MSB_LE
U20_MSB_LE
S20_MSB_BE
U20_MSB_BE
S24_MSB_LE
U24_MSB_LE
S24_MSB_BE
U24_MSB_BE
and one would know used formats exactly..., although I'm sure it is 
problematic in some ways, considering HDA currently exposes S32_LE and 
just sets msbits quietly and reports it after the fact to userspace.

Alternatively maybe, SUBFORMAT_STD could just mean LSB aligned where 
container is bigger than actual amount of bits and we can add 
SUBFORMAT_MSB, which can be then used with formats like S20_LE and 
S24_LE, to change bit justification, as it is unlikely to differ between 
formats?

Amadeusz