ASoC: hdac_hda: let the hda codec driver autosuspend

[jason77-wang]

On some Lenovo and HP laptops, if both codec driver and sof driver
are in runtime suspend mode, we plug a headset to the audio jack,
the headphone could be detected but Mic couldn't.

That is because when plugging, the headphone triggers a unsol event
first, and about 0.7s later (on the Lenovo X1 Carbon 7th), the Mic
triggers a unsol event. But if the codec driver enters runtime suspend
within 0.7s, the Mic can't trigger the unsol event.

If we don't set autosuspend_delay and enable autosuspend for hda codec
driver, it will enter runtime suspend immediately after the headphone
triggers the unsol event.

Refer to the legacy hda driver (pci/hda/hda_codec.c), we set the delay
and enable the autosuspend.

Cc: stable@vger.kernel.org
Signed-off-by: Hui Wang hui.wang@canonical.com

[kv2019i]

@jason77-wang Thanks for the patch! We have had a similar patch in review for a long time in #1689 . The consensus is that this change needs to go in, but we have a regression on some devices, which has delayed the merge. @plbossart would like to fix the regression at the same time.

[kv2019i]

@juimonen @ClarexZhou Can you test whether this version triggers the regression on Mantis?

[jason77-wang]

Good to know that you have noticed this issue. thx.

[Liviali155]

Test on Mantis with sof-dev+PR1837 & master,side effect is below

1. Connect Headset

2. "sudo reboot" to reboot system

3. Open sound setting ASAP, just after boot up. Paplay to check-> sound is very small 
   Recover method: Close sound setting and stop paplay, wait till runtime PM status to suspend, then paplay again, audio can output normally

In step3, if open paplay ASAP instead of open sound setting, error also can be reproduced.

This issue not exist without this PR

Error only occurred if open sound setting as soon as display is on. Error not seen if do this several seconds later after boot up.

[ClarexZhou]

Jack detection works OK on Lenovo X1 Yoga with this PR.

[juimonen]

so I guess the test results tell that this pr has the same side effect than #1689.

So the mantis side effect is still not root caused, I can try to look into that.

So is this better "place" to fix this issue than #1689? We can go either way...

[jason77-wang]

Just notice it will introduce a side effect on headphone output voluem. I will try to reproduce it and investigate it.

thx.

[jason77-wang]

About the side effect, I found the hda analogue codec needs to enter runtime_suspend at least once before it could output the sound normally. If we set autosuspend_delay too early, the codec will not enter runtime_suspend immediately when runtime pm is enabled in the snd_hda_codec_register(), that will introduce the side effect.

If we put the "enabling autosuspend" after the snd_card is registered, there will be no that side effect.

[juimonen]

@jason77-wang ok, I have the setup now for both dell and x1, which I can test. I'm still trying to find the best place in sof to do this, but if you find a good "spot" elsewhere just update this PR.

[RanderWang]

@jason77-wang , This delay depends on platform, you could check https://github.com/thesofproject/linux/blob/topic/sof-dev/sound/soc/intel/boards/sdw_rt711_rt1308_rt715.c#L227 to apply it only to X1 or HP

[jason77-wang]

@RanderWang,
Thanks for your suggestion, I guess the root cause for this problem is there is a combo audio jack (headphone + mic) on the machine and the hardware design doesn't use MIC_DET for mic detecting, that makes the mic can't trigger the unsol event during runtime suspend.

And this hw design is very common on Lenovo and HP machines, for LENOVO machines, We already have 8 models to support dmic and all of them have this issues. I guess it is similar for HP machines. And this delay will not add much power consumption on the Dell machines, could we apply the delay to all machines instead of introducing a quirk table?

[RanderWang]

@jason77-wang It depends on @ClarexZhou :). And DELL adopt software solution for Jack, but HP & Lenovo use hardware detection, so this issue is not detected on DELL devices. The last test show regression issue with this PR. Hi @ClarexZhou how about mantis ?

[ClarexZhou]

@Liviali155 has tested on Mantis, all work ok. No side effect on that.

[kv2019i]

@plbossart, here's actually more likely explanation why the delay is needed:

@jason77-wang wrote

Thanks for your suggestion, I guess the root cause for this problem is there is a combo audio jack (headphone + mic) on the machine and the hardware design doesn't use MIC_DET for mic detecting, that makes the mic can't trigger the unsol event during runtime suspend.

[Liviali155]

Test on Mantis with sof-dev+PR1837 & master,side effect is below

1. Connect Headset

2. "sudo reboot" to reboot system

3. Open sound setting ASAP, just after boot up. Paplay to check-> sound is very small 
   Recover method: Close sound setting and stop paplay, wait till runtime PM status to suspend, then paplay again, audio can output normally

In step3, if open paplay ASAP instead of open sound setting, error also can be reproduced.
This issue not exist without this PR

Error only occurred if open sound setting as soon as display is on. Error not seen if do this several seconds later after boot up.

Retested it on matains sof-dev+PR1837(PR1837 has been updated),the side effect can not be reproduced,jack dtection also works well

[ClarexZhou]

Test on lenovo x1, jack detection works OK. But both headphone and MIC status will not update via below scenarios.

1. sudo rtcwake -m mem -s 20
2. Plugin headset during system suspend
3. Wait till system wake up. wait till runtime PM status suspend (dmesg not update anymore), check "amixer contents|less", jack detection is on
4. Unplug headset, check "amixer contents | less" ->Dmesg not update, jack detection is still on.

Jack detection back to work if aplay or open sound setting. So this issue will not be observed by end user.

[jason77-wang]

Test on lenovo x1, jack detection works OK. But both headphone and MIC status will not update via below scenarios.

1. sudo rtcwake -m mem -s 20

2. Plugin headset during system suspend

3. Wait till system wake up. wait till runtime PM status suspend (dmesg not update anymore), check "amixer contents|less", jack detection is on

4. Unplug headset, check "amixer contents | less"  ->Dmesg not update, jack detection is still on.

Reproduced this new issue, will continue debugging.

thx.

[plbossart]

Either the commit message needs to be refined or there's a possible race condition.

[plbossart]

@wenqingfu same issue with check-sof-logger in CI...

[kv2019i]

@juimonen wrote:

So is this better "place" to fix this issue than #1689? We can go either way...

This patch is more focused as the non-immediate autosuspend value is only set for HDA codecs that have an analog DAI. #1726 is more general and sets the autosuspend to all (this is what the non-DSP snd-hda-intel driver does).

In bigger picture, both seem to be fine. If we anyways end up doing machine specific quirks, then setting the value in machine driver (like done in this PR), might be better as we can build on this. This will not be good, if we later figure out we need to set the autosuspend delay for HDMI codec as well, in which case we would need to patch many machine drivers separately. But based on current data we have, this is not the case, so the generic hda driver is an ok place for this.

[jason77-wang]

It is fine to drop this Pullrequest and keep the #1726

Today I did some debug on the LENOVO machines, I found those new LENOVO machines are cml platforms (8086:02c8), my old LENOVO machines are cnl/whl machines (8086:9dc8).
If I apply the patch (setting autosuspend_delay) to branch of sof-dev, only cnl/whl machines could detect the external mic, all cml machines can't.

If I apply the patch to ubuntu-5.0.0-oem (based on sof-5.0) kernel, both cnl/whl and cml machines could detect the external mic.

So It looks like the sof runtime_pm driver introduced sth which can't work on cml well after sof-5.0.

BTW, all LENOVO machines (cml and cnl/whl) work well with legacy hda driver, mic could be detected.

[jason77-wang]

Through bisecting, I found if I revert this commit, all lenovo machines (cml and cnl/whl) work well with the runtime_autosuspend patch. If I don't revert this commit, only cnl/whl Lenovo machines work with the runtime_autosuspend patch. (work well means the external mic could be detected, but those side effects still exist).

37a0794

ASoC: SOF: Intel: hda-loader: clear the IPC ack bit after FW_PURGE done

Set DONE bit after the FW_PURGE IPC is polled successfully, to clear the
interrupt and avoid the arrival of the confusing unexpected ipc.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

[ranj063]

Through bisecting, I found if I revert this commit, all lenovo machines (cml and cnl/whl) work well with the runtime_autosuspend patch. If I don't revert this commit, only cnl/whl Lenovo machines work with the runtime_autosuspend patch. (work well means the external mic could be detected, but those side effects still exist).

37a0794

ASoC: SOF: Intel: hda-loader: clear the IPC ack bit after FW_PURGE done

Set DONE bit after the FW_PURGE IPC is polled successfully, to clear the
interrupt and avoid the arrival of the confusing unexpected ipc.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

@keyonjie FYI. What connection do you think clearing the DONE bit has on runtime PM?

[keyonjie]

@ranj063 sorry I am not up to date about how exactly the runtime PM works between SOF driver and codec driver now. What I can imagine is that if removing my commit 37a0794, there could be one more "unexpected ipc" reply IPC going into our IPC interrupt handler, maybe this encounter there help to hide the runtime PM issue coincidentally?

[keyonjie]

hI @jason77-wang @ClarexZhou do we have github issue to track this, as @kv2019i mentioned, looks the connections among controller/codec runtime-PM status, codec jack detection, and my commit here are complicated, we need to understand how the issue happen.

[kv2019i]

@plbossart @ranj063 We have now evidence that if we follow same programming flow as legacy driver, we do not have the side-effect on WHL Mantis (#1837 (comment)). I'd propose to merge this patch as it fixes many known problems and aligning our programming flow to legacy DSP driver is a sensible rationale. Figuring out the other problem on CML (#1938) is separate.

Ok?

[plbossart]

@plbossart @ranj063 We have now evidence that if we follow same programming flow as legacy driver, we do not have the side-effect on WHL Mantis (#1837 (comment)). I'd propose to merge this patch as it fixes many known problems and aligning our programming flow to legacy DSP driver is a sensible rationale. Figuring out the other problem on CML (#1938) is separate.

Ok?

Sounds like a plan @kv2019i , but I didn't understand if you are asking me to approve the patches in this PR as is, or if this is a direction for the next step?

[juimonen]

@plbossart we can't currently pin point the exact reason why my PR has issues in Mantis and this PR doesn't. We have identified that this PR sets the suspend delay in later phase and is thus closer how the legacy works.

So yes we are asking you to accept this PR :)

Reasons:

1. We are not setting the codec suspend delay at all currently (legacy is doing that and we need it for mic detection)
2. This PR doesn't have issues in Mantis as delay is set in later phase (root cause still unclear)
3. third issue found by @jason77-wang in new lenovo laptops is unrelated and fixed/handled upstream by @jason77-wang

[plbossart]

@juimonen I could accept the first patch which sets the autosuspend delay.
I cannot accept the second revert patch which deals with the SOF IPC, that's seems like an unrelated problem, or a much larger one

[juimonen]

@plbossart, oh it is still here....
@jason77-wang can you remove the revert commit? As I understood it was just disabling PM altogether and should not be needed...

[kv2019i]

@juimonen wrote:

@plbossart, oh it is still here....
@jason77-wang can you remove the revert commit? As I understood it was just disabling PM altogether and should not be needed...

Ack, thanks guys. And yeah @plbossart this was just a direction check. I listed in #1837 (comment) changes I'd like to see, 1) commit message and 2) dropping the extra patch (there's a separate bug for this).

@jason77-wang Are you ok to update the PR? Let us know if you are busy with something else, we can also handle as a new PR.

[jason77-wang]

OK, I will update the PR later today. thx.

[kv2019i]

Thanks @jason77-wang ! This starts to look good. I have one comment on the commit summary. Others, I re-requested reviews, please check. This is now intended for merging.

[kv2019i]

@jason77-wang For the commit message. The driver is already using autosuspend, so the summary is not really accurate. How about:

ASoC: intel/skl/hda - set autosuspend timeout for hda analog codecs

[plbossart]

the commit message is also a bit unclear. Assume that all devices are already suspended, and the jack is plugged. something causes a wake-up to handle the first unsolicited answer for headphone. If the devices go back to suspend, what causes the second unsolicited answer for mic to be missed?
In other words, why can we handle the first case and not the second? It's not the suspended status, something is missing in the explanation.

[jason77-wang]

@jason77-wang For the commit message. The driver is already using autosuspend, so the summary is not really accurate. How about:

ASoC: intel/skl/hda - set autosuspend timeout for hda analog codecs

OK, will change it.

[kv2019i]

@plbossart This was the recommendation from Realtek. Jack detection causes the wake-up from D3, but the detection routine takes time to complete and sent out all the unsolicited response, if driver puts codec immediately back to sleep, it won't wake up again and we lose some responses -- i.e. this is a feature of some codecs. I think the commit msg accurately describes what is seen.

[jason77-wang]

the commit message is also a bit unclear. Assume that all devices are already suspended, and the jack is plugged. something causes a wake-up to handle the first unsolicited answer for headphone. If the devices go back to suspend, what causes the second unsolicited answer for mic to be missed?
In other words, why can we handle the first case and not the second? It's not the suspended status, something is missing in the explanation.

That is sth I don't know how to explain, I did a test before, I did not enable the unsol event on headphone pin, only enable the unsol event on the mic pin:
With the legacy hda driver, the mic can generate the unsol event when plugging/unplugging.
With the sof driver, the mic can't generate the unsol event when plugging/unplugging.
It looks like the mic need to depend on the headphone with the sof driver.

[plbossart]

the commit message is also a bit unclear. Assume that all devices are already suspended, and the jack is plugged. something causes a wake-up to handle the first unsolicited answer for headphone. If the devices go back to suspend, what causes the second unsolicited answer for mic to be missed?
In other words, why can we handle the first case and not the second? It's not the suspended status, something is missing in the explanation.

[lyakh]

nothing major from me, but others requested changes, so you'll probably be submitting updates anyway.

[lyakh]

how certain are we that the name will never change? If we have to rely on it maybe better replace it with a macro?

[jason77-wang]

OK, will add a macro.
thx.

[kv2019i]

@lyakh very certain :) this is defined in hdac_hda.c -- but it's true if this would ever change, it could silently break jack detection in this machine driver. So maybe a more future-proof solution is to apply the delay just to all codecs. This is what the legacy driver does as well. How about it @jason77-wang ?

[jason77-wang]

@kv2019i it is not easy to set for all codecs, I listed all codec-dais, they are:
intel-hdmi-hifi1
intel-hdmi-hifi2
intel-hdmi-hifi3
Analog Codec DAI
Digital Codec DAI
dmic-hifi
dmic-hifi
snd-soc-dummy-dai
We only need to set the delay for intel-hdmi-hifi1 and Analog Codec DAI, if skip other codec-dais, we also need to compare with name string.

So I add two macros which will be used by hdac_hda.c and skl_hda_dsp_generic.c, please take a look at my new patch.

[ClarexZhou]

Tested this 1 commit PR on lenovo. Mic jack detection works. Still has issue "Trigger again when runtime PM enter suspend" and it's caused by one upstream commit. See #1837 (comment). On sof-dev branch, revert that commit directly failed.

@Liviali155 also verified on Mantis. All works ok. Also no side effect.

[kv2019i]

Thanks @jason77-wang . This is starting to look good. I'm still looking at whether we could just call snd_hda_set_power_save() from here (it's exported from hda_codec.c) -- as we set the autosuspend delay to all codecs anyway, we could might as well call that. But haven't yet figured out how to best do this.

[jason77-wang]

Thanks @jason77-wang . This is starting to look good. I'm still looking at whether we could just call snd_hda_set_power_save() from here (it's exported from hda_codec.c) -- as we set the autosuspend delay to all codecs anyway, we could might as well call that. But haven't yet figured out how to best do this.

OK, thanks. If could call that function, it will be better.

[kv2019i]

I uploaded an alternative version as #1983 , please take a look.

[jason77-wang]

We have a new pr to replace this one, now close this pr. The new pr is #1983