[PATCH 0/5] ASoC: SOF: ipc4: Querry CPC value from firmware's manifest

[PATCH 0/5] ASoC: SOF: ipc4: Querry CPC value from firmware's manifest

[Peter Ujfalusi]

Hi,

Hi,

The MOD_INIT_INSTANCE message contains a CPC (Cycles Per Chunk/processing unit)
parameter.
This CPC value is used by the firmware to calculate the total cycles needed by
the enabled module instances and based on this it can decide to set the
frequency of the DSP core(s).

The manifest section of the firmware image contains a module configuration
section, where a per module table of configurations are listed with measured
CPC values as triplet of IBS/IBS/CPC (Input/Output buffer size - corresponding
to the selected audio format).

In case the CPC value is 0 (missing from the manifest or the configuration cannot
be matched) the firmware will force the DSP cores to maximum speed to avoid audio
glitches due to starvation.
In these cases the kernel will print a warning message to let the SOF developers
know about the gap and provide information to correct it with a firmware update.

Regards,
Peter
---
Peter Ujfalusi (5):
  ASoC: SOF: ipc4-loader: Drop unused bss_size from struct
    sof_ipc4_fw_module
  ASoC: SOF: ipc4-loader: Save a pointer to fm_config in
    sof_ipc4_fw_module
  ASoC: SOF: ipc4-topology: Rename sof_ipc4_update_pipeline_mem_usage()
    to be generic
  ASoC: SOF: ipc4-topology: Do not use the CPC value from topology
  ASoC: SOF: ipc4-loader/topology: Query the CPC value from manifest

 sound/soc/sof/ipc4-loader.c   | 72 ++++++++++++++++++++++++++++++++---
 sound/soc/sof/ipc4-priv.h     | 10 ++++-
 sound/soc/sof/ipc4-topology.c | 33 +++++++++-------
 3 files changed, 94 insertions(+), 21 deletions(-)

-- 
2.40.1

[PATCH 1/5] ASoC: SOF: ipc4-loader: Drop unused bss_size from struct sof_ipc4_fw_module

[Peter Ujfalusi]

The bss_size is only set, but not used by the code, remove it.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/ipc4-loader.c | 6 +-----
 sound/soc/sof/ipc4-priv.h   | 2 --
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/sound/soc/sof/ipc4-loader.c b/sound/soc/sof/ipc4-loader.c
index 1321acc402fd..bb215d33d455 100644
--- a/sound/soc/sof/ipc4-loader.c
+++ b/sound/soc/sof/ipc4-loader.c
@@ -112,16 +112,12 @@ static ssize_t sof_ipc4_fw_parse_ext_man(struct snd_sof_dev *sdev,
 				return -EINVAL;
 			}
 
-			/* a module's config is always the same size */
-			fw_module->bss_size = fm_config[fm_entry->cfg_offset].is_bytes;
 
 			dev_dbg(sdev->dev,
 				"module %s: UUID %pUL cfg_count: %u, bss_size: %#x\n",
 				fm_entry->name, &fm_entry->uuid, fm_entry->cfg_count,
-				fw_module->bss_size);
+				fm_config[fm_entry->cfg_offset].is_bytes);
 		} else {
-			fw_module->bss_size = 0;
-
 			dev_dbg(sdev->dev, "module %s: UUID %pUL\n", fm_entry->name,
 				&fm_entry->uuid);
 		}
diff --git a/sound/soc/sof/ipc4-priv.h b/sound/soc/sof/ipc4-priv.h
index f461b8c70df3..546e52746276 100644
--- a/sound/soc/sof/ipc4-priv.h
+++ b/sound/soc/sof/ipc4-priv.h
@@ -29,13 +29,11 @@ enum sof_ipc4_mtrace_type {
  * struct sof_ipc4_fw_module - IPC4 module info
  * @sof_man4_module: Module info
  * @m_ida: Module instance identifier
- * @bss_size: Module object size
  * @private: Module private data
  */
 struct sof_ipc4_fw_module {
 	struct sof_man4_module man4_module_entry;
 	struct ida m_ida;
-	u32 bss_size;
 	void *private;
 };
 
-- 
2.40.1

[PATCH 2/5] ASoC: SOF: ipc4-loader: Save a pointer to fm_config in sof_ipc4_fw_module

[Peter Ujfalusi]

Save a pointer to the firmware module configuration area in
sof_ipc4_fw_module struct for later use.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/ipc4-loader.c | 1 +
 sound/soc/sof/ipc4-priv.h   | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/sound/soc/sof/ipc4-loader.c b/sound/soc/sof/ipc4-loader.c
index bb215d33d455..3860d3455960 100644
--- a/sound/soc/sof/ipc4-loader.c
+++ b/sound/soc/sof/ipc4-loader.c
@@ -112,6 +112,7 @@ static ssize_t sof_ipc4_fw_parse_ext_man(struct snd_sof_dev *sdev,
 				return -EINVAL;
 			}
 
+			fw_module->fw_mod_cfg = &fm_config[fm_entry->cfg_offset];
 
 			dev_dbg(sdev->dev,
 				"module %s: UUID %pUL cfg_count: %u, bss_size: %#x\n",
diff --git a/sound/soc/sof/ipc4-priv.h b/sound/soc/sof/ipc4-priv.h
index 546e52746276..4b3495dc455d 100644
--- a/sound/soc/sof/ipc4-priv.h
+++ b/sound/soc/sof/ipc4-priv.h
@@ -28,11 +28,13 @@ enum sof_ipc4_mtrace_type {
 /**
  * struct sof_ipc4_fw_module - IPC4 module info
  * @sof_man4_module: Module info
+ * @fw_mod_cfg: Pointer to the module config start of the module
  * @m_ida: Module instance identifier
  * @private: Module private data
  */
 struct sof_ipc4_fw_module {
 	struct sof_man4_module man4_module_entry;
+	const struct sof_man4_module_config *fw_mod_cfg;
 	struct ida m_ida;
 	void *private;
 };
-- 
2.40.1

[PATCH 3/5] ASoC: SOF: ipc4-topology: Rename sof_ipc4_update_pipeline_mem_usage() to be generic

[Peter Ujfalusi]

Rename sof_ipc4_update_pipeline_mem_usage() to
sof_ipc4_update_resource_usage() in order to be re-usable for generic
resource storage, calculation of a module, like CPC adjustment.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/ipc4-topology.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/sound/soc/sof/ipc4-topology.c b/sound/soc/sof/ipc4-topology.c
index dce2f8f7f518..547b3a935931 100644
--- a/sound/soc/sof/ipc4-topology.c
+++ b/sound/soc/sof/ipc4-topology.c
@@ -936,8 +936,8 @@ static void sof_ipc4_widget_free_comp_process(struct snd_sof_widget *swidget)
 }
 
 static void
-sof_ipc4_update_pipeline_mem_usage(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget,
-				   struct sof_ipc4_base_module_cfg *base_config)
+sof_ipc4_update_resource_usage(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget,
+			       struct sof_ipc4_base_module_cfg *base_config)
 {
 	struct sof_ipc4_fw_module *fw_module = swidget->module_info;
 	struct snd_sof_widget *pipe_widget;
@@ -1711,7 +1711,7 @@ sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget,
 		       *data, copier_data->gtw_cfg.config_length * 4);
 
 	/* update pipeline memory usage */
-	sof_ipc4_update_pipeline_mem_usage(sdev, swidget, &copier_data->base_config);
+	sof_ipc4_update_resource_usage(sdev, swidget, &copier_data->base_config);
 
 	return 0;
 }
@@ -1748,7 +1748,7 @@ static int sof_ipc4_prepare_gain_module(struct snd_sof_widget *swidget,
 	}
 
 	/* update pipeline memory usage */
-	sof_ipc4_update_pipeline_mem_usage(sdev, swidget, &gain->base_config);
+	sof_ipc4_update_resource_usage(sdev, swidget, &gain->base_config);
 
 	return 0;
 }
@@ -1785,7 +1785,7 @@ static int sof_ipc4_prepare_mixer_module(struct snd_sof_widget *swidget,
 	}
 
 	/* update pipeline memory usage */
-	sof_ipc4_update_pipeline_mem_usage(sdev, swidget, &mixer->base_config);
+	sof_ipc4_update_resource_usage(sdev, swidget, &mixer->base_config);
 
 	return 0;
 }
@@ -1822,7 +1822,7 @@ static int sof_ipc4_prepare_src_module(struct snd_sof_widget *swidget,
 	}
 
 	/* update pipeline memory usage */
-	sof_ipc4_update_pipeline_mem_usage(sdev, swidget, &src->base_config);
+	sof_ipc4_update_resource_usage(sdev, swidget, &src->base_config);
 
 	/* update pipeline_params for sink widgets */
 	rate = hw_param_interval(pipeline_params, SNDRV_PCM_HW_PARAM_RATE);
@@ -1959,7 +1959,7 @@ static int sof_ipc4_prepare_process_module(struct snd_sof_widget *swidget,
 	}
 
 	/* update pipeline memory usage */
-	sof_ipc4_update_pipeline_mem_usage(sdev, swidget, &process->base_config);
+	sof_ipc4_update_resource_usage(sdev, swidget, &process->base_config);
 
 	/* ipc_config_data is composed of the base_config followed by an optional extension */
 	memcpy(cfg, &process->base_config, sizeof(struct sof_ipc4_base_module_cfg));
-- 
2.40.1

[PATCH 4/5] ASoC: SOF: ipc4-topology: Do not use the CPC value from topology

[Peter Ujfalusi]

Stop parsing the CPC value from topology to module_base_cfg.
The CPC value is only set for few modules in topology which makes the CPC
handling inconsistent.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/ipc4-topology.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/sound/soc/sof/ipc4-topology.c b/sound/soc/sof/ipc4-topology.c
index 547b3a935931..bdfc76591f76 100644
--- a/sound/soc/sof/ipc4-topology.c
+++ b/sound/soc/sof/ipc4-topology.c
@@ -39,8 +39,6 @@ static const struct sof_topology_token pipeline_tokens[] = {
 };
 
 static const struct sof_topology_token ipc4_comp_tokens[] = {
-	{SOF_TKN_COMP_CPC, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32,
-		offsetof(struct sof_ipc4_base_module_cfg, cpc)},
 	{SOF_TKN_COMP_IS_PAGES, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32,
 		offsetof(struct sof_ipc4_base_module_cfg, is_pages)},
 };
@@ -235,7 +233,7 @@ static int sof_ipc4_get_audio_fmt(struct snd_soc_component *scomp,
 		"Number of input audio formats: %d. Number of output audio formats: %d\n",
 		available_fmt->num_input_formats, available_fmt->num_output_formats);
 
-	/* set cpc and is_pages in the module's base_config */
+	/* set is_pages in the module's base_config */
 	ret = sof_update_ipc_object(scomp, module_base_cfg, SOF_COMP_TOKENS, swidget->tuples,
 				    swidget->num_tuples, sizeof(*module_base_cfg), 1);
 	if (ret) {
@@ -244,8 +242,8 @@ static int sof_ipc4_get_audio_fmt(struct snd_soc_component *scomp,
 		return ret;
 	}
 
-	dev_dbg(scomp->dev, "widget %s cpc: %d is_pages: %d\n",
-		swidget->widget->name, module_base_cfg->cpc, module_base_cfg->is_pages);
+	dev_dbg(scomp->dev, "widget %s: is_pages: %d\n", swidget->widget->name,
+		module_base_cfg->is_pages);
 
 	if (available_fmt->num_input_formats) {
 		in_format = kcalloc(available_fmt->num_input_formats,
@@ -723,9 +721,9 @@ static int sof_ipc4_widget_setup_comp_pga(struct snd_sof_widget *swidget)
 	}
 
 	dev_dbg(scomp->dev,
-		"pga widget %s: ramp type: %d, ramp duration %d, initial gain value: %#x, cpc %d\n",
+		"pga widget %s: ramp type: %d, ramp duration %d, initial gain value: %#x\n",
 		swidget->widget->name, gain->data.curve_type, gain->data.curve_duration_l,
-		gain->data.init_val, gain->base_config.cpc);
+		gain->data.init_val);
 
 	ret = sof_ipc4_widget_setup_msg(swidget, &gain->msg);
 	if (ret)
-- 
2.40.1

[PATCH 5/5] ASoC: SOF: ipc4-loader/topology: Query the CPC value from manifest

[Peter Ujfalusi]

The manifest's firmware module configuration section contains the measured
CPC values along with a matching IBS/OBS values.

The CPC can be looked up by looking for a matching IBS/OBS entry.
In case of multiple matches we will use the highest CPC value.

If there is no mod_cfg or no CPC value (all 0) or no match was found then
print warning message and use 0 as CPC value.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
---
 sound/soc/sof/ipc4-loader.c   | 65 +++++++++++++++++++++++++++++++++++
 sound/soc/sof/ipc4-priv.h     |  6 ++++
 sound/soc/sof/ipc4-topology.c |  7 ++++
 3 files changed, 78 insertions(+)

diff --git a/sound/soc/sof/ipc4-loader.c b/sound/soc/sof/ipc4-loader.c
index 3860d3455960..eaa04762eb11 100644
--- a/sound/soc/sof/ipc4-loader.c
+++ b/sound/soc/sof/ipc4-loader.c
@@ -423,6 +423,71 @@ int sof_ipc4_reload_fw_libraries(struct snd_sof_dev *sdev)
 	return ret;
 }
 
+/**
+ * sof_ipc4_update_cpc_from_manifest - Update the cpc in base config from manifest
+ * @sdev: SOF device
+ * @fw_module: pointer struct sof_ipc4_fw_module to parse
+ * @basecfg: Pointer to the base_config to update
+ */
+void sof_ipc4_update_cpc_from_manifest(struct snd_sof_dev *sdev,
+				       struct sof_ipc4_fw_module *fw_module,
+				       struct sof_ipc4_base_module_cfg *basecfg)
+{
+	const struct sof_man4_module_config *fw_mod_cfg;
+	u32 cpc_pick = 0;
+	u32 max_cpc = 0;
+	const char *msg;
+	int i;
+
+	if (!fw_module->fw_mod_cfg) {
+		msg = "No mod_cfg available for CPC lookup in the firmware file's manifest";
+		goto no_cpc;
+	}
+
+	/*
+	 * Find the best matching (highest) CPC value based on the module's
+	 * IBS/OBS configuration inferred from the audio format selection.
+	 *
+	 * The CPC value in each module config entry has been measured and
+	 * recorded as a IBS/OBS/CPC triplet and stored in the firmware file's
+	 * manifest
+	 */
+	fw_mod_cfg = fw_module->fw_mod_cfg;
+	for (i = 0; i < fw_module->man4_module_entry.cfg_count; i++) {
+		if (basecfg->obs == fw_mod_cfg[i].obs &&
+		    basecfg->ibs == fw_mod_cfg[i].ibs &&
+		    cpc_pick < fw_mod_cfg[i].cpc)
+			cpc_pick = fw_mod_cfg[i].cpc;
+
+		if (max_cpc < fw_mod_cfg[i].cpc)
+			max_cpc = fw_mod_cfg[i].cpc;
+	}
+
+	basecfg->cpc = cpc_pick;
+
+	/* We have a matching configuration for CPC */
+	if (basecfg->cpc)
+		return;
+
+	/*
+	 * No matching IBS/OBS found, the firmware manifest is missing
+	 * information in the module's module configuration table.
+	 */
+	if (!max_cpc)
+		msg = "No CPC value available in the firmware file's manifest";
+	else if (!cpc_pick)
+		msg = "No CPC match in the firmware file's manifest";
+
+no_cpc:
+	dev_warn(sdev->dev, "%s (UUID: %pUL): %s (ibs/obs: %u/%u)\n",
+		 fw_module->man4_module_entry.name,
+		 &fw_module->man4_module_entry.uuid, msg, basecfg->ibs,
+		 basecfg->obs);
+	dev_warn_once(sdev->dev, "Please try to update the firmware.\n");
+	dev_warn_once(sdev->dev, "If the issue persists, file a bug at\n");
+	dev_warn_once(sdev->dev, "https://github.com/thesofproject/sof/issues/\n");
+}
+
 const struct sof_ipc_fw_loader_ops ipc4_loader_ops = {
 	.validate = sof_ipc4_validate_firmware,
 	.parse_ext_manifest = sof_ipc4_fw_parse_basefw_ext_man,
diff --git a/sound/soc/sof/ipc4-priv.h b/sound/soc/sof/ipc4-priv.h
index 4b3495dc455d..a5d0b2eae464 100644
--- a/sound/soc/sof/ipc4-priv.h
+++ b/sound/soc/sof/ipc4-priv.h
@@ -114,4 +114,10 @@ int sof_ipc4_query_fw_configuration(struct snd_sof_dev *sdev);
 int sof_ipc4_reload_fw_libraries(struct snd_sof_dev *sdev);
 struct sof_ipc4_fw_module *sof_ipc4_find_module_by_uuid(struct snd_sof_dev *sdev,
 							const guid_t *uuid);
+
+struct sof_ipc4_base_module_cfg;
+void sof_ipc4_update_cpc_from_manifest(struct snd_sof_dev *sdev,
+				       struct sof_ipc4_fw_module *fw_module,
+				       struct sof_ipc4_base_module_cfg *basecfg);
+
 #endif
diff --git a/sound/soc/sof/ipc4-topology.c b/sound/soc/sof/ipc4-topology.c
index bdfc76591f76..d5e6d7caaaa3 100644
--- a/sound/soc/sof/ipc4-topology.c
+++ b/sound/soc/sof/ipc4-topology.c
@@ -966,6 +966,13 @@ sof_ipc4_update_resource_usage(struct snd_sof_dev *sdev, struct snd_sof_widget *
 	pipe_widget = swidget->spipe->pipe_widget;
 	pipeline = pipe_widget->private;
 	pipeline->mem_usage += total;
+
+	/* Update base_config->cpc from the module manifest */
+	sof_ipc4_update_cpc_from_manifest(sdev, fw_module, base_config);
+
+	dev_dbg(sdev->dev, "%s: ibs / obs / cpc: %u / %u / %u\n",
+		swidget->widget->name, base_config->ibs, base_config->obs,
+		base_config->cpc);
 }
 
 static int sof_ipc4_widget_assign_instance_id(struct snd_sof_dev *sdev,
-- 
2.40.1

Re: [PATCH 0/5] ASoC: SOF: ipc4: Querry CPC value from firmware's manifest

[Mark Brown]

On Mon, 22 May 2023 13:13:08 +0300, Peter Ujfalusi wrote:
> Hi,
> 
> The MOD_INIT_INSTANCE message contains a CPC (Cycles Per Chunk/processing unit)
> parameter.
> This CPC value is used by the firmware to calculate the total cycles needed by
> the enabled module instances and based on this it can decide to set the
> frequency of the DSP core(s).
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/5] ASoC: SOF: ipc4-loader: Drop unused bss_size from struct sof_ipc4_fw_module
      commit: d474809e9284848b6cb57a885f3252b86a0b485f
[2/5] ASoC: SOF: ipc4-loader: Save a pointer to fm_config in sof_ipc4_fw_module
      commit: fe04f300035d497a066172a9a9331439cc8300f6
[3/5] ASoC: SOF: ipc4-topology: Rename sof_ipc4_update_pipeline_mem_usage() to be generic
      commit: 19c745d1fd1a61a04a0b44623c70c4e71b6f274a
[4/5] ASoC: SOF: ipc4-topology: Do not use the CPC value from topology
      commit: 9caa90180512581821d7498132f952ebd4ba05ad
[5/5] ASoC: SOF: ipc4-loader/topology: Query the CPC value from manifest
      commit: d8a2c987934959dd1f27de75401625650cd25e47

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark