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

[Takashi Iwai <tiwai@suse.de>]

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