-
Notifications
You must be signed in to change notification settings - Fork 349
audio: codec_adapter: add DAX audio processing module #10241
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
Conversation
checkupup
commented
Sep 16, 2025
- Add DAX audio processing module
- Add topology support for MTL platform
- Add testbench topology support
|
Can one of the admins verify this patch?
|
|
@blin-dolby Hello, bin, This commit is waiting for CI checking. Is there any suggestion for reviewers of SOF? |
|
Let me check with Google team. |
singalsu
left a comment
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.
Looks good. I added some non-critical suggestions, and maybe spit this to multiple commits per topic.
If you'd like SOF CI to regularly test this component with testbench run, add run command to https://github.com/thesofproject/sof/blob/main/scripts/host-testbench.sh.
tools/topology/topology2/include/bench/dolby-dax_hda_route.conf
Outdated
Show resolved
Hide resolved
|
I added @andyross and myself as reviewers from Google since this will support Google projects. |
johnylin76
left a comment
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.
if commit "audio: codec_adapter: update code based on review results" is to address comments, you should merge it into the original one.
|
@checkupup @blin-dolby @johnylin76 I'm going to fork stable v2.14 tomorrow, but I'm open to delaying release a little if we can get the review comments resolved and this PR merged soon. |
d5f9206 to
6b2fa98
Compare
6b2fa98 to
b0bc86d
Compare
|
test this please |
@checkupup just to avoid misunderstanding - this wasn't directed to you or any other human, this was directed to our CI |
* Add DAX audio processing module * Add topology support for MTL platform * Add testbench topology support Signed-off-by: Jun Lai <jun.lai@dolby.com>
b0bc86d to
07acece
Compare
lyakh
left a comment
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.
just some ideas maybe for a follow-up, not critical for the initial merge
| uint32_t free; /* Free bytes for writing */ | ||
| }; | ||
|
|
||
| struct sof_dax { |
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.
Not worth an update, but if you do for some reason have to update or maybe do this as an incremental patch - maybe you could consider using standard C types in this and other structures, which aren't parts of an ABI:
|
|
||
| struct sof_dax { | ||
| /* SOF module parameters */ | ||
| uint32_t sof_period_bytes; |
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.
this could become size_t?
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.
struct comp_dev *dev = mod->dev;
dax_ctx->sof_period_bytes = dev->frames *
dax_ctx->output_media_format.num_channels *
dax_ctx->output_media_format.bytes_per_sample;sof_period_bytes uses the same type as dev->frames, and the remaining uint32_t type also serves the same purpose.
| uint32_t sof_period_bytes; | ||
|
|
||
| /* DAX state parameters */ | ||
| uint32_t period_bytes; |
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.
and this?
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.
same reason as above, period_bytes keep same type with sof_period_bytes.
|
|
||
| /* DAX state parameters */ | ||
| uint32_t period_bytes; | ||
| uint32_t period_us; |
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.
this depends on how it's used, where you get it from, maybe unsigned long?
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.
from:
dax_ctx->period_us = 1000000 * dax_ctx->period_bytes /
(dax_ctx->output_media_format.bytes_per_sample *
dax_ctx->output_media_format.num_channels *
dax_ctx->output_media_format.sampling_rate);period_us uses same types as period_bytes.
| struct dax_media_fmt output_media_format; | ||
|
|
||
| /* DAX control parameters */ | ||
| int32_t enable; |
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.
if this is an on-off flag, then bool?
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.
Each *_enable variables in sof_dax is an on-off flag. The bool type may encount a compiation error when using rebuild_testbench.sh -p ${platform} based on xtensa toolchains, so I avoid using bool type for compatibility.
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.
@checkupup sorry, this is strange. SOF has bool in a lot of places, which compilations break with it? You might need to include a suitable header for it, but I'd be surprised if there was a build option currently that didn't come across a single bool use.
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.
Could we can pick up this after merging this request if it is not critical for this merge? It's been quite a while since I last encountered this issue. I will try to reproduce bool error later.
|
@johnylin76 hello, could you help to merge this request if it is ok? |
|
SOFCI TEST |
|
@checkupup This looks good to go now. Let me rerun the one set of CI jobs that seems to be stuck. We should get results today and can then merge. |