| Message ID | 20211208111301.1817725-1-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ASoC: Intel: AVS - Audio DSP for cAVS | expand |
> As AudioDSP firmware which avs-driver communicates with supports a wide > range of audio formats, module configurations and multi-pipeline > streams, couple of new concepts are introduce to enable all those > functionalities: > > - Path template and path variants > - Runtime path > - Conditional path > - Granular sound cards (as opposed to 'super-cards') > > These are later better explained by their respective patches: > 'Topology parsing', 'Path management', 'Conditional-path support' and > 'Machine board registration'. > > A 'path' represents a DSP side of audio stream in runtime - is a logical > container for pipelines which are themselves composed of modules - > processing units. Due to high range of possible audio format > combinations, there can be more variants of given path (and thus, its > pipelines and modules) than total number of pipelines and module > instances which firmware supports concurrently, all the instance IDs are > allocated dynamically with help of IDA interface. 'Path templates' come > from topology file and describe a pattern which is later used to > actually create runtime 'path'. > > To support modern audio usecases such as WaveOnVoice and EchoReference, > conditional paths concept came into existence. These work similarly to > standard paths except are a consequence of other paths being created or > deleted, rather then being created when userspace app opens specific FE > for streaming. Their state machine is controlled by source and sink > paths which created them in the first place. > > Granular machine boards is a contrast to 'super-card' idea which is > currently widely used throughout Intel ASoC drivers. Major reasons are: > complexity reduction (each board now focuses on a single, concrete > device) and overall reduction of topology file size when entire > configuration is taken into account. This has functional benefits too: > one card failing won't prevent others from probing and being operative. Just to be clear, my name being listed in the Intel internal reviewers shall not be construed as an endorsement of this patch set. Parts of my feedback was taken into account, but I still have quite a bit of heartburn with 4 high-level design topics: a) The change list mentions sysfs being dropped, Patch 19/37 says otherwise. +++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-avs @@ -0,0 +1,24 @@ +What: /sys/devices/pci0000:00/<dev>/<tplg_name>/<path_template>:<path>/<pipeline>/state +Date: December 2021 +Contact: Cezary Rojewski <cezary.rojewski@intel.com> +Description: + File is only present when specific pipeline is instantiated in + driver. It is used to check or change pipeline state. Possible + read values: invalid, uninitialized, reset, paused, running, + eos, error_stop, saved, restored. + Allows only to set state to: reset, paused, running. That was my biggest worry in internal reviews, I do not see any rationale for exposing an interface to userspace to modify pipeline states. I believe the intent is to have a follow-up series on this topic, but it's not clear what problem this is trying to solve. There's a fundamental disconnect here as to why the kernel driver could not control states on its own, and it begs the question if the 37 patches actually work without this odd userspace interface. b) the concept of 'path' is totally specific to this driver and will not be used by any other Intel solution. The notion of having more flexibility in dynamic reconfiguration of a pipeline, e.g. to avoid instantiating an unnecessary sample-rate conversion, is on paper a good one and is used in Windows solutions, but in practice all the existing end-to-end integrations in Linux/Chrome do require fairly static configurations with fixed sample rates. In other words, it's debatable whether any end-user will see any benefits in terms of experience/power/performance, and the added complexity is handled with a custom solution instead of improvements to DAPM/DPCM - which as we found out does need significant love to support multiple streams being mixed/demuxed. At the ALSA/ASoC level, I believe we have more important priorities such as the notion of 'DAPM domain', constraint propagation and hardening for complex use-cases, and improvements to the pipeline handling shall be done at the framework level, not the platform-specific driver level. c) I do not get how interfaces can be split to define different cards, specifically in use cases where different types of interfaces are used concurrently - think echo cancellation with the reference coming from a I2S link and microphone data from a PDM link. This would result in independent cards being joined at the hip, with no ability to propagate DAPM events. Cezary assures me this was tested but I still don't get how this might work. For the SOF work, we did plan to spin-off HDMI to a different card with the 'SOF client', but stopped short of an interface-based split. c1) I don't really buy the notion of trying to keep going if one card fails to probe. "Fail big and fail early" is much easier to support, and in the case of interactions between interfaces you do need all cards to be functional anyways. c2) What this split also requires is the addition of ~13 odd new machine drivers, along with new topologies and new UCM files. This seems like a bridge too far to me, I don't see how end-users might transition to this new driver before the end of the support period where the community typically takes over legacy devices. In the mean time, the Skylake driver support will be required (5.15 is broken btw). d) Ranjani, Peter, Bard, Rander and I are progressing to provide support for the 'cAVS' IPC, aka IPC v4 in the SOF driver, with a repartitioning to support multiple IPCs, and already have working prototypes with basic functionality from Skylake, KabyLake, ApolloLake to newer platforms. The patches will be submitted for the next kernel cycle after the Winter break, and clearly with this patchset there is no plan for any reuse. I've personally spent two weeks of my life reviewing this code, shared internally only on October 28, and trying to align. Obviously I wasn't successful and probably wasted my time... I completely disagree with Cezary and his management's decision to float 37 patches upstream as RFC, with more coming. This goes against everything we've tried to do in the last 3 years to improve Intel's standing. I don't think it's right to ask for feedback from the maintainers and community when internally we were unable to make progress. What can I say other than 'this is really sad'. The work in the SOF driver will continue regardless of what happens with this patchset, which I am not going to comment further on. Cezary, I tried to help, didn't work, you're on your own now. Best of luck. > Changes [internal] RFC v2 -> [public] RFC v1: > - dropped any sysfs related changes from this series, moved to follow up > one > - dropped entire subscription-mechanism found in ipc.c. Handlers that > are delegated to service certain firmware notifications are now called > directly > - fixed kernel doc for snd_soc_dapm_new_dai_widgets() as reported by ikp > - prefixed snd_hda_codec_device_init() as suggested by Amadeo > - improved comments for d0ix transitions for APL-based platforms as > suggested by Pierre > - a ton of spelling related fixes in most of the commit messages > - fixed remaining warnings pointed by scan-build (variable assigned but > not used) > - replaced most of 'cAVS X.Y' expression usages with 'platform-based' > equivalents as suggested by Pierre e.g.: cAVS 1.5 -> SKL-based > > Changes [internal] RFC v1 -> [internal] RFC v2: > > - fixed memleak caused by lack of kfree(vols) if memory allocation fails > in avs_peakvol_create() as reported by Curtis > - fixed missing 'i' iterator incrementation in avs_widget_ready() > causing reference loss as reported by Curtis > - replace hardcode: 0x40 usage with snd_hdac_calc_stream_format as > suggested by Curtis. > In consequence, readability for all code loading (CL) procedues has > increased and such approach auto-documents the CL stream preparation > > - updated behavior of all index-fetching functions found in utils.c: > avs_module_entry_index(), avs_module_id_entry_index() and follow ups: > avs_get_module_entry(), avs_get_module_id_entry() to better conform to > linux-kernel standard when no entry is found (return -ENOENT) rather > than C++ standard (return -1, what in kernel case translated to -EPERM) > as suggested by Curtis and Peter > - several suggestions have been made regarding spacing, and so far, I've > agreed and applied with all of them. None proposed seemed out of place > or redundant > > - avs_path_stop() renamed to avs_path_pause() pipeline states are > represented by RESET/PAUSED/RUNNING. avs_path_reset() and > avs_path_run() were already there and avs_path_stop() just didn't look > cohesive > - added missing parsers for num_output_pin and num_input_pin which are > required for custom modules such as WAVES or DSM > - dropped 'priv_param_length' from custom module descriptor as this > field is obsolete in firmware > > - parse_dictionary() has been split into parse_dictionary_header() and > parse_dictionary_entries() to drop code duplication present in several > parsing function which could not re-use entire parse_dictionary() > - added avs_tplg_vendor_array_lookup_next() and > avs_tplg_vendor_entry_next() to drop code duplicated present in several > parsing functions. This change greatly impacted readability of all > parsers > - parsing helpers such avs_tplg_vendor_array_lookup() now return offset > by updating specified in function argument list u32 *offset variable. > This is to address problem when u32 offset would be greater than max > int, which is the return type for these functions > - AVS_DEFINE_PTR_PARSER() macro has been introduced to drop code > duplication for all ptr-parsing users > > - all struct avs_path_module creators have had their declaration > updated: function argument *owner ceased to exist as it could already > be accessed by mod->owner > > - fixed the order of operation for conditional paths (e.g.: echo > reference) so these are no longer controlled by "source" path and > instead are impacted by state changes of source and sink paths both. > Previously only source path e.g. playback sourcing echo reference > would trigger RUNNING status for conditional path. Equivalent RUNNING > on WoV path which is in this case sink path, would not do so, leading > to order-of-operation problems. Behavior has been changed to: both > source and sink need to be RUNNING for conditional path to be set to > RUNNING too. PAUSED for either source or sink will cause PAUSED > transition for conditional path. > - to achieve the above, path states are now saved in 'state' i.e. new > u32 field for struct avs_path > > - resigned from fw_filename field usage in favour of newly added > tplg_filename for machine board descriptors as suggested by Pierre > - platform descriptor fields have had their names update better reflect > their purpose as suggested by Pierre > - fixed comp_list missing locking when manipulated > - all message senders now accept request as pointer as suggeseted by > Peter > - resigned of AZX_ usage for all ADSP-related registers, leaving them > only for HOST memory space related operations > - fixed disable path for core DSP operations: power/reset/stall as > reported by Peter > > - safety when locking between received responses (reply vs notification) > has been lowered as suggested by Pierre. Most usages are not performed > in IRQ context and none is done in hard-IRQ one > - s/master/main/ plus AVS_MAIN_CORE_MASK has replaced ->master_mask > - several functions have had their logging updated - logs have been > moved to lower level functions as suggested by Pierre > - hdac_ext_stream usage has been streamlined to estream, hdac_streams > are represented by hstream instead > - hw_params() are resilient to scenarios when they are called mutliple > times as reported by Pierre > - avs_dsp_enable() now collapses if any of its steps fails as reported > by Pierre and Peter > - avs_module_ida_empty() now returns value of type bool as suggested by > Bard > > [1]: https://www.spinics.net/lists/alsa-devel/msg116440.html > [2]: https://www.spinics.net/lists/alsa-devel/msg116901.html > [3]: https://www.spinics.net/lists/alsa-devel/msg94199.html > [4]: https://www.spinics.net/lists/alsa-devel/msg92588.html > [5]: https://lore.kernel.org/all/20190808181549.12521-1-cezary.rojewski@intel.com/ > [6]: https://lore.kernel.org/all/YaDq7L1Mu++3UBL7@sirena.org.uk/T/ > > > Cezary Rojewski (37): > ALSA: hda: Add snd_hdac_ext_bus_link_at() helper > ALSA: hda: Update and expose snd_hda_codec_device_init() > ALSA: hda: Update and expose codec register procedures > ALSA: hda: Expose codec cleanup and power-save functions > ALSA: hda: Add helper macros for DSP capable devices > ASoC: Export DAI register and widget ctor and dctor functions > ASoC: Intel: Introduce AVS driver > ASoC: Intel: avs: Inter process communication > ASoC: Intel: avs: Add code loading requests > ASoC: Intel: avs: Add pipeline management requests > ASoC: Intel: avs: Add module management requests > ASoC: Intel: avs: Add power management requests > ASoC: Intel: avs: Add ROM requests > ASoC: Intel: avs: Add basefw runtime-parameter requests > ASoC: Intel: avs: Firmware resources management utilities > ASoC: Intel: avs: Declare module configuration types > ASoC: Intel: avs: Dynamic firmware resources management > ASoC: Intel: avs: Topology parsing > ASoC: Intel: avs: Path management > ASoC: Intel: avs: Conditional-path support > ASoC: Intel: avs: General code loading flow > ASoC: Intel: avs: Implement CLDMA transfer > ASoC: Intel: avs: Code loading over CLDMA > ASoC: Intel: avs: Code loading over HDA > ASoC: Intel: avs: Generic soc component driver > ASoC: Intel: avs: Generic PCM FE operations > ASoC: Intel: avs: non-HDA PCM BE operations > ASoC: Intel: avs: HDA PCM BE operations > ASoC: Intel: avs: Coredump and recovery flow > ASoC: Intel: avs: Prepare for firmware tracing > ASoC: Intel: avs: D0ix power state support > ASoC: Intel: avs: Event tracing > ASoC: Intel: avs: Machine board registration > ASoC: Intel: avs: PCI driver implementation > ASoC: Intel: avs: Power management > ASoC: Intel: avs: SKL-based platforms support > ASoC: Intel: avs: APL-based platforms support > > .../ABI/testing/sysfs-bus-pci-devices-avs | 24 + > include/sound/hda_codec.h | 11 +- > include/sound/hdaudio.h | 2 + > include/sound/hdaudio_ext.h | 50 + > include/sound/soc-acpi.h | 2 + > include/sound/soc-dapm.h | 1 + > include/uapi/sound/intel/avs/tokens.h | 147 ++ > sound/hda/ext/hdac_ext_controller.c | 31 +- > sound/pci/hda/hda_codec.c | 93 +- > sound/pci/hda/hda_local.h | 2 - > sound/soc/codecs/hdac_hda.c | 2 +- > sound/soc/intel/Kconfig | 19 + > sound/soc/intel/Makefile | 1 + > sound/soc/intel/avs/Makefile | 12 + > sound/soc/intel/avs/apl.c | 244 +++ > sound/soc/intel/avs/avs.h | 331 ++++ > sound/soc/intel/avs/board_selection.c | 459 +++++ > sound/soc/intel/avs/cldma.c | 328 ++++ > sound/soc/intel/avs/cldma.h | 29 + > sound/soc/intel/avs/core.c | 737 +++++++ > sound/soc/intel/avs/dsp.c | 326 ++++ > sound/soc/intel/avs/ipc.c | 612 ++++++ > sound/soc/intel/avs/loader.c | 672 +++++++ > sound/soc/intel/avs/messages.c | 674 +++++++ > sound/soc/intel/avs/messages.h | 813 ++++++++ > sound/soc/intel/avs/path.c | 1287 +++++++++++++ > sound/soc/intel/avs/path.h | 85 + > sound/soc/intel/avs/pcm.c | 1240 ++++++++++++ > sound/soc/intel/avs/registers.h | 83 + > sound/soc/intel/avs/skl.c | 127 ++ > sound/soc/intel/avs/topology.c | 1700 +++++++++++++++++ > sound/soc/intel/avs/topology.h | 207 ++ > sound/soc/intel/avs/trace.c | 34 + > sound/soc/intel/avs/trace.h | 158 ++ > sound/soc/intel/avs/utils.c | 305 +++ > sound/soc/soc-core.c | 1 + > sound/soc/soc-dapm.c | 15 + > 37 files changed, 10824 insertions(+), 40 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-devices-avs > create mode 100644 include/uapi/sound/intel/avs/tokens.h > create mode 100644 sound/soc/intel/avs/Makefile > create mode 100644 sound/soc/intel/avs/apl.c > create mode 100644 sound/soc/intel/avs/avs.h > create mode 100644 sound/soc/intel/avs/board_selection.c > create mode 100644 sound/soc/intel/avs/cldma.c > create mode 100644 sound/soc/intel/avs/cldma.h > create mode 100644 sound/soc/intel/avs/core.c > create mode 100644 sound/soc/intel/avs/dsp.c > create mode 100644 sound/soc/intel/avs/ipc.c > create mode 100644 sound/soc/intel/avs/loader.c > create mode 100644 sound/soc/intel/avs/messages.c > create mode 100644 sound/soc/intel/avs/messages.h > create mode 100644 sound/soc/intel/avs/path.c > create mode 100644 sound/soc/intel/avs/path.h > create mode 100644 sound/soc/intel/avs/pcm.c > create mode 100644 sound/soc/intel/avs/registers.h > create mode 100644 sound/soc/intel/avs/skl.c > create mode 100644 sound/soc/intel/avs/topology.c > create mode 100644 sound/soc/intel/avs/topology.h > create mode 100644 sound/soc/intel/avs/trace.c > create mode 100644 sound/soc/intel/avs/trace.h > create mode 100644 sound/soc/intel/avs/utils.c >
On Wed, Dec 08, 2021 at 10:27:43AM -0600, Pierre-Louis Bossart wrote: > @@ -0,0 +1,24 @@ > +What: > /sys/devices/pci0000:00/<dev>/<tplg_name>/<path_template>:<path>/<pipeline>/state > That was my biggest worry in internal reviews, I do not see any > rationale for exposing an interface to userspace to modify pipeline > states. I believe the intent is to have a follow-up series on this > topic, but it's not clear what problem this is trying to solve. There's > a fundamental disconnect here as to why the kernel driver could not > control states on its own, and it begs the question if the 37 patches > actually work without this odd userspace interface. If it's mainly used for debugging then it could be exposed through debugfs with less worry. > b) the concept of 'path' is totally specific to this driver and will not > be used by any other Intel solution. The notion of having more > flexibility in dynamic reconfiguration of a pipeline, e.g. to avoid > instantiating an unnecessary sample-rate conversion, is on paper a good > one and is used in Windows solutions, but in practice all the existing > end-to-end integrations in Linux/Chrome do require fairly static > configurations with fixed sample rates. In other words, it's debatable > whether any end-user will see any benefits in terms of > experience/power/performance, and the added complexity is handled with a > custom solution instead of improvements to DAPM/DPCM - which as we found > out does need significant love to support multiple streams being > mixed/demuxed. At the ALSA/ASoC level, I believe we have more important > priorities such as the notion of 'DAPM domain', constraint propagation > and hardening for complex use-cases, and improvements to the pipeline > handling shall be done at the framework level, not the platform-specific > driver level. I've not meaningfully looked at the series yet (it's quite large!) but commenting generally I do agree that if we're adding interfaces offering detailed control of the digital domain we should be doing this at the framework level - it's a common problem affecting a bunch of SoCs and some CODECs too and it's only going to get harder to address in a generic fashion if we add per driver interfaces. On the other hand if there's good interfaces that work for people in practice with driver specific implementations perhaps they can be adapted to be more generic. > I completely disagree with Cezary and his management's decision to float > 37 patches upstream as RFC, with more coming. This goes against > everything we've tried to do in the last 3 years to improve Intel's > standing. I don't think it's right to ask for feedback from the > maintainers and community when internally we were unable to make > progress. What can I say other than 'this is really sad'. > The work in the SOF driver will continue regardless of what happens with > this patchset, which I am not going to comment further on. This is obviously not ideal, I would like to have a consistent view from at least Intel about the direction this is heading but I understand that this might be difficult to achieve in such a large organization. Input from users like the distributions and PulseAudio/PipeWire is also very important here, they'll face a lot of the complexity and hassle from end users. What conversations have been had thus far? I guess ChromeOS is going to prefer some combination of sticking with what it's got for stability and transitioning to SoF for control of the firmware? I do see that the code is using snd_intel_dsp_driver_probe() so we should be able to manage any transition between implementations here, though for that to be fully effective we'd need to be able to build both from once.
On 2021-12-08 5:27 PM, Pierre-Louis Bossart wrote: > Just to be clear, my name being listed in the Intel internal reviewers > shall not be construed as an endorsement of this patch set. Parts of my > feedback was taken into account, but I still have quite a bit of > heartburn with 4 high-level design topics: > > a) The change list mentions sysfs being dropped, Patch 19/37 says otherwise. Not true. Functional code has been removed entirely, it's clear that a programmer missed cutting the Documentation-part. For the remainder of this patchset review, this topic shall be discarded and as we all agreed internally, moved to the separate subject. Email starting sysfs discussion should be sent within next few days. Please don't cloud the avs-driver-core discussion with subjects that are not part of it, thank you. > b) the concept of 'path' is totally specific to this driver and will not > be used by any other Intel solution. The notion of having more > flexibility in dynamic reconfiguration of a pipeline, e.g. to avoid > instantiating an unnecessary sample-rate conversion, is on paper a good > one and is used in Windows solutions, but in practice all the existing > end-to-end integrations in Linux/Chrome do require fairly static > configurations with fixed sample rates. In other words, it's debatable > whether any end-user will see any benefits in terms of > experience/power/performance, and the added complexity is handled with a > custom solution instead of improvements to DAPM/DPCM - which as we found > out does need significant love to support multiple streams being > mixed/demuxed. At the ALSA/ASoC level, I believe we have more important > priorities such as the notion of 'DAPM domain', constraint propagation > and hardening for complex use-cases, and improvements to the pipeline > handling shall be done at the framework level, not the platform-specific > driver level. For all the readers, the following problem that has been identified as one preventing the direct re-use of DAPM: Depending on audio format, path may take different form i.e. number of modules and pipelines may change within given path. DAPM widgets could help cover such situation if form changes for different PCMs. Here, however, change of form is done on the same PCM. To cover this with DAPM, a number of kcontrols would have to be engaged (and that number would scale with each format supported) - path template-path variant relation allows to do so without any userspace involved. Several discussions have been held internally regarding this subject and the TLDR is: 'correctness' vs 'less effort'. skylake-driver and its friends such as haswell-driver failed eventually due to being implemented against the firmware-spec and other recommendations. It's preferable to adhere to specs and follow the recommendations. 'it's debatable' - that's exactly why we have had several discussions regarding this, and there are pros and cons to each option. Also, this does not prevent DAPM/DPCM from being updated in the future if we find something driver-specific to be rather easy portable to the framework. Otherwise that's a separate subject - large framework changes should not be discussed in driver-specific threads. > c) I do not get how interfaces can be split to define different cards, > specifically in use cases where different types of interfaces are used > concurrently - think echo cancellation with the reference coming from a > I2S link and microphone data from a PDM link. This would result in > independent cards being joined at the hip, with no ability to propagate > DAPM events. Cezary assures me this was tested but I still don't get how > this might work. For the SOF work, we did plan to spin-off HDMI to a > different card with the 'SOF client', but stopped short of an > interface-based split. avs-driver validation hosts a wide range tests and CI farm, just like it was the case for catpt-driver. The cross-topology bindings work just fine. Again, "don't understand" is not a technical argument. > c1) I don't really buy the notion of trying to keep going if one card > fails to probe. "Fail big and fail early" is much easier to support, and > in the case of interactions between interfaces you do need all cards to > be functional anyways. Not true. In most cases sound devices are separate beings, and there is no reason to tie all of them together. There are user-experience benefits for separating them - HDMI failing to probe does not prevent your I2S speaker from being functional. > c2) What this split also requires is the addition of ~13 odd new machine > drivers, along with new topologies and new UCM files. This seems like a > bridge too far to me, I don't see how end-users might transition to this > new driver before the end of the support period where the community > typically takes over legacy devices. In the mean time, the Skylake > driver support will be required (5.15 is broken btw). 'lower effort job' is not a good argument. We should provide the right, the correct solution to the users, especially given the history behind sound/soc/intel/. catpt-driver and other changes were the steps in the right direction, this is yet another one. Once avs-driver is fully upstreamed, skylake-driver and its boards will be eventually removed - with avs-related boards replacing them. upstream support 'window' differs Intel-Client-relation one. The exact same motivation driven catpt - regardless of the fact that support window was about to close. Saying "times out, I'm out" to the community is not the right thing to do when the users and the problems are still there. > d) Ranjani, Peter, Bard, Rander and I are progressing to provide support > for the 'cAVS' IPC, aka IPC v4 in the SOF driver, with a repartitioning > to support multiple IPCs, and already have working prototypes with basic > functionality from Skylake, KabyLake, ApolloLake to newer platforms. The > patches will be submitted for the next kernel cycle after the Winter > break, and clearly with this patchset there is no plan for any reuse. That's something new.. avs-driver is a complete product, which is founded on the skylake-driver-refacting patches with some shared here, on alsa-devel in 2019. 'complete product' is the opposite to 'basic functionaity' and given the current SOF-framework architecture, it does not align with cAVS firmware interface recommendations. I fear that's going into the exact same trap skylake-driver got into years ago. > I've personally spent two weeks of my life reviewing this code, shared > internally only on October 28, and trying to align. Obviously I wasn't > successful and probably wasted my time... That's almost six weeks on the list. Also, many experienced audio developers helped shape the solution long before than that. About wasting time - sorry to hear, but dozen or so other reviewers from audio and other groups do not feel that way. Most of your comments have been applied, same as for other comments. The remaining points of yours are left at "I don't understand" point. Such reasoning cannot lead to solution being implemented in the incorrect manner or against the recommendations. > I completely disagree with Cezary and his management's decision to float > 37 patches upstream as RFC, with more coming. This goes against > everything we've tried to do in the last 3 years to improve Intel's > standing. I don't think it's right to ask for feedback from the > maintainers and community when internally we were unable to make > progress. What can I say other than 'this is really sad'. The management that owns SKL/KBL support and the vast majority of audio developers disagrees with your opinion. All this work is to provide the best for the community and some fresh view on subjects that have been left unattanded for too long. Last three years was a battle to repair all the mistakes introduced in sound/soc/intel for the community and the clients alike. IPG and the surrounding support teams received an excellent opinion and reviews addressing all the problems. > The work in the SOF driver will continue regardless of what happens with > this patchset, which I am not going to comment further on. > > Cezary, I tried to help, didn't work, you're on your own now. It's not "Cezary", it's Intel and IPG. Vast majority of developers is in favor of the decision made. The management is too. People found in SOF-framework team also commented and saw real, technical reasons behind this solution. > Best of luck. It's not about luck. It's about professionalism and bringing the best to the community. Regards, Czarek
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 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. ... > 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. Hello, Is the proposal described in my previous message acceptable on your end Mark? 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