-
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
topology2: cavs-sdw: Add Deep buffer PCM for sdw-amps #8879
Conversation
Add deep buffer PCM35 on sdw-amps for sof-lnl-rt722-l0 tplg. Signed-off-by: Naveen Manohar <[email protected]>
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.
@ranj063 @naveen-manohar should this be "Speaker" instead of "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.
@lgirdwood @ranj063: I noticed a uniformity throughout for 'Deepbuffer Amps'
development/mtl-sdw-hdmi.conf:83: DEEP_BUFFER_PCM_NAME_2 'Deepbuffer Amps'
development/cavs-sdw-hdmi.conf:83: DEEP_BUFFER_PCM_NAME_2 'Deepbuffer Amps'
development/cavs-sdw.conf:83: DEEP_BUFFER_PCM_NAME_2 'Deepbuffer Amps'
Please confirm if I need to modify to '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.
Good point - this may change UCM too, but the users today are all in "development" folder so are not used in production. @naveen-manohar I think we should change all to "Speaker" for clarity. @plbossart @ranj063 @singalsu anyone disagree ?
@@ -69,6 +69,11 @@ Define { | |||
DEEP_BUFFER_PIPELINE_SRC 'mixin.15.1' | |||
DEEP_BUFFER_PIPELINE_SINK 'mixout.1.1' | |||
DEEP_BUFFER_PCM_NAME 'Deepbuffer Jack Out' | |||
DEEP_BUFFER_PIPELINE_ID_2 16 | |||
DEEP_BUFFER_PCM_ID_2 35 |
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 can you remind me where this 35 comes from? I don't have access to my usual table of devices and this one does not ring a bell at all...
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.
Considered the next available PCM-ID similar to https://github.com/thesofproject/sof/blob/main/tools/topology/topology2/cavs-rt5682.conf#L80
arkham ~ # aplay -l
**** List of PLAYBACK Hardware Devices ****
card 0: sofsoundwire [sof-soundwire], device 0: Jack Out () []
Subdevices: 1/1
Subdevice #0: subdevice #0
card 0: sofsoundwire [sof-soundwire], device 2: Speaker () []
Subdevices: 1/1
Subdevice #0: subdevice #0
card 0: sofsoundwire [sof-soundwire], device 31: Deepbuffer Jack Out () []
Subdevices: 1/1
Subdevice #0: subdevice #0
card 0: sofsoundwire [sof-soundwire], device 35: Deepbuffer Amps () []
Subdevices: 1/1
Subdevice #0: subdevice #0
arkham ~ # aplay -Dhw:0,35 -c 2 -r 48000 -d 10 -f S16_LE /root/pubg.wav
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.
give me a week to look into this, I don't recall us ever agreeing on all 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.
After checking my internal cheat sheet, I can confirm that the device 35 shows as reserved.
PCM28 RESERVED
PCM29 RESERVED
PCM30 playback LowLatency
PCM31 playback DeepBuffer (for low-power)
PCM32 playback Media (e.g. movies)
PCM33 playback Voice (Communication with a human)
PCM34 playback Ultrasonics
PCM35 RESERVED
PCM36 RESERVED
PCM37 RESERVED
PCM38 RESERVED
PCM39 RESERVED
I don't mind reserving a Deep Buffer for amplifier endpoints, but note that we cannot keep adding in the PCM numbering both the content type and the endpoint. Are we e.g. going to have a LowLatency path for headphone AND speaker?
The "right" approach would be to keep the content-based stream generated by apps, and dynamically route to the current endpoint. What are the odds that we need a Deep buffer stream on the headphone and a Deep buffer stream on the amplifiers running concurrently?
That's what we had on the Merrifield and SKL drivers...The FE and BE were conditionally connected depending on a control switch.
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.
++ @yongzhi1 fyi
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.
@plbossart jfyi, Chrome PnP team was asking deepbuf for Amp only, during their tests, they don't connect headset jack any more.
@sathya-nujella @cujomalainey can you help with below Query,
What are the odds that we need a Deep buffer stream on the headphone and a Deep buffer stream on the amplifiers running concurrently?
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.
then remove the deep buffer for headsets and move it to amps...
I am still not clear on the benefits to be honest. Amps will by construction require 3+ digit milliwatts. In all my previous power optimizations, we optimized for the headset...
We can't continue adding buffers just in case they might be useful.
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.
completely agree with you, thank you @plbossart
@sathya-nujella we can close based on your comments.
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.
completely agree with you, thank you @plbossart @sathya-nujella we can close based on your comments.
If we have to choose one, then adding to speaker amp is needed as default BKM used by PnP team is that endpoint. Thanks!
@@ -69,6 +69,11 @@ Define { | |||
DEEP_BUFFER_PIPELINE_SRC 'mixin.15.1' | |||
DEEP_BUFFER_PIPELINE_SINK 'mixout.1.1' | |||
DEEP_BUFFER_PCM_NAME 'Deepbuffer Jack Out' | |||
DEEP_BUFFER_PIPELINE_ID_2 16 | |||
DEEP_BUFFER_PCM_ID_2 35 |
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 other question is: why add this for LNL only? if this is really useful why not add this capability in all Chrome topologies?
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.
As the current topology for sdw, had Deepbuffer option only for "device 31: Deepbuffer Jack Out"
thought of adding to Spk-amps too..
Dint intend to add for all Chrome devices, as only from MTL, we started to validate Multiple Stream.
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.
Is this a real need though? I mean, every deep-buffer stream will eat-up memory that will be reserved, so every time we add such a stream it means less memory for 3rd party libraries. Multiple streams for all endpoints has a cost in the end...
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 for the explanation about memory impact with this new stream.
In this scenario, we should remove existing "device 31: Deepbuffer Jack Out", until requested by customer ?
Add deep buffer PCM35 on sdw-amps for sof-lnl-rt722-l0 tplg.