| Message ID | 569838d406dde80dcc64989a663882817a54cbb2.1619621413.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Address some issues with PM runtime at media subsystem | expand |
Hi Mauro, Thanks a lot for taking care of this. On Wed, 2021-04-28 at 16:52 +0200, Mauro Carvalho Chehab wrote: > The device_run() first enables the clock and then > tries to resume PM runtime, checking for errors. > > Well, if for some reason the pm_runtime can not resume, > it would be better to detect it beforehand. > > So, change the order inside device_run(). > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Clocks could be behind power-domains, IIRC, so this change is fixing that. However, this has ever been a problem for this driver, so I don't think it makes sense to bother with Fixes tag. Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> Thanks, Ezequiel > --- > drivers/staging/media/hantro/hantro_drv.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > index 25fa36e7e773..67de6b15236d 100644 > --- a/drivers/staging/media/hantro/hantro_drv.c > +++ b/drivers/staging/media/hantro/hantro_drv.c > @@ -160,14 +160,14 @@ static void device_run(void *priv) > src = hantro_get_src_buf(ctx); > dst = hantro_get_dst_buf(ctx); > > - ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks); > - if (ret) > - goto err_cancel_job; > - > ret = pm_runtime_resume_and_get(ctx->dev->dev); > if (ret < 0) > goto err_cancel_job; > > + ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks); > + if (ret) > + goto err_cancel_job; > + > v4l2_m2m_buf_copy_metadata(src, dst, true); > > ctx->codec_ops->run(ctx);
Em Wed, 28 Apr 2021 14:17:50 -0300 Ezequiel Garcia <ezequiel@collabora.com> escreveu: > Hi Mauro, > > Thanks a lot for taking care of this. > > On Wed, 2021-04-28 at 16:52 +0200, Mauro Carvalho Chehab wrote: > > The device_run() first enables the clock and then > > tries to resume PM runtime, checking for errors. > > > > Well, if for some reason the pm_runtime can not resume, > > it would be better to detect it beforehand. > > > > So, change the order inside device_run(). > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > Clocks could be behind power-domains, IIRC, so this change > is fixing that. > > However, this has ever been a problem for this driver, > so I don't think it makes sense to bother with Fixes tag. I would prefer to move this patch to the first part of this series, together with other fixes, rebasing it to apply cleanly before the pm_runtime_resume_and_get() patch, with: Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver") This way, people that could be interested on backporting it will be capable to apply it as is to stable Kernel releases that came with this driver. > > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> > > Thanks, > Ezequiel > > > --- > > drivers/staging/media/hantro/hantro_drv.c | 8 ++++---- > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > > index 25fa36e7e773..67de6b15236d 100644 > > --- a/drivers/staging/media/hantro/hantro_drv.c > > +++ b/drivers/staging/media/hantro/hantro_drv.c > > @@ -160,14 +160,14 @@ static void device_run(void *priv) > > src = hantro_get_src_buf(ctx); > > dst = hantro_get_dst_buf(ctx); > > > > - ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks); > > - if (ret) > > - goto err_cancel_job; > > - > > ret = pm_runtime_resume_and_get(ctx->dev->dev); > > if (ret < 0) > > goto err_cancel_job; > > > > + ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks); > > + if (ret) > > + goto err_cancel_job; > > + > > v4l2_m2m_buf_copy_metadata(src, dst, true); > > > > ctx->codec_ops->run(ctx); > > Thanks, Mauro
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c index 25fa36e7e773..67de6b15236d 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c @@ -160,14 +160,14 @@ static void device_run(void *priv) src = hantro_get_src_buf(ctx); dst = hantro_get_dst_buf(ctx); - ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks); - if (ret) - goto err_cancel_job; - ret = pm_runtime_resume_and_get(ctx->dev->dev); if (ret < 0) goto err_cancel_job; + ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks); + if (ret) + goto err_cancel_job; + v4l2_m2m_buf_copy_metadata(src, dst, true); ctx->codec_ops->run(ctx);
The device_run() first enables the clock and then tries to resume PM runtime, checking for errors. Well, if for some reason the pm_runtime can not resume, it would be better to detect it beforehand. So, change the order inside device_run(). Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> --- drivers/staging/media/hantro/hantro_drv.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)