-
Notifications
You must be signed in to change notification settings - Fork 349
topology2: ace3: Add Deep-buffer to ptl-rt721-4ch target #10221
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: ace3: Add Deep-buffer to ptl-rt721-4ch target #10221
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.
Pull Request Overview
This PR adds deep-buffer and enhanced audio features to the ptl-rt721-4ch target configuration for the ACE3 platform. The changes enable advanced DMIC capture processing, Bluetooth PCM support, and deep buffer speaker processing with optimized DMA buffer settings.
- Enhanced DMIC0 capture with TDFB (Time Domain Fixed Beamformer) and DRC (Dynamic Range Control) processing
- Added Bluetooth PCM support with dedicated IDs and naming
- Enabled deep buffer speaker processing with 10ms DMA buffer configuration
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/topology/topology2/production/tplg-targets-ace3.cmake | Adds configuration parameters for enhanced DMIC capture, Bluetooth PCM, and deep buffer settings |
| tools/topology/topology2/cavs-sdw.conf | Defines additional deep buffer pipeline constants for secondary audio path |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Add support to extend ptl-rt721-4ch conf with audio processing: - Enhanced DMIC0 capture with TDFB & DRC processing - Enable deep buffer speaker processing with 10ms DMA buffer - Configure TDFB line4_pass and DRC dmic_default parameters Signed-off-by: Uday M Bhat <uday.m.bhat@intel.com> Signed-off-by: Naveen Manohar <naveen.m@intel.com>
5e525d0 to
a8c420b
Compare
kv2019i
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.
Ok with change. PLease update the commit message on the second patch.
a8c420b to
5b5dac8
Compare
ujfalusi
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.
@naveen-manohar, I'm not sure how this play with the existing DB definition for SDW speaker, something is not quite right if we need to make hoops.
Also to note: how this will work with the function topologies? FYI: @bardliao
| DEEP_BUFFER_PCM_ID_2 35 | ||
| DEEP_BUFFER_PIPELINE_SRC_2 'mixin.16.1' | ||
| DEEP_BUFFER_PIPELINE_SINK_2 'mixout.21.1' | ||
| DEEP_BUFFER_PCM_NAME_2 'Deepbuffer Amps' |
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 PCM name shall be "Deepbuffer Speaker"
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 we have now double definition of this DB for Speaker and yes, I realize that the existing naming also uses "Deepbuffer Amps", which is incorrect, it should be really Speaker. We have Amp(lifier) on headset as well..
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.
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 agree the "Deepbuffer Amps" name is bad, but it's already in use, so not sure if we can change without breaking existing UCM files.
@ujfalusi Where is the double definition?
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.
Yes, it is, because the Headset playback is named "Jack Out" :
cat /proc/asound/pcm
00-00: Jack Out ( * ) : : playback 1
00-01: Jack In ( * ) : : capture 1
00-02: Speaker ( * ) : : playback 1
00-04: Microphone ( * ) : : capture 1
00-05: HDMI1 ( * ) : : playback 1
00-06: HDMI2 ( * ) : : playback 1
00-07: HDMI3 ( * ) : : playback 1
00-31: Deepbuffer Jack Out ( * ) : : playback 1
Similarly, for "Speaker" we should have "Deepbuffer Speaker" for consistency
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.
Ok let me check the "Deepbuffer Speaker" change
Further as discussed, there is another issue noticed with just this patch naveen-manohar@cde8889 Deep-buffer should have worked, but it dint add DEEP_BUFFER_PIPELINE_ID_2 16 DEEP_BUFFER_PCM_ID_2 35 from https://github.com/thesofproject/sof/commit/5359a7f6743e0ea3d878f466c3b063169bcebc7a#diff-fc6601cfd…
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.
@ujfalusi @naveen-manohar how about something like #10226 ?
| PREPROCESS_PLUGINS=nhlt,NHLT_BIN=nhlt-sof-ptl-rt721-4ch.bin" | ||
| PREPROCESS_PLUGINS=nhlt,NHLT_BIN=nhlt-sof-ptl-rt721-4ch.bin,DMIC0_ENHANCED_CAPTURE=true,\ | ||
| EFX_DMIC0_TDFB_PARAMS=line4_pass,EFX_DMIC0_DRC_PARAMS=dmic_default,\ | ||
| DEEPBUFFER_FW_DMA_MS=10,DEEP_BUF_SPK=true" |
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'm not sure how this supposed to work, but the DEEP_BUF_SPK=true will enable the existing definition from tools/topology/topology2/platform/intel/deep-buffer.conf
# Spawn another instance
IncludeByKey.DEEP_BUF_SPK {
"true" {
Define {
DEEP_BUFFER_PIPELINE_ID_2 16
DEEP_BUFFER_PCM_ID_2 35
DEEP_BUFFER_PIPELINE_SRC_2 'mixin.16.1'
DEEP_BUFFER_PIPELINE_SINK_2 'mixout.21.1'
DEEP_BUFFER_PCM_NAME_2 'Deepbuffer Amps'
SPEAKER_PCM_CORE_ID 0
}
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.
Now, I am wondering is it necessary to add those defines to cavs-sdw.conf?
kv2019i
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.
Commit message is good now. For @ujfalusi and @lgirdwood , the naming conventions for speaker deep buffer have been use already in I2S config (that share definitions via intel/deep-buffer.conf). I guess the UCM files will be new for SDW machines, so we have a chance to change some (e.g. maybe the PCM name), but needs modifications to UCMs.
Otherwise, I'd like @bardliao to double-check how this plays together with functional topologies. I'm assuming it works as this is just another DSP-driven feature added and does not affect the definitions towards the DAIs. But please check.
| DEEP_BUFFER_PCM_ID_2 35 | ||
| DEEP_BUFFER_PIPELINE_SRC_2 'mixin.16.1' | ||
| DEEP_BUFFER_PIPELINE_SINK_2 'mixout.21.1' | ||
| DEEP_BUFFER_PCM_NAME_2 'Deepbuffer Amps' |
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 agree the "Deepbuffer Amps" name is bad, but it's already in use, so not sure if we can change without breaking existing UCM files.
@ujfalusi Where is the double definition?
5b5dac8 to
c335f13
Compare
The change is specific to sof-ptl-rt721-4ch.tplg. I don't think it will impact the function topologies. On the other hands, the change will not take effect if function topology is used. We need to remove |
So, only machines which uses rt721 and 4ch PCH-DMIC will have Deepbuffer on Speaker and all other sound cards will not? Is it granted that this configuration will only be available w/ Chrome and we will not have different PCM list on sof-soundwire cards? What I'm saying is that we should enable the "Deepbuffer Speaker" for all and every soundwire cards, not only trying to target a specific one and shot more in the process. |

topology2: ace3: Add Deep-buffer & enhanced audio features to ptl-rt721-4ch target
Verified on PTL & WCL
cc: uday.m.bhat@intel.com