Skip to content

ASoC: SOF: ipc4-pcm: do not report invalid delay values #5513

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

Merged
ujfalusi merged 1 commit into from
Sep 4, 2025
Merged

ASoC: SOF: ipc4-pcm: do not report invalid delay values #5513

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions sound/soc/sof/ipc4-pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ struct sof_ipc4_pcm_stream_priv {
*/
#define DELAY_BOUNDARY U32_MAX

#define DELAY_MAX (DELAY_BOUNDARY >> 1)

Copy link
Collaborator Author

@kv2019i kv2019i Aug 18, 2025

Choose a reason for hiding this comment

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

I did consider multiple options:

  • use runtime->buffer_size as reference for maximum sensible/valid delay. work in most cases, but in theory DSP could have bigger internal delays, so dropped this
  • define DELAY_MAX in wallclock time (e.g. 10 seconds of audio) and calculate a frame limit by multiplying with sampling rate -> in the end, this would require some overhead to calculate based on the stream or assume some maximum realistic sampling rate... and in the end, this is not any less adhoc than just defining BOUNDARY/2

... so went with BOUNDARY/2 in the end. This will primarily cover cases where the DSP pointers have gone out-of-sync (e.g. we had bugs with handling xruns and pauses in the past) and continuously report delay values close to DELAY_BOUNDARY

static inline struct sof_ipc4_timestamp_info *
sof_ipc4_sps_to_time_info(struct snd_sof_pcm_stream *sps)
{
Expand Down Expand Up @@ -1266,6 +1268,12 @@ static int sof_ipc4_pcm_pointer(struct snd_soc_component *component,
else
time_info->delay = head_cnt - tail_cnt;

if (time_info->delay > DELAY_MAX) {
Copy link
Collaborator

@ujfalusi ujfalusi Aug 28, 2025

Choose a reason for hiding this comment

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

when this happens it is because the dai ptr counter overtook the host pointer, which indicates incorrect start offset and a rare race when the dai / host DMA burst happens just a bit out of sync (or the pointer report) and things flop.
I think I'm fine with this filtering at the end.

Copy link
Collaborator Author

@kv2019i kv2019i Aug 28, 2025

Choose a reason for hiding this comment

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

Ack @ujfalusi . I think this is just a safety-valve against trusting the FW too much. If this is hit, it's an issue we would have to go and hunt for these cases and fix them one by one, but without an error printed, these are difficult to catch.

One option is to just leave this out-of-tree as a debugging patch. But if these do happen on end-user systems, letting clearly invalid delay values through will cause pretty major issues (e.g. video playback not working), so I'm leaning towards having a safety check built in.

dev_dbg_ratelimited(sdev->dev,
"inaccurate delay, host %llu dai_cnt %llu", host_cnt, dai_cnt);
time_info->delay = 0;
}

/*
* Convert the host byte counter to PCM pointer which wraps in buffer
* and it is in frames
Expand Down
Loading