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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel