Enable WAKEEN feature in sof hda driver #983
Conversation
sound/soc/sof/intel/hda-dsp.c
Outdated
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
yes, I will refine it. Thanks!
RanderWang
force-pushed
the
hda_jack
branch
2 times, most recently
from
202ff5b to
93977fc
Compare
|
Update PR, check runtime suspend |
|
@RanderWang |
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
Minor fixes needed (also fix typos in commit messages) but this looks fine in general. Thanks @RanderWang !
sound/soc/sof/intel/hda-dsp.c
Outdated
There was a problem hiding this comment.
bool suspend_runtime for consistency with sound/soc/sof/pm.c
sound/soc/sof/intel/hda-dsp.c
Outdated
There was a problem hiding this comment.
bool resume_runtime for consistency with sound/soc/sof/pm.c
sound/soc/sof/intel/hda-dsp.c
Outdated
sound/soc/sof/intel/hda-dsp.c
Outdated
sound/soc/sof/intel/hda-dsp.c
Outdated
sound/soc/sof/intel/hda-dsp.c
Outdated
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 |
so now CI works ? |
RanderWang
force-pushed
the
hda_jack
branch
2 times, most recently
from
d862d7b to
07f839b
Compare
|
@RanderWang |
|
@RanderWang It seems we have some code base regression or your patch have some issue. |
sound/soc/sof/intel/hda-codec.c
Outdated
There was a problem hiding this comment.
I'd make this
if (!status)
return;
sound/soc/sof/intel/hda-codec.c
Outdated
sound/soc/sof/intel/hda-dsp.c
Outdated
There was a problem hiding this comment.
I assume reading STATESTS here wouldn't work? It has to be done earlier?
There was a problem hiding this comment.
STATESTS would be cleared if chip is initialized, so read if before chip initialization.
|
@RanderWang @ranj063 |
|
update my PR according to advise from lyakh |
|
@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
left a comment
plbossart
left a comment
There was a problem hiding this comment.
one minor confusion in variable naming (likely copy/paste) but looks good otherwise.
Thanks @RanderWang
sound/soc/sof/intel/hda-dsp.c
Outdated
There was a problem hiding this comment.
very sorry, Thanks Pierre. I will correct it.
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
@RanderWang the code is fine but we can remove a few ifdefs and make the code more readable. see comments below.
sound/soc/sof/intel/hda-dsp.c
Outdated
There was a problem hiding this comment.
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
Outdated
There was a problem hiding this comment.
you can use a helper that does nothing when HDA_AUDIO_CODEC is not defined. This would make the code more readable
sound/soc/sof/intel/hda.h
Outdated
There was a problem hiding this comment.
no need for conditional definition of prototype
|
@plbossart thanks for your comments. This PR worked on my RVP but on production devices I found one issue: I will check it and update my PR later. Thanks! |
RanderWang
force-pushed
the
hda_jack
branch
2 times, most recently
from
f2fd76b to
f626004
Compare
|
@plbossart @ranj063 @keyonjie @kv2019i I and QA did stress test on a few whiskylake devices. I also validated it on apollolake up2 |
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
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.
sound/soc/sof/intel/hda-dsp.c
Outdated
There was a problem hiding this comment.
does this belong in the main sdev structure or in the hda-specific ones?
sound/soc/sof/intel/hda-dsp.c
Outdated
There was a problem hiding this comment.
should this be protected with the ipc_spinlock?
sound/soc/sof/intel/hda-ipc.c
Outdated
There was a problem hiding this comment.
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.
|
@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. |
|
@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. |
sound/soc/sof/intel/hda-codec.c
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
@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!
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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));
There was a problem hiding this comment.
@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);
There was a problem hiding this comment.
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!
sound/soc/sof/intel/hda-codec.c
Outdated
sound/soc/sof/intel/hda-dsp.c
Outdated
There was a problem hiding this comment.
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.
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. |
sound/soc/sof/intel/hda-codec.c
Outdated
There was a problem hiding this comment.
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
ifand run thelist_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.
There was a problem hiding this comment.
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 <rander.wang@linux.intel.com>
@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. |
RanderWang
force-pushed
the
hda_jack
branch
3 times, most recently
from
0d8e96d to
d229eae
Compare
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 <rander.wang@linux.intel.com>
|
@plbossart @ranj063 @lyakh @kv2019i Hi all, I updated my PR based on your comments. Now in hda_codec_jack_check To test it, PR #1013: Modified stream interrupt handler. and a commit lyakh@b91306f: free interrupts for suspend are needed. Thank! |
the trend is apparently that PR#1003 has no value
And this one isn't a PR yet. What am I supposed to do with this? |
@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. |
|
But I agree that this PR is complete |
| struct platform_device *dmic_dev; | ||
|
|
||
| /* hda codec mask excluding hdmi */ | ||
| u32 hda_codec_mask; |
There was a problem hiding this comment.
@RanderWang @plbossart @ranj063 Is this really needed? Wouldn't it be enough to check codec->jacktbl.used?
There was a problem hiding this comment.
hda_codec_mask is used by enable & disable WAKEEN, please check other change in the PR. codec->jacktbl.used is not available.
| mask = status ? status : hda->hda_codec_mask; | ||
|
|
||
| list_for_each_codec(codec, hbus) | ||
| if (mask & BIT(codec->core.addr)) |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
please check comments in this function
7 participants
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