Add disable_function_topology flag to disable loading function topologies

[bardliao]

User may not want to load topology dynamically. Add a flag to allow user to disable the loading function topologies feature.

[ujfalusi]

I would go about this in a different order: first to disable the function topologies if the filename is overridden then add the module parameter to disable the function topologies.

diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
index 84a9ef15593e..d2c7cf327ff1 100644
--- a/sound/soc/sof/core.c
+++ b/sound/soc/sof/core.c
@@ -636,8 +636,14 @@ sof_apply_profile_override(struct sof_loadable_file_profile *path_override)
 		path_override->fw_lib_path = override_lib_path;
 	if (override_tplg_path)
 		path_override->tplg_path = override_tplg_path;
-	if (override_tplg_filename)
+	if (override_tplg_filename) {
 		path_override->tplg_name = override_tplg_filename;
+		/*
+		 * User requested a specific topology file and expect it to be
+		 * loaded
+		 */
+		plat_data->disable_function_topology = true;
+	}
 }
 
 int snd_sof_device_probe(struct device *dev, struct snd_sof_pdata *plat_data)

diff --git a/sound/soc/sof/topology.c b/sound/soc/sof/topology.c
index 87fad91764af..756acc1a7445 100644
--- a/sound/soc/sof/topology.c
+++ b/sound/soc/sof/topology.c
@@ -19,6 +19,10 @@
 #include "sof-audio.h"
 #include "ops.h"
 
+static bool disable_function_topology;
+module_param(disable_function_topology, bool, 0444);
+MODULE_PARM_DESC(disable_function_topology, "Disable function topology loading");
+
 #define COMP_ID_UNASSIGNED		0xffffffff
 /*
  * Constants used in the computation of linear volume gain
@@ -2502,7 +2506,9 @@ int snd_sof_load_topology(struct snd_soc_component *scomp, const char *file)
 	if (!tplg_files)
 		return -ENOMEM;
 
-	if (sof_pdata->machine->get_function_tplg_files) {
+	if (!sof_pdata->disable_function_topology &&
+	    !disable_function_topology &&
+	    sof_pdata->machine->get_function_tplg_files) {
 		tplg_cnt = sof_pdata->machine->get_function_tplg_files(scomp->card,
 								       sof_pdata->machine,
 								       tplg_filename_prefix,

[bardliao]

I agree that we can reorder the commits. But why do we need split sof_pdata->disable_function_topology and disable_function_topology? topology.c and core.c are both belong to the snd-sof module

[ujfalusi]

I agree that we can reorder the commits. But why do we need split sof_pdata->disable_function_topology and disable_function_topology? topology.c and core.c are both belong to the snd-sof module

yes, but how I see is that the module parameter is specific to the topology.c file's internal operation.
we disable the function tplg if user asked for a specific one or user disabled the functionality. I don't see the relation between the two parameters. Probably it is only me...

[bardliao]

I agree that we can reorder the commits. But why do we need split sof_pdata->disable_function_topology and disable_function_topology? topology.c and core.c are both belong to the snd-sof module

yes, but how I see is that the module parameter is specific to the topology.c file's internal operation. we disable the function tplg if user asked for a specific one or user disabled the functionality. I don't see the relation between the two parameters. Probably it is only me...

You convinced me. Updated.

[bardliao]

Looks like we have new Sparse check error.

sound/soc/amd/acp/amd-acpi-mach.c:11:28: error: symbol 'amp_rt1019' was not declared. Should it be static?
sound/soc/amd/acp/amd-acpi-mach.c:16:28: error: symbol 'amp_max' was not declared. Should it be static?
sound/soc/amd/acp/amd-acpi-mach.c:21:26: error: symbol 'snd_soc_acpi_amd_acp_machines' was not declared. Should it be static?
sound/soc/amd/acp/amd-acpi-mach.c:52:26: error: symbol 'snd_soc_acpi_amd_rmb_acp_machines' was not declared. Should it be static?
sound/soc/amd/acp/amd-acpi-mach.c:73:26: error: symbol 'snd_soc_acpi_amd_acp63_acp_machines' was not declared. Should it be static?
sound/soc/amd/acp/amd-acpi-mach.c:82:26: error: symbol 'snd_soc_acpi_amd_acp70_acp_machines' was not declared. Should it be static?
make[6]: *** [scripts/Makefile.build:208: sound/soc/amd/acp/amd-acpi-mach.o] Error 1

@Venkata-Prasad-Potturu fyi

[ranj063]

@bardliao can you modify the commit message to say "Add a kernel module parameter to override loading function topologies when set"?

[bardliao]

@bardliao can you modify the commit message to say "Add a kernel module parameter to override loading function topologies when set"?

@ranj063 But the disable_function_topology module parameter doesn't use this flag. From @ujfalusi's point of view, the disable_function_topology flag and the disable_function_topology module parameter are 2 different parameters.
#5373 (comment)

Do you prefer we set the disable_function_topology flag if the disable_function_topology module parameter is set?
I am all ears.

[ranj063]

Ahh I see. Sorry i got confused there.

[ranj063]

@bardliao while this is OK for now, I feel like we need to do more to make it future-proof. We have a kernel module param to override the default topology today but when we move to using split topologies, what happens this? This parameter will not be enough isnt it? Will we need to override a function topology from userspace ever? Id think that it is a possibility.

[lgirdwood]

@bardliao while this is OK for now, I feel like we need to do more to make it future-proof. We have a kernel module param to override the default topology today but when we move to using split topologies, what happens this? This parameter will not be enough isnt it? Will we need to override a function topology from userspace ever? Id think that it is a possibility.

When we invoke this "override", split topologies would have to be manually loaded by the user (and it would be up to the user to pick the correct topology and order them correctly). This is more a dev/debug use case though.

[ujfalusi]

@bardliao while this is OK for now, I feel like we need to do more to make it future-proof. We have a kernel module param to override the default topology today but when we move to using split topologies, what happens this? This parameter will not be enough isnt it? Will we need to override a function topology from userspace ever? Id think that it is a possibility.

When we invoke this "override", split topologies would have to be manually loaded by the user (and it would be up to the user to pick the correct topology and order them correctly). This is more a dev/debug use case though.

we don't have means to load function topologies manually.
This PR does two separate things:

• disable the function topology loading if the user specified a monolithic, single topology via module parameter
• add a separate module parameter to disable the function topology loading. In this case the default topology is going to be loaded.

I guess the sound/soc/intel/common/sof-function-topology-lib.c can add module parameters to specify/override function topology names to be used when the topology name is not set and/or the function topology loading is allowed (not prevented by module parameter).
But that is not a scope of this PR, I believe.

[lgirdwood]

we don't have means to load function topologies manually. This PR does two separate things:

• disable the function topology loading if the user specified a monolithic, single topology via module parameter
• add a separate module parameter to disable the function topology loading. In this case the default topology is going to be loaded.

I guess the sound/soc/intel/common/sof-function-topology-lib.c can add module parameters to specify/override function topology names to be used when the topology name is not set and/or the function topology loading is allowed (not prevented by module parameter). But that is not a scope of this PR, I believe.

Lets plan this as useful dev/dbg feature i.e. no module recompile needed to try a test/debug/alternate topology change.