ASoC: SOF: ipc4-topology: implement KCPS adjustment #4213

lyakh · 2023-02-23T11:56:38Z

The SOF already supports the REGISTER_KCPS IPC, this patch also adds support for it to the driver. A new topology token is used for this, which specified the KPCS requirement for each pipeline.

Do I need to bump up any ABI version numbers for this?

ujfalusi · 2023-02-23T12:00:39Z

sound/soc/sof/ipc4-topology.c

Why __s32 and not just s32?

because it's an external parameter, a part of an ABI

ujfalusi · 2023-02-23T12:04:22Z

sound/soc/sof/ipc4-topology.c

You also need to add to the extension:
SOF_IPC4_MOD_EXT_MSG_SIZE(sizeof(adjust))

or you can also use the dedicated function to do a proper LARGE_CONFIG, see #4212.
Since you are always going to send a single u32, it might be justified to have it open coded, but in general I prefer to use the function which exists for the purpose.

oh... sof_ipc4_set_get_data() is a kind of a monster... In general - yes, I'd also rather use a proper abstraction, so... But well, I still think it's better to use the accessor. Thanks!

Yes, it is a monster, but it adds the benefit that we use common code and we can also dump the payload as hex in one place.
It is good to have these handled in one path to help debugging things.

ujfalusi · 2023-02-23T12:09:03Z

sound/soc/sof/ipc4-topology.c

Well, yes and no, it is used to track the payload size in the mailbox, it is used in the IPC low level code to decide if we need to copy something to there or not.
Can you drop this comment?

I think it's the msg.data_size, that's used for that in the IPC4 case, not the last parameter to sof_ipc_tx_message_no_reply(), but sure, I can drop it

Yes, which is coming from this size parameter:

linux/sound/soc/sof/ipc.c

Line 57 in cff45ee

msg->msg_size = msg_bytes;

I stand corrected - I was looking at an older kernel version. Now it's used indeed!

ujfalusi · 2023-02-23T12:13:04Z

sound/soc/sof/ipc4-topology.c

Why not make the sof_ipc4_pipeline_send_kcps() handle the !kcps_adjust internally and then you can have this implemented in a more optimistic way:

if (!ret) { ret = sof_ipc4_pipeline_send_kcps(sdev, swidget, kcps_adjust); if (ret) { dev_err(sdev->dev, "failed to increase KCPS: %d, continuing\n", ret); ret = 0; } else { /* the error case */ } }

I was comparing the two options - where to put the check for 0 KCPS adjustment, and in the end I chose this option. It's a really small addition outside and makes it clear immediately at this level, whereas in the function - you'd be calling it just to return immediately. So, unless you feel strongly about this, I'd keep it as is

I don't feel strongly about this, just it would make the caller sites cleaner, less number of if ().

ujfalusi · 2023-02-23T12:13:20Z

sound/soc/sof/ipc4-topology.c

Then this is a dev_warn()

yeah, dev_err() would be called by the CI, right? In fact I'm not even sure - should we make this a failure or not?

if it is an error, then we should treat it as an error, here we are ignoring it, so it is a warning, imho

ujfalusi · 2023-02-23T12:39:56Z

The SOF already supports the REGISTER_KCPS IPC, this patch also adds support for it to the driver. A new topology token is used for this, which specified the KPCS requirement for each pipeline.

Do I need to bump up any ABI version numbers for this?

No need for ABI version update (and it is not really defined for IPC4 anyways). The KCPS update is an existing FW feature, we just missed it.

lyakh · 2023-02-23T12:56:34Z

The SOF counterpart is thesofproject/sof#7160

ujfalusi

The open coded IPC message crafting is not correct (I would use the ops for it to do the right thing) and there is the wrong comment about the last parameter of sof_ipc_tx_message_no_reply()

ranj063 · 2023-02-23T16:00:40Z

sound/soc/sof/sof-audio.h

this is a very ipc4 specific parameter, can we put it somewhere in one of the ipc4 structs instead of swidget?

moved, thanks

plbossart

Rather lost of what this kpcs token means, and what exactly it changes.

plbossart · 2023-02-23T20:12:58Z

sound/soc/sof/ipc4-topology.c

it's the first time I hear about a pipeline-level cycle count. We already have a module 'cpc' (cycle per chunk) and 'cps" *cycles per second", so it's not clear to me how those 3 tokens would be used.

why we need kcps ? In current 0004 branch, kcps is built from cpc and we already have cpc in module now. Why do we need to add kcps ?

int kcps = cd->cpc * 1000 / ppl_data->p->period;

In 0004 branch, kcps is built from each module in pipeline with the above algorithm, why we need pipeline kcps ?

data.start = p->source_comp; data.p = p; walk_ctx.comp_func = add_pipeline_cps_consumption; if(!clocks_handled) ret = walk_ctx.comp_func(p->source_comp, NULL, &walk_ctx, PPL_DIR_DOWNSTREAM);

plbossart · 2023-02-23T20:14:51Z

sound/soc/sof/ipc4-topology.c

what does 'adjust' mean? is this to specify a kcycles value, or alter it from a baseline read in topology?

Completely unclear what this is about.

I can call it "delta" if that would help

or kcps_change?

if you call it 'delta' then the function should be not sof_ipc4_pipeline_send_kcps(), prehaps: sof_ipc4_send_kcps_update()?

plbossart · 2023-02-23T20:16:47Z

sound/soc/sof/ipc4-topology.c

I see nothing that's pipeline-specific here, I really don't understand what 'adjust' means here. If the message destination is the base firmware module how would one change pipeline Y ?

IIUC it doesn't matter which specific pipeline requests the change in clock rate, we just add up all pipelines and configure the clock with the sum

plbossart · 2023-02-23T20:18:41Z

sound/soc/sof/sof-audio.h

int or s32?

@plbossart the value that's sent out has to be s32, because it can be negative for a negative adjustment, and it has to be 32 bits because it's a part of an ABI. But here we're storing a generic integer unsigned value, so I think unsigned int is appropriate here.

lgirdwood · 2023-02-24T09:48:22Z

Base FW today supports a global per core DSP frequency manager to select the most power efficient clock based on DSP core loading. Today it uses the following rules

The FW frequency manager makes its frequency selection based on the kcps load value for each core.
The driver sends new kcps delta values to base FW for each pipeline loaded/unloaded (or start/stop)
The driver knows the cps value for each module from the ext manifest (these are non accurate test values that the driver is not reading today). Driver should sum these cps values and convert to a kcps when loading a pipeline with these modules.
The driver also now knows the per pipeline kcps value from topology which if exists can override the manifest cps values (this is this PR).

Rules 1, 2, & 3 are compatibility, rule 4 is extended feature improvement on Linux. Rule 3 is not implemented in driver today and has no tooling to capture per module cps or to regenerate signed FWs with new ext manifest cps data.

Long term the DSP FW should be responsible to monitor the per core loading with hints from driver (manifest, topology) and adjust frequency as necessary.

The SOF already supports the REGISTER_KCPS IPC, this patch also adds support for it to the driver. A new topology token is used for this, which specified the KCPS requirement for each pipeline. When a pipeline is created, that had a KCPS value specified for it in the topology, that value is sent to the firmware for clock adjustment. When such a pipeline is destroyed, the same negative value is sent to return the system clock rate to the original state. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

sound/soc/sof/ipc4-topology.c

ujfalusi

I would still hide the delta_kcps check fro 0 in sof_ipc4_send_kcps(), but it is probably just a matter of taste.

plbossart · 2023-02-27T14:46:28Z

I still don't get the relationship between pipeline and cores. We know that with the introduction of the DP domain the pipeline can be made of modules that are assigned on different cores. So how would the pipeline 'adjust' value be computed in this case?

Lost in space, and it's Monday 9:45am. Oh well.

RanderWang · 2023-02-28T01:51:13Z

I still don't know why we need kcps and how does fw support this. currently the FW algorithm is built based on module cpc. And I also found that cpc value in fw extended manifest was refined(mtl: add measured cps and cpc values). It seems that kernel driver should get cpc value from module extended config and send it to FW.

lyakh · 2023-02-28T07:12:46Z

I still don't get the relationship between pipeline and cores. We know that with the introduction of the DP domain the pipeline can be made of modules that are assigned on different cores. So how would the pipeline 'adjust' value be computed in this case?

@plbossart with this PR no KCPS calculations are done in the kernel. This PR does a very simple thing: takes values from the topology and sends them to the firmware.
As for different cores - I'm not sure we know that. We discussed that multiple times, also including in the DP PR, IIRC. And there as well I commented, that that isn't supported so far and it has been decided, that the DP doesn't have to run different modules on different cores, so as long as that isn't supported, we should avoid that. Note, that we have the same situation right now already - nothing prevents one from creating a topology, assigning connected pipelines or components on those pipelines to different cores. We just know, that that isn't supported and doesn't work, so we don't do that...

plbossart · 2023-03-06T16:53:52Z

too many double-negatives in your answer @lyakh. The point is that if a component is assigned to a different core the entire logic looks wrong. Either we accept component-level core assignment or we don't.

lgirdwood · 2023-03-13T17:08:42Z

sound/soc/sof/ipc4-topology.c

 		offsetof(struct sof_ipc4_pipeline, use_chain_dma)},
 	{SOF_TKN_SCHED_CORE, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32,
 		offsetof(struct sof_ipc4_pipeline, core_id)},
+	{SOF_TKN_SCHED_PIPELINE_KCPS, SND_SOC_TPLG_TUPLE_TYPE_WORD, get_token_u32,


@lyakh, @ujfalusi and @jsarha I had a chat with @plbossart and can we modify this attribute to be per module i.e. needs to be in the base class of each module. This would then allow us to mix scheduling types in a pipeline and scale across cores.

Thinking further, can we also extend this to include format and be an array i.e.

{SOF_TKN_SCHED_MODULE_KCPS_S16LE, SND_SOC_TPLG_TUPLE_TYPE_ARRAY, get_token_u32_array,

Where the array index would be the channel count + 1. i.e. when defined in topology

1000, 2000, 0, 4000, 0, 0, 0, 0

This would then specify 1000 kcps for mono S16_LE, 2000kcps for stereo S16_LE, 3 channel S16_LE not known (can use platform max default), and so on.

IIRC the CPC/CPS are kind of worst-case figures. They are not meant to be adjusted depending on channel count or input/output formats. We're probably looking at one broadbrush estimate for each module and correction factor to account for the known unknowns (cache, IPC, solar eclipses and neutrinos).

kv2019i · 2023-03-14T10:13:00Z

Clarification from FW is that kernel KCPS IPC should only sent in special cases to override the values on module config (defined in toml files). The implementation is not complete in SOF main but there is an early version available, see thesofproject/sof#7267

plbossart · 2023-03-14T12:58:21Z

Clarification from FW is that kernel KCPS IPC should only sent in special cases to override the values on module config (defined in toml files). The implementation is not complete in SOF main but there is an early version available, see thesofproject/sof#7267

Not clear if the ask is to override the CPC/CPS values for a module (with the updated value impacting all pipelines with a module instance), for a module instance (with impact limited to the pipeline in which it is inserted), or for a pipeline containing a module instance (directly changing the total maintained at the pipeline level - if this makes sense).

plbossart · 2023-04-17T20:23:42Z

@ranj063 @lyakh @kv2019i @ujfalusi I've lost track of this PR status, is this still needed as is, or evolutions needed? Or is this related to the CPC stuff that @ujfalusi was working on in #4262.

Bear with me, we have too many PRs in flight that need to be merged or closed.

lyakh · 2023-04-18T10:21:59Z

@ranj063 @lyakh @kv2019i @ujfalusi I've lost track of this PR status, is this still needed as is, or evolutions needed? Or is this related to the CPC stuff that @ujfalusi was working on in #4262.

@plbossart as far as I understand we decided, that we don't want KCPS set from the topology and instead we want a user-activated trigger to increase the clock and maybe also some situations where this will be done automatically

ujfalusi · 2023-04-18T10:54:02Z

Yes, we will need some place to store the magical KCPS for a platform and send that with a kcontrol.
afaik the KCPS == maximum core kHz, so we need to gather those for all platforms and store it somewhere, probably in sof_dev_desc.
I'm not sure if the KCPS is ncores * max_core_kHz.

plbossart · 2023-04-18T15:57:18Z

Yes, we will need some place to store the magical KCPS for a platform and send that with a kcontrol. afaik the KCPS == maximum core kHz, so we need to gather those for all platforms and store it somewhere, probably in sof_dev_desc. I'm not sure if the KCPS is ncores * max_core_kHz.

Kcontrols are typically exposed in topology though...

debugfs sounds like a better place for this kind of things. We should not use kcontrol IMHO.

ujfalusi · 2023-04-19T12:51:37Z

Yes, we will need some place to store the magical KCPS for a platform and send that with a kcontrol. afaik the KCPS == maximum core kHz, so we need to gather those for all platforms and store it somewhere, probably in sof_dev_desc. I'm not sure if the KCPS is ncores * max_core_kHz.

#4262 added the fallback platfomr KCPS support, we can use that number for the 'boost'

Kcontrols are typically exposed in topology though...

debugfs sounds like a better place for this kind of things. We should not use kcontrol IMHO.

debugfs would be really simple for sure, but I think the issue is that this has to be exposed to user space and debugfs is not a good place for UCM to touch it for example.
The kcontrol cannot come from topology as this needs special handling, it is not attached to any firmware module, it is a different message to be sent targeting the basefw.
Code-wise it is not a big task, but figuring out a correct place to create that kcontrol is not that straight forward, we need to find the correct hook and have the needed information available at that point.
Likely at the end of the tplg loading, but I don't like that piggy-backing too much.

plbossart · 2023-05-04T17:50:14Z

this PR is no longer aligned with our plans for MTL. We will first enable the parsing of CPC information from the manifest with #4338, and move all efforts to override the MCPS budget to a later point after revisiting the API for clock scaling with our firmware friends.

Let's close this for now.

lyakh requested review from ajitkupandey, bardliao, dbaluta, juimonen, kuanhsuncheng, kv2019i, lgirdwood, plbossart, ranj063, singalsu, ujfalusi and yaochunhung as code owners February 23, 2023 11:56

ujfalusi reviewed Feb 23, 2023

View reviewed changes

ujfalusi requested changes Feb 23, 2023

View reviewed changes

lyakh force-pushed the kcps branch from ec571f1 to a223fa0 Compare February 23, 2023 13:28

ranj063 reviewed Feb 23, 2023

View reviewed changes

plbossart reviewed Feb 23, 2023

View reviewed changes

lyakh force-pushed the kcps branch from a223fa0 to ce13062 Compare February 24, 2023 11:30

ujfalusi reviewed Feb 24, 2023

View reviewed changes

sound/soc/sof/ipc4-topology.c Show resolved Hide resolved

ujfalusi approved these changes Feb 24, 2023

View reviewed changes

ranj063 approved these changes Feb 25, 2023

View reviewed changes

kv2019i mentioned this pull request Mar 8, 2023

platform: mtl: lowest clock as default thesofproject/sof#7233

Closed

lgirdwood reviewed Mar 13, 2023

View reviewed changes

RanderWang approved these changes Apr 20, 2023

View reviewed changes

plbossart closed this May 4, 2023

ASoC: SOF: ipc4-topology: implement KCPS adjustment #4213

ASoC: SOF: ipc4-topology: implement KCPS adjustment #4213

Uh oh!

Conversation

lyakh commented Feb 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi commented Feb 23, 2023

Uh oh!

lyakh commented Feb 23, 2023

Uh oh!

ujfalusi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RanderWang Feb 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi Feb 23, 2023 •

edited

Loading

RanderWang Feb 27, 2023 •

edited

Loading

ujfalusi Feb 24, 2023 •

edited

Loading

lyakh Feb 24, 2023 •

edited

Loading