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

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

Hi Brandon,

Thanks for the patch.

On 10/23/2026 8:16 PM, Brnich, Brandon wrote:
> From: Brandon Brnich <b-brnich@ti.com>
> 
> 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.
> 
> Need to set last_context_run to NULL in resume hook. This will trigger
> QP value updates during next call to device_run.
> 
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> ---
>  .../platform/imagination/e5010-jpeg-enc.c     | 50 +++++++++----------
>  .../platform/imagination/e5010-jpeg-enc.h     |  1 +
>  2 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index c25ab9627518e..228137243f749 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_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)) {
> @@ -1107,6 +1110,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;
> +		}

I guess this is needed because this patch drops e5010_init_device() from
start_streaming(), which was the only init for the !CONFIG_PM case.
Please mention that in the commit message if correct.
>  	}
>  
>  	ret = video_register_device(e5010->vdev, VFL_TYPE_VIDEO, 0);
> @@ -1121,10 +1129,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_disable(dev);
> -	else
> +		pm_runtime_dont_use_autosuspend(dev);

pm_runtime_dont_use_autosuspend(dev) should be called first before
calling pm_runtime_disable.

> +	} else {
>  		clk_disable_unprepare(e5010->clk);
> +	}
>  fail_after_v4l2_m2m_init:
>  	v4l2_m2m_release(e5010->m2m_dev);
>  fail_after_v4l2_register:
> @@ -1139,6 +1149,7 @@ static void e5010_remove(struct platform_device *pdev)
>  	struct e5010_dev *e5010 = platform_get_drvdata(pdev);
>  
>  	pm_runtime_disable(e5010->dev);
> +	pm_runtime_dont_use_autosuspend(e5010->dev);

Same here.

>  	if (!pm_runtime_status_suspended(&pdev->dev))
>  		clk_disable_unprepare(e5010->clk);
>  	video_unregister_device(e5010->vdev);
> @@ -1304,31 +1315,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)
> @@ -1344,8 +1337,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)
> @@ -1358,7 +1349,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);
> @@ -1366,6 +1356,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++;
>  
> @@ -1487,7 +1485,8 @@ static void e5010_device_run(void *priv)
>  
>  device_busy_err:
>  	e5010_reset(e5010->dev, e5010->core_base, e5010->mmu_base);
> -

After the reset, the e5010 config registers including interrupt enable
etc are reset too. You need to also call e5010_init_device here post
reset and also e5010->last_context_run need to be set to NULL to reload
qp from saved ctx. You could move e5010->last_context_run = NULL to
e5010_init_device itself as suggested below.

> +	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);
> @@ -1501,7 +1500,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
> @@ -1523,6 +1521,8 @@ static int e5010_runtime_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	e5010->last_context_run = NULL;
> +

Above is a bug fix not entirely related to current series, please squash
it with [PATCH v6 3/4] media: imagination: e5010: Move e5010_init_device
to Runtime Resume Hook and reword the commit msg.
As without this the device would not be able to load qp from saved
context and instead use default qp after resume. As a rule of thumb
please set e5010->last_context_run = NULL whenever device is init. It
seems to me better to move e5010->last_context_run = NULL inside
e5010_init_device itself.


Regards
Devarsh

>  	return 0;
>  }
>  
> 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