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

[Brandon Brnich <b-brnich@ti.com>]

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
>>
>>