-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
dai-zephyr: check max block count supported by DMA #6823
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good, a few minor comments inline. I think this is at least better than having HDA specific code in dai-zephyr.c.
src/audio/dai-zephyr.c
Outdated
comp_info(dev, "dai_playback_params(): block count = %d not supported by DMA", period_count); | ||
buf_size = period_count * period_bytes; | ||
period_count = max_block_count; | ||
period_bytes = buf_size / period_count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add an assert for the case where this does not divide evenly? Should never happen of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, maybe there's some implicit rule that I'm not aware of, but by just looking at this, it might well not divide evenly. We enter here when the caller requested more periods than the DMA can handle. We then calculate a buffer size from that larger block count and divide it by the the DMA maximum block count. So if max was 16 and the caller requested 17 and the period size isn't a multiple of 16, this won't divide well. Or is this meant for cases where the requested period count is a multiple of the block count, fitting multiple periods in a single DMA block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only one real use case is limiting period_count to 1 for HDA DAI so more like as you said - fitting multiple period in a single DMA block, I thought we can make this more generic.
Division problem we can quickly solve with assert or ALIGN_DOWN but Im now not sure if this will result in a valid config, lets say 4x128 is still valid if requested 8x64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets not assert() here, just log() return an error up the stack to host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with alignment check and period count decreasing, please take a look if it's not over complicated
Re-running to clear https://sof-ci.01.org/sofpr/PR6823/build2944/boottest https://sof-ci.01.org/sofpr/PR6823/build2944/devicetest/index.html is all green! |
SOFCI TEST EDIT: https://sof-ci.01.org/sofpr/PR6823/build3199/boottest/index.html is now clear. |
b8bda47
to
a379898
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It would be good to update git commit message though.
Check max block count supported by DMA using new Zephyr API This will allow to prepare a correct config regardless of specific DMA capabilities Signed-off-by: Adrian Bonislawski <adrian.bonislawski@intel.com>
a379898
to
94e7541
Compare
Test results are good, including alsabat test now passing https://sof-ci.01.org/sofpr/PR6823/build3247/devicetest/index.html?model=TGLU_UP_HDA_IPC4ZPH&testcase=check-alsabat-headset-playback There are multiple checkpatch warnings from long log/trace lines, but I think this is ok as it is not recommended to split string literals across multiple lines. As this is a critical fix, proceeding with merge. |
check max block count supported by DMA to prepare a correct config for DMA
Fixes: #6709
OPEN: currently it is recalculating block count to max supported but it could also return error and recalculate to 1 only for HDA