Re: [PATCH v8 05/14] media: platform: Improve power on and power off flow

[Xia Jiang]

On Thu, 2020-05-21 at 15:22 +0000, Tomasz Figa wrote:
> Hi Xia,
> 
> On Fri, Apr 03, 2020 at 05:40:24PM +0800, Xia Jiang wrote:
> > Call pm_runtime_get_sync() before starting a frame and then
> > pm_runtime_put() after completing it. This can save power for the time
> > between processing two frames.
> > 
> > Signed-off-by: Xia Jiang <xia.jiang@xxxxxxxxxxxx>
> > ---
> >  .../media/platform/mtk-jpeg/mtk_jpeg_core.c   | 27 +++++--------------
> >  1 file changed, 6 insertions(+), 21 deletions(-)
> > 
> 
> Thank you for the patch. Please see my comments inline.
> 
> > diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > index a536fa95b3d6..dd5cadd101ef 100644
> > --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> > @@ -710,23 +710,6 @@ static struct vb2_v4l2_buffer *mtk_jpeg_buf_remove(struct mtk_jpeg_ctx *ctx,
> >  		return v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx);
> >  }
> >  
> > -static int mtk_jpeg_start_streaming(struct vb2_queue *q, unsigned int count)
> > -{
> > -	struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> > -	struct vb2_v4l2_buffer *vb;
> > -	int ret = 0;
> > -
> > -	ret = pm_runtime_get_sync(ctx->jpeg->dev);
> > -	if (ret < 0)
> > -		goto err;
> > -
> > -	return 0;
> > -err:
> > -	while ((vb = mtk_jpeg_buf_remove(ctx, q->type)))
> > -		v4l2_m2m_buf_done(vb, VB2_BUF_STATE_QUEUED);
> > -	return ret;
> > -}
> > -
> >  static void mtk_jpeg_stop_streaming(struct vb2_queue *q)
> >  {
> >  	struct mtk_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> > @@ -751,8 +734,6 @@ static void mtk_jpeg_stop_streaming(struct vb2_queue *q)
> >  
> >  	while ((vb = mtk_jpeg_buf_remove(ctx, q->type)))
> >  		v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
> > -
> > -	pm_runtime_put_sync(ctx->jpeg->dev);
> >  }
> >  
> >  static const struct vb2_ops mtk_jpeg_qops = {
> > @@ -761,7 +742,6 @@ static const struct vb2_ops mtk_jpeg_qops = {
> >  	.buf_queue          = mtk_jpeg_buf_queue,
> >  	.wait_prepare       = vb2_ops_wait_prepare,
> >  	.wait_finish        = vb2_ops_wait_finish,
> > -	.start_streaming    = mtk_jpeg_start_streaming,
> >  	.stop_streaming     = mtk_jpeg_stop_streaming,
> >  };
> >  
> > @@ -812,7 +792,7 @@ static void mtk_jpeg_device_run(void *priv)
> >  	struct mtk_jpeg_src_buf *jpeg_src_buf;
> >  	struct mtk_jpeg_bs bs;
> >  	struct mtk_jpeg_fb fb;
> > -	int i;
> > +	int i, ret;
> >  
> >  	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> >  	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> > @@ -832,6 +812,10 @@ static void mtk_jpeg_device_run(void *priv)
> >  		return;
> >  	}
> >  
> > +	ret = pm_runtime_get_sync(jpeg->dev);
> > +	if (ret < 0)
> > +		goto dec_end;
> > +
> >  	mtk_jpeg_set_dec_src(ctx, &src_buf->vb2_buf, &bs);
> >  	if (mtk_jpeg_set_dec_dst(ctx, &jpeg_src_buf->dec_param, &dst_buf->vb2_buf, &fb))
> >  		goto dec_end;
> > @@ -957,6 +941,7 @@ static irqreturn_t mtk_jpeg_dec_irq(int irq, void *priv)
> >  	v4l2_m2m_buf_done(src_buf, buf_state);
> >  	v4l2_m2m_buf_done(dst_buf, buf_state);
> >  	v4l2_m2m_job_finish(jpeg->m2m_dev, ctx->fh.m2m_ctx);
> > +	pm_runtime_put_sync(ctx->jpeg->dev);
> 
> The _sync variant explicitly waits until the asynchronous PM operation
> completes. This is usually undesired, because the CPU stays blocked for
> no good reason. In this context it is actually a bug, because this is an
> interrupt handler and it's not allowed to sleep. I wonder why this
> actually didn't crash in your testing. Please change to the regular
> pm_runtime_put().
Done.
> 
> Best regards,
> Tomasz