| Message ID | 0021158fb27035a56089683ee712fb3ed6f6032d.1619191723.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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@kernel.org> > --- > 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. Thanks, Ezequiel
Em Sat, 24 Apr 2021 20:23:53 -0300 Ezequiel Garcia <ezequiel@collabora.com> 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@kernel.org> > > --- > > 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@kernel.org> 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;
Em Mon, 26 Apr 2021 14:33:27 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu: > Em Sat, 24 Apr 2021 20:23:53 -0300 > Ezequiel Garcia <ezequiel@collabora.com> 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@kernel.org> > > > --- > > > 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@kernel.org> 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
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;
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@kernel.org> --- drivers/staging/media/hantro/hantro_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)