| 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: <70714bd9-e6a1-4104-b200-38f0019a107b@quicinc.com>
Date: Tue, 17 Dec 2024 13:32:19 +0800
From: Renjiang Han <quic_renjiang@...cinc.com>
To: Bjorn Andersson <bjorn.andersson@....qualcomm.com>
CC: Stanimir Varbanov <stanimir.k.varbanov@...il.com>,
Vikash Garodia
<quic_vgarodia@...cinc.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio
<konradybcio@...nel.org>,
Stanimir Varbanov <stanimir.varbanov@...aro.org>,
<linux-media@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/4] media: venus: core: use opp-table for the
frequency
On 12/17/2024 2:21 AM, Bjorn Andersson wrote:
> 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.
OK, thanks for pointing it out.
>
> 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.
Do you mean that the message about my code style changes only needs
to be added to the cover letter?
>
>> Signed-off-by: Renjiang Han <quic_renjiang@...cinc.com>
>> ---
>> 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.
Thanks for your review. There is really no need to initialize freq.
The default value of freq is 0 here. dev_pm_opp_find_freq_ceil() will
query the matching value from the opp table based on this default value.
>
> 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
You mean I only need to add variable definitions basing on the original
order of variable definitions, and I don't need to modify the order of
variable definitions specifically for the reverse Christmas tree code
style. Is that right?
>
>> 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
>>
--
Best Regards,
Renjiang
Powered by blists - more mailing lists