Skip to content

Conversation

@ujfalusi
Copy link
Collaborator

@ujfalusi ujfalusi commented Mar 24, 2023

Hi,

the MOD_INIT_INSTANCE message always contains a base_cfg struct and in there the CPC value must be set to non 0 to indicate the processing need of the instantiated module with ti's configuration.
Currently we have one module specific token to pass a single, static CPC value in topology. Some module has this set, some does not, but in any case it is not reflecting the real processing need based on the configuration, parameters.

The firmware's manifest however contains mod_cfg array which contains CPC value for the module's different configuration, the CPC number should be looked up by matching the obs/ibs in manifest mod_cfg and in the runtime base_cfg obs/ibs and pick the exact matched CPC or maximum CPC.

Since the manifest is part of the firmware binary, it is hard to modify and might contain out of date or non correct value, hence we should have a way to provide similar mod_cfg type of info via tplg and use that if it is present.

This series implements this CPC lookup and configuration for the modules.

return;

if (!fw_module || !fw_module->fm_config)
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens in this case? Why don't we use the ibs*obs/1000 in this case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With the new version we are going to use that at the end.

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.

Didn't get the problem statement nor the suggestion solution.

@ujfalusi ujfalusi force-pushed the peter/sof/pr/ipc4_cpc_from_manifest_01 branch from e59d061 to 1c3f5a2 Compare April 3, 2023 11:34
@ujfalusi ujfalusi changed the title ASoC: SOF: ipc4: fetch CPC from manifest as fallback ASoC: SOF: ipc4: Fetch CPC from tplg mod_cfg or from manifest Apr 3, 2023
@ujfalusi
Copy link
Collaborator Author

ujfalusi commented Apr 3, 2023

Didn't get the problem statement nor the suggestion solution.

Updated the cover letter with relevant background information.

@ujfalusi
Copy link
Collaborator Author

ujfalusi commented Apr 3, 2023

Changes since v1:

  • limit the manifest parsing to CPC lookup to clarify the functionality
  • Add support for tplg mod_cfg parsing

Related sof/topology2 PR: thesofproject/sof#7348

@ujfalusi ujfalusi force-pushed the peter/sof/pr/ipc4_cpc_from_manifest_01 branch from 1c3f5a2 to ee3c4eb Compare April 4, 2023 05:41
@ujfalusi
Copy link
Collaborator Author

ujfalusi commented Apr 4, 2023

Changes since v2:

  • Drop the "fallback" calculation of CPC , leave it 0 if it cannot be acquired from any of the sources
  • Rename one missed struct name (ipc4_audio_fmt_num_tokens -> ipc4_module_params_num_tokens)
  • Print the final module ibs/obs/cpc numbers unconditionally
  • Use consistent order in printing: ibs, obs, cpc
  • The tplg mod_cfg array reading is fixed (thanks to @ranj063 and @jsarha)

Copy link

@jsarha jsarha left a comment

Choose a reason for hiding this comment

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

Hmm, shouldn't the logic go so that if we do find an exact ibs & obs match from topology, then the manifest entry does not override it any more? Or have I misunderstood how this should go? At least the commit description in "ASoC: SOF: ipc4-loader/topology: Update CPC value from manifest if missing" suggest otherwise.

In other words, which setting has the priority, if the ibs&obs match is found from the both in the manifest and in the topology?

@ujfalusi
Copy link
Collaborator Author

ujfalusi commented Apr 4, 2023

Hmm, shouldn't the logic go so that if we do find an exact ibs & obs match from topology, then the manifest entry does not override it any more? Or have I misunderstood how this should go? At least the commit description in "ASoC: SOF: ipc4-loader/topology: Update CPC value from manifest if missing" suggest otherwise.

This is the logic.

In other words, which setting has the priority, if the ibs&obs match is found from the both in the manifest and in the topology?

the priority of the CPC is:
CPC is set in topology for the module as a single override value
CPC can be found in tplg mod_cfg with exact match based on obs/ibs
CPC can be found in manifest mod_cfg with exact match based on obs/ibs
Maximum CPC value for the module from topology mod_cfg
Maximum CPC value for the module from manifest mod_cfg

* @sdev: SOF device
* @fw_module: pointer struct sof_ipc4_fw_module to parse
* @basecfg: Pointer to the base_config to update
* @exact_match: Only consider exact matches based on obs/ibs
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Stale line about the removed CPC fallback calculation, needs to be removed.

}

dev_dbg(sdev->dev, "%s: CPC from manifest: %u\n",
fw_module->man4_module_entry.name, cpc_pick);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Stale line about the removed CPC fallback calculation, needs to be removed.

@ujfalusi
Copy link
Collaborator Author

ujfalusi commented Apr 4, 2023

@jsarha, the check is within the functions to return if the basecfg->cpc is not 0, so they will not override it. I wanted to avoid excess amount of if (base_config->cpc) goto out; at every step in ipc4-topology.c

@greg-intel
Copy link

SOFCI TEST

jsarha
jsarha previously approved these changes Apr 5, 2023
Copy link

@jsarha jsarha left a comment

Choose a reason for hiding this comment

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

About the problem statement and the solution, there may be gaps to fill, but code-wise I could not find anything wrong.

@ujfalusi ujfalusi requested a review from RanderWang as a code owner April 18, 2023 13:01
@ujfalusi
Copy link
Collaborator Author

Changes since v3:

  • Use step-by step cpc validity check fro better documentation in sof_ipc4_init_base_module_cfg() as suggested by @jsarha
  • Add final fallback to platform's maximum CPS number (iow, maximum core frequency), which is stored per platform family
  • If this is not available either, assume that the DSP can run up to 400MHz (all supported Intel DSPs are running at this speed).

@ujfalusi ujfalusi force-pushed the peter/sof/pr/ipc4_cpc_from_manifest_01 branch from 760b591 to 6376515 Compare April 18, 2023 13:08

/* Update base_config->cpc from the module manifest */
sof_ipc4_update_cpc_from_manifest(sdev, swidget->module_info, base_config);
/* Update base_config->cpc from the module manifest - direct match */
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

comment needs update for the two sof_ipc4_update_cpc_from_tplg() call.

@ujfalusi ujfalusi force-pushed the peter/sof/pr/ipc4_cpc_from_manifest_01 branch from 6376515 to 6c4fc21 Compare April 18, 2023 18:25
…module

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

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…module

Save a pointer to the firmware module configuration area in the manifest
for each module which has at least one for later use.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…ssing

If the CPC value is not specified for the module in topology, try to fetch
it from the manifest's firmware module configuration section.

If the CPC value is not valid (0) in there also then use the ibs*obs/1000
formula to avoid passing 0 to firmware.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
…ic name

Currently we are using the term available_audio_format for structures which
only contains the module's available input and output formats.
This can be seen as a common (as it is common among  all module types)
module parameters storage.

In order to be able to extend the generic/common module information, renmae
the sof_ipc4_available_audio_format to sof_ipc4_module_params.

Rename the (linux only) token identifier from SOF_AUDIO_FMT_NUM_TOKENS to
SOF_MODULE_PARAM_NUM_TOKENS.

No change in functionality apart from the renaming.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Add token information for OBS, IBS, CPC as module configuration parameters
and a token for the number of configuration entries for the module.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
The topology file can contain a mod_cfg array with obs/ibs/cpc numbers to
override the information present in the manifest.

This patch adds the code to query the mod_cfg from topology and use it if
needed.

The CPC value will be set on the first matching case of:
CPC is set in topology for the module as a single override value
CPC can be found in tplg mod_cfg with exact match based on obs/ibs
CPC can be found in manifest mod_cfg with exact match based on obs/ibs
Maximum CPC value for the module from topology mod_cfg
Maximum CPC value for the module from manifest mod_cfg

If the CPC is still 0, use a fallback calculation of cpc = obs * ibs / 1000

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
The max_kcps represents the maximum number of cycles per second the DSP is
capable of executing, iow it is equal to the maximum core frequency.

The KCPS value can be used in different ways:
- there is a message (not implemented yet) which can force the DSP run at
minimum of a requested KCPS frequency

- in case the CPC value is missing for a module we can use the max_kcps
to force the DSP to run at full speed to avoid starvation.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
Set the max_kcps to 400000 on all platforms as all of the DSPs are running
maximum of 400MHz.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
… CPC

If we cannot retrieve non 0 value for the module's CPC, use the paltform's
maximum CPS number to force the DSP to full speed to avoid underclocking
it.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
@ujfalusi
Copy link
Collaborator Author

Changes since v4:

  • Fix the comment around sof_ipc4_update_cpc_from_tplg()

Copy link

@RanderWang RanderWang left a comment

Choose a reason for hiding this comment

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

LGTM

@ujfalusi
Copy link
Collaborator Author

SOFCI TEST

struct snd_pcm_hw_params *params,
struct sof_ipc4_copier *ipc4_copier)
{
struct sof_ipc4_pin_format *pin_fmts = ipc4_copier->available_fmt.input_pin_fmts;
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo in commit "renmae"

return ret;
}

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

where are we doing the "use the ibs*obs/1000 formula" in this patch?

@ujfalusi
Copy link
Collaborator Author

Closing, replaced by #4315

@ujfalusi ujfalusi closed this Apr 20, 2023
@ujfalusi ujfalusi deleted the peter/sof/pr/ipc4_cpc_from_manifest_01 branch November 30, 2023 06:54
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