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

[Devarsh Thakkar <devarsht@ti.com>]

Hi Brandon,

On 25/09/26 00:55, Brandon Brnich wrote:
> 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@ti.com>
>>> ---
>>>   .../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. 

Yes basically, we don't power on the device and clock on probe. We only 
do it when application actually needs the hardware to be ON.

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

Thanks, also in v5 please put up a summary and changelong in the cover 
letter for reference.

Regards
Devarsh

> Best,
> Brandon
> 
>>
>> Regards
>> Devarsh
> 
>