ASoC: SOF: ipc4-topology: Change DeepBuffer from static to dynamic mode#5673
ASoC: SOF: ipc4-topology: Change DeepBuffer from static to dynamic mode#5673ujfalusi wants to merge 2 commits intothesofproject:topic/sof-devfrom
Conversation
ujfalusi
commented
Feb 19, 2026
|
The position and delay reporting is not affected by the change, everything works as it used to work. |
With Dynamic DeepBuffer the host DMA buffer size is calculated based on the requested ALSA period size (with a headroom between the two) using the DEEP_BUFFER token as a maximum limit for the host DMA buffer. This way applications can use the same DeepBuffer enabled PCM for different use cases and still benefit of the power saving of a bigger host DMA Kernel has been updated thesofproject/linux#5673 Signed-off-by: Uday M Bhat <uday.m.bhat@intel.com>
sound/soc/sof/ipc4-topology.c
Outdated
| if (deep_buffer_dma_ms) | ||
| dev_dbg(scomp->dev, | ||
| "%s, dma buffer%s: %u ms (max: %u) / %u bytes, ALSA period: %u / %u\n", | ||
| swidget->widget->name, deep_buffer_dma_ms ? " (using Deep Buffer)" : "", |
There was a problem hiding this comment.
you already know, that deep_buffer_dma_ms != 0
There was a problem hiding this comment.
yes, just occurred to me as well ;)
but the string is already too long :(
sound/soc/sof/intel/hda-pcm.c
Outdated
| SNDRV_PCM_HW_PARAM_PERIOD_TIME, | ||
| period_time * USEC_PER_MSEC, | ||
| spcm->stream[direction].dsp_min_burst_size_in_ms * USEC_PER_MSEC, | ||
| UINT_MAX); |
There was a problem hiding this comment.
just to double check - this is correct - using a "min" value to set a "max" constraint?
There was a problem hiding this comment.
using the min value to set the min constraint, max is UINT_MAX, This is why I'm renaming the variable.
97366b1 to
d53f6ea
Compare
|
Changes since v1:
|
With Dynamic DeepBuffer the host DMA buffer size is calculated based on the requested ALSA period size (with a headroom between the two) using the DEEP_BUFFER token as a maximum limit for the host DMA buffer. This way applications can use the same DeepBuffer enabled PCM for different use cases and still benefit of the power saving of a bigger host DMA Kernel has been updated thesofproject/linux#5673 Signed-off-by: Uday M Bhat <uday.m.bhat@intel.com>
With Dynamic DeepBuffer the host DMA buffer size is calculated based on the requested ALSA period size (with a headroom between the two) using the DEEP_BUFFER token as a maximum limit for the host DMA buffer. This way applications can use the same DeepBuffer enabled PCM for different use cases and still benefit of the power saving of a bigger host DMA Kernel has been updated thesofproject/linux#5673 Signed-off-by: Uday M Bhat <uday.m.bhat@intel.com>
d53f6ea to
c4b7bfb
Compare
|
Changes since v2:
|
ranj063
left a comment
There was a problem hiding this comment.
LGTM with some minor nits about a couple of typos
sound/soc/sof/ipc4-topology.c
Outdated
| * Add headroom (the minimum size of the host DMA buffer) between host | ||
| * DMA buffer and ALSA period size if the ALSA period is less than | ||
| * SOF_IPC4_NO_DMA_BUFFER_HEADROOM_MS, otherwise equal the host DMA | ||
| * buffer to ALSA period size, caped at the maximum DeepBuffer depth |
sound/soc/sof/ipc4-topology.h
Outdated
|
|
||
| /* | ||
| * When the host DMA buffer size is larger than 8ms, the firmware switches from | ||
| * a constant fill mode to burst mode, keeping a 4ms threashold to trigger a |
With Dynamic DeepBuffer the host DMA buffer size is calculated based on the requested ALSA period size (with a headroom between the two) using the DEEP_BUFFER token as a maximum limit for the host DMA buffer. This way applications can use the same DeepBuffer enabled PCM for different use cases and still benefit of the power saving of a bigger host DMA Kernel has been updated thesofproject/linux#5673 Signed-off-by: Uday M Bhat <uday.m.bhat@intel.com>
With Dynamic DeepBuffer the host DMA buffer size is calculated based on the requested ALSA period size (with a headroom between the two) using the DEEP_BUFFER token as a maximum limit for the host DMA buffer. This way applications can use the same DeepBuffer enabled PCM for different use cases and still benefit of the power saving of a bigger host DMA Kernel has been updated thesofproject/linux#5673 Signed-off-by: Uday M Bhat <uday.m.bhat@intel.com> (cherry picked from commit 9ce8671)
c4b7bfb to
f1b4091
Compare
|
Changes since v3:
|
With Dynamic DeepBuffer the host DMA buffer size is calculated based on the requested ALSA period size (with a headroom between the two) using the DEEP_BUFFER token as a maximum limit for the host DMA buffer. This way applications can use the same DeepBuffer enabled PCM for different use cases and still benefit of the power saving of a bigger host DMA Kernel has been updated thesofproject/linux#5673 Signed-off-by: Uday M Bhat <uday.m.bhat@intel.com> (cherry picked from commit 9ce8671)
kv2019i
left a comment
There was a problem hiding this comment.
Code itself looks good (and this is a great improvement), but I have some issues with the naming and commentary. The calculations done are somewhat hairy to follow and I think it's important this is made as easy to follow as possible as potentially a lot of people really have to figure out the logic. I admit most (if not all) of the problematic naming predates this PR, but given you do more math on the numbers, the confusion comes more problematic thatn before. Please take a look at comments.
sound/soc/sof/ipc4-topology.c
Outdated
| if (swidget->id == snd_soc_dapm_aif_in) | ||
| host_period_bytes = copier_data->base_config.ibs; | ||
| else | ||
| host_period_bytes = copier_data->base_config.obs; |
There was a problem hiding this comment.
I find the "host_period_bytes" variable name quite confusing. The ALSA host side "period" is after all different than DSP's internal "period" (which is usually 1ms in wall clock time and not direclty tied to ALSA period size). I think to understand this, it would be good to add a comment on the assumption made here that 'copier ibs/obs is set match one "DSP period" of data'. The correct variable would then be "fw_period_bytes".
There was a problem hiding this comment.
I'm really-really-really bad at this naming thing...
sound/soc/sof/ipc4-topology.c
Outdated
| host_period_bytes = copier_data->base_config.obs; | ||
|
|
||
| min_size = SOF_IPC4_MIN_DMA_BUFFER_SIZE * host_period_bytes; | ||
| no_headroom_mark = SOF_IPC4_NO_DMA_BUFFER_HEADROOM_MS * host_period_bytes; |
There was a problem hiding this comment.
The confusion has been in existing code, but now that you do more calculations, it is getting harder to ignore. Both SOF_IPC4_MIN_DMA_BUFFER_SIZE and SOF_IPC4_NO_DMA_BUFFER_HEADROOM_MS assume the DSP LL tick is 1ms. To be pedantic, these should be "bytes per DSP-period", so when you multiply with host_period_bytes/fw_period_bytes, the math makes sense.
There was a problem hiding this comment.
The calculation assumes that a tick is fw_period_bytes long in bytes and yes, the _MS is an indication that a tick is 1ms long.
I think this is a standing assumption even if the DMA might move more data at once (when bursting).
My biggest issue is with the BUFFER_SIZE name. It comes from one of the messages which defines the "buffer size" in ms and not in bytes, afaik.
A comment to explain this?
| deep_buffer_dma_ms ? deep_buffer_dma_ms : SOF_IPC4_MIN_DMA_BUFFER_SIZE, | ||
| buffer_bytes, fe_period_bytes / host_period_bytes, fe_period_bytes); | ||
|
|
||
| copier_data->gtw_cfg.dma_buffer_size = buffer_bytes; |
There was a problem hiding this comment.
I also wonder about using "host buffer size" (e.g. in sof_ipc4_set_host_dma_buffer_size() function name here. In DSP world, we use "host DMA" and "DAI DMA" terms universally to distinguish when we are talking about data transfer towards Linux host, or when it's about DMAs used to transfer data to audio codec interfaces.
But this is in context of Linux kernel. Here we have two buffers, one in host DDR (the ALSA buffer DMA buffer) and one in DSP SRAM. What this function calculates is the size of the DSP SRAM buffer, and I'd argue calling this the "host buffer" in this context, is hard to follow.
Not a blocker. If this is used in enough places elsewhere in the driver, the damage is already done and we can just move on, but please consider if I'm alone with my confusion, or could naming be made easier to follow.
There was a problem hiding this comment.
I see, but I will run out of columns on a 4k monitor if I go and exact-word name variables...
fw_host_dma_buffer_size, it is not a fw_dma_buffer as DAI side also have DMA buffer.
So I settled in the middle: ALSA buffer vs host DMA buffer.
as for the copier_data->gtw_cfg.dma_buffer_size, it is a message to the fw to configure the copier with that size of buffer.
But changing the function to sof_ipc4_set_fw_host_dma_buffer_size is a bit too much, it sets the dma_buffer_size for the host copier.
Any suggestions?
Currently the DeepBuffer results static host DMA buffer and thus static minimum ALSA period size which can be a limiting factor for user space. DeepBuffer of static 100ms host DMA buffer can only be opened with at least 110ms ALSA period size, if for the same endpoint there is a need for smaller (or larger) buffer then a new PCM device must be created with different DeepBuffer configuration. This does not scale in real life. With Dynamic DeepBuffer the host DMA buffer size is calculated based on the requested ALSA period size using the DEEP_BUFFER token as a maximum limit for the host DMA buffer. This way applications can use the same DeepBuffer enabled PCM for different use cases and still benefit of the power saving of a bigger host DMA buffer. As an example, the DEEP_BUFFER in topology is set to 100ms (interpreted as maximum size with this patch): ALSA period: 8 -> dma buffer: 4 ms ALSA period: 10 -> dma buffer: 6 ms ALSA period: 16 -> dma buffer: 12 ms ALSA period: 19 -> dma buffer: 15 ms ALSA period: 20 -> dma buffer: 20 ms ALSA period: 50 -> dma buffer: 50 ms ALSA period: 100 -> dma buffer: 100 ms ALSA period: 150 -> dma buffer: 100 ms ALSA period: 2000 -> dma buffer: 100 ms The Dynamic DeepBuffer will give applications the means to choose between lower latency (small host DMA buffer) or higher power save (big host DMA buffer) with higher latency on the same device with topology providing a meaningful upper limit of the buffer size. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…st_size_in_ms The meaning of the variable has changed with he Dynamic DeepBuffer and it reflects the smallest burst that the host DMA does. This can be used to set the minimum period time constraint to avoid overshooting by the DMA burst. Change the name and update the related code in Intel hda_dsp_pcm_open() to reflect the revised meaning. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
f1b4091 to
373fd82
Compare
|
Changes since v4:
|