[PATCH 0/7] ASoC: Intel: Skylake: Fix HDaudio and Dmic
Following is the list of fixes and updates targeting HDaudio +/- Dmic configuration on Intel DSP platforms.
- ASoC: Intel: Skylake: Remove superfluous chip initialization ASoC: Intel: Skylake: Select hda configuration permissively
First patch addresses race condition issue between i915 and hda controller. This is done by yielding priority to i915 so iDisp codec can enumerate properly: same codec_mask is now observed regardless of driver chosen (snd_hda_intel vs snd_soc_skl).
Second patch is a consequence of the first, to prevent driver from incorrectly aborting probe - rather than reorganizing Skylake's boot flow, small changed has been proposed.
- ASoC: Intel: Skylake: Shield against no-NHLT configurations
Some hardware has no NHLT exposed by BIOS (or an equivalent). Changes have been made to ensure driver is shielded against null-dereferences and such which occur when said table is absent.
- ASoC: Intel: skl_hda_dsp: Enable Dmic configuration
While DMIC is available on some production stuff, Intel platforms with Skylake driver do not treat it as a valid option if no additional I2S codec in present onboard. Update skl_hda_dsp board to expose Dmic connections too.
- ASoC: Intel: Allow for ROM init retry on CNL platforms ASoC: Intel: Skylake: Await purge request ack on CNL
Both address rom init timeouts during CNL/ CFL/ CML/ WHL boot up sequences. These provide retry mechanism and ensure purge request is acked before proceding with FW load. bxt-sst.c has had these fixes appended long ago - somehow someone forgotten about CNL family.
*****
Note: topology update is also needed to enable HDA +/- Dmic configuration as existing ones do not contain any routes or widgets required to enable it - these care about I2S only. We have prepared corresponding UCM files too. Will be sharing them shortly.
This patchset has been prepared internally for topmost linux-stable 5.5 and 4.20 (no 4.19 as skl_hda_dsp did not exist there yet).
Apart from our RVPs, we have run tests also on: - KBL Lenovo Carbon X1 - SKL Dell XPS 9350 - WHL Acer Swift 5
Honestly, I'd see HDaudio related patches being backport as low as 4.20 (although some changes had to be adjusted due to base differences between 4.20 and 5.5, can share these too). One could argue HDA + Dmic configuration should be available on 4.19 too - it's an LTS after all. However, that time, some changes could be counted as "feature" rather than fixes. Awaiting your replies and thoughts on that.
In consequence, I've appended "Fixes" only for last two patches for now - once decisions are made, can append adequate tags wherever necessary.
Cezary Rojewski (6): ASoC: Intel: Skylake: Remove superfluous chip initialization ASoC: Intel: Skylake: Select hda configuration permissively ASoC: Intel: Skylake: Enable codec wakeup during chip init ASoC: Intel: Skylake: Shield against no-NHLT configurations ASoC: Intel: Allow for ROM init retry on CNL platforms ASoC: Intel: Skylake: Await purge request ack on CNL
Mateusz Gorski (1): ASoC: Intel: skl_hda_dsp: Enable Dmic configuration
sound/soc/intel/boards/skl_hda_dsp_generic.c | 3 ++ sound/soc/intel/skylake/bxt-sst.c | 3 -- sound/soc/intel/skylake/cnl-sst.c | 35 ++++++++++++++++---- sound/soc/intel/skylake/skl-nhlt.c | 3 +- sound/soc/intel/skylake/skl-sst-dsp.h | 2 ++ sound/soc/intel/skylake/skl.c | 29 ++++++++-------- 6 files changed, 48 insertions(+), 27 deletions(-)
Skylake driver does the controller init operation twice: - first during probe (only to stop it just before scheduling probe_work) - and during said probe_work where the actual correct sequence is executed
To properly complete boot sequence when iDisp codec is present, bus initialization has to be called only after _i915_init() finishes. With additional _reset_list preceding _i915_init(), iDisp codec never gets the chance to enumerate on the link. Remove the superfluous initialization to address the issue.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/skylake/skl.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index f755ca2484cf..d66231525356 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -803,6 +803,9 @@ static void skl_probe_work(struct work_struct *work) return; }
+ skl_init_pci(skl); + skl_dum_set(bus); + err = skl_init_chip(bus, true); if (err < 0) { dev_err(bus->dev, "Init chip failed with err: %d\n", err); @@ -918,8 +921,6 @@ static int skl_first_init(struct hdac_bus *bus) return -ENXIO; }
- snd_hdac_bus_reset_link(bus, true); - snd_hdac_bus_parse_capabilities(bus);
/* check if PPCAP exists */ @@ -967,11 +968,7 @@ static int skl_first_init(struct hdac_bus *bus) if (err < 0) return err;
- /* initialize chip */ - skl_init_pci(skl); - skl_dum_set(bus); - - return skl_init_chip(bus, true); + return 0; }
static int skl_probe(struct pci_dev *pci, @@ -1064,8 +1061,6 @@ static int skl_probe(struct pci_dev *pci, if (bus->mlcap) snd_hdac_ext_bus_get_ml_capabilities(bus);
- snd_hdac_bus_stop_chip(bus); - /* create device for soc dmic */ err = skl_dmic_device_register(skl); if (err < 0) {
On 3/5/20 8:53 AM, Cezary Rojewski wrote:
Skylake driver does the controller init operation twice:
- first during probe (only to stop it just before scheduling probe_work)
- and during said probe_work where the actual correct sequence is
executed
To properly complete boot sequence when iDisp codec is present, bus initialization has to be called only after _i915_init() finishes. With additional _reset_list preceding _i915_init(), iDisp codec never gets the chance to enumerate on the link. Remove the superfluous initialization to address the issue.
Have you tested with with DRM built-in and as a module? that was enough to trigger race conditions in the past on Dell XPS9350.
On 2020-03-06 21:52, Pierre-Louis Bossart wrote:
On 3/5/20 8:53 AM, Cezary Rojewski wrote:
Skylake driver does the controller init operation twice:
- first during probe (only to stop it just before scheduling probe_work)
- and during said probe_work where the actual correct sequence is
executed
To properly complete boot sequence when iDisp codec is present, bus initialization has to be called only after _i915_init() finishes. With additional _reset_list preceding _i915_init(), iDisp codec never gets the chance to enumerate on the link. Remove the superfluous initialization to address the issue.
Have you tested with with DRM built-in and as a module? that was enough to trigger race conditions in the past on Dell XPS9350.
DRM is quite a tree, you got to be more specific. Tested with i915=m and DRM=m. I hope we mean the same thing when mentioning 'race'. There is an obvious initialization race between hda bus drv and i915 which requires one to follow a tight operation order in order to not lose i915 codec on hda link and thus be able to enumerate it properly.
On top of that, as you mentioned (by the link) this series addresses missing DMIC configuration in conjunction with HDA +/- iDisp AND shields against no-NHLT configuration. On Dell XPS 9350 lack on of NHLT was the biggest problem - that's why I'd like that issue not to be forgotten about.
Czarek
On 3/9/20 8:57 AM, Cezary Rojewski wrote:
On 2020-03-06 21:52, Pierre-Louis Bossart wrote:
On 3/5/20 8:53 AM, Cezary Rojewski wrote:
Skylake driver does the controller init operation twice:
- first during probe (only to stop it just before scheduling probe_work)
- and during said probe_work where the actual correct sequence is
executed
To properly complete boot sequence when iDisp codec is present, bus initialization has to be called only after _i915_init() finishes. With additional _reset_list preceding _i915_init(), iDisp codec never gets the chance to enumerate on the link. Remove the superfluous initialization to address the issue.
Have you tested with with DRM built-in and as a module? that was enough to trigger race conditions in the past on Dell XPS9350.
DRM is quite a tree, you got to be more specific. Tested with i915=m and DRM=m. I hope we mean the same thing when mentioning 'race'. There is an obvious initialization race between hda bus drv and i915 which requires one to follow a tight operation order in order to not lose i915 codec on hda link and thus be able to enumerate it properly.
I meant CONFIG_DRM=m, yes, thanks for the clarification.
With the DRM as module, it took more time to establish the communication. That's probably changed if we do all the inits in a workqueue now.
On top of that, as you mentioned (by the link) this series addresses missing DMIC configuration in conjunction with HDA +/- iDisp AND shields against no-NHLT configuration. On Dell XPS 9350 lack on of NHLT was the biggest problem - that's why I'd like that issue not to be forgotten about.
yes, but we don't want the driver to be auto-selected on SKL w/o DMICs, since it'd break existing devices who don't have a topology file installed.
On 2020-03-09 17:48, Pierre-Louis Bossart wrote:
DRM is quite a tree, you got to be more specific. Tested with i915=m and DRM=m. I hope we mean the same thing when mentioning 'race'. There is an obvious initialization race between hda bus drv and i915 which requires one to follow a tight operation order in order to not lose i915 codec on hda link and thus be able to enumerate it properly.
I meant CONFIG_DRM=m, yes, thanks for the clarification.
With the DRM as module, it took more time to establish the communication. That's probably changed if we do all the inits in a workqueue now.
So, does the DRM=m & i915=m test satisfy you?
On top of that, as you mentioned (by the link) this series addresses missing DMIC configuration in conjunction with HDA +/- iDisp AND shields against no-NHLT configuration. On Dell XPS 9350 lack on of NHLT was the biggest problem - that's why I'd like that issue not to be forgotten about.
yes, but we don't want the driver to be auto-selected on SKL w/o DMICs, since it'd break existing devices who don't have a topology file installed.
My patch does not touch any of kconfig stuff. I'd prefer anything that goes into disable/enable kconfig bucket to be split into a separate patch. Don't believe SKL somehow gets automatically selected on these. On the machines I've tested fixes with, legacy HDaudio driver was the primary option/ drv.
On 3/9/20 12:43 PM, Cezary Rojewski wrote:
On 2020-03-09 17:48, Pierre-Louis Bossart wrote:
DRM is quite a tree, you got to be more specific. Tested with i915=m and DRM=m. I hope we mean the same thing when mentioning 'race'. There is an obvious initialization race between hda bus drv and i915 which requires one to follow a tight operation order in order to not lose i915 codec on hda link and thus be able to enumerate it properly.
I meant CONFIG_DRM=m, yes, thanks for the clarification.
With the DRM as module, it took more time to establish the communication. That's probably changed if we do all the inits in a workqueue now.
So, does the DRM=m & i915=m test satisfy you?
Yes, that's all I wanted to confirm.
On top of that, as you mentioned (by the link) this series addresses missing DMIC configuration in conjunction with HDA +/- iDisp AND shields against no-NHLT configuration. On Dell XPS 9350 lack on of NHLT was the biggest problem - that's why I'd like that issue not to be forgotten about.
yes, but we don't want the driver to be auto-selected on SKL w/o DMICs, since it'd break existing devices who don't have a topology file installed.
My patch does not touch any of kconfig stuff. I'd prefer anything that goes into disable/enable kconfig bucket to be split into a separate patch. Don't believe SKL somehow gets automatically selected on these. On the machines I've tested fixes with, legacy HDaudio driver was the primary option/ drv.
The patch
ASoC: Intel: Skylake: Remove superfluous chip initialization
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 2ef81057d80456870b97890dd79c8f56a85b1242 Mon Sep 17 00:00:00 2001
From: Cezary Rojewski cezary.rojewski@intel.com Date: Thu, 5 Mar 2020 15:53:08 +0100 Subject: [PATCH] ASoC: Intel: Skylake: Remove superfluous chip initialization
Skylake driver does the controller init operation twice: - first during probe (only to stop it just before scheduling probe_work) - and during said probe_work where the actual correct sequence is executed
To properly complete boot sequence when iDisp codec is present, bus initialization has to be called only after _i915_init() finishes. With additional _reset_list preceding _i915_init(), iDisp codec never gets the chance to enumerate on the link. Remove the superfluous initialization to address the issue.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200305145314.32579-2-cezary.rojewski@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/skl.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index f755ca2484cf..d66231525356 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -803,6 +803,9 @@ static void skl_probe_work(struct work_struct *work) return; }
+ skl_init_pci(skl); + skl_dum_set(bus); + err = skl_init_chip(bus, true); if (err < 0) { dev_err(bus->dev, "Init chip failed with err: %d\n", err); @@ -918,8 +921,6 @@ static int skl_first_init(struct hdac_bus *bus) return -ENXIO; }
- snd_hdac_bus_reset_link(bus, true); - snd_hdac_bus_parse_capabilities(bus);
/* check if PPCAP exists */ @@ -967,11 +968,7 @@ static int skl_first_init(struct hdac_bus *bus) if (err < 0) return err;
- /* initialize chip */ - skl_init_pci(skl); - skl_dum_set(bus); - - return skl_init_chip(bus, true); + return 0; }
static int skl_probe(struct pci_dev *pci, @@ -1064,8 +1061,6 @@ static int skl_probe(struct pci_dev *pci, if (bus->mlcap) snd_hdac_ext_bus_get_ml_capabilities(bus);
- snd_hdac_bus_stop_chip(bus); - /* create device for soc dmic */ err = skl_dmic_device_register(skl); if (err < 0) {
With _reset_link removed from the probe sequence, codec_mask at the time skl_find_hda_machine() is invoked will always be 0, so hda machine will never be chosen. Rather than reorganizing boot flow, be permissive about invalid mask. codec_mask will be set to proper value during probe_work - before skl_codec_create() ever gets called.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/skylake/skl.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index d66231525356..4827fe6bc1cb 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -481,13 +481,8 @@ static struct skl_ssp_clk skl_ssp_clks[] = { static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl_dev *skl, struct snd_soc_acpi_mach *machines) { - struct hdac_bus *bus = skl_to_bus(skl); struct snd_soc_acpi_mach *mach;
- /* check if we have any codecs detected on bus */ - if (bus->codec_mask == 0) - return NULL; - /* point to common table */ mach = snd_soc_acpi_intel_hda_machines;
On 3/5/20 8:53 AM, Cezary Rojewski wrote:
With _reset_link removed from the probe sequence, codec_mask at the time skl_find_hda_machine() is invoked will always be 0, so hda machine will never be chosen. Rather than reorganizing boot flow, be permissive about invalid mask. codec_mask will be set to proper value during probe_work - before skl_codec_create() ever gets called.
humm, what would happen e.g. if you have select the SKL driver but there is no ACPI information to select an I2S-based machine driver, and HDaudio/iDISP are disabled? You would have no error checks then?
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com
sound/soc/intel/skylake/skl.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index d66231525356..4827fe6bc1cb 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -481,13 +481,8 @@ static struct skl_ssp_clk skl_ssp_clks[] = { static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl_dev *skl, struct snd_soc_acpi_mach *machines) {
struct hdac_bus *bus = skl_to_bus(skl); struct snd_soc_acpi_mach *mach;
/* check if we have any codecs detected on bus */
if (bus->codec_mask == 0)
return NULL;/* point to common table */ mach = snd_soc_acpi_intel_hda_machines;
On 2020-03-06 21:57, Pierre-Louis Bossart wrote:
On 3/5/20 8:53 AM, Cezary Rojewski wrote:
With _reset_link removed from the probe sequence, codec_mask at the time skl_find_hda_machine() is invoked will always be 0, so hda machine will never be chosen. Rather than reorganizing boot flow, be permissive about invalid mask. codec_mask will be set to proper value during probe_work - before skl_codec_create() ever gets called.
humm, what would happen e.g. if you have select the SKL driver but there is no ACPI information to select an I2S-based machine driver, and HDaudio/iDISP are disabled? You would have no error checks then?
Laptops I've been testing this with have had Realtek + iDisp present onboard. Now, if you disable Realtek + HDMI/DP modules within legacy HDaudio Kconfig and HD audio support within Intel Skylake tree then you end up with no required modules for said configuration at all. Nothing will happen really: no warnings, no sound card either.
Just run such kernel on my setup and results are quite obvious: - skl boots - no machines present - drv stays dormant
Czarek
On 3/9/20 8:47 AM, Cezary Rojewski wrote:
On 2020-03-06 21:57, Pierre-Louis Bossart wrote:
On 3/5/20 8:53 AM, Cezary Rojewski wrote:
With _reset_link removed from the probe sequence, codec_mask at the time skl_find_hda_machine() is invoked will always be 0, so hda machine will never be chosen. Rather than reorganizing boot flow, be permissive about invalid mask. codec_mask will be set to proper value during probe_work - before skl_codec_create() ever gets called.
humm, what would happen e.g. if you have select the SKL driver but there is no ACPI information to select an I2S-based machine driver, and HDaudio/iDISP are disabled? You would have no error checks then?
Laptops I've been testing this with have had Realtek + iDisp present onboard. Now, if you disable Realtek + HDMI/DP modules within legacy HDaudio Kconfig and HD audio support within Intel Skylake tree then you end up with no required modules for said configuration at all. Nothing will happen really: no warnings, no sound card either.
I meant enable the HDaudio controller but disable HDaudio codecs/HDMI at the BIOS level. In that case the codec_mask will never be set.
Just run such kernel on my setup and results are quite obvious:
- skl boots
- no machines present
- drv stays dormant
Czarek
On 2020-03-09 18:03, Pierre-Louis Bossart wrote:
On 3/9/20 8:47 AM, Cezary Rojewski wrote:
On 2020-03-06 21:57, Pierre-Louis Bossart wrote:
On 3/5/20 8:53 AM, Cezary Rojewski wrote:
With _reset_link removed from the probe sequence, codec_mask at the time skl_find_hda_machine() is invoked will always be 0, so hda machine will never be chosen. Rather than reorganizing boot flow, be permissive about invalid mask. codec_mask will be set to proper value during probe_work - before skl_codec_create() ever gets called.
humm, what would happen e.g. if you have select the SKL driver but there is no ACPI information to select an I2S-based machine driver, and HDaudio/iDISP are disabled? You would have no error checks then?
Laptops I've been testing this with have had Realtek + iDisp present onboard. Now, if you disable Realtek + HDMI/DP modules within legacy HDaudio Kconfig and HD audio support within Intel Skylake tree then you end up with no required modules for said configuration at all. Nothing will happen really: no warnings, no sound card either.
I meant enable the HDaudio controller but disable HDaudio codecs/HDMI at the BIOS level. In that case the codec_mask will never be set.
Applying diff (better than BIOS options which are not even present on some prod machines):
diff --git a/sound/hda/hdac_controller.c b/sound/hda/hdac_controller.c index 7e7be8e4dcf9..b0e4579f26f6 100644 --- a/sound/hda/hdac_controller.c +++ b/sound/hda/hdac_controller.c @@ -421,7 +421,8 @@ int snd_hdac_bus_reset_link(struct hdac_bus *bus, bool full_reset)
/* detect codecs */ if (!bus->codec_mask) { - bus->codec_mask = snd_hdac_chip_readw(bus, STATESTS); + //bus->codec_mask = snd_hdac_chip_readw(bus, STATESTS); + bus->codec_mask = 0; dev_dbg(bus->dev, "codec_mask = 0x%lx\n", bus->codec_mask); }
---
Results: - skl boots - no machines present - drv stays dormant
Dumping boot log below (notice the '[ 21.569291] snd_soc_skl 0000:00:1f.3: codec_mask = 0x0' message):
[ 21.462507] snd_soc_core:snd_soc_register_dai: snd-soc-dummy snd-soc-dummy: ASoC: dynamically register DAI snd-soc-dummy [ 21.462541] snd_soc_core:snd_soc_register_dai: snd-soc-dummy snd-soc-dummy: ASoC: Registered DAI 'snd-soc-dummy-dai' [ 21.536668] snd_soc_skl 0000:00:1f.3: DSP detected with PCI class/subclass/prog-if info 0x040380 [ 21.542406] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: Capability version: 0x0 [ 21.542412] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: HDA capability ID: 0x2 [ 21.542416] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: Found ML capability [ 21.542421] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: Capability version: 0x0 [ 21.542425] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: HDA capability ID: 0x3 [ 21.542429] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: Found PP capability offset=800 [ 21.542434] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: Capability version: 0x0 [ 21.542438] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: HDA capability ID: 0x1 [ 21.542443] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: Found GTS capability offset=500 [ 21.542448] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: Capability version: 0x0 [ 21.542451] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: HDA capability ID: 0x5 [ 21.542455] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: Found DRSM capability [ 21.542460] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: Capability version: 0x0 [ 21.542465] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: HDA capability ID: 0x4 [ 21.542468] snd_hda_core:snd_hdac_bus_parse_capabilities: snd_soc_skl 0000:00:1f.3: Found SPB capability [ 21.542672] snd_soc_skl:skl_first_init: snd_soc_skl 0000:00:1f.3: chipset global capabilities = 0x9701 [ 21.556416] snd_soc_skl:skl_nhlt_update_topology_bin: snd_soc_skl 0000:00:1f.3: oem_id LENOVO, oem_table_id TP-N23 oem_revision 4880 [ 21.557053] snd_soc_skl:skl_find_machine: snd_soc_skl 0000:00:1f.3: No matching I2S machine driver found [ 21.557650] snd_soc_skl:skl_init_dsp: snd_soc_skl 0000:00:1f.3: dsp registration status=0 [ 21.557658] snd_hda_ext_core:snd_hdac_ext_bus_get_ml_capabilities: snd_soc_skl 0000:00:1f.3: In snd_hdac_ext_bus_get_ml_capabilities Link count: 2 [ 21.558254] snd_soc_skl 0000:00:1f.3: bound 0000:00:02.0 (ops i915_audio_component_bind_ops [i915]) [ 21.558259] snd_hda_core:snd_hdac_display_power: snd_soc_skl 0000:00:1f.3: display power enable [ 21.558272] snd_hda_core:snd_hdac_set_codec_wakeup: snd_soc_skl 0000:00:1f.3: enable codec wakeup [ 21.561088] snd_hda_core:snd_hdac_set_codec_wakeup: snd_soc_skl 0000:00:1f.3: disable codec wakeup [ 21.562606] snd_soc_skl:skl_init_pci: snd_soc_skl 0000:00:1f.3: Clearing TCSEL [ 21.562617] snd_hda_core:snd_hdac_set_codec_wakeup: snd_soc_skl 0000:00:1f.3: enable codec wakeup [ 21.569291] snd_soc_skl 0000:00:1f.3: codec_mask = 0x0 [ 21.569306] snd_hda_core:snd_hdac_set_codec_wakeup: snd_soc_skl 0000:00:1f.3: disable codec wakeup [ 21.570856] snd_soc_skl 0000:00:1f.3: no hda codecs found! [ 21.570901] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.570923] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'SSP0 Pin' [ 21.570927] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.570947] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'SSP1 Pin' [ 21.570951] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.570971] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'SSP2 Pin' [ 21.570976] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.570996] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'SSP3 Pin' [ 21.571000] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.571020] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'SSP4 Pin' [ 21.571024] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.571044] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'SSP5 Pin' [ 21.571048] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.571067] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'iDisp1 Pin' [ 21.571077] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.571099] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'iDisp2 Pin' [ 21.571103] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.571125] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'iDisp3 Pin' [ 21.571129] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.571148] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'DMIC01 Pin' [ 21.571153] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.571172] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'DMIC16k Pin' [ 21.571176] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.571209] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'Analog CPU DAI' [ 21.571213] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.571230] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'Alt Analog CPU DAI' [ 21.571233] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: dynamically register DAI 0000:00:1f.3 [ 21.571250] snd_soc_core:snd_soc_register_dai: snd_soc_skl 0000:00:1f.3: ASoC: Registered DAI 'Digital CPU DAI' [ 21.571566] snd_hda_core:snd_hdac_display_power: snd_soc_skl 0000:00:1f.3: display power disable [ 21.571585] snd_soc_skl:skl_runtime_suspend: snd_soc_skl 0000:00:1f.3: in skl_runtime_suspend
The patch
ASoC: Intel: Skylake: Select hda configuration permissively
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From a66f88394a78fec9a05fa6e517e9603e8eca8363 Mon Sep 17 00:00:00 2001
From: Cezary Rojewski cezary.rojewski@intel.com Date: Thu, 5 Mar 2020 15:53:09 +0100 Subject: [PATCH] ASoC: Intel: Skylake: Select hda configuration permissively
With _reset_link removed from the probe sequence, codec_mask at the time skl_find_hda_machine() is invoked will always be 0, so hda machine will never be chosen. Rather than reorganizing boot flow, be permissive about invalid mask. codec_mask will be set to proper value during probe_work - before skl_codec_create() ever gets called.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200305145314.32579-3-cezary.rojewski@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/skl.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index d66231525356..4827fe6bc1cb 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -481,13 +481,8 @@ static struct skl_ssp_clk skl_ssp_clks[] = { static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl_dev *skl, struct snd_soc_acpi_mach *machines) { - struct hdac_bus *bus = skl_to_bus(skl); struct snd_soc_acpi_mach *mach;
- /* check if we have any codecs detected on bus */ - if (bus->codec_mask == 0) - return NULL; - /* point to common table */ mach = snd_soc_acpi_intel_hda_machines;
Follow the recommendation set by hda_intel.c and enable HDMI/DP codec wakeup during bus initialization procedure. Disable wakeup once init completes.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/skylake/skl.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 4827fe6bc1cb..e2e531c96dd1 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -130,6 +130,7 @@ static int skl_init_chip(struct hdac_bus *bus, bool full_reset) struct hdac_ext_link *hlink; int ret;
+ snd_hdac_set_codec_wakeup(bus, true); skl_enable_miscbdcge(bus->dev, false); ret = snd_hdac_bus_init_chip(bus, full_reset);
@@ -138,6 +139,7 @@ static int skl_init_chip(struct hdac_bus *bus, bool full_reset) writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV);
skl_enable_miscbdcge(bus->dev, true); + snd_hdac_set_codec_wakeup(bus, false);
return ret; }
The patch
ASoC: Intel: Skylake: Enable codec wakeup during chip init
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From e603f11d5df8997d104ab405ff27640b90baffaa Mon Sep 17 00:00:00 2001
From: Cezary Rojewski cezary.rojewski@intel.com Date: Thu, 5 Mar 2020 15:53:10 +0100 Subject: [PATCH] ASoC: Intel: Skylake: Enable codec wakeup during chip init
Follow the recommendation set by hda_intel.c and enable HDMI/DP codec wakeup during bus initialization procedure. Disable wakeup once init completes.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200305145314.32579-4-cezary.rojewski@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/skl.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 4827fe6bc1cb..e2e531c96dd1 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -130,6 +130,7 @@ static int skl_init_chip(struct hdac_bus *bus, bool full_reset) struct hdac_ext_link *hlink; int ret;
+ snd_hdac_set_codec_wakeup(bus, true); skl_enable_miscbdcge(bus->dev, false); ret = snd_hdac_bus_init_chip(bus, full_reset);
@@ -138,6 +139,7 @@ static int skl_init_chip(struct hdac_bus *bus, bool full_reset) writel(0, hlink->ml_addr + AZX_REG_ML_LOSIDV);
skl_enable_miscbdcge(bus->dev, true); + snd_hdac_set_codec_wakeup(bus, false);
return ret; }
Some configurations expose no NHLT table at all within their /sys/firmware/acpi/tables. To prevent NULL-dereference errors from occurring, adjust probe flow and append additional safety checks in functions involved in NHLT lifecycle.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/skylake/skl-nhlt.c | 3 ++- sound/soc/intel/skylake/skl.c | 9 +++++++-- 2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c index 19f328d71f24..d9c8f5cb389e 100644 --- a/sound/soc/intel/skylake/skl-nhlt.c +++ b/sound/soc/intel/skylake/skl-nhlt.c @@ -182,7 +182,8 @@ void skl_nhlt_remove_sysfs(struct skl_dev *skl) { struct device *dev = &skl->pci->dev;
- sysfs_remove_file(&dev->kobj, &dev_attr_platform_id.attr); + if (skl->nhlt) + sysfs_remove_file(&dev->kobj, &dev_attr_platform_id.attr); }
/* diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index e2e531c96dd1..7ad8a75759bd 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -633,6 +633,9 @@ static int skl_clock_device_register(struct skl_dev *skl) struct platform_device_info pdevinfo = {NULL}; struct skl_clk_pdata *clk_pdata;
+ if (!skl->nhlt) + return 0; + clk_pdata = devm_kzalloc(&skl->pci->dev, sizeof(*clk_pdata), GFP_KERNEL); if (!clk_pdata) @@ -1074,7 +1077,8 @@ static int skl_probe(struct pci_dev *pci, out_clk_free: skl_clock_device_unregister(skl); out_nhlt_free: - intel_nhlt_free(skl->nhlt); + if (skl->nhlt) + intel_nhlt_free(skl->nhlt); out_free: skl_free(bus);
@@ -1123,7 +1127,8 @@ static void skl_remove(struct pci_dev *pci) skl_dmic_device_unregister(skl); skl_clock_device_unregister(skl); skl_nhlt_remove_sysfs(skl); - intel_nhlt_free(skl->nhlt); + if (skl->nhlt) + intel_nhlt_free(skl->nhlt); skl_free(bus); dev_set_drvdata(&pci->dev, NULL); }
On 2020-03-06 22:03, Pierre-Louis Bossart wrote:
- intel_nhlt_free(skl->nhlt); + if (skl->nhlt) + intel_nhlt_free(skl->nhlt);
we could alternatively move the test in intel_nhlt_free, which seems like a more robust thing to do?
Depends. In general kernel-internal API trusts its caller and appending 'ifs' everywhere would unnecessarily slow entire kernel down. While intel_nhlt_free is called rarely, I'd still argue caller should be sane about its invocation.
'if' in skl_probe could be avoided had the function's structure been better. 'if' in skl_remove is just fine, though.
Let's leave it as is.
Czarek
On 3/9/20 8:03 AM, Cezary Rojewski wrote:
On 2020-03-06 22:03, Pierre-Louis Bossart wrote:
- intel_nhlt_free(skl->nhlt); + if (skl->nhlt) + intel_nhlt_free(skl->nhlt);
we could alternatively move the test in intel_nhlt_free, which seems like a more robust thing to do?
Depends. In general kernel-internal API trusts its caller and appending 'ifs' everywhere would unnecessarily slow entire kernel down. While intel_nhlt_free is called rarely, I'd still argue caller should be sane about its invocation.
'if' in skl_probe could be avoided had the function's structure been better. 'if' in skl_remove is just fine, though.
Let's leave it as is.
it's also used in SOF:
sound/soc/sof/intel/hda.c: intel_nhlt_free(nhlt);
that's why I suggested to factor the test so that both users don't need to add the if.
On 2020-03-09 18:01, Pierre-Louis Bossart wrote:
On 3/9/20 8:03 AM, Cezary Rojewski wrote:
On 2020-03-06 22:03, Pierre-Louis Bossart wrote:
- intel_nhlt_free(skl->nhlt); + if (skl->nhlt) + intel_nhlt_free(skl->nhlt);
we could alternatively move the test in intel_nhlt_free, which seems like a more robust thing to do?
Depends. In general kernel-internal API trusts its caller and appending 'ifs' everywhere would unnecessarily slow entire kernel down. While intel_nhlt_free is called rarely, I'd still argue caller should be sane about its invocation.
'if' in skl_probe could be avoided had the function's structure been better. 'if' in skl_remove is just fine, though.
Let's leave it as is.
it's also used in SOF:
sound/soc/sof/intel/hda.c: intel_nhlt_free(nhlt);
that's why I suggested to factor the test so that both users don't need to add the if.
I understand, and my explanation still applies.
SOF's intel_nhlt_free usage is great example actually. Caller is sane about its doings as it should be. Internal API needs not to suffer from callers irresponsibility.
PCM does not call ::free() if ::open() fails, same as device-driver model does not care about ::remove() if ::probe() fails.
Czarek
On 3/9/20 12:38 PM, Cezary Rojewski wrote:
On 2020-03-09 18:01, Pierre-Louis Bossart wrote:
On 3/9/20 8:03 AM, Cezary Rojewski wrote:
On 2020-03-06 22:03, Pierre-Louis Bossart wrote:
- intel_nhlt_free(skl->nhlt); + if (skl->nhlt) + intel_nhlt_free(skl->nhlt);
we could alternatively move the test in intel_nhlt_free, which seems like a more robust thing to do?
Depends. In general kernel-internal API trusts its caller and appending 'ifs' everywhere would unnecessarily slow entire kernel down. While intel_nhlt_free is called rarely, I'd still argue caller should be sane about its invocation.
'if' in skl_probe could be avoided had the function's structure been better. 'if' in skl_remove is just fine, though.
Let's leave it as is.
it's also used in SOF:
sound/soc/sof/intel/hda.c: intel_nhlt_free(nhlt);
that's why I suggested to factor the test so that both users don't need to add the if.
I understand, and my explanation still applies.
SOF's intel_nhlt_free usage is great example actually. Caller is sane about its doings as it should be. Internal API needs not to suffer from callers irresponsibility.
ok, indeed looking at the code there's no need for the test.
The patch
ASoC: Intel: Skylake: Shield against no-NHLT configurations
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 9e6c382f5a6161eb55115fb56614b9827f2e7da3 Mon Sep 17 00:00:00 2001
From: Cezary Rojewski cezary.rojewski@intel.com Date: Thu, 5 Mar 2020 15:53:11 +0100 Subject: [PATCH] ASoC: Intel: Skylake: Shield against no-NHLT configurations
Some configurations expose no NHLT table at all within their /sys/firmware/acpi/tables. To prevent NULL-dereference errors from occurring, adjust probe flow and append additional safety checks in functions involved in NHLT lifecycle.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200305145314.32579-5-cezary.rojewski@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/skl-nhlt.c | 3 ++- sound/soc/intel/skylake/skl.c | 9 +++++++-- 2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c index 19f328d71f24..d9c8f5cb389e 100644 --- a/sound/soc/intel/skylake/skl-nhlt.c +++ b/sound/soc/intel/skylake/skl-nhlt.c @@ -182,7 +182,8 @@ void skl_nhlt_remove_sysfs(struct skl_dev *skl) { struct device *dev = &skl->pci->dev;
- sysfs_remove_file(&dev->kobj, &dev_attr_platform_id.attr); + if (skl->nhlt) + sysfs_remove_file(&dev->kobj, &dev_attr_platform_id.attr); }
/* diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index e2e531c96dd1..7ad8a75759bd 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -633,6 +633,9 @@ static int skl_clock_device_register(struct skl_dev *skl) struct platform_device_info pdevinfo = {NULL}; struct skl_clk_pdata *clk_pdata;
+ if (!skl->nhlt) + return 0; + clk_pdata = devm_kzalloc(&skl->pci->dev, sizeof(*clk_pdata), GFP_KERNEL); if (!clk_pdata) @@ -1074,7 +1077,8 @@ static int skl_probe(struct pci_dev *pci, out_clk_free: skl_clock_device_unregister(skl); out_nhlt_free: - intel_nhlt_free(skl->nhlt); + if (skl->nhlt) + intel_nhlt_free(skl->nhlt); out_free: skl_free(bus);
@@ -1123,7 +1127,8 @@ static void skl_remove(struct pci_dev *pci) skl_dmic_device_unregister(skl); skl_clock_device_unregister(skl); skl_nhlt_remove_sysfs(skl); - intel_nhlt_free(skl->nhlt); + if (skl->nhlt) + intel_nhlt_free(skl->nhlt); skl_free(bus); dev_set_drvdata(&pci->dev, NULL); }
From: Mateusz Gorski mateusz.gorski@linux.intel.com
Address missing Dmic configuration for Intel DSP platforms with HDAudio codecs by updating DAPM route map with adequate connections.
Signed-off-by: Mateusz Gorski mateusz.gorski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/boards/skl_hda_dsp_generic.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/soc/intel/boards/skl_hda_dsp_generic.c b/sound/soc/intel/boards/skl_hda_dsp_generic.c index fe2d3a23a4ef..0126dfc7ac24 100644 --- a/sound/soc/intel/boards/skl_hda_dsp_generic.c +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c @@ -59,6 +59,9 @@ static const struct snd_soc_dapm_route skl_hda_map[] = { { "Digital CPU Capture", NULL, "Digital Codec Capture" }, { "codec2_in", NULL, "Alt Analog CPU Capture" }, { "Alt Analog CPU Capture", NULL, "Alt Analog Codec Capture" }, + + { "dmic01_hifi", NULL, "DMIC01 Rx" }, + { "DMIC01 Rx", NULL, "DMIC AIF" }, };
SND_SOC_DAILINK_DEF(dummy_codec,
Hey,
On Thu, 5 Mar 2020, Cezary Rojewski wrote:
--- a/sound/soc/intel/boards/skl_hda_dsp_generic.c +++ b/sound/soc/intel/boards/skl_hda_dsp_generic.c @@ -59,6 +59,9 @@ static const struct snd_soc_dapm_route skl_hda_map[] = { { "Digital CPU Capture", NULL, "Digital Codec Capture" }, { "codec2_in", NULL, "Alt Analog CPU Capture" }, { "Alt Analog CPU Capture", NULL, "Alt Analog Codec Capture" },
- { "dmic01_hifi", NULL, "DMIC01 Rx" },
- { "DMIC01 Rx", NULL, "DMIC AIF" },
hmm, we need to figure out something else for this. This very same table already has:
» /* digital mics */ » {"DMic", NULL, "SoC DMIC"},
.. so now we have dmic entries two times in the same initializer list.
But a more pressing issue is that this breaks platforms using SOF firmware:
[ 28.751756] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no sink widget found for dmic01_hifi [ 28.751987] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed to add route DMIC01 Rx -> direct -> dmic01_hifi
... maybe you can align the topology to mathc so we can reuse the same widget mapping for both SOF and SST firmwares..?
Br, Kai
But a more pressing issue is that this breaks platforms using SOF firmware:
[ 28.751756] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no sink widget found for dmic01_hifi [ 28.751987] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed to add route DMIC01 Rx -> direct -> dmic01_hifi
... maybe you can align the topology to mathc so we can reuse the same widget mapping for both SOF and SST firmwares..?
Yeah, I thought this would break userspace and installed topologies and that just confirms it. Adding hard-coded routes is really not recommended.
the alternate solution is what I suggested in another thread "No sound since 5.4 on skl_n88l25_s4567", we could mark this machine driver as having an incomplete topology and remove the topology checks in the core.
I couldn't really test my initial patch but that that Cezary unintentionally broke SOF actually that gives me a tool to test the solution.
On 2020-03-06 16:49, Pierre-Louis Bossart wrote:
But a more pressing issue is that this breaks platforms using SOF firmware:
[ 28.751756] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: no sink widget found for dmic01_hifi [ 28.751987] skl_hda_dsp_generic skl_hda_dsp_generic: ASoC: Failed to add route DMIC01 Rx -> direct -> dmic01_hifi
... maybe you can align the topology to mathc so we can reuse the same widget mapping for both SOF and SST firmwares..?
Yeah, I thought this would break userspace and installed topologies and that just confirms it. Adding hard-coded routes is really not recommended.
the alternate solution is what I suggested in another thread "No sound since 5.4 on skl_n88l25_s4567", we could mark this machine driver as having an incomplete topology and remove the topology checks in the core.
I couldn't really test my initial patch but that that Cezary unintentionally broke SOF actually that gives me a tool to test the solution.
Glad to help Pierre ^)^ and thanks for the review Kai.
Guys, we've simply taken long-standing working example such as skl_rt286 or bxt_rt298 and applied the missing diff between skl_hda_dsp's and said machine boards DAPM routes. skl-pcm.c exposes BE: DMIC01 Rx which Intel's SST topologies link against via dmic01_hifi. That has always been the case. No bad intentions, the exact opposite is true: taken old path approach to make sure nothing is broken. Turns out SOF does things differently. Thanks for spotting/ testing this out on your end.
Not a problem to adjust topology on our end, though. In fact, I've already done that on your requested, tested it out and it works just fine. In consequence: - this patch will be dropped from the series - topology patch provided for alsa-ucm-conf will be updated accordingly (dmic01_hifi widget will cease to exist)
Czarek
Guys, we've simply taken long-standing working example such as skl_rt286 or bxt_rt298 and applied the missing diff between skl_hda_dsp's and said machine boards DAPM routes. skl-pcm.c exposes BE: DMIC01 Rx which Intel's SST topologies link against via dmic01_hifi. That has always been the case. No bad intentions, the exact opposite is true: taken old path approach to make sure nothing is broken. Turns out SOF does things differently. Thanks for spotting/ testing this out on your end.
It's actually not SOF, but recent changes in the asoc core that will stop the probe if a route cannot be added, instead of just spewing a warning.
I think it's a good change to force topologies to be complete, but in cases where a previously released topology cannot be adjusted we need a backwards compatible solution. that's my plan for the rest of the afternoon.
Not a problem to adjust topology on our end, though. In fact, I've already done that on your requested, tested it out and it works just fine. In consequence:
- this patch will be dropped from the series
- topology patch provided for alsa-ucm-conf will be updated accordingly
(dmic01_hifi widget will cease to exist)
Sounds good, thanks Cezary.
On 2020-03-06 20:49, Pierre-Louis Bossart wrote:
Guys, we've simply taken long-standing working example such as skl_rt286 or bxt_rt298 and applied the missing diff between skl_hda_dsp's and said machine boards DAPM routes. skl-pcm.c exposes BE: DMIC01 Rx which Intel's SST topologies link against via dmic01_hifi. That has always been the case. No bad intentions, the exact opposite is true: taken old path approach to make sure nothing is broken. Turns out SOF does things differently. Thanks for spotting/ testing this out on your end.
It's actually not SOF, but recent changes in the asoc core that will stop the probe if a route cannot be added, instead of just spewing a warning.
I think it's a good change to force topologies to be complete, but in cases where a previously released topology cannot be adjusted we need a backwards compatible solution. that's my plan for the rest of the afternoon.
Agree, missing topology routes should not be permissive. Although my point was about the fact that those 2 routes actually exist in every SST machine driver (in essence linking DMIC01 Rx with platform via internal dmic01_hifi widget).
Not a problem to adjust topology on our end, though. In fact, I've already done that on your requested, tested it out and it works just fine. In consequence:
- this patch will be dropped from the series
- topology patch provided for alsa-ucm-conf will be updated
accordingly (dmic01_hifi widget will cease to exist)
Sounds good, thanks Cezary.
Awaiting comments for the rest of the patches if any! Especially how low - kernel version wise - should the fixes be backported.
Thanks in advance for your input and time.
Czarek
Due to unconditional initial timeouts, firmware may fail to load during its initialization. This issue cannot be resolved on driver side as it is caused by external sources such as CSME but has to be accounted for nonetheless.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com Fixes: cb6a55284629 ("ASoC: Intel: cnl: Add sst library functions for cnl platform") --- sound/soc/intel/skylake/bxt-sst.c | 2 -- sound/soc/intel/skylake/cnl-sst.c | 15 ++++++++++----- sound/soc/intel/skylake/skl-sst-dsp.h | 1 + 3 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index 92a82e6b5fe6..cdafade8abd6 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -38,8 +38,6 @@ /* Delay before scheduling D0i3 entry */ #define BXT_D0I3_DELAY 5000
-#define BXT_FW_ROM_INIT_RETRY 3 - static unsigned int bxt_get_errorcode(struct sst_dsp *ctx) { return sst_dsp_shim_read(ctx, BXT_ADSP_ERROR_CODE); diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 4f64f097e9ae..060e47ae3391 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -109,7 +109,7 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx) { struct firmware stripped_fw; struct skl_dev *cnl = ctx->thread_context; - int ret; + int ret, i;
if (!ctx->fw) { ret = request_firmware(&ctx->fw, ctx->fw_name, ctx->dev); @@ -131,12 +131,16 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx) stripped_fw.size = ctx->fw->size; skl_dsp_strip_extended_manifest(&stripped_fw);
- ret = cnl_prepare_fw(ctx, stripped_fw.data, stripped_fw.size); - if (ret < 0) { - dev_err(ctx->dev, "prepare firmware failed: %d\n", ret); - goto cnl_load_base_firmware_failed; + for (i = 0; i < BXT_FW_ROM_INIT_RETRY; i++) { + ret = cnl_prepare_fw(ctx, stripped_fw.data, stripped_fw.size); + if (!ret) + break; + dev_dbg(ctx->dev, "prepare firmware failed: %d\n", ret); }
+ if (ret < 0) + goto cnl_load_base_firmware_failed; + ret = sst_transfer_fw_host_dma(ctx); if (ret < 0) { dev_err(ctx->dev, "transfer firmware failed: %d\n", ret); @@ -158,6 +162,7 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx) return 0;
cnl_load_base_firmware_failed: + dev_err(ctx->dev, "firmware load failed: %d\n", ret); release_firmware(ctx->fw); ctx->fw = NULL;
diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index cdfec0fca577..067d1ea11cde 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -67,6 +67,7 @@ struct skl_dev;
#define SKL_FW_INIT 0x1 #define SKL_FW_RFW_START 0xf +#define BXT_FW_ROM_INIT_RETRY 3
#define SKL_ADSPIC_IPC 1 #define SKL_ADSPIS_IPC 1
The patch
ASoC: Intel: Allow for ROM init retry on CNL platforms
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 024aa45f55ccd40704cfdef61b2a8b6d0de9cdd1 Mon Sep 17 00:00:00 2001
From: Cezary Rojewski cezary.rojewski@intel.com Date: Thu, 5 Mar 2020 15:53:13 +0100 Subject: [PATCH] ASoC: Intel: Allow for ROM init retry on CNL platforms
Due to unconditional initial timeouts, firmware may fail to load during its initialization. This issue cannot be resolved on driver side as it is caused by external sources such as CSME but has to be accounted for nonetheless.
Fixes: cb6a55284629 ("ASoC: Intel: cnl: Add sst library functions for cnl platform") Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200305145314.32579-7-cezary.rojewski@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/bxt-sst.c | 2 -- sound/soc/intel/skylake/cnl-sst.c | 15 ++++++++++----- sound/soc/intel/skylake/skl-sst-dsp.h | 1 + 3 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index 92a82e6b5fe6..cdafade8abd6 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -38,8 +38,6 @@ /* Delay before scheduling D0i3 entry */ #define BXT_D0I3_DELAY 5000
-#define BXT_FW_ROM_INIT_RETRY 3 - static unsigned int bxt_get_errorcode(struct sst_dsp *ctx) { return sst_dsp_shim_read(ctx, BXT_ADSP_ERROR_CODE); diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 4f64f097e9ae..060e47ae3391 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -109,7 +109,7 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx) { struct firmware stripped_fw; struct skl_dev *cnl = ctx->thread_context; - int ret; + int ret, i;
if (!ctx->fw) { ret = request_firmware(&ctx->fw, ctx->fw_name, ctx->dev); @@ -131,12 +131,16 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx) stripped_fw.size = ctx->fw->size; skl_dsp_strip_extended_manifest(&stripped_fw);
- ret = cnl_prepare_fw(ctx, stripped_fw.data, stripped_fw.size); - if (ret < 0) { - dev_err(ctx->dev, "prepare firmware failed: %d\n", ret); - goto cnl_load_base_firmware_failed; + for (i = 0; i < BXT_FW_ROM_INIT_RETRY; i++) { + ret = cnl_prepare_fw(ctx, stripped_fw.data, stripped_fw.size); + if (!ret) + break; + dev_dbg(ctx->dev, "prepare firmware failed: %d\n", ret); }
+ if (ret < 0) + goto cnl_load_base_firmware_failed; + ret = sst_transfer_fw_host_dma(ctx); if (ret < 0) { dev_err(ctx->dev, "transfer firmware failed: %d\n", ret); @@ -158,6 +162,7 @@ static int cnl_load_base_firmware(struct sst_dsp *ctx) return 0;
cnl_load_base_firmware_failed: + dev_err(ctx->dev, "firmware load failed: %d\n", ret); release_firmware(ctx->fw); ctx->fw = NULL;
diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index cdfec0fca577..067d1ea11cde 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -67,6 +67,7 @@ struct skl_dev;
#define SKL_FW_INIT 0x1 #define SKL_FW_RFW_START 0xf +#define BXT_FW_ROM_INIT_RETRY 3
#define SKL_ADSPIC_IPC 1 #define SKL_ADSPIS_IPC 1
Each purge request is sent by driver after master core is powered up and unresetted but before it is unstalled. On unstall, ROM begins processing the request and initializing environment for FW load. Host should await ROM's ack before moving forward. Without doing so, ROM init poll may start too early and false timeouts can occur.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com Fixes: cb6a55284629 ("ASoC: Intel: cnl: Add sst library functions for cnl platform") --- sound/soc/intel/skylake/bxt-sst.c | 1 - sound/soc/intel/skylake/cnl-sst.c | 20 ++++++++++++++++++-- sound/soc/intel/skylake/skl-sst-dsp.h | 1 + 3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index cdafade8abd6..38b9d7494083 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -17,7 +17,6 @@ #include "skl.h"
#define BXT_BASEFW_TIMEOUT 3000 -#define BXT_INIT_TIMEOUT 300 #define BXT_ROM_INIT_TIMEOUT 70 #define BXT_IPC_PURGE_FW 0x01004000
diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 060e47ae3391..c6abcd5aa67b 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -57,18 +57,34 @@ static int cnl_prepare_fw(struct sst_dsp *ctx, const void *fwdata, u32 fwsize) ctx->dsp_ops.stream_tag = stream_tag; memcpy(ctx->dmab.area, fwdata, fwsize);
+ ret = skl_dsp_core_power_up(ctx, SKL_DSP_CORE0_MASK); + if (ret < 0) { + dev_err(ctx->dev, "dsp core0 power up failed\n"); + ret = -EIO; + goto base_fw_load_failed; + } + /* purge FW request */ sst_dsp_shim_write(ctx, CNL_ADSP_REG_HIPCIDR, CNL_ADSP_REG_HIPCIDR_BUSY | (CNL_IPC_PURGE | ((stream_tag - 1) << CNL_ROM_CTRL_DMA_ID)));
- ret = cnl_dsp_enable_core(ctx, SKL_DSP_CORE0_MASK); + ret = skl_dsp_start_core(ctx, SKL_DSP_CORE0_MASK); if (ret < 0) { - dev_err(ctx->dev, "dsp boot core failed ret: %d\n", ret); + dev_err(ctx->dev, "Start dsp core failed ret: %d\n", ret); ret = -EIO; goto base_fw_load_failed; }
+ ret = sst_dsp_register_poll(ctx, CNL_ADSP_REG_HIPCIDA, + CNL_ADSP_REG_HIPCIDA_DONE, + CNL_ADSP_REG_HIPCIDA_DONE, + BXT_INIT_TIMEOUT, "HIPCIDA Done"); + if (ret < 0) { + dev_err(ctx->dev, "timeout for purge request: %d\n", ret); + goto base_fw_load_failed; + } + /* enable interrupt */ cnl_ipc_int_enable(ctx); cnl_ipc_op_int_enable(ctx); diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index 067d1ea11cde..1df9ef422f61 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -68,6 +68,7 @@ struct skl_dev; #define SKL_FW_INIT 0x1 #define SKL_FW_RFW_START 0xf #define BXT_FW_ROM_INIT_RETRY 3 +#define BXT_INIT_TIMEOUT 300
#define SKL_ADSPIC_IPC 1 #define SKL_ADSPIS_IPC 1
The patch
ASoC: Intel: Skylake: Await purge request ack on CNL
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 7693cadac86548b30389a6e11d78c38db654f393 Mon Sep 17 00:00:00 2001
From: Cezary Rojewski cezary.rojewski@intel.com Date: Thu, 5 Mar 2020 15:53:14 +0100 Subject: [PATCH] ASoC: Intel: Skylake: Await purge request ack on CNL
Each purge request is sent by driver after master core is powered up and unresetted but before it is unstalled. On unstall, ROM begins processing the request and initializing environment for FW load. Host should await ROM's ack before moving forward. Without doing so, ROM init poll may start too early and false timeouts can occur.
Fixes: cb6a55284629 ("ASoC: Intel: cnl: Add sst library functions for cnl platform") Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200305145314.32579-8-cezary.rojewski@intel.com Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/intel/skylake/bxt-sst.c | 1 - sound/soc/intel/skylake/cnl-sst.c | 20 ++++++++++++++++++-- sound/soc/intel/skylake/skl-sst-dsp.h | 1 + 3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/sound/soc/intel/skylake/bxt-sst.c b/sound/soc/intel/skylake/bxt-sst.c index cdafade8abd6..38b9d7494083 100644 --- a/sound/soc/intel/skylake/bxt-sst.c +++ b/sound/soc/intel/skylake/bxt-sst.c @@ -17,7 +17,6 @@ #include "skl.h"
#define BXT_BASEFW_TIMEOUT 3000 -#define BXT_INIT_TIMEOUT 300 #define BXT_ROM_INIT_TIMEOUT 70 #define BXT_IPC_PURGE_FW 0x01004000
diff --git a/sound/soc/intel/skylake/cnl-sst.c b/sound/soc/intel/skylake/cnl-sst.c index 060e47ae3391..c6abcd5aa67b 100644 --- a/sound/soc/intel/skylake/cnl-sst.c +++ b/sound/soc/intel/skylake/cnl-sst.c @@ -57,18 +57,34 @@ static int cnl_prepare_fw(struct sst_dsp *ctx, const void *fwdata, u32 fwsize) ctx->dsp_ops.stream_tag = stream_tag; memcpy(ctx->dmab.area, fwdata, fwsize);
+ ret = skl_dsp_core_power_up(ctx, SKL_DSP_CORE0_MASK); + if (ret < 0) { + dev_err(ctx->dev, "dsp core0 power up failed\n"); + ret = -EIO; + goto base_fw_load_failed; + } + /* purge FW request */ sst_dsp_shim_write(ctx, CNL_ADSP_REG_HIPCIDR, CNL_ADSP_REG_HIPCIDR_BUSY | (CNL_IPC_PURGE | ((stream_tag - 1) << CNL_ROM_CTRL_DMA_ID)));
- ret = cnl_dsp_enable_core(ctx, SKL_DSP_CORE0_MASK); + ret = skl_dsp_start_core(ctx, SKL_DSP_CORE0_MASK); if (ret < 0) { - dev_err(ctx->dev, "dsp boot core failed ret: %d\n", ret); + dev_err(ctx->dev, "Start dsp core failed ret: %d\n", ret); ret = -EIO; goto base_fw_load_failed; }
+ ret = sst_dsp_register_poll(ctx, CNL_ADSP_REG_HIPCIDA, + CNL_ADSP_REG_HIPCIDA_DONE, + CNL_ADSP_REG_HIPCIDA_DONE, + BXT_INIT_TIMEOUT, "HIPCIDA Done"); + if (ret < 0) { + dev_err(ctx->dev, "timeout for purge request: %d\n", ret); + goto base_fw_load_failed; + } + /* enable interrupt */ cnl_ipc_int_enable(ctx); cnl_ipc_op_int_enable(ctx); diff --git a/sound/soc/intel/skylake/skl-sst-dsp.h b/sound/soc/intel/skylake/skl-sst-dsp.h index 067d1ea11cde..1df9ef422f61 100644 --- a/sound/soc/intel/skylake/skl-sst-dsp.h +++ b/sound/soc/intel/skylake/skl-sst-dsp.h @@ -68,6 +68,7 @@ struct skl_dev; #define SKL_FW_INIT 0x1 #define SKL_FW_RFW_START 0xf #define BXT_FW_ROM_INIT_RETRY 3 +#define BXT_INIT_TIMEOUT 300
#define SKL_ADSPIC_IPC 1 #define SKL_ADSPIS_IPC 1
Hi Cezary,
Thank you for this series. You should have mentioned that this fixes an issue that's been around since September 2018, and for which there's currently no solution on KabyLake/AmberLake platforms - PCI_DEVICE(0x8086, 0x9D71)
https://bugzilla.kernel.org/show_bug.cgi?id=201251
This patchset has been prepared internally for topmost linux-stable 5.5 and 4.20 (no 4.19 as skl_hda_dsp did not exist there yet).
Apart from our RVPs, we have run tests also on:
- KBL Lenovo Carbon X1
- SKL Dell XPS 9350
- WHL Acer Swift 5
Honestly, I'd see HDaudio related patches being backport as low as 4.20 (although some changes had to be adjusted due to base differences between 4.20 and 5.5, can share these too). One could argue HDA + Dmic configuration should be available on 4.19 too - it's an LTS after all. However, that time, some changes could be counted as "feature" rather than fixes. Awaiting your replies and thoughts on that.
In consequence, I've appended "Fixes" only for last two patches for now
- once decisions are made, can append adequate tags wherever necessary.
There's been a couple of accidental regressions already on stable, now fixed, and my understanding is that the bar for inclusion is higher, so let's assume this counts as a new feature: it never worked before with an upstream kernel and distributions haven't had access to topology files either. If a specific distribution wants to backport on top of a -stable version, that's entirely possible but that would be their decision. They would not only need to update the kernel but topology and UCM as well.
One comment though: in the absence of blacklist/voodoo magic, this solution will not be selected with GLK+/WHL, the default with dmics is SOF. Even on KBL, the legacy driver would be selected, we only select the SST driver for SKL and KBL Chromebooks.
You would have to modify the dsp-config stuff [1] to load the skl driver on KBL, which likely requires a FLAG_SST_ONLY_IF_DMIC definition, and probably add a Kconfig since SOF will at some point support KBL, and we want to prevent conflicts between PCI drivers registered for the same ID. The order in that table might be enough though, it's much better than the selection based on a Makefile inclusion.
Patches 6/7 don't seem to be related to DMICs, so we should probably discuss them separately.
Thanks -Pierre
[1] https://elixir.bootlin.com/linux/latest/source/sound/hda/intel-dsp-config.c#...
On Fri, Mar 06, 2020 at 02:48:39PM -0600, Pierre-Louis Bossart wrote:
There's been a couple of accidental regressions already on stable, now fixed, and my understanding is that the bar for inclusion is higher, so let's assume this counts as a new feature: it never worked before with an
There's been no change in the requirements for stable, those regressions have come from patches that have been pulled in automatically by the bot Sasha runs which doesn't pay attention to the requirements but tries to identify patches automatically.
On 2020-03-09 12:38, Mark Brown wrote:
On Fri, Mar 06, 2020 at 02:48:39PM -0600, Pierre-Louis Bossart wrote:
There's been a couple of accidental regressions already on stable, now fixed, and my understanding is that the bar for inclusion is higher, so let's assume this counts as a new feature: it never worked before with an
There's been no change in the requirements for stable, those regressions have come from patches that have been pulled in automatically by the bot Sasha runs which doesn't pay attention to the requirements but tries to identify patches automatically.
Mark, what's your take on backporting these to 4.19 LTS? Do we abandon the subject and "just" merge (once reviewed/ approved) into 5.5? I believe addressing all the issues mentioned on 4.19 is important.
On Mon, Mar 09, 2020 at 03:02:25PM +0100, Cezary Rojewski wrote:
Mark, what's your take on backporting these to 4.19 LTS? Do we abandon the subject and "just" merge (once reviewed/ approved) into 5.5? I believe addressing all the issues mentioned on 4.19 is important.
I didn't actually look at the patches since by the time I went to look at them it was clear that there was going to be a new version. Pierre was saying that they added new functionality which would generally not be suitable.
See Documentation/process/stable-kernel-rules.rst for the official rules.
On 2020-03-09 17:54, Mark Brown wrote:
On Mon, Mar 09, 2020 at 03:02:25PM +0100, Cezary Rojewski wrote:
Mark, what's your take on backporting these to 4.19 LTS? Do we abandon the subject and "just" merge (once reviewed/ approved) into 5.5? I believe addressing all the issues mentioned on 4.19 is important.
I didn't actually look at the patches since by the time I went to look at them it was clear that there was going to be a new version. Pierre was saying that they added new functionality which would generally not be suitable.
See Documentation/process/stable-kernel-rules.rst for the official rules.
Ok, sure. Should the 'Fixes' be appended regardless or leave it as is?
On Mon, Mar 09, 2020 at 06:48:35PM +0100, Cezary Rojewski wrote:
On 2020-03-09 17:54, Mark Brown wrote:
I didn't actually look at the patches since by the time I went to look at them it was clear that there was going to be a new version. Pierre was saying that they added new functionality which would generally not be suitable.
Ok, sure. Should the 'Fixes' be appended regardless or leave it as is?
There's never any harm in adding them.
I didn't actually look at the patches since by the time I went to look at them it was clear that there was going to be a new version. Pierre was saying that they added new functionality which would generally not be suitable.
I am ok with the patches, as long as "[PATCH 5/7] ASoC: Intel: skl_hda_dsp: Enable Dmic configuration" is dropped as discussed.
I don't know if that requires a v2 or if Mark you can just drop this one patch?
So for all Patches except Patch5:
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Thanks Cezary!
On 2020-03-10 17:03, Pierre-Louis Bossart wrote:
I didn't actually look at the patches since by the time I went to look at them it was clear that there was going to be a new version. Pierre was saying that they added new functionality which would generally not be suitable.
I am ok with the patches, as long as "[PATCH 5/7] ASoC: Intel: skl_hda_dsp: Enable Dmic configuration" is dropped as discussed.
I don't know if that requires a v2 or if Mark you can just drop this one patch?
So for all Patches except Patch5:
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Thanks Cezary!
Thanks for quick review and upstream!
In fact, I wasn't even able to append missing 'Fixes' here and there. While indeed on 4.19 hdadsp machine driver does not exist so these patches can count as "new feature", for some other kernels such as 5.4 that ain't a case.
Not very familiar with the cherry-pick mechanism for stable kernels, but is it possible for patches that ain't flagged with 'Fixes' tag to get backported? Having all of these at least on 5.4 is in my opinion desirable.
Czarek
On Wed, Mar 11, 2020 at 04:49:41PM +0100, Cezary Rojewski wrote:
Not very familiar with the cherry-pick mechanism for stable kernels, but is it possible for patches that ain't flagged with 'Fixes' tag to get backported? Having all of these at least on 5.4 is in my opinion desirable.
The document I pointed you at for the stable process includes information on how to submit patches for stable via e-mail.
participants (4)
-
Cezary Rojewski -
Kai Vehmanen -
Mark Brown -
Pierre-Louis Bossart