Enable WAKEEN feature in sof hda driver #983

RanderWang · 2019-05-27T09:56:37Z

In commit 7d4f606 ("ALSA: hda - WAKEEN feature enabling for
runtime pm"), legacy HD-A driver sets hda controller in reset mode after
entering runtime-suspend. And when resuming from suspend mode, it checks
hda controller & codec status to detect headphone hotplug event. Now
this patch does the same job in SOF runtime pm functions.

Tested on whiskylake with hda codecs

This fixes #909

keyonjie

Great job, looks good to me.

ranj063 · 2019-05-28T03:32:59Z

sound/soc/sof/intel/hda-dsp.c

@@ -375,6 +384,14 @@ static int hda_resume(struct snd_sof_dev *sdev)
 	snd_hdac_ext_bus_ppcap_enable(bus, true);
 	snd_hdac_ext_bus_ppcap_int_enable(bus, true);

+	/* check jack status based on controller status */
+	hda_codec_jack_poll(sdev, status);


@RanderWang the legacy hda driver seems to do this only for runtime resume. Should we also have a check here for runtime resume and do this only then?

yes, I will refine it. Thanks!

RanderWang · 2019-05-28T09:28:28Z

Update PR, check runtime suspend

xiulipan · 2019-05-28T11:06:28Z

@RanderWang
This PR will make device hang when try to play with Port5 on APL_UP2_PCM512X device.
Please manually check.

plbossart

Minor fixes needed (also fix typos in commit messages) but this looks fine in general. Thanks @RanderWang !

plbossart · 2019-05-28T18:00:25Z

sound/soc/sof/intel/hda-dsp.c

@@ -282,7 +282,7 @@ void hda_dsp_ipc_int_disable(struct snd_sof_dev *sdev)
 			HDA_DSP_REG_HIPCCTL_BUSY | HDA_DSP_REG_HIPCCTL_DONE, 0);
 }

-static int hda_suspend(struct snd_sof_dev *sdev, int state)
+static int hda_suspend(struct snd_sof_dev *sdev, int state, int runtime)


bool suspend_runtime for consistency with sound/soc/sof/pm.c

plbossart · 2019-05-28T18:00:45Z

sound/soc/sof/intel/hda-dsp.c

@@ -340,11 +347,12 @@ static int hda_suspend(struct snd_sof_dev *sdev, int state)
 	return 0;
 }

-static int hda_resume(struct snd_sof_dev *sdev)
+static int hda_resume(struct snd_sof_dev *sdev, int runtime)


bool resume_runtime for consistency with sound/soc/sof/pm.c

sound/soc/sof/intel/hda-dsp.c

plbossart · 2019-05-28T18:03:00Z

sound/soc/sof/intel/hda-dsp.c

@@ -422,19 +445,19 @@ static int hda_resume(struct snd_sof_dev *sdev)
 int hda_dsp_resume(struct snd_sof_dev *sdev)
 {
 	/* init hda controller. DSP cores will be powered up during fw boot */
-	return hda_resume(sdev);
+	return hda_resume(sdev, 0);


plbossart · 2019-05-28T18:03:11Z

sound/soc/sof/intel/hda-dsp.c

 }

 int hda_dsp_runtime_resume(struct snd_sof_dev *sdev)
 {
 	/* init hda controller. DSP cores will be powered up during fw boot */
-	return hda_resume(sdev);
+	return hda_resume(sdev, 1);


plbossart · 2019-05-28T18:03:20Z

sound/soc/sof/intel/hda-dsp.c

 }

 int hda_dsp_runtime_suspend(struct snd_sof_dev *sdev, int state)
 {
 	/* stop hda controller and power dsp off */
-	return hda_suspend(sdev, state);
+	return hda_suspend(sdev, state, 1);


plbossart · 2019-05-28T18:03:27Z

sound/soc/sof/intel/hda-dsp.c

@@ -443,7 +466,7 @@ int hda_dsp_suspend(struct snd_sof_dev *sdev, int state)
 	int ret;

 	/* stop hda controller and power dsp off */
-	ret = hda_suspend(sdev, state);
+	ret = hda_suspend(sdev, state, 0);


plbossart · 2019-05-28T18:05:50Z

@RanderWang
This PR will make device hang when try to play with Port5 on APL_UP2_PCM512X device.
Please manually check.

likely because it should only be used with an external HDaudio codec?

RanderWang · 2019-05-29T02:25:21Z

@RanderWang
This PR will make device hang when try to play with Port5 on APL_UP2_PCM512X device.
Please manually check.

likely because it should only be used with an external HDaudio codec?

yes, it should be. I will check how to make APL_UP2_PCM512X hang

RanderWang · 2019-05-29T02:55:44Z

@RanderWang
This PR will make device hang when try to play with Port5 on APL_UP2_PCM512X device.
Please manually check.

so now CI works ?

xiulipan · 2019-05-29T08:54:43Z

@RanderWang
Some error logs here. Not sure if we have some regression on PCM512X tplg or some otherthing.

[    4.505626 <    0.000004>] sof-audio-pci 0000:00:0e.0: tplg: 1 hw_configs found, default id: 1!
[    4.505647 <    0.000021>] sof-audio-pci 0000:00:0e.0: ipc tx: 0x80010000: GLB_DAI_MSG: CONFIG
[    4.505707 <    0.000060>] sof-audio-pci 0000:00:0e.0: error: ipc error for 0x80010000 size 12
[    4.505709 <    0.000002>] sof-audio-pci 0000:00:0e.0: error: failed to set DAI config for direction:0 of HDA dai 0
[    4.505710 <    0.000001>] sof-audio-pci 0000:00:0e.0: error: failed to process hda dai link iDisp1

xiulipan · 2019-05-29T09:18:19Z

@RanderWang
conformed with manually check

It seems we have some code base regression or your patch have some issue.

[   22.447793] sof-audio-pci 0000:00:0e.0: error: ipc error for 0x80010000 size 12
[   22.447798] sof-audio-pci 0000:00:0e.0: error: failed to set DAI config for direction:0 of HDA dai 0
[   22.447800] sof-audio-pci 0000:00:0e.0: error: failed to process hda dai link iDisp1
[   22.447802] sof-audio-pci 0000:00:0e.0: ASoC: physical link loading failed
[   22.447821] sof-audio-pci 0000:00:0e.0: FW Poll Status: reg=0x1010303 successful
[   22.447825] sof-audio-pci 0000:00:0e.0: FW Poll Status: reg=0x1000303 successful
[   22.447828] sof-audio-pci 0000:00:0e.0: DSP core(s) enabled? 0 : core_mask 1
[   22.447833] sof-audio-pci 0000:00:0e.0: FW Poll Status: reg=0x303 successful
[   22.447837] sof-audio-pci 0000:00:0e.0: FW Poll Status: reg=0x303 successful
[   22.447840] sof-audio-pci 0000:00:0e.0: DSP core(s) enabled? 0 : core_mask 1
[   22.447845] sof-audio-pci 0000:00:0e.0: FW Poll Status: reg=0x303 successful
[   22.447849] sof-audio-pci 0000:00:0e.0: FW Poll Status: reg=0x303 successful
[   22.447852] sof-audio-pci 0000:00:0e.0: DSP core(s) enabled? 0 : core_mask 1
[   22.447857] sof-audio-pci 0000:00:0e.0: FW Poll Status: reg=0x303 successful
[   22.447861] sof-audio-pci 0000:00:0e.0: FW Poll Status: reg=0x303 successful
[   22.447864] sof-audio-pci 0000:00:0e.0: DSP core(s) enabled? 0 : core_mask 1
[   22.447870] sof-audio-pci 0000:00:0e.0: FW Poll Status: reg=0x303 successful
[   22.447874] sof-audio-pci 0000:00:0e.0: FW Poll Status: reg=0x303 successful
[   22.447877] sof-audio-pci 0000:00:0e.0: DSP core(s) enabled? 0 : core_mask 1
[   22.447882] sof-audio-pci 0000:00:0e.0: error: tplg component load failed -5
[   22.447894] sof-audio-pci 0000:00:0e.0: error: failed to load DSP topology -22
[   22.447896] sof-audio-pci 0000:00:0e.0: ASoC: failed to probe component -22
[   22.447928] bxt-pcm512x bxt-pcm512x: ASoC: failed to instantiate card -22
[   22.447963] bxt-pcm512x bxt-pcm512x: snd_soc_register_card failed -22
[   22.447978] bxt-pcm512x: probe of bxt-pcm512x failed with error -22

lyakh · 2019-05-29T09:03:36Z

sound/soc/sof/intel/hda-codec.c

+	struct hda_bus *bus = sof_to_hbus(sdev);
+	struct hda_codec *codec;
+
+	if (status) {


I'd make this

if (!status) return;

thanks, I will refine it.

lyakh · 2019-05-29T09:03:53Z

sound/soc/sof/intel/hda-codec.c

+			if (status & (1 << codec->core.addr)) {
+				schedule_delayed_work(&codec->jackpoll_work,
+						      codec->jackpoll_interval);
+			}


superfluous braces

thanks, I will refine it.

lyakh · 2019-05-29T09:17:01Z

sound/soc/sof/intel/hda-dsp.c

@@ -375,6 +395,18 @@ static int hda_resume(struct snd_sof_dev *sdev)
 	snd_hdac_ext_bus_ppcap_enable(bus, true);
 	snd_hdac_ext_bus_ppcap_int_enable(bus, true);

+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
+	if (resume_runtime) {


I assume reading STATESTS here wouldn't work? It has to be done earlier?

STATESTS would be cleared if chip is initialized, so read if before chip initialization.

xiulipan · 2019-05-29T09:31:20Z

@RanderWang @ranj063
Sorry to board, debug to find the code base mismatch for #895 and thesofproject/sof#1354
@keyonjie
Same to your #888

RanderWang · 2019-05-29T10:11:30Z

update my PR according to advise from lyakh

plbossart · 2019-05-29T15:11:23Z

@ranj063 need to root cause HDAudio issues here as well. Something's not right.

ranj063 · 2019-05-29T15:14:29Z

@ranj063 need to root cause HDAudio issues here as well. Something's not right.

@plbossart I just checked the dmesg for the Up2 failure and it looks like the fw change from 1354 was not merged when this test was run. I will test UP2 today just in case

plbossart

one minor confusion in variable naming (likely copy/paste) but looks good otherwise.
Thanks @RanderWang

plbossart · 2019-05-29T15:12:55Z

sound/soc/sof/intel/hda-dsp.c

@@ -282,7 +282,7 @@ void hda_dsp_ipc_int_disable(struct snd_sof_dev *sdev)
 			HDA_DSP_REG_HIPCCTL_BUSY | HDA_DSP_REG_HIPCCTL_DONE, 0);
 }

-static int hda_suspend(struct snd_sof_dev *sdev, int state)
+static int hda_suspend(struct snd_sof_dev *sdev, int state, bool resume_runtime)


should be suspend_runtime...

very sorry, Thanks Pierre. I will correct it.

plbossart

@RanderWang the code is fine but we can remove a few ifdefs and make the code more readable. see comments below.

plbossart · 2019-05-30T04:12:59Z

sound/soc/sof/intel/hda-dsp.c

@@ -295,6 +296,15 @@ static int hda_suspend(struct snd_sof_dev *sdev, int state)
 	hda_dsp_ipc_int_disable(sdev);

 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
+	if (runtime_suspend) {


You could make the code more readable with

if (IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC) && runtime_suspend)

sound/soc/sof/intel/hda-dsp.c

plbossart · 2019-05-30T04:14:31Z

sound/soc/sof/intel/hda-dsp.c

+				     snd_hdac_chip_readw(bus, WAKEEN) &
+				     ~STATESTS_INT_MASK);
+	}
+#endif


you can use a helper that does nothing when HDA_AUDIO_CODEC is not defined. This would make the code more readable

plbossart · 2019-05-30T04:14:59Z

sound/soc/sof/intel/hda.h

-
+#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
+void hda_codec_jack_poll(struct snd_sof_dev *sdev, int status);
+#endif


no need for conditional definition of prototype

RanderWang · 2019-05-30T10:48:46Z

@plbossart thanks for your comments. This PR worked on my RVP but on production devices I found one issue:
kernel reports a warning.
WARN_ON_ONCE(timer->function != delayed_work_timer_fn);
WARN_ON_ONCE(timer_pending(timer));

I will check it and update my PR later.

Thanks!

RanderWang · 2019-06-04T09:35:58Z

@plbossart @ranj063 @keyonjie @kv2019i
I updated the PR. Now this feature works with both legacy interrupt and MSI interrupt.
The wake-up feature will introduce more interrupts and we need to refine our interrupt function
to make it work.

I and QA did stress test on a few whiskylake devices. I also validated it on apollolake up2

plbossart

This looks like good progress but we should probably dig a bit deeper before merging. I saw weird interrupts when SoundWire is enabled, so something is not quite right in our interrupt handling, maybe we are missing a sequence.

plbossart · 2019-06-04T15:20:20Z

sound/soc/sof/intel/hda-dsp.c

@@ -266,6 +266,8 @@ void hda_dsp_ipc_int_enable(struct snd_sof_dev *sdev)
 	/* enable IPC interrupt */
 	snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR, HDA_DSP_REG_ADSPIC,
 				HDA_DSP_ADSPIC_IPC, HDA_DSP_ADSPIC_IPC);
+
+	sdev->ipc_enabled = 1;


does this belong in the main sdev structure or in the hda-specific ones?

plbossart · 2019-06-04T15:20:56Z

sound/soc/sof/intel/hda-dsp.c

@@ -266,6 +266,8 @@ void hda_dsp_ipc_int_enable(struct snd_sof_dev *sdev)
 	/* enable IPC interrupt */
 	snd_sof_dsp_update_bits(sdev, HDA_DSP_BAR, HDA_DSP_REG_ADSPIC,
 				HDA_DSP_ADSPIC_IPC, HDA_DSP_ADSPIC_IPC);
+
+	sdev->ipc_enabled = 1;


should this be protected with the ipc_spinlock?

plbossart · 2019-06-04T15:22:30Z

sound/soc/sof/intel/hda-ipc.c

@@ -244,6 +244,9 @@ irqreturn_t hda_dsp_ipc_irq_handler(int irq, void *context)

 	spin_lock(&sdev->hw_lock);

+	if (!sdev->ipc_enabled)
+		goto out;


this is really interesting. In the SoundWire work, we also get interrupts on WHL before they are even enabled. There is something really fishy here, it's as if the interrupt masking doesn't fully work. We should check with hardware folks here.

RanderWang · 2019-06-05T01:29:47Z

@plbossart yes, we need to dig a bit deeper before merging. This feature reveals the defect of our interrupt handling, and the IPC fix may enlighten us to find root reason.

ranj063 · 2019-06-05T03:23:00Z

@RanderWang the other missing piece in the MSI/WAKEEN puzzle is that after we enable WAKEEN during runtime suspend and the headset is plugged in, the audio device enters D0 and never goes back to D3 even if no audio is playing and the headset is plugged out.

lyakh · 2019-06-05T17:43:49Z

sound/soc/sof/intel/hda-codec.c

+	list_for_each_codec(codec, hbus)
+		if (status & (1 << codec->core.addr))
+			schedule_delayed_work(&codec->jackpoll_work,
+					      codec->jackpoll_interval);


This doesn't work, it only produces kernel warnings, since the jackpool_work isn't initialised in SOF. Further, even though with some tricking that work can be abused for our purpose, its initial purpose is codec polling when the codec cannot send unsolicited events.

I didn't make some tricking thing and both I and QA tested on a few devices.
This initial purpose has been reused in runtime resume function , please check __azx_runtime_resume

This doesn't work, it only produces kernel warnings, since the jackpool_work isn't initialised in SOF.

@lyakh Can you share me your test device info so that I can debug on it. Thanks!

@lyakh jackpoo_work is initialized by hdac_hda codec. I checked kernel document and didn't find any limitation about this case. Could do share with us more information ? thanks!

@RanderWang I was getting these warnings, here from a Wasp log:

Jun 4 15:30:40 wasp kernel: [ 10.429822] WARNING: CPU: 1 PID: 62 at /usr/local/src/project/51/src/linux/kernel/workqueue.c:1626 __queue_delayed_work+0x68/0x80 Jun 4 15:30:40 wasp kernel: [ 10.429822] Modules linked in: uvcvideo videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videobuf2_common videodev media snd_soc_skl_hda_dsp snd_soc_hdac_hdmi snd_soc_dmic snd_hda_codec_realtek snd_hda_codec_generic sof_pci_dev snd_sof_intel_hda_common snd_soc_hdac_hda snd_sof_intel_hda snd_sof_intel_byt snd_soc_acpi_intel_match snd_sof_intel_ipc snd_sof snd_sof_xtensa_dsp snd_soc_acpi snd_hda_ext_core snd_soc_core x86_pkg_temp_thermal snd_hda_codec intel_powerclamp snd_hwdep snd_hda_core hid_multitouch iwlmvm snd_pcm mei_me iwlwifi mei processor_thermal_device intel_soc_dts_iosf intel_pch_thermal int3403_thermal int340x_thermal_zone int3400_thermal acpi_thermal_rel efivarfs sdhci_pci xhci_pci intel_lpss_pci cqhci intel_lpss xhci_hcd mfd_core sdhci i2c_hid Jun 4 15:30:40 wasp kernel: [ 10.429837] CPU: 1 PID: 62 Comm: kworker/1:1 Not tainted 5.2.0-rc1+ #39 Jun 4 15:30:40 wasp kernel: [ 10.429838] Hardware name: Dell Inc. Vostro 5391/, BIOS 0.1.4 05/02/2019 Jun 4 15:30:40 wasp kernel: [ 10.429841] Workqueue: pm pm_runtime_work Jun 4 15:30:40 wasp kernel: [ 10.429843] RIP: 0010:__queue_delayed_work+0x68/0x80 Jun 4 15:30:40 wasp kernel: [ 10.429844] Code: 48 01 c1 41 83 f8 40 48 89 4a 30 75 2a e9 80 62 05 00 44 89 c7 e9 88 f4 ff ff 0f 0b eb ce 0f 0b 48 81 7a 38 a0 28 c8 b8 74 ae <0f> 0b 48 83 7a 28 00 74 ac 0f 0b eb a8 44 89 c6 e9 23 53 05 00 0f Jun 4 15:30:40 wasp kernel: [ 10.429844] RSP: 0018:ffffafd540f3bc60 EFLAGS: 00010007 Jun 4 15:30:40 wasp kernel: [ 10.429845] RAX: 0000000000000002 RBX: 0000000000000283 RCX: 0000000000000000 Jun 4 15:30:40 wasp kernel: [ 10.429846] RDX: ffff9d9961f8e610 RSI: ffff9d9963c10600 RDI: ffff9d9961f8e630 Jun 4 15:30:40 wasp kernel: [ 10.429846] RBP: ffff9d9961f8c888 R08: 0000000000000040 R09: ffff9d9961f8d618 Jun 4 15:30:40 wasp kernel: [ 10.429846] R10: 00000000000000fb R11: 000000000000072b R12: 0000000000000005 Jun 4 15:30:40 wasp kernel: [ 10.429847] R13: ffffffffc0671aa0 R14: 0000000000000005 R15: ffff9d9961f8cce0 Jun 4 15:30:40 wasp kernel: [ 10.429848] FS: 0000000000000000(0000) GS:ffff9d9964240000(0000) knlGS:0000000000000000 Jun 4 15:30:40 wasp kernel: [ 10.429848] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 Jun 4 15:30:40 wasp kernel: [ 10.429849] CR2: 00005561398d3038 CR3: 000000014320a002 CR4: 00000000003606e0 Jun 4 15:30:40 wasp kernel: [ 10.429849] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 Jun 4 15:30:40 wasp kernel: [ 10.429850] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 Jun 4 15:30:40 wasp kernel: [ 10.429850] Call Trace: Jun 4 15:30:40 wasp kernel: [ 10.429853] queue_delayed_work_on+0x11/0x20 Jun 4 15:30:40 wasp kernel: [ 10.429855] hda_codec_jack_check+0x94/0xa0 [snd_sof_intel_hda] Jun 4 15:30:40 wasp kernel: [ 10.429858] hda_resume+0x14e/0x180 [snd_sof_intel_hda_common] Jun 4 15:30:40 wasp kernel: [ 10.429861] ? pci_restore_standard_config+0x40/0x40 Jun 4 15:30:40 wasp kernel: [ 10.429863] sof_resume+0x129/0x470 [snd_sof] Jun 4 15:30:40 wasp kernel: [ 10.429864] ? pci_restore_standard_config+0x40/0x40 Jun 4 15:30:40 wasp kernel: [ 10.429865] pci_pm_runtime_resume+0x6c/0xc0 Jun 4 15:30:40 wasp kernel: [ 10.429867] ? pci_restore_standard_config+0x30/0x40 Jun 4 15:30:40 wasp kernel: [ 10.429868] __rpm_callback+0xc1/0x120 Jun 4 15:30:40 wasp kernel: [ 10.429869] ? pci_restore_standard_config+0x40/0x40 Jun 4 15:30:40 wasp kernel: [ 10.429870] rpm_callback+0x18/0x80 Jun 4 15:30:40 wasp kernel: [ 10.429871] ? pci_restore_standard_config+0x40/0x40 Jun 4 15:30:40 wasp kernel: [ 10.429872] rpm_resume+0x4ea/0x670 Jun 4 15:30:40 wasp kernel: [ 10.429873] rpm_suspend+0x53b/0x650 Jun 4 15:30:40 wasp kernel: [ 10.429875] pm_runtime_work+0x82/0x90 Jun 4 15:30:40 wasp kernel: [ 10.429876] process_one_work+0x15d/0x3b0 Jun 4 15:30:40 wasp kernel: [ 10.429877] worker_thread+0x62/0x400 Jun 4 15:30:40 wasp kernel: [ 10.429879] ? rescuer_thread+0x340/0x340 Jun 4 15:30:40 wasp kernel: [ 10.429880] kthread+0x11e/0x150 Jun 4 15:30:40 wasp kernel: [ 10.429882] ? kthread_bind+0x10/0x10 Jun 4 15:30:40 wasp kernel: [ 10.429884] ret_from_fork+0x1f/0x40 Jun 4 15:30:40 wasp kernel: [ 10.429885] ---[ end trace 722112b9053fe9bc ]---

I'm trying to recover the state, when I was getting those warnings to study them in a bit more detail.

It cost me about half a day, but now I have it. Basically you need something like:

diff --git a/include/sound/hda_codec.h b/include/sound/hda_codec.h index cc7c8d4..4daf126 100644 --- a/include/sound/hda_codec.h +++ b/include/sound/hda_codec.h @@ -533,4 +533,6 @@ void snd_hda_codec_load_dsp_cleanup(struct hda_codec *codec, struct snd_dma_buffer *dmab) {} #endif +void hda_jackpoll_work(struct work_struct *work); + #endif /* __SOUND_HDA_CODEC_H */ diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index b20eb7f..5b70532 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -650,7 +650,7 @@ static void restore_shutup_pins(struct hda_codec *codec) } #endif -static void hda_jackpoll_work(struct work_struct *work) +void hda_jackpoll_work(struct work_struct *work) { struct hda_codec *codec = container_of(work, struct hda_codec, jackpoll_work.work); @@ -664,6 +664,7 @@ static void hda_jackpoll_work(struct work_struct *work) schedule_delayed_work(&codec->jackpoll_work, codec->jackpoll_interval); } +EXPORT_SYMBOL_GPL(hda_jackpoll_work); /* release all pincfg lists */ static void free_init_pincfgs(struct hda_codec *codec) diff --git a/sound/soc/codecs/hdac_hda.h b/sound/soc/codecs/hdac_hda.h index 6b1bd4f..e68be25 100644 --- a/sound/soc/codecs/hdac_hda.h +++ b/sound/soc/codecs/hdac_hda.h @@ -6,6 +6,8 @@ #ifndef __HDAC_HDA_H__ #define __HDAC_HDA_H__ +#include <sound/hda_codec.h> + struct hdac_hda_pcm { int stream_tag[2]; unsigned int format_val[2]; diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c index 660e058..b961270 100644 --- a/sound/soc/codecs/hdac_hdmi.c +++ b/sound/soc/codecs/hdac_hdmi.c @@ -32,6 +32,7 @@ #include <sound/hda_chmap.h> #include "../../hda/local.h" #include "hdac_hdmi.h" +#include "hdac_hda.h" #define NAME_SIZE 32 @@ -1987,13 +1988,17 @@ static int hdac_hdmi_get_spk_alloc(struct hdac_device *hdev, int pcm_idx) static int hdac_hdmi_dev_probe(struct hdac_device *hdev) { - struct hdac_hdmi_priv *hdmi_priv = NULL; + struct hdac_hdmi_priv *hdmi_priv; struct snd_soc_dai_driver *hdmi_dais = NULL; - struct hdac_ext_link *hlink = NULL; + struct hdac_ext_link *hlink; int num_dais = 0; int ret = 0; struct hdac_driver *hdrv = drv_to_hdac_driver(hdev->dev.driver); const struct hda_device_id *hdac_id = hdac_get_device_id(hdev, hdrv); + struct hdac_hda_priv *hda_priv = container_of(hdev, struct hdac_hda_priv, codec.core); + + dev_info(&hdev->dev, "%s(): priv %p\n", __func__, hda_priv); + INIT_DELAYED_WORK(&hda_priv->codec.jackpoll_work, hda_jackpoll_work); /* hold the ref while we probe */ hlink = snd_hdac_ext_bus_get_link(hdev->bus, dev_name(&hdev->dev));

@RanderWang I actually think it would be better to avoid setting WAKEEN bits for codecs like HDMI. Feel free to use this diff instead of the previous one:

diff --git a/sound/soc/sof/intel/hda-codec.c b/sound/soc/sof/intel/hda-codec.c index 2ae7783..465d6cd 100644 --- a/sound/soc/sof/intel/hda-codec.c +++ b/sound/soc/sof/intel/hda-codec.c @@ -45,19 +45,15 @@ void hda_codec_jack_check(struct snd_sof_dev *sdev, int status) struct hdac_bus *bus = sof_to_bus(sdev); struct hda_codec *codec; - if (!status) - goto disable; - - list_for_each_codec(codec, hbus) - if (status & (1 << codec->core.addr)) - schedule_delayed_work(&codec->jackpoll_work, - codec->jackpoll_interval); - -disable: /* disable controller Wake Up event*/ snd_hdac_chip_writew(bus, WAKEEN, snd_hdac_chip_readw(bus, WAKEEN) & ~STATESTS_INT_MASK); + + list_for_each_codec(codec, hbus) + if (status & (1 << codec->core.addr) && codec->jacktbl.used) + schedule_delayed_work(&codec->jackpoll_work, + codec->jackpoll_interval); } #else void hda_codec_jack_check(struct snd_sof_dev *sdev, int status) {} diff --git a/sound/soc/sof/intel/hda-dsp.c b/sound/soc/sof/intel/hda-dsp.c index b4611e7..f1483c5 100644 --- a/sound/soc/sof/intel/hda-dsp.c +++ b/sound/soc/sof/intel/hda-dsp.c @@ -288,7 +288,9 @@ static int hda_suspend(struct snd_sof_dev *sdev, int state, struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata; const struct sof_intel_dsp_desc *chip = hda->desc; #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) + struct hda_bus *hbus = sof_to_hbus(sdev); struct hdac_bus *bus = sof_to_bus(sdev); + struct hda_codec *codec; #endif int ret; @@ -296,11 +298,17 @@ static int hda_suspend(struct snd_sof_dev *sdev, int state, hda_dsp_ipc_int_disable(sdev); #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) - if (IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC) && runtime_suspend) + if (IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC) && runtime_suspend) { + unsigned int mask = 0; + + list_for_each_codec(codec, hbus) + if (codec->jacktbl.used) + mask |= BIT(codec->core.addr); + /* enable controller wake up event */ snd_hdac_chip_writew(bus, WAKEEN, - snd_hdac_chip_readw(bus, WAKEEN) | - STATESTS_INT_MASK); + snd_hdac_chip_readw(bus, WAKEEN) | mask); + } /* power down all hda link */ snd_hdac_ext_bus_link_power_down_all(bus);

Thanks! These warning message is caused by interrupt issue and I will validate it again on WASP with patch for interrupt. Thanks for your reference!

lyakh · 2019-06-05T17:45:17Z

sound/soc/sof/intel/hda-codec.c

+		goto disable;
+
+	list_for_each_codec(codec, hbus)
+		if (status & (1 << codec->core.addr))


BIT(codec->core.addr)

yes, I got the same idea

lyakh · 2019-06-05T17:48:47Z

sound/soc/sof/intel/hda-dsp.c

+		/* enable controller wake up event */
+		snd_hdac_chip_writew(bus, WAKEEN,
+				     snd_hdac_chip_readw(bus, WAKEEN) |
+				     STATESTS_INT_MASK);


In my branch I modified this your patch to do

snd_hdac_chip_writew(bus, WAKEEN, snd_hdac_chip_readw(bus, WAKEEN) | bus->codec_mask);

That's a bit better than enabling all interrupt sources, but still not perfect. Maybe we should exclude HDMI codecs too.

RanderWang · 2019-06-06T07:04:03Z

@RanderWang the other missing piece in the MSI/WAKEEN puzzle is that after we enable WAKEEN during runtime suspend and the headset is plugged in, the audio device enters D0 and never goes back to D3 even if no audio is playing and the headset is plugged out.

I also validated it but got a different behavior. I will check again and update this PR when all the interrupt issues have been resolved.

lyakh · 2019-06-07T06:58:49Z

sound/soc/sof/intel/hda-codec.c

+	struct hda_codec *codec;
+
+	if (!status)
+		goto disable;


I also don't think a goto is justified here. Maybe you can choose one of these solutions to eliminate it:

swap them

/* disable controller Wake Up event*/ snd_hdac_chip_writew(bus, WAKEEN, snd_hdac_chip_readw(bus, WAKEEN) & ~STATESTS_INT_MASK); if (!status) return; list_for_each_codec(...) ...

sacrifice performance - typically you'll have 1 or two codecs, so, you can just remove that if and run the list_for_each_codec(...) loop directly, checking each status bit in the loop.

explicit if

if (status) list_for_each_codec(codec, hbus) { ...

IMHO either of those options is better than a goto.

Thanks, I have another patch for other feature and goto will be discarded.

Move the code for hda to one point Signed-off-by: Rander Wang <[email protected]>

RanderWang · 2019-06-11T02:58:50Z

.

@ranj063

@RanderWang the other missing piece in the MSI/WAKEEN puzzle is that after we enable WAKEEN during runtime suspend and the headset is plugged in, the audio device enters D0 and never goes back to D3 even if no audio is playing and the headset is plugged out.

@ranj063 For D0 & D3 issue, I got it on sof-dev but there is not such issue on sof-v5.0. So it is a regression issue.

In commit 7d4f606 ("ALSA: hda - WAKEEN feature enabling for runtime pm"), legacy HD-A driver sets hda controller in reset mode after entering runtime-suspend. And when resuming from suspend mode, it checks hda controller & codec status to detect headphone hotplug event. Now this patch does the same job in SOF runtime pm functions. And we need to check all the non-hdmi codecs for some cases like playback with HDMI or capture with DMIC connected to dsp. In these cases, only controller is active and codecs are suspended, so codecs can't send unsolicited event to controller. The jack polling operation will activate codecs and unsolicited event can work even codecs become suspended later. Tested on whiskylake with hda codecs. Signed-off-by: Rander Wang <[email protected]>

RanderWang · 2019-06-11T08:40:35Z

@plbossart @ranj063 @lyakh @kv2019i

Hi all, I updated my PR based on your comments. Now in hda_codec_jack_check
I refine it to make jack detection fully work. please check the comments.

To test it, PR #1013: Modified stream interrupt handler.
PR: #1003: SOF idle callback and HDA codec ordering (if controller is suspended before codec, WAKEEN feature doesn't work)

and a commit lyakh@b91306f: free interrupts for suspend are needed.

Thank!

plbossart · 2019-06-11T16:25:20Z

@plbossart @ranj063 @lyakh @kv2019i

Hi all, I updated my PR based on your comments. Now in hda_codec_jack_check
I refine it to make jack detection fully work. please check the comments.

To test it, PR #1013: Modified stream interrupt handler.
PR: #1003: SOF idle callback and HDA codec ordering (if controller is suspended before codec, WAKEEN feature doesn't work)

the trend is apparently that PR#1003 has no value

and a commit lyakh@b91306f: free interrupts for suspend are needed.

And this one isn't a PR yet.

What am I supposed to do with this?

ranj063 · 2019-06-11T21:46:24Z

controller

#1003 is not really a requirement for this, is it? I agree that we absolutely need @lyakh 's commit. Can you please add it to this PR?

ranj063 · 2019-06-11T21:53:36Z

@ranj063 I can't get HDaudio to work reliably without this PR. Is there any reason why we should delay the merge further?

@plbossart I'm surprised to hear that. Without PR 1013 or Guennadi's patch merging this will cause too many errors right away after boot.

ranj063 · 2019-06-11T21:53:56Z

But I agree that this PR is complete

lyakh · 2019-06-12T07:21:30Z

sound/soc/sof/intel/hda.h

@@ -392,6 +392,9 @@ struct sof_intel_hda_dev {

 	/* DMIC device */
 	struct platform_device *dmic_dev;
+
+	/* hda codec mask excluding hdmi */
+	u32 hda_codec_mask;


@RanderWang @plbossart @ranj063 Is this really needed? Wouldn't it be enough to check codec->jacktbl.used?

hda_codec_mask is used by enable & disable WAKEEN, please check other change in the PR. codec->jacktbl.used is not available.

lyakh · 2019-06-12T09:02:41Z

sound/soc/sof/intel/hda-codec.c

+	mask = status ? status : hda->hda_codec_mask;
+
+	list_for_each_codec(codec, hbus)
+		if (mask & BIT(codec->core.addr))


@RanderWang @plbossart Sorry, I don't understand this. Now you check jack-status of every codec on every runtime-resume?.. This seems wrong to me. Jack status change should be detected when the change happens, not when the next rtpm-resume is happening. This really doesn't look right to me, please, explain.

please check comments in this function

RanderWang requested review from lgirdwood and plbossart as code owners May 27, 2019 09:56

RanderWang requested review from ranj063 and keyonjie May 27, 2019 09:57

keyonjie previously approved these changes May 27, 2019

View reviewed changes

ranj063 reviewed May 28, 2019

View reviewed changes

RanderWang dismissed keyonjie’s stale review via 202ff5b May 28, 2019 07:21

RanderWang force-pushed the hda_jack branch 2 times, most recently from 202ff5b to 93977fc Compare May 28, 2019 07:33

plbossart requested changes May 28, 2019

View reviewed changes

RanderWang force-pushed the hda_jack branch 2 times, most recently from d862d7b to 07f839b Compare May 29, 2019 08:18

xiulipan mentioned this pull request May 29, 2019

ASoC: SOF: topology: refine multi-core core status management #888

Closed

lyakh suggested changes May 29, 2019

View reviewed changes

RanderWang force-pushed the hda_jack branch from 07f839b to 72a3dee Compare May 29, 2019 10:08

plbossart requested changes May 29, 2019

View reviewed changes

RanderWang force-pushed the hda_jack branch from 72a3dee to 0ed1c3a Compare May 30, 2019 02:54

plbossart requested changes May 30, 2019

View reviewed changes

RanderWang force-pushed the hda_jack branch from afe755d to a57baa0 Compare June 4, 2019 09:23

RanderWang requested a review from juimonen as a code owner June 4, 2019 09:23

RanderWang force-pushed the hda_jack branch 2 times, most recently from f2fd76b to f626004 Compare June 4, 2019 09:32

plbossart reviewed Jun 4, 2019

View reviewed changes

lyakh reviewed Jun 5, 2019

View reviewed changes

This was referenced Jun 6, 2019

Modified stream interrupt handler. Mimics the legacy interrupt handler #1013

Merged

[BUG] No update in sound-setting upon headset hotplug after long time idle #909

Closed

lyakh reviewed Jun 7, 2019

View reviewed changes

ASoC: SOF: Intel: hda: reduce ifdef usage for hda

9ea886f

Move the code for hda to one point Signed-off-by: Rander Wang <[email protected]>

RanderWang force-pushed the hda_jack branch 3 times, most recently from 0d8e96d to d229eae Compare June 11, 2019 08:22

plbossart added the Unclear No agreement on problem statement and resolution label Jun 11, 2019

ranj063 approved these changes Jun 11, 2019

View reviewed changes

plbossart approved these changes Jun 11, 2019

View reviewed changes

plbossart merged commit 7e43c6c into thesofproject:topic/sof-dev Jun 11, 2019

lyakh reviewed Jun 12, 2019

View reviewed changes

Enable WAKEEN feature in sof hda driver #983

Enable WAKEEN feature in sof hda driver #983

Conversation

RanderWang commented May 27, 2019 • edited Loading

keyonjie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang commented May 28, 2019

xiulipan commented May 28, 2019

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart commented May 28, 2019

RanderWang commented May 29, 2019

RanderWang commented May 29, 2019

xiulipan commented May 29, 2019

xiulipan commented May 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiulipan commented May 29, 2019

RanderWang commented May 29, 2019

plbossart commented May 29, 2019

ranj063 commented May 29, 2019

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang commented May 30, 2019

RanderWang commented Jun 4, 2019 • edited Loading

plbossart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang commented Jun 5, 2019

ranj063 commented Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

RanderWang Jun 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang commented Jun 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang commented Jun 11, 2019

RanderWang commented Jun 11, 2019 • edited Loading

plbossart commented Jun 11, 2019

ranj063 commented Jun 11, 2019

ranj063 commented Jun 11, 2019

ranj063 commented Jun 11, 2019

lyakh Jun 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RanderWang commented May 27, 2019 •

edited

Loading

RanderWang commented Jun 4, 2019 •

edited

Loading

ranj063 commented Jun 5, 2019 •

edited

Loading

RanderWang Jun 6, 2019 •

edited

Loading

RanderWang commented Jun 11, 2019 •

edited

Loading

lyakh Jun 12, 2019 •

edited

Loading