Skip to content

Conversation

@kv2019i
Copy link
Collaborator

@kv2019i kv2019i commented Jun 3, 2019

Patchset to implement similar runtime PM idle logic as in HDA legacy driver to ensure codec is powered off first before HDA controller and DSP are put to suspend. To impelemnt this, add infrastructure support for the idle callback.

libinyang
libinyang previously approved these changes Jun 3, 2019
Copy link

@libinyang libinyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here please change %08X to %lX for I got warning with your patch

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RanderWang Thanks! The cast should avoid the warning, but agreed better to solve this without any cast. I'll upload an updated version.

Copy link
Member

@plbossart plbossart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kv2019i wondering why this is for CNL only, this could apply for all SKL+ platforms, no?
Also we should submit this on alsa-devel, this is a change in the SOF core and we should get feedback directly from a wider community.

@ranj063
Copy link
Collaborator

ranj063 commented Jun 4, 2019

Also we should submit this on alsa-devel, this is a change in the SOF core and we should get feedback directly from a wider community.

@plbossart @kv2019i we should run some suspend stress tests with this before we submit it to alsa-devel.
The last time i tested another variant of this, i saw issues with the SOF device not being able to suspend

@RanderWang
Copy link

I tested again and found one issue: when getting back from system s3, controller invokes idle function and finds a codec is still active, so controller doesn't go to runtime suspend. Then the codec goes to runtime suspend mode. But now the controller is always active until a playback or capture happens. It seems that there is no chance for suspend controller if idle function return false. I also found this issue with legacy hda driver.

@RanderWang
Copy link

I tested again and found one issue: when getting back from system s3, controller invokes idle function and finds a codec is still active, so controller doesn't go to runtime suspend. Then the codec goes to runtime suspend mode. But now the controller is always active until a playback or capture happens. It seems that there is no chance for suspend controller if idle function return false. I also found this issue with legacy hda driver.

My last comments was wrong. I debugged this issue and found I missed the commit for HDMI.
Now controller could go to suspend mode.

@plbossart
Copy link
Member

@kv2019i is this PR still candidate for merge? Doesn't look like it's moved in 6 days.

@kv2019i kv2019i added the Unclear No agreement on problem statement and resolution label Jun 11, 2019
@kv2019i
Copy link
Collaborator Author

kv2019i commented Jun 11, 2019

@kv2019i is this PR still candidate for merge? Doesn't look like it's moved in 6 days.

@ranj063 , @RanderWang and @lyakh what do you think? Based on the validation data we have gathered since last week, it seems this is not strictly needed. That means the change is more about alignment to legacy driver rather than fixing a specific concrete problem. If we don't need the codec_powered based check, then we don't have a real need for the idle callback either.

In any case, this should first be sent to alsa-devel as it changes SOF, so I'll mark it "unclear" in GH.

@RanderWang
Copy link

RanderWang commented Jun 12, 2019

@kv2019i is this PR still candidate for merge? Doesn't look like it's moved in 6 days.

@ranj063 , @RanderWang and @lyakh what do you think? Based on the validation data we have gathered since last week, it seems this is not strictly needed. That means the change is more about alignment to legacy driver rather than fixing a specific concrete problem. If we don't need the codec_powered based check, then we don't have a real need for the idle callback either.

In any case, this should first be sent to alsa-devel as it changes SOF, so I'll mark it "unclear" in GH.

It is needed by jack detection. When resuming from S3, controller is suspended before codecs, and WAKEEN feature doesn't work for this case. And I also checked this case with legacy hda driver which
also can't detect jack event. We need this patch to keep correct power sequence. Please refine it, thanks!

lyakh
lyakh previously approved these changes Jun 12, 2019
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

subject to testing, since I cannot really evaluate the effects of this change. I also agree with Pierre's comment - looks like this should be added to all architectures.

@kv2019i
Copy link
Collaborator Author

kv2019i commented Jun 12, 2019

Patch updated to include support for APL as well, and sent to alsa-devel for review as requested.
UPDATE: patch in alsa-devel https://mailman.alsa-project.org/pipermail/alsa-devel/2019-June/150923.html

@kv2019i kv2019i removed the Unclear No agreement on problem statement and resolution label Jun 12, 2019
@kv2019i kv2019i added the upstream Patch has been sent to upstream (e.g. alsa-devel) label Jun 12, 2019
@RanderWang
Copy link

please check ci build failed

@kv2019i
Copy link
Collaborator Author

kv2019i commented Jun 12, 2019

please check ci build failed

Oh, interesting. It seems this has slipped in via commit dec1616 (already merged).

I can make a PR to fix this.
UDPATE: done -> #1030

@ranj063
Copy link
Collaborator

ranj063 commented Jun 12, 2019

@kv2019i I know this has already been sent upstream but just wanted to report I have run the S3 stress test with this patchset and it passed!

RanderWang
RanderWang previously approved these changes Jun 13, 2019
Copy link

@RanderWang RanderWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ranj063
ranj063 previously approved these changes Jun 13, 2019
@ranj063
Copy link
Collaborator

ranj063 commented Jun 13, 2019

@plbossart Takashi has already ack'ed these patches but the ones sent upstream are slightly different because of the ordering of the patches.

@kv2019i
Copy link
Collaborator Author

kv2019i commented Jun 18, 2019

v2 sent to alsa-devel

ranj063
ranj063 previously approved these changes Jun 18, 2019
Copy link
Collaborator

@ranj063 ranj063 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. can be merged when the patches are applied upstream

RanderWang
RanderWang previously approved these changes Jun 19, 2019
Copy link

@RanderWang RanderWang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kv2019i
Copy link
Collaborator Author

kv2019i commented Jun 28, 2019

Still waiting for upstream to take these. I'll send a reminder next week if no activity in 2 weeks.

@ranj063
Copy link
Collaborator

ranj063 commented Jul 7, 2019

@kv2019i merged upstream. do you mind resolving the conflicts?

kv2019i added 3 commits July 8, 2019 15:41
Report codec power status to the HDA codec bus from runtime pm
suspend and resume callbacks. This is required to implement
runtime idle logic that relies on 'codec_powered' field of hdac_bus
to be maintained for all codecs.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Link: https://lore.kernel.org/r/20190702132428.13129-2-kai.vehmanen@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Add ability to implement a SOF device level runtime idle callback.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Link: https://lore.kernel.org/r/20190702132428.13129-3-kai.vehmanen@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
Implement runtime idle for CNL/APL devices using similar runtime
PM idle logic as the Intel AZX HDA driver. If any HDA codecs are
powered when runtime suspend request comes, return -EBUSY. By doing
this, strict ordering is enforced between HDA codec and the HDA
controller.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Link: https://lore.kernel.org/r/20190702132428.13129-4-kai.vehmanen@linux.intel.com
Signed-off-by: Mark Brown <broonie@kernel.org>
@kv2019i kv2019i dismissed stale reviews from RanderWang and ranj063 via 1d070fd July 8, 2019 12:53
@kv2019i kv2019i force-pushed the topic/sof-idle-callback branch from 0372519 to 1d070fd Compare July 8, 2019 12:53
@kv2019i
Copy link
Collaborator Author

kv2019i commented Jul 8, 2019

@kv2019i merged upstream. do you mind resolving the conflicts?

Ack @ranj063 rebased the versions merged upstream, solved the one conflict and reuploaded.

@plbossart
Copy link
Member

@kv2019i can you double-check all the patches were added with today's upstream merge and close this PR if we are good?

@ranj063
Copy link
Collaborator

ranj063 commented Jul 18, 2019

@plbossart Kai is on vacation for the next couple of weeks. I can confirm all patches in this PR are merged upstream.

@ranj063 ranj063 closed this Jul 18, 2019
fredoh9 pushed a commit to fredoh9/linux that referenced this pull request Aug 5, 2019
Refactor the NE_FIT_TYPE split case when it comes to an allocation of one
extra object.  We need it in order to build a remaining space.  The
preload is done per CPU in non-atomic context with GFP_KERNEL flags.

More permissive parameters can be beneficial for systems which are suffer
from high memory pressure or low memory condition.  For example on my KVM
system(4xCPUs, no swap, 256MB RAM) i can simulate the failure of page
allocation with GFP_NOWAIT flags.  Using "stress-ng" tool and starting N
workers spinning on fork() and exit(), i can trigger below trace:

<snip>
[  179.815161] stress-ng-fork: page allocation failure: order:0, mode:0x40800(GFP_NOWAIT|__GFP_COMP), nodemask=(null),cpuset=/,mems_allowed=0
[  179.815168] CPU: 0 PID: 12612 Comm: stress-ng-fork Not tainted 5.2.0-rc3+ thesofproject#1003
[  179.815170] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
[  179.815171] Call Trace:
[  179.815178]  dump_stack+0x5c/0x7b
[  179.815182]  warn_alloc+0x108/0x190
[  179.815187]  __alloc_pages_slowpath+0xdc7/0xdf0
[  179.815191]  __alloc_pages_nodemask+0x2de/0x330
[  179.815194]  cache_grow_begin+0x77/0x420
[  179.815197]  fallback_alloc+0x161/0x200
[  179.815200]  kmem_cache_alloc+0x1c9/0x570
[  179.815202]  alloc_vmap_area+0x32c/0x990
[  179.815206]  __get_vm_area_node+0xb0/0x170
[  179.815208]  __vmalloc_node_range+0x6d/0x230
[  179.815211]  ? _do_fork+0xce/0x3d0
[  179.815213]  copy_process.part.46+0x850/0x1b90
[  179.815215]  ? _do_fork+0xce/0x3d0
[  179.815219]  _do_fork+0xce/0x3d0
[  179.815226]  ? __do_page_fault+0x2bf/0x4e0
[  179.815229]  do_syscall_64+0x55/0x130
[  179.815231]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
[  179.815234] RIP: 0033:0x7fedec4c738b
...
[  179.815237] RSP: 002b:00007ffda469d730 EFLAGS: 00000246 ORIG_RAX: 0000000000000038
[  179.815239] RAX: ffffffffffffffda RBX: 00007ffda469d730 RCX: 00007fedec4c738b
[  179.815240] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000001200011
[  179.815241] RBP: 00007ffda469d780 R08: 00007fededd6e300 R09: 00007ffda47f50a0
[  179.815242] R10: 00007fededd6e5d0 R11: 0000000000000246 R12: 0000000000000000
[  179.815243] R13: 0000000000000020 R14: 0000000000000000 R15: 0000000000000000
[  179.815245] Mem-Info:
[  179.815249] active_anon:12686 inactive_anon:14760 isolated_anon:0
                active_file:502 inactive_file:61 isolated_file:70
                unevictable:2 dirty:0 writeback:0 unstable:0
                slab_reclaimable:2380 slab_unreclaimable:7520
                mapped:15069 shmem:14813 pagetables:10833 bounce:0
                free:1922 free_pcp:229 free_cma:0
<snip>

Link: http://lkml.kernel.org/r/20190606120411.8298-3-urezki@gmail.com
Signed-off-by: Uladzislau Rezki (Sony) <urezki@gmail.com>
Cc: Hillf Danton <hdanton@sina.com>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Oleksiy Avramchenko <oleksiy.avramchenko@sonymobile.com>
Cc: Roman Gushchin <guro@fb.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

upstream Patch has been sent to upstream (e.g. alsa-devel)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants