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
