| Message ID | 20210323114327.3969072-1-codrin.ciubotariu@microchip.com |
|---|---|
| Headers | show |
| Series | Separate BE DAI HW constraints from FE ones | expand |
Dne 23. 03. 21 v 12:43 Codrin Ciubotariu napsal(a): > To achieve this, the first thing needed is to detect whether a HW > constraint rule is enforced by a FE or a BE DAI. This means that > snd_pcm_hw_rule_add() needs to be able to differentiate between the two > type of DAIs. For this, the runtime pointer to struct snd_pcm_runtime is > replaced with a pointer to struct snd_pcm_substream, to be able to reach > substream->pcm->internal to differentiate between FE and BE DAIs. Think about other not-so-invasive solution. What about to use 'runtime->private_data' (struct snd_soc_pcm_runtime *) to determine FE / BE? Jaroslav
On 23.03.2021 14:15, Jaroslav Kysela wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Dne 23. 03. 21 v 12:43 Codrin Ciubotariu napsal(a): > >> To achieve this, the first thing needed is to detect whether a HW >> constraint rule is enforced by a FE or a BE DAI. This means that >> snd_pcm_hw_rule_add() needs to be able to differentiate between the two >> type of DAIs. For this, the runtime pointer to struct snd_pcm_runtime is >> replaced with a pointer to struct snd_pcm_substream, to be able to reach >> substream->pcm->internal to differentiate between FE and BE DAIs. > > Think about other not-so-invasive solution. What about to use > 'runtime->private_data' (struct snd_soc_pcm_runtime *) to determine FE / BE? > I think struct snd_soc_pcm_runtime * is placed in substream->private_data, while runtime->private_data is used more by the platform drivers. runtime->trigger_master could be an idea, but it looks like it's initialized just before the trigger callback is called, way after the constraint rules are added and I am not sure it can be initialized earlier... Best regards, Codrin
On 24.03.2021 17:28, Pierre-Louis Bossart wrote: >> I am using hw_params_fixup, but it's not enough. The first thing I do is >> to not add the BE HW constraint rules in runtime->hw_constraints, >> because this will potentially affect the FE HW params. If the FE does >> sampling rate conversion, for example, applying the sampling rate >> constrain rules from a BE codec on FE is useless. The second thing I do >> is to apply these BE HW constraint rules to the BE HW params. It's true >> that the BE HW params can be fine tuned via hw_params_fixup (topology, >> device-tree or even static parameters) and set in such a way that avoid >> the BE HW constraints, so we could ignore the constraint rules added by >> their drivers. It's not every elegant and running the BE HW constraint >> rules for the fixup BE HW params can be a sanity check. Also, I am >> thinking that if the FE does no conversion (be_hw_params_fixup missing) >> and more than one BEs are available, applying the HW constraint rules on >> the same set of BE HW params could rule out the incompatible BE DAI >> links and start the compatible ones only. I am not sure this is a real >> usecase. > > Even after a second cup of coffee I was not able to follow why the > hw_params_fixup was not enough? That paragraph is rather dense. Right, sorry about that. :) > > And to be frank I don't fully understand the problem statement above: > "separate the FE HW constraints from the BE HW constraints". We have > existing solutions with a DSP-based SRC adjusting FE rates to what is > required by the BE dailink. > > Maybe it would help to show examples of what you can do today and what > you cannot, so that we are on the same wavelength on what the > limitations are and how to remove them? For example, ad1934 driver sets a constraint to always have 32 sample bits [1] and this rule will be added in struct snd_pcm_runtime -> hw_constraints [2]. As you can see, this is added early and always. So this rule will affect the HW parameters used from the user-space [3] and, in our example, only audio streams that have formats of 4B containers will be used (S24_LE, S32_LE, etc). For playback, if the audio stream won't have this format, the stream will need to be converted in software, using plug ALSA plugin for example. This is OK for normal PCM: HW params user <----------> CPU <---> AD1934 space ^ DAI | | | -------------------------| sample bits constraint rule (32b) For DPCM, we have the same behavior (unfortunately). ad1934, as a BE codec, will add it's rule in the same place, which means that it will again affect the HW parameters set by user-space. So user-space will have to convert all audio streams to have a 32b sample size. If the FE is capable of format conversing, this software conversion is useless. FE HW params BE HW params user <----------> FE <--------------> BE CPU <----> BE AD1934 space ^ DAI DAI | | | --------------------------------------------------| sample bits constraint rule (32b) The format conversion will be done in software instead of hardware (FE). I hope I was able to be more clear now. Thanks for taking time to understand this. I owe you a coffee. :) Best regards, Codrin [1] https://elixir.bootlin.com/linux/v5.12-rc4/source/sound/soc/codecs/ad193x.c#L366 [2] https://elixir.bootlin.com/linux/v5.12-rc4/source/sound/core/pcm_lib.c#L1141 [3] https://elixir.bootlin.com/linux/v5.12-rc4/source/sound/core/pcm_native.c#L692
On 23.03.2021 16:18, Codrin.Ciubotariu@microchip.com wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 23.03.2021 14:15, Jaroslav Kysela wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> Dne 23. 03. 21 v 12:43 Codrin Ciubotariu napsal(a): >> >>> To achieve this, the first thing needed is to detect whether a HW >>> constraint rule is enforced by a FE or a BE DAI. This means that >>> snd_pcm_hw_rule_add() needs to be able to differentiate between the two >>> type of DAIs. For this, the runtime pointer to struct snd_pcm_runtime is >>> replaced with a pointer to struct snd_pcm_substream, to be able to reach >>> substream->pcm->internal to differentiate between FE and BE DAIs. >> >> Think about other not-so-invasive solution. What about to use >> 'runtime->private_data' (struct snd_soc_pcm_runtime *) to determine FE / BE? >> > > I think struct snd_soc_pcm_runtime * is placed in > substream->private_data, while runtime->private_data is used more by the > platform drivers. runtime->trigger_master could be an idea, but it looks > like it's initialized just before the trigger callback is called, way > after the constraint rules are added and I am not sure it can be > initialized earlier... > > Best regards, > Codrin > How about using a different API for ASoC only, since that's the place of DPCM. Only drivers that do not involve DSPs would have to to be changed to call the new snd_pcm_hw_rule_add() variant. Another solution would be to have a different snd_soc_pcm_runtime for BE interfaces (with a new hw_constraints member of course). What do you think? Thanks! Codrin
On Wed, Apr 14, 2021 at 02:58:10PM +0000, Codrin.Ciubotariu@microchip.com wrote: > How about using a different API for ASoC only, since that's the place of > DPCM. Only drivers that do not involve DSPs would have to to be changed > to call the new snd_pcm_hw_rule_add() variant. > Another solution would be to have a different snd_soc_pcm_runtime for BE > interfaces (with a new hw_constraints member of course). What do you think? I'm really not convinced we want to continue to pile stuff on top of DPCM, it is just fundamentally not up to modelling what modern systems are able to do - it's already making things more fragile than they should be and more special cases seems like it's going to end up making that worse. That said I've not seen the code but...
On 15.04.2021 19:17, Mark Brown wrote: > On Wed, Apr 14, 2021 at 02:58:10PM +0000, Codrin.Ciubotariu@microchip.com wrote: > >> How about using a different API for ASoC only, since that's the place of >> DPCM. Only drivers that do not involve DSPs would have to to be changed >> to call the new snd_pcm_hw_rule_add() variant. >> Another solution would be to have a different snd_soc_pcm_runtime for BE >> interfaces (with a new hw_constraints member of course). What do you think? > > I'm really not convinced we want to continue to pile stuff on top of > DPCM, it is just fundamentally not up to modelling what modern systems > are able to do - it's already making things more fragile than they > should be and more special cases seems like it's going to end up making > that worse. That said I've not seen the code but... > Are there any plans for refactoring DPCM? any ideas ongoing? I also have some changes for PCM dmaengine, in the same 'style', similar to what I sent some time ago... I can adjust to different ideas, if there are any, but, for a start, can anyone confirm that the problem I am trying to fix is real? Best regards, Codrin
On Thu, Apr 15, 2021 at 04:56:00PM +0000, Codrin.Ciubotariu@microchip.com wrote: > Are there any plans for refactoring DPCM? any ideas ongoing? I also have > some changes for PCM dmaengine, in the same 'style', similar to what I > sent some time ago... > I can adjust to different ideas, if there are any, but, for a start, can > anyone confirm that the problem I am trying to fix is real? Lars-Peter's presentation from ELC in 2016 (!) is probably the clearest summary of the ideas: https://elinux.org/images/e/e7/Audio_on_Linux.pdf https://youtu.be/6oQF2TzCYtQ Essentially the idea is to represent everything, including the DSPs, as ASoC components and be able to represent the digital links between components in the DAPM graph in the same way we currently represent analogue links. This means we need a way to propagate digital configuration along the DAPM graph (or a parallel, cross linked digital one). Sadly I'm not really aware of anyone actively working on the actual conversion at the minute, Morimoto-san has done a lot of great preparatory work to make everything a component which makes the whole thing more tractable.
On 16.04.2021 19:31, Mark Brown wrote: > On Fri, Apr 16, 2021 at 04:03:05PM +0000, Codrin.Ciubotariu@microchip.com wrote: > >> Thank you for the links! So basically the machine driver disappears and >> all the components will be visible in user-space. > > Not entirely - you still need something to say how they're wired > together but it'll be a *lot* simpler for anything that currently used > DPCM. Right, device-tree/acpi wiring is not enough... > >> If there is a list with the 'steps' or tasks to achieve this? I can try >> to pitch in. > > Not really written down that I can think of. I think the next steps > that I can think of right now are unfortunately bigger and harder ones, > mainly working out a way to represent digital configuration as a graph > that can be attached to/run in parallel with DAPM other people might > have some better ideas though. Sorry, I appreciate that this isn't > super helpful :/ > I think I have good picture, a more robust card->dai_link, not with just FEs and BEs ... I will try to monitor the alsa list more, see what other people are working on. Thank you for your time! Best regards, Codrin
On 4/16/21 1:55 PM, Mark Brown wrote: > On Fri, Apr 16, 2021 at 11:47:01AM -0500, Pierre-Louis Bossart wrote: >> On 4/16/21 11:31 AM, Mark Brown wrote: > >>> Not really written down that I can think of. I think the next steps >>> that I can think of right now are unfortunately bigger and harder ones, >>> mainly working out a way to represent digital configuration as a graph >>> that can be attached to/run in parallel with DAPM other people might >>> have some better ideas though. Sorry, I appreciate that this isn't >>> super helpful :/ > >> I see a need for this in our future SoundWire/SDCA work. So far I was >> planning to model the entities as 'widgets' and lets DAPM propagate >> activation information for power management, however there are also bits of >> information in 'Clusters' (number of channels and spatial relationships) >> that could change dynamically and would be interesting to propagate across >> entities, so that when we get a stream of data on the bus we know what it >> is. > > Yes, I was thinking along similar lines last time I looked at it - I was > using the term digital domains. You'd need some impedence matching > objects for things like SRCs, and the ability to flag which > configuration matters within a domain (eg, a lot of things will covert > to the maximum supported bit width for internal operation so bit width > only matters on external interfaces) but I think for a first pass we can > get away with forcing everything other than what DPCM has as front ends > into static configurations. You lost me on the last sentence. did you mean "forcing everything into static configurations except for what DPCM has as front-ends"? It may already be too late for static configurations, Intel, NXP and others have started to enable cases where the dailink configuration varies. FWIW both the USB and SDCA class document are very careful with the notion of constraints and whether an entity is implemented in the analog or digital domains. There are 'clock sources' that may be used in specific terminals but no notion of explicit SRC in the graph to leave implementers a lot of freedom. There is a notion of 'Usage' that describes e.g. FullBand or WideBand without defining what the representation is. The bit width is also not described except where necessary (history buffers and external bus-facing interfaces). Like you said it's mostly the boundaries of the domains that matter.
On Fri, Apr 16, 2021 at 02:39:25PM -0500, Pierre-Louis Bossart wrote: > On 4/16/21 1:55 PM, Mark Brown wrote: > > to the maximum supported bit width for internal operation so bit width > > only matters on external interfaces) but I think for a first pass we can > > get away with forcing everything other than what DPCM has as front ends > > into static configurations. > You lost me on the last sentence. did you mean "forcing everything into > static configurations except for what DPCM has as front-ends"? Yes... > It may already be too late for static configurations, Intel, NXP and others > have started to enable cases where the dailink configuration varies. Well, they won't be able to use any new stuff until someone implements support for dynamic configurations in the new stuff.