dai: use ibs/obs for dma buffer size calculation #8983
Conversation
iganakov
requested review from
lgirdwood,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
src/audio/dai-zephyr.c
Outdated
| else | ||
| dma_buff_ms_size = dd->ipc_config.dma_buffer_size / copier_cfg->base.obs; | ||
|
|
||
| period_count = MAX(period_count, dma_buff_ms_size); |
There was a problem hiding this comment.
Not quite following the logic. Period size may not always be 1ms, so how come we mix "period_count" and "dma_buff_ms_size" here? Or is the assumption that copier "ibs" must be aligned to period size of the pipeline. This would be good to capture in a comment. The variable name still needs to be corrected, it needs to be "dma_buff_size_in_periods" or something like that.
There was a problem hiding this comment.
ibs/obs size depends on module processing domain, in case of Copier(LL) period will be 1ms. In Copier gtw config we receive DMA size aligned with ibs/obs. Based on previous assumptions we can calculate DMA buffer length in ms, which is also DMA period_count. I agree the name of the variable and comment should be changed.
iganakov
force-pushed
the
iganakov/dai_dma_buff_size
branch
from
95ea8f4 to
fdb4a42
Compare
src/audio/dai-zephyr.c
Outdated
| * should aggregate two streams 1ch/24_32bit/48kHz and 3ch/24_32bit/48kHz into one | ||
| * 4ch/24_32bit/48kHz stream, then FW will have to allocate two DMA's with sizes | ||
| * 1920 and 5760 bytes respectively and same 10 ms length (period_count) for both | ||
| * to avoid glitches. |
There was a problem hiding this comment.
this explanation is very very hard to follow. For playback we typically send the same data to different links. For capture, we typically have a symmetry between the number of channels on different links.
The explanation with 1 and 3 channels does not describe which scenario you are trying to address, and whether this is an actual use case in a product or a test configuration.
In addition, the aggregation is at the link level, and one would hope that we don't have a 10ms buffer? There's clearly a problem if we again have to rely on more buffers to get this solution to work properly...
There was a problem hiding this comment.
@plbossart there is no need for any additional buffers. I'm not going to change FW aggregation mechanism.
I found a bug in Windows production scenario (actual use case). When trying to aggregate two streams with different number of channels e.g. 1ch and 3ch (mono and 2.1) and 10ms DMA buffer requested from driver multiple glitches and first stream delay were observed. This change fixes that special case.
src/audio/dai-zephyr.c
Outdated
| else | ||
| dma_buff_length_ms = dd->ipc_config.dma_buffer_size / copier_cfg->base.obs; | ||
|
|
||
| period_count = MAX(period_count, dma_buff_length_ms); |
There was a problem hiding this comment.
Comparing period_count and milliseconds doesn't seem quite right. Maybe it works for the special case of a period being exactly one ms...
There was a problem hiding this comment.
@iganakov Can you modify the variable/comments here? This file is generic SOF code and the assumption of period to be 1ms only applies to current Intel IPC4 configurations. I.e. in the same function, we use "dev->frames" to calculate period size. It's very odd in the same function to assume a hardcoded period size later on.
I'd propose:
uint32_t dma_buff_length_periods;
/* copier ibs/obs is set to size of one period */
if (dev->direction == SOF_IPC_STREAM_CAPTURE)
dma_buff_length_periods = dd->ipc_config.dma_buffer_size / copier_cfg->base.ibs;
else
dma_buff_length_periods = dd->ipc_config.dma_buffer_size / copier_cfg->base.obs;
period_count = MAX(period_count, dma_buff_length_periods);
There was a problem hiding this comment.
It seems reasonable, thanks @kv2019i
src/audio/dai-zephyr.c
Outdated
| else | ||
| dma_buff_length_ms = dd->ipc_config.dma_buffer_size / copier_cfg->base.obs; | ||
|
|
||
| period_count = MAX(period_count, dma_buff_length_ms); |
There was a problem hiding this comment.
@iganakov Can you modify the variable/comments here? This file is generic SOF code and the assumption of period to be 1ms only applies to current Intel IPC4 configurations. I.e. in the same function, we use "dev->frames" to calculate period size. It's very odd in the same function to assume a hardcoded period size later on.
I'd propose:
uint32_t dma_buff_length_periods;
/* copier ibs/obs is set to size of one period */
if (dev->direction == SOF_IPC_STREAM_CAPTURE)
dma_buff_length_periods = dd->ipc_config.dma_buffer_size / copier_cfg->base.ibs;
else
dma_buff_length_periods = dd->ipc_config.dma_buffer_size / copier_cfg->base.obs;
period_count = MAX(period_count, dma_buff_length_periods);
Use ibs/obs size from ipc4_base_module_cfg to properly calculate period_count. It is especially important when FW aggregation mode is enabled and there are multiple DMAs allocated under one copier instance. That way period count for every DMA will be correctly evaluated and used for DMA buffer size calculation. Signed-off-by: Ievgen Ganakov <ievgen.ganakov@intel.com>
iganakov
force-pushed
the
iganakov/dai_dma_buff_size
branch
from
fdb4a42 to
8c40f41
Compare
Use ibs/obs size from ipc4_base_module_cfg to properly calculate period_count. It is especially important when FW aggregation mode is enabled and there are multiple DMAs allocated under one copier instance. That way period count for every DMA will be correctly evaluated and used for DMA buffer size calculation.
E.g. if
dma_buffer_sizereceived in Copier gtw config is 7680 bytes and FW should aggregate two streams 1ch/24_32bit/48kHz and 3ch/24_32bit/48kHz into one 4ch/24_32bit/48kHz stream, then FW will have to allocate two DMA's with sizes1920 and 5760 bytes respectively and same 10 ms period for both to avoid glitches.