[PATCH v4 0/4] E5010 Probe Cleanup and Adding autosuspend

[PATCH v4 0/4] E5010 Probe Cleanup and Adding autosuspend

[Brandon Brnich]

Hello all,

This series has changed a bit since v3 so thought it was appropriate to
add in a cover letter.

I noticed a report from the intel bot[0] which pointed out a valid issue
with the driver never calling e5010_init_device unless CONFIG_PM was
enabled. That has now been addressed in v4 by moving the call to probe.

While looking at probe though, I noticed that the clk_enable was never
properly called. That has now been fixed with a proper remove path as
well.

Lastly, I cleaned up the goto jumps in probe to fix an issue where the
m2m_dev wasn't being freed.

Changes since v3:

  - patches 1 and 2 are new in v4
  - patch 3 remains unchanged
  - patch 4
      > updated goto to match changes in patch 1
      > update PM calls in probe

Best,
Brandon

[0]: https://lore.kernel.org/oe-kbuild-all/202608140323.qyc8mKlp-lkp@intel.com/

Brandon Brnich (4):
  media: imagination: e5010: Properly Release m2m_dev if probe fails
  media: imagination: e5010: Fix clk never enabled without CONFIG_PM
  media: imagination: e5010: Move e5010_init_device to Runtime Resume
    Hook
  media: imagination: e5010: Enable autosuspend for runtime PM

 .../platform/imagination/e5010-jpeg-enc.c     | 76 +++++++++++--------
 1 file changed, 44 insertions(+), 32 deletions(-)

-- 
2.43.0

[PATCH v4 1/4] media: imagination: e5010: Properly Release m2m_dev if probe fails

[Brandon Brnich]

After the call to v4l2_m2m_init() is successful, the following
initialization functions should go to fail_after_video_register_device.

Fixes: a1e294045885 ("media: imagination: Add E5010 JPEG Encoder driver")
Cc: stable@vger.kernel.org
Signed-off-by: Brandon Brnich <b-brnich@ti.com>
---
 drivers/media/platform/imagination/e5010-jpeg-enc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
index 42ad9ee3993b4..d3c11337c815d 100644
--- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
+++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
@@ -1072,14 +1072,14 @@ static int e5010_probe(struct platform_device *pdev)
 	if (IS_ERR(e5010->core_base)) {
 		ret = PTR_ERR(e5010->core_base);
 		dev_err_probe(dev, ret, "Missing 'core' resources area\n");
-		goto fail_after_v4l2_register;
+		goto fail_after_video_register_device;
 	}
 
 	e5010->mmu_base = devm_platform_ioremap_resource_byname(pdev, "mmu");
 	if (IS_ERR(e5010->mmu_base)) {
 		ret = PTR_ERR(e5010->mmu_base);
 		dev_err_probe(dev, ret, "Missing 'mmu' resources area\n");
-		goto fail_after_v4l2_register;
+		goto fail_after_video_register_device;
 	}
 
 	e5010->last_context_run = NULL;
@@ -1089,14 +1089,14 @@ static int e5010_probe(struct platform_device *pdev)
 			       E5010_MODULE_NAME, e5010);
 	if (ret) {
 		dev_err_probe(dev, ret, "failed to register IRQ %d\n", irq);
-		goto fail_after_v4l2_register;
+		goto fail_after_video_register_device;
 	}
 
 	e5010->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(e5010->clk)) {
 		ret = PTR_ERR(e5010->clk);
 		dev_err_probe(dev, ret, "failed to get clock\n");
-		goto fail_after_v4l2_register;
+		goto fail_after_video_register_device;
 	}
 
 	pm_runtime_enable(dev);
-- 
2.43.0

[PATCH v4 2/4] media: imagination: e5010: Fix clk never enabled without CONFIG_PM

[Brandon Brnich]

clk_prepare_enable is not called anywhere unless CONFIG_PM is set. This
means without CONFIG_PM, the JPEG Encoder will not function properly.

Add this call during the probe sequence to properly configure the device.
If video_registration fails after clk is enabled, clk would be left on.
Make a label to cover this case.

Fixes: a1e294045885 ("media: imagination: Add E5010 JPEG Encoder driver")
Cc: stable@vger.kernel.org
Signed-off-by: Brandon Brnich <b-brnich@ti.com>
---
 .../media/platform/imagination/e5010-jpeg-enc.c    | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
index d3c11337c815d..e98fcd3fcd9ae 100644
--- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
+++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
@@ -1099,19 +1099,28 @@ static int e5010_probe(struct platform_device *pdev)
 		goto fail_after_video_register_device;
 	}
 
-	pm_runtime_enable(dev);
+	ret = clk_prepare_enable(e5010->clk);
+	if (ret) {
+		dev_err_probe(dev, ret, "failed to enable clock\n");
+		goto fail_after_video_register_device;
+	}
+
 
 	ret = video_register_device(e5010->vdev, VFL_TYPE_VIDEO, 0);
 	if (ret) {
 		dev_err_probe(dev, ret, "failed to register video device\n");
-		goto fail_after_video_register_device;
+		goto fail_after_clock_enable;
 	}
 
+	pm_runtime_enable(dev);
+
 	v4l2_info(&e5010->v4l2_dev, "Device registered as /dev/video%d\n",
 		  e5010->vdev->num);
 
 	return 0;
 
+fail_after_clock_enable:
+	clk_disable_unprepare(e5010->clk);
 fail_after_video_register_device:
 	v4l2_m2m_release(e5010->m2m_dev);
 fail_after_v4l2_register:
@@ -1126,6 +1135,7 @@ static void e5010_remove(struct platform_device *pdev)
 	struct e5010_dev *e5010 = platform_get_drvdata(pdev);
 
 	pm_runtime_disable(e5010->dev);
+	clk_disable_unprepare(e5010->clk);
 	video_unregister_device(e5010->vdev);
 	v4l2_m2m_release(e5010->m2m_dev);
 	v4l2_device_unregister(&e5010->v4l2_dev);
-- 
2.43.0

[PATCH v4 3/4] media: imagination: e5010: Move e5010_init_device to Runtime Resume Hook

[Brandon Brnich]

The PM framework skips the runtime resume hook if the device was never on
in the first place. This results in a crash in the system resume hook when
a call to e5010_init_device attempts a write to a powered down device.

Move e5010_init_device to the resume hook to ensure register write only
occurs when device is properly powered on.

Fixes: a1e294045885 ("media: imagination: Add E5010 JPEG Encoder driver")
Cc: stable@vger.kernel.org
Signed-off-by: Brandon Brnich <b-brnich@ti.com>
Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
Reviewed-by: Kendall Willis <k-willis@ti.com>
---
 drivers/media/platform/imagination/e5010-jpeg-enc.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
index e98fcd3fcd9ae..488f5d7e4c9d5 100644
--- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
+++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
@@ -1511,6 +1511,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;
 }
 
@@ -1543,12 +1550,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

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

[Brandon Brnich]

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;
+	}
 
 	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);
+	pm_runtime_use_autosuspend(dev);
+	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
+	pm_runtime_idle(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);
+		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);
@@ -1484,6 +1482,7 @@ static void e5010_device_run(void *priv)
 	e5010_reset(e5010->dev, e5010->core_base, e5010->mmu_base);
 
 no_ready_buf_err:
+	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);
@@ -1518,6 +1517,8 @@ static int e5010_runtime_resume(struct device *dev)
 		return ret;
 	}
 
+	e5010->last_context_run = NULL;
+
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH v4 1/4] media: imagination: e5010: Properly Release m2m_dev if probe fails

[Devarsh Thakkar]

Hi Brandon,

Thanks for the patch.

On 01/09/26 03:02, Brandon Brnich wrote:
> After the call to v4l2_m2m_init() is successful, the following
> initialization functions should go to fail_after_video_register_device.
> 
> Fixes: a1e294045885 ("media: imagination: Add E5010 JPEG Encoder driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> ---
>   drivers/media/platform/imagination/e5010-jpeg-enc.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index 42ad9ee3993b4..d3c11337c815d 100644
> --- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
> +++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> @@ -1072,14 +1072,14 @@ static int e5010_probe(struct platform_device *pdev)
>   	if (IS_ERR(e5010->core_base)) {
>   		ret = PTR_ERR(e5010->core_base);
>   		dev_err_probe(dev, ret, "Missing 'core' resources area\n");
> -		goto fail_after_v4l2_register;
> +		goto fail_after_video_register_device;

The label naming looks incorrect semantically since 
video_register_device happens after all this at last actually. Please 
change it to fail_after_v4l2_m2m_init instead.

>   	}
>   
>   	e5010->mmu_base = devm_platform_ioremap_resource_byname(pdev, "mmu");
>   	if (IS_ERR(e5010->mmu_base)) {
>   		ret = PTR_ERR(e5010->mmu_base);
>   		dev_err_probe(dev, ret, "Missing 'mmu' resources area\n");
> -		goto fail_after_v4l2_register;
> +		goto fail_after_video_register_device;
>   	}
>   

goto fail_after_v4l2_m2m_init

>   	e5010->last_context_run = NULL;
> @@ -1089,14 +1089,14 @@ static int e5010_probe(struct platform_device *pdev)
>   			       E5010_MODULE_NAME, e5010);
>   	if (ret) {
>   		dev_err_probe(dev, ret, "failed to register IRQ %d\n", irq);
> -		goto fail_after_v4l2_register;
> +		goto fail_after_video_register_device;
>   	}

goto fail_after_v4l2_m2m_init

>   
>   	e5010->clk = devm_clk_get(dev, NULL);
>   	if (IS_ERR(e5010->clk)) {
>   		ret = PTR_ERR(e5010->clk);
>   		dev_err_probe(dev, ret, "failed to get clock\n");
> -		goto fail_after_v4l2_register;
> +		goto fail_after_video_register_device;
>   	}

goto fail_after_v4l2_m2m_init
>   
>   	pm_runtime_enable(dev);

Regards
Devarsh

Re: [PATCH v4 2/4] media: imagination: e5010: Fix clk never enabled without CONFIG_PM

[Devarsh Thakkar]

Hi Brandon,

Thanks for the patch.

On 01/09/26 03:02, Brandon Brnich wrote:
> clk_prepare_enable is not called anywhere unless CONFIG_PM is set. This
> means without CONFIG_PM, the JPEG Encoder will not function properly.
> 
> Add this call during the probe sequence to properly configure the device.
> If video_registration fails after clk is enabled, clk would be left on.
> Make a label to cover this case.
> 
> Fixes: a1e294045885 ("media: imagination: Add E5010 JPEG Encoder driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> ---
>   .../media/platform/imagination/e5010-jpeg-enc.c    | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index d3c11337c815d..e98fcd3fcd9ae 100644
> --- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
> +++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> @@ -1099,19 +1099,28 @@ static int e5010_probe(struct platform_device *pdev)
>   		goto fail_after_video_register_device;
>   	}
>   
> -	pm_runtime_enable(dev);
> +	ret = clk_prepare_enable(e5010->clk);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "failed to enable clock\n");
> +		goto fail_after_video_register_device;
> +	}
> +
>   
>   	ret = video_register_device(e5010->vdev, VFL_TYPE_VIDEO, 0);
>   	if (ret) {
>   		dev_err_probe(dev, ret, "failed to register video device\n");
> -		goto fail_after_video_register_device;
> +		goto fail_after_clock_enable;
>   	}
>   
> +	pm_runtime_enable(dev);
> +
>   	v4l2_info(&e5010->v4l2_dev, "Device registered as /dev/video%d\n",
>   		  e5010->vdev->num);
>   
>   	return 0;
>   
> +fail_after_clock_enable:
> +	clk_disable_unprepare(e5010->clk);
>   fail_after_video_register_device:
>   	v4l2_m2m_release(e5010->m2m_dev);
>   fail_after_v4l2_register:
> @@ -1126,6 +1135,7 @@ static void e5010_remove(struct platform_device *pdev)
>   	struct e5010_dev *e5010 = platform_get_drvdata(pdev);
>   
>   	pm_runtime_disable(e5010->dev);
> +	clk_disable_unprepare(e5010->clk);

I think you should only disable if driver was not suspended as you 
already do a clk_disable in suspend path, i think it should be :

         if (!pm_runtime_status_suspended(&pdev->dev)) 

		clk_disable_unprepare(e5010->clk);

Regards
Devarsh
>   	video_unregister_device(e5010->vdev);
>   	v4l2_m2m_release(e5010->m2m_dev);
>   	v4l2_device_unregister(&e5010->v4l2_dev);

Re: [PATCH v4 3/4] media: imagination: e5010: Move e5010_init_device to Runtime Resume Hook

[Devarsh Thakkar]

On 01/09/26 03:02, Brandon Brnich wrote:
> The PM framework skips the runtime resume hook if the device was never on
> in the first place. This results in a crash in the system resume hook when
> a call to e5010_init_device attempts a write to a powered down device.
> 
> Move e5010_init_device to the resume hook to ensure register write only
> occurs when device is properly powered on.
> 
> Fixes: a1e294045885 ("media: imagination: Add E5010 JPEG Encoder driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com>
> Reviewed-by: Kendall Willis <k-willis@ti.com>

Reviewed-by: Devarsh Thakkar <devarsht@ti.com>

Regards
Devarsh

> ---
>   drivers/media/platform/imagination/e5010-jpeg-enc.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index e98fcd3fcd9ae..488f5d7e4c9d5 100644
> --- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
> +++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> @@ -1511,6 +1511,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;
>   }
>   
> @@ -1543,12 +1550,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;

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

[Devarsh Thakkar]

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

}


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

> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_active(dev);
>   	pm_runtime_enable(dev);
> +	pm_runtime_idle(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);

Regards
Devarsh


> +		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);
> @@ -1484,6 +1482,7 @@ static void e5010_device_run(void *priv)
>   	e5010_reset(e5010->dev, e5010->core_base, e5010->mmu_base);
>   
>   no_ready_buf_err:
> +	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);
> @@ -1518,6 +1517,8 @@ static int e5010_runtime_resume(struct device *dev)
>   		return ret;
>   	}
>   
> +	e5010->last_context_run = NULL;
> +
>   	return 0;
>   }
>   

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

[Brandon Brnich]

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

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

[Devarsh Thakkar]

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