Re: [RFC 00/37] ASoC: Intel: AVS - Audio DSP for cAVS

From: Cezary Rojewski <cezary.rojewski@intel.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	<alsa-devel@alsa-project.org>
Cc: upstream@semihalf.com, harshapriya.n@intel.com,
	yung-chuan.liao@linux.intel.com, rad@semihalf.com,
	tiwai@suse.com, hdegoede@redhat.com, broonie@kernel.org,
	ranjani.sridharan@linux.intel.com,
	amadeuszx.slawinski@linux.intel.com, cujomalainey@chromium.org,
	peter.ujfalusi@linux.intel.com, lma@semihalf.com
Subject: Re: [RFC 00/37] ASoC: Intel: AVS - Audio DSP for cAVS
Date: Thu, 9 Dec 2021 10:59:44 +0100	[thread overview]
Message-ID: <d569e8bd-a22b-bfc2-f799-ad63ea38c455@intel.com> (raw)
In-Reply-To: <59681534-aeee-003b-0fea-af17503bb813@linux.intel.com>

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