ASoC: SOF: Intel: optimise jack detection by lyakh · Pull Request #1031 · thesofproject/linux

lyakh · 2019-06-12T13:26:14Z

No need to schedule jack-checking work upon every runtime PM
resume, it should only be checked when the status indicates a
change.
The additional HDA codec mask isn't needed, just check the codec
table.
Use u32 for the type of the "status" bitfield.

plbossart

LGTM but there is one condition that can be clarified for the rest of us.

plbossart · 2019-06-12T14:53:12Z

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


 	list_for_each_codec(codec, hbus)
-		if (mask & BIT(codec->core.addr))
+		if (status & BIT(codec->core.addr) && codec->jacktbl.used) {


this is a bit confusing to have both types of AND in the same test, I don't even know what precedence rules apply?

RanderWang

I don't agree this change. Jack detection doesn't work when checking sound setting.
@ClarexZhou can you test this patch to check sound setting case.

RanderWang · 2019-06-13T03:16:08Z

As discussed in my patch for jack detection, 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. At this time if a headphone or headset is plugged in, WAKEEN feature will not trigger interrupt to resume sof (it is already resumed).

How to do ? I checked the legacy hda (please check the WSR sent by me) and found one way to fix it. Just check the jack status of non-hdmi codec, which is activated by this poll operation, when controller is resuming, and then unsolicited event can be sent to controller to do jack detection. Even then non-hdmi codec goes to sleep, the unsolicited event still works. This is what I learned from legacy hda driver.

This costs little time . We already reload FW and power up the dsp.

More info: for sound setting case, pulse audio will setup a capture stream with DMIC. At this time, hda codec is still suspended and the status of WAKEEN interrupt will be zero because the resume is triggered by capture not by WAKEEN interrupt.

ClarexZhou · 2019-06-13T03:32:14Z

Check jack detection function with https://github.com/lyakh/linux/commits/ipc-no_disable, which include #1031. Jack detection still not work in below 2 scenarios:
Scenario 1: Plug out headset -> do system reboot->wait several seconds->open sound setting->plug in headset. ->Error reproduced. Jack detection not work.
Scenario 2: Plug out headset->play music->wait several seconds till runtime PM status suspended.->open sound setting->plug in headset->Error reproduced. Jack detection not work.

lyakh · 2019-06-13T07:50:13Z

@RanderWang thanks for reviewing. I have been able to reproduce the problem by waiting for runtime-suspend and then starting HDMI-audio playback. Which HDMI-playback was active no jack detection was possible. Is this the problem, that you're describing? Same with DMIC or, I guess, any other connection, not using the respective codec. As far as I understand your patch, you force wake up to the HDA codec by scheduling the jack-detection work, is my understanding correct? That means, that one of the ways to fix this problem would be to always force any HDA codecs with jacks to wake up when the controller wakes up. Would the other option work too - if we only enable WAKEEN when the codec runtime-suspends, and disable WAKEEN when the codec wakes up? I.e. would WAKEEN work when the controller is active but the codec is suspended?

lyakh · 2019-06-13T08:45:24Z

Interesting, just keeping WAKEEN always set doesn't seem to help.

lyakh · 2019-06-13T09:36:02Z

@RanderWang ok, I updated the patch. I kept your approach to schedule a jack check for all jack-capable codecs, although I don't quite understand why it works. In principle what we want is when the controller wakes up to also wake up those codecs, right? But I briefly checked the jack-detection path and I didn't find anything like that there. I didn't check codec methods though. But I also don't understand how the codec then stays awake for the whole duration of the controller being awake and then suspends again... Anyway, I think this now should be quite close to your version, just with less added code.

RanderWang · 2019-06-13T09:56:11Z

@RanderWang. But I also don't understand how the codec then stays awake for the whole duration of the controller being awake and then suspends again

Actually , codec will suspends if it is not used, not staying awake. But with these operations, codec could send unsolicited event to controller, even codec is suspended. I checked legacy hda driver and did a few experiments, then found this behavior.

RanderWang · 2019-06-13T10:05:58Z

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

 		snd_hdac_chip_writew(bus, WAKEEN,
-				     snd_hdac_chip_readw(bus, WAKEEN) |
-				     hda->hda_codec_mask);
+				     snd_hdac_chip_readw(bus, WAKEEN) | mask);


you changed hda->hda_codec_mask to ~STATESTS_INT_MASK in hda_codec_jack_check, why not here ?

we only enable the bits, that we need, but it doesn't hurt disabling all possible bits.

can you implement a function in hda-codec.c to do this ? Just like hda_codec_jack_check. Here only
if (runtime)
xxx_xxxx_xxx
also move if (IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC) && runtime_suspend) to the function to make codec beautiful.

RanderWang · 2019-06-13T10:08:49Z

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

-	 * even codecs become suspended later.
-	 */
-	mask = status ? status : hda->hda_codec_mask;
-


don't check status ? If there is one codec, there is no different for this change. But if there are two codecs, the checking will save one jack polling operation.

but do we want that? I thought that actually would be bad? If there are 2 codecs and one of them has a bit set in the status, we will only check it, so the other codec will stay suspended? And if headphones are plugged into it, they won't be detected?

Good question: one popular design: two codecs, one for headphone, anther one with amplifier for speaker. Some vendors may add a special codec for DMIC.

I don't insist on checking status. It is a just optional optimization.

On my PC I have 8 3.5 mm jack ports - 2 on the front panel and 6 on the back. They aren't all handled by the same codec, are they? How common are designs with multiple codecs providing multiple jack connectors?

for HDaudio designs, it's typically to have an all-in-one integrated codec which can deal with amplification and jack detection. I don't think we have a need to handle jack detection across multiple HDaudio codecs, it's difficult enough for a single codec to get things right.

yes, but there are still two codecs cases. I also talked to @bardliao and conformed this case. Takashi also had a UCM patch for two codecs. But I think this case it is not a common case and we haven't test SOF on two codecs device.

yes, but there are still two codecs cases. I also talked to @bardliao and conformed this case.

@RanderWang Sorry I thought we were talking about I2S codec. I know that there are some designs with more than one codec. E.g. one for headset and the other for speaker.

we haven't test SOF on two codecs device.

Yes, we have.

lyakh · 2019-06-13T10:25:39Z

@RanderWang. But I also don't understand how the codec then stays awake for the whole duration of the controller being awake and then suspends again

Actually , codec will suspends if it is not used, not staying awake. But with these operations, codec could send unsolicited event to controller, even codec is suspended. I checked legacy hda driver and did a few experiments, then found this behavior. But controller and codec in legacy hda driver are always active in default.

Interesting, do we know what exactly changes the codec "mode" from "completely suspended" to "suspended but sending UNSOLs?"

ranj063 · 2019-06-13T22:06:12Z

@lyakh the previous version was a bit more intuitive ie check the status for which codec generated the unsol event and check the jack status for that one. But with this version, you wake up all codecs that are jack capable. Why is this better than that?

lyakh · 2019-06-14T06:21:29Z

@lyakh the previous version was a bit more intuitive ie check the status for which codec generated the unsol event and check the jack status for that one. But with this version, you wake up all codecs that are jack capable. Why is this better than that?

@ranj063 you can see an explanation in my discussion with Rander above. If you look at the present code that's also what it does - almost. But IMHO in a less obvious way. It checks if the status != 0, then it uses it. If status == 0, then it checks all the codecs... So if the system has more than one codec with jack connectors, with the current code it seems like some of those connectors can stay suspended and will miss any plugin events.

ranj063 · 2019-06-14T16:34:26Z

@ranj063 you can see an explanation in my discussion with Rander above. If you look at the present code that's also what it does - almost. But IMHO in a less obvious way. It checks if the status != 0, then it uses it. If status == 0, then it checks all the codecs... So if the system has more than one codec with jack connectors, with the current code it seems like some of those connectors can stay suspended and will miss any plugin events.

@lyakh hmm yes I missed that part, thanks. I think it makes sense now.

ranj063 · 2019-06-14T16:36:08Z

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

+
+		list_for_each_codec(codec, hbus)
+			if (codec->jacktbl.used)
+				mask |= BIT(codec->core.addr);


@lyakh can you please add a comment here to explain that we want to wake-up all jack capable codecs irrespective of status

RanderWang · 2019-06-17T00:46:33Z

Interesting, do we know what exactly changes the codec "mode" from "completely suspended" to "suspended but sending UNSOLs?"

In my opinion, after codec resumes, controller setup a link for this codec. And this link is still available even codec sleeps. If the controller is suspended, the link is invalid.

RanderWang

Minor fix needed but this looks fine in general. Thanks

RanderWang · 2019-06-19T07:45:12Z

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

@@ -297,10 +297,7 @@ static int hda_suspend(struct snd_sof_dev *sdev, int state,

 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
 	if (IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC) && runtime_suspend)


just remove IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)

why? jack detection doesn't really make sense for HDMI, does it?

please check hda_codec_jack_wake_enable, the CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC is there.

#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC)
hda_codec_jack_wake_enable(){
....
}
#else
hda_codec_jack_wake_enable() {}
#endif

?? what are you trying to say?

@plbossart: @RanderWang is saying, that you don't need to check for CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC before calling hda_codec_jack_wake_enable() because if the CONFIG_... isn't selected, the function is defined as a dummy, so, it's still safe to call it. @RanderWang you're right, I'll update that, thanks

thanks for the translation. I'd like this PR to be completed soonish so that we can move on.

thanks @lyakh for your explanation. Actually, this idea was suggested by @plbossart when I implemented hda_codec_jack_check.

RanderWang

LGTM

plbossart

nit-pick below, but this looks quite good.

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

plbossart

LGTM, need a 2nd reviewer. @RanderWang @ranj063 please chime in

lyakh · 2019-07-02T12:09:44Z

As for the failing CI, this patch only touches HDA so I cannot see how it can break aplay on BYT. Interestingly, just because #1020 has also been updated yesterday, the same tests are failing there. So I'm pretty certain it's a regression elsewhere.

xiulipan · 2019-07-02T15:04:34Z

@lyakh There is a regression #1599 and already fixed. I will re-trigger this PR for test.

lyakh · 2019-07-02T15:53:42Z

@ranj063 @plbossart please, have a look

ranj063 · 2019-07-02T17:42:55Z

@lyakh can you please resolve the conflicts?

ranj063 · 2019-07-03T14:28:59Z

Need another approval. @RanderWang @kv2019i ?

kv2019i

Almost good, @lyakh please check my one comment.

kv2019i · 2019-07-03T15:15:57Z

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

-		snd_hdac_chip_writew(bus, WAKEEN,
-				     snd_hdac_chip_readw(bus, WAKEEN) |
-				     hda->hda_codec_mask);
+		snd_hdac_chip_updatew(bus, WAKEEN, 0, hda->hda_codec_mask);


Hmm, the macros in hdaudio.h are a bit odd in that the mask is not applied to the value, so this still works, but it looks odd. Can you fix this to

snd_hdac_chip_updatew(bus, WAKEEN, hda->hda_codec_mask, hda->hda_codec_mask);

... just in case someone refactors the hdaudio.h macros (to behave like e.g. snd_sof_dsp_update_bits() family works).

@kv2019i I'm not sure snd_sof_dsp_update_bits() is a common way to handle bitmasks, cf. set_mask_bits()

@lyakh True, but in ALSA we seem to be using the sof_dsp_update_bits style (see e.g. use of snd_hdac_chip_updateb in hda_controller.c). Also regmap_update_bits applies the mask to value.

RanderWang

LGTM

Use snd_hdac_chip_updatew() to update WAKEEN instead of open-coding. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

1. The additional HDA codec mask isn't needed, just check the codec jack table. 2. Remove the "status" parameter. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

ranj063 · 2019-07-08T16:10:40Z

@kv2019i @juimonen could either of your approve this PR again? Thanks!

kv2019i · 2019-07-08T17:20:10Z

@kv2019i @juimonen could either of your approve this PR again? Thanks!

@ranj063 done

lyakh requested review from RanderWang and plbossart June 12, 2019 13:26

lyakh requested a review from lgirdwood as a code owner June 12, 2019 13:26

plbossart requested changes Jun 12, 2019

View reviewed changes

RanderWang suggested changes Jun 13, 2019

View reviewed changes

lyakh force-pushed the wakeen branch from c1c1e6c to 44ce600 Compare June 13, 2019 09:31

RanderWang reviewed Jun 13, 2019

View reviewed changes

ranj063 reviewed Jun 14, 2019

View reviewed changes

lyakh force-pushed the wakeen branch from 44ce600 to cf9706b Compare June 17, 2019 14:22

ranj063 previously approved these changes Jun 17, 2019

View reviewed changes

RanderWang suggested changes Jun 19, 2019

View reviewed changes

lyakh dismissed ranj063’s stale review via ee5039a June 20, 2019 07:01

lyakh force-pushed the wakeen branch from cf9706b to ee5039a Compare June 20, 2019 07:01

RanderWang previously approved these changes Jun 20, 2019

View reviewed changes

plbossart requested changes Jun 20, 2019

View reviewed changes

sound/soc/sof/intel/hda-dsp.c Show resolved Hide resolved

lyakh dismissed RanderWang’s stale review via 40e6653 June 20, 2019 15:23

lyakh force-pushed the wakeen branch from ee5039a to 40e6653 Compare June 20, 2019 15:23

plbossart previously approved these changes Jun 21, 2019

View reviewed changes

ranj063 previously approved these changes Jul 2, 2019

View reviewed changes

lyakh dismissed ranj063’s stale review via 79e019d July 3, 2019 06:41

lyakh force-pushed the wakeen branch from edd78a1 to 79e019d Compare July 3, 2019 06:41

ranj063 previously approved these changes Jul 3, 2019

View reviewed changes

ranj063 requested review from RanderWang, bardliao and kv2019i July 3, 2019 15:08

kv2019i requested changes Jul 3, 2019

View reviewed changes

lyakh dismissed ranj063’s stale review via df31ff0 July 5, 2019 11:33

lyakh force-pushed the wakeen branch from 79e019d to df31ff0 Compare July 5, 2019 11:33

ranj063 previously approved these changes Jul 5, 2019

View reviewed changes

RanderWang previously approved these changes Jul 8, 2019

View reviewed changes

kv2019i previously approved these changes Jul 8, 2019

View reviewed changes

lyakh added 2 commits July 8, 2019 17:59

ASoC: SOF: Intel: use snd_hdac_chip_updatew() for WAKEEN

d360ab8

Use snd_hdac_chip_updatew() to update WAKEEN instead of open-coding. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

ASoC: SOF: Intel: optimise jack detection

863fff8

1. The additional HDA codec mask isn't needed, just check the codec jack table. 2. Remove the "status" parameter. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh dismissed stale reviews from kv2019i, RanderWang, and ranj063 via 863fff8 July 8, 2019 16:06

lyakh force-pushed the wakeen branch from df31ff0 to 863fff8 Compare July 8, 2019 16:06

ranj063 approved these changes Jul 8, 2019

View reviewed changes

kv2019i approved these changes Jul 8, 2019

View reviewed changes

ranj063 merged commit 1da8b5f into thesofproject:topic/sof-dev Jul 8, 2019

lyakh deleted the wakeen branch July 8, 2019 19:57

		@@ -297,10 +297,7 @@ static int hda_suspend(struct snd_sof_dev *sdev, int state,

		#if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA)
		if (IS_ENABLED(CONFIG_SND_SOC_SOF_HDA_AUDIO_CODEC) && runtime_suspend)

Conversation

lyakh commented Jun 12, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ClarexZhou commented Jun 13, 2019

Uh oh!

lyakh commented Jun 13, 2019

Uh oh!

lyakh commented Jun 13, 2019

Uh oh!

lyakh commented Jun 13, 2019

Uh oh!

RanderWang commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh commented Jun 13, 2019

Uh oh!

ranj063 commented Jun 13, 2019

Uh oh!

lyakh commented Jun 14, 2019

Uh oh!

ranj063 commented Jun 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Jun 17, 2019

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

RanderWang commented Jun 13, 2019 •

edited

Loading

RanderWang commented Jun 13, 2019 •

edited

Loading