| Message ID | 20200305145314.32579-2-cezary.rojewski@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2ef81057d80456870b97890dd79c8f56a85b1242 |
| Headers | show |
| Series | ASoC: Intel: Skylake: Fix HDaudio and Dmic | expand |
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.
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) {
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(-)