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.