Re: [PATCH v8 4/4] media: imagination: e5010: Enable autosuspend for runtime PM

["Thakkar, Devarsh" <devarsht@ti.com>]

On 11/10/2026 10:02 PM, Brnich, Brandon wrote:
> Current implementation only has one call to turn the device on -
> start_streaming. This can waste power of system as the device can be in
> STREAM_ON state, but no buffers are actually being submitted.
> 

You should also mention how current patch fixes it:
i.e by taking runtime PM reference device_run and releasing it in
completion interrupt and using PM autosuspend.
> e5010_init_device is tightly coupled to runtime resume hook with
> autosuspend addition and has been removed from start_streaming accordingly.

Has been removed is not right wording since it tends to infer that it
was removed prior to this patch but its actually this one which is
moving out.

> However, there is the case where runtime PM is not configured which would
> leave the device permantately uninitialized. Add call to e5010_init_device
> to e5010_probe to account for this case.

Yes, before this patch it was handled by start_streaming.

I think better wording would be as below:
Since e5010_runtime_resume() now initialises the hardware on every
power-up, drop the e5010_init_device() call from start_streaming().
Without runtime PM, that call in start_streaming() was the only place
the hardware got initialised, so call e5010_init_device() from probe
when runtime PM is not enabled.

> 
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> ---
>  .../platform/imagination/e5010-jpeg-enc.c     | 48 +++++++++----------
>  .../platform/imagination/e5010-jpeg-enc.h     |  1 +
>  2 files changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index 4e77112b59197..3c7c5f121a32f 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);
> @@ -1101,6 +1102,8 @@ static int e5010_probe(struct platform_device *pdev)
>  		goto fail_after_v4l2_m2m_init;
>  	}
>  
> +	pm_runtime_set_autosuspend_delay(dev, JPEG_AUTOSUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_enable(dev);
>  
>  	if (!pm_runtime_enabled(dev)) {
> @@ -1109,6 +1112,11 @@ static int e5010_probe(struct platform_device *pdev)
>  			dev_err_probe(dev, ret, "failed to enable clock\n");
>  			goto fail_after_v4l2_m2m_init;
>  		}
> +		ret = e5010_init_device(e5010);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "failed to init device\n");
> +			goto fail_after_clock_enable;
> +		}
>  	}
>  
>  	ret = video_register_device(e5010->vdev, VFL_TYPE_VIDEO, 0);
> @@ -1123,10 +1131,12 @@ static int e5010_probe(struct platform_device *pdev)
>  	return 0;
>  
>  fail_after_clock_enable:
> -	if (pm_runtime_enabled(dev))
> +	if (pm_runtime_enabled(dev)) {
> +		pm_runtime_dont_use_autosuspend(dev);
>  		pm_runtime_disable(dev);
> -	else
> +	} else {
>  		clk_disable_unprepare(e5010->clk);
> +	}
>  fail_after_v4l2_m2m_init:
>  	v4l2_m2m_release(e5010->m2m_dev);
>  fail_after_v4l2_register:
> @@ -1143,10 +1153,10 @@ static void e5010_remove(struct platform_device *pdev)
>  	video_unregister_device(e5010->vdev);
>  	v4l2_m2m_release(e5010->m2m_dev);
>  	v4l2_device_unregister(&e5010->v4l2_dev);
> +	pm_runtime_dont_use_autosuspend(e5010->dev);
>  	pm_runtime_disable(e5010->dev);
>  	if (!pm_runtime_status_suspended(&pdev->dev))
>  		clk_disable_unprepare(e5010->clk);
> -

nitpick: Line deletion.

With above changes,
Reviewed-by: Devarsh Thakkar <devarsht@ti.com>

Regards
Devarsh

>  }
>  
>  static void e5010_vb2_buffers_return(struct vb2_queue *q, enum vb2_buffer_state state)
> @@ -1307,31 +1317,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)
> @@ -1347,8 +1339,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)
> @@ -1361,7 +1351,6 @@ static void e5010_device_run(void *priv)
>  	unsigned long flags;
>  	int num_planes = ctx->out_queue.fmt->num_planes;
>  
> -	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);
> @@ -1369,6 +1358,14 @@ static void e5010_device_run(void *priv)
>  	if (!s_vb || !d_vb)
>  		goto no_ready_buf_err;
>  
> +	ret = pm_runtime_resume_and_get(e5010->dev);
> +	if (ret < 0) {
> +		dev_err(e5010->dev, "Device failed to turn on\n");
> +		goto no_ready_buf_err;
> +	}
> +
> +	spin_lock_irqsave(&e5010->hw_lock, flags);
> +
>  	s_vb->sequence = ctx->out_queue.sequence++;
>  	d_vb->sequence = ctx->cap_queue.sequence++;
>  
> @@ -1493,6 +1490,8 @@ static void e5010_device_run(void *priv)
>  	ret = e5010_init_device(e5010);
>  	if (ret)
>  		v4l2_err(&e5010->v4l2_dev, "failed to initialize device after reset\n");
> +	spin_unlock_irqrestore(&e5010->hw_lock, flags);
> +	pm_runtime_put_autosuspend(e5010->dev);
>  no_ready_buf_err:
>  	if (s_vb) {
>  		v4l2_m2m_src_buf_remove_by_buf(ctx->fh.m2m_ctx, s_vb);
> @@ -1506,7 +1505,6 @@ static void e5010_device_run(void *priv)
>  		v4l2_m2m_buf_done(d_vb, VB2_BUF_STATE_ERROR);
>  	}
>  	v4l2_m2m_job_finish(e5010->m2m_dev, ctx->fh.m2m_ctx);
> -	spin_unlock_irqrestore(&e5010->hw_lock, flags);
>  }
>  
>  #ifdef CONFIG_PM
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.h b/drivers/media/platform/imagination/e5010-jpeg-enc.h
> index da57bc1baa464..807821ded127e 100644
> --- a/drivers/media/platform/imagination/e5010-jpeg-enc.h
> +++ b/drivers/media/platform/imagination/e5010-jpeg-enc.h
> @@ -23,6 +23,7 @@
>  #define DEFAULT_HEIGHT			480
>  #define E5010_MODULE_NAME		"e5010"
>  #define JPEG_MAX_BYTES_PER_PIXEL	2
> +#define JPEG_AUTOSUSPEND_DELAY_MS	100
>  
>  /* JPEG marker definitions */
>  #define START_OF_IMAGE			0xFFD8