ASoC: SOF: Intel: hda: refine suspend with stop chip by zhuyingjiang · Pull Request #975 · thesofproject/linux

zhuyingjiang · 2019-05-23T07:30:27Z

Define a SOF hda_dsp_ctrl_stop_chip and use this in hda_suspend, so the disable and clear HDA irq and stream irqs steps all called during the suspend. Codec and HDA link related registers are write if CONFIG_SND_SOC_SOF_HDA is true.
Use the SOF defined ppcap functions in hda_suspend/hda_resume.

plbossart

comments below

plbossart · 2019-05-23T13:37:46Z

+	snd_sof_dsp_update_bits(sdev, HDA_DSP_HDA_BAR, SOF_HDA_INTCTL,
+				SOF_HDA_INT_ALL_STREAM,	0);
+
+	/* disable hda bus irq */


disable controller CIE and GIE

OK, thanks!

no, not disable CIE and GIE, please check this commit torvalds@7d4f606.
Codec can send out INT or wake up controller depending on whether CIE or GIE enabled.(Figure 4, Interupt structure).

@RanderWang Please check
azx_stop_chip
=>snd_hdac_bus_stop_chip
==>azx_int_disable

yes, I tried this PR with my 983 PR for headphone detection on whiskylake. Headphone detection worked with it.

plbossart · 2019-05-23T13:41:25Z

+	/* disable CORB/RIRB */
+	snd_hdac_bus_stop_cmd_io(bus);
+
+	/* disable position buffer */


is this really dependent on SOF_HDA? This is the position buffer that we can use even for NOCODEC for DPIB.

@plbossart OK, thanks for confirm, I will still follow a PR with change the same in hda_dsp_ctrl_init_chip.

It was a question, have you confirmed if yes/no this code is needed for NOCODEC?
And if yes, then the fix is incorrect since you are using snd_hdac_chip_writel() so introduced a dependency on hdac.

@plbossart Yes, I am confirm that this is needed for NOCODEC. for position buffer is according to HDA stream. What ever codec is using, the position buffer need to be set at beginning and reset at end. It is only that NOCODEC will not cause a issue if not set.
I will update the snd_hdac_chip_writel with SOF functions, thanks!

zhuyingjiang · 2019-05-24T07:54:02Z

@plbossart updated

zhuyingjiang · 2019-05-29T08:58:08Z

@plbossart I think this can be merged?

RanderWang

LGTM

plbossart · 2019-05-29T15:23:15Z

this is going to conflict with @RanderWang changes, and I'd like his PR to go in first.

kv2019i

The first commit message doesn't explain why the change is needed.

The description of third commit (for example "If CONFIG_SND_SOC_SOF_HDA is
defined, also disable the CORB/RIRB, and stop i/o.") should go to the second commit as that's where the new stop i/o is added.

In general this looks good and helps with readability.

plbossart

same comment as @kv2019i the code looks well structured but the commit messages need more work for upstream. Thanks!

zhuyingjiang · 2019-06-04T02:34:39Z

@plbossart @kv2019i Updated, thanks for your suggestions!

kv2019i

Thanks, the commits are now better, but e.g. the last commit still needs work. How about

First:
Unify ppcap function setup by using SOF common functions for both
HDA and non-HDA cases.

Second:
Add common hda_dsp_ctrl_stop_chip() function to stop controller with
the same function handling both HDA and non-HDA cases. This function
disables IRQs and clears status masks. When CONFIG_SND_SOC_SOF_HDA
is defined, also disables the CORB/RIRB, and stops i/o.

Third:
Unify suspend code by using SOF common function
hda_dsp_ctrl_stop_chip() which can handle both HDA and non-HDA cases.

Unify ppcap function setup by using SOF common functions for both HDA and non-HDA cases. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

Add common hda_dsp_ctrl_stop_chip() function to stop controller with the same function handling both HDA and non-HDA cases. This function disables IRQs and clears status masks. When CONFIG_SND_SOC_SOF_HDA is defined, also disables the CORB/RIRB, and stops i/o. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

Unify suspend code by using SOF common function hda_dsp_ctrl_stop_chip() which can handle both HDA and non-HDA cases. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

zhuyingjiang · 2019-06-05T02:10:24Z

@kv2019i Updated, thanks for your suggestion!

kv2019i

Thank you, looks good now!

plbossart

Thanks @zhuyingjiang

After 963 iterations of generic/530, it deadlocked during recovery on a pinned inode cluster buffer like so: XFS (pmem1): Starting recovery (logdev: internal) INFO: task kworker/8:0:306037 blocked for more than 122 seconds. Not tainted 5.17.0-rc6-dgc+ thesofproject#975 "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. task:kworker/8:0 state:D stack:13024 pid:306037 ppid: 2 flags:0x00004000 Workqueue: xfs-inodegc/pmem1 xfs_inodegc_worker Call Trace: <TASK> __schedule+0x30d/0x9e0 schedule+0x55/0xd0 schedule_timeout+0x114/0x160 __down+0x99/0xf0 down+0x5e/0x70 xfs_buf_lock+0x36/0xf0 xfs_buf_find+0x418/0x850 xfs_buf_get_map+0x47/0x380 xfs_buf_read_map+0x54/0x240 xfs_trans_read_buf_map+0x1bd/0x490 xfs_imap_to_bp+0x4f/0x70 xfs_iunlink_map_ino+0x66/0xd0 xfs_iunlink_map_prev.constprop.0+0x148/0x2f0 xfs_iunlink_remove_inode+0xf2/0x1d0 xfs_inactive_ifree+0x1a3/0x900 xfs_inode_unlink+0xcc/0x210 xfs_inodegc_worker+0x1ac/0x2f0 process_one_work+0x1ac/0x390 worker_thread+0x56/0x3c0 kthread+0xf6/0x120 ret_from_fork+0x1f/0x30 </TASK> task:mount state:D stack:13248 pid:324509 ppid:324233 flags:0x00004000 Call Trace: <TASK> __schedule+0x30d/0x9e0 schedule+0x55/0xd0 schedule_timeout+0x114/0x160 __down+0x99/0xf0 down+0x5e/0x70 xfs_buf_lock+0x36/0xf0 xfs_buf_find+0x418/0x850 xfs_buf_get_map+0x47/0x380 xfs_buf_read_map+0x54/0x240 xfs_trans_read_buf_map+0x1bd/0x490 xfs_imap_to_bp+0x4f/0x70 xfs_iget+0x300/0xb40 xlog_recover_process_one_iunlink+0x4c/0x170 xlog_recover_process_iunlinks.isra.0+0xee/0x130 xlog_recover_finish+0x57/0x110 xfs_log_mount_finish+0xfc/0x1e0 xfs_mountfs+0x540/0x910 xfs_fs_fill_super+0x495/0x850 get_tree_bdev+0x171/0x270 xfs_fs_get_tree+0x15/0x20 vfs_get_tree+0x24/0xc0 path_mount+0x304/0xba0 __x64_sys_mount+0x108/0x140 do_syscall_64+0x35/0x80 entry_SYSCALL_64_after_hwframe+0x44/0xae </TASK> task:xfsaild/pmem1 state:D stack:14544 pid:324525 ppid: 2 flags:0x00004000 Call Trace: <TASK> __schedule+0x30d/0x9e0 schedule+0x55/0xd0 io_schedule+0x4b/0x80 xfs_buf_wait_unpin+0x9e/0xf0 __xfs_buf_submit+0x14a/0x230 xfs_buf_delwri_submit_buffers+0x107/0x280 xfs_buf_delwri_submit_nowait+0x10/0x20 xfsaild+0x27e/0x9d0 kthread+0xf6/0x120 ret_from_fork+0x1f/0x30 We have the mount process waiting on an inode cluster buffer read, inodegc doing unlink waiting on the same inode cluster buffer, and the AIL push thread blocked in writeback waiting for the inode cluster buffer to become unpinned. What has happened here is that the AIL push thread has raced with the inodegc process modifying, committing and pinning the inode cluster buffer here in xfs_buf_delwri_submit_buffers() here: blk_start_plug(&plug); list_for_each_entry_safe(bp, n, buffer_list, b_list) { if (!wait_list) { if (xfs_buf_ispinned(bp)) { pinned++; continue; } Here >>>>>> if (!xfs_buf_trylock(bp)) continue; Basically, the AIL has found the buffer wasn't pinned and got the lock without blocking, but then the buffer was pinned. This implies the processing here was pre-empted between the pin check and the lock, because the pin count can only be increased while holding the buffer locked. Hence when it has gone to submit the IO, it has blocked waiting for the buffer to be unpinned. With all executing threads now waiting on the buffer to be unpinned, we normally get out of situations like this via the background log worker issuing a log force which will unpinned stuck buffers like this. But at this point in recovery, we haven't started the log worker. In fact, the first thing we do after processing intents and unlinked inodes is *start the log worker*. IOWs, we start it too late to have it break deadlocks like this. Avoid this and any other similar deadlock vectors in intent and unlinked inode recovery by starting the log worker before we recover intents and unlinked inodes. This part of recovery runs as though the filesystem is fully active, so we really should have the same infrastructure running as we normally do at runtime. Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Darrick J. Wong <djwong@kernel.org> Reviewed-by: Chandan Babu R <chandan.babu@oracle.com> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

AIL flushing can get stuck here: [316649.005769] INFO: task xfsaild/pmem1:324525 blocked for more than 123 seconds. [316649.007807] Not tainted 5.17.0-rc6-dgc+ thesofproject#975 [316649.009186] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [316649.011720] task:xfsaild/pmem1 state:D stack:14544 pid:324525 ppid: 2 flags:0x00004000 [316649.014112] Call Trace: [316649.014841] <TASK> [316649.015492] __schedule+0x30d/0x9e0 [316649.017745] schedule+0x55/0xd0 [316649.018681] io_schedule+0x4b/0x80 [316649.019683] xfs_buf_wait_unpin+0x9e/0xf0 [316649.021850] __xfs_buf_submit+0x14a/0x230 [316649.023033] xfs_buf_delwri_submit_buffers+0x107/0x280 [316649.024511] xfs_buf_delwri_submit_nowait+0x10/0x20 [316649.025931] xfsaild+0x27e/0x9d0 [316649.028283] kthread+0xf6/0x120 [316649.030602] ret_from_fork+0x1f/0x30 in the situation where flushing gets preempted between the unpin check and the buffer trylock under nowait conditions: blk_start_plug(&plug); list_for_each_entry_safe(bp, n, buffer_list, b_list) { if (!wait_list) { if (xfs_buf_ispinned(bp)) { pinned++; continue; } Here >>>>>> if (!xfs_buf_trylock(bp)) continue; This means submission is stuck until something else triggers a log force to unpin the buffer. To get onto the delwri list to begin with, the buffer pin state has already been checked, and hence it's relatively rare we get a race between flushing and encountering a pinned buffer in delwri submission to begin with. Further, to increase the pin count the buffer has to be locked, so the only way we can hit this race without failing the trylock is to be preempted between the pincount check seeing zero and the trylock being run. Hence to avoid this problem, just invert the order of trylock vs pin check. We shouldn't hit that many pinned buffers here, so optimising away the trylock for pinned buffers should not matter for performance at all. Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Chandan Babu R <chandan.babu@oracle.com> Reviewed-by: Darrick J. Wong <djwong@kernel.org> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

zhuyingjiang requested review from lgirdwood and plbossart as code owners May 23, 2019 07:30

zhuyingjiang mentioned this pull request May 23, 2019

[RFC] ASoC: SOF: Intel: hda: refine init/stop chip in hda_suspend/hda_resume #965

Closed

plbossart requested changes May 23, 2019

View reviewed changes

zhuyingjiang force-pushed the topic/sof-dev-refine-suspend-with-stop-chip branch from dd69928 to f1fff35 Compare May 24, 2019 07:52

zhuyingjiang requested a review from ranj063 May 24, 2019 07:54

zhuyingjiang mentioned this pull request May 27, 2019

[APL][GLK] harden: delays after programming registers (and/or loops to check status changes) #777

Closed

zhuyingjiang force-pushed the topic/sof-dev-refine-suspend-with-stop-chip branch from f1fff35 to f19fe55 Compare May 27, 2019 05:04

zhuyingjiang requested review from RanderWang and plbossart May 27, 2019 05:08

RanderWang previously approved these changes May 29, 2019

View reviewed changes

kv2019i requested changes Jun 3, 2019

View reviewed changes

plbossart requested changes Jun 3, 2019

View reviewed changes

zhuyingjiang dismissed RanderWang’s stale review via ab34e87 June 4, 2019 02:32

zhuyingjiang force-pushed the topic/sof-dev-refine-suspend-with-stop-chip branch from f19fe55 to ab34e87 Compare June 4, 2019 02:32

zhuyingjiang requested review from RanderWang, kv2019i and plbossart June 4, 2019 02:34

kv2019i requested changes Jun 4, 2019

View reviewed changes

zhuyingjiang added 3 commits June 5, 2019 09:56

ASoC: SOF: Intel: hda: use the SOF defined ppcap functions

026c06a

Unify ppcap function setup by using SOF common functions for both HDA and non-HDA cases. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

ASoC: SOF: Intel: hda: use the defined stop chip in suspend

6322320

Unify suspend code by using SOF common function hda_dsp_ctrl_stop_chip() which can handle both HDA and non-HDA cases. Signed-off-by: Zhu Yingjiang <yingjiang.zhu@linux.intel.com>

zhuyingjiang force-pushed the topic/sof-dev-refine-suspend-with-stop-chip branch from ab34e87 to 6322320 Compare June 5, 2019 02:09

zhuyingjiang requested a review from kv2019i June 5, 2019 04:22

kv2019i approved these changes Jun 5, 2019

View reviewed changes

plbossart approved these changes Jun 5, 2019

View reviewed changes

plbossart merged commit 7f300c7 into thesofproject:topic/sof-dev Jun 5, 2019

Conversation

zhuyingjiang commented May 23, 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!

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!

zhuyingjiang commented May 24, 2019

Uh oh!

zhuyingjiang commented May 29, 2019

Uh oh!

RanderWang left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart commented May 29, 2019

Uh oh!

kv2019i left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang commented Jun 4, 2019

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

zhuyingjiang commented Jun 5, 2019

Uh oh!

kv2019i 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kv2019i left a comment •

edited

Loading