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. Meanwhile, if the subformats bitmap is with explicit bit flags, i.e. 24|16, we can reduce the bitmap easily depending on the format. But, OTOH, having all subformat bits is cumbersome, as you mentioned. Hmm... Takashi
