| Message ID | 20201011095346.49589-1-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: Intel: Do not load legacy SST driver on BYT when SND_SOC_SOF_BAYTRAIL is enabled | expand |
On 2020-10-11 11:53 AM, Hans de Goede wrote: > The legacy 80860F28 / sst_acpi_baytrail_desc match in sst_acpi_match > is already conditional on the the newer SND_SST_IPC_ACPI driver not > being enabled. > > But now that we have an even newer driver in the form of SOF support > for BYT devices, we also need to take this into account, so we also > must not include the sst_acpi_baytrail_desc match when > SND_SOC_SOF_BAYTRAIL is enabled. > > This fixes snd-soc-sst-acpi binding to the 80860F28 platform device, > blocking snd-sof-acpi from binding, which breaks audio support. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- Hello, Series: [PATCH v2 00/13] ASoC: Intel: Remove obsolete solutions and components https://lore.kernel.org/alsa-devel/20201006064907.16277-1-cezary.rojewski@intel.com/ removes sst-acpi component along with many others so further changes to said component will only cause conflicts -or- require commit reordering. I'd advice against that. Thanks, Czarek
Hi, On 10/12/20 9:24 AM, Rojewski, Cezary wrote: > On 2020-10-11 11:53 AM, Hans de Goede wrote: >> The legacy 80860F28 / sst_acpi_baytrail_desc match in sst_acpi_match >> is already conditional on the the newer SND_SST_IPC_ACPI driver not >> being enabled. >> >> But now that we have an even newer driver in the form of SOF support >> for BYT devices, we also need to take this into account, so we also >> must not include the sst_acpi_baytrail_desc match when >> SND_SOC_SOF_BAYTRAIL is enabled. >> >> This fixes snd-soc-sst-acpi binding to the 80860F28 platform device, >> blocking snd-sof-acpi from binding, which breaks audio support. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- > > Hello, > > Series: > [PATCH v2 00/13] ASoC: Intel: Remove obsolete solutions and components > https://lore.kernel.org/alsa-devel/20201006064907.16277-1-cezary.rojewski@intel.com/ > > removes sst-acpi component along with many others so further changes to > said component will only cause conflicts -or- require commit reordering. > I'd advice against that. As I already mentioned in the private-thread which Pierre-Louis started with me, Jaroslav Kysela and Liam about this I would advice against applying that series for now. First we need to put in more work to make sure that the new drivers are actually ready. Also I must say that I'm quite disappointed that since I, as the person who more or less single handedly have made sure that audio works properly o Bay Trail and Cherry Traul devices (*), has not been Cc-ed on that series, that seems like a huge oversight. Anyways I will reply in the thread of the series and ask Mark to revert the entire series. Since IMHO the new drivers are clearly not ready yet. Yesterday I ran my first set of tested and I immediately hit a DSP hang doing just a few very basic tests. Regards, Hans *) And kept it working properly despite other people breaking it with changes like moving the userspace stuff to UCM2.
On 2020-10-12 9:42 AM, Hans de Goede wrote: > Hi, > > On 10/12/20 9:24 AM, Rojewski, Cezary wrote: ... >> >> Hello, >> >> Series: >> [PATCH v2 00/13] ASoC: Intel: Remove obsolete solutions and components >> https://lore.kernel.org/alsa-devel/20201006064907.16277-1-cezary.rojewski@intel.com/ >> >> >> removes sst-acpi component along with many others so further changes to >> said component will only cause conflicts -or- require commit reordering. >> I'd advice against that. > > As I already mentioned in the private-thread which Pierre-Louis started > with me, Jaroslav Kysela and Liam about this I would advice against > applying > that series for now. First we need to put in more work to make sure that > the new drivers are actually ready. > > Also I must say that I'm quite disappointed that since I, as the person > who more or less single handedly have made sure that audio works properly o > Bay Trail and Cherry Traul devices (*), has not been Cc-ed on that series, > that seems like a huge oversight. > > Anyways I will reply in the thread of the series and ask Mark to revert > the entire series. Since IMHO the new drivers are clearly not ready yet. > Yesterday I ran my first set of tested and I immediately hit a DSP > hang doing just a few very basic tests. > > Regards, > > Hans > > > > *) And kept it working properly despite other people breaking it with > changes > like moving the userspace stuff to UCM2. > Hello, What's the name of the private-thread? Or perhaps I'm not even invited there? Please, elaborate "new drivers". /baytrail/ has been deprecated for years with only two available boards (machine boards) to it - which are somewhat duplicates of /atom/ -or- SOF equivalents (bytcr-xxxx). From linux-kernel perspective, having 3x baytrail driver is simply bad. Several teams, clients and groups have been asked on multiple occasions about the usage of the /baytrail/ folder. Not once positive answer has been given. Thanks, Czarek
Hi Czarek, On 10/12/20 9:58 AM, Rojewski, Cezary wrote: > On 2020-10-12 9:42 AM, Hans de Goede wrote: >> Hi, >> >> On 10/12/20 9:24 AM, Rojewski, Cezary wrote: > > ... > >>> >>> Hello, >>> >>> Series: >>> [PATCH v2 00/13] ASoC: Intel: Remove obsolete solutions and components >>> https://lore.kernel.org/alsa-devel/20201006064907.16277-1-cezary.rojewski@intel.com/ >>> >>> >>> removes sst-acpi component along with many others so further changes to >>> said component will only cause conflicts -or- require commit reordering. >>> I'd advice against that. >> >> As I already mentioned in the private-thread which Pierre-Louis started >> with me, Jaroslav Kysela and Liam about this I would advice against >> applying >> that series for now. First we need to put in more work to make sure that >> the new drivers are actually ready. >> >> Also I must say that I'm quite disappointed that since I, as the person >> who more or less single handedly have made sure that audio works properly o >> Bay Trail and Cherry Traul devices (*), has not been Cc-ed on that series, >> that seems like a huge oversight. >> >> Anyways I will reply in the thread of the series and ask Mark to revert >> the entire series. Since IMHO the new drivers are clearly not ready yet. >> Yesterday I ran my first set of tested and I immediately hit a DSP >> hang doing just a few very basic tests. >> >> Regards, >> >> Hans >> >> >> >> *) And kept it working properly despite other people breaking it with >> changes >> like moving the userspace stuff to UCM2. >> > > Hello, > > What's the name of the private-thread? Or perhaps I'm not even invited > there? You were not on the Cc when Pierre-Louis started the thread, I'll try to remember to Cc you on further replies. I'll give a summary at the end of this email, since this is probably useful info for everyone reading along to have. > Please, elaborate "new drivers". /baytrail/ has been deprecated for > years with only two available boards (machine boards) to it - which are > somewhat duplicates of /atom/ -or- SOF equivalents (bytcr-xxxx). From > linux-kernel perspective, having 3x baytrail driver is simply bad. Ah, sorry I think I jumped the gun a bit on becoming grumpy about this. On second reading I see you are removing the old-old Bay Trail code, while keeping the medium-old (CONFIG_SND_SST_IPC_ACPI) code around for now, correct ? Yes that should be fine. One request though in the future please Cc me on (non trivial) changes impacting Bay and Cherry Trail devices. > Several teams, clients and groups have been asked on multiple occasions > about the usage of the /baytrail/ folder. Not once positive answer has > been given. Right, I don't know about other distros but in Fedora we have had the use of the old sound/soc/intel/common/sst-acpi.c code disabled for BYT/CHT for a while now. Note Fedora does have the common/sst-acpi.c Broadwell / Haswell bits enabled all the way up to kernel 5.9, but lets discuss that in the thread where you remove the common/sst-acpi.c code. Regards, Hans p.s. The promised summary: Pierre-Louis contacted me about moving BYT/CHT devices to the SOF driver so that the medium-old / CONFIG_SND_SST_IPC_ACPI drivers can eventually also be removed. I agreed with that plan, but I was and still am against doing it immediately as I want to first run a set of tests to make sure the switch will go smoothly. The Bay / Cherry Trail work I do is a personal-time side project, which means mostly working on it in the weekend. As discussed in the off-list discussion at a minimum I would like to run the following setups: Realtek codecs: BYT(CR) RT5640 SSP0 AIF1 BYT(CR) RT5640 SSP0 AIF2 BYT RT5640 SSP2 CHT RT5640 (HP pavilion X2 10-p002nd uses this weird combo) CHT RT5645 BYT(CR) RT5651 CHT RT5651 BYT RT5672 Other: BYT(CR) ESS8316 CHT ESS8316 CHT NAU8824 Through the following test plan: 1. Test speakers 2. Test internal mic. 3. Plugin headset, test headphones 4. Test headset-mic 5. Stop all audio, suspend + resume, test speakers 6. suspend + resume while playing audio, audio should resume playing after resume. This weekend I ran the test-plan on the first setup and at step 3 (a couple of minutes into testing) I hit a DSP hang (which I could not reproduce). Other then the hang the testing went smooth. We will need to see if the hang was a glitch or if I will hit it more often when I test the other setups. I have dmesg output from the hang, if someone is interested.
diff --git a/sound/soc/intel/common/sst-acpi.c b/sound/soc/intel/common/sst-acpi.c index 5854868650b9..b4b2e1e0e845 100644 --- a/sound/soc/intel/common/sst-acpi.c +++ b/sound/soc/intel/common/sst-acpi.c @@ -198,7 +198,7 @@ static struct sst_acpi_desc sst_acpi_broadwell_desc = { .dma_size = SST_LPT_DSP_DMA_SIZE, }; -#if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI) +#if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI) && !IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) static struct sst_acpi_desc sst_acpi_baytrail_desc = { .drv_name = "baytrail-pcm-audio", .machines = snd_soc_acpi_intel_baytrail_legacy_machines, @@ -214,7 +214,7 @@ static struct sst_acpi_desc sst_acpi_baytrail_desc = { static const struct acpi_device_id sst_acpi_match[] = { { "INT33C8", (unsigned long)&sst_acpi_haswell_desc }, { "INT3438", (unsigned long)&sst_acpi_broadwell_desc }, -#if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI) +#if !IS_ENABLED(CONFIG_SND_SST_IPC_ACPI) && !IS_ENABLED(CONFIG_SND_SOC_SOF_BAYTRAIL) { "80860F28", (unsigned long)&sst_acpi_baytrail_desc }, #endif { }
The legacy 80860F28 / sst_acpi_baytrail_desc match in sst_acpi_match is already conditional on the the newer SND_SST_IPC_ACPI driver not being enabled. But now that we have an even newer driver in the form of SOF support for BYT devices, we also need to take this into account, so we also must not include the sst_acpi_baytrail_desc match when SND_SOC_SOF_BAYTRAIL is enabled. This fixes snd-soc-sst-acpi binding to the 80860F28 platform device, blocking snd-sof-acpi from binding, which breaks audio support. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- sound/soc/intel/common/sst-acpi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)