Em Wed, 05 May 2021 11:01:35 -0300 Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> > > > > Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver") > > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> > > > > > > It seems this is wrong now, as this series doesn't have > > > > > > https://lore.kernel.org/linux-media/803c39fafdd62efc6f9e4d99a372af2c6955143b.1619621413.git.mchehab+huawei@xxxxxxxxxx/ > > > > > > 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@xxxxxxxxxxxxx> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver") Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> 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 = {