Repository navigation
ASoC: SOF: ipc4-topology: Add module parameter to ignore the CPC value - #4531
Conversation
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Naming is hard, so I have one alternative in the comment. Let's see what others think. I'm good with the original name (end-users will anyways need some context to use this option).
| static bool force_cpc_to_zero; | ||
| module_param_named(ipc4_force_cpc_to_zero, force_cpc_to_zero, bool, 0444); | ||
| MODULE_PARM_DESC(ipc4_force_cpc_to_zero, | ||
| "Ignore CPC value for modules and force it to be 0."); |
There was a problem hiding this comment.
This is a bit hard for outsiders as you'd have to know that zero has special meaning to make any sense of this. Additionally CPC should be explains somehow.
But, but, I don't really have very good alternatives. One solution:
MODULE_PARM_DESC(ipc4_ignore_fw_cpc,
"Ignore the cycle budget (CPC) data for modules"
There was a problem hiding this comment.
This is a bit hard for outsiders as you'd have to know that zero has special meaning to make any sense of this. Additionally CPC should be explains somehow.
Are we going to document IPC4, clock scaling, clock sources and all of the stuff around CPC?
But, but, I don't really have very good alternatives. One solution:
MODULE_PARM_DESC(ipc4_ignore_fw_cpc, "Ignore the cycle budget (CPC) data for modules"
CPC == Cycle Budget?
Initially I had this: ipc4_ignore_cpc
My problem was and still is (after I thought about it) that the 'ignore' implies that we would do, use, rely on a 'cpc' value, but we are just reading it from manifest (at the moment, but we might have topology override and in multiple levels).
So we should have 'ipc4_ignore_queried_cpc_value', but ignoring does not tell that we are going to send 0 or cpc_max or 42.
I'm not sure if the added 'fw' helps in this.
Sure, let's roll a dice, I'm fine with whatever name ;)
There was a problem hiding this comment.
inline comments are free here, so we can be a little more verbose and say "... in other words forces DSP clock to max frequency"
There was a problem hiding this comment.
"force_cpc_to_zero" can only be understood if the reviewer knows that this zero value disables clock scaling.
If we want to avoid a description, 'ignore' is better IMHO. ipc4_ignore_fw_manifest_cpc ?
There was a problem hiding this comment.
it is going to ignore any CPC regardless of source and it might include in the future CPC from topology.
Should we go with a really generic ipc4_ignore_cpc.
I don't think we can expect any user to know what CPC is and what it does by the name of the parameter.
| static bool force_cpc_to_zero; | ||
| module_param_named(ipc4_force_cpc_to_zero, force_cpc_to_zero, bool, 0444); | ||
| MODULE_PARM_DESC(ipc4_force_cpc_to_zero, | ||
| "Ignore CPC value for modules and force it to be 0."); |
There was a problem hiding this comment.
inline comments are free here, so we can be a little more verbose and say "... in other words forces DSP clock to max frequency"
| #include "ipc4-topology.h" | ||
| #include "ops.h" | ||
|
|
||
| static bool force_cpc_to_zero; |
There was a problem hiding this comment.
Commit messsage:
Add a new module parameter ipc4_force_cpc_to_zero which can be used to
force the kernel to ignore the queried CPC value for the given module
and send 0 instead.
"module" has two meanings in this sentence, we are talking about kernel module and firmware module, not the same thing.
Also this ignores ALL values for ALL firmware modules, there is no selectivity or granularity as hinted by "the given module".
The flag is intended to be used only when there is a need to validate the
firmware behavior regarding to clock scaling since the 0 CPC value will
force the DSP to run in full speed.
this patch does the exact opposite, it prevents the DSP from using clock scaling, it does not validate anything related to clock scaling.
There was a problem hiding this comment.
@plbossart, let me reword. It provides validation for clock scaling by disabling to provide additional data point to rule out clock scaling caused failure.
| static bool force_cpc_to_zero; | ||
| module_param_named(ipc4_force_cpc_to_zero, force_cpc_to_zero, bool, 0444); | ||
| MODULE_PARM_DESC(ipc4_force_cpc_to_zero, | ||
| "Ignore CPC value for modules and force it to be 0."); |
There was a problem hiding this comment.
"force_cpc_to_zero" can only be understood if the reviewer knows that this zero value disables clock scaling.
If we want to avoid a description, 'ignore' is better IMHO. ipc4_ignore_fw_manifest_cpc ?
83ec534 to
54a1cc6
Compare
|
Changes since v1:
|
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Commit and code don't match. Commit has "Add a new module parameter ipc4_force_cpc_to_zero " but code has "module_param_named(ipc4_ignore_cpc, ignore_cpc, bool, 0444);".
Otherwise looks good.
Add a new module parameter ipc4_ignore_cpc which can be used to force the kernel to ignore the queried CPC value for all firmware modules and use 0 instead. The CPC lookup is still done to report missing configurations and the debug print is going to be different to be explicit that the CPC is ignored and what was the value we would have used. The CPC value is sent to the firmware with the MOD_INIT_INSTANCE message and it is used by the firmware as a parameter for clock scaling. The flag is intended to be used only when there is a need to validate the firmware behavior regarding to clock scaling since the 0 CPC value will force the DSP to run in full speed, disabling the scaling and provides additional counter point to rule out clock management related issues. Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
54a1cc6 to
823ee45
Compare
|
Changes since v2:
|
5 participants
Add a new module parameter ipc4_force_cpc_to_zero which can be used to force the kernel to ignore the queried CPC value for the given module and send 0 instead.
The CPC lookup is still done to report missing configurations and the debug print is going to be different to be explicit that the CPC is ignored and what was the value we would have used.
The flag is intended to be used only when there is a need to validate the firmware behavior regarding to clock scaling since the 0 CPC value will force the DSP to run in full speed.