On Thu, Jan 02, 2025 at 11:47:22AM +0800, Renjiang Han wrote:
>
> 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.
One-line wrappers don't make the code easier to read. Please inline it.
--
With best wishes
Dmitry
