Skip to content

Conversation

@kv2019i
Copy link
Collaborator

@kv2019i kv2019i commented Sep 9, 2025

Related to #10221 and solving the issue found in #10165

Marking as draft, need to do more testing, but getting this out early as this impacts #10221 review process.

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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some typos in git commit, will fix:

  • this also changes the defaults, need to address that (not just SDW)
  • will need to add a note this will not change the name for old I2S topologies (these have been shipping for some time and will be a bigger problem if the PCM name changes)

@kv2019i kv2019i requested review from macchian and yongzhi1 September 9, 2025 10:56
Follow similar split between defaults and defitinions for deep buffer
as has been used for Bluetooth, HDMI and other optional features
features.

This fixes a regression in commit b6ee409
("Topology2: Add sof-ptl-rt712-l3-rt1320-l3/-4ch support"). This
commit changed the defaults to match DSP topology of cavs-sdw.conf and broke
topologies based on cavs-rt5682.conf. cavs-rt5682.conf had correct settings,
but these were ignored as deep-buffer.conf is included in wrong order.

Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Change the default PCM name for deep buffer speaker to "Deepbuffer
Speaker". After this change, the naming is more consistent:

 - Jack Out
 - Speaker
 - Deepbuffer Jack Out
 - Deepbuffer Speaker

Keep the existing topologies based on cavs-rt5682.conf unaffected
(keep using old "Deepbuffer Amps" PCM name) as these are widely used.

Deep buffer has so far been enabled only in one Soundwire based
configuration (sof-ptl-rt712-l3-rt1320-l3-4ch.tplg). Change the PCM name
as this is not yet referred in any upstream UCM definition yet.

Suggest-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
@kv2019i kv2019i force-pushed the 202509-deepbuf-tplg-refactor branch from 8191f75 to 4dd67e9 Compare September 9, 2025 11:36
@kv2019i
Copy link
Collaborator Author

kv2019i commented Sep 9, 2025

V2:

  • version that actually works, marking ready for review
  • improved the commit message

@kv2019i kv2019i marked this pull request as ready for review September 9, 2025 11:37
Copilot AI review requested due to automatic review settings September 9, 2025 11:37
Copy link

Copilot AI left a 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 extracts deep buffer default configuration values from the main topology files into a separate configuration file to improve modularity and reduce code duplication.

  • Creates a new deep-buffer-default.conf file containing default deep buffer configuration values
  • Removes duplicate default definitions from deep-buffer.conf
  • Updates multiple topology configuration files to include the new defaults file

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tools/topology/topology2/sof-hda-generic.conf Adds include for new deep-buffer-default.conf
tools/topology/topology2/platform/intel/deep-buffer.conf Removes local default definitions and some pipeline ID constants
tools/topology/topology2/platform/intel/deep-buffer-default.conf New file containing extracted deep buffer default configurations
tools/topology/topology2/cavs-sdw.conf Adds include for deep-buffer-default.conf and defines missing pipeline constants
tools/topology/topology2/cavs-rt5682.conf Adds include for deep-buffer-default.conf
tools/topology/topology2/cavs-nocodec.conf Adds include for deep-buffer-default.conf

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

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
Copy link

Copilot AI Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of DEEP_BUFFER_PIPELINE_ID_2, DEEP_BUFFER_PCM_ID_2, DEEP_BUFFER_PIPELINE_SRC_2, DEEP_BUFFER_PIPELINE_SINK_2, and DEEP_BUFFER_PCM_NAME_2 definitions from this conditional block may cause undefined variable errors when DEEP_BUF_SPK is true, since these variables are still referenced in the pipeline configuration below but are no longer defined here.

Suggested change
SPEAKER_PCM_CORE_ID 0
SPEAKER_PCM_CORE_ID 0
DEEP_BUFFER_PIPELINE_ID_2 2
DEEP_BUFFER_PCM_ID_2 2
DEEP_BUFFER_PIPELINE_SRC_2 host-copier.$DEEP_BUFFER_PCM_ID_2.playback
DEEP_BUFFER_PIPELINE_SINK_2 gain.$DEEP_BUFFER_PIPELINE_ID_2.1
DEEP_BUFFER_PCM_NAME_2 "Deep Buffer Playback 2"

Copilot uses AI. Check for mistakes.
Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

DEEP_BUFFER_PIPELINE_SRC_2 'mixin.16.1'
DEEP_BUFFER_PIPELINE_SINK_2 'mixout.21.1'
DEEP_BUFFER_PCM_NAME_2 'Deepbuffer Amps'
DEEP_BUFFER_PCM_NAME_2 'Deepbuffer Speaker'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we are doing more of a mechanical move in this PR, but I think it makes sense to look at our _ID_n and align them with a string name.

@WeirdTreeThing
Copy link

Tested on google karis (mtl), this fixes the issue.

Copy link
Contributor

@naveen-manohar naveen-manohar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this works at my end on RT721..
I have updated https://github.com/thesofproject/sof/pull/10221/commits

image

@lgirdwood lgirdwood merged commit 319943d into thesofproject:main Sep 10, 2025
35 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants