| Message ID | 20211208111301.1817725-1-cezary.rojewski@intel.com |
|---|---|
| Headers | show |
| Series | ASoC: Intel: AVS - Audio DSP for cAVS | expand |
On Wed, Dec 08, 2021 at 12:12:24PM +0100, Cezary Rojewski wrote: > A continuation of cleanup work of Intel SST solutions found in > sound/soc/intel/. With two major chapters released last year catpt [1] > and removal of haswell solution [2], time has come for Skylake-driver. ... > Note: this series does not add fully functional driver as its size would > get out of control. Focus is put on adding new code. A So, I've spent some time looking at this but I think there's just too much in this patch set for me to get through in a timely fashion even with the efforts you've noted above in that direction and that the best thing to do is to look at how to make things a bit more managable. It's a big series and the time of year does mean time for review is a bit more limited. From that point of view I think the big thing to do is to reduce the amount of interesting or new things that are being done and make the series a simple as possible. That'd be a limited but hopefully routine driver which should be much easier to review and would allow the more interesting bits to be focused on separately without getting lost in the bulk of code that's more routine. This applies more to bits at the top of the stack that interface with the framework than DSP/hardware facing bits (eg, stuff like the tracing is not really getting in the way). Tactically the code is basically fine, there's going to be some issues but really it's the big picture stuff that needs more consideration. In terms of things that could be split out there's a couple of big things that jump out. One is the paths code which feels like something that should perhaps be pulled up a level to the framework since it feels like the problems that it is addressing are general problems that all DSPs face. Doing something like hard coding this to some very simple use case that does minimal to no processing would allow the driver to load and function, then the path code can get a proper review separately. The other thing is the instantiating of multiple machine drivers on a single system. That's something I've seen occasionally from other vendors and I do have concerns about how use cases where someone wants to route audio in ways that result in cross links between cards so those ended up being integrated. The question here isn't really if it works in testing (no matter how thorough that testing is), the question is if userspace software doing generic things will be confused by it and if some combination of future framework changes and user creativity can turn up issues. There's also the issue of how quirk handling would work in this setup, and the issue with needing another set of machine drivers. It's one point where input from users and distros would be especially good. This would be harder to cut out for later since there's not so much code which supports it directly (TBH this is part of the concern), one thing might be to just only support a subset of hardware (eg, HDA only or I2S only) such that only one machine driver can ever be instantiated on a system. One more tactical thing is that I did comment on earlier was the use of atomics - in general atomics are error prone and hard to reason about, unless you're doing something like transferring the audio data using PIO it's probably better to use higher level concurrency primitives. Any performance difference is unlikely to register and the maintainability is a lot better. It'd also be good to get this well enough integrated with the intel-dsp-config code to avoid the need for the dependency on SND_SOC_INTEL_SKYLAKE_FAMILY=n. If both are built then it could start off with always require a command line override to select the new driver with a _DSP_DRIVER_AVS constant, this can be revisited later. That mechanism is really nice for distros and users since it allows people to do binary distributions without having to worry about committing to one driver or another, reducing the risks for things like breakage on upgrade for some small subset of machines.
On 2021-12-24 2:06 PM, Mark Brown wrote: > On Wed, Dec 08, 2021 at 12:12:24PM +0100, Cezary Rojewski wrote: > >> A continuation of cleanup work of Intel SST solutions found in >> sound/soc/intel/. With two major chapters released last year catpt [1] >> and removal of haswell solution [2], time has come for Skylake-driver. > > ... > >> Note: this series does not add fully functional driver as its size would >> get out of control. Focus is put on adding new code. A > > So, I've spent some time looking at this but I think there's just > too much in this patch set for me to get through in a timely > fashion even with the efforts you've noted above in that > direction and that the best thing to do is to look at how to make > things a bit more managable. It's a big series and the time of > year does mean time for review is a bit more limited. From that > point of view I think the big thing to do is to reduce the amount > of interesting or new things that are being done and make the > series a simple as possible. That'd be a limited but hopefully > routine driver which should be much easier to review and would > allow the more interesting bits to be focused on separately > without getting lost in the bulk of code that's more routine. > This applies more to bits at the top of the stack that interface > with the framework than DSP/hardware facing bits (eg, stuff like > the tracing is not really getting in the way). Tactically the > code is basically fine, there's going to be some issues but > really it's the big picture stuff that needs more consideration. Your comments and review is much appreciated. While we did separate the series into chunks, I'm keen to agree we could have moved a little bit further with the separation. Below you'll find the list of patches and my thoughts after taking your feedback into consideration. There's also a TLDR if there's not enough coffee in the pot to cover the summary. 1/37 ALSA: hda: Add snd_hdac_ext_bus_link_at() helper 2/37 ALSA: hda: Update and expose snd_hda_codec_device_init() 3/37 ALSA: hda: Update and expose codec register procedures 4/37 ALSA: hda: Expose codec cleanup and power-save functions 6/37 ASoC: Export DAI register and widget ctor and dctor functions As current RFC allows one to see the reasoning behind adding these five patches, I believe they could be sent as a separate series. A cover letter for that series would mention their purpose nonetheless of course. Note: patch 6/37 has been re-ordered with 5/37 as 6th patch fits the generic-theme whereas 5th I believe does not. 5/37 ALSA: hda: Add helper macros for DSP capable devices While this patch _could_ be merged with above, it's not as generic and the other five. It seems more reasonable to leave it with the avs-core series as its specific dependency. 7/37 ASoC: Intel: Introduce AVS driver 8/37 ASoC: Intel: avs: Inter process communication 9/37 ASoC: Intel: avs: Add code loading requests 10/37 ASoC: Intel: avs: Add pipeline management requests 11/37 ASoC: Intel: avs: Add module management requests 12/37 ASoC: Intel: avs: Add power management requests 13/37 ASoC: Intel: avs: Add ROM requests 14/37 ASoC: Intel: avs: Add basefw runtime-parameter requests If one were to specify the pillars of a DSP driver (for simplicity sake, let's discard all the standard driver needs which are provided or satisfied by kernel's interfaces and resources anyway), firmware (IPC) communication and the topology (stream layout) are the two major ones. Pillar #1, base firmware (IPC) communication is complete at this point. 15/37 ASoC: Intel: avs: Firmware resources management utilities 16/37 ASoC: Intel: avs: Declare module configuration types 17/37 ASoC: Intel: avs: Dynamic firmware resources management Prerequisites for below, define all the look ups and boundaries for the runtime operations. 18/37 ASoC: Intel: avs: Topology parsing Pillar #2, base topology (stream layout) is complete at this point. 19/37 ASoC: Intel: avs: Path management Streaming runtime i.e. reflect data provided from topology file - a recipe for a stream - on DSP side. 20/37 ASoC: Intel: avs: Conditional-path support Extension of standard path management. Could be separated from avs-core. 21/37 ASoC: Intel: avs: General code loading flow 22/37 ASoC: Intel: avs: Implement CLDMA transfer 23/37 ASoC: Intel: avs: Code loading over CLDMA 24/37 ASoC: Intel: avs: Code loading over HDA All of them are avs-core. SKL-based and APL-based platforms differ in code-loading (base firmware, dynamically loaded libraries) thus the two methods. These could be moved *before* topology/path related patches with a consequence: code loading is dependent on some of the bits provided by the topology/path implementations so additional changes (a patch perhaps) would be required as a preparation step for these four. 25/37 ASoC: Intel: avs: Generic soc component driver 26/37 ASoC: Intel: avs: Generic PCM FE operations 27/37 ASoC: Intel: avs: non-HDA PCM BE operations 28/37 ASoC: Intel: avs: HDA PCM BE operations At this point PCM operations are complete. FE is _generic_ regardless of interface (BE) type it's dealing with. HDA BE is covered by the last of these whereas I2S/DMIC by the second to last. I'm unsure about PCM operations being separated from the avs-core. My current opinion: leave as is. 29/37 ASoC: Intel: avs: Coredump and recovery flow 30/37 ASoC: Intel: avs: Prepare for firmware tracing 31/37 ASoC: Intel: avs: D0ix power state support 32/37 ASoC: Intel: avs: Event tracing 33/37 ASoC: Intel: avs: Machine board registration All of these could be moved into the separate series with the exact same consequence as with code-loading: a preparation step would be required as mixing code addition with 'making room code' would cloud the view. If we're strict and focused on patch separation then while very important features are added here, these are not avs-core per se. 34/37 ASoC: Intel: avs: PCI driver implementation 35/37 ASoC: Intel: avs: Power management Here, the question is: how bare can the base (pci) driver be in the initial avs-core series? 36/37 ASoC: Intel: avs: SKL-based platforms support 37/37 ASoC: Intel: avs: APL-based platforms support These two are very easy to separate from the avs-core as these are the last in the series. No problems or consequences here. TLDR: Separate series #1: 1/37 ALSA: hda: Add snd_hdac_ext_bus_link_at() helper 2/37 ALSA: hda: Update and expose snd_hda_codec_device_init() 3/37 ALSA: hda: Update and expose codec register procedures 4/37 ALSA: hda: Expose codec cleanup and power-save functions 6/37 ASoC: Export DAI register and widget ctor and dctor functions Separate series #2: <everything else not listed here> Note: patches 21-24/37 get reordered to prepend topology and path management (currently, patches 18/37 and 19/37 respectively). While right now I don't see a reason for doing so, this also provides a possibility for separation or division of these last two mentioned patches if need be. Separate series #3: 20/37 ASoC: Intel: avs: Conditional-path support 29/37 ASoC: Intel: avs: Coredump and recovery flow 30/37 ASoC: Intel: avs: Prepare for firmware tracing 31/37 ASoC: Intel: avs: D0ix power state support 32/37 ASoC: Intel: avs: Event tracing 33/37 ASoC: Intel: avs: Machine board registration 36/37 ASoC: Intel: avs: SKL-based platforms support 37/37 ASoC: Intel: avs: APL-based platforms support The last three could be separated too as all of them touch on isolated subject: recognize ID: XXX to support YYY. > In terms of things that could be split out there's a couple of > big things that jump out. > > One is the paths code which feels like something that should > perhaps be pulled up a level to the framework since it feels like > the problems that it is addressing are general problems that all > DSPs face. Doing something like hard coding this to some very > simple use case that does minimal to no processing would allow > the driver to load and function, then the path code can get a > proper review separately. Must admit, right now I'm not seeing what could be added from avs-path into the framework. Not saying 'no', just after seeing the avs_path stripped from all the cAVS firmware specifics there's basically nothing left. Let's take a look at the standard path (discarding all the conditional path bits): struct avs_path { u32 dma_id; struct list_head ppl_list; u32 state; struct avs_tplg_path *template; struct avs_dev *owner; /* device path management */ struct list_head node; }; 'dma_id' and 'template' are avs-driver specific. To be honest, stream division into pipelines and modules as done in cAVS firmware is also specific and a different DSP or a different firmware may expect things to be laid out differently, so 'ppl_list' is yet another candidate for not being framework friendly. Let's also take a look at the interface: a) struct avs_path *avs_path_create(struct avs_dev *adev, u32 dma_id, struct avs_tplg_path_template *template, struct snd_pcm_hw_params *fe_params, struct snd_pcm_hw_params *be_params); Compound step, generally speaking this maps to IPCs: CREATE_PIPELINE(s) + INIT_INSTANCE(s) void avs_path_free(struct avs_path *path); Compound step, generally speaking this maps to IPC: DELETE_PIPELINE(s) b) int avs_path_bind(struct avs_path *path); int avs_path_unbind(struct avs_path *path); Arm/disarm steps, map to IPCs: BIND/UNBIND respectively. c) int avs_path_reset(struct avs_path *path); int avs_path_pause(struct avs_path *path); int avs_path_run(struct avs_path *path, int trigger); To easily modify state of all the pipelines that are part of the given stream. Other DSP may expose more or less pipeline states, or may not expose any at all. Again, pipeline representation as seen in cAVS firmware may also not exist. These steps map to IPC: SET_PIPELINE_STATE. TLDR: avs_path is basically a wrapper for a list of pipelines which shape given stream - from ASoC side, that's a FE <-> BE relation. These pipelines exist only on the DSP side and are tied to cAVS firmware expectations and architecture. Again, if one strips the avs_path interface from cAVS IPC logic, then there's basically nothing left. We could have dropped the 'avs_path' and instead inline all the pipeline-looping but that makes all the PCM handling rather unreadable and much harder to maintain. > The other thing is the instantiating of multiple machine drivers > on a single system. That's something I've seen occasionally from > other vendors and I do have concerns about how use cases where > someone wants to route audio in ways that result in cross links > between cards so those ended up being integrated. The question > here isn't really if it works in testing (no matter how thorough > that testing is), the question is if userspace software doing > generic things will be confused by it and if some combination of > future framework changes and user creativity can turn up issues. > There's also the issue of how quirk handling would work in this > setup, and the issue with needing another set of machine drivers. > It's one point where input from users and distros would be > especially good. This would be harder to cut out for later since > there's not so much code which supports it directly (TBH this is > part of the concern), one thing might be to just only support a > subset of hardware (eg, HDA only or I2S only) such that only one > machine driver can ever be instantiated on a system. We're open for more input from the users and distros. That does not mean we did not do our homework before moving to the coding part. In our research it turned out that 'different device equals different card' is a popular and easy to follow notion. These results are of course influenced by the other OSes where such separation is more common and users got used to such model. It's worth noting that we did make use of the APIs that are already available in ASoC. There are no hacks or hooks here, just the usage of the available interfaces. The granular-cards approach, while preferred, also does not prevent super-cards from being integrated with avs-driver. In fact for some more specific scenarios e.g.: when there's no codec driver at all (as the codec is being managed externally), we do make use of such cards. In the HDA vs I2S case, the selection is done based on the existence of codecs on the HDA-bus or their ACPI IDs: if codec XXX is configured as HDA, then its ACPI ID won't be found. Only the enumeration on HDA-bus would happen - creating hda-related machine board in the process. If the opposite is true (configured as I2S) then HDA codec enumeration won't find our codec - the ACPI ID would pop up instead causing the I2S-related machine board to be created. By default, all the cards are independent of each other. avs-driver supports 'cross linking' by the means of the conditional path. The 'conditional' is a key word here. These paths are a 'side effect' of other paths being open simultaneously. If there requirements are not met e.g.: a FE is not running as it simply can't be - some specific card exposing it is not present - then the 'side effect' path would not get instantiated on DSP side at all. Conditional paths are not launched by users performing some aplay or arecord (or any other app) operation directly. The requirements i.e. the FEs/BEs required to be running simultaneously are specified by the topology. In regard to quirk handling, could you elaborate? Right now all the supported cross linking and the machine board division scenarios are not causing any repercussions as it seems avs-driver gets credit for. I understand that it's good to think about far reaching consequences sooner than later, but the APIs allowing for the granular-card approach are here for a very long time and the card/device division has been seen in practice already. > One more tactical thing is that I did comment on earlier was the > use of atomics - in general atomics are error prone and hard to > reason about, unless you're doing something like transferring the > audio data using PIO it's probably better to use higher level > concurrency primitives. Any performance difference is unlikely > to register and the maintainability is a lot better. Agreed and ack. One again, that's for spotting the problem out! > It'd also be good to get this well enough integrated with the > intel-dsp-config code to avoid the need for the dependency on > SND_SOC_INTEL_SKYLAKE_FAMILY=n. If both are built then it could > start off with always require a command line override to select > the new driver with a _DSP_DRIVER_AVS constant, this can be > revisited later. That mechanism is really nice for distros and > users since it allows people to do binary distributions without > having to worry about committing to one driver or another, > reducing the risks for things like breakage on upgrade for some > small subset of machines. Hmm.. this means that in time (once skylake-driver is removed) two values would translate to avs-driver selection rather than one. Value '2' is being used for skylake-driver and we don't want to force users to manually change it to anything else (i.e. to the to be added avs-driver selection value) when the time comes. Not against, just stating the consequence. Regards, Czarek
On Tue, Jan 18, 2022 at 10:42:08AM +0100, Cezary Rojewski wrote: > On 2022-01-06 2:39 PM, Cezary Rojewski wrote: > > Your comments and review is much appreciated. While we did separate the > > series into chunks, I'm keen to agree we could have moved a little bit > > further with the separation. Below you'll find the list of patches and > > my thoughts after taking your feedback into consideration. There's also > > a TLDR if there's not enough coffee in the pot to cover the summary. > Is the proposal described in my previous message acceptable on your end > Mark? Your mail was very long and I've not yet had a chance to go through it properly yet - I was on holiday last week, and just after the merge window is quite a busy time.
On Thu, Jan 06, 2022 at 02:39:56PM +0100, Cezary Rojewski wrote: > On 2021-12-24 2:06 PM, Mark Brown wrote: > > On Wed, Dec 08, 2021 at 12:12:24PM +0100, Cezary Rojewski wrote: > 1/37 ALSA: hda: Add snd_hdac_ext_bus_link_at() helper > 2/37 ALSA: hda: Update and expose snd_hda_codec_device_init() > 3/37 ALSA: hda: Update and expose codec register procedures > 4/37 ALSA: hda: Expose codec cleanup and power-save functions > 6/37 ASoC: Export DAI register and widget ctor and dctor functions > > As current RFC allows one to see the reasoning behind adding these five > patches, I believe they could be sent as a separate series. A cover letter > for that series would mention their purpose nonetheless of course. > Note: patch 6/37 has been re-ordered with 5/37 as 6th patch fits the > generic-theme whereas 5th I believe does not. The first 5 also need review from Takashi more than me. > Note: patches 21-24/37 get reordered to prepend topology and path > management (currently, patches 18/37 and 19/37 respectively). While right > now I don't see a reason for doing so, this also provides a possibility for > separation or division of these last two mentioned patches if need be. Part of the question is if path management is even something we want at the driver level or if it should be done at more of a framework level. Something that splits out any externally visible effect of that for separate consideration would help a lot with reducing the cognative load here. The issue isn't just the sheer size, it's also that it's not just a routine driver. > > One is the paths code which feels like something that should > > perhaps be pulled up a level to the framework since it feels like > > the problems that it is addressing are general problems that all > > DSPs face. Doing something like hard coding this to some very > > simple use case that does minimal to no processing would allow > > the driver to load and function, then the path code can get a > > proper review separately. > Must admit, right now I'm not seeing what could be added from avs-path into > the framework. Not saying 'no', just after seeing the avs_path stripped from > all the cAVS firmware specifics there's basically nothing left. AIUI the firmware itself has a bunch of DSP modules that can be dynamically instantiated and what the path stuff is doing is providing fixed sets of instantiations that can be switched between. It seems like it should be possible to pull out the bit where we have sets of modules we can instantiate from the mechanics of knowing what modules are there and actually setting them up and tearing them down, other DSP implementations would probably be able to benefit from that (at least the larger ones) and I imagine more advanced users would find it useful to be able to reconfigure the DSP pipelines separately from getting firmware releases. > Let's take a look at the standard path (discarding all the conditional path > bits): > struct avs_path { > u32 dma_id; > struct list_head ppl_list; > u32 state; > > struct avs_tplg_path *template; > struct avs_dev *owner; > /* device path management */ > struct list_head node; > }; > 'dma_id' and 'template' are avs-driver specific. To be honest, stream > division into pipelines and modules as done in cAVS firmware is also > specific and a different DSP or a different firmware may expect things to be > laid out differently, so 'ppl_list' is yet another candidate for not being > framework friendly. I suspect that at least the template could be pulled apart, and that the DMA ID is identifying one end of the pipeline which seems like a concept that could be made generic, even though the specific implementation of it is going to be firmware/hardware specific. > TLDR: > avs_path is basically a wrapper for a list of pipelines which shape given > stream - from ASoC side, that's a FE <-> BE relation. These pipelines exist > only on the DSP side and are tied to cAVS firmware expectations and > architecture. Again, if one strips the avs_path interface from cAVS IPC > logic, then there's basically nothing left. I think part of the problem here is that there's missing framework, coupled with the scaling issues that DPCM has. Ideally routing in a digital context shouldn't be fundamentally different to how we route in an analogue context, there's new bits needed for format management (both tracking what's valid and ensuring there's appropriate conversions) and we really want to be able to dynamically add and remove purely software components. Unfortunately work on actually implementing this mostly stalled out. > We're open for more input from the users and distros. That does not mean we > did not do our homework before moving to the coding part. In our research it > turned out that 'different device equals different card' is a popular and > easy to follow notion. These results are of course influenced by the other > OSes where such separation is more common and users got used to such model. The user/distro thing is kind of separate to the splitting things out into different devices thing (though there's overlap). The issue for users and distros is if they're OK with the change management that'd be involved in shipping new firmwares and dealing with any user visible changes. The multiple cards thing is partly user visible but it's also a framework thing - is the framework going to get confused trying to join things up between different cards? Especially with a DSP one can imagine a use case where someone does something like play the same audio stream to multiple devices for example. > It's worth noting that we did make use of the APIs that are already > available in ASoC. There are no hacks or hooks here, just the usage of the > available interfaces. The granular-cards approach, while preferred, also It's not just using the interfaces that exist, it's also using them in a way that's (ideally) simple and idiomatic so that we don't make it hard to refactor and improve things in future or otherwise create landmines for ourselves. It's a lot easier to not support something for now and then extend the framework than to have to pull something too fancy out of a driver. There's tradeoffs for maintainability, in general we aim to have drivers that are dumb or at least look a lot like each other so that it's easier to work over the subsystem as a whole. > By default, all the cards are independent of each other. avs-driver supports > 'cross linking' by the means of the conditional path. The 'conditional' is a > key word here. These paths are a 'side effect' of other paths being open > simultaneously. If there requirements are not met e.g.: a FE is not running Right, this sort of thing is what I'm worried about with splitting the cards - it's not impossible to manage but it's asking for trouble as things change. > In regard to quirk handling, could you elaborate? Right now all the > supported cross linking and the machine board division scenarios are not > causing any repercussions as it seems avs-driver gets credit for. I > understand that it's good to think about far reaching consequences sooner > than later, but the APIs allowing for the granular-card approach are here > for a very long time and the card/device division has been seen in practice > already. We end up needing a lot of system specific quirks for x86 systems since the idiom for ACPI firmware is to only have basic information in firmware and rely on the OS using DMI information or something to figure out critical bits of information about how the system is wired up. Some of that ends up in the CODEC drivers so should be easily shared but some of it ends up in the various x86 machine drivers and if there's two possible machine drivers for the same platform then both of them will need to separately add any quirks. > > It'd also be good to get this well enough integrated with the > > intel-dsp-config code to avoid the need for the dependency on > > SND_SOC_INTEL_SKYLAKE_FAMILY=n. If both are built then it could > > start off with always require a command line override to select > > the new driver with a _DSP_DRIVER_AVS constant, this can be > > revisited later. That mechanism is really nice for distros and > > users since it allows people to do binary distributions without > > having to worry about committing to one driver or another, > > reducing the risks for things like breakage on upgrade for some > > small subset of machines. > Hmm.. this means that in time (once skylake-driver is removed) two values > would translate to avs-driver selection rather than one. Value '2' is being > used for skylake-driver and we don't want to force users to manually change > it to anything else (i.e. to the to be added avs-driver selection value) > when the time comes. Yeah, that doesn't seem like an unreasonable outcome. Thinking about it I'm not sure the existing code handles the case where the user specified a specific driver on the command line but then didn't actually build it (which this'd just be a version of) well - haven't actually checked though.
On 2022-01-28 6:00 PM, Mark Brown wrote: Plenty of good comments, thank you. As there are several subjects that are part of current discussion, in this reply I've decided to focus on 'avs_path'. I'll continue the discussion regarding rest of the subjects later on. > AIUI the firmware itself has a bunch of DSP modules that can be > dynamically instantiated and what the path stuff is doing is providing > fixed sets of instantiations that can be switched between. It seems > like it should be possible to pull out the bit where we have sets of > modules we can instantiate from the mechanics of knowing what modules > are there and actually setting them up and tearing them down, other DSP > implementations would probably be able to benefit from that (at least > the larger ones) and I imagine more advanced users would find it useful > to be able to reconfigure the DSP pipelines separately from getting > firmware releases. There is also a notion of 'pipeline'. In cAVS ADSP case, almost all modules require parent pipeline in order to be instantiated. Mentioning this as modules alone are insufficient to create an audio stream. 'avs_path' is a runtime representative. 'avs_path_template' is a recipe for avs_path. All templates are created during topology load procedure. No modules or pipelines exist on DSP side until driver begins the (CREATE_PIPELINE + INIT_INSTANCE) IPC sequence. That happens during ->hw_params() callback of a DAI. So, avs_path_template provides a fixed set of recipes to create concrete avs_path what effectively creates modules and pipelines on DSP side. Mentioned all of this as _fixed sets of instantiations that can be switched between_ in my opinion implies existence of some sort of pre-allocated paths (modules, pipelines) on DSP side, what is not the case here. > I suspect that at least the template could be pulled apart, and that the > DMA ID is identifying one end of the pipeline which seems like a concept > that could be made generic, even though the specific implementation of > it is going to be firmware/hardware specific. ... > I think part of the problem here is that there's missing framework, > coupled with the scaling issues that DPCM has. Ideally routing in a > digital context shouldn't be fundamentally different to how we route in > an analogue context, there's new bits needed for format management (both > tracking what's valid and ensuring there's appropriate conversions) and > we really want to be able to dynamically add and remove purely software > components. Unfortunately work on actually implementing this mostly > stalled out. path-API found in path.h is limited and maps nicely to DAI operations: avs_path_create() avs_path_bind(struct avs_path *path) used during DAI's ->hw_params() avs_path_free(struct avs_path *path) avs_path_unbind(struct avs_path *path) used during DAI's ->hw_free() avs_path_reset(struct avs_path *path) avs_path_pause(struct avs_path *path) avs_path_run(struct avs_path *path, int trigger) state setters, used during DAI's ->prepare() and ->trigger() given this picture, one could say that there are framework elements that allow driver writer to implement whatever is needed for DSP-capable driver. And now back to the _full picture_ that I'm clearly not seeing yet. How do you envision interfaces that should be added to the ASoC framework? Are we talking about soc-path.c level of a change? It would be helpful to have even a single operation (from the list above) drawn as an example of what is expected. Regards, Czarek
On 1/30/2022 8:15 PM, Cezary Rojewski wrote: > > path-API found in path.h is limited and maps nicely to DAI operations: > > avs_path_create() > avs_path_bind(struct avs_path *path) > used during DAI's ->hw_params() > > avs_path_free(struct avs_path *path) > avs_path_unbind(struct avs_path *path) > used during DAI's ->hw_free() > > avs_path_reset(struct avs_path *path) > avs_path_pause(struct avs_path *path) > avs_path_run(struct avs_path *path, int trigger) > state setters, used during DAI's ->prepare() and ->trigger() > > given this picture, one could say that there are framework elements that > allow driver writer to implement whatever is needed for DSP-capable driver. Although Cezary wrote that avs_path_reset/_pause/_run maps nicely to trigger operation it's not direct mapping. AVS FW has requirements on order of operations on pipelines (which are grouped in paths on kernel side). For example on TRIGGER_STOP we need to first pause all pipelines before issuing reset to any of them. This is required by FW, so that if there are two pipelines it doesn't pause and reset one of them, while the other one is still in running state, as this causes xruns on FW side. Relevant fragment from "[RFC 27/37] ASoC: Intel: avs: non-HDA PCM BE operations" + case SNDRV_PCM_TRIGGER_STOP: + ret = avs_path_pause(data->path); + if (ret < 0) + dev_err(dai->dev, "pause BE path failed: %d\n", ret); + + if (cmd == SNDRV_PCM_TRIGGER_STOP) { + ret = avs_path_reset(data->path); + if (ret < 0) + dev_err(dai->dev, "reset FE path failed: %d\n", ret); + } + break; + I would say that such behavior doesn't translate nicely to generic API. I tried looking once again at how one would split the path concept to make it more generic, but it is hard. On one hand paths are tied to AVS driver topology design, on the other hand we have (mentioned above) FW requirements. To describe it in more detail, in AVS we need topology as it describes bindings between paths. Simple topologies have route map similar to this one: SectionGraph."ssp0_Tx_spt-audio-playback" { index "0" lines [ "ssp0 Tx, , ssp0p_be" "ssp0p_be, , ssp0p_fe" "ssp0p_fe, , spt-audio-playback" ] } where ssp0p_be and ssp0p_fe are widgets describing BE and FE configuration. Taking for example FE widget we have: SectionWidget."ssp0p_fe" { index "0" type "scheduler" no_pm "true" ignore_suspend "false" data [ "path_tmpl2_data" ] } where we can see that apart from its own configuration it has additional data describing path inside it: SectionData."path_tmpl2_data" { tuples [ "path_tmpl2_tuples" "path_tmpl2_path0_tuples" "path_tmpl2_path0_ppl0_tuples" "path_tmpl2_path0_ppl0_mod0_tuples" "path_tmpl2_path0_ppl0_bindid0_tuples" ] } now for the concept of paths the most interesting field is "path_tmpl2_path0_ppl0_bindid0_tuples" as it describes to which path we want to bind. It is done this way as FW modules internally have pins, and while in most cases one wants to just bind on pin 0, sometimes there is a need to describe more complicated connections. And so we circled back to FW requirements. Overall I would say that path design in AVS is tied too much to FW requirements to be made generic. And even if some general API was provided we would still need most of current code on AVS path to handle the requirements, while we would have additional constrains coming from API above. > And now back to the _full picture_ that I'm clearly not seeing yet. How > do you envision interfaces that should be added to the ASoC framework? > Are we talking about soc-path.c level of a change? It would be helpful > to have even a single operation (from the list above) drawn as an > example of what is expected. Similarly to the above I'm open to suggestions on how such API may look like. Best Regards, Amadeusz
On Sun, Jan 30, 2022 at 08:15:26PM +0100, Cezary Rojewski wrote: > On 2022-01-28 6:00 PM, Mark Brown wrote: > > AIUI the firmware itself has a bunch of DSP modules that can be > > dynamically instantiated and what the path stuff is doing is providing > > fixed sets of instantiations that can be switched between. It seems > > like it should be possible to pull out the bit where we have sets of > > modules we can instantiate from the mechanics of knowing what modules > > are there and actually setting them up and tearing them down, other DSP > > implementations would probably be able to benefit from that (at least > > the larger ones) and I imagine more advanced users would find it useful > > to be able to reconfigure the DSP pipelines separately from getting > > firmware releases. > There is also a notion of 'pipeline'. In cAVS ADSP case, almost all modules > require parent pipeline in order to be instantiated. Mentioning this as > modules alone are insufficient to create an audio stream. > 'avs_path' is a runtime representative. > 'avs_path_template' is a recipe for avs_path. All templates are created > during topology load procedure. > No modules or pipelines exist on DSP side until driver begins the > (CREATE_PIPELINE + INIT_INSTANCE) IPC sequence. That happens during > ->hw_params() callback of a DAI. That doesn't sound like a particularly unsurprising requirement for firmware to have TBH - I'd expect we'd need generic handling for partially constructed paths, including only actually instantiating them when they're complete (in a similar manner to only powering on analogue paths when everything is joined up). > So, avs_path_template provides a fixed set of recipes to create concrete > avs_path what effectively creates modules and pipelines on DSP side. Sure, I get that that's what it's doing. > path-API found in path.h is limited and maps nicely to DAI operations: > avs_path_create() > avs_path_bind(struct avs_path *path) > used during DAI's ->hw_params() > avs_path_free(struct avs_path *path) > avs_path_unbind(struct avs_path *path) > used during DAI's ->hw_free() > avs_path_reset(struct avs_path *path) > avs_path_pause(struct avs_path *path) > avs_path_run(struct avs_path *path, int trigger) > state setters, used during DAI's ->prepare() and ->trigger() > given this picture, one could say that there are framework elements that > allow driver writer to implement whatever is needed for DSP-capable driver. Right, which points towards pulling bits of it that can be made generic out of the driver and shared with other DSP implementations. > And now back to the _full picture_ that I'm clearly not seeing yet. How do > you envision interfaces that should be added to the ASoC framework? Are we > talking about soc-path.c level of a change? It would be helpful to have even > a single operation (from the list above) drawn as an example of what is > expected. I don't have an off the shelf answer for you here, like I said half the thing here is to split this out from the rest of the series so it can be considered separately. The digital domain stuff is obviously key here, the main extra bit for any sort of dynamic DSP routing seems to be working out a way for userspace to set up and remove new paths - that's probably new userspace ABI. Perhaps that's a runtime thing with initial setup in UCM. Or perhaps it's better to have something closer to what you have done but split out like topology is so that the bulk of the code is reusable with other firmwares and there's a thinner layer with the firmware specific bits in it.
On Wed, Feb 02, 2022 at 02:26:01PM +0100, Amadeusz Sławiński wrote: > Although Cezary wrote that avs_path_reset/_pause/_run maps nicely to trigger > operation it's not direct mapping. AVS FW has requirements on order of > operations on pipelines (which are grouped in paths on kernel side). For > example on TRIGGER_STOP we need to first pause all pipelines before issuing > reset to any of them. This is required by FW, so that if there are two > pipelines it doesn't pause and reset one of them, while the other one is > still in running state, as this causes xruns on FW side. ... > I would say that such behavior doesn't translate nicely to generic API. This doesn't sound particularly strange, it's not a million miles away from the requirements we have from hardware around keeping clocks alive. > I tried looking once again at how one would split the path concept to make > it more generic, but it is hard. On one hand paths are tied to AVS driver > topology design, on the other hand we have (mentioned above) FW > requirements. Please understand that it is incredibly common for people to belive that their system is somehow unique and needs to special case things that on further examination turn out to be perfectly reasonable to handle in a generic fashion. Sometimes it's simply a case of just needing to do the work, sometimes small enhancements are needed to the generic framework and sometimes it's a case of refactoring the code so that some bits land in generic code and some bits land in the driver. Especially with enormous amounts of code like you've got here there's a natural bias towards wanting to make minimal changes. > now for the concept of paths the most interesting field is > "path_tmpl2_path0_ppl0_bindid0_tuples" as it describes to which path we want > to bind. It is done this way as FW modules internally have pins, and while > in most cases one wants to just bind on pin 0, sometimes there is a need to > describe more complicated connections. And so we circled back to FW > requirements. The idea of an algorithm having multiple inputs or outputs seems logical and generic - the examples that spring to mind are things like mixers, beam forming or echo/noise cancellation. These seem like they're going to be present in a wide range of DSP firmwares.
On 2022-02-02 3:41 PM, Mark Brown wrote: > I don't have an off the shelf answer for you here, like I said half the > thing here is to split this out from the rest of the series so it can be > considered separately. The digital domain stuff is obviously key here, > the main extra bit for any sort of dynamic DSP routing seems to be > working out a way for userspace to set up and remove new paths - that's > probably new userspace ABI. Perhaps that's a runtime thing with initial > setup in UCM. Or perhaps it's better to have something closer to what > you have done but split out like topology is so that the bulk of the > code is reusable with other firmwares and there's a thinner layer with > the firmware specific bits in it. I've re-organized the series and sent three chunks that could be sent immediately, at least in my opinion. HDA bits and IPC protocol plus code loading make the first two and are probably least important for the current discussion. Two patches that target topology parsing and path management have been split into total of 13 patches and sent as an RFC [1]. This should help in further discussion, extracting the framework-friendly bits and possibly shaping the new framework interface. [1]: https://lore.kernel.org/alsa-devel/20220207132532.3782412-1-cezary.rojewski@intel.com/T/#t Regards, Czarek