ASoC: SOF: ipc4-topology/Intel: hda-pcm: Fix ChainDMA host buffer time and move constraint to period time #5540
Conversation
ujfalusi
requested review from
bardliao,
dbaluta,
kv2019i,
lgirdwood,
lyakh,
plbossart and
ranj063
as code owners
ujfalusi
changed the title
ujfalusi
force-pushed
the
peter/sof/pr/host-buffer-constraint-fixes
branch
from
29dc68c to
99b779c
Compare
|
Changes since v1:
|
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Looks good and will improve compatibility with ALSA apps. Added a note on possible way to go even further with this line of thought...
ujfalusi
force-pushed
the
peter/sof/pr/host-buffer-constraint-fixes
branch
from
99b779c to
13829cc
Compare
|
Changes since v2:
|
ujfalusi
force-pushed
the
peter/sof/pr/host-buffer-constraint-fixes
branch
from
13829cc to
615bcab
Compare
|
Changes since v3:
This is needed as FW reads faster after the initial burst for few internal periods, which can lead to the hw_ptr moving to the next period, which might not be prepared by the application - causing xrun. The minimum period size is proposed to be used as headroom for PW to avoid xrun loops: https://gitlab.freedesktop.org/pipewire/wireplumber/-/merge_requests/740#note_3108033 |
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
This will fix for now - we can find better HW specific solution for future.
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Patches look great, I think this is the right approach. Some minor nits as inline comments.
|
|
||
| #define SOF_IPC4_DMA_DEVICE_MAX_COUNT 16 | ||
|
|
||
| #define SOF_IPC4_CHAIN_DMA_NODE_ID 0x7fffffff |
There was a problem hiding this comment.
Minor nit in commit message: the firmware change was for a particular Intel platforms. But yeah, in practise it covers all platforms covered by this ipc4 kernel code at the moment.
There was a problem hiding this comment.
the fw change was for all IPC4 platforms (all Intel platforms which can run IPC4), it is fair to say that for IPC4 the host buffer size has been 4ms and not 2ms.
There was a problem hiding this comment.
Right, that is true, but it's just hard to verify this looking at the commit and then looking at the FW commit. Reader has to have outside information that Intel ACE variant happens to be the only IPC4 supporting platform and the various other platforms in SOF (with 2ms default) are never used in IPC4 mode.
There was a problem hiding this comment.
CAVS2.5 works (but not officially supported) with IPC4 and that also uses 4ms host buffer. At this time IPC4 as such uses 4ms host buffer on all supported platforms.
Yes, we will have support for this information to be queried from firmware and can get rid of this define, but as far as IPC4 concerns, we have been mistakenly assumed 2ms host buffer when in fact IPC4 is using 4ms.
sound/soc/sof/intel/hda-pcm.c
Outdated
| if (spcm->stream[direction].dsp_max_burst_size_in_ms) { | ||
| unsigned int period_time = spcm->stream[direction].dsp_max_burst_size_in_ms; | ||
|
|
||
| period_time += min(period_time, 10); |
There was a problem hiding this comment.
Should this 10 be a define?
There was a problem hiding this comment.
I have hard time to come up with a define name, should a comment be sufficient?
/*
* add maximum of 10ms headroom over the maximum burst size to
* cover the time needed for the DMA pace to settle
*/
ujfalusi
force-pushed
the
peter/sof/pr/host-buffer-constraint-fixes
branch
from
615bcab to
b027e86
Compare
|
Changes since v4:
|
The firmware has changed the minimum host buffer size from 2 periods to 4 periods (1 period is 1ms) which was missed by the kernel side. Adjust the SOF_IPC4_MIN_DMA_BUFFER_SIZE to 4 ms to align with firmware. Link: thesofproject/sof@f0a14a3 Fixes: a2db533 ("ASoC: SOF: ipc4-topology: Do not parse the DMA_BUFFER_SIZE token") Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…size For ChainDMA the firmware allocates 5ms host buffer instead of the standard 4ms which should be taken into account when setting the constraint on the buffer size. Fixes: 72af064 ("ASoC: SOF: ipc4-topology: Save the DMA maximum burst size for PCMs") Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…d of buffer time Instead of constraining the ALSA buffer time to be double of the firmware host buffer size, it is better to set it for the period time. This will implicitly constrain the buffer time to a safe value (num_periods is at least 2) and prohibits applications to set smaller period size than what will be covered by the initial DMA burst. Fixes: 02ea2a0 ("ASoC: SOF: Intel: hda-pcm: Use dsp_max_burst_size_in_ms to place constraint") Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
|
|
||
| #define SOF_IPC4_DMA_DEVICE_MAX_COUNT 16 | ||
|
|
||
| #define SOF_IPC4_CHAIN_DMA_NODE_ID 0x7fffffff |
There was a problem hiding this comment.
Right, that is true, but it's just hard to verify this looking at the commit and then looking at the FW commit. Reader has to have outside information that Intel ACE variant happens to be the only IPC4 supporting platform and the various other platforms in SOF (with 2ms default) are never used in IPC4 mode.
5 participants
Hi,
ChainDMA PCM use 5ms host buffer by default and not 2ms, this needs to be handled correctly to be able to place correct constraint in platform code.
We should also move the constraint from buffer time to period time - which will implicitly applies the same constraint on the buffer size to make sure that applications will not use smaller periods than the initial DMA burst.