On 2021-12-08 5:27 PM, Pierre-Louis Bossart wrote:
Just to be clear, my name being listed in the Intel internal reviewers shall not be construed as an endorsement of this patch set. Parts of my feedback was taken into account, but I still have quite a bit of heartburn with 4 high-level design topics:
a) The change list mentions sysfs being dropped, Patch 19/37 says otherwise.
Not true. Functional code has been removed entirely, it's clear that a programmer missed cutting the Documentation-part. For the remainder of this patchset review, this topic shall be discarded and as we all agreed internally, moved to the separate subject. Email starting sysfs discussion should be sent within next few days. Please don't cloud the avs-driver-core discussion with subjects that are not part of it, thank you.
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.
For all the readers, the following problem that has been identified as one preventing the direct re-use of DAPM:
Depending on audio format, path may take different form i.e. number of modules and pipelines may change within given path. DAPM widgets could help cover such situation if form changes for different PCMs. Here, however, change of form is done on the same PCM. To cover this with DAPM, a number of kcontrols would have to be engaged (and that number would scale with each format supported) - path template-path variant relation allows to do so without any userspace involved.
Several discussions have been held internally regarding this subject and the TLDR is: 'correctness' vs 'less effort'. skylake-driver and its friends such as haswell-driver failed eventually due to being implemented against the firmware-spec and other recommendations. It's preferable to adhere to specs and follow the recommendations. 'it's debatable' - that's exactly why we have had several discussions regarding this, and there are pros and cons to each option. Also, this does not prevent DAPM/DPCM from being updated in the future if we find something driver-specific to be rather easy portable to the framework. Otherwise that's a separate subject - large framework changes should not be discussed in driver-specific threads.
c) I do not get how interfaces can be split to define different cards, specifically in use cases where different types of interfaces are used concurrently - think echo cancellation with the reference coming from a I2S link and microphone data from a PDM link. This would result in independent cards being joined at the hip, with no ability to propagate DAPM events. Cezary assures me this was tested but I still don't get how this might work. For the SOF work, we did plan to spin-off HDMI to a different card with the 'SOF client', but stopped short of an interface-based split.
avs-driver validation hosts a wide range tests and CI farm, just like it was the case for catpt-driver. The cross-topology bindings work just fine. Again, "don't understand" is not a technical argument.
c1) I don't really buy the notion of trying to keep going if one card fails to probe. "Fail big and fail early" is much easier to support, and in the case of interactions between interfaces you do need all cards to be functional anyways.
Not true. In most cases sound devices are separate beings, and there is no reason to tie all of them together. There are user-experience benefits for separating them - HDMI failing to probe does not prevent your I2S speaker from being functional.
c2) What this split also requires is the addition of ~13 odd new machine drivers, along with new topologies and new UCM files. This seems like a bridge too far to me, I don't see how end-users might transition to this new driver before the end of the support period where the community typically takes over legacy devices. In the mean time, the Skylake driver support will be required (5.15 is broken btw).
'lower effort job' is not a good argument. We should provide the right, the correct solution to the users, especially given the history behind sound/soc/intel/. catpt-driver and other changes were the steps in the right direction, this is yet another one. Once avs-driver is fully upstreamed, skylake-driver and its boards will be eventually removed - with avs-related boards replacing them.
upstream support 'window' differs Intel-Client-relation one. The exact same motivation driven catpt - regardless of the fact that support window was about to close. Saying "times out, I'm out" to the community is not the right thing to do when the users and the problems are still there.
d) Ranjani, Peter, Bard, Rander and I are progressing to provide support for the 'cAVS' IPC, aka IPC v4 in the SOF driver, with a repartitioning to support multiple IPCs, and already have working prototypes with basic functionality from Skylake, KabyLake, ApolloLake to newer platforms. The patches will be submitted for the next kernel cycle after the Winter break, and clearly with this patchset there is no plan for any reuse.
That's something new.. avs-driver is a complete product, which is founded on the skylake-driver-refacting patches with some shared here, on alsa-devel in 2019. 'complete product' is the opposite to 'basic functionaity' and given the current SOF-framework architecture, it does not align with cAVS firmware interface recommendations. I fear that's going into the exact same trap skylake-driver got into years ago.
I've personally spent two weeks of my life reviewing this code, shared internally only on October 28, and trying to align. Obviously I wasn't successful and probably wasted my time...
That's almost six weeks on the list. Also, many experienced audio developers helped shape the solution long before than that. About wasting time - sorry to hear, but dozen or so other reviewers from audio and other groups do not feel that way. Most of your comments have been applied, same as for other comments. The remaining points of yours are left at "I don't understand" point. Such reasoning cannot lead to solution being implemented in the incorrect manner or against the recommendations.
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 management that owns SKL/KBL support and the vast majority of audio developers disagrees with your opinion. All this work is to provide the best for the community and some fresh view on subjects that have been left unattanded for too long. Last three years was a battle to repair all the mistakes introduced in sound/soc/intel for the community and the clients alike. IPG and the surrounding support teams received an excellent opinion and reviews addressing all the problems.
The work in the SOF driver will continue regardless of what happens with this patchset, which I am not going to comment further on.
Cezary, I tried to help, didn't work, you're on your own now.
It's not "Cezary", it's Intel and IPG. Vast majority of developers is in favor of the decision made. The management is too. People found in SOF-framework team also commented and saw real, technical reasons behind this solution.
Best of luck.
It's not about luck. It's about professionalism and bringing the best to the community.
Regards, Czarek