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.