Skip to content

ASoC: SOF: ipc4-loader/topology: Query the module's CPC from firmware manifest only#4338

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
plbossart merged 5 commits into from
May 11, 2023
Merged

ASoC: SOF: ipc4-loader/topology: Query the module's CPC from firmware manifest only #4338

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
71fe0a3
ASoC: SOF: ipc4-loader: Drop unused bss_size from struct sof_ipc4_fw_…
ujfalusi Mar 24, 2023
180955c
ASoC: SOF: ipc4-loader: Save a a pointer to fm_config in sof_ipc4_fw_…
ujfalusi Mar 24, 2023
6bb9f78
ASoC: SOF: ipc4-topology: Rename sof_ipc4_update_pipeline_mem_usage()…
ujfalusi Apr 26, 2023
d8bf981
ASoC: SOF: ipc4-topology: Do not use the CPC value from topology
ujfalusi May 4, 2023
dd24b70
ASoC: SOF: ipc4-loader/topology: Query the CPC value from manifest
ujfalusi Mar 24, 2023
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
72 changes: 67 additions & 5 deletions sound/soc/sof/ipc4-loader.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -112,16 +112,13 @@ 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;
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",
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);
}
Expand DownExpand Up@@ -426,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",

@plbossartplbossartMay 10, 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.

the problem is that no one looks at warnings, because they are not clear.

In this case, the message makes zero sense for an average user who doesn't even know what the CPC is.

It should really be "Invalid configuration: , the firmware will force the DSP clock to run with fastest clock" or something more meaningful

And I would use dev_err() as a way to catch those errors in CI, we do not track dev_warn, ever.

@ranj063ranj063May 10, 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.

But @plbossart an error would mean we need thesofproject/sof#7574 before we merge this

@plbossartplbossartMay 10, 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.

thesofproject/sof#7575 was merged, it replaced thesofproject/sof#7574

we can add a dev_err() in a follow-up patch later, for our own CI tracking.

I still think the message is far from clear. Put yourself in the shoes of johnny 4-pack, how on earth could they know what CPC is.

Maybe we can deal with this as a fixup, I'll approve to avoid blocking progress (or the new definition of progress).

@ranj063ranj063May 10, 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.

7575 only went into mtl-005-drop-stable. I dont see anything for main

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,
Expand Down
10 changes: 8 additions & 2 deletions sound/soc/sof/ipc4-priv.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -28,14 +28,14 @@ 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
* @bss_size: Module object size
* @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;
u32 bss_size;
void *private;
};

Expand DownExpand Up@@ -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
33 changes: 19 additions & 14 deletions sound/soc/sof/ipc4-topology.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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)},
};
Expand DownExpand Up@@ -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) {
Expand All@@ -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,
Expand DownExpand Up@@ -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)
Expand DownExpand Up@@ -936,8 +934,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;
Expand DownExpand Up@@ -968,6 +966,13 @@ sof_ipc4_update_pipeline_mem_usage(struct snd_sof_dev *sdev, struct snd_sof_widg
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);
Comment thread
plbossart marked this conversation as resolved.
}

static int sof_ipc4_widget_assign_instance_id(struct snd_sof_dev *sdev,
Expand DownExpand Up@@ -1739,7 +1744,7 @@ sof_ipc4_prepare_copier_module(struct snd_sof_widget *swidget,
&ipc4_copier->dma_config_tlv, dma_config_tlv_size);

/* 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;
}
Expand DownExpand Up@@ -1776,7 +1781,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;
}
Expand DownExpand Up@@ -1813,7 +1818,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;
}
Expand DownExpand Up@@ -1850,7 +1855,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);
Expand DownExpand Up@@ -1987,7 +1992,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));
Expand Down