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 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. Takashi
