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.