Revert "ipc4: relax the IPC timeout checks and be nicer to other thre… #8756
Conversation
RanderWang
requested review from
lgirdwood,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
|
fw warning with reverted commit: |
…ads" When a stream is triggered to start, host kernel first sendis trigger start ipc message to fw and then start host dma for this stream. Ipc_wait_for_compound_msg is used to wait for all pipelines in the stream to be complete and need to be done fast since it blocks host to start hda dma. The reverted commit adds a 10 ms delay and results to host copier xrun warning for it can't get data from host dma. And this commit also makes a negative effect for stream_start_offset calculation which is for the difference between dai gateway and host gateway. We calculate it in host start function but host dma is started 10ms later, so there will be at least 10ms data error. This reverts commit 909a327. Signed-off-by: Rander Wang <rander.wang@intel.com>
RanderWang
force-pushed
the
pipeline_wait
branch
from
f029d92
to
c142b7d
Compare
There was a problem hiding this comment.
Thanks @RanderWang , this can help potentially in other cases as well. I don't think this was the original intent of the change, but this will delay any IPC that requires to be handled in pipeline context, by 10ms.
|
Note, the original patch has been referred to fix #7774 so we need to track test results carefully. |
|
|
||
| while (atomic_read(&msg_data.delayed_reply)) { | ||
| k_sleep(Z_TIMEOUT_MS(10)); | ||
| k_sleep(Z_TIMEOUT_US(250)); |
There was a problem hiding this comment.
btian1
250 --> 10ms? it is 40 times enlarge? may miss something during sleep? how about 1ms?
above is the comments I added when the change first submitted, my question is: how 250us * 30 = 7.5ms comes from?
is it based on experience value to make total wait time < 10ms?
There was a problem hiding this comment.
it is set by experiments with test and log. Most waiting time is less than 1ms. I don't want to use 1ms since it will miss one schedule period in FW.
There was a problem hiding this comment.
What context is this called from ? My expectation is the IPC thread and if so not sure how we can block LL from running ?
There was a problem hiding this comment.
@lgirdwood This is the IPC thread. It doesn't block the LL thread, but it will block processing of IPC messages until the LL timer fires next time, runs the IPC processing (that needs to happen in LL context), acks it as done, and then the IPC thread can continue. Now this adds a mandatory 10ms delay to each such handover from IPC thread to LL thread.
There was a problem hiding this comment.
ok, so we have a timing dependency between LL thread and IPC thread and this is convergence. This need to be spelled out in the inline comments alongside setting a better tries count as 30 means about 7.5 LL ticks. i.e. we need to say > 1.5 LL ticks would be a significant failure
There was a problem hiding this comment.
@kv2019i , @RanderWang is it better if 150 * 50? to make IPC response more quick?
There was a problem hiding this comment.
@lgirdwood This was the mechanism added by @lyakh to synchronize pipeline trigger processing with LL thread (so that pipeline state changes are not done async with the pipeline exection). @lgirdwood @btian1 We can improve the logic (or even devise a different way to wait for next ll tick, I think the original concern of trying to avoid unnecessary thread context switches is as valid as ever), but can we do that as a follow-up and merge this plain revert first? This helps with multiple pressing issues (AV sync and this seems to help with multicore scenarios), and we have done significant testing of this version with @RanderWang . If we want a modified new mechanism, that needs to be tested well, pushing the merge out.
There was a problem hiding this comment.
I'm fine with a revert, but we need a folloup PR that spells out the relationships with some inline commentary.
|
One fail in https://sof-ci.01.org/sofpr/PR8756/build1997/devicetest/index.html?model=MTLP_RVP_NOCODEC&testcase=multiple-pause-resume-50 -- no FW/drv errors in the logs. This looks like a case of #8770 . I'll proceed wit merge |
|
According to @keqiaozhang this revert causes the following regression: |
…ads"
When a stream is triggered to start, host kernel first sendis trigger start ipc message to fw and then start host dma for this stream. Ipc_wait_for_compound_msg is used to wait for all pipelines in the stream to be complete and need to be done fast since it blocks host to start hda dma. The reverted commit adds a 10 ms delay and results to host copier xrun warning for it can't get data from host dma. And this commit also makes a negative effect for stream_start_offset calculation, so revert it.
This reverts commit 909a327.
one fix for thesofproject/linux#4781