Re: [PATCH v8 1/3] media: venus: pm_helpers: use opp-table for the frequency

[Renjiang Han <quic_renjiang@quicinc.com>]

On 5/31/2025 4:26 AM, Dmitry Baryshkov wrote:
> On Fri, May 30, 2025 at 09:32:13AM +0530, Renjiang Han wrote:
>> The frequency value in the opp-table in the device tree and the freq_tbl
>> in the driver are the same.
>>
>> Therefore, update pm_helpers.c to use the opp-table for frequency values
>> for the v4 core.
> You are kind of missing the linking between the first two sentences. "The
> tables are the same, so use the second one." You need to explain that
> some of the platforms (provide examples) use the same core, but
> different frequency tables. Using OPP tables allows us to abstract core
> description from the frequency data and use fallback compatibles.
OK. Thanks for your comment. I'll update this message in next version.
>
>> If getting data from the opp table fails, fall back to using the frequency
>> table.
>>
>> Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/venus/pm_helpers.c | 53 +++++++++++++++++++-------
>>   1 file changed, 39 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index 409aa9bd0b5d099c993eedb03177ec5ed918b4a0..434dd66076e8faf7f3feac6c29152789f8d2f81b 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -43,14 +43,20 @@ 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;
>> +	struct device *dev = core->dev;
>> +	struct dev_pm_opp *opp;
>>   	unsigned long freq;
>>   	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)) {
>> @@ -631,12 +637,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;
>>   	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 +668,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);
>> @@ -1082,7 +1095,9 @@ static int load_scale_v4(struct venus_inst *inst)
>>   	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,19 +1123,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