Re: [PATCH v4 2/4] media: venus: core: use opp-table for the frequency

[Renjiang Han <quic_renjiang@quicinc.com>]

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@quicinc.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