Re: [GIT PULL] sound updates for 4.21
On 1/4/19 6:34 PM, Azat Khuzhin wrote: This is unfortunately a known issue with this driver, Takashi and I had a couple of email threads on this. Even without errors removing the module doesn't seem to release all resources. I don't like this at all, and for the Sound Open Firmware (SOF) driver I mandated module load-unload as a functional requirement along with zero warnings w/ Sparse, Coccinelle and friends, but on this legacy code I am afraid there is no simple fix - at least not in a merge window or a single kernel cycle. And the fact that kernel crashes after? Just want extra confirmation The kernel crash is related to an HDMI issue that we haven't been able to reproduce on our development and validation devices so far but that's not an excuse and we do need to figure it out. Things work e.g. fine with the KBL Dell XPS13 but not on Linus' SKL Dell XPS13 device, so I'll get one and will also work on additional devices remotely.
Re: [GIT PULL] sound updates for 4.21
> This is unfortunately a known issue with this driver, Takashi and I had > a couple of email threads on this. Even without errors removing the > module doesn't seem to release all resources. I don't like this at all, > and for the Sound Open Firmware (SOF) driver I mandated module > load-unload as a functional requirement along with zero warnings w/ > Sparse, Coccinelle and friends, but on this legacy code I am afraid > there is no simple fix - at least not in a merge window or a single > kernel cycle. And the fact that kernel crashes after? Just want extra confirmation Thanks, Azat.
Re: [GIT PULL] sound updates for 4.21
Hi, On 12/31/18 9:10 PM, Pierre-Louis Bossart wrote: On 12/31/18 12:15 PM, Takashi Iwai wrote: On Mon, 31 Dec 2018 11:24:41 +0100, Pierre-Louis Bossart wrote: On 12/31/18 2:11 AM, Takashi Iwai wrote: On Mon, 31 Dec 2018 00:17:58 +0100, Pierre-Louis Bossart wrote: BTW, one thing I'd really like to avoid is to rearrange the probe procedure of the legacy HDA driver (so that we can get codec_mask during pci probe() call). The async probe is the result of the many struggles with the various and complex configurations. Moving the codec probe to the beginning isn't trivial and quite risky to break something else. Agree, mucking with the probe isn't something we should look into, especially with this Skylake driver being eventually deprecated once SOF is at feature parity. This set of autodetection patches for 4.21 was really targeting CFL/WHL+ devices, where the DSP usage is mandatory when directly-attached digital microphones are used. For Skylake and kabylake using the legacy by default is just fine. OK, then how about applying the PCI class check only for such ones like the patch below? The macro isn't sexy and can be replaced with another way, but you have an idea. The two patches which added the PCI class checks were supposed to be a simple bullet-proof way of detecting the DSP presence and solving a problem of coexistence between two drivers. At this point if we start adding quirks and still have unclear issues with HDMI support which isn't different for CFL+, it may be wiser to revert them to let the 4.21 merge window progress? It's frustrating but I'd rather solve this problem the right way than with multiple iterations rushed because of the merge window timing. Fair enough, let's revert them for now. I'm going to submit the revert patch. Thanks Takashi. If everyone who has been impacted by this issue can send me privately the result of the following two commands it'd help us figure out which machines expose the DSP - we may ask for additional information, e.g. NHLT tables. We clearly need to widen the validation scope since there is obviously a disconnect between what 3rd party BIOS expose and the technical consensus within Intel audio teams. Thanks in advance for your time. -Pierre On my Apollo Lake laptop which is also affected by this: lspci -s 0:1f.3 -vn 00:0e.0 0403: 8086:5a98 (rev 0b) (prog-if 80) Subsystem: 10ec:111a Flags: bus master, fast devsel, latency 0, IRQ 128 Memory at 91218000 (64-bit, non-prefetchable) [size=16K] Memory at 9100 (64-bit, non-prefetchable) [size=1M] Capabilities: Kernel driver in use: snd_hda_intel Kernel modules: snd_hda_intel, snd_soc_skl more /sys/bus/pci/devices/\:00\:1f.3/class (should return 0x040100 or 0x040380, if the value is 0x040300 then the DSP is not exposed) 0x040380 Regards, Hans
Re: [GIT PULL] sound updates for 4.21
On 12/31/18 12:15 PM, Takashi Iwai wrote: On Mon, 31 Dec 2018 11:24:41 +0100, Pierre-Louis Bossart wrote: On 12/31/18 2:11 AM, Takashi Iwai wrote: On Mon, 31 Dec 2018 00:17:58 +0100, Pierre-Louis Bossart wrote: BTW, one thing I'd really like to avoid is to rearrange the probe procedure of the legacy HDA driver (so that we can get codec_mask during pci probe() call). The async probe is the result of the many struggles with the various and complex configurations. Moving the codec probe to the beginning isn't trivial and quite risky to break something else. Agree, mucking with the probe isn't something we should look into, especially with this Skylake driver being eventually deprecated once SOF is at feature parity. This set of autodetection patches for 4.21 was really targeting CFL/WHL+ devices, where the DSP usage is mandatory when directly-attached digital microphones are used. For Skylake and kabylake using the legacy by default is just fine. OK, then how about applying the PCI class check only for such ones like the patch below? The macro isn't sexy and can be replaced with another way, but you have an idea. The two patches which added the PCI class checks were supposed to be a simple bullet-proof way of detecting the DSP presence and solving a problem of coexistence between two drivers. At this point if we start adding quirks and still have unclear issues with HDMI support which isn't different for CFL+, it may be wiser to revert them to let the 4.21 merge window progress? It's frustrating but I'd rather solve this problem the right way than with multiple iterations rushed because of the merge window timing. Fair enough, let's revert them for now. I'm going to submit the revert patch. Thanks Takashi. If everyone who has been impacted by this issue can send me privately the result of the following two commands it'd help us figure out which machines expose the DSP - we may ask for additional information, e.g. NHLT tables. We clearly need to widen the validation scope since there is obviously a disconnect between what 3rd party BIOS expose and the technical consensus within Intel audio teams. Thanks in advance for your time. -Pierre lspci -s 0:1f.3 -vn more /sys/bus/pci/devices/\:00\:1f.3/class (should return 0x040100 or 0x040380, if the value is 0x040300 then the DSP is not exposed) (replace 1f.3 with 0e.0 for ApolloLake/GeminiLake)
Re: [GIT PULL] sound updates for 4.21
On Mon, 31 Dec 2018 11:24:41 +0100, Pierre-Louis Bossart wrote: > > > On 12/31/18 2:11 AM, Takashi Iwai wrote: > > On Mon, 31 Dec 2018 00:17:58 +0100, > > Pierre-Louis Bossart wrote: > >>> BTW, one thing I'd really like to avoid is to rearrange the probe > >>> procedure of the legacy HDA driver (so that we can get codec_mask > >>> during pci probe() call). The async probe is the result of the many > >>> struggles with the various and complex configurations. Moving the > >>> codec probe to the beginning isn't trivial and quite risky to break > >>> something else. > >> Agree, mucking with the probe isn't something we should look into, > >> especially with this Skylake driver being eventually deprecated once > >> SOF is at feature parity. This set of autodetection patches for 4.21 > >> was really targeting CFL/WHL+ devices, where the DSP usage is > >> mandatory when directly-attached digital microphones are used. For > >> Skylake and kabylake using the legacy by default is just fine. > > OK, then how about applying the PCI class check only for such ones > > like the patch below? The macro isn't sexy and can be replaced with > > another way, but you have an idea. > > The two patches which added the PCI class checks were supposed to be a > simple bullet-proof way of detecting the DSP presence and solving a > problem of coexistence between two drivers. At this point if we start > adding quirks and still have unclear issues with HDMI support which > isn't different for CFL+, it may be wiser to revert them to let the > 4.21 merge window progress? It's frustrating but I'd rather solve this > problem the right way than with multiple iterations rushed because of > the merge window timing. Fair enough, let's revert them for now. I'm going to submit the revert patch. thanks, Takashi
Re: [GIT PULL] sound updates for 4.21
On 12/31/18 7:43 AM, Azat Khuzhin wrote:
+/* CFL and later models, preferring ASoC when DSP is available */
+#define IS_CFL_PLUS(pci)\
+ ((pci)->vendor == 0x8086 && \
+((pci)->device == 0xa348 || \
+ (pci)->device == 0x9dc8 || \
+ (pci)->device == 0x34c8))
+
static char *driver_short_names[] = {
[AZX_DRIVER_ICH] = "HDA Intel",
[AZX_DRIVER_PCH] = "HDA Intel PCH",
@@ -2056,7 +2063,7 @@ static int azx_probe(struct pci_dev *pci,
if (pci_id->driver_data & AZX_DCAPS_INTEL_SHARED) {
switch (skl_pci_binding) {
case SND_SKL_PCI_BIND_AUTO:
- if (pci->class != 0x040300) {
+ if (pci->class != 0x040300 && IS_CFL_PLUS(pci)) {
dev_info(>dev, "The DSP is enabled on this
platform, aborting probe\n");
return -ENODEV;
}
lenovo thinkpad carbon 6th gen - no sound after this patch, and this
patch should fix sound issue for it (not tested, just checking the
condition and pci attrs)
But what interesting is that I cannot remove snd_soc_skl module
without reboot (to adjust "pci_binding=1" so make sound works),
because kernel hang after short period doing it:
# rmmod snd_soc_skl_ssp_clk
# rmmod snd_soc_skl
WARN_ON triggered on rmmod:
This is unfortunately a known issue with this driver, Takashi and I had
a couple of email threads on this. Even without errors removing the
module doesn't seem to release all resources. I don't like this at all,
and for the Sound Open Firmware (SOF) driver I mandated module
load-unload as a functional requirement along with zero warnings w/
Sparse, Coccinelle and friends, but on this legacy code I am afraid
there is no simple fix - at least not in a merge window or a single
kernel cycle.
Re: [GIT PULL] sound updates for 4.21
> +/* CFL and later models, preferring ASoC when DSP is available */
> +#define IS_CFL_PLUS(pci)\
> + ((pci)->vendor == 0x8086 && \
> +((pci)->device == 0xa348 || \
> + (pci)->device == 0x9dc8 || \
> + (pci)->device == 0x34c8))
> +
> static char *driver_short_names[] = {
> [AZX_DRIVER_ICH] = "HDA Intel",
> [AZX_DRIVER_PCH] = "HDA Intel PCH",
> @@ -2056,7 +2063,7 @@ static int azx_probe(struct pci_dev *pci,
> if (pci_id->driver_data & AZX_DCAPS_INTEL_SHARED) {
> switch (skl_pci_binding) {
> case SND_SKL_PCI_BIND_AUTO:
> - if (pci->class != 0x040300) {
> + if (pci->class != 0x040300 && IS_CFL_PLUS(pci)) {
> dev_info(>dev, "The DSP is enabled on
> this platform, aborting probe\n");
> return -ENODEV;
> }
lenovo thinkpad carbon 6th gen - no sound after this patch, and this
patch should fix sound issue for it (not tested, just checking the
condition and pci attrs)
But what interesting is that I cannot remove snd_soc_skl module
without reboot (to adjust "pci_binding=1" so make sound works),
because kernel hang after short period doing it:
# rmmod snd_soc_skl_ssp_clk
# rmmod snd_soc_skl
WARN_ON triggered on rmmod:
Dec 30 19:29:38 WARNING: CPU: 0 PID: 22941 at
sound/hda/hdac_component.c:327 snd_hdac_acomp_exit+0x69/0x90
[snd_hda_core]
Dec 30 19:29:38 Modules linked in: snd_hda_intel snd_usb_audio
snd_usbmidi_lib snd_rawmidi snd_seq_device cdc_ether usbnet r8152 mii
hid_apple hid_generic usbhid hid thunderbolt tun
bluetooth ecdh_generic nf_conntrack_netlink nfnetlink xfrm_user
xfrm_algo xt_addrtype xt_conntrack br_netfilter iptable_mangle
xt_CHECKSUM iptable_nat joydev mousedev rmi_smbus rmi_c
ore ipt_MASQUERADE nf_nat_ipv4 nf_nat nf_conntrack nf_defrag_ipv6
nf_defrag_ipv4 libcrc32c xt_tcpudp overlay bridge stp llc
iptable_filter ccm algif_aead cbc snd_soc_hdac_hdmi des_ge
neric zram arc4 lz4 lz4_compress cmac msr md4 algif_hash i915
snd_soc_dmic snd_soc_skl(-) iwlmvm snd_soc_skl_ipc snd_soc_sst_ipc
snd_soc_sst_dsp snd_hda_ext_core snd_soc_acpi_intel_m
atch snd_soc_acpi mac80211 snd_soc_core snd_compress iTCO_wdt ac97_bus
kvmgt iTCO_vendor_support intel_rapl snd_pcm_dmaengine vfio_mdev mdev
x86_pkg_temp_thermal uvcvideo intel_power
clamp coretemp vfio_iommu_type1 kvm_intel vfio snd_hda_codec
videobuf2_vmalloc i2c_algo_bit
Dec 30 19:29:38 videobuf2_memops iwlwifi videobuf2_v4l2
videobuf2_common snd_hda_core videodev drm_kms_helper tpm_crb kvm
nls_iso8859_1 snd_hwdep cfg80211 drm nls_cp437 snd_pcm irqb
ypass thinkpad_acpi intel_cstate intel_uncore nvram psmouse intel_gtt
e1000e snd_timer agpgart ledtrig_audio intel_rapl_perf syscopyarea snd
input_leds processor_thermal_device pcspk
r tpm_tis tpm_tis_core sysfillrect i2c_i801 media wmi_bmof
intel_wmi_thunderbolt tpm ucsi_acpi typec_ucsi mei_me int3403_thermal
sysimgblt rfkill mei fb_sys_fops typec intel_soc_dts_
iosf intel_pch_thermal rng_core soundcore battery ac
int340x_thermal_zone evdev mac_hid int3400_thermal acpi_thermal_rel
pcc_cpufreq vboxnetflt(OE) vboxnetadp(OE) vboxpci(OE) vboxdrv
(OE) crypto_user ip_tables x_tables ext4 crc32c_generic crc16 mbcache
jbd2 fscrypto algif_skcipher af_alg dm_crypt dm_mod crct10dif_pclmul
crc32_pclmul crc32c_intel ghash_clmulni_int
el serio_raw atkbd libps2 aesni_intel aes_x86_64 xhci_pci crypto_simd
cryptd xhci_hcd glue_helper
Dec 30 19:29:38 wmi i8042 serio vfat fat [last unloaded: snd_soc_skl_ssp_clk]
Dec 30 19:29:38 CPU: 0 PID: 22941 Comm: rmmod Tainted: G U OE
4.20.0-custom-06428-g00c569b567c7 #1
Dec 30 19:29:38 Hardware name: LENOVO 20KH006MRT/20KH006MRT, BIOS
N23ET50W (1.25 ) 06/25/2018
Dec 30 19:29:38 RIP: 0010:snd_hdac_acomp_exit+0x69/0x90 [snd_hda_core]
Dec 30 19:29:38 Code: 0d 83 5f c5 48 89 ef 31 c9 31 d2 48 c7 83 a0 04
00 00 00 00 00 00 48 c7 c6 80 6b ba c0 e8 4f 35 60 c5 31 c0 5b 5d c3
31 c0 c3 <0f> 0b 48 8b 50 08 48 85 d2 74 ae
48 8b 52 10 48 8b 38 e8 d0 a1 c5
Dec 30 19:29:38 RSP: 0018:a73f42b97de8 EFLAGS: 00010202
Dec 30 19:29:38 RAX: 94464d71e7f8 RBX: 94464a22c018 RCX:
Dec 30 19:29:38 RDX: 0001 RSI: RDI:
94464a22c018
Dec 30 19:29:38 RBP: 94464fa690b0 R08: 944651c02480 R09:
944651c024f8
Dec 30 19:29:38 R10: R11: 86e4a478 R12:
94464fa690b0
Dec 30 19:29:38 R13: c0b2e070 R14: 94464ffb7c60 R15:
dead0100
Dec 30 19:29:38 FS: 77983b80() GS:94465240()
knlGS:
Dec 30 19:29:38 CS: 0010 DS: ES: CR0: 80050033
Dec 30 19:29:38 CR2: 55784098 CR3: 000304682003 CR4:
003606f0
Dec 30 19:29:38 Call Trace:
Dec 30 19:29:38 skl_free+0x7e/0x90 [snd_soc_skl]
Dec 30 19:29:38 skl_remove+0x9f/0xb0 [snd_soc_skl]
Dec 30 19:29:38 pci_device_remove+0x3b/0xc0
Dec 30 19:29:38
Re: [GIT PULL] sound updates for 4.21
On 12/31/18 2:11 AM, Takashi Iwai wrote:
On Mon, 31 Dec 2018 00:17:58 +0100,
Pierre-Louis Bossart wrote:
BTW, one thing I'd really like to avoid is to rearrange the probe
procedure of the legacy HDA driver (so that we can get codec_mask
during pci probe() call). The async probe is the result of the many
struggles with the various and complex configurations. Moving the
codec probe to the beginning isn't trivial and quite risky to break
something else.
Agree, mucking with the probe isn't something we should look into,
especially with this Skylake driver being eventually deprecated once
SOF is at feature parity. This set of autodetection patches for 4.21
was really targeting CFL/WHL+ devices, where the DSP usage is
mandatory when directly-attached digital microphones are used. For
Skylake and kabylake using the legacy by default is just fine.
OK, then how about applying the PCI class check only for such ones
like the patch below? The macro isn't sexy and can be replaced with
another way, but you have an idea.
The two patches which added the PCI class checks were supposed to be a
simple bullet-proof way of detecting the DSP presence and solving a
problem of coexistence between two drivers. At this point if we start
adding quirks and still have unclear issues with HDMI support which
isn't different for CFL+, it may be wiser to revert them to let the 4.21
merge window progress? It's frustrating but I'd rather solve this
problem the right way than with multiple iterations rushed because of
the merge window timing.
thanks,
Takashi
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -380,6 +380,13 @@ enum {
#define IS_BXT(pci) ((pci)->vendor == 0x8086 && (pci)->device == 0x5a98)
#define IS_CFL(pci) ((pci)->vendor == 0x8086 && (pci)->device == 0xa348)
+/* CFL and later models, preferring ASoC when DSP is available */
+#define IS_CFL_PLUS(pci)\
+ ((pci)->vendor == 0x8086 && \
+((pci)->device == 0xa348 || \
+ (pci)->device == 0x9dc8 || \
+ (pci)->device == 0x34c8))
+
static char *driver_short_names[] = {
[AZX_DRIVER_ICH] = "HDA Intel",
[AZX_DRIVER_PCH] = "HDA Intel PCH",
@@ -2056,7 +2063,7 @@ static int azx_probe(struct pci_dev *pci,
if (pci_id->driver_data & AZX_DCAPS_INTEL_SHARED) {
switch (skl_pci_binding) {
case SND_SKL_PCI_BIND_AUTO:
- if (pci->class != 0x040300) {
+ if (pci->class != 0x040300 && IS_CFL_PLUS(pci)) {
dev_info(>dev, "The DSP is enabled on this
platform, aborting probe\n");
return -ENODEV;
}
Re: [GIT PULL] sound updates for 4.21
On Mon, 31 Dec 2018 00:17:58 +0100,
Pierre-Louis Bossart wrote:
>
> > BTW, one thing I'd really like to avoid is to rearrange the probe
> > procedure of the legacy HDA driver (so that we can get codec_mask
> > during pci probe() call). The async probe is the result of the many
> > struggles with the various and complex configurations. Moving the
> > codec probe to the beginning isn't trivial and quite risky to break
> > something else.
> Agree, mucking with the probe isn't something we should look into,
> especially with this Skylake driver being eventually deprecated once
> SOF is at feature parity. This set of autodetection patches for 4.21
> was really targeting CFL/WHL+ devices, where the DSP usage is
> mandatory when directly-attached digital microphones are used. For
> Skylake and kabylake using the legacy by default is just fine.
OK, then how about applying the PCI class check only for such ones
like the patch below? The macro isn't sexy and can be replaced with
another way, but you have an idea.
thanks,
Takashi
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -380,6 +380,13 @@ enum {
#define IS_BXT(pci) ((pci)->vendor == 0x8086 && (pci)->device == 0x5a98)
#define IS_CFL(pci) ((pci)->vendor == 0x8086 && (pci)->device == 0xa348)
+/* CFL and later models, preferring ASoC when DSP is available */
+#define IS_CFL_PLUS(pci)\
+ ((pci)->vendor == 0x8086 && \
+((pci)->device == 0xa348 || \
+ (pci)->device == 0x9dc8 || \
+ (pci)->device == 0x34c8))
+
static char *driver_short_names[] = {
[AZX_DRIVER_ICH] = "HDA Intel",
[AZX_DRIVER_PCH] = "HDA Intel PCH",
@@ -2056,7 +2063,7 @@ static int azx_probe(struct pci_dev *pci,
if (pci_id->driver_data & AZX_DCAPS_INTEL_SHARED) {
switch (skl_pci_binding) {
case SND_SKL_PCI_BIND_AUTO:
- if (pci->class != 0x040300) {
+ if (pci->class != 0x040300 && IS_CFL_PLUS(pci)) {
dev_info(>dev, "The DSP is enabled on this
platform, aborting probe\n");
return -ENODEV;
}
Re: [GIT PULL] sound updates for 4.21
On 12/30/18 6:19 PM, Linus Torvalds wrote: On Sun, Dec 30, 2018 at 3:18 PM Pierre-Louis Bossart wrote: The KabyLake Dell XPS13 was initially used for the ASoC driver for HDaudio, so there is no known hardware-related reason why this problem happens. Mine isn't the Kabylake one, it's the older XPS13 9350 (2015 - Skylake) one. ok. Skylake and Kabylake are nearly identical in terms of audio support so the difference should be minor. I'll try to get a Skylake version so that this never happens again. The simplest way to make the problem go away is to force the legacy driver to bind with the (untested) diff below. That diff is wrong, since it changes the meaning of the binding numbers, but then the module interface is wrong: static int skl_pci_binding; module_param_named(pci_binding, skl_pci_binding, int, 0444); MODULE_PARM_DESC(pci_binding, "PCI binding (0=auto, 1=only legacy, 2=only asoc"); so I think Takashi's patch to just change the default value of skl_pci_binding is the better one. You're right, this description should be changed as well to align the text to enum values, but the idea is identical to Takashi's: use the HDaudio legacy by default. -Pierre
Re: [GIT PULL] sound updates for 4.21
On Sun, Dec 30, 2018 at 3:18 PM Pierre-Louis Bossart wrote: > > The KabyLake Dell XPS13 was initially used for the ASoC driver for > HDaudio, so there is no known hardware-related reason why this problem > happens. Mine isn't the Kabylake one, it's the older XPS13 9350 (2015 - Skylake) one. > The simplest way to make the problem go away is to force the legacy > driver to bind with the (untested) diff below. That diff is wrong, since it changes the meaning of the binding numbers, but then the module interface is wrong: static int skl_pci_binding; module_param_named(pci_binding, skl_pci_binding, int, 0444); MODULE_PARM_DESC(pci_binding, "PCI binding (0=auto, 1=only legacy, 2=only asoc"); so I think Takashi's patch to just change the default value of skl_pci_binding is the better one. Linus
Re: [GIT PULL] sound updates for 4.21
On Fri, 28 Dec 2018 20:04:48 +0100,
Linus Torvalds wrote:
>
> On Fri, Dec 28, 2018 at 9:07 AM Takashi Iwai wrote:
> >
> > 1) Whether ASoC driver cannot work with these Dell machines at all
>
> I'm willing to test patches.
>
> Right now, the reason it fails to even load is that "codec_mask" is 0x0001.
>
> And the skl_hda_dsp_generic.c code requires
>
> // This will be 1 for 0x0001
> codec_count = hweight_long(codec_mask);
>
> if (codec_count == 1 && codec_mask & IDISP_CODEC_MASK) {
>.. this is ok ..
> } else if (codec_count == 2 && codec_mask & IDISP_CODEC_MASK) {
>.. as is this ..
> } else {
>.. but here we return -EINVAL;
> }
>
> and that basically requires that IDISP_CODEC_MASK be part of
> codec_mask. Which it isn't (IDISP_CODEC_MASK is 0x4).
>
> Anyway, as long as the ASoC driver refuses to touch this, the default
> pretty much *has* to be the legacy PCI driver.
Thanks, the above seems to be the likely cause.
This is a good news and a bad news, actually. The good news is that
we know the cause, and this can be detected earlier, too. The bad
news is that the possible detection (the probe of codec mask) is done
in an async probe work in the legacy HDA driver, hence the PCI driver
probe() call cannot return -ENODEV for this.
And, it brings me a question: does the audio routing work without
iDISP at all? The current code looks mandating the iDISP, and I
thought this is a core part of HD-audio routing on DSP.
Intel people, could you clarify this?
If iDISP is mandatory and really missing on these machines, we need
another way to identify such systems without codec mask checks.
Maybe DMI or such listing; ugly but should be still manageable since
the number of devices hitting the problem must be fairly low.
Or, an alternative is to let ASoC driver bind at first, e.g. by
changing the driver name from snd-soc-skl to snd-hda-asoc, and drop
the selective binding from snd-hda-intel, so that it'd work as
catch-all after binding with ASoC fails.
The above would work for the modules, but for built-in, we need to
rearrange the link order, too, supposedly.
(For the mixed case with built-in and module? I don't know, maybe we
may forgive us by the presence of module options to control the
binding.)
BTW, one thing I'd really like to avoid is to rearrange the probe
procedure of the legacy HDA driver (so that we can get codec_mask
during pci probe() call). The async probe is the result of the many
struggles with the various and complex configurations. Moving the
codec probe to the beginning isn't trivial and quite risky to break
something else.
thanks,
Takashi
Re: [GIT PULL] sound updates for 4.21
On Fri, Dec 28, 2018 at 9:07 AM Takashi Iwai wrote:
>
> 1) Whether ASoC driver cannot work with these Dell machines at all
I'm willing to test patches.
Right now, the reason it fails to even load is that "codec_mask" is 0x0001.
And the skl_hda_dsp_generic.c code requires
// This will be 1 for 0x0001
codec_count = hweight_long(codec_mask);
if (codec_count == 1 && codec_mask & IDISP_CODEC_MASK) {
.. this is ok ..
} else if (codec_count == 2 && codec_mask & IDISP_CODEC_MASK) {
.. as is this ..
} else {
.. but here we return -EINVAL;
}
and that basically requires that IDISP_CODEC_MASK be part of
codec_mask. Which it isn't (IDISP_CODEC_MASK is 0x4).
Anyway, as long as the ASoC driver refuses to touch this, the default
pretty much *has* to be the legacy PCI driver.
Linus
Re: [GIT PULL] sound updates for 4.21
On Fri, 28 Dec 2018 13:43:03 +0100,
Ingo Molnar wrote:
>
>
> * Linus Torvalds wrote:
>
> > On Thu, Dec 20, 2018 at 7:38 AM Takashi Iwai wrote:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> > > tags/sound-4.21-rc1
> >
> > Hmm.
> >
> > It turns out that commit c337104b1a16 ("ALSA: HD-Audio: SKL+: abort
> > probe if DSP is present and Skylake driver selected") causes my laptop
> > (XPS13 9350) to no longer suspend.
>
> Just a wild guess, I can see two ways in which that commit could make a
> difference on your setup:
>
> 1)
>
> If any of these is not set in your .config:
>
> + select SND_HDA_INTEL_DSP_DETECTION_SKL if SND_SOC_INTEL_SKL
> + select SND_HDA_INTEL_DSP_DETECTION_APL if SND_SOC_INTEL_APL
> + select SND_HDA_INTEL_DSP_DETECTION_KBL if SND_SOC_INTEL_KBL
> + select SND_HDA_INTEL_DSP_DETECTION_GLK if SND_SOC_INTEL_GLK
> + select SND_HDA_INTEL_DSP_DETECTION_CNL if SND_SOC_INTEL_CNL
> + select SND_HDA_INTEL_DSP_DETECTION_CFL if SND_SOC_INTEL_CFL
>
> I.e. I'd enable all of the SND_SOC_INTEL_* options to cover this angle.
>
> 2)
>
> There's the added logic of checking whether the DSP is enabled:
>
> + /* check if this driver can be used on SKL+ Intel platforms */
> + if ((pci_id->driver_data & AZX_DCAPS_INTEL_SHARED) &&
> + pci->class != 0x040300)
> + return -ENODEV;
> +
>
> if pci->class is not 0x040300 the driver could end up not detecting the
> device while previously it would.
>
> That code goes through several transformations later on - but the hack
> below should make the commit an invariant. I think. Totally untested
> though.
Yes, Linus pointed out a similar "fix" to make things working again in
a later thread without Cc to ML.
The problem seems to be that this laptop doesn't work with ASoC Intel
SST driver now we're trying to bind when DSP is available. From
heuristics, it's identified from PCI class number, but this doesn't
seem enough on some models, unfortunately.
And, the old behavior (bind with "legacy" HDA-Intel driver) should be
achieved simply by passing pci_binding=1 option to snd-hda-intel
module, i.e. a patch like below would be enough:
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -172,7 +172,7 @@ module_param_array(beep_mode, bool, NULL, 0444);
MODULE_PARM_DESC(beep_mode, "Select HDA Beep registration mode "
"(0=off, 1=on) (default=1).");
#endif
-static int skl_pci_binding;
+static int skl_pci_binding = 1;
module_param_named(pci_binding, skl_pci_binding, int, 0444);
MODULE_PARM_DESC(pci_binding, "PCI binding (0=auto, 1=only legacy, 2=only
asoc");
===
Actually, there are two aspects wrt this problem:
1) Whether ASoC driver cannot work with these Dell machines at all
2) Whether we want to keep binding with the legacy HDA driver even if
DSP is available
AFAIK, the problem seems specific to some Dell models (XPS13 or such),
and I thought Intel already tested with these laptops. But maybe the
behavior differs depending on the model number or BIOS. IIRC, XPS13
already showed a drastic change of the HD-audio binding by BIOS
upgrades in the past, too. So (1) might be some machine-specific
fixes in the end.
But, even if (1) is fixed somehow, the user-visible behavior may be
slightly different from the earlier kernels (e.g. routing setup
required, etc), and this might annoy existing users. That's the
question in (2).
If we prefer being conservative and keeping the binding with the
legacy HDA driver as-is, it'd be reasonable to provide (yet) another
Kconfig to choose the default option value above.
Or, we may add some blacklisting (e.g. via DMI matching) in HDA-Intel
driver side to skip the faulty PCI class check.
I'm not sure which solution we'll take in the end, but certainly it's
a bug that has to be fixed soonish.
thanks,
Takashi
Re: [GIT PULL] sound updates for 4.21
* Linus Torvalds wrote:
> On Thu, Dec 20, 2018 at 7:38 AM Takashi Iwai wrote:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> > tags/sound-4.21-rc1
>
> Hmm.
>
> It turns out that commit c337104b1a16 ("ALSA: HD-Audio: SKL+: abort
> probe if DSP is present and Skylake driver selected") causes my laptop
> (XPS13 9350) to no longer suspend.
Just a wild guess, I can see two ways in which that commit could make a
difference on your setup:
1)
If any of these is not set in your .config:
+ select SND_HDA_INTEL_DSP_DETECTION_SKL if SND_SOC_INTEL_SKL
+ select SND_HDA_INTEL_DSP_DETECTION_APL if SND_SOC_INTEL_APL
+ select SND_HDA_INTEL_DSP_DETECTION_KBL if SND_SOC_INTEL_KBL
+ select SND_HDA_INTEL_DSP_DETECTION_GLK if SND_SOC_INTEL_GLK
+ select SND_HDA_INTEL_DSP_DETECTION_CNL if SND_SOC_INTEL_CNL
+ select SND_HDA_INTEL_DSP_DETECTION_CFL if SND_SOC_INTEL_CFL
I.e. I'd enable all of the SND_SOC_INTEL_* options to cover this angle.
2)
There's the added logic of checking whether the DSP is enabled:
+ /* check if this driver can be used on SKL+ Intel platforms */
+ if ((pci_id->driver_data & AZX_DCAPS_INTEL_SHARED) &&
+ pci->class != 0x040300)
+ return -ENODEV;
+
if pci->class is not 0x040300 the driver could end up not detecting the
device while previously it would.
That code goes through several transformations later on - but the hack
below should make the commit an invariant. I think. Totally untested
though.
Thanks,
Ingo
==>
sound/pci/hda/hda_intel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index e42cc2230977..f9e9c87f6d15 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2056,7 +2056,7 @@ static int azx_probe(struct pci_dev *pci,
if (pci_id->driver_data & AZX_DCAPS_INTEL_SHARED) {
switch (skl_pci_binding) {
case SND_SKL_PCI_BIND_AUTO:
- if (pci->class != 0x040300) {
+ if (0 && pci->class != 0x040300) {
dev_info(>dev, "The DSP is enabled on this
platform, aborting probe\n");
return -ENODEV;
}
Re: [GIT PULL] sound updates for 4.21
On Thu, Dec 20, 2018 at 7:38 AM Takashi Iwai wrote:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git
> tags/sound-4.21-rc1
Hmm.
It turns out that commit c337104b1a16 ("ALSA: HD-Audio: SKL+: abort
probe if DSP is present and Skylake driver selected") causes my laptop
(XPS13 9350) to no longer suspend.
The screen goes dark, but the power light never turns off. There's
nothing in the logs.
> Pierre-Louis Bossart (16):
> ALSA: HD-Audio: SKL+: abort probe if DSP is present and Skylake driver
> selected
Sadly, that commit does not revert cleanly, since it's preparatory for
a lot of other changes.
Here's the lspci output:
/sbin/lspci -s 0:1f.3 -vn
00:1f.3 0403: 8086:9d70 (rev 21) (prog-if 80)
Subsystem: 1028:0704
Flags: bus master, fast devsel, latency 32, IRQ 16
Memory at dc328000 (64-bit, non-prefetchable) [size=16K]
Memory at dc30 (64-bit, non-prefetchable) [size=64K]
Capabilities:
Kernel driver in use: snd_soc_skl
Kernel modules: snd_hda_intel, snd_soc_skl
and it's a bog-standard Intel i7-6560U laptop.
I have no real information, except that I bisected the (reliable)
behavior to this commit. Before that commit, suspend/resume works
fine. After that commit, suspend just hangs and leaves a dead machine.
Linus
Re: [GIT PULL] sound updates for 4.21
The pull request you sent on Thu, 20 Dec 2018 16:38:30 +0100: > git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git > tags/sound-4.21-rc1 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/8e61e7b5c4de2bea534438bd7a008accd85492b0 Thank you! -- Deet-doot-dot, I am a bot. https://korg.wiki.kernel.org/userdoc/prtracker
[GIT PULL] sound updates for 4.21
Linus, please pull sound updates for v4.21 from: git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git tags/sound-4.21-rc1 The topmost commit is d82b51c855a20eb456ac09f2f40ea98312373263 sound updates for 4.21 There are no intensive changes in both ALSA and ASoC core parts while rather most of changes are a bunch of driver fixes and updates. A large diff pattern appears in ASoC TI part which now merges both OMAP and DaVinci stuff, but the rest spreads allover the places. Note that this pull request includes also some updates for LED trigger and platform drivers for mute LEDs, appearing in the diffstat as well. Some highlights: ASoC: - Preparatory work for merging the audio-graph and audio-graph-scu cards - A merge of TI OMAP and DaVinci directories, as both product lines get merged together. Also including a few architecture changes as well. - Major cleanups of the Maxim MAX9867 driver - Small fixes for tablets & co with Intel BYT/CHT chips - Lots of rsnd updates as usual - Support for Asahi Kaesi AKM4118, AMD ACP3x, Intel platforms with RT5660, Meson AXG S/PDIF inputs, several Qualcomm IPs and Xilinx I2S controllers HD-audio: - Introduce audio-mute LED trigger for replacing the former hackish dynamic binding - Huawei WMI hotkey and mute LED support - Refactoring of PM code and display power controls - Headset button support in the generic jack code - A few updates for Tegra - Fixups for HP EliteBook and ASUS UX391UA - Lots of updates for Intel ASoC HD-audio, including the improved DSP detection and the fallback binding from ASoC SST to legacy HD-audio controller drivers Others: - Updates for FireWire TASCAM and Fireface devices, some other fixes - A few potential Spectre v1 fixes that are all trivial Adrien Charruel (1): ASoC: ak4118: Add support for AK4118 S/PDIF transceiver Arnd Bergmann (6): ASoC: wm97xx: fix uninitialized regmap pointer problem ASoC: Intel: mrfld: fix uninitialized variable access ASoC: pxa: change ac97 dependencies ASoC: sdm845: add rt5663 codec select ASoC: simple-card-utils: fix build warning without CONFIG_OF ALSA: hda/ca0132 - make pci_iounmap() call conditional Axel Lin (1): ASoC: ak5558: Remove redundant snd_soc_component_read32 calls Ayman Bagabas (3): ALSA: hda: fix front speakers on Huawei MBXP platform/x86: add support for Huawei WMI hotkeys ALSA: hda: add support for Huawei WMI micmute LED Bard liao (2): ASoC: Intel: common: add SOF information for APL RVP ASoC: Intel: hdac_hdmi: add Icelake support Chen-Yu Tsai (2): ASoC: dt-bindings: sun50i-codec-analog: Add headphone amp regulator supply ASoC: sunxi: sun50i-codec-analog: Add support for cpvdd regulator supply Cheng-Yi Chiang (7): ASoC: rt5663: Add regulator support ASoC: rt5663: Add documentation for power supply support ASoC: rt5663: Fix error handling of regulator_set_load ASoC: qcom: sdm845: Add board specific dapm widgets ASoC: qcom: sdm845: Create and setup jack in init callback ASoC: sdm845: Add TDM configuration for speaker ASoC: sdm845: Add configuration for headset codec Clément Péron (1): ASoC: dt-bindings: add bindings for AK4118 transceiver Colin Ian King (8): ASoC: stm32: sai: fix less than zero comparison on unsigned int ASoC: amd: fix memory leak of i2s_data on error return ASoC: tlv320aic31xx: asihpi: clean up indentation, remove extraneous tab ASoC: tlv320dac33: clean up indentation, remove extraneous tab ASoC: arizona: fix indentation issue with return statement ASoC: qcom: clean up indentation, remove extraneous tab ASoC: amd: fix spelling mistake "Inavlid" -> "Invalid" ALSA: asihpi: clean up indentation, replace spaces with tab Dan Carpenter (1): ASoC: amd: Fix a NULL vs IS_ERR() check in probe Daniel Mack (5): ASoC: pxa: remove raumfeld machine driver ASoC: dt-bindings: cs4270: use 'reset-gpios' rather than 'reset-gpio' ASoC: codecs: cs4270: move to GPIO consumer API ASoC: dt-bindings: ak4104: use 'reset-gpios' rather than 'reset-gpio' ASoC: codecs: ak4104: move to GPIO consumer API David Lin (2): ASoC: nau8822: convert to SPDX identifiers ASoC: nau8822: convert to SPDX identifiers Dimitris Papavasiliou (1): ASoC: pcm512x: Implement the digital_mute interface Fabio Estevam (2): ASoC: fsl: Fix SND_SOC_EUKREA_TLV320 build error on i.MX8M ASoC: fsl-sai: Fix typo in "transmitter" Fabrizio Castro (1): ASoC: rsnd: Add r8a774c0 support Gustavo A. R. Silva (4): ALSA: emux: Fix potential Spectre v1 vulnerabilities ALSA: pcm: Fix potential Spectre v1 vulnerability ALSA: rme9652: Fix potential Spectre v1 vulnerability ALSA: emu10k1: Fix potential
