Skip to content

topology1: Use DYNAMIC for ADL and RPL topologies - #8093

Merged
kv2019i merged 1 commit into
thesofproject:mainfrom
Vamshigopal:dynamic-pipeline
Sep 18, 2023
Merged

topology1: Use DYNAMIC for ADL and RPL topologies#8093
kv2019i merged 1 commit into
thesofproject:mainfrom
Vamshigopal:dynamic-pipeline

Conversation

@Vamshigopal

@VamshigopalVamshigopal commented Aug 25, 2023

Copy link
Copy Markdown
Contributor

use dynamic for all the adl and rpl topologies except non 3p(waves,DTS)

lgirdwood
lgirdwood approved these changes Aug 25, 2023
View reviewed changes

@lgirdwoodlgirdwood left a comment

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.

@ranj063 good for you ?

plbossart
plbossart requested changes Aug 25, 2023
View reviewed changes

@plbossartplbossart left a comment

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 looks a bit too much of a sledgehammer patch that will break more eggs than intended.

What is the problem we are trying to fix anyways? Why is this necessary and why now?

And there's also a kernel dependency, so there's a risk of breaking existing platforms if they update sof-bin but still use a kernel < 5.19

Comment threadtools/topology/topology1/CMakeLists.txt Outdated
"sof-tgl-max98357a-rt5682\;sof-tgl-max98357a-rt5682\;-DCODEC=MAX98357A\;-DFMT=s16le\;-DPLATFORM=tgl\;-DLINUX_MACHINE_DRIVER=sof_rt5682\;-DAMP_SSP=1"
"sof-tgl-max98357a-rt5682\;sof-adl-max98357a-rt5682\;-DCODEC=MAX98357A\;-DFMT=s16le\;-DPLATFORM=adl\;-DLINUX_MACHINE_DRIVER=sof_rt5682\;-DAMP_SSP=2"
"sof-tgl-max98357a-rt5682\;sof-adl-max98357a-rt5682-rtnr\;-DCODEC=MAX98357A\;-DFMT=s16le\;-DPLATFORM=adl\;-DLINUX_MACHINE_DRIVER=sof_rt5682\;-DAMP_SSP=2\;-DCHANNELS=2\;-DRTNR"
"sof-tgl-max98357a-rt5682\;sof-tgl-max98357a-rt5682\;-DCODEC=MAX98357A\;-DFMT=s16le\;-DPLATFORM=tgl\;-DLINUX_MACHINE_DRIVER=sof_rt5682\;-DAMP_SSP=1\;-DDYNAMIC=1"

@plbossartplbossartAug 25, 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 a TGL topology?

@VamshigopalVamshigopalAug 28, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ack, it was added by mistake. I'll correct it.

Comment threadtools/topology/topology1/CMakeLists.txt
@Vamshigopal

Vamshigopal commented Aug 28, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

This looks a bit too much of a sledgehammer patch that will break more eggs than intended.

What is the problem we are trying to fix anyways?

With Dynamic pipelines enabled, some of the power KPI show better numbers. We discussed with our customer and have agreed to move to dymanic pipeline in a phased manner - this is first set for ADL and RPL programs which are already verified.

Why is this necessary and why now?

We are planning to keep dynamic from cavs2.5 onwards.

And there's also a kernel dependency, so there's a risk of breaking existing platforms if they update sof-bin but still use a kernel < 5.19

our customer kernels for ADL , ADL- N and RPL programs all have the required kernel support too. I believe older kernels will not check for DYNAMIC flag and hence this feature wont be impacted ?

@Vamshigopal

Vamshigopal commented Sep 4, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@plbossart may i know your thoughts ?

@plbossart

plbossart commented Sep 5, 2023•
edited
Loading

Copy link
Copy Markdown
Member

IIRC there are dependencies on the kernel to support the DYNAMIC pipelines.

I am not sure there is any merit in changing all topologies blindly, clearly you have not tested the Dell/SoundWire topologies so there's at least a very large validation gap....

@ranj063 please chime in.

@johnylin76

johnylin76 commented Sep 6, 2023

Copy link
Copy Markdown
Contributor

IIRC there are dependencies on the kernel to support the DYNAMIC pipelines.

I am not sure there is any merit in changing all topologies blindly, clearly you have not tested the Dell/SoundWire topologies so there's at least a very large validation gap....

@ranj063 please chime in.

I think you made a good point that I just overlooked. Apparently my proposed code lacks many details. Although in my view that is still worth when the new config is targeted to be set massively and can be deduced from the existing ones.

I don't mind to add the config case by case in CMakeLists.txt since the deduction logic is a bit complicated.

johnylin76
johnylin76 approved these changes Sep 11, 2023
View reviewed changes

@johnylin76johnylin76 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@lgirdwood

lgirdwood commented Sep 13, 2023

Copy link
Copy Markdown
Member

I don't mind to add the config case by case in CMakeLists.txt since the deduction logic is a bit complicated.

Can we target this on case by case, IIUC @plbossart can approve that.

@plbossart

plbossart commented Sep 13, 2023

Copy link
Copy Markdown
Member

I don't mind to add the config case by case in CMakeLists.txt since the deduction logic is a bit complicated.

Can we target this on case by case, IIUC @plbossart can approve that.

I don't mind changes to the Chrome topologies if they have been tested and are compatible with the Chrome kernel.
But don't change all topologies randomly, e..g the ones used by Dell SoundWire devices should not be changed by the Chrome team.

@Vamshigopal

Vamshigopal commented Sep 14, 2023

Copy link
Copy Markdown
ContributorAuthor

I don't mind to add the config case by case in CMakeLists.txt since the deduction logic is a bit complicated.

Can we target this on case by case, IIUC @plbossart can approve that.

I don't mind changes to the Chrome topologies if they have been tested and are compatible with the Chrome kernel. But don't change all topologies randomly, e..g the ones used by Dell SoundWire devices should not be changed by the Chrome team.

Ack, Updated PR excluding Dell SoundWire topologies.

plbossart
plbossart requested changes Sep 14, 2023
View reviewed changes
Comment threadtools/topology/topology1/CMakeLists.txt Outdated
@VamshigopalVamshi Krishna Gopal
topology1: Use DYNAMIC for ADL and RPL topologies
0bf88ce
use dynamic for all the adl and rpl topologies except 3p(waves,DTS),
excluded Dell sdw topologies which are not tested.
Signed-off-by: Vamshi Krishna Gopal <vamshi.krishna.gopal@intel.com>
@Vamshigopal

Vamshigopal commented Sep 18, 2023

Copy link
Copy Markdown
ContributorAuthor

Thanks @plbossart for suggestions and reviews.
@lgirdwood can you merge this PR ?

@kv2019i

kv2019i commented Sep 18, 2023

Copy link
Copy Markdown
Collaborator

@Vamshigopal I'll proceed with merge. Can you make a backport to stable-v2.2 ? I will soon remove this file from mainline as we no longer support IPC3 for Intel targets in mainline, so any patch that is not in stable-v2.2 is at risk of getting lost.

@kv2019i
kv2019i merged commit 6f2475b into thesofproject:mainSep 18, 2023
@VamshigopalVamshigopal mentioned this pull request Sep 19, 2023
Merged
@Vamshigopal

Vamshigopal commented Sep 19, 2023

Copy link
Copy Markdown
ContributorAuthor

@Vamshigopal I'll proceed with merge. Can you make a backport to stable-v2.2 ? I will soon remove this file from mainline as we no longer support IPC3 for Intel targets in mainline, so any patch that is not in stable-v2.2 is at risk of getting lost.

ack, #8229 for stable-v2.2 branch

@VamshigopalVamshigopal mentioned this pull request Sep 19, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@plbossartplbossartplbossart approved these changes
@lgirdwoodlgirdwoodlgirdwood approved these changes
@johnylin76johnylin76johnylin76 approved these changes
@sathyap-chromesathyap-chromesathyap-chrome approved these changes
@marc-hbmarc-hbAwaiting requested review from marc-hb
@aborisovichaborisovichAwaiting requested review from aborisovich
@ranj063ranj063Awaiting requested review from ranj063ranj063 is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Vamshigopal@plbossart@johnylin76@lgirdwood@kv2019i@sathyap-chrome