Re: [PATCH v4 1/2] venus: pm_helpers: add compatibility for dev_pm_genpd_set_hwmode on V4

[Renjiang Han <quic_renjiang@quicinc.com>]

On 5/29/2025 3:27 AM, Dmitry Baryshkov wrote:
> On Tue, Feb 18, 2025 at 04:03:20PM +0530, Renjiang Han wrote:
>> There are two ways to switch GDSC mode. One is to write the POWER_CONTROL
>> register and the other is to use dev_pm_genpd_set_hwmode(). However, they
>> rely on different clock driver flags. dev_pm_genpd_set_hwmode() depends on
>> the HW_CTRL_TRIGGER flag and POWER_CONTROL register depends on the HW_CTRL
>> flag.
>>
>> By default, the dev_pm_genpd_set_hwmode() is used to switch the GDSC mode.
>> If it fails and dev_pm_genpd_set_hwmode() returns -EOPNOTSUPP, it means
>> that the clock driver uses the HW_CTRL flag. At this time, the GDSC mode
>> is switched to write the POWER_CONTROL register.
>>
>> Clock driver is using HW_CTRL_TRIGGER flag with V6. So hwmode_dev is
>> always true on using V6 platform. Conversely, if hwmode_dev is false, this
>> platform must be not using V6. Therefore, replace IS_V6 in poweroff_coreid
>> with hwmode_dev. Also, with HW_CTRL_TRIGGER flag, the vcodec gdsc gets
>> enabled in SW mode by default. Therefore, before disabling the GDSC, GDSC
>> should be switched to SW mode so that GDSC gets enabled in SW mode in the
>> next enable.
>>
>> Signed-off-by: Renjiang Han <quic_renjiang@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/venus/core.h       |  2 ++
>>   drivers/media/platform/qcom/venus/pm_helpers.c | 38 ++++++++++++++------------
>>   2 files changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
>> index 43532543292280be15adf688fc0c30f44e207c7f..0ccce89d3f54cf685ecce5b339a51e44f6ea3704 100644
>> --- a/drivers/media/platform/qcom/venus/core.h
>> +++ b/drivers/media/platform/qcom/venus/core.h
>> @@ -168,6 +168,7 @@ struct venus_format {
>>    * @root:	debugfs root directory
>>    * @venus_ver:	the venus firmware version
>>    * @dump_core:	a flag indicating that a core dump is required
>> + * @hwmode_dev:	a flag indicating that HW_CTRL_TRIGGER is used in clock driver
>>    */
>>   struct venus_core {
>>   	void __iomem *base;
>> @@ -230,6 +231,7 @@ struct venus_core {
>>   		u32 rev;
>>   	} venus_ver;
>>   	unsigned long dump_core;
>> +	bool hwmode_dev;
>>   };
>>   
>>   struct vdec_controls {
>> diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
>> index 33a5a659c0ada0ca97eebb5522c5f349f95c49c7..409aa9bd0b5d099c993eedb03177ec5ed918b4a0 100644
>> --- a/drivers/media/platform/qcom/venus/pm_helpers.c
>> +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
>> @@ -412,9 +412,17 @@ static int vcodec_control_v4(struct venus_core *core, u32 coreid, bool enable)
>>   	u32 val;
>>   	int ret;
>>   
>> -	if (IS_V6(core))
>> -		return dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[coreid], !enable);
>> -	else if (coreid == VIDC_CORE_ID_1) {
>> +	ret = dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[coreid], !enable);
>> +	if (ret == -EOPNOTSUPP) {
>> +		core->hwmode_dev = false;
>> +		goto legacy;
>> +	}
>> +
>> +	core->hwmode_dev = true;
>> +	return ret;
>> +
>> +legacy:
>> +	if (coreid == VIDC_CORE_ID_1) {
>>   		ctrl = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_CONTROL;
>>   		stat = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_STATUS;
>>   	} else {
>> @@ -450,7 +458,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
>>   
>>   		vcodec_clks_disable(core, core->vcodec0_clks);
>>   
>> -		if (!IS_V6(core)) {
>> +		if (!core->hwmode_dev) {
>>   			ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
>>   			if (ret)
>>   				return ret;
>> @@ -468,7 +476,7 @@ static int poweroff_coreid(struct venus_core *core, unsigned int coreid_mask)
>>   
>>   		vcodec_clks_disable(core, core->vcodec1_clks);
>>   
>> -		if (!IS_V6(core)) {
>> +		if (!core->hwmode_dev) {
>>   			ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
>>   			if (ret)
>>   				return ret;
>> @@ -491,11 +499,9 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
>>   		if (ret < 0)
>>   			return ret;
>>   
>> -		if (!IS_V6(core)) {
>> -			ret = vcodec_control_v4(core, VIDC_CORE_ID_1, true);
>> -			if (ret)
>> -				return ret;
>> -		}
>> +		ret = vcodec_control_v4(core, VIDC_CORE_ID_1, true);
>> +		if (ret)
>> +			return ret;
>>   
>>   		ret = vcodec_clks_enable(core, core->vcodec0_clks);
>>   		if (ret)
>> @@ -511,11 +517,9 @@ static int poweron_coreid(struct venus_core *core, unsigned int coreid_mask)
>>   		if (ret < 0)
>>   			return ret;
>>   
>> -		if (!IS_V6(core)) {
>> -			ret = vcodec_control_v4(core, VIDC_CORE_ID_2, true);
>> -			if (ret)
>> -				return ret;
>> -		}
>> +		ret = vcodec_control_v4(core, VIDC_CORE_ID_2, true);
>> +		if (ret)
>> +			return ret;
>>   
>>   		ret = vcodec_clks_enable(core, core->vcodec1_clks);
>>   		if (ret)
>> @@ -811,7 +815,7 @@ static int vdec_power_v4(struct device *dev, int on)
>>   	else
>>   		vcodec_clks_disable(core, core->vcodec0_clks);
>>   
>> -	vcodec_control_v4(core, VIDC_CORE_ID_1, false);
>> +	ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
>
> return vcodec_control_v4(...);
OK, thanks for your comments. This patch has already been merged. I have
another patch that cleans up code and removes dead code, but I haven’t
submitted it yet. It depends on the videocc flag, so I plan to wait until
the videocc patch is picked before submitting it.
>
>>   
>>   	return ret;
>>   }
>> @@ -856,7 +860,7 @@ static int venc_power_v4(struct device *dev, int on)
>>   	else
>>   		vcodec_clks_disable(core, core->vcodec1_clks);
>>   
>> -	vcodec_control_v4(core, VIDC_CORE_ID_2, false);
>> +	ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
> And here.
>
>>   
>>   	return ret;
>>   }
>>
>> -- 
>> 2.34.1
>>
-- 
Best Regards,
Renjiang