Skip to content

Fix delay error #4791

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

Closed
RanderWang wants to merge 1 commit into from
Closed

Fix delay error #4791

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: 4 additions & 4 deletions sound/soc/sof/ipc4-pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -891,17 +891,17 @@ static snd_pcm_sframes_t sof_ipc4_pcm_delay(struct snd_soc_component *component,
tmp_ptr -= time_info->stream_start_offset;

/* Calculate the delay taking into account that both pointer can wrap */
div64_u64_rem(tmp_ptr, substream->runtime->boundary, &tmp_ptr);
div64_u64_rem(tmp_ptr, substream->runtime->buffer_size, &tmp_ptr);
Copy link
Collaborator

@kv2019i kv2019i Jan 24, 2024

Choose a reason for hiding this comment

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

Hmm, buffer-size and boundary-size could be both used as modulo basis, but ALSA uses the adhoc large value of "boundary" so that differences larger than buffer_size can be detected. With just the above check, you cannot detect whether the value wrapped around or whether link/host position difference is just larger than buffer_size. I think we need to cover this case as well (especially given FW just had a bug around this area so it is possible we get a completely wrong value from FW and kernel will have to recover gracefully). So I think we should keep using boundary.

OTOH, if the delay difference is larger than buffer_size, something is seriously wrong, and we should not return such delay values, but gap the value. So in this sense this patch is much safer way to calculate the delay.

How about we keep the current logic in tact but add add a check:

if (head_ptr < tail_ptr)
     delay = substream->runtime->boundary - tail_ptr + head_ptr;
else
     delay = head_ptr - tail_ptr;

if (delay > substream->runtime->buffers_size) {
      dev_dbg(sdev->dev, "Link position exceeding buffer size, head %lu tail %lu\n", head_ptr, tail_ptr);
      delay = 0;
}

return delay;

So we'd never return larger delay than buffer-size, but we'd also detect the case where link position drift too far from host position, and not ignore this silently (buffer_size module would hide this).

Copy link
Author

@RanderWang RanderWang Jan 29, 2024

Choose a reason for hiding this comment

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

@kv2019i snd_sof_pcm_platform_pointer() return the position in [0, buffer_size], we need to wrap tmp_ptr to [0, buffer_size] so that we can compare them. Or after few cycles, head_ptr is alway < tail_ptr for playback.

Copy link
Author

@RanderWang RanderWang Jan 29, 2024

Choose a reason for hiding this comment

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

I dump the log for driver,

[521646.446395] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 32 tail 16402
[521646.446696] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 32 tail 16402
[521646.531253] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 4112 tail 20482
[521646.531503] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 4112 tail 20482
[521646.616390] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 8192 tail 24562
[521646.616660] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 8192 tail 24562
[521646.702375] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 12320 tail 28690
[521646.702642] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 12320 tail 28690
[521646.787340] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 16 tail 32770
[521646.787616] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 16 tail 32770
[521646.872362] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 4096 tail 36850
[521646.872613] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 4096 tail 36850
[521646.958319] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 8224 tail 40978
[521646.958682] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 8224 tail 40978
[521647.043383] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 12304 tail 45058
[521647.043649] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 12304 tail 45058
[521647.128393] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 0 tail 49138
[521647.128650] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 0 tail 49138
[521647.214363] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 4128 tail 53266
[521647.214616] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 4128 tail 53266
[521647.299364] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 8208 tail 57346
[521647.299613] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 8208 tail 57346
[521647.384365] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 12288 tail 61426
[521647.384584] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 12288 tail 61426
[521647.470367] sof-audio-pci-intel-mtl Link position exceeding buffer size, head 32 tail 65554

Copy link
Collaborator

@kv2019i kv2019i Jan 30, 2024

Choose a reason for hiding this comment

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

Right, sorry @RanderWang , I missed you changed also to use the snd_sof_pcm_platform_pointer.

Hmm, we do lose the ability to express delays longer than buffer_size (and ability to detect such cases reliably, I don't think we in practise need to support such delays with SOF).

OTOH, so with now, if tmp_ptr is read with get_position, it is a value between 0 and buffersize, while the LLP read case will return a value that will wrap only at UINT64_MAX?

Copy link
Collaborator

@ujfalusi ujfalusi Jan 30, 2024

Choose a reason for hiding this comment

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

the pointer callback is returning the DMA position between [0 , buffer_size - 1] in frames. The LLP is a linear position in frames, so the LLP needs to be normalized down with buffer_size to be usable.

Copy link
Collaborator

@ujfalusi ujfalusi Jan 30, 2024

Choose a reason for hiding this comment

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

Right, sorry @RanderWang , I missed you changed also to use the snd_sof_pcm_platform_pointer.

Hmm, we do lose the ability to express delays longer than buffer_size (and ability to detect such cases reliably, I don't think we in practise need to support such delays with SOF).

Hrm, that is an interesting problem. Indeed we could have a long DSP processing pipeline which would take longer to pass through than the ALSA buffer, it is a valid concern I think

OTOH, so with now, if tmp_ptr is read with get_position, it is a value between 0 and buffersize, while the LLP read case will return a value that will wrap only at UINT64_MAX?

if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
head_ptr = substream->runtime->status->hw_ptr;
head_ptr = snd_sof_pcm_platform_pointer(sdev, substream);
tail_ptr = tmp_ptr;
} else {
head_ptr = tmp_ptr;
tail_ptr = substream->runtime->status->hw_ptr;
tail_ptr = snd_sof_pcm_platform_pointer(sdev, substream);
Copy link
Collaborator

@ujfalusi ujfalusi Jan 19, 2024

Choose a reason for hiding this comment

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

commit title: ASoC: SOF: ipc4-pcm: ...

Copy link
Member

@plbossart plbossart Jan 19, 2024

Choose a reason for hiding this comment

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

and it's not clear to the average reviewer what snd_sof_pcm_platform_pointer() might do either or what this improves.

Copy link
Author

@RanderWang RanderWang Jan 22, 2024

Choose a reason for hiding this comment

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

use snd_sof_pcm_platform_pointer() to get latest host dma position. Updated commit message, thanks for review

Copy link
Collaborator

@kv2019i kv2019i Jan 24, 2024

Choose a reason for hiding this comment

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

Ack, at least I understand the commit description now.

}

if (head_ptr < tail_ptr)
return substream->runtime->boundary - tail_ptr + head_ptr;
return substream->runtime->buffer_size - tail_ptr + head_ptr;
Copy link
Member

@plbossart plbossart Jan 19, 2024

Choose a reason for hiding this comment

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

This begs the question "how is the boundary taken into account"?

This looks like a controversial change without much justification. That is unlikely to be accepted upstream....

Copy link
Author

@RanderWang RanderWang Jan 22, 2024

Choose a reason for hiding this comment

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

boundary is used for app_ptr and hw_ptr wrapping. Now we use snd_sof_pcm_platform_pointer which is wrapped with runtime->buffer_size, so we need to wrap be dai link position with runtime->buffer_size so that we can compare the snd_sof_pcm_platform_pointer result with be dai link position

Copy link
Author

@RanderWang RanderWang Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The size of boundary is always multiple of the size of period in intermediate buffer for user space,up to UINT_MAX. It is not fit for us now. Please check the dump of hw:0,0 on my device

Plug PCM: Hardware PCM card 0 'sof-nocodec' device 0 subdevice 0
Its setup is:
  stream       : PLAYBACK
  access       : RW_INTERLEAVED
  format       : S16_LE
  subformat    : STD
  channels     : 2
  rate         : 48000
  exact rate   : 48000 (48000/1)
  msbits       : 16
  buffer_size  : 16384
  period_size  : 4096
  period_time  : 85333
 ......
  boundary     : 4611686018427387904 // 4000000000000000₁₆
  appl_ptr     : 0
  hw_ptr       : 0


return head_ptr - tail_ptr;
}
Expand Down