Re: [RESEND PATCH v2 0/7] ASoC: Intel: Skylake: Driver fundaments overhaul

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 2019-07-23 20:07, Pierre-Louis Bossart wrote:
> On 7/23/19 10:44 AM, Mark Brown wrote:
>> On Tue, Jul 23, 2019 at 04:58:47PM +0200, Cezary Rojewski wrote:
>>> Skylake driver is divided into two modules:
>>> - snd_soc_skl
>>> - snd_soc_skl_ipc
>>
>> Pierre?
> 
> Sorry I was traveling and while I saw this series I never found the time 
> to review it.
> 
> I have really mixed feelings here and would like to make sure we are all 
> aligned on how we deal with the Skylake driver.
> 
> On one side I see that Cezary's team has done a genuine effort to 
> clean-up the code, show their technical skills, provide and listen to 
> review feedback, and improve the quality of upstream code. It wouldn't 
> be fair to shoot down well-intended developers who are making an honest 
> effort after years of embarrassing contributions. Intel and the Linux 
> community also have a shared interest in making sure newer kernel 
> versions improve quality and conversely that existing solutions can be 
> upgraded to improve security while also improving audio.

Thank you for the kind words.
This is not a charity, though. It's not just "Cezary's team". Throughout 
the healing process several teams have been engaged: SW Linux, SW 
Windows, FW, FDK, intel-next, system integration and even our clients. 
This is not to be treated as "wanna be". Skylake in current form is a 
disgrace to Intel's reputation. This mistake should be corrected, though 
we cannot do so without maintainers acceptance.

> On the other hand, I still see an opaque design (no obvious 
> core/platform split, mind-boggling use of topology with closed tools, 
> IPC that I still don't get), limited information on testing. I don't 
> think anyone challenges the fact that this driver is what it is, and not 
> the foundation for future upstream work. And there are about 100 
> additional clean-up/updates patches to be submitted for this driver, 
> which I don't personally have the time to look into since I am already 
> fully-booked on SoundWire work.
It's good that we agree on topology subject. It's questionable at best, 
scales poorly with newer FW releases. Quality of previous closed tools 
was on par with /skylake. Meaning there was none. These have been 
rewritten entirely, and yes it's still close source for now as without 
management agreement, my hands are tied from sharing them.

Design pattern differs from SOF one, it can be confusing if you always 
look at it from SOF perspective. There are no obvious splits - audio hw 
didn't really change that much and thus division is mainly motivated by 
DSP firmware capabilities.
Available are following buckets:
- SKL/ KBL/ KBL-R/ ABL (cAVS 1.5)
-> skl
- APL/ GLK (cAVS 1.5+)
-> skl -> bxt
- CNL/ CFL/ WHL/ CML (cAVS 1.8)
-> skl -> bxt -> cnl
- ICL/ LKF (cAVS 2.0)
- TGL/ EHL (cAVS 2.5)
-> skl -> bxt -> cnl -> icl
- more..

Each "-> xxx" denotes the xxx-sst and inheritance chain. "icl" segment 
not present on upstream. For most functionalities, DSP firmware inherits 
previous implementations in consequence making older xxx-ssts on 
software side valid too, even for topmost platforms. Reduces development 
burden, greatly.

Until core skylake is overhauled, I don't see the point of me stating: 
"tested on all buckets" - even though I do have these assembled. Will 
people believe me? Pretty sure each /skylake update in the past was 
prefixed with "tested on (...)" - and where did it lead us? Again, I 
prefer to do the ground work first, and yes we can help with CIs as we 
do have platforms connected to ours internally.

100 patches is probably an underestimation : )

> Overall my recommendations would be to:
> - give Cezary's team the benefit of the doubt for their Skylake reworks, 
> and add him as mandatory reviewer for the skylake parts. That doesn't 
> mean merging blindly but recognizing that no one else at Intel has a 
> better understanding of this code.

While not being bad myself, got the pleasure of working with best DSP 
guys in business at IGK, people I call friends. Due to the scale of a 
problem, before acting, one had to understand the history behind this. 
That took a lot of time - you can trust me on this : ). So many strings 
were pulled, so many people showed professionalism and helped us out. 
What I'm saying is despite the division which this disgrace of a 
"driver" caused, past months showed that when necessary we can unite and 
stand as One.

> - add CI support w/ Skylake devices so that we can have a better feel 
> for compilation/testing support. we've talked about having upstream 
> automatic build/hardware tests, maybe now is the time to do something 
> about it.
> - draw the line at "no new features" after e.g. 5.5 and "no new 
> platforms when SOF provides a solution". SOF was expected to reach 
> feature parity by the end of 2019 so it's not a random date I just made up.
> - I am even tempted to recommend de-featuring HDaudio codec support in 
> the Skylake driver since we already know of a broken probe that was 
> found on Linus' laptop and a slew of changes applied to SOF/legacy that 
> are missing in this driver.
> 
> Comments and feedback welcome.
> 
> -Pierre

While I can agree on the "no new features" line, the date is a loose 
subject. Honestly, I could've probably called first ~70-80 patches: "a 
fix". Validation team managed to mark half scenarios a failure 
immediately. Then developers were set loose. With enough motivation, we 
have managed to crash even the most simple scenarios. I do not call a 
folder with bunch of code not following any specification, design 
patter, lacking verification and testing and confirmed to be harmful a 
"driver". And thus, "new features" gets entirely different meaning when 
applied to /skylake.

Does not take a rocket scientist to realize the scale of a problem after 
reading commit msgs of recent series (and ones already applied). In the 
end, everything culminates with the broken architecture, which by now 
most should be aware of - based on DAPM and separated from standard PCM 
path. DAPM is a happy path, while IPCs can and do fail. Moreover, there 
are hw registers to be polled. TLDR: PCM code always assumes a success 
(it has to, after all DAPM path does not return err codes) what leads to 
undefined behavior in case of any failure of in preceding DAPM path. 
This is also why debugging /skylake is so complicated - people send us 
logs thinking: "this is the place!" when in fact the actual failure 
occurred much much earlier.

So, I leave you gentlemen with a decision to make: either there is an 
agreement and willingness to correct existing "driver", which requires a 
lot of effort (i.e.: patches) -or- it's left alone as is, dysfunctional.

And last, Pierre, I have a mixed feelings too - would like to enter 
Linux Kernel development in different circumstances. Some of us - 
including me - where even part of SOF early last year, but I believe 
there was real reason for pulling them out - /skylake has clients, it 
works there only because of heap of patches applied on top of it. 
Question is: should upstream be ignored?

Czarek