-
Notifications
You must be signed in to change notification settings - Fork 318
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
topology2: sof-mtl-rt5650: add SSP FMT 24 bits support #8917
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
34e7f94
topology2: cavs-mixin-mixout-ssp: fix the indentation
macchian 7ba72d8
topology2: cavs-mixin-mixout-eqiir-dts: fix the indentation
macchian eea15f0
topology2: remove USE_CHAIN_DMA flag as it's applied to iDisp links.
macchian 3f6cb04
topology2: fix nhlt binary file naming
macchian 7c1d274
topology2: sof-mtl-rt5650: add SSP FMT 24 bits support
macchian File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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, with MSB_INTEGER, the data sent to SSP will be basicly same as before, 32bit containers with just the lower 8 bits zeroed out. Not sure if this makes a different to the codec. I'm I missing something, does the codec DAI specification also need a change ("sample_bits 24"), so that we change what is sent over I2S:.?
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.
@kv2019i , codec dai no need to change the sample bits 24 bits. Because the valid bit 24 bits can be in 32 container. Thanks for pointing out, I think the SOF FW currently set LSB_INTEGER and it will use sample type to choose format conversion. It makes no difference to the codec with MSB_INTEGER. Remove it, any concerns?
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.
I guess we don't need to overwrite
out_fmt_cfg
if we don't set out_sample_type here right? @kv2019i @ranj063There 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.
Right, but I think the problem is what we want to achieve (see Pierre's comment above). While ALH (for which we have tplg examples for 24bit output) takes MSB-format 32bit values, SSP behaves differently. We don't have any examples for 24bit SSP in upstream, so this is not so obvious, but SSP hw requires 24bit in the lower bits, so if the valid bits is 24bits in the SSP configuration, then copier needs to be configured to SAMPLE_TYPE_LSB_INTEGER here, and SSP configuration needs to set to have a 24bit configuration.
DSP/Copier will send 32bit containers with audio in the lower 24bit, SSP will send the 24bit over SSP bus.
But yeah, @macchian , is above the correct configuration? The current cavs-rt5682.conf only defines 32bit SSP config. And if that is correct, then you don't need anything special in copier configuration. It outputs 32/32bit by default (MSB/LSB makes no differences if valid_bits==container_bits).
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.
@kv2019i ,
yes, correct! It's what we'd like to do. As the exact as you mentioned, we want to achieve valid 24 bits / 32 bits container support.
"DSP/Copier will send 32bit containers with audio in the lower 24bit, SSP will send the 24bit over SSP bus."
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.
@macchian Right, so then your patch is correct but the sample type needs to be "out_sample_type $SAMPLE_TYPE_LSB_INTEGER". This is very confusing, but hopefully having more examples will help. The reason for confusion stems from fact that historically the FW has "fixed up" some bad configurations. See e.g. commit 383d17a . So even if host/topology sets copier to input/output 24bit in a certain format, the FW has based on the link type (.e.g whether it's SSP/DMIC/ALH) overwritten the configuration and done the right thing. There are some legacy cases FW still needs to support, but we are trying to get rid of this in topology and have the correct formats in topology and not rely on the FW to fixup the values. Problem is that one must still enter some value in topology and having values in topology than in practise will be ignored by FW, is source for a lot of confusion. Hopefully this clarifies. So some short term pain to adapt, but in longer term this should be easier for both tplg and FW development.
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.
@kv2019i , yes, it's clear to me. Add in/out_sample_type $SAMPLE_TYPE_LSB_INTEGER for easier review by everyone.