Re: [RFC PATCH] ALSA: pcm: Introduce MSBITS subformat API extension

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

--
Jaroslav Kysela <perex@xxxxxxxx>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux