On Fri, Dec 13, 2024 at 03:26:47PM +0530, Renjiang Han wrote:
> Get frequency value from the opp-table of devicetree for the v4 core.
> For compatibility, if getting data from the opp table fails, the data
> in the frequency table will be used.
>
> The order of variable definitions is adjusted only to keep the reverse
> Christmas tree order coding style.
>
1) Do the best you can to add your variables while trying to maintain
the order, but if it's not possible better leave it than making it hard
to parse logical change from shuffling of code.
2) This comment is useful during review, but not necessarily so in the
git history, so I'd suggest to keep it below the '---' line.
> Signed-off-by: Renjiang Han <quic_renjiang@xxxxxxxxxxx>
> ---
> drivers/media/platform/qcom/venus/pm_helpers.c | 67 ++++++++++++++++++--------
> 1 file changed, 46 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> index 33a5a659c0ada0ca97eebb5522c5f349f95c49c7..a5c3f9ad2088d8c80247b52d5c1b8e053f499bfe 100644
> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> @@ -40,17 +40,23 @@ static int core_clks_get(struct venus_core *core)
>
> static int core_clks_enable(struct venus_core *core)
> {
> - const struct venus_resources *res = core->res;
> const struct freq_tbl *freq_tbl = core->res->freq_tbl;
> unsigned int freq_tbl_size = core->res->freq_tbl_size;
> - unsigned long freq;
> + const struct venus_resources *res = core->res;
> + struct device *dev = core->dev;
> + unsigned long freq = 0;
Is it really necessary to initialize this? I'd expect that
dev_pm_opp_find_freq_ceil() would either initialize freq or return a
failure, in which case you assign freq.
Perhaps the compiler isn't clever enough to see this?
> + struct dev_pm_opp *opp;
> unsigned int i;
> int ret;
>
> - if (!freq_tbl)
> - return -EINVAL;
> -
> - freq = freq_tbl[freq_tbl_size - 1].freq;
> + opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> + if (IS_ERR(opp)) {
> + if (!freq_tbl)
> + return -EINVAL;
> + freq = freq_tbl[freq_tbl_size - 1].freq;
> + } else {
> + dev_pm_opp_put(opp);
> + }
>
> for (i = 0; i < res->clks_num; i++) {
> if (IS_V6(core)) {
> @@ -627,12 +633,15 @@ min_loaded_core(struct venus_inst *inst, u32 *min_coreid, u32 *min_load, bool lo
>
> static int decide_core(struct venus_inst *inst)
> {
> + const struct freq_tbl *freq_tbl = inst->core->res->freq_tbl;
> const u32 ptype = HFI_PROPERTY_CONFIG_VIDEOCORES_USAGE;
> - struct venus_core *core = inst->core;
> - u32 min_coreid, min_load, cur_inst_load;
> u32 min_lp_coreid, min_lp_load, cur_inst_lp_load;
> + u32 min_coreid, min_load, cur_inst_load;
> + struct venus_core *core = inst->core;
> struct hfi_videocores_usage_type cu;
> - unsigned long max_freq;
> + unsigned long max_freq = ULONG_MAX;
> + struct device *dev = core->dev;
> + struct dev_pm_opp *opp;
Here the line shuffling makes it hard to determine what is part of the
logical change and what is just style changes...
Regards,
Bjorn
> int ret = 0;
>
> if (legacy_binding) {
> @@ -655,7 +664,11 @@ static int decide_core(struct venus_inst *inst)
> cur_inst_lp_load *= inst->clk_data.low_power_freq;
> /*TODO : divide this inst->load by work_route */
>
> - max_freq = core->res->freq_tbl[0].freq;
> + opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
> + if (IS_ERR(opp))
> + max_freq = freq_tbl[0].freq;
> + else
> + dev_pm_opp_put(opp);
>
> min_loaded_core(inst, &min_coreid, &min_load, false);
> min_loaded_core(inst, &min_lp_coreid, &min_lp_load, true);
> @@ -1073,12 +1086,14 @@ static unsigned long calculate_inst_freq(struct venus_inst *inst,
>
> static int load_scale_v4(struct venus_inst *inst)
> {
> + const struct freq_tbl *table = inst->core->res->freq_tbl;
> + unsigned int num_rows = inst->core->res->freq_tbl_size;
> + unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0;
> struct venus_core *core = inst->core;
> - const struct freq_tbl *table = core->res->freq_tbl;
> - unsigned int num_rows = core->res->freq_tbl_size;
> + unsigned long max_freq = ULONG_MAX;
> struct device *dev = core->dev;
> - unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0;
> unsigned long filled_len = 0;
> + struct dev_pm_opp *opp;
> int i, ret = 0;
>
> for (i = 0; i < inst->num_input_bufs; i++)
> @@ -1104,19 +1119,29 @@ static int load_scale_v4(struct venus_inst *inst)
>
> freq = max(freq_core1, freq_core2);
>
> - if (freq > table[0].freq) {
> - dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n",
> - freq, table[0].freq);
> + opp = dev_pm_opp_find_freq_floor(dev, &max_freq);
> + if (IS_ERR(opp))
> + max_freq = table[0].freq;
> + else
> + dev_pm_opp_put(opp);
>
> - freq = table[0].freq;
> + if (freq > max_freq) {
> + dev_dbg(dev, VDBGL "requested clock rate: %lu scaling clock rate : %lu\n",
> + freq, max_freq);
> + freq = max_freq;
> goto set_freq;
> }
>
> - for (i = num_rows - 1 ; i >= 0; i--) {
> - if (freq <= table[i].freq) {
> - freq = table[i].freq;
> - break;
> + opp = dev_pm_opp_find_freq_ceil(dev, &freq);
> + if (IS_ERR(opp)) {
> + for (i = num_rows - 1 ; i >= 0; i--) {
> + if (freq <= table[i].freq) {
> + freq = table[i].freq;
> + break;
> + }
> }
> + } else {
> + dev_pm_opp_put(opp);
> }
>
> set_freq:
>
> --
> 2.34.1
>
