Re: [PATCH 13/78] staging: media: hantro_drv: use pm_runtime_resume_and_get()

From: Mauro Carvalho Chehab
Date: Mon Apr 26 2021 - 08:42:25 EST


Em Mon, 26 Apr 2021 14:33:27 +0200
Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> escreveu:

> Em Sat, 24 Apr 2021 20:23:53 -0300
> Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> escreveu:
>
> > Hi Mauro,
> >
> > On Sat, 2021-04-24 at 08:44 +0200, Mauro Carvalho Chehab wrote:
> > > Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > > added pm_runtime_resume_and_get() in order to automatically handle
> > > dev->power.usage_count decrement on errors.
> > >
> > > Use the new API, in order to cleanup the error check logic.
> > >
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>
> > > ---
> > >  drivers/staging/media/hantro/hantro_drv.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > > index 595e82a82728..3147dcbebeb9 100644
> > > --- a/drivers/staging/media/hantro/hantro_drv.c
> > > +++ b/drivers/staging/media/hantro/hantro_drv.c
> > > @@ -155,7 +155,7 @@ static void device_run(void *priv)
> > >         ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> > >         if (ret)
> > >                 goto err_cancel_job;
> > > -       ret = pm_runtime_get_sync(ctx->dev->dev);
> > > +       ret = pm_runtime_resume_and_get(ctx->dev->dev);
> > >         if (ret < 0)
> > >                 goto err_cancel_job;
> > >  
> >
> > Seems this one needs a different fix: err_cancel_job
> > will call hantro_job_finish which has a pm_runtime put.
>
> Good point. Thanks for reviewing it!
>
> It sounds that this is a place where the best seems
> to keep using pm_runtime_get_sync(), but let's at least add a
> comment explaining why it should be kept here. This should
> help to avoid people to copy-and-paste the code on situations
> where pm_runtime_resume_and_get() should be used instead.
>
> See enclosed patch.
>
> Thanks,
> Mauro
>
> [PATCH] media: hantro: document the usage of pm_runtime_get_sync()
>
> Despite other *_get()/*_put() functions, where usage count is
> incremented only if not errors, the pm_runtime_get_sync() has
> a different behavior, incrementing the counter *even* on
> errors.
>
> That's an error prone behavior, as people often forget to
> decrement the usage counter.
>
> However, the hantro driver depends on this behavior, as it
> will decrement the usage_count unconditionally at the m2m
> job finish time, which makes sense.
>
> So, intead of using the pm_runtime_resume_and_get() that
> would decrement the counter on error, keep the current
> API, but add a documentation explaining the rationale for
> keep using pm_runtime_get_sync().
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx>

Hmm... maybe it can, instead, use the same solution as the
rkvdec driver does, having a job_finish_no_pm() plus the normal
job_finish().

What do you think?

Regards,
Mauro

>
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 595e82a82728..96f940c1c85c 100644
> --- a/drivers/staging/media/hantro/hantro_drv.c
> +++ b/drivers/staging/media/hantro/hantro_drv.c
> @@ -155,6 +155,13 @@ static void device_run(void *priv)
> ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> if (ret)
> goto err_cancel_job;
> +
> + /*
> + * The pm_runtime_get_sync() will increment dev->power.usage_count,
> + * even on errors. That's the expected behavior here, since the
> + * hantro_job_finish() function at the error handling code
> + * will internally call pm_runtime_put_autosuspend().
> + */
> ret = pm_runtime_get_sync(ctx->dev->dev);
> if (ret < 0)
> goto err_cancel_job;
>
>



Thanks,
Mauro