Skip to content
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

topology1: Use DYNAMIC for ADL and RPL topologies #8093

Merged
merged 1 commit into from
Sep 18, 2023

Conversation

Vamshigopal
Copy link
Contributor

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

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.

@ranj063 good for you ?

Copy link
Member

@plbossart plbossart left a comment

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

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

Copy link
Contributor Author

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.

tools/topology/topology1/CMakeLists.txt Show resolved Hide resolved
@Vamshigopal
Copy link
Contributor Author

Vamshigopal commented Aug 28, 2023

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

Vamshigopal commented Sep 4, 2023

@plbossart may i know your thoughts ?

@plbossart
Copy link
Member

plbossart commented Sep 5, 2023

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

Copy link
Contributor

@johnylin76 johnylin76 left a comment

Choose a reason for hiding this comment

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

LGTM

@lgirdwood
Copy link
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
Copy link
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
Copy link
Contributor Author

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.

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 <[email protected]>
@Vamshigopal
Copy link
Contributor Author

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

@kv2019i
Copy link
Collaborator

kv2019i commented Sep 18, 2023

@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:main Sep 18, 2023
41 checks passed
@Vamshigopal
Copy link
Contributor Author

@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

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.

6 participants