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

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

[Brandon Brnich]

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.

The rest of the series is setting up the driver for properly adding in
support for autosuspend. Last patch adds in autosuspend support and
moves e5010_init_device into probe for cases where PM isn't enabled.

Changes since v7:

- patch 3 subject wasn't formatted correctly after I updated it so
  wasn't properly showing up

Changes since v6:

- Patch 2 changed order of calls during remove cleanup. video unregister
  now happens before pm disable
- patch 3 was updated to account for the case where a reset was called
  and the device was never properly reinitialized
- patch 3 has moved setting the last_run_context to NULL as well to
  always update QP values when device is turned back on/reinitialized
- removed the r-b by Kendall, Devarsh, and Jai since I have made a
  slight code change requested by Devarsh
- patch 4 remove the last_run_context in runtime resume hook that it had
  before because I moved it to patch 3
- patch 4 has updates to commit message to accurately reflect why
  init_device was added to probe
- patch 4 has updates where disable autosuspend now comes before disable
  pm

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: Update e5010_init_device Call Sites to
    Ensure Proper Device State
  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     | 91 +++++++++++--------
 .../platform/imagination/e5010-jpeg-enc.h     |  1 +
 2 files changed, 56 insertions(+), 36 deletions(-)

-- 
2.43.0

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

[Brandon 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>
Reviewed-by: Devarsh Thakkar <devarsht@ti.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

[PATCH v8 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.

Update e5010_remove to ensure that clock is getting disabled in the case
where e5010 was not suspended when remove gets called.

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

diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
index 23d06709a6929..17ea616ff1795 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:
@@ -1125,10 +1138,13 @@ static void e5010_remove(struct platform_device *pdev)
 {
 	struct e5010_dev *e5010 = platform_get_drvdata(pdev);
 
-	pm_runtime_disable(e5010->dev);
 	video_unregister_device(e5010->vdev);
 	v4l2_m2m_release(e5010->m2m_dev);
 	v4l2_device_unregister(&e5010->v4l2_dev);
+	pm_runtime_disable(e5010->dev);
+	if (!pm_runtime_status_suspended(&pdev->dev))
+		clk_disable_unprepare(e5010->clk);
+
 }
 
 static void e5010_vb2_buffers_return(struct vb2_queue *q, enum vb2_buffer_state state)
-- 
2.43.0

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

[Thakkar, Devarsh]

Hi,

On 11/10/2026 10:02 PM, Brnich, Brandon 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.
> 
> Update e5010_remove to ensure that clock is getting disabled in the case
> where e5010 was not suspended when remove gets called.
> 
> Fixes: a1e294045885 ("media: imagination: Add E5010 JPEG Encoder driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> ---
>  .../platform/imagination/e5010-jpeg-enc.c     | 20 +++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index 23d06709a6929..17ea616ff1795 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:
> @@ -1125,10 +1138,13 @@ static void e5010_remove(struct platform_device *pdev)
>  {
>  	struct e5010_dev *e5010 = platform_get_drvdata(pdev);
>  
> -	pm_runtime_disable(e5010->dev);
>  	video_unregister_device(e5010->vdev);
>  	v4l2_m2m_release(e5010->m2m_dev);
>  	v4l2_device_unregister(&e5010->v4l2_dev);
> +	pm_runtime_disable(e5010->dev);
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		clk_disable_unprepare(e5010->clk);
> +

nit: Extra Line added.
With above fixed,
Reviewed-by: Devarsh Thakkar <devarsht@ti.com>

Regards
Devarsh

>  }
>  
>  static void e5010_vb2_buffers_return(struct vb2_queue *q, enum vb2_buffer_state state)

[PATCH v8 3/4] media: imagination: e5010: Update e5010_init_device Call Sites to Ensure Proper Device State

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

e5010_init_device needs to be called after e5010_reset for next
device_run to have all registers properly reset. Without this call,
register values are stale. Similarly, e5010_last_context_run should be
set to NULL in e5010_init_device so the next device_run call does not
use stale QP values.

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

diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
index 17ea616ff1795..4e77112b59197 100644
--- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
+++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
@@ -986,6 +986,8 @@ static int e5010_init_device(struct e5010_dev *e5010)
 {
 	int ret = 0;
 
+	e5010->last_context_run = NULL;
+
 	/*TODO: Set MMU in bypass mode until support for the same is added in driver*/
 	e5010_hw_bypass_mmu(e5010->mmu_base, 1);
 
@@ -1488,7 +1490,9 @@ static void e5010_device_run(void *priv)
 
 device_busy_err:
 	e5010_reset(e5010->dev, e5010->core_base, e5010->mmu_base);
-
+	ret = e5010_init_device(e5010);
+	if (ret)
+		v4l2_err(&e5010->v4l2_dev, "failed to initialize device after reset\n");
 no_ready_buf_err:
 	if (s_vb) {
 		v4l2_m2m_src_buf_remove_by_buf(ctx->fh.m2m_ctx, s_vb);
@@ -1517,6 +1521,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;
 }
 
@@ -1549,12 +1560,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

Re: [PATCH v8 3/4] media: imagination: e5010: Update e5010_init_device Call Sites to Ensure Proper Device State

[Thakkar, Devarsh]

On 11/10/2026 10:02 PM, Brnich, Brandon 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.
> 
> e5010_init_device needs to be called after e5010_reset for next
> device_run to have all registers properly reset. Without this call,
> register values are stale.

Not stale but carrying default un-programmed reset-level values.

 Similarly, e5010_last_context_run should be

There is no var as e5010_last_context_run.

> set to NULL in e5010_init_device so the next device_run call does not
> use stale QP values.

Maybe good to reword above para as below:
e5010_reset() resets the whole IP, which reverts all registers,
including the interrupt enables, input source and quantization
tables, to their reset defaults. Call e5010_init_device() after the
reset so the next device_run() starts from a properly programmed
device. Also clear e5010->last_context_run in e5010_init_device(), so
that the next device_run() reloads the user-configured QP tables
instead of encoding with the default ones.

With above suggestions,
Reviewed-by: Devarsh Thakkar <devarsht@ti.com>

Regards
Devarsh

> 
> Fixes: a1e294045885 ("media: imagination: Add E5010 JPEG Encoder driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> ---
>  .../platform/imagination/e5010-jpeg-enc.c     | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index 17ea616ff1795..4e77112b59197 100644
> --- a/drivers/media/platform/imagination/e5010-jpeg-enc.c
> +++ b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> @@ -986,6 +986,8 @@ static int e5010_init_device(struct e5010_dev *e5010)
>  {
>  	int ret = 0;
>  
> +	e5010->last_context_run = NULL;
> +
>  	/*TODO: Set MMU in bypass mode until support for the same is added in driver*/
>  	e5010_hw_bypass_mmu(e5010->mmu_base, 1);
>  
> @@ -1488,7 +1490,9 @@ static void e5010_device_run(void *priv)
>  
>  device_busy_err:
>  	e5010_reset(e5010->dev, e5010->core_base, e5010->mmu_base);
> -
> +	ret = e5010_init_device(e5010);
> +	if (ret)
> +		v4l2_err(&e5010->v4l2_dev, "failed to initialize device after reset\n");
>  no_ready_buf_err:
>  	if (s_vb) {
>  		v4l2_m2m_src_buf_remove_by_buf(ctx->fh.m2m_ctx, s_vb);
> @@ -1517,6 +1521,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;
>  }
>  
> @@ -1549,12 +1560,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;

[PATCH v8 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 system as the device can be in
STREAM_ON state, but no buffers are actually being submitted.

e5010_init_device is tightly coupled to runtime resume hook with
autosuspend addition and has been removed from start_streaming accordingly.
However, there is the case where runtime PM is not configured which would
leave the device permantately uninitialized. Add call to e5010_init_device
to e5010_probe to account for this case.

Signed-off-by: Brandon Brnich <b-brnich@ti.com>
---
 .../platform/imagination/e5010-jpeg-enc.c     | 48 +++++++++----------
 .../platform/imagination/e5010-jpeg-enc.h     |  1 +
 2 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
index 4e77112b59197..3c7c5f121a32f 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);
@@ -1101,6 +1102,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)) {
@@ -1109,6 +1112,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);
@@ -1123,10 +1131,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_dont_use_autosuspend(dev);
 		pm_runtime_disable(dev);
-	else
+	} else {
 		clk_disable_unprepare(e5010->clk);
+	}
 fail_after_v4l2_m2m_init:
 	v4l2_m2m_release(e5010->m2m_dev);
 fail_after_v4l2_register:
@@ -1143,10 +1153,10 @@ static void e5010_remove(struct platform_device *pdev)
 	video_unregister_device(e5010->vdev);
 	v4l2_m2m_release(e5010->m2m_dev);
 	v4l2_device_unregister(&e5010->v4l2_dev);
+	pm_runtime_dont_use_autosuspend(e5010->dev);
 	pm_runtime_disable(e5010->dev);
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		clk_disable_unprepare(e5010->clk);
-
 }
 
 static void e5010_vb2_buffers_return(struct vb2_queue *q, enum vb2_buffer_state state)
@@ -1307,31 +1317,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)
@@ -1347,8 +1339,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)
@@ -1361,7 +1351,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);
@@ -1369,6 +1358,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++;
 
@@ -1493,6 +1490,8 @@ static void e5010_device_run(void *priv)
 	ret = e5010_init_device(e5010);
 	if (ret)
 		v4l2_err(&e5010->v4l2_dev, "failed to initialize device after reset\n");
+	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);
@@ -1506,7 +1505,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
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 v8 4/4] media: imagination: e5010: Enable autosuspend for runtime PM

[Thakkar, Devarsh]

On 11/10/2026 10:02 PM, Brnich, Brandon wrote:
> Current implementation only has one call to turn the device on -
> start_streaming. This can waste power of system as the device can be in
> STREAM_ON state, but no buffers are actually being submitted.
> 

You should also mention how current patch fixes it:
i.e by taking runtime PM reference device_run and releasing it in
completion interrupt and using PM autosuspend.
> e5010_init_device is tightly coupled to runtime resume hook with
> autosuspend addition and has been removed from start_streaming accordingly.

Has been removed is not right wording since it tends to infer that it
was removed prior to this patch but its actually this one which is
moving out.

> However, there is the case where runtime PM is not configured which would
> leave the device permantately uninitialized. Add call to e5010_init_device
> to e5010_probe to account for this case.

Yes, before this patch it was handled by start_streaming.

I think better wording would be as below:
Since e5010_runtime_resume() now initialises the hardware on every
power-up, drop the e5010_init_device() call from start_streaming().
Without runtime PM, that call in start_streaming() was the only place
the hardware got initialised, so call e5010_init_device() from probe
when runtime PM is not enabled.

> 
> Signed-off-by: Brandon Brnich <b-brnich@ti.com>
> ---
>  .../platform/imagination/e5010-jpeg-enc.c     | 48 +++++++++----------
>  .../platform/imagination/e5010-jpeg-enc.h     |  1 +
>  2 files changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/media/platform/imagination/e5010-jpeg-enc.c b/drivers/media/platform/imagination/e5010-jpeg-enc.c
> index 4e77112b59197..3c7c5f121a32f 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);
> @@ -1101,6 +1102,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)) {
> @@ -1109,6 +1112,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);
> @@ -1123,10 +1131,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_dont_use_autosuspend(dev);
>  		pm_runtime_disable(dev);
> -	else
> +	} else {
>  		clk_disable_unprepare(e5010->clk);
> +	}
>  fail_after_v4l2_m2m_init:
>  	v4l2_m2m_release(e5010->m2m_dev);
>  fail_after_v4l2_register:
> @@ -1143,10 +1153,10 @@ static void e5010_remove(struct platform_device *pdev)
>  	video_unregister_device(e5010->vdev);
>  	v4l2_m2m_release(e5010->m2m_dev);
>  	v4l2_device_unregister(&e5010->v4l2_dev);
> +	pm_runtime_dont_use_autosuspend(e5010->dev);
>  	pm_runtime_disable(e5010->dev);
>  	if (!pm_runtime_status_suspended(&pdev->dev))
>  		clk_disable_unprepare(e5010->clk);
> -

nitpick: Line deletion.

With above changes,
Reviewed-by: Devarsh Thakkar <devarsht@ti.com>

Regards
Devarsh

>  }
>  
>  static void e5010_vb2_buffers_return(struct vb2_queue *q, enum vb2_buffer_state state)
> @@ -1307,31 +1317,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)
> @@ -1347,8 +1339,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)
> @@ -1361,7 +1351,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);
> @@ -1369,6 +1358,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++;
>  
> @@ -1493,6 +1490,8 @@ static void e5010_device_run(void *priv)
>  	ret = e5010_init_device(e5010);
>  	if (ret)
>  		v4l2_err(&e5010->v4l2_dev, "failed to initialize device after reset\n");
> +	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);
> @@ -1506,7 +1505,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
> 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