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

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

[b-brnich]

From: Brandon Brnich <b-brnich@ti.com>

Hello all,

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. 

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

- Fix patch 1 not properly removing all references to the
  fail_after_video_register_device label

Changes since v4:

- Change authorship of patch 1 since Myeonghun sent a similar patch
  first
- addressed Devarsh's comments in Patch 1 to follow similar naming
  structure as rest of probe function
- Moved the clk_enable call after pm_runtime_enable here so I can check
  success of that call. If runtime pm is enabled then don't enable the
  clock here
- Patch 3 no changes
- Patch 4 I added the macro requested by Devarsh instead of using magic
  number
- failure to power on now goes through the existing no_ready_buf_err path,
  so buffers are marked as error and the job is finished

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 (3):
  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

Myeonghun Pak (1):
  media: imagination: e5010: release m2m device on probe failure

 .../platform/imagination/e5010-jpeg-enc.c     | 86 +++++++++++--------
 .../platform/imagination/e5010-jpeg-enc.h     |  1 +
 2 files changed, 52 insertions(+), 35 deletions(-)

-- 
2.43.0

[PATCH v6 1/4] media: imagination: e5010: release m2m device on probe failure

[b-brnich]

From: Myeonghun Pak <mhun512@gmail.com>

The probe path allocates the mem2mem device with v4l2_m2m_init(),
but several later failures jump directly to the v4l2_device_unregister()
unwind path. Since the driver core does not call .remove() after a failed
probe, those paths leak the m2m device.

Route post-v4l2_m2m_init() failures through the existing m2m release step
before unregistering the v4l2 device.

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

diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
index 420cb8bfdb12a..23d06709a6929 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_v4l2_m2m_init;
 	}
 
 	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_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_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_v4l2_m2m_init;
 	}
 
 	pm_runtime_enable(dev);
@@ -1104,7 +1104,7 @@ static int e5010_probe(struct platform_device *pdev)
 	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_v4l2_m2m_init;
 	}
 
 	v4l2_info(&e5010->v4l2_dev, "Device registered as /dev/video%d\n",
@@ -1112,7 +1112,7 @@ static int e5010_probe(struct platform_device *pdev)
 
 	return 0;
 
-fail_after_video_register_device:
+fail_after_v4l2_m2m_init:
 	v4l2_m2m_release(e5010->m2m_dev);
 fail_after_v4l2_register:
 	v4l2_device_unregister(&e5010->v4l2_dev);
-- 
2.43.0

Re: [PATCH v6 1/4] media: imagination: e5010: release m2m device on probe failure

[Thakkar, Devarsh]

On 23/10/26 20:16, Brnich, Brandon wrote:
> From: Myeonghun Pak <mhun512@gmail.com>
> 
> The probe path allocates the mem2mem device with v4l2_m2m_init(),
> but several later failures jump directly to the v4l2_device_unregister()
> unwind path. Since the driver core does not call .remove() after a failed
> probe, those paths leak the m2m device.
> 
> Route post-v4l2_m2m_init() failures through the existing m2m release step
> before unregistering the v4l2 device.
> 
> Fixes: a1e294045885 ("media: imagination: Add E5010 JPEG Encoder driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>

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

Regards
Devarsh

> ---
>  drivers/media/platform/imagination/e5010-jpeg-enc.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index 420cb8bfdb12a..23d06709a6929 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_v4l2_m2m_init;
>  	}
>  
>  	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_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_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_v4l2_m2m_init;
>  	}
>  
>  	pm_runtime_enable(dev);
> @@ -1104,7 +1104,7 @@ static int e5010_probe(struct platform_device *pdev)
>  	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_v4l2_m2m_init;
>  	}
>  
>  	v4l2_info(&e5010->v4l2_dev, "Device registered as /dev/video%d\n",
> @@ -1112,7 +1112,7 @@ static int e5010_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> -fail_after_video_register_device:
> +fail_after_v4l2_m2m_init:
>  	v4l2_m2m_release(e5010->m2m_dev);
>  fail_after_v4l2_register:
>  	v4l2_device_unregister(&e5010->v4l2_dev);

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

[b-brnich]

From: Brandon Brnich <b-brnich@ti.com>

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 | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
index 23d06709a6929..b8e24f816648d 100644
--- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
+++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
@@ -1101,10 +1101,18 @@ static int e5010_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
+	if (!pm_runtime_enabled(dev)) {
+		ret = clk_prepare_enable(e5010->clk);
+		if (ret) {
+			dev_err_probe(dev, ret, "failed to enable clock\n");
+			goto fail_after_v4l2_m2m_init;
+		}
+	}
+
 	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_v4l2_m2m_init;
+		goto fail_after_clock_enable;
 	}
 
 	v4l2_info(&e5010->v4l2_dev, "Device registered as /dev/video%d\n",
@@ -1112,6 +1120,11 @@ static int e5010_probe(struct platform_device *pdev)
 
 	return 0;
 
+fail_after_clock_enable:
+	if (pm_runtime_enabled(dev))
+		pm_runtime_disable(dev);
+	else
+		clk_disable_unprepare(e5010->clk);
 fail_after_v4l2_m2m_init:
 	v4l2_m2m_release(e5010->m2m_dev);
 fail_after_v4l2_register:
@@ -1126,6 +1139,8 @@ static void e5010_remove(struct platform_device *pdev)
 	struct e5010_dev *e5010 = platform_get_drvdata(pdev);
 
 	pm_runtime_disable(e5010->dev);
+	if (!pm_runtime_status_suspended(&pdev->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

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

[Thakkar, Devarsh]

Hi Brandon,

On 23/10/26 20:16, Brnich, Brandon wrote:
> From: Brandon Brnich <b-brnich@ti.com>
> 
> 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 | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index 23d06709a6929..b8e24f816648d 100644
> --- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
> +++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> @@ -1101,10 +1101,18 @@ static int e5010_probe(struct platform_device *pdev)
>  
>  	pm_runtime_enable(dev);
>  
> +	if (!pm_runtime_enabled(dev)) {
> +		ret = clk_prepare_enable(e5010->clk);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "failed to enable clock\n");
> +			goto fail_after_v4l2_m2m_init;
> +		}
> +	}
> +
>  	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_v4l2_m2m_init;
> +		goto fail_after_clock_enable;
>  	}
>  
>  	v4l2_info(&e5010->v4l2_dev, "Device registered as /dev/video%d\n",
> @@ -1112,6 +1120,11 @@ static int e5010_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> +fail_after_clock_enable:
> +	if (pm_runtime_enabled(dev))
> +		pm_runtime_disable(dev);
> +	else
> +		clk_disable_unprepare(e5010->clk);
>  fail_after_v4l2_m2m_init:
>  	v4l2_m2m_release(e5010->m2m_dev);
>  fail_after_v4l2_register:
> @@ -1126,6 +1139,8 @@ static void e5010_remove(struct platform_device *pdev)
>  	struct e5010_dev *e5010 = platform_get_drvdata(pdev);
>  
>  	pm_runtime_disable(e5010->dev);
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		clk_disable_unprepare(e5010->clk);

I think better to disable clocks after the video device is unregistered.

Regards
Devarsh

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

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

[b-brnich]

From: Brandon Brnich <b-brnich@ti.com>

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>
---
 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 b8e24f816648d..c25ab9627518e 100644
--- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
+++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
@@ -1516,6 +1516,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;
 }
 
@@ -1548,12 +1555,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 v6 4/4] media: imagination: e5010: Enable autosuspend for runtime PM

[b-brnich]

From: Brandon Brnich <b-brnich@ti.com>

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     | 50 +++++++++----------
 .../platform/imagination/e5010-jpeg-enc.h     |  1 +
 2 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
index c25ab9627518e..228137243f749 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);
@@ -1099,6 +1100,8 @@ static int e5010_probe(struct platform_device *pdev)
 		goto fail_after_v4l2_m2m_init;
 	}
 
+	pm_runtime_set_autosuspend_delay(dev, JPEG_AUTOSUSPEND_DELAY_MS);
+	pm_runtime_use_autosuspend(dev);
 	pm_runtime_enable(dev);
 
 	if (!pm_runtime_enabled(dev)) {
@@ -1107,6 +1110,11 @@ static int e5010_probe(struct platform_device *pdev)
 			dev_err_probe(dev, ret, "failed to enable clock\n");
 			goto fail_after_v4l2_m2m_init;
 		}
+		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);
@@ -1121,10 +1129,12 @@ static int e5010_probe(struct platform_device *pdev)
 	return 0;
 
 fail_after_clock_enable:
-	if (pm_runtime_enabled(dev))
+	if (pm_runtime_enabled(dev)) {
 		pm_runtime_disable(dev);
-	else
+		pm_runtime_dont_use_autosuspend(dev);
+	} else {
 		clk_disable_unprepare(e5010->clk);
+	}
 fail_after_v4l2_m2m_init:
 	v4l2_m2m_release(e5010->m2m_dev);
 fail_after_v4l2_register:
@@ -1139,6 +1149,7 @@ static void e5010_remove(struct platform_device *pdev)
 	struct e5010_dev *e5010 = platform_get_drvdata(pdev);
 
 	pm_runtime_disable(e5010->dev);
+	pm_runtime_dont_use_autosuspend(e5010->dev);
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		clk_disable_unprepare(e5010->clk);
 	video_unregister_device(e5010->vdev);
@@ -1304,31 +1315,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)
@@ -1344,8 +1337,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)
@@ -1358,7 +1349,6 @@ static void e5010_device_run(void *priv)
 	unsigned long flags;
 	int num_planes = ctx->out_queue.fmt->num_planes;
 
-	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);
@@ -1366,6 +1356,14 @@ static void e5010_device_run(void *priv)
 	if (!s_vb || !d_vb)
 		goto no_ready_buf_err;
 
+	ret = pm_runtime_resume_and_get(e5010->dev);
+	if (ret < 0) {
+		dev_err(e5010->dev, "Device failed to turn on\n");
+		goto no_ready_buf_err;
+	}
+
+	spin_lock_irqsave(&e5010->hw_lock, flags);
+
 	s_vb->sequence = ctx->out_queue.sequence++;
 	d_vb->sequence = ctx->cap_queue.sequence++;
 
@@ -1487,7 +1485,8 @@ static void e5010_device_run(void *priv)
 
 device_busy_err:
 	e5010_reset(e5010->dev, e5010->core_base, e5010->mmu_base);
-
+	spin_unlock_irqrestore(&e5010->hw_lock, flags);
+	pm_runtime_put_autosuspend(e5010->dev);
 no_ready_buf_err:
 	if (s_vb) {
 		v4l2_m2m_src_buf_remove_by_buf(ctx->fh.m2m_ctx, s_vb);
@@ -1501,7 +1500,6 @@ static void e5010_device_run(void *priv)
 		v4l2_m2m_buf_done(d_vb, VB2_BUF_STATE_ERROR);
 	}
 	v4l2_m2m_job_finish(e5010->m2m_dev, ctx->fh.m2m_ctx);
-	spin_unlock_irqrestore(&e5010->hw_lock, flags);
 }
 
 #ifdef CONFIG_PM
@@ -1523,6 +1521,8 @@ static int e5010_runtime_resume(struct device *dev)
 		return ret;
 	}
 
+	e5010->last_context_run = NULL;
+
 	return 0;
 }
 
diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.h b/drivers/media/platform/imagination/e5010-jpeg-enc.h
index da57bc1baa464..807821ded127e 100644
--- a/drivers/media/platform/imagination/e5010-jpeg-enc.h
+++ b/drivers/media/platform/imagination/e5010-jpeg-enc.h
@@ -23,6 +23,7 @@
 #define DEFAULT_HEIGHT			480
 #define E5010_MODULE_NAME		"e5010"
 #define JPEG_MAX_BYTES_PER_PIXEL	2
+#define JPEG_AUTOSUSPEND_DELAY_MS	100
 
 /* JPEG marker definitions */
 #define START_OF_IMAGE			0xFFD8
-- 
2.43.0

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

[Thakkar, Devarsh]

Hi Brandon,

Thanks for the patch.

On 10/23/2026 8:16 PM, Brnich, Brandon wrote:
> From: Brandon Brnich <b-brnich@ti.com>
> 
> 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     | 50 +++++++++----------
>  .../platform/imagination/e5010-jpeg-enc.h     |  1 +
>  2 files changed, 26 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index c25ab9627518e..228137243f749 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);
> @@ -1099,6 +1100,8 @@ static int e5010_probe(struct platform_device *pdev)
>  		goto fail_after_v4l2_m2m_init;
>  	}
>  
> +	pm_runtime_set_autosuspend_delay(dev, JPEG_AUTOSUSPEND_DELAY_MS);
> +	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_enable(dev);
>  
>  	if (!pm_runtime_enabled(dev)) {
> @@ -1107,6 +1110,11 @@ static int e5010_probe(struct platform_device *pdev)
>  			dev_err_probe(dev, ret, "failed to enable clock\n");
>  			goto fail_after_v4l2_m2m_init;
>  		}
> +		ret = e5010_init_device(e5010);
> +		if (ret) {
> +			dev_err_probe(dev, ret, "failed to init device\n");
> +			goto fail_after_clock_enable;
> +		}

I guess this is needed because this patch drops e5010_init_device() from
start_streaming(), which was the only init for the !CONFIG_PM case.
Please mention that in the commit message if correct.
>  	}
>  
>  	ret = video_register_device(e5010->vdev, VFL_TYPE_VIDEO, 0);
> @@ -1121,10 +1129,12 @@ static int e5010_probe(struct platform_device *pdev)
>  	return 0;
>  
>  fail_after_clock_enable:
> -	if (pm_runtime_enabled(dev))
> +	if (pm_runtime_enabled(dev)) {
>  		pm_runtime_disable(dev);
> -	else
> +		pm_runtime_dont_use_autosuspend(dev);

pm_runtime_dont_use_autosuspend(dev) should be called first before
calling pm_runtime_disable.

> +	} else {
>  		clk_disable_unprepare(e5010->clk);
> +	}
>  fail_after_v4l2_m2m_init:
>  	v4l2_m2m_release(e5010->m2m_dev);
>  fail_after_v4l2_register:
> @@ -1139,6 +1149,7 @@ static void e5010_remove(struct platform_device *pdev)
>  	struct e5010_dev *e5010 = platform_get_drvdata(pdev);
>  
>  	pm_runtime_disable(e5010->dev);
> +	pm_runtime_dont_use_autosuspend(e5010->dev);

Same here.

>  	if (!pm_runtime_status_suspended(&pdev->dev))
>  		clk_disable_unprepare(e5010->clk);
>  	video_unregister_device(e5010->vdev);
> @@ -1304,31 +1315,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)
> @@ -1344,8 +1337,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)
> @@ -1358,7 +1349,6 @@ static void e5010_device_run(void *priv)
>  	unsigned long flags;
>  	int num_planes = ctx->out_queue.fmt->num_planes;
>  
> -	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);
> @@ -1366,6 +1356,14 @@ static void e5010_device_run(void *priv)
>  	if (!s_vb || !d_vb)
>  		goto no_ready_buf_err;
>  
> +	ret = pm_runtime_resume_and_get(e5010->dev);
> +	if (ret < 0) {
> +		dev_err(e5010->dev, "Device failed to turn on\n");
> +		goto no_ready_buf_err;
> +	}
> +
> +	spin_lock_irqsave(&e5010->hw_lock, flags);
> +
>  	s_vb->sequence = ctx->out_queue.sequence++;
>  	d_vb->sequence = ctx->cap_queue.sequence++;
>  
> @@ -1487,7 +1485,8 @@ static void e5010_device_run(void *priv)
>  
>  device_busy_err:
>  	e5010_reset(e5010->dev, e5010->core_base, e5010->mmu_base);
> -

After the reset, the e5010 config registers including interrupt enable
etc are reset too. You need to also call e5010_init_device here post
reset and also e5010->last_context_run need to be set to NULL to reload
qp from saved ctx. You could move e5010->last_context_run = NULL to
e5010_init_device itself as suggested below.

> +	spin_unlock_irqrestore(&e5010->hw_lock, flags);
> +	pm_runtime_put_autosuspend(e5010->dev);
>  no_ready_buf_err:
>  	if (s_vb) {
>  		v4l2_m2m_src_buf_remove_by_buf(ctx->fh.m2m_ctx, s_vb);
> @@ -1501,7 +1500,6 @@ static void e5010_device_run(void *priv)
>  		v4l2_m2m_buf_done(d_vb, VB2_BUF_STATE_ERROR);
>  	}
>  	v4l2_m2m_job_finish(e5010->m2m_dev, ctx->fh.m2m_ctx);
> -	spin_unlock_irqrestore(&e5010->hw_lock, flags);
>  }
>  
>  #ifdef CONFIG_PM
> @@ -1523,6 +1521,8 @@ static int e5010_runtime_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	e5010->last_context_run = NULL;
> +

Above is a bug fix not entirely related to current series, please squash
it with [PATCH v6 3/4] media: imagination: e5010: Move e5010_init_device
to Runtime Resume Hook and reword the commit msg.
As without this the device would not be able to load qp from saved
context and instead use default qp after resume. As a rule of thumb
please set e5010->last_context_run = NULL whenever device is init. It
seems to me better to move e5010->last_context_run = NULL inside
e5010_init_device itself.


Regards
Devarsh

>  	return 0;
>  }
>  
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.h b/drivers/media/platform/imagination/e5010-jpeg-enc.h
> index da57bc1baa464..807821ded127e 100644
> --- a/drivers/media/platform/imagination/e5010-jpeg-enc.h
> +++ b/drivers/media/platform/imagination/e5010-jpeg-enc.h
> @@ -23,6 +23,7 @@
>  #define DEFAULT_HEIGHT			480
>  #define E5010_MODULE_NAME		"e5010"
>  #define JPEG_MAX_BYTES_PER_PIXEL	2
> +#define JPEG_AUTOSUSPEND_DELAY_MS	100
>  
>  /* JPEG marker definitions */
>  #define START_OF_IMAGE			0xFFD8