bugfix in DP/Src: some audio formats have incorrectly estimate period #9927
Conversation
marcinszkudlinski
requested review from
abonislawski,
jxstelter,
ranj063 and
singalsu
as code owners
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
@marcinszkudlinski fwiw, @jsarha is working on adding a test DP topology that can be used in PR testing CI. Hopefully this will land soon to help test more DP use cases.
| unsigned int sink_period = | ||
| (uint64_t)1000000 * (uint64_t)sink_get_min_free_space(mod->sinks[i]) / | ||
| ((uint64_t)sink_get_frame_bytes(mod->sinks[i]) * | ||
| (uint64_t)sink_get_rate(mod->sinks[i])); |
There was a problem hiding this comment.
Nit: In multiplication it's enough to type cast the first operand to int64_t to calculate the product as 64 bit. It would give a bit more compact code.
| * use 64bit integers to avoid overflows | ||
| */ | ||
| unsigned int sink_period = | ||
| (uint64_t)1000000 * (uint64_t)sink_get_min_free_space(mod->sinks[i]) / |
There was a problem hiding this comment.
I think you can use 1000000ULL
src/audio/src/src_ipc4.c
Outdated
| */ | ||
| dev->period = 1000000 * sink_get_min_free_space(sink) / | ||
| (sink_get_frame_bytes(sink) * sink_get_rate(sink)); | ||
| dev->period = (uint64_t)1000000 * (uint64_t)sink_get_min_free_space(sink) / |
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
No blockers, but some question/notes, please see inline.
| module_adapter_calculate_dp_period(dev); | ||
|
|
||
| if (dev->period < LL_TIMER_PERIOD_US) { | ||
| comp_err(dev, "DP Module period too short (%u uS), must be at least 1LL cycle (%llu uS)", |
There was a problem hiding this comment.
Btw, why "uS" micro siemens? https://en.wikipedia.org/wiki/Siemens_(unit)
|
|
||
| comp_info(dev, "SRC DP period calculated as: %u", dev->period); | ||
| comp_info(dev, "SRC DP period calculated as: %u", dev->period); | ||
| } |
There was a problem hiding this comment.
Could this be a helper or otherwise shared code? Looks wrong to have DP constraints in multiple places in code. I.e. if the period needs to be aligned because of SRC specific reasons, it's good to have here, but if it's a limitation of DP infra, then it should be a common function used by all DP modules.
There was a problem hiding this comment.
unfortunately SRC module needs a calculated period for internal purposes at "set params". It happens a bit earlier than "prepare", when module adapter do a "general" period calculation
So, in case of current SRC implementation, it cannot be unified.
in general, if a module wants to enforce its own period instead of a calculated one, it can do it just by setting dev->period to desired value. SRC does it - but for different reasons
There was a problem hiding this comment.
Lets make the automatic DP config calculation the default state, but allow the modules to override with some API calls as needed. This should all be visible in the logs for debug.
There was a problem hiding this comment.
@singalsu fyi - some SRC updates may be needed for DP.
There was a problem hiding this comment.
Maybe calculating some things in SRC can be moved to prepare() if need. SRC needs a target period duration and it tries to match than with polyphase filter bank(s) constraints. In some cases with 44.1 family <--> 48 family rates those can't be met exactly.
As DP EDF scheduler runs in LL cycles, DP period should be always calculated at granularity of LL cycle Fail preparation if a module calculated period shorter than 1LL cycle Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
for some huge bitrates there was an overflow in 32bit numbers in period calculations, i.e: dev->period = 1000000 * sink_get_min_free_space(sink) for 192000-32-7 format it was 5824000000, more than maxint32bit Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>
marcinszkudlinski
force-pushed
the
dp_period
branch
from
74dc9fc to
a82618b
Compare
singalsu
left a comment
singalsu
left a comment
There was a problem hiding this comment.
Looks good to me, good catch with that dev->period calculate overflow!
|
|
||
| comp_info(dev, "SRC DP period calculated as: %u", dev->period); | ||
| comp_info(dev, "SRC DP period calculated as: %u", dev->period); | ||
| } |
There was a problem hiding this comment.
Maybe calculating some things in SRC can be moved to prepare() if need. SRC needs a target period duration and it tries to match than with polyphase filter bank(s) constraints. In some cases with 44.1 family <--> 48 family rates those can't be met exactly.
5 participants
For DP there's a crucial part to properly estimate period that the module will be called. Period value is used in several places in DP scheduling
There were 3 defects:
period should always be a multiplication of LL cycle, currently 1000us. If estimated period is i.e. 1300us, it should be aligned down to 1000
periods lower that 1LL cycle is not allowed, DP scheduler is triggered from LL
for some enormous high bitrates, like 192KHz/8 channels, there was an 32bit overflow in period calculations