| Message ID | 20230912162526.7138-1-perex@perex.cz (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] ALSA: pcm: Introduce MSBITS subformat API extension | expand |
On 2023-09-12 6:25 PM, Jaroslav Kysela wrote: > Improve granularity of format selection for linear formats by adding > constants representing MAX, 20, 24 most significant bits. > > The MAX means the maximum number of significant bits which can > the physical format hold. For 32-bit formats, MAX is related > to 32 bits. For 8-bit formats, MAX is related to 8 bits etc. > > The drivers may use snd_pcm_hw_constraint_subformats with > a simple format -> subformats table. The code looks good overall. I have few comments and nitpicks regarding readability - comes from person who recently was digging through hw_rule and related code and found themselves lost. Examples such as this patch are good how-to references in hw_rule world. - The message lacks reference to the original patchset. I'd suggest to have it here. Either that or incorporate it directly into the patchset. And I must admit I'm a bit surprised by the lack of few CCs when compared to the original subject. > Cc: Cezary Rojewski <cezary.rojewski@intel.com> > Signed-off-by: Jaroslav Kysela <perex@perex.cz> > --- > include/sound/pcm.h | 17 +++++++++ > include/uapi/sound/asound.h | 7 ++-- > sound/core/pcm_lib.c | 59 +++++++++++++++++++++++++++++++ > sound/core/pcm_native.c | 18 +++++++--- > tools/include/uapi/sound/asound.h | 7 ++-- > 5 files changed, 100 insertions(+), 8 deletions(-) > > diff --git a/include/sound/pcm.h b/include/sound/pcm.h > index 2a815373dac1..59ad45b42e03 100644 > --- a/include/sound/pcm.h > +++ b/include/sound/pcm.h > @@ -217,6 +217,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_MAX _SNDRV_PCM_SUBFMTBIT(MSBITS_MAX) > +#define SNDRV_PCM_SUBFMTBIT_MSBITS_20 _SNDRV_PCM_SUBFMTBIT(MSBITS_20) > +#define SNDRV_PCM_SUBFMTBIT_MSBITS_24 _SNDRV_PCM_SUBFMTBIT(MSBITS_24) > + > struct snd_pcm_file { > struct snd_pcm_substream *substream; > int no_compat_mmap; > @@ -290,6 +296,13 @@ struct snd_pcm_hw_constraint_ranges { > unsigned int mask; > }; > > +#define SNDRV_PCM_FORMAT_CONSTRAINT_END (~0) > + > +struct snd_pcm_hw_constraint_subformat { > + snd_pcm_format_t format; /* SNDRV_PCM_FORMAT_* */ > + u32 subformats; /* SNDRV_PCM_SUBFMTBIT_* */ From what I know, we are dealing with u64 masks here. Why u32 here? > +}; > + > /* > * userspace-provided audio timestamp config to kernel, > * structure is for internal use only and filled with dedicated unpack routine > @@ -375,6 +388,7 @@ struct snd_pcm_runtime { > unsigned int rate_num; > unsigned int rate_den; > unsigned int no_period_wakeup: 1; > + unsigned int subformat_constraint: 1; > > /* -- SW params; see struct snd_pcm_sw_params for comments -- */ > int tstamp_mode; > @@ -1068,6 +1082,9 @@ int snd_pcm_hw_constraint_ratdens(struct snd_pcm_runtime *runtime, > unsigned int cond, > snd_pcm_hw_param_t var, > const struct snd_pcm_hw_constraint_ratdens *r); > +int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime, > + unsigned int cond, > + struct snd_pcm_hw_constraint_subformat *subformats); > int snd_pcm_hw_constraint_msbits(struct snd_pcm_runtime *runtime, > unsigned int cond, > unsigned int width, ... > diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c > index a11cd7d6295f..f414f8fd217b 100644 > --- a/sound/core/pcm_lib.c > +++ b/sound/core/pcm_lib.c > @@ -1404,6 +1404,65 @@ int snd_pcm_hw_constraint_ratdens(struct snd_pcm_runtime *runtime, > } > EXPORT_SYMBOL(snd_pcm_hw_constraint_ratdens); > > +static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params, > + struct snd_pcm_hw_rule *rule) > +{ > + const struct snd_pcm_hw_constraint_subformat *sf; What's 'sf'? I'd suggest to be more descriptive here. > + snd_pcm_format_t k; Internally I was utilizing 'f' as that's what macro expects in its declaration. s/k/f/ > + 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); So, the reason I opted for 'subformat_mask' and 'format_mask' is that otherwise reader is crowded with ambiguous 'mask' and its relatives. I'd avoid shortcuts when multiple variables touch the same subject. s/fmask/format_mask/ s/mask/subformat_mask/ > + snd_mask_none(&m); > + snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_STD); > + bool found; Suggestion is to add newline before declaration and execution blocks. Also, why not reserve-christmass-tree model? There quite a few variables here. > + pcm_for_each_format(k) { > + if (!snd_mask_test(fmask, k)) > + continue; Similarly here. A newline would effectively separate conditional for-loop from the actual execution block. > + found = false; > + for (sf = rule->private; sf && sf->format != SNDRV_PCM_FORMAT_CONSTRAINT_END; sf++) { > + if (sf->format != k) > + continue; > + found = true; > + m.bits[0] |= sf->subformats; > + break; > + } > + if (!found && snd_pcm_format_linear(k)) For my own education, why checking if format is linear is essential here? Perhaps a comment? > + snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX); > + } > + return snd_mask_refine(mask, &m); > +} > + > +/** > + * snd_pcm_hw_constraint_subformats - add a hw constraint subformats rule > + * @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. I do not believe 'This constaint' brings any value. Reader is already aware of it. Starting from the 'Set' part brings the same value with fewer words. > + * The STD and MAX subformats are handled automatically. If the driver > + * does not set this constraint, only STD and MAX subformats are handled. > + * > + * Return: Zero if successful, or a negative error code on failure. > + */ > +int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime, > + unsigned int cond, > + struct snd_pcm_hw_constraint_subformat *subformats) > +{ > + int ret; > + > + ret = snd_pcm_hw_rule_add(runtime, cond, -1, > + snd_pcm_hw_rule_subformats, > + (void*) subformats, > + SNDRV_PCM_HW_PARAM_SUBFORMAT, > + SNDRV_PCM_HW_PARAM_FORMAT, -1); > + if (ret < 0) > + return ret; > + runtime->subformat_constraint = 1; > + return 0; > +} > +EXPORT_SYMBOL(snd_pcm_hw_constraint_subformats); > + > static int snd_pcm_hw_rule_msbits(struct snd_pcm_hw_params *params, > struct snd_pcm_hw_rule *rule) > { > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index bd9ddf412b46..69609e6aa507 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -479,6 +479,7 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream, > { > const struct snd_interval *i; > const struct snd_mask *m; > + struct snd_mask *m_rw; Two masks named 'm' and 'm_rw' is confusing in my opinion. The 'm_rw' is used only in subformat case so the name could be more descriptive. > int err; > > if (!params->msbits) { > @@ -487,6 +488,14 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream, > params->msbits = snd_interval_value(i); > } > > + if (params->msbits) { > + m = hw_param_mask_c(params, SNDRV_PCM_HW_PARAM_FORMAT); > + if (snd_mask_single(m) && snd_pcm_format_width(snd_mask_min(m)) != params->msbits) { > + m_rw = hw_param_mask(params, SNDRV_PCM_HW_PARAM_SUBFORMAT); > + snd_mask_reset(m_rw, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX); > + } > + } > + > if (!params->rate_den) { > i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_RATE); > if (snd_interval_single(i)) { > @@ -2634,10 +2643,11 @@ static int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream) > if (err < 0) > return err; > > - err = snd_pcm_hw_constraint_mask(runtime, SNDRV_PCM_HW_PARAM_SUBFORMAT, > - PARAM_MASK_BIT(SNDRV_PCM_SUBFORMAT_STD)); > - if (err < 0) > - return err; > + if (!runtime->subformat_constraint) { I'd try to avoid another special-bit in the runtime space. But I might be wrong here and it's unavoidable. Let me ask though, why cannot we do the constraint unconditionally? > + err = snd_pcm_hw_constraint_subformats(runtime, 0, NULL); > + if (err < 0) > + return err; > + } > > err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_CHANNELS, > hw->channels_min, hw->channels_max);
On 13. 09. 23 10:22, Cezary Rojewski wrote: >> +struct snd_pcm_hw_constraint_subformat { >> + snd_pcm_format_t format; /* SNDRV_PCM_FORMAT_* */ >> + u32 subformats; /* SNDRV_PCM_SUBFMTBIT_* */ > > From what I know, we are dealing with u64 masks here. Why u32 here? It's not true. See the removed code which calls snd_pcm_hw_constraint_mask() for SNDRV_PCM_HW_PARAM_SUBFORMAT. Only the format is handled for 64 bits and the handling of other bits is purely optional, because masks can handle up to 256 bits: #define SNDRV_MASK_MAX 256 struct snd_mask { __u32 bits[(SNDRV_MASK_MAX+31)/32]; }; Because we used only few bits for SUBFORMAT, the 32 bit code is enough. We can expand this latter without any impact to the user space interface. >> +static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params, >> + struct snd_pcm_hw_rule *rule) >> +{ >> + const struct snd_pcm_hw_constraint_subformat *sf; > > What's 'sf'? I'd suggest to be more descriptive here. SubFormat. The larger name will not make the for loop more readable. The function is small, so reader is not lost. >> + snd_pcm_format_t k; > > Internally I was utilizing 'f' as that's what macro expects in its > declaration. s/k/f/ Copied from pcm_native.c - snd_pcm_hw_rule_format(). >> + 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); > > So, the reason I opted for 'subformat_mask' and 'format_mask' is that > otherwise reader is crowded with ambiguous 'mask' and its relatives. I'd > avoid shortcuts when multiple variables touch the same subject. > > s/fmask/format_mask/ > s/mask/subformat_mask/ Too long, the function is small. >> + snd_mask_none(&m); >> + snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_STD); >> + bool found; > > Suggestion is to add newline before declaration and execution blocks. > Also, why not reserve-christmass-tree model? There quite a few variables > here. Yes it should be shuffled. >> + pcm_for_each_format(k) { >> + if (!snd_mask_test(fmask, k)) >> + continue; > > Similarly here. A newline would effectively separate conditional > for-loop from the actual execution block. It's questionable. >> + found = false; >> + for (sf = rule->private; sf && sf->format != SNDRV_PCM_FORMAT_CONSTRAINT_END; sf++) { >> + if (sf->format != k) >> + continue; >> + found = true; >> + m.bits[0] |= sf->subformats; >> + break; >> + } >> + if (!found && snd_pcm_format_linear(k)) > > For my own education, why checking if format is linear is essential > here? Perhaps a comment? Subformat MSBITS are valid only for linear formats, aren't? It does not make sense to set MAX for other formats. >> + snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX); >> + } >> + return snd_mask_refine(mask, &m); >> +} >> + >> +/** >> + * snd_pcm_hw_constraint_subformats - add a hw constraint subformats rule >> + * @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. > > I do not believe 'This constaint' brings any value. Reader is already > aware of it. Starting from the 'Set' part brings the same value with > fewer words. Copied from snd_pcm_hw_constraint_msbits function. >> + * The STD and MAX subformats are handled automatically. If the driver >> + * does not set this constraint, only STD and MAX subformats are handled. >> + * >> + * Return: Zero if successful, or a negative error code on failure. >> + */ >> +int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime, >> + unsigned int cond, >> + struct snd_pcm_hw_constraint_subformat *subformats) >> +{ ... >> - err = snd_pcm_hw_constraint_mask(runtime, SNDRV_PCM_HW_PARAM_SUBFORMAT, >> - PARAM_MASK_BIT(SNDRV_PCM_SUBFORMAT_STD)); >> - if (err < 0) >> - return err; >> + if (!runtime->subformat_constraint) { > > I'd try to avoid another special-bit in the runtime space. But I might > be wrong here and it's unavoidable. Let me ask though, why cannot we do > the constraint unconditionally? This is for a special case when the drivers do not set snd_pcm_hw_constraint_subformats (all current drivers). In this case, the default is to handle STD and MAX subformat bits. This constraint should be applied only one time. So this prevents to install it twice. I'll send v2 to address some things from this discussion and kernel test robot. Jaroslav
On 2023-09-13 10:57 AM, Jaroslav Kysela wrote: > On 13. 09. 23 10:22, Cezary Rojewski wrote: > >>> +struct snd_pcm_hw_constraint_subformat { >>> + snd_pcm_format_t format; /* SNDRV_PCM_FORMAT_* */ >>> + u32 subformats; /* SNDRV_PCM_SUBFMTBIT_* */ >> >> From what I know, we are dealing with u64 masks here. Why u32 here? > > It's not true. See the removed code which calls > snd_pcm_hw_constraint_mask() for SNDRV_PCM_HW_PARAM_SUBFORMAT. Only the > format is handled for 64 bits and the handling of other bits is purely > optional, because masks can handle up to 256 bits: > > #define SNDRV_MASK_MAX 256 > > struct snd_mask { > __u32 bits[(SNDRV_MASK_MAX+31)/32]; > }; > > Because we used only few bits for SUBFORMAT, the 32 bit code is enough. > We can expand this latter without any impact to the user space interface. Thanks for explaining. I must admit that this "balance" is confusing though - up to 256 bits are supported, yet depending on the use-case, size is lowered for optimization reasons. >>> +static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params, >>> + struct snd_pcm_hw_rule *rule) >>> +{ >>> + const struct snd_pcm_hw_constraint_subformat *sf; >> >> What's 'sf'? I'd suggest to be more descriptive here. > > SubFormat. The larger name will not make the for loop more readable. The > function is small, so reader is not lost. Decided to do a "mix" of shortcuts and explicitness - fmask for format-mask, sfmask for subformat-mask and sf for cursor variable. >>> + snd_pcm_format_t k; >> >> Internally I was utilizing 'f' as that's what macro expects in its >> declaration. s/k/f/ > > Copied from pcm_native.c - snd_pcm_hw_rule_format(). I understand but this is not a technical argument. It's clearly more informative and cohesive to utilize 'f' in place of 'k'. The aforementioned function should be updated in the future too. >>> + 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); >> >> So, the reason I opted for 'subformat_mask' and 'format_mask' is that >> otherwise reader is crowded with ambiguous 'mask' and its relatives. I'd >> avoid shortcuts when multiple variables touch the same subject. >> >> s/fmask/format_mask/ >> s/mask/subformat_mask/ > > Too long, the function is small. As mentioned above, decided to do a mix instead of ignoring completely. I value readability much, and I believe many on the list value it too. The combination of m+mask+fmask is confusing at best. >>> + snd_mask_none(&m); >>> + snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_STD); >>> + bool found; >> >> Suggestion is to add newline before declaration and execution blocks. >> Also, why not reserve-christmass-tree model? There quite a few variables >> here. > > Yes it should be shuffled. Ack and addressed in the v2 series. >>> + pcm_for_each_format(k) { >>> + if (!snd_mask_test(fmask, k)) >>> + continue; >> >> Similarly here. A newline would effectively separate conditional >> for-loop from the actual execution block. > > It's questionable. A so-called blob of text should be avoided. This is not a trivial part of the code. Least we can do is flesh it out a bit plus provide a more self-explanatory variable names. >>> + found = false; >>> + for (sf = rule->private; sf && sf->format != >>> SNDRV_PCM_FORMAT_CONSTRAINT_END; sf++) { >>> + if (sf->format != k) >>> + continue; >>> + found = true; >>> + m.bits[0] |= sf->subformats; >>> + break; >>> + } >>> + if (!found && snd_pcm_format_linear(k)) >> >> For my own education, why checking if format is linear is essential >> here? Perhaps a comment? > > Subformat MSBITS are valid only for linear formats, aren't? It does not > make sense to set MAX for other formats. I cannot deduce any comment-to-add from this, so opted-out of adding any comment. >>> + snd_mask_set(&m, (__force >>> unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX); >>> + } >>> + return snd_mask_refine(mask, &m); >>> +} >>> + >>> +/** >>> + * snd_pcm_hw_constraint_subformats - add a hw constraint subformats >>> rule >>> + * @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. >> >> I do not believe 'This constaint' brings any value. Reader is already >> aware of it. Starting from the 'Set' part brings the same value with >> fewer words. > > Copied from snd_pcm_hw_constraint_msbits function. Again, I do understand the origin of this, but this is not a technical argument - if something can be improved even when copying, we should do so. >>> + * The STD and MAX subformats are handled automatically. If the driver >>> + * does not set this constraint, only STD and MAX subformats are >>> handled. >>> + * >>> + * Return: Zero if successful, or a negative error code on failure. >>> + */ >>> +int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime, >>> + unsigned int cond, >>> + struct snd_pcm_hw_constraint_subformat >>> *subformats) >>> +{ > > ... > >>> - err = snd_pcm_hw_constraint_mask(runtime, >>> SNDRV_PCM_HW_PARAM_SUBFORMAT, >>> - PARAM_MASK_BIT(SNDRV_PCM_SUBFORMAT_STD)); >>> - if (err < 0) >>> - return err; >>> + if (!runtime->subformat_constraint) { >> >> I'd try to avoid another special-bit in the runtime space. But I might >> be wrong here and it's unavoidable. Let me ask though, why cannot we do >> the constraint unconditionally? > > This is for a special case when the drivers do not set > snd_pcm_hw_constraint_subformats (all current drivers). In this case, > the default is to handle STD and MAX subformat bits. > > This constraint should be applied only one time. So this prevents to > install it twice. I believe we could avoid special-case approach. Have a copy/intersection helpers in place and utilize iterations-with-sentinel-entry. Provided such in v2 of my series. > I'll send v2 to address some things from this discussion and kernel test > robot.
On 18. 09. 23 15:55, Cezary Rojewski wrote: >>>> + if (!runtime->subformat_constraint) { >>> >>> I'd try to avoid another special-bit in the runtime space. But I might >>> be wrong here and it's unavoidable. Let me ask though, why cannot we do >>> the constraint unconditionally? >> >> This is for a special case when the drivers do not set >> snd_pcm_hw_constraint_subformats (all current drivers). In this case, >> the default is to handle STD and MAX subformat bits. >> >> This constraint should be applied only one time. So this prevents to >> install it twice. > > I believe we could avoid special-case approach. Have a copy/intersection > helpers in place and utilize iterations-with-sentinel-entry. Provided > such in v2 of my series. I don't think that it's required to carry the format->subformat map in struct snd_pcm_hardware. Only few drivers will use it, so the separate constraint is fine. Also, you can remove a lot of your added code to the pcm_misc and ASoC core (copy, masking, allocating) when the affected drivers install the map using the constraint call. Few points: 1) PCM API interface changes should be separate, you mixing unused helpers and separating vital functionality for drivers 2) if you copy 90% of my code, I don't think that Suggested-by: tag is fine Could you do your work on top of my patch? Jaroslav
On 2023-09-18 5:04 PM, Jaroslav Kysela wrote: > On 18. 09. 23 15:55, Cezary Rojewski wrote: ... >>> This is for a special case when the drivers do not set >>> snd_pcm_hw_constraint_subformats (all current drivers). In this case, >>> the default is to handle STD and MAX subformat bits. >>> >>> This constraint should be applied only one time. So this prevents to >>> install it twice. >> >> I believe we could avoid special-case approach. Have a copy/intersection >> helpers in place and utilize iterations-with-sentinel-entry. Provided >> such in v2 of my series. > > I don't think that it's required to carry the format->subformat map in > struct snd_pcm_hardware. Only few drivers will use it, so the separate > constraint is fine. Also, you can remove a lot of your added code to the > pcm_misc and ASoC core (copy, masking, allocating) when the affected > drivers install the map using the constraint call. I believe the question isn't how few or how many, but are there users or not. The answer to that question is: there are users of the subformat feature. Adding an array of subformats to the snd_pcm_hardware makes things explicit, example being sound/soc/intel/avs/pcm.c. That's a win from maintenance point of view. Another thing is that we could utilize subformat to drop msbits entirely in the future. To summarize, to make subformat a first class citizen, we should avoid special-casing anything related to it. > Few points: > > 1) PCM API interface changes should be separate, you mixing unused > helpers and separating vital functionality for drivers What I could do is shuffle the code a bit e.g.: let snd_pcm_hw_copy() be introduced along the ASoC changes. Frankly that was the initial approach (forgotten to update the commit message of 04/17 so it still talks about code that's no longer part of the commit). > 2) if you copy 90% of my code, I don't think that Suggested-by: tag is fine > > Could you do your work on top of my patch? I'm afraid this isn't a fair claim. The feature is driven by validation and this has been conducted be me or my folks entirely. Given the scarce guidance provided in [1] I still provided a valid WIP in [2] and expected to iterate over it given the feedback. Closing the discussion by taking a single patch away from the series and re-authoring it is not a welcoming way to do a review. Perhaps Co-developed-by: then? Please note that the code differs. I do believe that splitting the API and the constrains into separate patches is a better approach from maintenance point of view. Proposed readability improvements have also been applied in v2. For reasons provided in previous paragraphs, I chose to avoid the chicken-bit and treat subformat constraints in generic fashion. Also, validation shows that without updating snd_pcm_subformat_names[] in pcm.c the code ends with UBSAN during runtime. I've already addressed that, even in v1. I'm happy to continue the technical discussion as there are still points to discuss. Let's do so in v2 of the series [3]. Kind regards, Czarek [1]: https://lore.kernel.org/alsa-devel/337fe790-fdbc-1208-080d-5bcf9264fc65@perex.cz/ [2]: https://lore.kernel.org/alsa-devel/8d76a1d8-e85c-b699-34a0-ecea6edc2fe1@intel.com/ [3]: https://lore.kernel.org/alsa-devel/20230918133940.3676091-1-cezary.rojewski@intel.com/
On 19. 09. 23 11:28, Cezary Rojewski wrote: > On 2023-09-18 5:04 PM, Jaroslav Kysela wrote: >> On 18. 09. 23 15:55, Cezary Rojewski wrote: > > ... > >>>> This is for a special case when the drivers do not set >>>> snd_pcm_hw_constraint_subformats (all current drivers). In this case, >>>> the default is to handle STD and MAX subformat bits. >>>> >>>> This constraint should be applied only one time. So this prevents to >>>> install it twice. >>> >>> I believe we could avoid special-case approach. Have a copy/intersection >>> helpers in place and utilize iterations-with-sentinel-entry. Provided >>> such in v2 of my series. >> >> I don't think that it's required to carry the format->subformat map in >> struct snd_pcm_hardware. Only few drivers will use it, so the separate >> constraint is fine. Also, you can remove a lot of your added code to the >> pcm_misc and ASoC core (copy, masking, allocating) when the affected >> drivers install the map using the constraint call. > > I believe the question isn't how few or how many, but are there users or > not. The answer to that question is: there are users of the subformat > feature. > > Adding an array of subformats to the snd_pcm_hardware makes things > explicit, example being sound/soc/intel/avs/pcm.c. That's a win from > maintenance point of view. Another thing is that we could utilize > subformat to drop msbits entirely in the future. To summarize, to make > subformat a first class citizen, we should avoid special-casing anything > related to it. I would argue that the snd_pcm_hardware is just a static template which is refined by constraints (runtime) anyway. The new constraint which is called directly in the PCM open callback means really minimal changes in the core code and ASoC core code. We can implement more robust way on demand in future when we have a better picture for the subformat mask usage. >> Few points: >> >> 1) PCM API interface changes should be separate, you mixing unused >> helpers and separating vital functionality for drivers > > What I could do is shuffle the code a bit e.g.: let snd_pcm_hw_copy() be > introduced along the ASoC changes. Frankly that was the initial approach > (forgotten to update the commit message of 04/17 so it still talks about > code that's no longer part of the commit). The first patch should cover the vital code which is required for the subformat extension in the PCM core code. When we have this base, you can work on other things. >> 2) if you copy 90% of my code, I don't think that Suggested-by: tag is fine >> >> Could you do your work on top of my patch? > > I'm afraid this isn't a fair claim. The feature is driven by validation > and this has been conducted be me or my folks entirely. Given the scarce > guidance provided in [1] I still provided a valid WIP in [2] and > expected to iterate over it given the feedback. Closing the discussion > by taking a single patch away from the series and re-authoring it is not > a welcoming way to do a review. Perhaps Co-developed-by: then? I don't follow you. My patch can be also changed - I've not heard any review except cosmetic changes. I am just telling you that the patch is a good base for all other changes. I think that the best way is to finish the base extension in sound/core at first without any helpers and so on and then work on other parts. > Please note that the code differs. I do believe that splitting the API > and the constrains into separate patches is a better approach from > maintenance point of view. It does not make sense to extend API without constraints. The splitting does not help here. > Proposed readability improvements have also > been applied in v2. For reasons provided in previous paragraphs, I chose > to avoid the chicken-bit and treat subformat constraints in generic > fashion. Also, validation shows that without updating > snd_pcm_subformat_names[] in pcm.c the code ends with UBSAN during Yes, I missed that. I can put it to my v3 when we agree on the constraints. > runtime. I've already addressed that, even in v1. > > I'm happy to continue the technical discussion as there are still points > to discuss. Let's do so in v2 of the series [3]. Unfortunately, you are not working with the technical discussion anyway. Changing comments adding empty lines, renaming variables to make you happy is not a nice way to co-operate with others and then act as the author of the CODE (not comments) is really bad. Everyone has own coding style and you're forcing your opinion. Also, I though about {} end-of-array mark (remove SNDRV_PCM_FORMAT_CONSTRAINT_END), but I found that I prefer to have the possibility to skip the MSBITS_MAX settings for the given format. It may make sense in a situation when the MSBITS configuration is too rare to be added as the API bit. Jaroslav
On 2023-09-19 12:07 PM, Jaroslav Kysela wrote: > On 19. 09. 23 11:28, Cezary Rojewski wrote: ... >>> 2) if you copy 90% of my code, I don't think that Suggested-by: tag >>> is fine >>> >>> Could you do your work on top of my patch? >> >> I'm afraid this isn't a fair claim. The feature is driven by validation >> and this has been conducted be me or my folks entirely. Given the scarce >> guidance provided in [1] I still provided a valid WIP in [2] and >> expected to iterate over it given the feedback. Closing the discussion >> by taking a single patch away from the series and re-authoring it is not >> a welcoming way to do a review. Perhaps Co-developed-by: then? > > I don't follow you. My patch can be also changed - I've not heard any > review except cosmetic changes. I am just telling you that the patch is > a good base for all other changes. I think that the best way is to > finish the base extension in sound/core at first without any helpers and > so on and then work on other parts. (save) >> Please note that the code differs. I do believe that splitting the API >> and the constrains into separate patches is a better approach from >> maintenance point of view. > > It does not make sense to extend API without constraints. The splitting > does not help here. > >> Proposed readability improvements have also >> been applied in v2. For reasons provided in previous paragraphs, I >> chose > to avoid the chicken-bit and treat subformat constraints in >> generic >> fashion. Also, validation shows that without updating >> snd_pcm_subformat_names[] in pcm.c the code ends with UBSAN during > > Yes, I missed that. I can put it to my v3 when we agree on the constraints. > >> runtime. I've already addressed that, even in v1. >> >> I'm happy to continue the technical discussion as there are still points >> to discuss. Let's do so in v2 of the series [3]. > > Unfortunately, you are not working with the technical discussion anyway. > Changing comments adding empty lines, renaming variables to make you > happy is not a nice way to co-operate with others and then act as the > author of the CODE (not comments) is really bad. Everyone has own coding > style and you're forcing your opinion. We have a different view on the subject. Readability shall be treated on the same level as performance is. It should not be marginalized. When taking a look at RFC series [1] and the WIP [2] there's a clear resemblance to the code here. I believe the differences between this and v2 [3] are also being skipped over. And as stated previously, closing the review by separating a patch from the series and sending it without any references to the original series and modifying the CC list is not nice. What I propose is: leave the MSBITS-introduction and MSBITS-constraints patches split. Have my authorship for the former and yours for the latter with relevant Co-developed-by: tags added in both of them. Is this proposal acceptable from your perspective? Kind regards, Czarek [1]: https://lore.kernel.org/alsa-devel/20230811164853.1797547-1-cezary.rojewski@intel.com/ [2]: https://lore.kernel.org/alsa-devel/8d76a1d8-e85c-b699-34a0-ecea6edc2fe1@intel.com/ [3]: https://lore.kernel.org/alsa-devel/20230918133940.3676091-1-cezary.rojewski@intel.com/ > Also, I though about {} end-of-array mark (remove > SNDRV_PCM_FORMAT_CONSTRAINT_END), but I found that I prefer to have the > possibility to skip the MSBITS_MAX settings for the given format. It may > make sense in a situation when the MSBITS configuration is too rare to > be added as the API bit.
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 2a815373dac1..59ad45b42e03 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -217,6 +217,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_MAX _SNDRV_PCM_SUBFMTBIT(MSBITS_MAX) +#define SNDRV_PCM_SUBFMTBIT_MSBITS_20 _SNDRV_PCM_SUBFMTBIT(MSBITS_20) +#define SNDRV_PCM_SUBFMTBIT_MSBITS_24 _SNDRV_PCM_SUBFMTBIT(MSBITS_24) + struct snd_pcm_file { struct snd_pcm_substream *substream; int no_compat_mmap; @@ -290,6 +296,13 @@ struct snd_pcm_hw_constraint_ranges { unsigned int mask; }; +#define SNDRV_PCM_FORMAT_CONSTRAINT_END (~0) + +struct snd_pcm_hw_constraint_subformat { + snd_pcm_format_t format; /* SNDRV_PCM_FORMAT_* */ + u32 subformats; /* SNDRV_PCM_SUBFMTBIT_* */ +}; + /* * userspace-provided audio timestamp config to kernel, * structure is for internal use only and filled with dedicated unpack routine @@ -375,6 +388,7 @@ struct snd_pcm_runtime { unsigned int rate_num; unsigned int rate_den; unsigned int no_period_wakeup: 1; + unsigned int subformat_constraint: 1; /* -- SW params; see struct snd_pcm_sw_params for comments -- */ int tstamp_mode; @@ -1068,6 +1082,9 @@ int snd_pcm_hw_constraint_ratdens(struct snd_pcm_runtime *runtime, unsigned int cond, snd_pcm_hw_param_t var, const struct snd_pcm_hw_constraint_ratdens *r); +int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime, + unsigned int cond, + struct snd_pcm_hw_constraint_subformat *subformats); int snd_pcm_hw_constraint_msbits(struct snd_pcm_runtime *runtime, unsigned int cond, unsigned int width, diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h index f9939da41122..d5b9cfbd9cea 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, 15) +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 16) typedef unsigned long snd_pcm_uframes_t; typedef signed long snd_pcm_sframes_t; @@ -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_MAX ((__force snd_pcm_subformat_t) 1) +#define SNDRV_PCM_SUBFORMAT_MSBITS_20 ((__force snd_pcm_subformat_t) 2) +#define SNDRV_PCM_SUBFORMAT_MSBITS_24 ((__force snd_pcm_subformat_t) 3) +#define SNDRV_PCM_SUBFORMAT_LAST SNDRV_PCM_SUBFORMAT_MSBITS_24 #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 a11cd7d6295f..f414f8fd217b 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -1404,6 +1404,65 @@ int snd_pcm_hw_constraint_ratdens(struct snd_pcm_runtime *runtime, } EXPORT_SYMBOL(snd_pcm_hw_constraint_ratdens); +static int snd_pcm_hw_rule_subformats(struct snd_pcm_hw_params *params, + struct snd_pcm_hw_rule *rule) +{ + const struct snd_pcm_hw_constraint_subformat *sf; + snd_pcm_format_t k; + 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); + 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; + found = false; + for (sf = rule->private; sf && sf->format != SNDRV_PCM_FORMAT_CONSTRAINT_END; sf++) { + if (sf->format != k) + continue; + found = true; + m.bits[0] |= sf->subformats; + break; + } + if (!found && snd_pcm_format_linear(k)) + snd_mask_set(&m, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX); + } + return snd_mask_refine(mask, &m); +} + +/** + * snd_pcm_hw_constraint_subformats - add a hw constraint subformats rule + * @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 + * does not set this constraint, only STD and MAX subformats are handled. + * + * Return: Zero if successful, or a negative error code on failure. + */ +int snd_pcm_hw_constraint_subformats(struct snd_pcm_runtime *runtime, + unsigned int cond, + struct snd_pcm_hw_constraint_subformat *subformats) +{ + int ret; + + ret = snd_pcm_hw_rule_add(runtime, cond, -1, + snd_pcm_hw_rule_subformats, + (void*) subformats, + SNDRV_PCM_HW_PARAM_SUBFORMAT, + SNDRV_PCM_HW_PARAM_FORMAT, -1); + if (ret < 0) + return ret; + runtime->subformat_constraint = 1; + return 0; +} +EXPORT_SYMBOL(snd_pcm_hw_constraint_subformats); + static int snd_pcm_hw_rule_msbits(struct snd_pcm_hw_params *params, struct snd_pcm_hw_rule *rule) { diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index bd9ddf412b46..69609e6aa507 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -479,6 +479,7 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream, { const struct snd_interval *i; const struct snd_mask *m; + struct snd_mask *m_rw; int err; if (!params->msbits) { @@ -487,6 +488,14 @@ static int fixup_unreferenced_params(struct snd_pcm_substream *substream, params->msbits = snd_interval_value(i); } + if (params->msbits) { + m = hw_param_mask_c(params, SNDRV_PCM_HW_PARAM_FORMAT); + if (snd_mask_single(m) && snd_pcm_format_width(snd_mask_min(m)) != params->msbits) { + m_rw = hw_param_mask(params, SNDRV_PCM_HW_PARAM_SUBFORMAT); + snd_mask_reset(m_rw, (__force unsigned)SNDRV_PCM_SUBFORMAT_MSBITS_MAX); + } + } + if (!params->rate_den) { i = hw_param_interval_c(params, SNDRV_PCM_HW_PARAM_RATE); if (snd_interval_single(i)) { @@ -2634,10 +2643,11 @@ static int snd_pcm_hw_constraints_complete(struct snd_pcm_substream *substream) if (err < 0) return err; - err = snd_pcm_hw_constraint_mask(runtime, SNDRV_PCM_HW_PARAM_SUBFORMAT, - PARAM_MASK_BIT(SNDRV_PCM_SUBFORMAT_STD)); - if (err < 0) - return err; + if (!runtime->subformat_constraint) { + err = snd_pcm_hw_constraint_subformats(runtime, 0, NULL); + if (err < 0) + return err; + } err = snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_CHANNELS, hw->channels_min, hw->channels_max); diff --git a/tools/include/uapi/sound/asound.h b/tools/include/uapi/sound/asound.h index f9939da41122..d5b9cfbd9cea 100644 --- a/tools/include/uapi/sound/asound.h +++ b/tools/include/uapi/sound/asound.h @@ -142,7 +142,7 @@ struct snd_hwdep_dsp_image { * * *****************************************************************************/ -#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 15) +#define SNDRV_PCM_VERSION SNDRV_PROTOCOL_VERSION(2, 0, 16) typedef unsigned long snd_pcm_uframes_t; typedef signed long snd_pcm_sframes_t; @@ -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_MAX ((__force snd_pcm_subformat_t) 1) +#define SNDRV_PCM_SUBFORMAT_MSBITS_20 ((__force snd_pcm_subformat_t) 2) +#define SNDRV_PCM_SUBFORMAT_MSBITS_24 ((__force snd_pcm_subformat_t) 3) +#define SNDRV_PCM_SUBFORMAT_LAST SNDRV_PCM_SUBFORMAT_MSBITS_24 #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 linear formats by adding constants representing MAX, 20, 24 most significant bits. The MAX means the maximum number of significant bits which can the physical format hold. For 32-bit formats, MAX is related to 32 bits. For 8-bit formats, MAX is related to 8 bits etc. The drivers may use snd_pcm_hw_constraint_subformats with a simple format -> subformats table. Cc: Cezary Rojewski <cezary.rojewski@intel.com> Signed-off-by: Jaroslav Kysela <perex@perex.cz> --- include/sound/pcm.h | 17 +++++++++ include/uapi/sound/asound.h | 7 ++-- sound/core/pcm_lib.c | 59 +++++++++++++++++++++++++++++++ sound/core/pcm_native.c | 18 +++++++--- tools/include/uapi/sound/asound.h | 7 ++-- 5 files changed, 100 insertions(+), 8 deletions(-)