Skip to content

hda: chain transition states fix #6983

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
lgirdwood merged 2 commits into from
Jan 30, 2023
Merged

hda: chain transition states fix #6983

lgirdwood merged 2 commits into from
Jan 30, 2023

Conversation

@makarukp
Copy link
Contributor

@makarukp makarukp commented Jan 23, 2023

Require maintenance in logic to handle multiple start/pause requests by one chain instance.

Signed-off-by: Piotr Makaruk piotr.makaruk@intel.com

@makarukp makarukp mentioned this pull request Jan 23, 2023
Merged
@ujfalusi ujfalusi mentioned this pull request Jan 23, 2023
Closed
@ujfalusi
Copy link
Contributor

ujfalusi commented Jan 23, 2023

Created a draft PR with this and a revert to enable the ChainDMA on tgl: #6984

jsarha
jsarha reviewed Jan 23, 2023
View reviewed changes
Copy link
Contributor

@jsarha jsarha left a comment

Choose a reason for hiding this comment

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

This is a definite improvement over the situation before this PR, since before this a resume after pause did not work at all. Now it works, at least most of the time. If I run "sof-test/test-case/multiple-pause-resume.sh -r 50" on my upx-i11 it fails more often than succeeds. However, hitting the failure usually takes quite a while. When looking at FW logs, I can see several:
[ 16.733930] copier: comp:0 0x40000 failed to find dai comp or sink pipeline not running.
and even more:
[ 20.268613] dai_comp: comp:1 0x40001 dai_copy(): nothing to copy

@jsarha
Copy link
Contributor

jsarha commented Jan 23, 2023

I tried adding "pm_policy_state_lock_put(PM_STATE_RUNTIME_IDLE, PM_ALL_SUBSTATES);" after "cd->chain_task.state = SOF_TASK_STATE_INIT;" but it does not appear to help (nor make the behavior worse. One example of failure in the test logs:

(3/50) pcm'Deepbuffer HDA Analog' cmd'aplay' id'5': Wait for 44 ms before resume
(3/50) pcm'HDA Analog' cmd'aplay' id'3': Wait for 49 ms before resume
(4/50) pcm'HDA Analog' cmd'aplay' id'3': Wait for 49 ms before pause
(4/50) pcm'Deepbuffer HDA Analog' cmd'aplay' id'5': Wait for 48 ms before pause
(4/50) pcm'Deepbuffer HDA Analog' cmd'aplay' id'5': Wait for 49 ms before resume
(4/50) pcm'HDA Analog' cmd'aplay' id'3': Wait for 42 ms before resume
aplay: pcm_write:2086: write error: Input/output error
aplay: pcm_write:2086: write error: Input/output error

2023-01-23 22:38:09 UTC [REMOTE_INFO] pipeline: HDA Analog with aplay
2023-01-23 22:38:09 UTC [REMOTE_INFO] pipeline: Deepbuffer HDA Analog with aplay
2023-01-23 22:38:09 UTC [REMOTE_INFO] Check expect exit status
declare -- cmd="journalctl_cmd --since=@1674513395"
2023-01-23 22:38:09 UTC [REMOTE_ERROR] pause resume PID 5752 had non-zero exit status

lyakh
lyakh reviewed Jan 24, 2023
View reviewed changes
@makarukp makarukp force-pushed the hw_hda_chain_states_transition_fix branch 2 times, most recently from 5ea2d11 to 4606391 Compare January 24, 2023 11:40
@makarukp
hda: chain transition states fix prepare
87d1a69 
Rearrange functions order.

Signed-off-by: Piotr Makaruk <piotr.makaruk@intel.com>
@makarukp makarukp force-pushed the hw_hda_chain_states_transition_fix branch from 4606391 to e5811db Compare January 25, 2023 09:08
jsarha
jsarha approved these changes Jan 25, 2023
View reviewed changes
Copy link
Contributor

@jsarha jsarha left a comment

Choose a reason for hiding this comment

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

Now after over 20 test runs of multiple-pause-resume I now saw an error also from the FW build where I had 6983 & 6656 reverted. So I guess its reasonable assume 6656 with 6983 is Ok. My review comments were also well explained. Approved.

lyakh
lyakh reviewed Jan 26, 2023
View reviewed changes
@makarukp makarukp force-pushed the hw_hda_chain_states_transition_fix branch from e5811db to 45cb6d9 Compare January 27, 2023 11:18
lgirdwood
lgirdwood approved these changes Jan 27, 2023
View reviewed changes
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

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

I will rerun CI after some regression fixes are merged today.

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 27, 2023

SOFCI TEST

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 27, 2023

topology/topology2/avs-tplg/sof-nocodec-bt-lbm.tplg build failure in https://sof-ci.01.org/sofpr/PR6983/build3565/build/tools.txt , https://sof-ci.01.org/sofpr/PR6983/build3565/build https://github.com/thesofproject/sof/actions/runs/4023985041/jobs/6915487545 and many others fixed by #6999

@marc-hb
Copy link
Collaborator

marc-hb commented Jan 27, 2023

@makarukp to fix Github Actions and retest with #6999 you must perform an empty git commit --amend -C HEAD to change the SHA1 and then force-push.

Don't rebase as this makes it very difficult to see that nothing changed. Most CIs perform a merge before testing and our Github Actions do.

@makarukp
hda: chain transition states fix
43640e4 
Require maintenance in logic to handle multiple start/pause requests by
one chain instance.

Signed-off-by: Piotr Makaruk <piotr.makaruk@intel.com>
@makarukp makarukp force-pushed the hw_hda_chain_states_transition_fix branch from 45cb6d9 to 43640e4 Compare January 30, 2023 07:59
@kv2019i kv2019i requested a review from lyakh January 30, 2023 11:00
@kv2019i
Copy link
Collaborator

kv2019i commented Jan 30, 2023

@lyakh good to go?

@lgirdwood lgirdwood merged commit 7383cbe into thesofproject:main Jan 30, 2023
@makarukp makarukp mentioned this pull request Feb 7, 2023
Closed
@miRoox miRoox mentioned this pull request Feb 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dabekjakub dabekjakub dabekjakub left review comments

@jsarha jsarha jsarha approved these changes

@lgirdwood lgirdwood lgirdwood approved these changes

@dnikodem dnikodem dnikodem approved these changes

@tmleman tmleman tmleman approved these changes

@ranj063 ranj063 Awaiting requested review from ranj063

@ujfalusi ujfalusi Awaiting requested review from ujfalusi

@kv2019i kv2019i Awaiting requested review from kv2019i kv2019i is a code owner

@plbossart plbossart Awaiting requested review from plbossart plbossart is a code owner

@mmaka1 mmaka1 Awaiting requested review from mmaka1 mmaka1 is a code owner

@lbetlej lbetlej Awaiting requested review from lbetlej lbetlej is a code owner

@dbaluta dbaluta Awaiting requested review from dbaluta dbaluta is a code owner

@lyakh lyakh Awaiting requested review from lyakh

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@makarukp @ujfalusi @jsarha @marc-hb @kv2019i @lyakh @lgirdwood @dabekjakub @dnikodem @tmleman