Added sof-ptl-cs42l43-agg-l3-cs35l56-l2-dax.tplg for PTL platform #10411
Conversation
checkupup
changed the title
|
Can one of the admins verify this patch?
|
|
I guess you could un-draft this PR to start the review then |
checkupup
force-pushed
the
dax_ptl_tplg
branch
from
541ac9b to
9d694de
Compare
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Thanks @checkupup , looks good! Some comments inline, but nothing really that would block this PR.
|
|
||
| IncludeByKey.SDW_SPK_ENHANCED_PLAYBACK { | ||
| "true" { | ||
| IncludeByKey.USE_DAX { |
There was a problem hiding this comment.
The indentation is a bit off here, but it seems there are indentation inconsistencies in this file already before, so no reason to block this PR. Somebody needs to clean this up and can be done in a separate PR.
@ranj063 @jsarha @bardliao @singalsu The "IncludeByKey" method is proving to be very useful, but we need to start thinking about how to scale this. Having four or more layers of nested conditional branches in the same alsaconf file, is getting out of hand. Not an issue for this PR, but something to put in our backlog for improvement.
There was a problem hiding this comment.
I very much agree in general, maybe a little in this case too. Our nocodec topology is very good, or actually bad, example of this. It has two topologies written into one, the passthrough, and the one with all the bells and whistles we could not think a better place to put them in for testing. I tried to separate those two toplogies some time back to make it more manageable, but failed. It should be a straight forward job, but the nested IncludeByKey section with intersecting variables made it too big a task to accomplish in a day or two. We should avoid ending up in the similar situation with other topologies.
Added sof-ptl-cs42l43-agg-l3-cs35l56-l2-dax.tplg for PTL platform. Signed-off-by: Jun Lai <jun.lai@dolby.com>
checkupup
force-pushed
the
dax_ptl_tplg
branch
from
9d694de to
a9a9e60
Compare
checkupup
left a comment
checkupup
left a comment
There was a problem hiding this comment.
Seems like this PR is ready for merging
app/overlays/ptl/dax_overlay.conf
Outdated
| # LLEXT | ||
| CONFIG_LLEXT_HEAP_SIZE=32 | ||
|
|
||
| # For sof-ptl-cs42l43-agg-l3-cs35l56-l2-dax.tplg |
There was a problem hiding this comment.
Hello @johnylin76, the following modules are required for sof-ptl-cs42l43-agg-l3-cs35l56-l2-dax.tplg, but there is currently no commit that actively includes them. However, other standalone modules should not be introduced in dax_overlay.conf. Is there a suitable location to introduce the following modules to avoid missing dependencies when generating sof-ptl-cs42l43-agg-l3-cs35l56-l2-dax.tplg?
There was a problem hiding this comment.
I have removed this modules in dax_overlay.conf.
| # PREPROCESS_PLUGINS=nhlt,NHLT_BIN=nhlt-sof-ptl-cs42l43-agg-l3-cs35l56-l2-4ch.bin,\ | ||
| # HDMI1_ID=7,HDMI2_ID=8,HDMI3_ID=9,DEEPBUFFER_FW_DMA_MS=10,DEEP_BUF_SPK=true,\ | ||
| # BT_PCM_ID=20,BT_ID=10,BT_PCM_NAME=Bluetooth,ADD_BT=true,USE_DAX=true,DOLBY_DAX_CORE_ID=0" | ||
|
|
There was a problem hiding this comment.
Hi @johnylin76 , I'm not inclined to keep this change because BT_ID changed from 10 to 8, and BT_ID=8 doesn't work on my device. I have no way of knowing about these physical connection changes. Can I just remove this modification outright?
There was a problem hiding this comment.
It's OK to keep this tplg in main branch.
There was a problem hiding this comment.
You cannot use BT_ID=8 because you already set HDMI2_ID=8 in your config.
As this tplg build is based on sof-ptl-cs42l43-agg-l3-cs35l56-l2 (line111), you should align all configs with that except for DAX-related ones, i.e. +USE_DAX=true,DOLBY_DAX_CORE_ID=0
@johnylin76 if your testing on your end we can wait or we can merge and fix any issue incrementally ? |
| # PREPROCESS_PLUGINS=nhlt,NHLT_BIN=nhlt-sof-ptl-cs42l43-agg-l3-cs35l56-l2-4ch.bin,\ | ||
| # HDMI1_ID=7,HDMI2_ID=8,HDMI3_ID=9,DEEPBUFFER_FW_DMA_MS=10,DEEP_BUF_SPK=true,\ | ||
| # BT_PCM_ID=20,BT_ID=10,BT_PCM_NAME=Bluetooth,ADD_BT=true,USE_DAX=true,DOLBY_DAX_CORE_ID=0" | ||
|
|
There was a problem hiding this comment.
You cannot use BT_ID=8 because you already set HDMI2_ID=8 in your config.
As this tplg build is based on sof-ptl-cs42l43-agg-l3-cs35l56-l2 (line111), you should align all configs with that except for DAX-related ones, i.e. +USE_DAX=true,DOLBY_DAX_CORE_ID=0
| ] | ||
|
|
||
| IncludeByKey.SDW_SPK_ENHANCED_PLAYBACK { | ||
| "true" { |
There was a problem hiding this comment.
nit: I feel that USE_DAX should not be conditioned under SDW_SPK_ENHANCED_PLAYBACK. It would be better if we use switch-case structure rather than nested if. e.g.
IncludeByKey.SDW_SPK_ENHANCED_PROC {
"DRC" {
...
}
"DAX" {
...
}
}
6 participants
Added sof-ptl-cs42l43-agg-l3-cs35l56-l2-dax.tplg for PTL platform.