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
