Re: [PATCH v2 2/2] venus: pm_helpers: Use dev_pm_genpd_set_hwmode to switch GDSC mode on V4

[Renjiang Han]

On 12/23/2024 7:41 PM, Dmitry Baryshkov wrote:
> > diff --git a/drivers/media/platform/qcom/venus/pm_helpers.c b/drivers/media/platform/qcom/venus/pm_helpers.c
> > index 33a5a659c0ada0ca97eebb5522c5f349f95c49c7..a2062b366d4aedba3eb5e4be456a005847eaec0b 100644
> > --- a/drivers/media/platform/qcom/venus/pm_helpers.c
> > +++ b/drivers/media/platform/qcom/venus/pm_helpers.c
> > @@ -412,7 +412,7 @@ static int vcodec_control_v4(struct venus_core *core, u32 coreid, bool enable)
> >   	u32 val;
> >   	int ret;
> >   
> > -	if (IS_V6(core))
> > +	if (IS_V6(core) || IS_V4(core))
> >   		return dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[coreid], !enable);
> It is being called only for v4 and v6 targets. Drop the rest of the
> function and inline the result. I'd suggest keeping it as two patches
> though: this one which adds IS_V4() all over the place and the next one
> which performs cleanup of the dead code.
 Thanks for your comment. poweron_coreid(), poweroff_coreid() and
 vcodec_control_v4() are called only for v4 and v6. I will clean up
 the dead code with another patch.
> >   	else if (coreid == VIDC_CORE_ID_1) {
> >   		ctrl = core->wrapper_base + WRAPPER_VCODEC0_MMCC_POWER_CONTROL;
> > @@ -450,7 +450,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 (!IS_V6(core) && !IS_V4(core)) {
> >   			ret = vcodec_control_v4(core, VIDC_CORE_ID_1, false);
> >   			if (ret)
> >   				return ret;
> > @@ -468,7 +468,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 (!IS_V6(core) && !IS_V4(core)) {
> >   			ret = vcodec_control_v4(core, VIDC_CORE_ID_2, false);
> The poweron_coreid() and poweroff_coreid() functions are called only for
> v4 and v6. The v6 case was masked out earlier. Now you've removed the v4
> case too. Can we drop such vcodec_control_v4() calls completely?

 I cleaned up the code, and finally vcodec_control_v4() looks like this.

static int vcodec_control_v4(struct venus_core *core, u32 coreid, bool 
enable)
{
    return dev_pm_genpd_set_hwmode(core->pmdomains->pd_devs[coreid], 
!enable);

}

 Functionally, we can drop vcodec_control_v4(), but architecturally, I
 don’t recommend removing this function, because I think it’s easier to
 read the code with this function.

--
Best Regards,
Renjiang