[PATCH v3 11/79] media: rga-buf: use pm_runtime_resume_and_get()

[PATCH v3 11/79] media: rga-buf: use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

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/media/platform/rockchip/rga/rga-buf.c | 3 +--
 drivers/media/platform/rockchip/rga/rga.c     | 4 +++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/rockchip/rga/rga-buf.c b/drivers/media/platform/rockchip/rga/rga-buf.c
index bf9a75b75083..81508ed5abf3 100644
--- a/drivers/media/platform/rockchip/rga/rga-buf.c
+++ b/drivers/media/platform/rockchip/rga/rga-buf.c
@@ -79,9 +79,8 @@ static int rga_buf_start_streaming(struct vb2_queue *q, unsigned int count)
 	struct rockchip_rga *rga = ctx->rga;
 	int ret;
 
-	ret = pm_runtime_get_sync(rga->dev);
+	ret = pm_runtime_resume_and_get(rga->dev);
 	if (ret < 0) {
-		pm_runtime_put_noidle(rga->dev);
 		rga_buf_return_buffers(q, VB2_BUF_STATE_QUEUED);
 		return ret;
 	}
diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
index 9d122429706e..bf3fd71ec3af 100644
--- a/drivers/media/platform/rockchip/rga/rga.c
+++ b/drivers/media/platform/rockchip/rga/rga.c
@@ -866,7 +866,9 @@ static int rga_probe(struct platform_device *pdev)
 		goto unreg_video_dev;
 	}
 
-	pm_runtime_get_sync(rga->dev);
+	ret = pm_runtime_resume_and_get(rga->dev);
+	if (ret < 0)
+		goto unreg_video_dev;
 
 	rga->version.major = (rga_read(rga, RGA_VERSION_INFO) >> 24) & 0xFF;
 	rga->version.minor = (rga_read(rga, RGA_VERSION_INFO) >> 20) & 0x0F;
-- 
2.30.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 11/79] media: rga-buf: use pm_runtime_resume_and_get()

[Ezequiel Garcia]

On Tue, 2021-04-27 at 12:26 +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>

Sorry, I just replied to the wrong patch version.
Again:

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

Thanks,
Ezequiel

> ---
>  drivers/media/platform/rockchip/rga/rga-buf.c | 3 +--
>  drivers/media/platform/rockchip/rga/rga.c     | 4 +++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rga/rga-buf.c b/drivers/media/platform/rockchip/rga/rga-buf.c
> index bf9a75b75083..81508ed5abf3 100644
> --- a/drivers/media/platform/rockchip/rga/rga-buf.c
> +++ b/drivers/media/platform/rockchip/rga/rga-buf.c
> @@ -79,9 +79,8 @@ static int rga_buf_start_streaming(struct vb2_queue *q, unsigned int count)
>         struct rockchip_rga *rga = ctx->rga;
>         int ret;
>  
> -       ret = pm_runtime_get_sync(rga->dev);
> +       ret = pm_runtime_resume_and_get(rga->dev);
>         if (ret < 0) {
> -               pm_runtime_put_noidle(rga->dev);
>                 rga_buf_return_buffers(q, VB2_BUF_STATE_QUEUED);
>                 return ret;
>         }
> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> index 9d122429706e..bf3fd71ec3af 100644
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
> @@ -866,7 +866,9 @@ static int rga_probe(struct platform_device *pdev)
>                 goto unreg_video_dev;
>         }
>  
> -       pm_runtime_get_sync(rga->dev);
> +       ret = pm_runtime_resume_and_get(rga->dev);
> +       if (ret < 0)
> +               goto unreg_video_dev;
>  
>         rga->version.major = (rga_read(rga, RGA_VERSION_INFO) >> 24) & 0xFF;
>         rga->version.minor = (rga_read(rga, RGA_VERSION_INFO) >> 20) & 0x0F;



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 20/79] staging: media: rkvdec: fix pm_runtime_get_sync() usage count

[Mauro Carvalho Chehab]

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.
Replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
in order to properly decrement the usage counter and avoid memory
leaks.

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/staging/media/rkvdec/rkvdec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkvdec/rkvdec.c b/drivers/staging/media/rkvdec/rkvdec.c
index d821661d30f3..8c17615f3a7a 100644
--- a/drivers/staging/media/rkvdec/rkvdec.c
+++ b/drivers/staging/media/rkvdec/rkvdec.c
@@ -658,7 +658,7 @@ static void rkvdec_device_run(void *priv)
 	if (WARN_ON(!desc))
 		return;
 
-	ret = pm_runtime_get_sync(rkvdec->dev);
+	ret = pm_runtime_resume_and_get(rkvdec->dev);
 	if (ret < 0) {
 		rkvdec_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
 		return;
-- 
2.30.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 70/79] media: rkisp1: use pm_runtime_resume_and_get()

[Mauro Carvalho Chehab]

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/media/platform/rockchip/rkisp1/rkisp1-capture.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
index 5f6c9d1623e4..3730376897d9 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
@@ -1003,9 +1003,8 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 	if (ret)
 		goto err_pipeline_stop;
 
-	ret = pm_runtime_get_sync(cap->rkisp1->dev);
+	ret = pm_runtime_resume_and_get(cap->rkisp1->dev);
 	if (ret < 0) {
-		pm_runtime_put_noidle(cap->rkisp1->dev);
 		dev_err(cap->rkisp1->dev, "power up failed %d\n", ret);
 		goto err_destroy_dummy;
 	}
-- 
2.30.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v3 79/79] media: hantro: document the usage of pm_runtime_get_sync()

[Mauro Carvalho Chehab]

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>
---
 drivers/staging/media/hantro/hantro_drv.c | 7 +++++++
 1 file changed, 7 insertions(+)

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;
-- 
2.30.2


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 79/79] media: hantro: document the usage of pm_runtime_get_sync()

[Robin Murphy]

On 2021-04-27 11:27, Mauro Carvalho Chehab wrote:
> 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>
> ---
>   drivers/staging/media/hantro/hantro_drv.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> 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;

..except this can also cause the same pm_runtime_put_autosuspend() call 
without even reaching the "matching" get below, so rather than some kind 
of cleverness it seems more like it's just broken :/

Robin.

> +
> +	/*
> +	 * 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;
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 79/79] media: hantro: document the usage of pm_runtime_get_sync()

[Ezequiel Garcia]

On Tue, 2021-04-27 at 16:08 +0100, Robin Murphy wrote:
> On 2021-04-27 11:27, Mauro Carvalho Chehab wrote:
> > 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>
> > ---
> >   drivers/staging/media/hantro/hantro_drv.c | 7 +++++++
> >   1 file changed, 7 insertions(+)
> > 
> > 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;
> 
> ..except this can also cause the same pm_runtime_put_autosuspend() call 
> without even reaching the "matching" get below, so rather than some kind 
> of cleverness it seems more like it's just broken :/
> 

Indeed, I was trying to find time to cook a quick patch, but kept
getting preempted.

Feel free to submit a fix for this, otherwise, I'll try to find
time later this week.

Thanks,
Ezequiel


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 79/79] media: hantro: document the usage of pm_runtime_get_sync()

[Mauro Carvalho Chehab]

Em Tue, 27 Apr 2021 12:18:32 -0300
Ezequiel Garcia <ezequiel@collabora.com> escreveu:

> On Tue, 2021-04-27 at 16:08 +0100, Robin Murphy wrote:
> > On 2021-04-27 11:27, Mauro Carvalho Chehab wrote:  
> > > 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>
> > > ---
> > >   drivers/staging/media/hantro/hantro_drv.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > > 
> > > 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;  
> > 
> > ..except this can also cause the same pm_runtime_put_autosuspend() call 
> > without even reaching the "matching" get below, so rather than some kind 
> > of cleverness it seems more like it's just broken :/
> >   
> 
> Indeed, I was trying to find time to cook a quick patch, but kept
> getting preempted.
> 
> Feel free to submit a fix for this, otherwise, I'll try to find
> time later this week.

What about doing this instead:

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 595e82a82728..67de6b15236d 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,13 @@ static void device_run(void *priv)
 	src = hantro_get_src_buf(ctx);
 	dst = hantro_get_dst_buf(ctx);
 
+	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;
-	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 +174,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 = {

Thanks,
Mauro

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 79/79] media: hantro: document the usage of pm_runtime_get_sync()

[Mauro Carvalho Chehab]

Em Wed, 28 Apr 2021 08:27:42 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Tue, 27 Apr 2021 12:18:32 -0300
> Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> 
> > On Tue, 2021-04-27 at 16:08 +0100, Robin Murphy wrote:  
> > > On 2021-04-27 11:27, Mauro Carvalho Chehab wrote:    
> > > > 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>
> > > > ---
> > > >   drivers/staging/media/hantro/hantro_drv.c | 7 +++++++
> > > >   1 file changed, 7 insertions(+)
> > > > 
> > > > 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;    
> > > 
> > > ..except this can also cause the same pm_runtime_put_autosuspend() call 
> > > without even reaching the "matching" get below, so rather than some kind 
> > > of cleverness it seems more like it's just broken :/
> > >     
> > 
> > Indeed, I was trying to find time to cook a quick patch, but kept
> > getting preempted.
> > 
> > Feel free to submit a fix for this, otherwise, I'll try to find
> > time later this week.  
> 
> What about doing this instead:
> 
> diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> index 595e82a82728..67de6b15236d 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,13 @@ static void device_run(void *priv)
>  	src = hantro_get_src_buf(ctx);
>  	dst = hantro_get_dst_buf(ctx);
>  
> +	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;
> -	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 +174,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 = {
> 
> Thanks,
> Mauro

Actually, the order at the finish logic should change as well.
Maybe like this:

<snip>
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;

	src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
	dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);

	if (WARN_ON(!src))
		return;
	if (WARN_ON(!dst))
		return;

	src->sequence = ctx->sequence_out++;
	dst->sequence = ctx->sequence_cap++;

	v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx,
					 result);
}

static void hantro_job_finish(struct hantro_dev *vpu,
			      struct hantro_ctx *ctx,
			      enum vb2_buffer_state result)
{

	hantro_job_finish_no_pm(vpu, ctx, result);

	clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);

	pm_runtime_mark_last_busy(vpu->dev);
	pm_runtime_put_autosuspend(vpu->dev);
}

static void device_run(void *priv)
{
	struct hantro_ctx *ctx = priv;
	struct vb2_v4l2_buffer *src, *dst;
	int ret;

	src = hantro_get_src_buf(ctx);
	dst = hantro_get_dst_buf(ctx);

	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);
	return;

err_cancel_job:
	hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
}
</snip>


Thanks,
Mauro

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3 79/79] media: hantro: document the usage of pm_runtime_get_sync()

[Ezequiel Garcia]

Hi Mauro,

On Wed, 2021-04-28 at 08:44 +0200, Mauro Carvalho Chehab wrote:
> Em Wed, 28 Apr 2021 08:27:42 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:
> 
> > Em Tue, 27 Apr 2021 12:18:32 -0300
> > Ezequiel Garcia <ezequiel@collabora.com> escreveu:
> > 
> > > On Tue, 2021-04-27 at 16:08 +0100, Robin Murphy wrote:  
> > > > On 2021-04-27 11:27, Mauro Carvalho Chehab wrote:    
> > > > > 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>
> > > > > ---
> > > > >   drivers/staging/media/hantro/hantro_drv.c | 7 +++++++
> > > > >   1 file changed, 7 insertions(+)
> > > > > 
> > > > > 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;    
> > > > 
> > > > ..except this can also cause the same pm_runtime_put_autosuspend() call 
> > > > without even reaching the "matching" get below, so rather than some kind 
> > > > of cleverness it seems more like it's just broken :/
> > > >     
> > > 
> > > Indeed, I was trying to find time to cook a quick patch, but kept
> > > getting preempted.
> > > 
> > > Feel free to submit a fix for this, otherwise, I'll try to find
> > > time later this week.  
> > 
> > What about doing this instead:
> > 
> > diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
> > index 595e82a82728..67de6b15236d 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,13 @@ static void device_run(void *priv)
> >         src = hantro_get_src_buf(ctx);
> >         dst = hantro_get_dst_buf(ctx);
> >  
> > +       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;
> > -       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 +174,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 = {
> > 
> > Thanks,
> > Mauro
> 
> Actually, the order at the finish logic should change as well.
> Maybe like this:
> 

This one looks good.

Thanks!
Ezequiel

> <snip>
> 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;
> 
>         src = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
>         dst = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> 
>         if (WARN_ON(!src))
>                 return;
>         if (WARN_ON(!dst))
>                 return;
> 
>         src->sequence = ctx->sequence_out++;
>         dst->sequence = ctx->sequence_cap++;
> 
>         v4l2_m2m_buf_done_and_job_finish(ctx->dev->m2m_dev, ctx->fh.m2m_ctx,
>                                          result);
> }
> 
> static void hantro_job_finish(struct hantro_dev *vpu,
>                               struct hantro_ctx *ctx,
>                               enum vb2_buffer_state result)
> {
> 
>         hantro_job_finish_no_pm(vpu, ctx, result);
> 
>         clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> 
>         pm_runtime_mark_last_busy(vpu->dev);
>         pm_runtime_put_autosuspend(vpu->dev);
> }
> 
> static void device_run(void *priv)
> {
>         struct hantro_ctx *ctx = priv;
>         struct vb2_v4l2_buffer *src, *dst;
>         int ret;
> 
>         src = hantro_get_src_buf(ctx);
>         dst = hantro_get_dst_buf(ctx);
> 
>         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);
>         return;
> 
> err_cancel_job:
>         hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> }
> </snip>
> 
> 
> Thanks,
> Mauro



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip