| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a23d759a-9048-42e8-b72d-34c1e913e7fe@quicinc.com>
Date: Wed, 11 Jun 2025 09:46:46 +0800
From: Renjiang Han <quic_renjiang@...cinc.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Vikash Garodia
<quic_vgarodia@...cinc.com>,
Dikshita Agarwal <quic_dikshita@...cinc.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>
CC: <linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] media: venus: pm_helpers: use opp-table for the frequency
On 6/11/2025 3:01 AM, Bryan O'Donoghue wrote:
> On 10/06/2025 10:13, Renjiang Han wrote:
>> The frequency values in the opp-table of the device tree and the
>> freq_tbl
>> in the driver are the same. So the driver can choose either table for
>> freq
>> values.
>
> I'd completely drop this sentence as you basically contradict it in
> the next line.
>
>> However,
>
> Drop
>
> some platforms (such as qcs615 and sc7180) use the same core but
>> have different frequency tables. Using the opp-table allows us to
>> separate
>> the core description from the frequency data and supports the use of
>> fallback compatibles.
>
> This is a bit better.
>
> Basically you can have identical bindings, even identical compats but
> for reasons such as binning of silicon parts you might run at higher
> or lower frequencies so static tables in the driver are not
> appropriate and are better represented in the DT.
>
>
>> Reviewed-by: Vikash Garodia <quic_vgarodia@...cinc.com>
>> Signed-off-by: Renjiang Han <quic_renjiang@...cinc.com>
>> ---
>> The frequency values in the opp-table of the device tree and the
>> freq_tbl
>> in the driver are the same. So the driver can choose either table for
>> freq
>> values.
>>
>> However, some platforms (such as qcs615 and sc7180) use the same core
>> but
>> have different frequency tables. Using the opp-table allows us to
>> separate
>> the core description from the frequency data and supports the use of
>> fallback compatibles.
>>
>> Therefore, it is necessary to update pm_helpers.c to use the frequency
>> values from the opp-table for the v4 core.
>>
>> Note:
>> Earlier discussion was concluded in [1] where it was agreed to rely on
>> opp-table for QCS615.
>>
>> [1]
>> https://lore.kernel.org/linux-arm-msm/c9b83c8b-68d1-43bc-99d6-d2d2b9e445f4@oss.qualcomm.com/
>> ---
>> drivers/media/platform/qcom/venus/pm_helpers.c | 58
>> +++++++++++++++-----------
>> 1 file changed, 34 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c
>> b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index
>> 409aa9bd0b5d099c993eedb03177ec5ed918b4a0..8dd5a9b0d060cddfeafd4da477ade0c7aeb6c390
>> 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -41,16 +41,14 @@ 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;
>> + struct device *dev = core->dev;
>> + unsigned long freq = 0;
>> + 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);
>> + dev_pm_opp_put(opp);
>> for (i = 0; i < res->clks_num; i++) {
>> if (IS_V6(core)) {
>> @@ -636,7 +634,9 @@ static int decide_core(struct venus_inst *inst)
>> u32 min_coreid, min_load, cur_inst_load;
>> u32 min_lp_coreid, min_lp_load, cur_inst_lp_load;
>> 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;
>> int ret = 0;
>> if (legacy_binding) {
>> @@ -659,7 +659,8 @@ 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);
>> + 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);
>> @@ -949,7 +950,10 @@ static int core_resets_get(struct venus_core *core)
>> static int core_get_v4(struct venus_core *core)
>> {
>> struct device *dev = core->dev;
>> + const struct freq_tbl *freq_tbl = core->res->freq_tbl;
>> + unsigned int num_rows = core->res->freq_tbl_size;
>> const struct venus_resources *res = core->res;
>> + unsigned int i;
>> int ret;
>> ret = core_clks_get(core);
>> @@ -986,9 +990,17 @@ static int core_get_v4(struct venus_core *core)
>> if (core->res->opp_pmdomain) {
>> ret = devm_pm_opp_of_add_table(dev);
>> - if (ret && ret != -ENODEV) {
>> - dev_err(dev, "invalid OPP table in device tree\n");
>> - return ret;
>> + if (ret) {
>> + if (ret == -ENODEV) {
>> + for (i = 0; i < num_rows; i++) {
>> + ret = dev_pm_opp_add(dev, freq_tbl[i].freq, 0);
>> + if (ret)
>> + return ret;
>> + }
>> + } else {
>> + dev_err(dev, "invalid OPP table in device tree\n");
>> + return ret;
>> + }
>> }
>> }
>> @@ -1078,11 +1090,11 @@ static unsigned long
>> calculate_inst_freq(struct venus_inst *inst,
>> static int load_scale_v4(struct venus_inst *inst)
>> {
>> struct venus_core *core = inst->core;
>> - const struct freq_tbl *table = core->res->freq_tbl;
>> - unsigned int num_rows = core->res->freq_tbl_size;
>> struct device *dev = core->dev;
>> unsigned long freq = 0, freq_core1 = 0, freq_core2 = 0;
>> + unsigned long max_freq = ULONG_MAX;
>> unsigned long filled_len = 0;
>> + struct dev_pm_opp *opp;
>> int i, ret = 0;
>> for (i = 0; i < inst->num_input_bufs; i++)
>> @@ -1108,20 +1120,18 @@ 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);
>> + 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);
>> + dev_pm_opp_put(opp);
>> set_freq:
>>
>> ---
>> base-commit: b27cc623e01be9de1580eaa913508b237a7a9673
>> change-id: 20250610-use_freq_with_opp_table-b81162cfecba
>>
>> Best regards,
> Please make your commit log more succinct and precise then add.
Sure, thanks for your comment. I'll update this with next version.
>
> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
>
> ---
> bod
--
Best Regards,
Renjiang
Powered by blists - more mailing lists