Skip to content

[cavs2.5-001] topology1: sof-hda-generic: enable dts playback #8550

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.

Sign up for GitHub

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

Merged
mwasko merged 1 commit into from
Dec 11, 2023
Merged

[cavs2.5-001] topology1: sof-hda-generic: enable dts playback #8550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tools/topology/topology1/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ set(TPLGS
"sof-hda-generic\;sof-hda-generic-2ch\;-DCHANNELS=2\;-DHSPROC=volume\;-DDMICPROC_FILTER1=eq_iir_coef_highpass_40hz_20db_48khz.m4\;-DDMIC16KPROC_FILTER1=eq_iir_coef_highpass_40hz_20db_16khz.m4\;-DDYNAMIC=1"
"sof-hda-generic\;sof-hda-generic-3ch\;-DCHANNELS=4\;-DHSPROC=volume\;-DDMICPROC_FILTER1=eq_iir_coef_highpass_40hz_20db_48khz.m4\;-DDMIC16KPROC_FILTER1=eq_iir_coef_highpass_40hz_20db_16khz.m4\;-DDYNAMIC=1"
"sof-hda-generic\;sof-hda-generic-4ch\;-DCHANNELS=4\;-DHSPROC=volume\;-DDMICPROC_FILTER1=eq_iir_coef_highpass_40hz_20db_48khz.m4\;-DDMIC16KPROC_FILTER1=eq_iir_coef_highpass_40hz_20db_16khz.m4\;-DDYNAMIC=1"
"sof-hda-generic\;sof-hda-generic-4ch-dts\;-DCHANNELS=4\;-DHSPROC=volume\;-DDMICPROC_FILTER1=eq_iir_coef_highpass_40hz_20db_48khz.m4\;-DDMIC16KPROC_FILTER1=eq_iir_coef_highpass_40hz_20db_16khz.m4\;-DDYNAMIC=1\;-DDTS=`DTS'"

@plbossart plbossart Nov 30, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have mixed feelings about this, all existing HDaudio topologies don't have any processing, so new topologies with processing should be handled in a well-identified block. @singalsu was also working on changes to HDaudio, we should place all 'enhanced' topologies together IMHO.

At a minimum this should be a different comment block, worst-case a different file.

@macchian macchian Dec 4, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@plbossart , please correct me if I misunderstand your suggestions. After reading the singalsu's PR, I think new hda file with process.m4 could be a source topology for all other 3P codecs directed pipe.

So create a new file e.g. sof-hda-generic-pproc.m4 (copy from sof-hda-generic.m4 with fixed missing pipe here. Currently this way might be worst case.

+"sof-hda-generic-pproc;sof-hda-generic-pproc-dts;-DCHANNELS=4;-DHSPROC=volume;-DDMICPROC_FILTER1=eq_iir_coef_highpass_40hz_20db_48khz.m4;-DDMIC16KPROC_FILTER1=eq_iir_coef_highpass_40hz_20db_16khz.m4;-DDYNAMIC=1;-DPPROC=pipe-eq-iir-dts-codec-playback.m4"

@plbossart plbossart Dec 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@macchian the point is that DTS is not an open-sourced library that is distributed as part of sof-bin, so we want this topology to be identified and not released by sof-bin (or released in a very clear directory that says CHROME_ONLY or something).

Adding @marc-hb for further comments.

@lgirdwood lgirdwood Dec 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is needed for Chrome cavs-2.5 and main (tplg2) branches but not needed in stable-v2.2.

@lgirdwood lgirdwood Dec 4, 2023 •
edited by macchian
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@macchian do you have a similar PR with enhanced HDA for main branch ? We cant use 3P for HDA main, but we can use EQ and other open modules. Please work with @singalsu when he is back next week on module/pipeline definition (that can also be used in CI) around HDA. Thanks !
@lgirdwood , no, I don't have it on main branch. In this case it needs on cavs2.5. On main branch, I might need to consult @singalsu how to use EQ as a similar example as you mentioned.

@marc-hb marc-hb Dec 5, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

the point is that DTS is not an open-sourced library that is distributed as part of sof-bin, so we want this topology to be identified and not released by sof-bin
Adding @marc-hb for further comments.

This is needed for Chrome cavs-2.5 and main (tplg2) branches but not needed in stable-v2.2.

It is also my understanding that this cavs25 branch will never be released to sof-bin.

Now we should probably still have a good answer to @plbossart for the longer term (new subdirectory?) but not needed in the cavs2.5 branch AFAIK.

Even if it were accidentally released to sof-bin, how much damage would that do?

@plbossart plbossart Dec 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I will assert that all topologies released in sof-bin should be compatible with firmware released in sof-bin. Otherwise, there's a risk of someone loading a topology that will result in an IPC error when trying to access a non-existent module.

So while I have no objection to the notion of adding more topologies in main for Chrome or others to make downstream project management easier, I kindly request that they be managed separately instead of the same directory and Cmake file.

@lgirdwood lgirdwood Dec 5, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cavs-2.5 branch is downstream Chrome so not in sof-bin.
We can make sure a similar PR for main uses the EQ (or similar open module) as an example module.

@marc-hb marc-hb Dec 5, 2023

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Otherwise, there's a risk of someone loading a topology that will result in an IPC error when trying to access a non-existent module.

Is a kernel parameter the only way to attempt this?

## end HDaudio codec topologies

"sof-hda-generic-idisp\;sof-hda-generic-idisp\;-DCHANNELS=0\;-DDYNAMIC=1"
Expand Down
11 changes: 10 additions & 1 deletion tools/topology/topology1/sof-hda-generic.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,22 @@ DAI_ADD(PIPE_HEADSET_PLAYBACK,

# Low Latency playback pipeline 1 on PCM 30 using max 2 channels of s32le.
# 1000us deadline on core 0 with priority 0
PIPELINE_PCM_ADD(sof/pipe-host-volume-playback.m4,
PIPELINE_PCM_ADD(
ifdef(`DTS', sof/pipe-eq-iir-dts-codec-playback.m4, sof/pipe-host-volume-playback.m4),
30, 0, 2, s32le,
1000, 0, 0,
48000, 48000, 48000,
SCHEDULE_TIME_DOMAIN_TIMER,
PIPELINE_PLAYBACK_SCHED_COMP_1)

ifdef(`DTS',
`
# Because there is no dai pipeline.30 for HDA0.OUT in pipe-eq-iir-dts-codec-playback.m4, so
# using macro defined W_PIPELINE_TOP() to add missing dai pipeline back. Instead of
# modifying in pipe-eq-iir-dts-codec-playback.m4, these changes are not necessary for all others.
W_PIPELINE_TOP(30, HDA0.OUT, 1000, 0, 0, 1, pipe_dai_schedule_plat)
')

# Deep buffer playback pipeline 31 on PCM 31 using max 2 channels of s32le
# Set 1000us deadline on core 0 with priority 0.
# TODO: Modify pipeline deadline to account for deep buffering
Expand Down