Re: [PATCH 09/25] media: hantro: do a PM resume earlier

[Mauro Carvalho Chehab]

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 = {