Re: [PATCH v2 2/2] media: imagination: e5010: Enable autosuspend for runtime PM

[Jai Luthra]

Hi Brandon,

Thanks for the patch.

Quoting Brandon Brnich (2026-07-23 21:11:17)
> Current implementation only has one call to turn the device on -
> start_streaming. This can waste power of device as the device can be in
> STREAM_ON state, but no buffers are actually being submitted.
> 
> Signed-off-by: Brandon Brnich <b-brnich@xxxxxx>
> ---
>  .../platform/imagination/e5010-jpeg-enc.c     | 32 +++++++------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index 2b98d230429d1..580490d1a0f45 100644
> --- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
> +++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> @@ -976,6 +976,7 @@ static irqreturn_t e5010_irq(int irq, void *data)
>  
>         v4l2_m2m_job_finish(e5010->m2m_dev, ctx->fh.m2m_ctx);
>         dprintk(e5010, 3, "ctx: 0x%p Finish job\n", ctx);
> +       pm_runtime_put_autosuspend(e5010->dev);
>  
>  job_unlock:
>         spin_unlock(&e5010->hw_lock);
> @@ -1099,6 +1100,8 @@ static int e5010_probe(struct platform_device *pdev)
>                 goto fail_after_v4l2_register;
>         }
>  
> +       pm_runtime_set_autosuspend_delay(dev, 100);
> +       pm_runtime_use_autosuspend(dev);
>         pm_runtime_enable(dev);
>  
>         ret = video_register_device(e5010->vdev, VFL_TYPE_VIDEO, 0);
> @@ -1289,31 +1292,13 @@ static int e5010_encoder_cmd(struct file *file, void *priv,
>  static int e5010_start_streaming(struct vb2_queue *q, unsigned int count)
>  {
>         struct e5010_context *ctx = vb2_get_drv_priv(q);
> -       int ret;
>  
>         struct e5010_q_data *queue = get_queue(ctx, q->type);
>  
>         v4l2_m2m_update_start_streaming_state(ctx->fh.m2m_ctx, q);
>         queue->sequence = 0;
>  
> -       ret = pm_runtime_resume_and_get(ctx->e5010->dev);
> -       if (ret < 0) {
> -               v4l2_err(&ctx->e5010->v4l2_dev, "failed to power up jpeg\n");
> -               goto fail;
> -       }
> -
> -       ret = e5010_init_device(ctx->e5010);
> -       if (ret) {
> -               v4l2_err(&ctx->e5010->v4l2_dev, "failed to Enable e5010 device\n");
> -               goto fail;
> -       }
> -
>         return 0;
> -
> -fail:
> -       e5010_vb2_buffers_return(q, VB2_BUF_STATE_QUEUED);
> -
> -       return ret;
>  }
>  
>  static void e5010_stop_streaming(struct vb2_queue *q)
> @@ -1329,8 +1314,6 @@ static void e5010_stop_streaming(struct vb2_queue *q)
>             v4l2_m2m_has_stopped(ctx->fh.m2m_ctx)) {
>                 v4l2_event_queue_fh(&ctx->fh, &e5010_eos_event);
>         }
> -
> -       pm_runtime_put_sync(ctx->e5010->dev);
>  }
>  
>  static void e5010_device_run(void *priv)
> @@ -1343,7 +1326,15 @@ static void e5010_device_run(void *priv)
>         unsigned long flags;
>         int num_planes = ctx->out_queue.fmt->num_planes;
>  
> +       ret = pm_runtime_resume_and_get(e5010->dev);
> +       if (ret < 0) {
> +               dev_err(e5010->dev, "Device failed to turn on\n");
> +               v4l2_m2m_job_finish(e5010->m2m_dev, ctx->fh.m2m_ctx);
> +               return;
> +       }
> +

Sashiko (LLM bot) made a comment here [1] that the device may end up using
default qp table values after resuming from power down because the state
isn't cleared properly.. is there any validity to that?

[1] https://sashiko.dev/#/patchset/20260723154117.1917644-1-b-brnich%40ti.com

LLMs can give plausible sounding false positives too, but I'll still
withold my R-By tag until confirmation or testing from you and/or Devarsh.

Thanks,
    Jai

>         spin_lock_irqsave(&e5010->hw_lock, flags);
> +
>         s_vb = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
>         WARN_ON(!s_vb);
>         d_vb = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
> @@ -1474,6 +1465,7 @@ static void e5010_device_run(void *priv)
>         e5010_reset(e5010->dev, e5010->core_base, e5010->mmu_base);
>  
>  no_ready_buf_err:
> +       pm_runtime_put_autosuspend(e5010->dev);
>         if (s_vb) {
>                 v4l2_m2m_src_buf_remove_by_buf(ctx->fh.m2m_ctx, s_vb);
>                 v4l2_m2m_buf_done(s_vb, VB2_BUF_STATE_ERROR);
> -- 
> 2.43.0
>