| Message ID | 20190723145854.8527-1-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | ASoC: Intel: Skylake: Driver fundaments overhaul | expand |
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?
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. 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. 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. - 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
On 23-07-19, 16:58, Cezary Rojewski wrote: > Skylake driver is divided into two modules: > - snd_soc_skl > - snd_soc_skl_ipc > > and nothing would be wrong if not for the fact that both cannot exist > without one another. IPC module is not some kind of extension, as it is > the case for snd_hda_ext_core which is separated from snd_hda_core - > legacy hda interface. It's as much core Skylake module as snd_soc_skl > is. Well it is an extension as it represents the view of the firmware and we always had moving IPCs! Even when skylake was developed we had two different versions and even when i left we had two (sof being other one at that time) The reason for split was to ensure we can modularize lower level IPCs. We have hardware layouts change, fw formats change so it helped if that route was again taken! IIRC even the IPC was built on generic IPC work by Liam, so yes there are layers but for a reason. If you feel merging the two helps, I am okay with the change. > Statement backup by existence of circular dependency between this two. > To eliminate said problem, struct skl_sst has been created. From that > momment, Skylake has been plagued by header errors (incomplete sturcts, > unknown references etc.) whenever something new is to be added or code > is cleaned up. Any reason why new is being added here, aren't you guys moving to SOF? > Fix this flawed design by merging snd_soc_skl and snd_soc_skl_ipc. > Also, do not forget about struct skl_sst redundancy. > Followup changes address harmful assumptions and false logic which > driver currently implements e.g.: attempt to take role of master for > DSP scheduling when in fact entire control takes place in DSP. Where is the basis of that assumption, driver was a mere accomplice, getting the data of topology and passing it down to firmware, whilst try to do some book keeping and keep things for falling! > > Changes since v1: > - Rebased onto 5.4 > > Amadeusz Sławiński (2): > ASoC: Intel: Skylake: Combine snd_soc_skl_ipc and snd_soc_skl > ASoC: Intel: Skylake: Do not disable FW notifications > > Cezary Rojewski (5): > ASoC: Intel: Skylake: Merge skl_sst and skl into skl_dev struct > ASoC: Intel: Skylake: Remove MCPS available check > ASoC: Intel: Skylake: Remove memory available check > ASoC: Intel: Skylake: Make MCPS and CPS params obsolete > ASoC: Intel: Skylake: Cleanup skl_module_cfg declaration > > sound/soc/intel/common/sst-ipc.h | 1 + > sound/soc/intel/skylake/Makefile | 12 +- > sound/soc/intel/skylake/bxt-sst.c | 50 +-- > sound/soc/intel/skylake/cnl-sst-dsp.h | 7 +- > sound/soc/intel/skylake/cnl-sst.c | 37 +- > sound/soc/intel/skylake/skl-debug.c | 14 +- > sound/soc/intel/skylake/skl-messages.c | 245 ++++++------- > sound/soc/intel/skylake/skl-nhlt.c | 18 +- > sound/soc/intel/skylake/skl-pcm.c | 74 ++-- > sound/soc/intel/skylake/skl-ssp-clk.c | 4 +- > sound/soc/intel/skylake/skl-sst-dsp.c | 10 +- > sound/soc/intel/skylake/skl-sst-dsp.h | 29 +- > sound/soc/intel/skylake/skl-sst-ipc.c | 8 +- > sound/soc/intel/skylake/skl-sst-ipc.h | 52 +-- > sound/soc/intel/skylake/skl-sst-utils.c | 37 +- > sound/soc/intel/skylake/skl-sst.c | 51 +-- > sound/soc/intel/skylake/skl-topology.c | 441 ++++++++---------------- > sound/soc/intel/skylake/skl-topology.h | 43 +-- > sound/soc/intel/skylake/skl.c | 54 +-- > sound/soc/intel/skylake/skl.h | 102 ++++-- > 20 files changed, 546 insertions(+), 743 deletions(-) > > -- > 2.17.1 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 23-07-19, 16:58, Cezary Rojewski wrote: > Skylake driver is divided into two modules: > - snd_soc_skl > - snd_soc_skl_ipc > > and nothing would be wrong if not for the fact that both cannot exist > without one another. IPC module is not some kind of extension, as it is > the case for snd_hda_ext_core which is separated from snd_hda_core - > legacy hda interface. It's as much core Skylake module as snd_soc_skl > is. > > Statement backup by existence of circular dependency between this two. > To eliminate said problem, struct skl_sst has been created. From that > momment, Skylake has been plagued by header errors (incomplete sturcts, > unknown references etc.) whenever something new is to be added or code > is cleaned up. > > Fix this flawed design by merging snd_soc_skl and snd_soc_skl_ipc. > Also, do not forget about struct skl_sst redundancy. > Followup changes address harmful assumptions and false logic which > driver currently implements e.g.: attempt to take role of master for > DSP scheduling when in fact entire control takes place in DSP. > > Changes since v1: > - Rebased onto 5.4 On a lighter note: Did this series time travel from the future! Did you charge the flux capacitor to go back? > > Amadeusz Sławiński (2): > ASoC: Intel: Skylake: Combine snd_soc_skl_ipc and snd_soc_skl > ASoC: Intel: Skylake: Do not disable FW notifications > > Cezary Rojewski (5): > ASoC: Intel: Skylake: Merge skl_sst and skl into skl_dev struct > ASoC: Intel: Skylake: Remove MCPS available check > ASoC: Intel: Skylake: Remove memory available check > ASoC: Intel: Skylake: Make MCPS and CPS params obsolete > ASoC: Intel: Skylake: Cleanup skl_module_cfg declaration > > sound/soc/intel/common/sst-ipc.h | 1 + > sound/soc/intel/skylake/Makefile | 12 +- > sound/soc/intel/skylake/bxt-sst.c | 50 +-- > sound/soc/intel/skylake/cnl-sst-dsp.h | 7 +- > sound/soc/intel/skylake/cnl-sst.c | 37 +- > sound/soc/intel/skylake/skl-debug.c | 14 +- > sound/soc/intel/skylake/skl-messages.c | 245 ++++++------- > sound/soc/intel/skylake/skl-nhlt.c | 18 +- > sound/soc/intel/skylake/skl-pcm.c | 74 ++-- > sound/soc/intel/skylake/skl-ssp-clk.c | 4 +- > sound/soc/intel/skylake/skl-sst-dsp.c | 10 +- > sound/soc/intel/skylake/skl-sst-dsp.h | 29 +- > sound/soc/intel/skylake/skl-sst-ipc.c | 8 +- > sound/soc/intel/skylake/skl-sst-ipc.h | 52 +-- > sound/soc/intel/skylake/skl-sst-utils.c | 37 +- > sound/soc/intel/skylake/skl-sst.c | 51 +-- > sound/soc/intel/skylake/skl-topology.c | 441 ++++++++---------------- > sound/soc/intel/skylake/skl-topology.h | 43 +-- > sound/soc/intel/skylake/skl.c | 54 +-- > sound/soc/intel/skylake/skl.h | 102 ++++-- > 20 files changed, 546 insertions(+), 743 deletions(-) > > -- > 2.17.1 > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, Jul 23, 2019 at 01:07:31PM -0500, Pierre-Louis Bossart wrote: > 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. This seems like a good idea, Cezary could you send a patch adding yourself to MAINTAINERS please? > - 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. Let's review that nearer the time? I don't want to impose a deadline in advance and have people needlessly rushing to hit that deadline when things might wind down themselves naturally.
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
On Wed, Jul 24, 2019 at 07:14:52PM +0200, Cezary Rojewski wrote: > On 2019-07-23 20:07, Pierre-Louis Bossart wrote: > > - 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. > 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. If there's things that fix bugs then they won't be covered by any wind down in new features so that's a separate thing whatever happens there.
On 2019-07-24 14:42, Vinod Koul wrote: > On 23-07-19, 16:58, Cezary Rojewski wrote: >> Changes since v1: >> - Rebased onto 5.4 > > On a lighter note: > > Did this series time travel from the future! Did you charge the flux > capacitor to go back? > Damn, seems MLs are still merciless : ) Sorry for the typo - while doing rebases on "for-5.4" internally for future releases, simply smudged my commit messages.