| Message ID | 82114a4bd9c7bc1188c6a7167a6e74bb3360961d.1620207353.git.mchehab+huawei@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix some PM runtime issues at the media subsystem | expand |
On Wed, 5 May 2021 11:41:59 +0200 Mauro Carvalho Chehab <mchehab+huawei@kernel.org> 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(). > > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> > Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver") > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> Does this move not result in a potential call of clk_bulk_disable() for clocks that aren't enabled? > --- > drivers/staging/media/hantro/hantro_drv.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > index 595e82a82728..4387edaa1d0d 100644 > --- a/drivers/staging/media/hantro/hantro_drv.c > +++ b/drivers/staging/media/hantro/hantro_drv.c > @@ -152,13 +152,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_get_sync(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);
Hi Mauro, Thanks for working on this. On Wed, 2021-05-05 at 11:41 +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(). > > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> > Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver") > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> It seems this is wrong now, as this series doesn't have https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@kernel.org/ I don't fully understand why all the back and forth happening on this series, but the former Hantro patches looked good (despite perhaps unclear commit messages). Any issues just squashing these two commits from "[PATCH v4 00/79] Address some issues with PM runtime at media subsystem": media: hantro: use pm_runtime_resume_and_get() media: hantro: do a PM resume earlier ? Thanks, Ezequiel > drivers/staging/media/hantro/hantro_drv.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c > index 595e82a82728..4387edaa1d0d 100644 > --- a/drivers/staging/media/hantro/hantro_drv.c > +++ b/drivers/staging/media/hantro/hantro_drv.c > @@ -152,13 +152,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_get_sync(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, 05 May 2021 10:22:03 -0300 Ezequiel Garcia <ezequiel@collabora.com> escreveu: > Hi Mauro, > > Thanks for working on this. > > On Wed, 2021-05-05 at 11:41 +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(). > > > > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> > > Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver") > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > It seems this is wrong now, as this series doesn't have > > https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@kernel.org/ > > I don't fully understand why all the back and forth > happening on this series, but the former Hantro patches > looked good (despite perhaps unclear commit messages). There was a request to break the original /79 series into smaller ones, to make easier for reviewers. So, I opted to split it into (probably) 3 series: 1. Fixes (this series); 2. "use pm_runtime_resume_and_get" for the I2C drivers; 3. "use pm_runtime_resume_and_get" for remaining ones. Before flooding everybody's email's with series (2) and (3), better to focus at the fixes first. I'll probably send the other two series by tomorrow. > Any issues just squashing these two commits from "[PATCH v4 00/79] Address some issues with PM runtime at media subsystem": > > media: hantro: use pm_runtime_resume_and_get() > media: hantro: do a PM resume earlier The problem is that pm_runtime_resume_and_get() was added only recently (Kernel v5.10). So, I opted to place the fix patches before the changes, as this way, most (all?) patches can be easily be backported to legacy Kernels as needed. Thanks, Mauro
On Wed, 2021-05-05 at 15:46 +0200, Mauro Carvalho Chehab wrote: > Em Wed, 05 May 2021 10:22:03 -0300 > Ezequiel Garcia <ezequiel@collabora.com> escreveu: > > > Hi Mauro, > > > > Thanks for working on this. > > > > On Wed, 2021-05-05 at 11:41 +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(). > > > > > > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> > > > Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver") > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > > > It seems this is wrong now, as this series doesn't have > > > > https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@kernel.org/ > > > > I don't fully understand why all the back and forth > > happening on this series, but the former Hantro patches > > looked good (despite perhaps unclear commit messages). > > There was a request to break the original /79 series into smaller ones, > to make easier for reviewers. So, I opted to split it into (probably) > 3 series: > > 1. Fixes (this series); > 2. "use pm_runtime_resume_and_get" for the I2C drivers; > 3. "use pm_runtime_resume_and_get" for remaining ones. > > Before flooding everybody's email's with series (2) and (3), better > to focus at the fixes first. I'll probably send the other two series > by tomorrow. > > > Any issues just squashing these two commits from "[PATCH v4 00/79] Address some issues with PM runtime at media subsystem": > > > > media: hantro: use pm_runtime_resume_and_get() > > media: hantro: do a PM resume earlier > > The problem is that pm_runtime_resume_and_get() was added only > recently (Kernel v5.10). > > So, I opted to place the fix patches before the changes, as this > way, most (all?) patches can be easily be backported to legacy Kernels > as needed. > Got it. Maybe the better fix would be the squash of [PATCH v4 78/79] media: hantro: use pm_runtime_resume_and_get() and [PATCH v4 79/79] media: hantro: do a PM resume earlier but keeping pm_runtime_get_sync. And then you can replace the pm_runtime_get_sync with pm_runtime_resume_and_get. Thanks, Ezequiel
Em Wed, 05 May 2021 11:01:35 -0300 Ezequiel Garcia <ezequiel@collabora.com> escreveu: > On Wed, 2021-05-05 at 15:46 +0200, Mauro Carvalho Chehab wrote: > > Em Wed, 05 May 2021 10:22:03 -0300 > > Ezequiel Garcia <ezequiel@collabora.com> escreveu: > > > > > Hi Mauro, > > > > > > Thanks for working on this. > > > > > > On Wed, 2021-05-05 at 11:41 +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(). > > > > > > > > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> > > > > Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver") > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org> > > > > > > It seems this is wrong now, as this series doesn't have > > > > > > https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@kernel.org/ > > > > > > I don't fully understand why all the back and forth > > > happening on this series, but the former Hantro patches > > > looked good (despite perhaps unclear commit messages). > > > > There was a request to break the original /79 series into smaller ones, > > to make easier for reviewers. So, I opted to split it into (probably) > > 3 series: > > > > 1. Fixes (this series); > > 2. "use pm_runtime_resume_and_get" for the I2C drivers; > > 3. "use pm_runtime_resume_and_get" for remaining ones. > > > > Before flooding everybody's email's with series (2) and (3), better > > to focus at the fixes first. I'll probably send the other two series > > by tomorrow. > > > > > Any issues just squashing these two commits from "[PATCH v4 00/79] Address some issues with PM runtime at media subsystem": > > > > > > media: hantro: use pm_runtime_resume_and_get() > > > media: hantro: do a PM resume earlier > > > > The problem is that pm_runtime_resume_and_get() was added only > > recently (Kernel v5.10). > > > > So, I opted to place the fix patches before the changes, as this > > way, most (all?) patches can be easily be backported to legacy Kernels > > as needed. > > > > Got it. > > Maybe the better fix would be the squash of [PATCH v4 78/79] media: hantro: use pm_runtime_resume_and_get() > and [PATCH v4 79/79] media: hantro: do a PM resume earlier but keeping pm_runtime_get_sync. > > And then you can replace the pm_runtime_get_sync with pm_runtime_resume_and_get. Works for me. So, the fixes patch will be the enclosed one, right? Btw, I agree with Jonathan that the best would be to also move this: clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks); out of hantro_job_finish_no_pm(), as, when an error happens at device_run(), the clock lines won't be enabled at the first place. > Thanks, > Ezequiel Thanks, Mauro [PATCH] media: hantro: do a PM resume earlier 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(). Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver") 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..bdb57fb56f47 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c @@ -56,14 +56,12 @@ dma_addr_t hantro_get_ref(struct hantro_ctx *ctx, u64 ts) return hantro_get_dec_buf_addr(ctx, buf); } -static void hantro_job_finish(struct hantro_dev *vpu, - struct hantro_ctx *ctx, - enum vb2_buffer_state result) +static void hantro_job_finish_no_pm(struct hantro_dev *vpu, + struct hantro_ctx *ctx, + enum vb2_buffer_state result) { struct vb2_v4l2_buffer *src, *dst; - pm_runtime_mark_last_busy(vpu->dev); - pm_runtime_put_autosuspend(vpu->dev); clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks); src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx); @@ -81,6 +79,16 @@ static void hantro_job_finish(struct hantro_dev *vpu, result); } +static void hantro_job_finish(struct hantro_dev *vpu, + struct hantro_ctx *ctx, + enum vb2_buffer_state result) +{ + pm_runtime_mark_last_busy(vpu->dev); + pm_runtime_put_autosuspend(vpu->dev); + + hantro_job_finish_no_pm(vpu, ctx, result); +} + void hantro_irq_done(struct hantro_dev *vpu, enum vb2_buffer_state result) { @@ -152,12 +160,15 @@ static void device_run(void *priv) src = hantro_get_src_buf(ctx); dst = hantro_get_dst_buf(ctx); + ret = pm_runtime_get_sync(ctx->dev->dev); + if (ret < 0) { + pm_runtime_put_noidle(ctx->dev->dev); + goto err_cancel_job; + } + 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); - if (ret < 0) - goto err_cancel_job; v4l2_m2m_buf_copy_metadata(src, dst, true); @@ -165,7 +176,7 @@ static void device_run(void *priv) return; err_cancel_job: - hantro_job_finish(ctx->dev, ctx, VB2_BUF_STATE_ERROR); + hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR); } static struct v4l2_m2m_ops vpu_m2m_ops = {
diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c index 595e82a82728..4387edaa1d0d 100644 --- a/drivers/staging/media/hantro/hantro_drv.c +++ b/drivers/staging/media/hantro/hantro_drv.c @@ -152,13 +152,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_get_sync(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);