.../platform/imagination/e5010-jpeg-enc.c | 47 +++++++++---------- 1 file changed, 21 insertions(+), 26 deletions(-)
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.
e5010_init_device needs to be moved to the resume hook to ensure
register state is correct during any power on. This also fixes an issue
when device was never powered on, but system resumes from deep sleep.
The PM framework skips the runtime resume hook since device was never on
in the first place. This causes e5010_init_device to be called for a
powered off device leading to invalid write and system crash.
Fixes: a1e2940458853 ("media: imagination: Add E5010 JPEG Encoder driver")
Signed-off-by: Brandon Brnich <b-brnich@ti.com>
---
.../platform/imagination/e5010-jpeg-enc.c | 47 +++++++++----------
1 file changed, 21 insertions(+), 26 deletions(-)
diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
index 42ad9ee3993b4..189c147cb9705 100644
--- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
+++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
@@ -976,6 +976,8 @@ 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_mark_last_busy(e5010->dev);
+ pm_runtime_put_autosuspend(e5010->dev);
job_unlock:
spin_unlock(&e5010->hw_lock);
@@ -1099,6 +1101,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 +1293,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 +1315,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 +1327,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;
+ }
+
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 +1466,8 @@ static void e5010_device_run(void *priv)
e5010_reset(e5010->dev, e5010->core_base, e5010->mmu_base);
no_ready_buf_err:
+ pm_runtime_mark_last_busy(e5010->dev);
+ 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);
@@ -1501,6 +1495,13 @@ static int e5010_runtime_resume(struct device *dev)
return ret;
}
+ ret = e5010_init_device(e5010);
+ if (ret) {
+ dev_err(dev, "Failed to re-enable e5010 device\n");
+ clk_disable_unprepare(e5010->clk);
+ return ret;
+ }
+
return 0;
}
@@ -1533,12 +1534,6 @@ static int e5010_resume(struct device *dev)
if (ret < 0)
return ret;
- ret = e5010_init_device(e5010);
- if (ret) {
- dev_err(dev, "Failed to re-enable e5010 device\n");
- return ret;
- }
-
v4l2_m2m_resume(e5010->m2m_dev);
return ret;
--
2.43.0Hi Brandon,
Thank you for the patch.
Quoting Brandon Brnich (2026-07-23 02:38:45)
> 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.
>
> e5010_init_device needs to be moved to the resume hook to ensure
> register state is correct during any power on. This also fixes an issue
> when device was never powered on, but system resumes from deep sleep.
> The PM framework skips the runtime resume hook since device was never on
> in the first place. This causes e5010_init_device to be called for a
> powered off device leading to invalid write and system crash.
There are two separate things be solved by this patch, one is the bug fix
and the other is the good-to-have feature for autosuspending the device
between buffers.
It would make it much easier to review the logic if v2 splits the two, and
only keeps the Fixes tag (+cc stable) for the bug fix.
>
> Fixes: a1e2940458853 ("media: imagination: Add E5010 JPEG Encoder driver")
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> ---
> .../platform/imagination/e5010-jpeg-enc.c | 47 +++++++++----------
> 1 file changed, 21 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index 42ad9ee3993b4..189c147cb9705 100644
> --- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
> +++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> @@ -976,6 +976,8 @@ 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_mark_last_busy(e5010->dev);
> + pm_runtime_put_autosuspend(e5010->dev);
The implementation of put_autosuspend() already handles mark_last_busy()
since b3db492e8335 ("PM: runtime: Mark last busy stamp in
pm_runtime_put_autosuspend()")
>
> job_unlock:
> spin_unlock(&e5010->hw_lock);
> @@ -1099,6 +1101,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 +1293,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 +1315,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 +1327,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;
> + }
> +
> 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 +1466,8 @@ static void e5010_device_run(void *priv)
> e5010_reset(e5010->dev, e5010->core_base, e5010->mmu_base);
>
> no_ready_buf_err:
> + pm_runtime_mark_last_busy(e5010->dev);
> + pm_runtime_put_autosuspend(e5010->dev);
Same here.
Thanks,
Jai
> 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);
> @@ -1501,6 +1495,13 @@ static int e5010_runtime_resume(struct device *dev)
> return ret;
> }
>
> + ret = e5010_init_device(e5010);
> + if (ret) {
> + dev_err(dev, "Failed to re-enable e5010 device\n");
> + clk_disable_unprepare(e5010->clk);
> + return ret;
> + }
> +
> return 0;
> }
>
> @@ -1533,12 +1534,6 @@ static int e5010_resume(struct device *dev)
> if (ret < 0)
> return ret;
>
> - ret = e5010_init_device(e5010);
> - if (ret) {
> - dev_err(dev, "Failed to re-enable e5010 device\n");
> - return ret;
> - }
> -
> v4l2_m2m_resume(e5010->m2m_dev);
>
> return ret;
> --
> 2.43.0
>
>
Hi Jai,
Thanks for the review.
On 7/23/26 06:29, Jai Luthra wrote:
> Hi Brandon,
>
> Thank you for the patch.
>
> Quoting Brandon Brnich (2026-07-23 02:38:45)
>> 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.
>>
>> e5010_init_device needs to be moved to the resume hook to ensure
>> register state is correct during any power on. This also fixes an issue
>> when device was never powered on, but system resumes from deep sleep.
>> The PM framework skips the runtime resume hook since device was never on
>> in the first place. This causes e5010_init_device to be called for a
>> powered off device leading to invalid write and system crash.
>
> There are two separate things be solved by this patch, one is the bug fix
> and the other is the good-to-have feature for autosuspending the device
> between buffers.
>
> It would make it much easier to review the logic if v2 splits the two, and
> only keeps the Fixes tag (+cc stable) for the bug fix.
Understood, I will split in 2 patches for v2.
>
>>
>> Fixes: a1e2940458853 ("media: imagination: Add E5010 JPEG Encoder driver")
>> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
>> ---
>> .../platform/imagination/e5010-jpeg-enc.c | 47 +++++++++----------
>> 1 file changed, 21 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
>> index 42ad9ee3993b4..189c147cb9705 100644
>> --- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
>> +++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
>> @@ -976,6 +976,8 @@ 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_mark_last_busy(e5010->dev);
>> + pm_runtime_put_autosuspend(e5010->dev);
>
> The implementation of put_autosuspend() already handles mark_last_busy()
> since b3db492e8335 ("PM: runtime: Mark last busy stamp in
> pm_runtime_put_autosuspend()")
Good to know! I will remove these calls in v2.
Best,
Brandon
>
>>
>> job_unlock:
>> spin_unlock(&e5010->hw_lock);
>> @@ -1099,6 +1101,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 +1293,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 +1315,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 +1327,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;
>> + }
>> +
>> 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 +1466,8 @@ static void e5010_device_run(void *priv)
>> e5010_reset(e5010->dev, e5010->core_base, e5010->mmu_base);
>>
>> no_ready_buf_err:
>> + pm_runtime_mark_last_busy(e5010->dev);
>> + pm_runtime_put_autosuspend(e5010->dev);
>
> Same here.
>
> Thanks,
> Jai
>
>> 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);
>> @@ -1501,6 +1495,13 @@ static int e5010_runtime_resume(struct device *dev)
>> return ret;
>> }
>>
>> + ret = e5010_init_device(e5010);
>> + if (ret) {
>> + dev_err(dev, "Failed to re-enable e5010 device\n");
>> + clk_disable_unprepare(e5010->clk);
>> + return ret;
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -1533,12 +1534,6 @@ static int e5010_resume(struct device *dev)
>> if (ret < 0)
>> return ret;
>>
>> - ret = e5010_init_device(e5010);
>> - if (ret) {
>> - dev_err(dev, "Failed to re-enable e5010 device\n");
>> - return ret;
>> - }
>> -
>> v4l2_m2m_resume(e5010->m2m_dev);
>>
>> return ret;
>> --
>> 2.43.0
>>
>>
© 2016 - 2026 Red Hat, Inc.