ASoC: SOF: Intel: hda: call snd_hda_set_power_save from SOF code #1689
Conversation
|
I'm not sure if this is the correct fix, but this is making the mic detect to work for X1 with ALC285... |
juimonen
force-pushed
the
hda_power_save
branch
from
0dd7f76 to
177bf44
Compare
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
I think this is the correct change. The legacy driver sets the power save value for codecs as well, and before your patch, SOF is not setting it. So I think this is correct.
Only change I'd propose is to change the wording a bit. snd_hda_set_power_save() sets the value for all HDA codecs, so better to rephrase the commit to and talk about "HDA codecs" rather than "HDA parts".
juimonen
force-pushed
the
hda_power_save
branch
from
177bf44 to
e763a97
Compare
|
@kv2019i ok thanks. I updated the commit message, but yeah, I'm still struggling if the description and terms are correct... |
|
@juimonen wrote:
I'd change this first sentence still: |
|
@plbossart @ranj063 so what is the way to fix this and where? I was asking from chrome team folks how this can work with legacy driver if the CONFIG_SND_HDA_POWER_SAVE_DEFAULT is 0. Shouldn't the legacy also loose the mic events also and suspend immediately? So should we set CONFIG_SND_HDA_POWER_SAVE_DEFAULT to 2s (same as sof) in all our HDA configs and then let the quirks disable the power save if needed? |
@juimonen this question has been answered in my PR. Basically is power_save_delay is 0 but auto_runtime_pm for the codec is set, the power save delay is bumped up to 3s. So, the question is where is the auto_runtime_pm set? |
|
@ranj063 I tried to read through the comments in your PR and didn't find any clear answer to my question... so auto_runtime is set in hda_intel.c in setup_vga_switcheroo_runtime_pm and I think this is actually called by display driver. So how would this be better than setting the hda delay in config to 2s? |
@juimonen both my PR and yours set the delay unconditionally but I suppose this is only needed for the ALC269. Can we maybe make it specific to it? |
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
This can do with a comment explaining why this is being manually done here so anyone looking at the code undertsnds the flow with HDA codec.
|
@ranj063 @lgirdwood so I'm not totally sure what is the original idea with hda codecs pm flags and who should "control" it if it is used together with SOF.
Now we have a situation where we want to have the codec to have a non-zero value even if kernel config is 0. So @ranj063 you want me to set auto_runtime_pm in sof? AFAIK it has exactly same effect as just setting the delay directly (well instead of sof 2s if sets it to 3s)... |
|
@ranj063 @lgirdwood so for example in kernel config I got from canonical/lenovo we have: In SOF configs these are not set or are 0. |
@juimonen this is codec specific problem here isnt it, so I was wondering if it makes sense to make it specific when this codec is used instead of setting it unconditionally. As for how to set it, im not sure, its just too confusing. @kv2019i has the most experience with HDA codec driver implementation. maybe he can help? |
|
@ranj063 chrome team was making this as a codec specific patch in patch_realtek.c in hda... So we could try to walk that route. I just feel that it is somehow strange if we set SOF suspend to 2s, why wouldn't we try to set HDA also to that same value... I really can't tell is it "natural" that codec has different suspend delay than SOF itself. |
@juimonen there's no parent-child relationship between the codec and the SOF device. So, I'm not sure what you mean by "natural" here. |
|
@ranj063 @plbossart @juimonen @kv2019i @lgirdwood |
|
@ranj063 well it is more like a philosophical question (so exactly the point I didn't understand), so that this is a SOF driver, SOF takes control what is happening and tells codecs when to suspend. If they are considered "de-coupled", let's do this then in codec. It might be quite a job to justify this in upstream as the legacy driver works... |
@juimonen when you say the legacy driver works, does it work because it sets the suspend delay for all the codecs? |
@ranj063 yes legacy driver works as it sets the suspend delay for all codecs. |
@JeevakaPrabu @juimonen in that case, can we just add a comment like I had in my PR #977 commit message where the legacy driver forbids autosuspend based on quirks? This will need to be handled in SOF at some point as well. |
plbossart
left a comment
plbossart
left a comment
There was a problem hiding this comment.
The commit message is very confusing and I am not able to even comment on the merits of the patch, since I don't know what is broken and why setting two independent time outs to the same value would even work.
sound/soc/sof/intel/hda.c
Outdated
There was a problem hiding this comment.
the commit message is far from clear to me, so I don't get the problem statement;
Even though SOF sets its own suspend delay, the possible HDA codec of
the driver ignores it.
SOF sets is own pm_runtime autosuspend delay?
what does "the possible HDA codec of the driver ignores it." mean? What is ignored by what? Which driver?
So even as SOF sets the suspend delay to 2s, hda
codec will suspend immediately or depending on the value of
CONFIG_SND_HDA_POWER_SAVE_DEFAULT.
Why? Is this because there is no parent-child relationship between the SOF PCI device and the codec device? What causes this behavior? You are describing the result, not the reason for the immediate suspend.
This will lead in some cases to jack
pins being detected incorrectly.
Why? what does pin/jack detection have to do with suspend?
So set the suspend delay time in HDA
codec to same value as SOF tries to set itself.
why does this solve anything? The autosuspend delay on the SOF side is not a strict value, it's subject to kernel scheduling, so I don't see how you avoid a race condition with transitions on the codec side. If you can't control the scheduling, you have to use a significantly larger value on one side so that even in the worst possible case nothing bad happens.
|
@plbossart Well there are parts also unclear to me... Currently with at least alc285 (we have reports of other codecs also) if there isn't any suspend delay, the mic jack detect doesn't work. This is the reason for this PR. Why this happens inside the codec, I have absolutely no idea. We just gotten some informal from realtek that maybe it is not wise to suspend the codec immediately. And yes it seems that if I put some suspend delay to the HDA, the mic jack starts to work. Now, people claim that mic detect works in legacy HDA, and I can only see that those people have hda suspend delay in their kernel config or that legacy hda probe sets the auto suspend, which makes suspend delay to 3s. Otherwise I just don't understand how this would work in legacy and not in sof. So to make the mic detect work with sof, I need to be able to put some suspend delay to hda. I don't understand is it ok to have different suspend times in sof (for dsp) and hda (for codec). For me it would be logical they would have the same suspend time (?). Anyway I don't really care, I just want to make the mic detect work. And the difference I see in sof vs hda legacy is either in kernel config for suspend delay or the probe code which might set the auto_suspend. It might be that the hda legacy is also working by "luck". |
|
@ranj063 one option would be to expose the static hda delay setting function in hda_intel.c (which has the quirks) and call that from sof and not duplicate the quirks... I don't see why that would be an issue for upstream. |
|
@plbossart @ranj063 @RanderWang @ClarexZhou test update: I just tested this patch with Dell Mantis and headphone/headset detection works in boot. I was using our 5.0 kernel branch and signed 1.3 firmware. @RanderWang so don't know what was the issue you we're observing way back and what sw versions? to me this seems to work... |
|
@juimonen Test again, error still can be reproduced on Mantis. But error not occurred on other hda device, eg. Selek MLK and Shuri.
And behavior is a little different on sof-dev and sof-v5.0 branch: And error not seen without this PR. Customer still haven't feedback this issue #1628 right now. I think we can consider to merge this PR once customer feedback. |
|
@ClarexZhou @plbossart @kv2019i @ranj063 So according to my and Clare's testing other platforms should work with this patch, and Mantis has the boot up "delay" (+ of course we get mic detection working in lenovo devices). As I see it this should be merged and Mantis codec should be blacklisted for PM. What do you think? |
@juimonen what do you mean by blacklisting Mantis codec for PM? Do you have a patch for this? |
|
@ranj063 I mean that if we have 1 codec that doesn't like PM, it is quite good candidate for blacklisting in PM sense. I think HDA has this kind of PM blacklist for non-behaving codecs. I don't have a patch for it. (and as tests pointed out, the issues with Mantis is in very early phase after boot, then the device recovers, so this might be even "won't fix") I'm just trying to filter out a solution to maximize devices with working jack detection. I'm kind of running out of my own skills of convincing people with this PR :) So if this doesn't move forward, let's close it. Maybe we can add this only to 5.0 branch etc. |
|
@juimonen @ranj063 @RanderWang I also think we can close this PR atm and look for a codec specific fix later. This PR is to fix ALC285 jack detection issue #1628 on a WHL-based X1 laptop. This issue is not urgent. And we needn't apply it to 5.0 branch now, as 5.0 branch is shared with other CML product devices with other codecs. |
Hmm, I'm just worried a bit that this is a fairly popular device and the issue persists (versus on Mantis where the problem is only present in very early boot). Based on description from @bardliao in #1628 (comment) , this could be a fairly common feature for other Realtek codecs as well, so could affect more devices. It now seems SOF default for powersave is different from legacy HDA driver, and this could trigger more problems down the road.
Ack, I think having a more reasonable default in upstream would be more important. |
|
I want to reopen this PR. We have confirmation from codec vendor that immediate suspend will break jack detection, so the current behaviour in master is not ok. I propose to merge this patch as the failure on Mantis is a much less severe one. @ranj063 Can you review again? @juimonen Is the patch still valid? |
|
@kv2019i yes at least from my point of view it is valid :) |
|
Test on Mantis with sof-dev+PR1689 & master,side effect is below
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. |
|
I am not hot on merging stuff that work on some platforms and breaks another. That doesn't seem quite right. |
|
@plbossart I would argue here that this patch fixes more major existing issues than causes minor side effects i.e. sof driver is IMHO broken without this. If Mantis needs a codec power up/down/up sequence in boot for jack detection to work immediately in UI, and we can't merge this patch for that, that just sounds like we are trusting on some really strange codec behavior... if someone puts on some hda_suspend_delay in kconfig mantis will be "broken" similar way. |
|
@juimonen I am not going to maintain code based on statistical analysis of failure or run an auction to define what goes in. If Mantis is broken let's fix it. |
|
@plbossart @kv2019i FYI: #1837 |
|
@ClarexZhou I can't reproduce the Mantis issue.... I'm using now the new pulseaudio version: @jason77-wang FYI see above, I back ported a lot of your patches, seems to work better for me at least and fixes some recording issues... |
|
@ClarexZhou @jason77-wang ok now I get it... had wrong power save suspend delay value. @jason77-wang any idea why mantis behaves differently than other dell machines or X1? I can work around this by having suspend counter which suspends immediately with first suspend, then after with delay. Seems to work on both X1 and Dell, but is quite a hack. |
|
Latest status: #1837 (comment) |
mengdonglin
added
HDA
|
adding here dmesg from boot, with this pr (jaska.txt) and without this pr (nojaska.txt) in mantis, with a lot of soc debugs on (codec suspends visible). If someone has time to look at those and has ideas, what could cause the issues in Mantis... |
|
@juimonen This can be closed now..? |
|
closing as we have solution sent to upstream |
Always run fbdev removal first to remove simpledrm via sysfb_disable(). This clears the internal state. The later call to drm_aperture_detach_drivers() then does nothing. Otherwise, with drm_aperture_detach_drivers() running first, the call to sysfb_disable() uses inconsistent state. Example backtrace show below: [ 11.663422] ================================================================== [ 11.663426] BUG: KASAN: use-after-free in device_del+0x79/0x5f0 [ 11.663435] Read of size 8 at addr ffff888108185050 by task systemd-udevd/311 [ 11.663440] CPU: 0 PID: 311 Comm: systemd-udevd Tainted: G E 5 .19.0-rc2-1-default+ #1689 [ 11.663445] Hardware name: HP ProLiant DL120 G7, BIOS J01 04/21/2011 [ 11.663447] Call Trace: [ 11.663449] <TASK> [ 11.663451] ? device_del+0x79/0x5f0 [ 11.663456] dump_stack_lvl+0x5b/0x73 [ 11.663462] print_address_description.constprop.0+0x1f/0x1b0 [ 11.663468] ? device_del+0x79/0x5f0 [ 11.663471] ? device_del+0x79/0x5f0 [ 11.663475] print_report.cold+0x3c/0x21c [ 11.663481] ? lock_acquired+0x87/0x1e0 [ 11.663484] ? lock_acquired+0x87/0x1e0 [ 11.663489] ? device_del+0x79/0x5f0 [ 11.663492] kasan_report+0xbf/0xf0 [ 11.663498] ? device_del+0x79/0x5f0 [ 11.663503] device_del+0x79/0x5f0 [ 11.663509] ? device_remove_attrs+0x170/0x170 [ 11.663514] ? lock_is_held_type+0xe8/0x140 [ 11.663523] platform_device_del.part.0+0x19/0xe0 [ 11.663530] platform_device_unregister+0x1c/0x30 [ 11.663535] sysfb_disable+0x2d/0x70 [ 11.663540] remove_conflicting_framebuffers+0x1c/0xf0 [ 11.663546] remove_conflicting_pci_framebuffers+0x130/0x1a0 [ 11.663554] drm_aperture_remove_conflicting_pci_framebuffers+0x86/0xb0 [ 11.663561] ? mgag200_pci_remove+0x30/0x30 [mgag200] [ 11.663578] mgag200_pci_probe+0x2d/0x140 [mgag200] Reported-by: Zack Rusin <zackr@vmware.com> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Reviewed-by: Zack Rusin <zackr@vmware.com> Fixes: ee7a69a ("fbdev: Disable sysfb device registration when removing conflicting FBs") Cc: Javier Martinez Canillas <javierm@redhat.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Helge Deller <deller@gmx.de> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Zhen Lei <thunder.leizhen@huawei.com> Cc: Changcheng Deng <deng.changcheng@zte.com.cn> Link: https://patchwork.freedesktop.org/patch/msgid/20220617121027.30273-1-tzimmermann@suse.de (cherry picked from commit fb84efa) Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Always run fbdev removal first to remove simpledrm via sysfb_disable(). This clears the internal state. The later call to drm_aperture_detach_drivers() then does nothing. Otherwise, with drm_aperture_detach_drivers() running first, the call to sysfb_disable() uses inconsistent state. Example backtrace show below: BUG: KASAN: use-after-free in device_del+0x79/0x5f0 Read of size 8 at addr ffff888108185050 by task systemd-udevd/311 CPU: 0 PID: 311 Comm: systemd-udevd Tainted: G E 5.19.0-rc2-1-default+ #1689 Hardware name: HP ProLiant DL120 G7, BIOS J01 04/21/2011 Call Trace: device_del+0x79/0x5f0 platform_device_del.part.0+0x19/0xe0 platform_device_unregister+0x1c/0x30 sysfb_disable+0x2d/0x70 remove_conflicting_framebuffers+0x1c/0xf0 remove_conflicting_pci_framebuffers+0x130/0x1a0 drm_aperture_remove_conflicting_pci_framebuffers+0x86/0xb0 mgag200_pci_probe+0x2d/0x140 [mgag200] Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Fixes: 873eb3b ("fbdev: Disable sysfb device registration when removing conflicting FBs") Cc: Javier Martinez Canillas <javierm@redhat.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Helge Deller <deller@gmx.de> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Zhen Lei <thunder.leizhen@huawei.com> Cc: Changcheng Deng <deng.changcheng@zte.com.cn> Reviewed-by: Zack Rusin <zackr@vmware.com> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
10 participants
Even though SOF sets its own suspend delay, the possible HDA part of the
driver ignores it. So even as SOF sets the suspend delay to 2s, hda part
will suspend immeadiately or depending on the setup of
CONFIG_SND_HDA_POWER_SAVE_DEFAULT. This will lead in some cases to jack
pins being detected incorrectly. So setup the HDA suspend delay time in
HDA to same value as SOF tries to set itself.
Signed-off-by: Jaska Uimonen jaska.uimonen@linux.intel.com