| Message ID | 20230811164853.1797547-2-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA/ASoC: hda: Address format selection limitations and ambiguity | expand |
On Fri, 11 Aug 2023 18:48:37 +0200, Cezary Rojewski wrote: > > --- a/sound/core/pcm_lib.c > +++ b/sound/core/pcm_lib.c > @@ -1706,6 +1706,36 @@ int snd_pcm_hw_param_last(struct snd_pcm_substream *pcm, > } > EXPORT_SYMBOL(snd_pcm_hw_param_last); > > +/** > + * params_bps - Get the number of bits per the sample. > + * @p: hardware parameters > + * > + * Return: The number of bits per sample based on the format, > + * subformat and msbits the specified hw params has. > + */ > +int params_bps(const struct snd_pcm_hw_params *p) It's a too generic name. Please put the suffix like snd_xxx_. There are a few params_xxx() in the current tree, but those are rather macros, and left so just for laziness to convert every piece. You don't need to follow that pattern. thanks, Takashi
On 2023-08-14 4:35 PM, Takashi Iwai wrote: > On Fri, 11 Aug 2023 18:48:37 +0200, > Cezary Rojewski wrote: >> >> --- a/sound/core/pcm_lib.c >> +++ b/sound/core/pcm_lib.c >> @@ -1706,6 +1706,36 @@ int snd_pcm_hw_param_last(struct snd_pcm_substream *pcm, >> } >> EXPORT_SYMBOL(snd_pcm_hw_param_last); >> >> +/** >> + * params_bps - Get the number of bits per the sample. >> + * @p: hardware parameters >> + * >> + * Return: The number of bits per sample based on the format, >> + * subformat and msbits the specified hw params has. >> + */ >> +int params_bps(const struct snd_pcm_hw_params *p) > > It's a too generic name. Please put the suffix like snd_xxx_. > > There are a few params_xxx() in the current tree, but those are rather > macros, and left so just for laziness to convert every piece. You > don't need to follow that pattern. Up for that! Followed the existing pattern, no magic there. Guess it is going to end up as: snd_pcm_hw_params_bps().
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.
Jaroslav
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. thanks, Takashi
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. Jaroslav
On 2023-08-22 5:07 PM, 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. Bull's eye. We do want to be able to distinguish between "default" and "must be 32/32" cases. In fact we do have a test where each iteration takes a different --subformat option. On some configurations, iteration with --subformat=MSBITS_32 is expected to fail and PCM device remains closed - negative test case. If given configuration supports up to 24/32, then 32/32 shall not be allowed. Option --subformat=STD is expected to select the default max e.g.: 24/32. Czarek
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. Takashi
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). Jaroslav
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. Kind regards, Czarek
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. Jaroslav
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
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
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
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. Jaroslav
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
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
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
Hi, On Wed, 23 Aug 2023, Amadeusz Sławiński wrote: > 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. Not just HDA but this is basicly all drivers. And this is ok in most cases. I mean with the MSB variants, applications can produce/consume data with full container bit width (like S32_LE) and for many usages, this will just work (the LSB data will be zero, or non-zero and ignored). Reporting the effective msbit is really important though, as apps _should_ check the field and dither when needed (although mileage varies, if you look up common ALSA apps, few bother to check msbit info). Anyways, this information definitely needs to be exposed and made available to apps. But right, this is a bit orthogonal to the ability to _select_ a specific variant. That is currently not possible without your patchset. Br, Kai
On 23. 08. 23 18:29, Amadeusz Sławiński wrote: > 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: Unfortunately, you've not got the point that the subformat contents depends on the selected format. So the subformat mask is for ALL formats selected in the configuration space. The only valid contents for one format is when application or kernel reduces the format to single one. And applications can do: 1) set format to S32_LE 2) call refine 3) get subformat bits for single S32_LE format from the refined cfg space In this case, queries and specific msbits selection will work. It's the standard refine mechanism which works also for all other fields from the parameter configuration space etc. If you look to all other fields from the parameter configuration space, you cannot predict the exact parameters (buffer size, period size, channels) until you do more refining to set all parameters to exact values (single value). In other words, the above example: FORMAT: S16_LE S32_LE SUBFORMAT: STD MSBITS_20 MSBITS_MAX .. means - at least one format supports maximal msbits for the given format. Jaroslav
On 2023-08-24 9:31 AM, Jaroslav Kysela wrote: > On 23. 08. 23 18:29, Amadeusz Sławiński wrote: ... >> 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: > > Unfortunately, you've not got the point that the subformat contents > depends on the selected format. So the subformat mask is for ALL formats > selected in the configuration space. The only valid contents for one > format is when application or kernel reduces the format to single one. > And applications can do: > > 1) set format to S32_LE > 2) call refine > 3) get subformat bits for single S32_LE format from the refined cfg space > > In this case, queries and specific msbits selection will work. It's the > standard refine mechanism which works also for all other fields from the > parameter configuration space etc. If you look to all other fields from > the parameter configuration space, you cannot predict the exact > parameters (buffer size, period size, channels) until you do more > refining to set all parameters to exact values (single value). > > In other words, the above example: > > FORMAT: S16_LE S32_LE > SUBFORMAT: STD MSBITS_20 MSBITS_MAX > > .. means - at least one format supports maximal msbits for the given > format. After reading all of this again, I'm fine with rewording MSBITS_32 to MSBITS_MAX. As I do not see any other points to address here and review of v1 has no points to address either, I'll send v2 with this single change. If I'd missed anything, let me know. Kind regards, Czarek
On 08. 09. 23 16:36, Cezary Rojewski wrote: > On 2023-08-24 9:31 AM, Jaroslav Kysela wrote: >> On 23. 08. 23 18:29, Amadeusz Sławiński wrote: > > ... > >>> 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: >> >> Unfortunately, you've not got the point that the subformat contents >> depends on the selected format. So the subformat mask is for ALL formats >> selected in the configuration space. The only valid contents for one >> format is when application or kernel reduces the format to single one. >> And applications can do: >> >> 1) set format to S32_LE >> 2) call refine >> 3) get subformat bits for single S32_LE format from the refined cfg space >> >> In this case, queries and specific msbits selection will work. It's the >> standard refine mechanism which works also for all other fields from the >> parameter configuration space etc. If you look to all other fields from >> the parameter configuration space, you cannot predict the exact >> parameters (buffer size, period size, channels) until you do more >> refining to set all parameters to exact values (single value). >> >> In other words, the above example: >> >> FORMAT: S16_LE S32_LE >> SUBFORMAT: STD MSBITS_20 MSBITS_MAX >> >> .. means - at least one format supports maximal msbits for the given >> format. > > After reading all of this again, I'm fine with rewording MSBITS_32 to > MSBITS_MAX. > > As I do not see any other points to address here and review of v1 has no > points to address either, I'll send v2 with this single change. If I'd > missed anything, let me know. The subformat bitmask should be also refined/updated depending on the selected format. https://lore.kernel.org/alsa-devel/f97bbbd5-1397-f5d3-5ccf-420ec813deac@perex.cz/ It requires new code in pcm_lib.c and ASoC PCM core code. Jaroslav
On 2023-09-11 9:35 AM, Jaroslav Kysela wrote: > On 08. 09. 23 16:36, Cezary Rojewski wrote: ... >> After reading all of this again, I'm fine with rewording MSBITS_32 to >> MSBITS_MAX. >> >> As I do not see any other points to address here and review of v1 has no >> points to address either, I'll send v2 with this single change. If I'd >> missed anything, let me know. > > The subformat bitmask should be also refined/updated depending on the > selected format. > > https://lore.kernel.org/alsa-devel/f97bbbd5-1397-f5d3-5ccf-420ec813deac@perex.cz/ > > It requires new code in pcm_lib.c and ASoC PCM core code. Could you help me understand what new code is needed? The get_subformat() example raised more questions than answers. The patchset defines snd_pcm_subformat_width(), perhaps you meant that I should update that function by adding paramter 'format' to its parameters list and handle it accordingly? Any guidance would be much appreciated.
On 2023-09-11 10:43 AM, Cezary Rojewski wrote: > On 2023-09-11 9:35 AM, Jaroslav Kysela wrote: >> On 08. 09. 23 16:36, Cezary Rojewski wrote: ... >>> After reading all of this again, I'm fine with rewording MSBITS_32 to >>> MSBITS_MAX. >>> >>> As I do not see any other points to address here and review of v1 has no >>> points to address either, I'll send v2 with this single change. If I'd >>> missed anything, let me know. >> >> The subformat bitmask should be also refined/updated depending on the >> selected format. >> >> https://lore.kernel.org/alsa-devel/f97bbbd5-1397-f5d3-5ccf-420ec813deac@perex.cz/ >> >> It requires new code in pcm_lib.c and ASoC PCM core code. > > Could you help me understand what new code is needed? The > get_subformat() example raised more questions than answers. The patchset > defines snd_pcm_subformat_width(), perhaps you meant that I should > update that function by adding paramter 'format' to its parameters list > and handle it accordingly? > > Any guidance would be much appreciated. What I come up with is a hw_rule for subformat that I add in snd_pcm_hw_constraints_init(). That piece, plus both STD and MSBITS_MAX ORed into hw->subformats in snd_pcm_hw_constraints_complete() make things spin. static int snd_pcm_hw_rule_subformat(struct snd_pcm_hw_params *params, struct snd_pcm_hw_rule *rule) { struct snd_mask *subformat_mask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_SUBFORMAT); struct snd_mask *format_mask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); snd_pcm_format_t f; struct snd_mask m; int width; snd_mask_none(&m); snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_STD); snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_MAX); pcm_for_each_format(f) { if (!snd_mask_test_format(format_mask, f)) continue; width = snd_pcm_format_width(f); switch (width) { case 32: snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_20); snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_24); break; default: break; } } return snd_mask_refine(subformat_mask, &m); } However, this means snd_hdac_query_supported_pcm() becomes confusing as you need to MSBITS_MAX regardless of what the codec supports. After spending additional few hours on this, I'd say I preferred how things look with MSBITS_32 instead. STD and MSBITS_MAX existing simultaneously is confusing too. Czarek
On 11. 09. 23 17:45, Cezary Rojewski wrote: > On 2023-09-11 10:43 AM, Cezary Rojewski wrote: >> On 2023-09-11 9:35 AM, Jaroslav Kysela wrote: >>> On 08. 09. 23 16:36, Cezary Rojewski wrote: > > ... > >>>> After reading all of this again, I'm fine with rewording MSBITS_32 to >>>> MSBITS_MAX. >>>> >>>> As I do not see any other points to address here and review of v1 has no >>>> points to address either, I'll send v2 with this single change. If I'd >>>> missed anything, let me know. >>> >>> The subformat bitmask should be also refined/updated depending on the >>> selected format. >>> >>> https://lore.kernel.org/alsa-devel/f97bbbd5-1397-f5d3-5ccf-420ec813deac@perex.cz/ >>> >>> It requires new code in pcm_lib.c and ASoC PCM core code. >> >> Could you help me understand what new code is needed? The >> get_subformat() example raised more questions than answers. The patchset >> defines snd_pcm_subformat_width(), perhaps you meant that I should >> update that function by adding paramter 'format' to its parameters list >> and handle it accordingly? >> >> Any guidance would be much appreciated. > > What I come up with is a hw_rule for subformat that I add in > snd_pcm_hw_constraints_init(). That piece, plus both STD and MSBITS_MAX > ORed into hw->subformats in snd_pcm_hw_constraints_complete() make > things spin. > > static int snd_pcm_hw_rule_subformat(struct snd_pcm_hw_params *params, > struct snd_pcm_hw_rule *rule) > { > struct snd_mask *subformat_mask = hw_param_mask(params, > SNDRV_PCM_HW_PARAM_SUBFORMAT); > struct snd_mask *format_mask = hw_param_mask(params, > SNDRV_PCM_HW_PARAM_FORMAT); > snd_pcm_format_t f; > struct snd_mask m; > int width; > > snd_mask_none(&m); > snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_STD); > snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_MAX); > > pcm_for_each_format(f) { > if (!snd_mask_test_format(format_mask, f)) > continue; > > width = snd_pcm_format_width(f); > switch (width) { > case 32: > snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_20); > snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_24); > break; > default: > break; > } > } > > return snd_mask_refine(subformat_mask, &m); > } > > > However, this means snd_hdac_query_supported_pcm() becomes confusing as > you need to MSBITS_MAX regardless of what the codec supports. > After spending additional few hours on this, I'd say I preferred how > things look with MSBITS_32 instead. STD and MSBITS_MAX existing > simultaneously is confusing too. This is not a correct implementation. Many things are missing including the proper subformats filter. I sent my own version here for discussion: https://lore.kernel.org/alsa-devel/20230912162526.7138-1-perex@perex.cz/ Jaroslav
On 2023-09-12 6:30 PM, Jaroslav Kysela wrote: > On 11. 09. 23 17:45, Cezary Rojewski wrote: >> On 2023-09-11 10:43 AM, Cezary Rojewski wrote: ... >>> >>> Could you help me understand what new code is needed? The >>> get_subformat() example raised more questions than answers. The patchset >>> defines snd_pcm_subformat_width(), perhaps you meant that I should >>> update that function by adding paramter 'format' to its parameters list >>> and handle it accordingly? >>> >>> Any guidance would be much appreciated. >> >> What I come up with is a hw_rule for subformat that I add in >> snd_pcm_hw_constraints_init(). That piece, plus both STD and MSBITS_MAX >> ORed into hw->subformats in snd_pcm_hw_constraints_complete() make >> things spin. >> >> static int snd_pcm_hw_rule_subformat(struct snd_pcm_hw_params *params, >> struct snd_pcm_hw_rule *rule) >> { >> struct snd_mask *subformat_mask = hw_param_mask(params, >> SNDRV_PCM_HW_PARAM_SUBFORMAT); >> struct snd_mask *format_mask = hw_param_mask(params, >> SNDRV_PCM_HW_PARAM_FORMAT); >> snd_pcm_format_t f; >> struct snd_mask m; >> int width; >> >> snd_mask_none(&m); >> snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_STD); >> snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_MAX); >> >> pcm_for_each_format(f) { >> if (!snd_mask_test_format(format_mask, f)) >> continue; >> >> width = snd_pcm_format_width(f); >> switch (width) { >> case 32: >> snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_20); >> snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_24); >> break; >> default: >> break; >> } >> } >> >> return snd_mask_refine(subformat_mask, &m); >> } >> >> >> However, this means snd_hdac_query_supported_pcm() becomes confusing as >> you need to MSBITS_MAX regardless of what the codec supports. >> After spending additional few hours on this, I'd say I preferred how >> things look with MSBITS_32 instead. STD and MSBITS_MAX existing >> simultaneously is confusing too. > > This is not a correct implementation. Many things are missing including > the proper subformats filter. I sent my own version here for discussion: > > https://lore.kernel.org/alsa-devel/20230912162526.7138-1-perex@perex.cz/ I do appreciate the input but I expected that, through guidance, this patch gets updated. Sending a separate patch, not connected to this patchset - not even a single reference within the commit message body - is not nice. I'd rather send v2 with your patch incorporated as a part of the patchset. The subformat improvement alone, without showcasing the users is confusing to the reviewers. Czarek
On 13. 09. 23 9:44, Cezary Rojewski wrote: > On 2023-09-12 6:30 PM, Jaroslav Kysela wrote: >> On 11. 09. 23 17:45, Cezary Rojewski wrote: >>> On 2023-09-11 10:43 AM, Cezary Rojewski wrote: > > ... > >>>> >>>> Could you help me understand what new code is needed? The >>>> get_subformat() example raised more questions than answers. The patchset >>>> defines snd_pcm_subformat_width(), perhaps you meant that I should >>>> update that function by adding paramter 'format' to its parameters list >>>> and handle it accordingly? >>>> >>>> Any guidance would be much appreciated. >>> >>> What I come up with is a hw_rule for subformat that I add in >>> snd_pcm_hw_constraints_init(). That piece, plus both STD and MSBITS_MAX >>> ORed into hw->subformats in snd_pcm_hw_constraints_complete() make >>> things spin. >>> >>> static int snd_pcm_hw_rule_subformat(struct snd_pcm_hw_params *params, >>> struct snd_pcm_hw_rule *rule) >>> { >>> struct snd_mask *subformat_mask = hw_param_mask(params, >>> SNDRV_PCM_HW_PARAM_SUBFORMAT); >>> struct snd_mask *format_mask = hw_param_mask(params, >>> SNDRV_PCM_HW_PARAM_FORMAT); >>> snd_pcm_format_t f; >>> struct snd_mask m; >>> int width; >>> >>> snd_mask_none(&m); >>> snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_STD); >>> snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_MAX); >>> >>> pcm_for_each_format(f) { >>> if (!snd_mask_test_format(format_mask, f)) >>> continue; >>> >>> width = snd_pcm_format_width(f); >>> switch (width) { >>> case 32: >>> snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_20); >>> snd_mask_set(&m, SNDRV_PCM_SUBFORMAT_MSBITS_24); >>> break; >>> default: >>> break; >>> } >>> } >>> >>> return snd_mask_refine(subformat_mask, &m); >>> } >>> >>> >>> However, this means snd_hdac_query_supported_pcm() becomes confusing as >>> you need to MSBITS_MAX regardless of what the codec supports. >>> After spending additional few hours on this, I'd say I preferred how >>> things look with MSBITS_32 instead. STD and MSBITS_MAX existing >>> simultaneously is confusing too. >> >> This is not a correct implementation. Many things are missing including >> the proper subformats filter. I sent my own version here for discussion: >> >> https://lore.kernel.org/alsa-devel/20230912162526.7138-1-perex@perex.cz/ > > I do appreciate the input but I expected that, through guidance, this > patch gets updated. Sending a separate patch, not connected to this > patchset - not even a single reference within the commit message body - > is not nice. The basic API extension is self-contained and I marked it as RFC. The connection was added to this thread. > I'd rather send v2 with your patch incorporated as a part of the > patchset. No problem. Please, add these cosmetic changes to my patch: diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index f414f8fd217b..cb376e428f59 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1412,9 +1412,10 @@ static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params, struct snd_mask m; struct snd_mask *fmask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_FORMAT); struct snd_mask *mask = hw_param_mask(params, SNDRV_PCM_HW_PARAM_SUBFORMAT); + bool found; + snd_mask_none(&m); snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_STD); - bool found; pcm_for_each_format(k) { if (!snd_mask_test(fmask, k)) continue; @@ -1437,7 +1438,6 @@ static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params, * @runtime: PCM runtime instance * @cond: condition bits * @subformats: array with struct snd_pcm_subformat elements - * @nmemd: size of array with struct snd_pcm_subformat elements * * This constraint will set relation between format and subformats. * The STD and MAX subformats are handled automatically. If the driver Jaroslav
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 19f564606ac4..f78441ff9d85 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -31,6 +31,7 @@ struct snd_pcm_hardware { unsigned int info; /* SNDRV_PCM_INFO_* */ u64 formats; /* SNDRV_PCM_FMTBIT_* */ + u64 subformats; /* SNDRV_PCM_SUBFMTBIT_* */ unsigned int rates; /* SNDRV_PCM_RATE_* */ unsigned int rate_min; /* min rate */ unsigned int rate_max; /* max rate */ @@ -219,6 +220,12 @@ struct snd_pcm_ops { #define SNDRV_PCM_FMTBIT_U20 SNDRV_PCM_FMTBIT_U20_BE #endif +#define _SNDRV_PCM_SUBFMTBIT(fmt) BIT((__force int)SNDRV_PCM_SUBFORMAT_##fmt) +#define SNDRV_PCM_SUBFMTBIT_STD _SNDRV_PCM_SUBFMTBIT(STD) +#define SNDRV_PCM_SUBFMTBIT_MSBITS_20 _SNDRV_PCM_SUBFMTBIT(MSBITS_20) +#define SNDRV_PCM_SUBFMTBIT_MSBITS_24 _SNDRV_PCM_SUBFMTBIT(MSBITS_24) +#define SNDRV_PCM_SUBFMTBIT_MSBITS_32 _SNDRV_PCM_SUBFMTBIT(MSBITS_32) + struct snd_pcm_file { struct snd_pcm_substream *substream; int no_compat_mmap; @@ -1130,6 +1137,7 @@ int snd_pcm_format_physical_width(snd_pcm_format_t format); /* in bits */ ssize_t snd_pcm_format_size(snd_pcm_format_t format, size_t samples); const unsigned char *snd_pcm_format_silence_64(snd_pcm_format_t format); int snd_pcm_format_set_silence(snd_pcm_format_t format, void *buf, unsigned int frames); +int snd_pcm_subformat_width(snd_pcm_subformat_t subformat); void snd_pcm_set_ops(struct snd_pcm * pcm, int direction, const struct snd_pcm_ops *ops); diff --git a/include/sound/pcm_params.h b/include/sound/pcm_params.h index ba184f49f7e1..f09ae37ed44d 100644 --- a/include/sound/pcm_params.h +++ b/include/sound/pcm_params.h @@ -362,6 +362,8 @@ static inline int params_physical_width(const struct snd_pcm_hw_params *p) return snd_pcm_format_physical_width(params_format(p)); } +int params_bps(const struct snd_pcm_hw_params *p); + static inline void params_set_format(struct snd_pcm_hw_params *p, snd_pcm_format_t fmt) { diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index d6864f672630..27db9f3558fd 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -268,7 +268,10 @@ typedef int __bitwise snd_pcm_format_t; typedef int __bitwise snd_pcm_subformat_t; #define SNDRV_PCM_SUBFORMAT_STD ((__force snd_pcm_subformat_t) 0) -#define SNDRV_PCM_SUBFORMAT_LAST SNDRV_PCM_SUBFORMAT_STD +#define SNDRV_PCM_SUBFORMAT_MSBITS_20 ((__force snd_pcm_subformat_t) 1) +#define SNDRV_PCM_SUBFORMAT_MSBITS_24 ((__force snd_pcm_subformat_t) 2) +#define SNDRV_PCM_SUBFORMAT_MSBITS_32 ((__force snd_pcm_subformat_t) 3) +#define SNDRV_PCM_SUBFORMAT_LAST SNDRV_PCM_SUBFORMAT_MSBITS_32 #define SNDRV_PCM_INFO_MMAP 0x00000001 /* hardware supports mmap */ #define SNDRV_PCM_INFO_MMAP_VALID 0x00000002 /* period data are valid during transfer */ diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 9c121a921b04..4a60632220c2 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1706,6 +1706,36 @@ int snd_pcm_hw_param_last(struct snd_pcm_substream *pcm, } EXPORT_SYMBOL(snd_pcm_hw_param_last); +/** + * params_bps - Get the number of bits per the sample. + * @p: hardware parameters + * + * Return: The number of bits per sample based on the format, + * subformat and msbits the specified hw params has. + */ +int params_bps(const struct snd_pcm_hw_params *p) +{ + snd_pcm_subformat_t subformat = params_subformat(p); + snd_pcm_format_t format = params_format(p); + int width; + + switch (format) { + case SNDRV_PCM_FORMAT_S32_LE: + case SNDRV_PCM_FORMAT_U32_LE: + case SNDRV_PCM_FORMAT_S32_BE: + case SNDRV_PCM_FORMAT_U32_BE: + width = snd_pcm_subformat_width(subformat); + if (width) + return width; + if (p->msbits) + return (int)p->msbits; + fallthrough; + default: + return snd_pcm_format_width(format); + } +} +EXPORT_SYMBOL(params_bps); + static int snd_pcm_lib_ioctl_reset(struct snd_pcm_substream *substream, void *arg) { diff --git a/sound/core/pcm_misc.c b/sound/core/pcm_misc.c index 5588b6a1ee8b..127bb36fd2e4 100644 --- a/sound/core/pcm_misc.c +++ b/sound/core/pcm_misc.c @@ -482,6 +482,29 @@ int snd_pcm_format_set_silence(snd_pcm_format_t format, void *data, unsigned int } EXPORT_SYMBOL(snd_pcm_format_set_silence); +/** + * snd_pcm_subformat_width - return the bit-width of the subformat + * @subformat: the subformat to check + * + * Return: The bit-width of the subformat, or a negative error code + * otherwise. + */ +int snd_pcm_subformat_width(snd_pcm_subformat_t subformat) +{ + switch (subformat) { + case SNDRV_PCM_SUBFORMAT_MSBITS_20: + return 20; + case SNDRV_PCM_SUBFORMAT_MSBITS_24: + return 24; + case SNDRV_PCM_SUBFORMAT_MSBITS_32: + return 32; + case SNDRV_PCM_SUBFORMAT_STD: + default: + return 0; + } +} +EXPORT_SYMBOL(snd_pcm_subformat_width); + /** * snd_pcm_hw_limit_rates - determine rate_min/rate_max fields * @hw: the pcm hw instance diff --git a/tools/include/uapi/sound/asound.h b/tools/include/uapi/sound/asound.h index 0aa955aa8246..eb45a698767f 100644 --- a/tools/include/uapi/sound/asound.h +++ b/tools/include/uapi/sound/asound.h @@ -267,7 +267,10 @@ typedef int __bitwise snd_pcm_format_t; typedef int __bitwise snd_pcm_subformat_t; #define SNDRV_PCM_SUBFORMAT_STD ((__force snd_pcm_subformat_t) 0) -#define SNDRV_PCM_SUBFORMAT_LAST SNDRV_PCM_SUBFORMAT_STD +#define SNDRV_PCM_SUBFORMAT_MSBITS_20 ((__force snd_pcm_subformat_t) 1) +#define SNDRV_PCM_SUBFORMAT_MSBITS_24 ((__force snd_pcm_subformat_t) 2) +#define SNDRV_PCM_SUBFORMAT_MSBITS_32 ((__force snd_pcm_subformat_t) 3) +#define SNDRV_PCM_SUBFORMAT_LAST SNDRV_PCM_SUBFORMAT_MSBITS_32 #define SNDRV_PCM_INFO_MMAP 0x00000001 /* hardware supports mmap */ #define SNDRV_PCM_INFO_MMAP_VALID 0x00000002 /* period data are valid during transfer */
Improve granularity of format selection for S32/U32 formats by adding constants representing 20, 24 and 32 most significant bits. To make it easy for drivers to utilize those constants, introduce snd_pcm_subformat_width() and params_bps(). While the former is self-explanatory, the latter returns the bit-per-sample value based on format, subformat and msbits characteristics of the provided hw_params. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- include/sound/pcm.h | 8 ++++++++ include/sound/pcm_params.h | 2 ++ include/uapi/sound/asound.h | 5 ++++- sound/core/pcm_lib.c | 30 ++++++++++++++++++++++++++++++ sound/core/pcm_misc.c | 23 +++++++++++++++++++++++ tools/include/uapi/sound/asound.h | 5 ++++- 6 files changed, 71 insertions(+), 2 deletions(-)