Re: [PATCH v4 4/4] media: imagination: e5010: Enable autosuspend for runtime PM
From: Brandon Brnich
Date: Thu Sep 24 2026 - 15:43:31 EST
Hi Devarsh,
Thanks for the review.
On 9/18/26 10:55, Devarsh Thakkar wrote:
Hi Brandon,
Thanks for the patch.
On 01/09/26 03:02, Brandon Brnich wrote:
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@xxxxxx>
---
.../platform/imagination/e5010-jpeg-enc.c | 41 ++++++++++---------
1 file changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/ drivers/media/platform/imagination/e5010-jpeg-enc.c
index 488f5d7e4c9d5..79347014562bd 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);
@@ -1105,6 +1106,11 @@ static int e5010_probe(struct platform_device *pdev)
goto fail_after_video_register_device;
}
+ ret = e5010_init_device(e5010);
+ if (ret) {
+ dev_err_probe(dev, ret, "failed to init device\n");
+ goto fail_after_clock_enable;
+ }
I think we already do e5010_init_device(e5010) in runtime_resume,
I understand your goal here is to handle for scenario where PM is not enabled but it's kind of duplication for the scenario where PM is enabled already. I would suggest to wrap this (and also the clk_enable from previous patch) with a check :
I think in probe itself have a check :
if (!pm_runtime_enabled(dev)) {
// do clock enable
// do init device
}
I will update v5 to move this check to after pm_runtime_enable is made.
ret = video_register_device(e5010->vdev, VFL_TYPE_VIDEO, 0);
if (ret) {
@@ -1112,7 +1118,11 @@ static int e5010_probe(struct platform_device *pdev)
goto fail_after_clock_enable;
}
+ pm_runtime_set_autosuspend_delay(dev, 100);
Better to use a macro for the timeout delay.
Adding a macro. Do you see any issues with the delay period or is 100ms sufficient?
+ pm_runtime_use_autosuspend(dev);
+ pm_runtime_set_active(dev);
pm_runtime_enable(dev);
+ pm_runtime_idle(dev);
If I add the conditional for enabling clock and doing init_device, then I will remove the set_active and runtime_idle calls. My goal here was to cover the case where pm was enabled at kernel level and we had just set clock to enabled state. With the conditional you are suggesting it should just be:
pm_runtime_set_autosuspend_delay
pm_runtime_use_autosuspend
pm_runtime_enable
if (!pm_runtime_enabled(dev)) {
...
}
v4l2_info(&e5010->v4l2_dev, "Device registered as /dev/video%d\n",
e5010->vdev->num);
@@ -1299,31 +1309,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)
@@ -1339,8 +1331,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)
@@ -1353,7 +1343,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);
You also need to remove buffers from queue and return back as VB2_BUF_ERROR I think you should do this after the initial check in driver, something like below should work :
if (!s_vb || !d_vb) {
/* Buffers aren't even ready; end the job immediately without touching power */
v4l2_m2m_job_finish(e5010->m2m_dev, ctx->fh.m2m_ctx);
return;
}
ret = pm_runtime_resume_and_get(e5010->dev);
if (ret < 0) {
dev_err(e5010->dev, "Device failed to turn on\n");
/* Jump directly to the clean-up path that skips unlocking the unheld spinlock */
goto device_turn_on_fail;
}
spin_lock_irqsave(&e5010->hw_lock, flags);
...
...
device_busy_err:
e5010_reset(e5010->dev, e5010->core_base, e5010->mmu_base);
spin_unlock_irqrestore(&e5010->hw_lock, flags);
device_turn_on_fail:
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);
}
if (d_vb) {
v4l2_m2m_dst_buf_remove_by_buf(ctx->fh.m2m_ctx, d_vb);
/* Payload set to 1 since 0 payload can trigger EOS */
vb2_set_plane_payload(&d_vb->vb2_buf, 0, 1);
v4l2_m2m_buf_done(d_vb, VB2_BUF_STATE_ERROR);
}
v4l2_m2m_job_finish(e5010->m2m_dev, ctx->fh.m2m_ctx);
Agreed with above code block and I add this in v5 of the series.
Best,
Brandon
Regards
Devarsh