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