Skip to content
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.

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

topology2: Add new token 'playback_pause_supported' and set it true f… #9202

Merged

Conversation

ujfalusi
Copy link
Contributor

@ujfalusi ujfalusi commented Jun 5, 2024

We need to face with reality that the pause/resume is a feature that is not
well tested (end users are using audio via audio servers and they don't
use pause/resume) causing constant issues with no real life benefit:
With IPC4 multiple pause/resume will make the delay reporting to be
exponentially shoot out, making the reported delay to be unusable.

Looks like suspend/resume with paused stream has been broken for a long
time and just got noticed (since it was not tested).

Add a new token to allow selected PCMs to advertise pause support and
keep it false by default.

The kernel side will allow ignoring the flag to keep the pause advertised
for continued testing while protecting accidental use of it by users.

Signed-off-by: Peter Ujfalusi [email protected]
Link: thesofproject/linux#5035

@ujfalusi
Copy link
Contributor Author

ujfalusi commented Jun 5, 2024

Changes since v1:

  • add token for capture_pause_supported as well and enable it for nocodec

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Patch looks good. I only concern is whether we should have more conservative opt-out approach to deploy this to existing system, see thesofproject/linux#5041 (review)

@plbossart
Copy link
Member

plbossart commented Jun 5, 2024

Not in agreement, see comments on the kernel side. I want to keep those tests as telltales of possible issues. Exhibit A is the SoundWire bank switch time out found mostly with pause-release and now found in regular cases now that we're looking into it.

@@ -1374,6 +1383,8 @@ IncludeByKey.SSP1_ENABLED {
name "$SSP1_PCM_NAME"
id 1
direction "duplex"
playback_pause_supported "true"
capture_pause_supported "true"
Copy link
Member

Choose a reason for hiding this comment

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

if we use the hda_force_pause_support kernel parameter we don't need to special-case the nocodec topology.

Best to define the token as false always, and gradually re-add support as 'true' if/when ready on a platform-by-platform basis

@ujfalusi ujfalusi force-pushed the peter/pr/pause-resume-token-01 branch from f9c6de0 to 76910e9 Compare June 7, 2024 09:12
@ujfalusi
Copy link
Contributor Author

ujfalusi commented Jun 7, 2024

Changes since v2:

  • Commit message reworded
  • Drop the changes for nocodec

@ujfalusi ujfalusi force-pushed the peter/pr/pause-resume-token-01 branch from 76910e9 to 0c44e4b Compare June 7, 2024 10:04
… by default

We need to face with reality that the pause/resume is a feature that is not
well tested (end users are using audio via audio servers and they don't
use pause/resume) causing constant issues with no real life benefit:
With IPC4 multiple pause/resume will make the delay reporting to be
exponentially shoot out, making the reported delay to be unusable.

Looks like suspend/resume with paused stream has been broken for a long
time and just got noticed (since it was not tested).

Add a new token to allow selected PCMs to advertise pause support and
keep it false by default.

The kernel side will allow ignoring the flag to keep the pause advertised
for continued testing while protecting accidental use of it by users.

Signed-off-by: Peter Ujfalusi <[email protected]>
Link: thesofproject/linux#5035
@ujfalusi
Copy link
Contributor Author

ujfalusi commented Jun 7, 2024

Change since v3:

  • commit title changed to not only refer to playback direction

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.

@lgirdwood @kv2019i this can be merged without waiting for CI to be ready - it's however required on the kernel side.

@lgirdwood lgirdwood added this to the v2.10 milestone Jun 7, 2024
@lgirdwood lgirdwood merged commit c1ad36d into thesofproject:main Jun 7, 2024
38 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants