| Message ID | 20240222173649.1447549-1-perex@perex.cz (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 85df6b5a6658c788d7e27d52ea334aa83abcbf56 |
| Headers | show |
| Series | ALSA: pcm: clarify and fix default msbits value for all formats | expand |
On Thu, 22 Feb 2024 18:36:49 +0100, Jaroslav Kysela wrote: > > Return used most significant bits from sample bit-width rather than the whole > physical sample word size. The starting bit offset is defined in the format > itself. > > The behaviour is not changed for 32-bit formats like S32_LE. But with this > change - msbits value 24 instead 32 is returned for 24-bit formats like S24_LE > etc. > > Also, commit 2112aa034907 ("ALSA: pcm: Introduce MSBITS subformat interface") > compares sample bit-width not physical sample bit-width to reset MSBITS_MAX bit > from the subformat bitmask. > > Probably no applications are using msbits value for other than S32_LE/U32_LE > formats, because no drivers are reducing msbits value for other formats (with > the msb offset) at the moment. > > For sanity, increase PCM protocol version, letting the user space to detect > the changed behaviour. > > Signed-off-by: Jaroslav Kysela <perex@perex.cz> Hmm, the idea is nice, but I hesitate to take this as is. Basically the kernel should keep the backward compatibility, and this won't (although the feature is very minor). IMO, it'd be safer to check user_pversion and applies the new behavior only with it being >= 2.0.17, for example. thanks, Takashi > --- > include/uapi/sound/asound.h | 4 ++-- > sound/core/pcm_native.c | 5 +++++ > 2 files changed, 7 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h > index d5b9cfbd9cea..628d46a0da92 100644 > --- a/include/uapi/sound/asound.h > +++ b/include/uapi/sound/asound.h > @@ -142,7 +142,7 @@ struct snd_hwdep_dsp_image { > * * > *****************************************************************************/ > > -#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 16) > +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 17) > > typedef unsigned long snd_pcm_uframes_t; > typedef signed long snd_pcm_sframes_t; > @@ -416,7 +416,7 @@ struct snd_pcm_hw_params { > unsigned int rmask; /* W: requested masks */ > unsigned int cmask; /* R: changed masks */ > unsigned int info; /* R: Info flags for returned setup */ > - unsigned int msbits; /* R: used most significant bits */ > + unsigned int msbits; /* R: used most significant bits (in sample bit-width) */ > unsigned int rate_num; /* R: rate numerator */ > unsigned int rate_den; /* R: rate denominator */ > snd_pcm_uframes_t fifo_size; /* R: chip FIFO size in frames */ > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index f5ff00f99788..21baf6bf7e25 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -486,6 +486,11 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream, > i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS); > if (snd_interval_single(i)) > params->msbits = snd_interval_value(i); > + m = hw_param_mask_c(params, SNDRV_PCM_HW_PARAM_FORMAT); > + if (snd_mask_single(m)) { > + snd_pcm_format_t format = (__force snd_pcm_format_t)snd_mask_min(m); > + params->msbits = snd_pcm_format_width(format); > + } > } > > if (params->msbits) { > -- > 2.43.0 >
On 22. 02. 24 19:33, Takashi Iwai wrote: > On Thu, 22 Feb 2024 18:36:49 +0100, > Jaroslav Kysela wrote: >> >> Return used most significant bits from sample bit-width rather than the whole >> physical sample word size. The starting bit offset is defined in the format >> itself. >> >> The behaviour is not changed for 32-bit formats like S32_LE. But with this >> change - msbits value 24 instead 32 is returned for 24-bit formats like S24_LE >> etc. >> >> Also, commit 2112aa034907 ("ALSA: pcm: Introduce MSBITS subformat interface") >> compares sample bit-width not physical sample bit-width to reset MSBITS_MAX bit >> from the subformat bitmask. >> >> Probably no applications are using msbits value for other than S32_LE/U32_LE >> formats, because no drivers are reducing msbits value for other formats (with >> the msb offset) at the moment. >> >> For sanity, increase PCM protocol version, letting the user space to detect >> the changed behaviour. >> >> Signed-off-by: Jaroslav Kysela <perex@perex.cz> > > Hmm, the idea is nice, but I hesitate to take this as is. > > Basically the kernel should keep the backward compatibility, and this > won't (although the feature is very minor). > > IMO, it'd be safer to check user_pversion and applies the new behavior > only with it being >= 2.0.17, for example. It's not so easy. The user_pversion is detached from substream (pcm_file structure) and propagating this value to all related functions creates really ugly code. If you won't accept this ASIS, I propose to add new flags to the hw_params structure to alter the msbits behaviour. But I can hardly see any impact to the user space. The affected format/msbits combination was never used in the kernel drivers. It's more like a clarification than a fix. Jaroslav
On Thu, 22 Feb 2024 19:59:14 +0100, Jaroslav Kysela wrote: > > On 22. 02. 24 19:33, Takashi Iwai wrote: > > On Thu, 22 Feb 2024 18:36:49 +0100, > > Jaroslav Kysela wrote: > >> > >> Return used most significant bits from sample bit-width rather than the whole > >> physical sample word size. The starting bit offset is defined in the format > >> itself. > >> > >> The behaviour is not changed for 32-bit formats like S32_LE. But with this > >> change - msbits value 24 instead 32 is returned for 24-bit formats like S24_LE > >> etc. > >> > >> Also, commit 2112aa034907 ("ALSA: pcm: Introduce MSBITS subformat interface") > >> compares sample bit-width not physical sample bit-width to reset MSBITS_MAX bit > >> from the subformat bitmask. > >> > >> Probably no applications are using msbits value for other than S32_LE/U32_LE > >> formats, because no drivers are reducing msbits value for other formats (with > >> the msb offset) at the moment. > >> > >> For sanity, increase PCM protocol version, letting the user space to detect > >> the changed behaviour. > >> > >> Signed-off-by: Jaroslav Kysela <perex@perex.cz> > > > > Hmm, the idea is nice, but I hesitate to take this as is. > > > > Basically the kernel should keep the backward compatibility, and this > > won't (although the feature is very minor). > > > > IMO, it'd be safer to check user_pversion and applies the new behavior > > only with it being >= 2.0.17, for example. > > It's not so easy. The user_pversion is detached from substream > (pcm_file structure) and propagating this value to all related > functions creates really ugly code. > > If you won't accept this ASIS, I propose to add new flags to the > hw_params structure to alter the msbits behaviour. But I can hardly > see any impact to the user space. The affected format/msbits > combination was never used in the kernel drivers. It's more like a > clarification than a fix. OK, then let's take this. I checked the major sound backend apps, and they don't care about msbits, so the influence must be pretty low. thanks, Takashi
On Fri, 23 Feb 2024 09:45:40 +0100, Takashi Iwai wrote: > > On Thu, 22 Feb 2024 19:59:14 +0100, > Jaroslav Kysela wrote: > > > > On 22. 02. 24 19:33, Takashi Iwai wrote: > > > On Thu, 22 Feb 2024 18:36:49 +0100, > > > Jaroslav Kysela wrote: > > >> > > >> Return used most significant bits from sample bit-width rather than the whole > > >> physical sample word size. The starting bit offset is defined in the format > > >> itself. > > >> > > >> The behaviour is not changed for 32-bit formats like S32_LE. But with this > > >> change - msbits value 24 instead 32 is returned for 24-bit formats like S24_LE > > >> etc. > > >> > > >> Also, commit 2112aa034907 ("ALSA: pcm: Introduce MSBITS subformat interface") > > >> compares sample bit-width not physical sample bit-width to reset MSBITS_MAX bit > > >> from the subformat bitmask. > > >> > > >> Probably no applications are using msbits value for other than S32_LE/U32_LE > > >> formats, because no drivers are reducing msbits value for other formats (with > > >> the msb offset) at the moment. > > >> > > >> For sanity, increase PCM protocol version, letting the user space to detect > > >> the changed behaviour. > > >> > > >> Signed-off-by: Jaroslav Kysela <perex@perex.cz> > > > > > > Hmm, the idea is nice, but I hesitate to take this as is. > > > > > > Basically the kernel should keep the backward compatibility, and this > > > won't (although the feature is very minor). > > > > > > IMO, it'd be safer to check user_pversion and applies the new behavior > > > only with it being >= 2.0.17, for example. > > > > It's not so easy. The user_pversion is detached from substream > > (pcm_file structure) and propagating this value to all related > > functions creates really ugly code. > > > > If you won't accept this ASIS, I propose to add new flags to the > > hw_params structure to alter the msbits behaviour. But I can hardly > > see any impact to the user space. The affected format/msbits > > combination was never used in the kernel drivers. It's more like a > > clarification than a fix. > > OK, then let's take this. I checked the major sound backend apps, and > they don't care about msbits, so the influence must be pretty low. To be more exact, both PA and PW have the same code to check the msbits but they merely set "alsa.resolution_bits" property, and it's not referred in the standard configs. Takashi
diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index d5b9cfbd9cea..628d46a0da92 100644 --- a/include/uapi/sound/asound.h +++ b/include/uapi/sound/asound.h @@ -142,7 +142,7 @@ struct snd_hwdep_dsp_image { * * *****************************************************************************/ -#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 16) +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 17) typedef unsigned long snd_pcm_uframes_t; typedef signed long snd_pcm_sframes_t; @@ -416,7 +416,7 @@ struct snd_pcm_hw_params { unsigned int rmask; /* W: requested masks */ unsigned int cmask; /* R: changed masks */ unsigned int info; /* R: Info flags for returned setup */ - unsigned int msbits; /* R: used most significant bits */ + unsigned int msbits; /* R: used most significant bits (in sample bit-width) */ unsigned int rate_num; /* R: rate numerator */ unsigned int rate_den; /* R: rate denominator */ snd_pcm_uframes_t fifo_size; /* R: chip FIFO size in frames */ diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index f5ff00f99788..21baf6bf7e25 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -486,6 +486,11 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream, i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_SAMPLE_BITS); if (snd_interval_single(i)) params->msbits = snd_interval_value(i); + m = hw_param_mask_c(params, SNDRV_PCM_HW_PARAM_FORMAT); + if (snd_mask_single(m)) { + snd_pcm_format_t format = (__force snd_pcm_format_t)snd_mask_min(m); + params->msbits = snd_pcm_format_width(format); + } } if (params->msbits) {
Return used most significant bits from sample bit-width rather than the whole physical sample word size. The starting bit offset is defined in the format itself. The behaviour is not changed for 32-bit formats like S32_LE. But with this change - msbits value 24 instead 32 is returned for 24-bit formats like S24_LE etc. Also, commit 2112aa034907 ("ALSA: pcm: Introduce MSBITS subformat interface") compares sample bit-width not physical sample bit-width to reset MSBITS_MAX bit from the subformat bitmask. Probably no applications are using msbits value for other than S32_LE/U32_LE formats, because no drivers are reducing msbits value for other formats (with the msb offset) at the moment. For sanity, increase PCM protocol version, letting the user space to detect the changed behaviour. Signed-off-by: Jaroslav Kysela <perex@perex.cz> --- include/uapi/sound/asound.h | 4 ++-- sound/core/pcm_native.c | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-)