* [PATCH v3] media: hantro: release runtime resources when device_run fails
@ 2026-07-26 2:42 ` Tharit Tangkijwanichakul
0 siblings, 0 replies; 26+ messages in thread
From: Tharit Tangkijwanichakul @ 2026-07-26 2:42 UTC (permalink / raw)
To: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel,
Mauro Carvalho Chehab
Cc: Ezequiel Garcia, Hans Verkuil, linux-media, linux-rockchip,
linux-kernel, linux-kernel-mentees, skhan, me, jkoolstra,
Frank.li, Tharit Tangkijwanichakul
device_run() acquires a runtime PM reference and enables the VPU clocks
before invoking the codec-specific run callback.
If clk_bulk_enable() fails, the runtime PM reference is left held. If
the codec-specific run callback fails, both the enabled clocks and the
runtime PM reference are left held.
Make the clocks part of the device's runtime PM state by enabling them
from the runtime-resume callback and disabling them from the
runtime-suspend callback.
Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
Signed-off-by: Tharit Tangkijwanichakul <tharitt97@gmail.com>
---
Changes in v3:
- Move clock enable and disable operations into the runtime PM
callbacks, as suggested by Frank Li.
- Remove vpu from argument in hantro_job_finish() and
hantro_job_finish_no_pm(). It is accessible by ctx.
Changes in v2:
- Fix the codec run failure path to disable the clocks before dropping
the runtime PM reference.
- Use a local vpu variable in device_run().
Tested on a ROCK 5B (RK3588) using the Hantro G1 decoder and Fluster:
H.264 (JVT-AVC_V1): 129/135, unchanged
MPEG-2 (MPEG2_VIDEO-MAIN): 23/43, unchanged
VP8 (VP8-TEST-VECTORS): 61/61, unchanged
.../media/platform/verisilicon/hantro_drv.c | 68 +++++++++++--------
1 file changed, 39 insertions(+), 29 deletions(-)
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 2e81877f640f..7978bb72569e 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -59,8 +59,7 @@ static const struct v4l2_event hantro_eos_event = {
.type = V4L2_EVENT_EOS
};
-static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
- struct hantro_ctx *ctx,
+static void hantro_job_finish_no_pm(struct hantro_ctx *ctx,
enum vb2_buffer_state result)
{
struct vb2_v4l2_buffer *src, *dst;
@@ -86,15 +85,13 @@ static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
result);
}
-static void hantro_job_finish(struct hantro_dev *vpu,
- struct hantro_ctx *ctx,
+static void hantro_job_finish(struct hantro_ctx *ctx,
enum vb2_buffer_state result)
{
- pm_runtime_put_autosuspend(vpu->dev);
-
- clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
+ struct hantro_dev *vpu = ctx->dev;
- hantro_job_finish_no_pm(vpu, ctx, result);
+ pm_runtime_put_autosuspend(vpu->dev);
+ hantro_job_finish_no_pm(ctx, result);
}
void hantro_irq_done(struct hantro_dev *vpu,
@@ -111,7 +108,7 @@ void hantro_irq_done(struct hantro_dev *vpu,
if (cancel_delayed_work(&vpu->watchdog_work)) {
if (result == VB2_BUF_STATE_DONE && ctx->codec_ops->done)
ctx->codec_ops->done(ctx);
- hantro_job_finish(vpu, ctx, result);
+ hantro_job_finish(ctx, result);
}
}
@@ -127,7 +124,7 @@ void hantro_watchdog(struct work_struct *work)
vpu_err("frame processing timed out!\n");
if (ctx->codec_ops->reset)
ctx->codec_ops->reset(ctx);
- hantro_job_finish(vpu, ctx, VB2_BUF_STATE_ERROR);
+ hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
}
}
@@ -170,29 +167,24 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
static void device_run(void *priv)
{
struct hantro_ctx *ctx = priv;
+ struct hantro_dev *vpu = ctx->dev;
struct vb2_v4l2_buffer *src, *dst;
int ret;
src = hantro_get_src_buf(ctx);
dst = hantro_get_dst_buf(ctx);
- ret = pm_runtime_resume_and_get(ctx->dev->dev);
- if (ret < 0)
- goto err_cancel_job;
-
- ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
- if (ret)
- goto err_cancel_job;
+ ret = pm_runtime_resume_and_get(vpu->dev);
+ if (ret < 0) {
+ hantro_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
+ return;
+ }
v4l2_m2m_buf_copy_metadata(src, dst);
- if (ctx->codec_ops->run(ctx))
- goto err_cancel_job;
-
- return;
-
-err_cancel_job:
- hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
+ ret = ctx->codec_ops->run(ctx);
+ if (ret)
+ hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
}
static const struct v4l2_m2m_ops vpu_m2m_ops = {
@@ -1292,22 +1284,40 @@ static void hantro_remove(struct platform_device *pdev)
pm_runtime_disable(vpu->dev);
}
-#ifdef CONFIG_PM
static int hantro_runtime_resume(struct device *dev)
{
struct hantro_dev *vpu = dev_get_drvdata(dev);
+ int ret;
- if (vpu->variant->runtime_resume)
- return vpu->variant->runtime_resume(vpu);
+ ret = clk_bulk_enable(vpu->variant->num_clocks, vpu->clocks);
+ if (ret)
+ return ret;
+
+ if (vpu->variant->runtime_resume) {
+ ret = vpu->variant->runtime_resume(vpu);
+ if (ret)
+ goto err_disable_clocks;
+ }
return 0;
+
+err_disable_clocks:
+ clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
+ return ret;
+}
+
+static int hantro_runtime_suspend(struct device *dev)
+{
+ struct hantro_dev *vpu = dev_get_drvdata(dev);
+
+ clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
+ return 0;
}
-#endif
static const struct dev_pm_ops hantro_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
pm_runtime_force_resume)
- SET_RUNTIME_PM_OPS(NULL, hantro_runtime_resume, NULL)
+ SET_RUNTIME_PM_OPS(hantro_runtime_suspend, hantro_runtime_resume, NULL)
};
static struct platform_driver hantro_driver = {
--
2.47.3
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v3] media: hantro: release runtime resources when device_run fails
2026-07-26 2:42 ` Tharit Tangkijwanichakul
@ 2026-07-27 15:18 ` Frank Li
-1 siblings, 0 replies; 26+ messages in thread
From: Frank Li @ 2026-07-27 15:18 UTC (permalink / raw)
To: Tharit Tangkijwanichakul
Cc: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel,
Mauro Carvalho Chehab, Ezequiel Garcia, Hans Verkuil, linux-media,
linux-rockchip, linux-kernel, linux-kernel-mentees, skhan, me,
jkoolstra
On Sun, Jul 26, 2026 at 02:42:43AM +0000, Tharit Tangkijwanichakul wrote:
> [You don't often get email from tharitt97@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> device_run() acquires a runtime PM reference and enables the VPU clocks
> before invoking the codec-specific run callback.
>
> If clk_bulk_enable() fails, the runtime PM reference is left held. If
> the codec-specific run callback fails, both the enabled clocks and the
> runtime PM reference are left held.
>
> Make the clocks part of the device's runtime PM state by enabling them
> from the runtime-resume callback and disabling them from the
> runtime-suspend callback.
>
> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> Signed-off-by: Tharit Tangkijwanichakul <tharitt97@gmail.com>
> ---
> Changes in v3:
> - Move clock enable and disable operations into the runtime PM
> callbacks, as suggested by Frank Li.
> - Remove vpu from argument in hantro_job_finish() and
> hantro_job_finish_no_pm(). It is accessible by ctx.
>
> Changes in v2:
> - Fix the codec run failure path to disable the clocks before dropping
> the runtime PM reference.
> - Use a local vpu variable in device_run().
>
> Tested on a ROCK 5B (RK3588) using the Hantro G1 decoder and Fluster:
>
> H.264 (JVT-AVC_V1): 129/135, unchanged
> MPEG-2 (MPEG2_VIDEO-MAIN): 23/43, unchanged
> VP8 (VP8-TEST-VECTORS): 61/61, unchanged
>
> .../media/platform/verisilicon/hantro_drv.c | 68 +++++++++++--------
> 1 file changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 2e81877f640f..7978bb72569e 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -59,8 +59,7 @@ static const struct v4l2_event hantro_eos_event = {
> .type = V4L2_EVENT_EOS
> };
>
> -static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> - struct hantro_ctx *ctx,
> +static void hantro_job_finish_no_pm(struct hantro_ctx *ctx,
> enum vb2_buffer_state result)
> {
> struct vb2_v4l2_buffer *src, *dst;
> @@ -86,15 +85,13 @@ static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> result);
> }
>
> -static void hantro_job_finish(struct hantro_dev *vpu,
> - struct hantro_ctx *ctx,
> +static void hantro_job_finish(struct hantro_ctx *ctx,
> enum vb2_buffer_state result)
> {
> - pm_runtime_put_autosuspend(vpu->dev);
> -
> - clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> + struct hantro_dev *vpu = ctx->dev;
>
> - hantro_job_finish_no_pm(vpu, ctx, result);
> + pm_runtime_put_autosuspend(vpu->dev);
> + hantro_job_finish_no_pm(ctx, result);
> }
>
> void hantro_irq_done(struct hantro_dev *vpu,
> @@ -111,7 +108,7 @@ void hantro_irq_done(struct hantro_dev *vpu,
> if (cancel_delayed_work(&vpu->watchdog_work)) {
> if (result == VB2_BUF_STATE_DONE && ctx->codec_ops->done)
> ctx->codec_ops->done(ctx);
> - hantro_job_finish(vpu, ctx, result);
> + hantro_job_finish(ctx, result);
> }
> }
>
> @@ -127,7 +124,7 @@ void hantro_watchdog(struct work_struct *work)
> vpu_err("frame processing timed out!\n");
> if (ctx->codec_ops->reset)
> ctx->codec_ops->reset(ctx);
> - hantro_job_finish(vpu, ctx, VB2_BUF_STATE_ERROR);
> + hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
> }
> }
>
> @@ -170,29 +167,24 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
> static void device_run(void *priv)
> {
> struct hantro_ctx *ctx = priv;
> + struct hantro_dev *vpu = ctx->dev;
> struct vb2_v4l2_buffer *src, *dst;
> int ret;
>
> src = hantro_get_src_buf(ctx);
> dst = hantro_get_dst_buf(ctx);
>
> - ret = pm_runtime_resume_and_get(ctx->dev->dev);
> - if (ret < 0)
> - goto err_cancel_job;
> -
> - ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> - if (ret)
> - goto err_cancel_job;
> + ret = pm_runtime_resume_and_get(vpu->dev);
> + if (ret < 0) {
> + hantro_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
> + return;
> + }
>
> v4l2_m2m_buf_copy_metadata(src, dst);
>
> - if (ctx->codec_ops->run(ctx))
> - goto err_cancel_job;
> -
> - return;
> -
> -err_cancel_job:
> - hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> + ret = ctx->codec_ops->run(ctx);
> + if (ret)
> + hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
> }
>
> static const struct v4l2_m2m_ops vpu_m2m_ops = {
> @@ -1292,22 +1284,40 @@ static void hantro_remove(struct platform_device *pdev)
> pm_runtime_disable(vpu->dev);
> }
>
> -#ifdef CONFIG_PM
> static int hantro_runtime_resume(struct device *dev)
> {
> struct hantro_dev *vpu = dev_get_drvdata(dev);
> + int ret;
>
> - if (vpu->variant->runtime_resume)
> - return vpu->variant->runtime_resume(vpu);
> + ret = clk_bulk_enable(vpu->variant->num_clocks, vpu->clocks);
> + if (ret)
> + return ret;
> +
> + if (vpu->variant->runtime_resume) {
> + ret = vpu->variant->runtime_resume(vpu);
> + if (ret)
> + goto err_disable_clocks;
> + }
>
> return 0;
> +
> +err_disable_clocks:
> + clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> + return ret;
> +}
> +
> +static int hantro_runtime_suspend(struct device *dev)
> +{
> + struct hantro_dev *vpu = dev_get_drvdata(dev);
> +
> + clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> + return 0;
> }
> -#endif
>
> static const struct dev_pm_ops hantro_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> pm_runtime_force_resume)
> - SET_RUNTIME_PM_OPS(NULL, hantro_runtime_resume, NULL)
> + SET_RUNTIME_PM_OPS(hantro_runtime_suspend, hantro_runtime_resume, NULL)
You need convert to model RUMTIME_PM_OPS and SYSTEM_SLEEP_PM_OPS
pm_ptr(hantro_pm_ops) at below hantro_driver
and remove CONFIG_PM first,
then add move clock manage into runtime pm management
If you remove "CONFIG_PM" here, cause build error at some special config.
Or keep CONFIG_PM at this patch, then convert to RUMTIME_PM_OPS() in
following patch.
Frank
> };
>
> static struct platform_driver hantro_driver = {
> --
> 2.47.3
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v3] media: hantro: release runtime resources when device_run fails
@ 2026-07-27 15:18 ` Frank Li
0 siblings, 0 replies; 26+ messages in thread
From: Frank Li @ 2026-07-27 15:18 UTC (permalink / raw)
To: Tharit Tangkijwanichakul
Cc: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel,
Mauro Carvalho Chehab, Ezequiel Garcia, Hans Verkuil, linux-media,
linux-rockchip, linux-kernel, linux-kernel-mentees, skhan, me,
jkoolstra
On Sun, Jul 26, 2026 at 02:42:43AM +0000, Tharit Tangkijwanichakul wrote:
> [You don't often get email from tharitt97@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> device_run() acquires a runtime PM reference and enables the VPU clocks
> before invoking the codec-specific run callback.
>
> If clk_bulk_enable() fails, the runtime PM reference is left held. If
> the codec-specific run callback fails, both the enabled clocks and the
> runtime PM reference are left held.
>
> Make the clocks part of the device's runtime PM state by enabling them
> from the runtime-resume callback and disabling them from the
> runtime-suspend callback.
>
> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> Signed-off-by: Tharit Tangkijwanichakul <tharitt97@gmail.com>
> ---
> Changes in v3:
> - Move clock enable and disable operations into the runtime PM
> callbacks, as suggested by Frank Li.
> - Remove vpu from argument in hantro_job_finish() and
> hantro_job_finish_no_pm(). It is accessible by ctx.
>
> Changes in v2:
> - Fix the codec run failure path to disable the clocks before dropping
> the runtime PM reference.
> - Use a local vpu variable in device_run().
>
> Tested on a ROCK 5B (RK3588) using the Hantro G1 decoder and Fluster:
>
> H.264 (JVT-AVC_V1): 129/135, unchanged
> MPEG-2 (MPEG2_VIDEO-MAIN): 23/43, unchanged
> VP8 (VP8-TEST-VECTORS): 61/61, unchanged
>
> .../media/platform/verisilicon/hantro_drv.c | 68 +++++++++++--------
> 1 file changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 2e81877f640f..7978bb72569e 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -59,8 +59,7 @@ static const struct v4l2_event hantro_eos_event = {
> .type = V4L2_EVENT_EOS
> };
>
> -static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> - struct hantro_ctx *ctx,
> +static void hantro_job_finish_no_pm(struct hantro_ctx *ctx,
> enum vb2_buffer_state result)
> {
> struct vb2_v4l2_buffer *src, *dst;
> @@ -86,15 +85,13 @@ static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> result);
> }
>
> -static void hantro_job_finish(struct hantro_dev *vpu,
> - struct hantro_ctx *ctx,
> +static void hantro_job_finish(struct hantro_ctx *ctx,
> enum vb2_buffer_state result)
> {
> - pm_runtime_put_autosuspend(vpu->dev);
> -
> - clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> + struct hantro_dev *vpu = ctx->dev;
>
> - hantro_job_finish_no_pm(vpu, ctx, result);
> + pm_runtime_put_autosuspend(vpu->dev);
> + hantro_job_finish_no_pm(ctx, result);
> }
>
> void hantro_irq_done(struct hantro_dev *vpu,
> @@ -111,7 +108,7 @@ void hantro_irq_done(struct hantro_dev *vpu,
> if (cancel_delayed_work(&vpu->watchdog_work)) {
> if (result == VB2_BUF_STATE_DONE && ctx->codec_ops->done)
> ctx->codec_ops->done(ctx);
> - hantro_job_finish(vpu, ctx, result);
> + hantro_job_finish(ctx, result);
> }
> }
>
> @@ -127,7 +124,7 @@ void hantro_watchdog(struct work_struct *work)
> vpu_err("frame processing timed out!\n");
> if (ctx->codec_ops->reset)
> ctx->codec_ops->reset(ctx);
> - hantro_job_finish(vpu, ctx, VB2_BUF_STATE_ERROR);
> + hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
> }
> }
>
> @@ -170,29 +167,24 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
> static void device_run(void *priv)
> {
> struct hantro_ctx *ctx = priv;
> + struct hantro_dev *vpu = ctx->dev;
> struct vb2_v4l2_buffer *src, *dst;
> int ret;
>
> src = hantro_get_src_buf(ctx);
> dst = hantro_get_dst_buf(ctx);
>
> - ret = pm_runtime_resume_and_get(ctx->dev->dev);
> - if (ret < 0)
> - goto err_cancel_job;
> -
> - ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> - if (ret)
> - goto err_cancel_job;
> + ret = pm_runtime_resume_and_get(vpu->dev);
> + if (ret < 0) {
> + hantro_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
> + return;
> + }
>
> v4l2_m2m_buf_copy_metadata(src, dst);
>
> - if (ctx->codec_ops->run(ctx))
> - goto err_cancel_job;
> -
> - return;
> -
> -err_cancel_job:
> - hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> + ret = ctx->codec_ops->run(ctx);
> + if (ret)
> + hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
> }
>
> static const struct v4l2_m2m_ops vpu_m2m_ops = {
> @@ -1292,22 +1284,40 @@ static void hantro_remove(struct platform_device *pdev)
> pm_runtime_disable(vpu->dev);
> }
>
> -#ifdef CONFIG_PM
> static int hantro_runtime_resume(struct device *dev)
> {
> struct hantro_dev *vpu = dev_get_drvdata(dev);
> + int ret;
>
> - if (vpu->variant->runtime_resume)
> - return vpu->variant->runtime_resume(vpu);
> + ret = clk_bulk_enable(vpu->variant->num_clocks, vpu->clocks);
> + if (ret)
> + return ret;
> +
> + if (vpu->variant->runtime_resume) {
> + ret = vpu->variant->runtime_resume(vpu);
> + if (ret)
> + goto err_disable_clocks;
> + }
>
> return 0;
> +
> +err_disable_clocks:
> + clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> + return ret;
> +}
> +
> +static int hantro_runtime_suspend(struct device *dev)
> +{
> + struct hantro_dev *vpu = dev_get_drvdata(dev);
> +
> + clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> + return 0;
> }
> -#endif
>
> static const struct dev_pm_ops hantro_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> pm_runtime_force_resume)
> - SET_RUNTIME_PM_OPS(NULL, hantro_runtime_resume, NULL)
> + SET_RUNTIME_PM_OPS(hantro_runtime_suspend, hantro_runtime_resume, NULL)
You need convert to model RUMTIME_PM_OPS and SYSTEM_SLEEP_PM_OPS
pm_ptr(hantro_pm_ops) at below hantro_driver
and remove CONFIG_PM first,
then add move clock manage into runtime pm management
If you remove "CONFIG_PM" here, cause build error at some special config.
Or keep CONFIG_PM at this patch, then convert to RUMTIME_PM_OPS() in
following patch.
Frank
> };
>
> static struct platform_driver hantro_driver = {
> --
> 2.47.3
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v3] media: hantro: release runtime resources when device_run fails
2026-07-27 15:18 ` Frank Li
@ 2026-07-28 4:30 ` Tharit Tangkijwanichakul
-1 siblings, 0 replies; 26+ messages in thread
From: Tharit Tangkijwanichakul @ 2026-07-28 4:30 UTC (permalink / raw)
To: Frank Li
Cc: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel,
Mauro Carvalho Chehab, Ezequiel Garcia, Hans Verkuil, linux-media,
linux-rockchip, linux-kernel, linux-kernel-mentees, skhan, me,
jkoolstra
> You need convert to model RUMTIME_PM_OPS and SYSTEM_SLEEP_PM_OPS
>
> pm_ptr(hantro_pm_ops) at below hantro_driver
>
> and remove CONFIG_PM first,
>
> then add move clock manage into runtime pm management
>
> If you remove "CONFIG_PM" here, cause build error at some special config.
>
> Or keep CONFIG_PM at this patch, then convert to RUMTIME_PM_OPS() in
> following patch.
>
> Frank
I see, the compiler may flag resume/suspend callback unused if
CONFIG_PM=n and cause build error. Thanks for pointing this out.
I will keep CONFIG_PM and send a conversion in a subsequent patch.
Best,
Tharit
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3] media: hantro: release runtime resources when device_run fails
@ 2026-07-28 4:30 ` Tharit Tangkijwanichakul
0 siblings, 0 replies; 26+ messages in thread
From: Tharit Tangkijwanichakul @ 2026-07-28 4:30 UTC (permalink / raw)
To: Frank Li
Cc: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel,
Mauro Carvalho Chehab, Ezequiel Garcia, Hans Verkuil, linux-media,
linux-rockchip, linux-kernel, linux-kernel-mentees, skhan, me,
jkoolstra
> You need convert to model RUMTIME_PM_OPS and SYSTEM_SLEEP_PM_OPS
>
> pm_ptr(hantro_pm_ops) at below hantro_driver
>
> and remove CONFIG_PM first,
>
> then add move clock manage into runtime pm management
>
> If you remove "CONFIG_PM" here, cause build error at some special config.
>
> Or keep CONFIG_PM at this patch, then convert to RUMTIME_PM_OPS() in
> following patch.
>
> Frank
I see, the compiler may flag resume/suspend callback unused if
CONFIG_PM=n and cause build error. Thanks for pointing this out.
I will keep CONFIG_PM and send a conversion in a subsequent patch.
Best,
Tharit
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4] media: hantro: release runtime resources when device_run fails
2026-07-26 2:42 ` Tharit Tangkijwanichakul
@ 2026-07-28 4:59 ` Tharit Tangkijwanichakul
-1 siblings, 0 replies; 26+ messages in thread
From: Tharit Tangkijwanichakul @ 2026-07-28 4:59 UTC (permalink / raw)
To: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel,
Mauro Carvalho Chehab
Cc: Ezequiel Garcia, Hans Verkuil, linux-media, linux-rockchip,
linux-kernel, linux-arm-kernel, linux-kernel-mentees, skhan, me,
jkoolstra, Frank.li, Tharit Tangkijwanichakul
device_run() acquires a runtime PM reference and enables the VPU clocks
before invoking the codec-specific run callback.
If clk_bulk_enable() fails, the runtime PM reference is left held. If
the codec-specific run callback fails, both the enabled clocks and the
runtime PM reference are left held.
Make the clocks part of the device's runtime PM state by enabling them
from the runtime-resume callback and disabling them from the
runtime-suspend callback.
Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
Signed-off-by: Tharit Tangkijwanichakul <tharitt97@gmail.com>
---
Changes in v4:
- Keep CONFIG_PM guard to avoid possible build error, as suggested by Frank Li.
Changes in v3:
- Move clock enable and disable operations into the runtime PM
callbacks, as suggested by Frank Li.
- Remove vpu from argument in hantro_job_finish() and
hantro_job_finish_no_pm(). It is accessible by ctx.
Changes in v2:
- Fix the codec run failure path to disable the clocks before dropping
the runtime PM reference.
- Use a local vpu variable in device_run().
Tested on a ROCK 5B (RK3588) using the Hantro G1 decoder and Fluster:
H.264 (JVT-AVC_V1): 129/135, unchanged
MPEG-2 (MPEG2_VIDEO-MAIN): 23/43, unchanged
VP8 (VP8-TEST-VECTORS): 61/61, unchanged
.../media/platform/verisilicon/hantro_drv.c | 66 +++++++++++--------
1 file changed, 39 insertions(+), 27 deletions(-)
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 2e81877f640f..d6fcd4f7f9da 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -59,8 +59,7 @@ static const struct v4l2_event hantro_eos_event = {
.type = V4L2_EVENT_EOS
};
-static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
- struct hantro_ctx *ctx,
+static void hantro_job_finish_no_pm(struct hantro_ctx *ctx,
enum vb2_buffer_state result)
{
struct vb2_v4l2_buffer *src, *dst;
@@ -86,15 +85,13 @@ static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
result);
}
-static void hantro_job_finish(struct hantro_dev *vpu,
- struct hantro_ctx *ctx,
+static void hantro_job_finish(struct hantro_ctx *ctx,
enum vb2_buffer_state result)
{
- pm_runtime_put_autosuspend(vpu->dev);
-
- clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
+ struct hantro_dev *vpu = ctx->dev;
- hantro_job_finish_no_pm(vpu, ctx, result);
+ pm_runtime_put_autosuspend(vpu->dev);
+ hantro_job_finish_no_pm(ctx, result);
}
void hantro_irq_done(struct hantro_dev *vpu,
@@ -111,7 +108,7 @@ void hantro_irq_done(struct hantro_dev *vpu,
if (cancel_delayed_work(&vpu->watchdog_work)) {
if (result == VB2_BUF_STATE_DONE && ctx->codec_ops->done)
ctx->codec_ops->done(ctx);
- hantro_job_finish(vpu, ctx, result);
+ hantro_job_finish(ctx, result);
}
}
@@ -127,7 +124,7 @@ void hantro_watchdog(struct work_struct *work)
vpu_err("frame processing timed out!\n");
if (ctx->codec_ops->reset)
ctx->codec_ops->reset(ctx);
- hantro_job_finish(vpu, ctx, VB2_BUF_STATE_ERROR);
+ hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
}
}
@@ -170,29 +167,24 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
static void device_run(void *priv)
{
struct hantro_ctx *ctx = priv;
+ struct hantro_dev *vpu = ctx->dev;
struct vb2_v4l2_buffer *src, *dst;
int ret;
src = hantro_get_src_buf(ctx);
dst = hantro_get_dst_buf(ctx);
- ret = pm_runtime_resume_and_get(ctx->dev->dev);
- if (ret < 0)
- goto err_cancel_job;
-
- ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
- if (ret)
- goto err_cancel_job;
+ ret = pm_runtime_resume_and_get(vpu->dev);
+ if (ret < 0) {
+ hantro_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
+ return;
+ }
v4l2_m2m_buf_copy_metadata(src, dst);
- if (ctx->codec_ops->run(ctx))
- goto err_cancel_job;
-
- return;
-
-err_cancel_job:
- hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
+ ret = ctx->codec_ops->run(ctx);
+ if (ret)
+ hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
}
static const struct v4l2_m2m_ops vpu_m2m_ops = {
@@ -1296,10 +1288,30 @@ static void hantro_remove(struct platform_device *pdev)
static int hantro_runtime_resume(struct device *dev)
{
struct hantro_dev *vpu = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_bulk_enable(vpu->variant->num_clocks, vpu->clocks);
+ if (ret)
+ return ret;
+
+ if (vpu->variant->runtime_resume) {
+ ret = vpu->variant->runtime_resume(vpu);
+ if (ret)
+ goto err_disable_clocks;
+ }
+
+ return 0;
- if (vpu->variant->runtime_resume)
- return vpu->variant->runtime_resume(vpu);
+err_disable_clocks:
+ clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
+ return ret;
+}
+static int hantro_runtime_suspend(struct device *dev)
+{
+ struct hantro_dev *vpu = dev_get_drvdata(dev);
+
+ clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
return 0;
}
#endif
@@ -1307,7 +1319,7 @@ static int hantro_runtime_resume(struct device *dev)
static const struct dev_pm_ops hantro_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
pm_runtime_force_resume)
- SET_RUNTIME_PM_OPS(NULL, hantro_runtime_resume, NULL)
+ SET_RUNTIME_PM_OPS(hantro_runtime_suspend, hantro_runtime_resume, NULL)
};
static struct platform_driver hantro_driver = {
--
2.47.3
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4] media: hantro: release runtime resources when device_run fails
@ 2026-07-28 4:59 ` Tharit Tangkijwanichakul
0 siblings, 0 replies; 26+ messages in thread
From: Tharit Tangkijwanichakul @ 2026-07-28 4:59 UTC (permalink / raw)
To: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel,
Mauro Carvalho Chehab
Cc: Ezequiel Garcia, Hans Verkuil, linux-media, linux-rockchip,
linux-kernel, linux-arm-kernel, linux-kernel-mentees, skhan, me,
jkoolstra, Frank.li, Tharit Tangkijwanichakul
device_run() acquires a runtime PM reference and enables the VPU clocks
before invoking the codec-specific run callback.
If clk_bulk_enable() fails, the runtime PM reference is left held. If
the codec-specific run callback fails, both the enabled clocks and the
runtime PM reference are left held.
Make the clocks part of the device's runtime PM state by enabling them
from the runtime-resume callback and disabling them from the
runtime-suspend callback.
Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
Signed-off-by: Tharit Tangkijwanichakul <tharitt97@gmail.com>
---
Changes in v4:
- Keep CONFIG_PM guard to avoid possible build error, as suggested by Frank Li.
Changes in v3:
- Move clock enable and disable operations into the runtime PM
callbacks, as suggested by Frank Li.
- Remove vpu from argument in hantro_job_finish() and
hantro_job_finish_no_pm(). It is accessible by ctx.
Changes in v2:
- Fix the codec run failure path to disable the clocks before dropping
the runtime PM reference.
- Use a local vpu variable in device_run().
Tested on a ROCK 5B (RK3588) using the Hantro G1 decoder and Fluster:
H.264 (JVT-AVC_V1): 129/135, unchanged
MPEG-2 (MPEG2_VIDEO-MAIN): 23/43, unchanged
VP8 (VP8-TEST-VECTORS): 61/61, unchanged
.../media/platform/verisilicon/hantro_drv.c | 66 +++++++++++--------
1 file changed, 39 insertions(+), 27 deletions(-)
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 2e81877f640f..d6fcd4f7f9da 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -59,8 +59,7 @@ static const struct v4l2_event hantro_eos_event = {
.type = V4L2_EVENT_EOS
};
-static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
- struct hantro_ctx *ctx,
+static void hantro_job_finish_no_pm(struct hantro_ctx *ctx,
enum vb2_buffer_state result)
{
struct vb2_v4l2_buffer *src, *dst;
@@ -86,15 +85,13 @@ static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
result);
}
-static void hantro_job_finish(struct hantro_dev *vpu,
- struct hantro_ctx *ctx,
+static void hantro_job_finish(struct hantro_ctx *ctx,
enum vb2_buffer_state result)
{
- pm_runtime_put_autosuspend(vpu->dev);
-
- clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
+ struct hantro_dev *vpu = ctx->dev;
- hantro_job_finish_no_pm(vpu, ctx, result);
+ pm_runtime_put_autosuspend(vpu->dev);
+ hantro_job_finish_no_pm(ctx, result);
}
void hantro_irq_done(struct hantro_dev *vpu,
@@ -111,7 +108,7 @@ void hantro_irq_done(struct hantro_dev *vpu,
if (cancel_delayed_work(&vpu->watchdog_work)) {
if (result == VB2_BUF_STATE_DONE && ctx->codec_ops->done)
ctx->codec_ops->done(ctx);
- hantro_job_finish(vpu, ctx, result);
+ hantro_job_finish(ctx, result);
}
}
@@ -127,7 +124,7 @@ void hantro_watchdog(struct work_struct *work)
vpu_err("frame processing timed out!\n");
if (ctx->codec_ops->reset)
ctx->codec_ops->reset(ctx);
- hantro_job_finish(vpu, ctx, VB2_BUF_STATE_ERROR);
+ hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
}
}
@@ -170,29 +167,24 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
static void device_run(void *priv)
{
struct hantro_ctx *ctx = priv;
+ struct hantro_dev *vpu = ctx->dev;
struct vb2_v4l2_buffer *src, *dst;
int ret;
src = hantro_get_src_buf(ctx);
dst = hantro_get_dst_buf(ctx);
- ret = pm_runtime_resume_and_get(ctx->dev->dev);
- if (ret < 0)
- goto err_cancel_job;
-
- ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
- if (ret)
- goto err_cancel_job;
+ ret = pm_runtime_resume_and_get(vpu->dev);
+ if (ret < 0) {
+ hantro_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
+ return;
+ }
v4l2_m2m_buf_copy_metadata(src, dst);
- if (ctx->codec_ops->run(ctx))
- goto err_cancel_job;
-
- return;
-
-err_cancel_job:
- hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
+ ret = ctx->codec_ops->run(ctx);
+ if (ret)
+ hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
}
static const struct v4l2_m2m_ops vpu_m2m_ops = {
@@ -1296,10 +1288,30 @@ static void hantro_remove(struct platform_device *pdev)
static int hantro_runtime_resume(struct device *dev)
{
struct hantro_dev *vpu = dev_get_drvdata(dev);
+ int ret;
+
+ ret = clk_bulk_enable(vpu->variant->num_clocks, vpu->clocks);
+ if (ret)
+ return ret;
+
+ if (vpu->variant->runtime_resume) {
+ ret = vpu->variant->runtime_resume(vpu);
+ if (ret)
+ goto err_disable_clocks;
+ }
+
+ return 0;
- if (vpu->variant->runtime_resume)
- return vpu->variant->runtime_resume(vpu);
+err_disable_clocks:
+ clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
+ return ret;
+}
+static int hantro_runtime_suspend(struct device *dev)
+{
+ struct hantro_dev *vpu = dev_get_drvdata(dev);
+
+ clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
return 0;
}
#endif
@@ -1307,7 +1319,7 @@ static int hantro_runtime_resume(struct device *dev)
static const struct dev_pm_ops hantro_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
pm_runtime_force_resume)
- SET_RUNTIME_PM_OPS(NULL, hantro_runtime_resume, NULL)
+ SET_RUNTIME_PM_OPS(hantro_runtime_suspend, hantro_runtime_resume, NULL)
};
static struct platform_driver hantro_driver = {
--
2.47.3
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3] media: hantro: release runtime resources when device_run fails
2026-07-26 2:42 ` Tharit Tangkijwanichakul
@ 2026-07-28 15:53 ` Frank Li
-1 siblings, 0 replies; 26+ messages in thread
From: Frank Li @ 2026-07-28 15:53 UTC (permalink / raw)
To: Tharit Tangkijwanichakul
Cc: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel,
Mauro Carvalho Chehab, Ezequiel Garcia, Hans Verkuil, linux-media,
linux-rockchip, linux-kernel, linux-kernel-mentees, skhan, me,
jkoolstra
On Sun, Jul 26, 2026 at 02:42:43AM +0000, Tharit Tangkijwanichakul wrote:
> [You don't often get email from tharitt97@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> device_run() acquires a runtime PM reference and enables the VPU clocks
> before invoking the codec-specific run callback.
>
> If clk_bulk_enable() fails, the runtime PM reference is left held. If
> the codec-specific run callback fails, both the enabled clocks and the
> runtime PM reference are left held.
>
> Make the clocks part of the device's runtime PM state by enabling them
> from the runtime-resume callback and disabling them from the
> runtime-suspend callback.
>
> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> Signed-off-by: Tharit Tangkijwanichakul <tharitt97@gmail.com>
> ---
> Changes in v3:
> - Move clock enable and disable operations into the runtime PM
> callbacks, as suggested by Frank Li.
> - Remove vpu from argument in hantro_job_finish() and
> hantro_job_finish_no_pm(). It is accessible by ctx.
>
> Changes in v2:
> - Fix the codec run failure path to disable the clocks before dropping
> the runtime PM reference.
> - Use a local vpu variable in device_run().
>
> Tested on a ROCK 5B (RK3588) using the Hantro G1 decoder and Fluster:
>
> H.264 (JVT-AVC_V1): 129/135, unchanged
> MPEG-2 (MPEG2_VIDEO-MAIN): 23/43, unchanged
> VP8 (VP8-TEST-VECTORS): 61/61, unchanged
>
> .../media/platform/verisilicon/hantro_drv.c | 68 +++++++++++--------
> 1 file changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 2e81877f640f..7978bb72569e 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -59,8 +59,7 @@ static const struct v4l2_event hantro_eos_event = {
> .type = V4L2_EVENT_EOS
> };
>
> -static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> - struct hantro_ctx *ctx,
> +static void hantro_job_finish_no_pm(struct hantro_ctx *ctx,
> enum vb2_buffer_state result)
> {
> struct vb2_v4l2_buffer *src, *dst;
> @@ -86,15 +85,13 @@ static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> result);
> }
>
> -static void hantro_job_finish(struct hantro_dev *vpu,
> - struct hantro_ctx *ctx,
> +static void hantro_job_finish(struct hantro_ctx *ctx,
> enum vb2_buffer_state result)
> {
> - pm_runtime_put_autosuspend(vpu->dev);
> -
> - clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> + struct hantro_dev *vpu = ctx->dev;
>
> - hantro_job_finish_no_pm(vpu, ctx, result);
> + pm_runtime_put_autosuspend(vpu->dev);
> + hantro_job_finish_no_pm(ctx, result);
> }
>
> void hantro_irq_done(struct hantro_dev *vpu,
> @@ -111,7 +108,7 @@ void hantro_irq_done(struct hantro_dev *vpu,
> if (cancel_delayed_work(&vpu->watchdog_work)) {
> if (result == VB2_BUF_STATE_DONE && ctx->codec_ops->done)
> ctx->codec_ops->done(ctx);
> - hantro_job_finish(vpu, ctx, result);
> + hantro_job_finish(ctx, result);
> }
> }
>
> @@ -127,7 +124,7 @@ void hantro_watchdog(struct work_struct *work)
> vpu_err("frame processing timed out!\n");
> if (ctx->codec_ops->reset)
> ctx->codec_ops->reset(ctx);
> - hantro_job_finish(vpu, ctx, VB2_BUF_STATE_ERROR);
> + hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
> }
> }
>
> @@ -170,29 +167,24 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
> static void device_run(void *priv)
> {
> struct hantro_ctx *ctx = priv;
> + struct hantro_dev *vpu = ctx->dev;
> struct vb2_v4l2_buffer *src, *dst;
> int ret;
>
> src = hantro_get_src_buf(ctx);
> dst = hantro_get_dst_buf(ctx);
>
> - ret = pm_runtime_resume_and_get(ctx->dev->dev);
> - if (ret < 0)
> - goto err_cancel_job;
> -
> - ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> - if (ret)
> - goto err_cancel_job;
> + ret = pm_runtime_resume_and_get(vpu->dev);
> + if (ret < 0) {
> + hantro_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
> + return;
> + }
>
> v4l2_m2m_buf_copy_metadata(src, dst);
>
> - if (ctx->codec_ops->run(ctx))
> - goto err_cancel_job;
> -
> - return;
> -
> -err_cancel_job:
> - hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> + ret = ctx->codec_ops->run(ctx);
> + if (ret)
> + hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
> }
>
> static const struct v4l2_m2m_ops vpu_m2m_ops = {
> @@ -1292,22 +1284,40 @@ static void hantro_remove(struct platform_device *pdev)
> pm_runtime_disable(vpu->dev);
> }
>
> -#ifdef CONFIG_PM
You need sperate patch to remove this. simple remove this here may cause
build error or warning at NO CONFIG_PM config
I suggest use following patch do below thing
1. Remove CONFIG_PM
2. Use DEFINE_RUNTIME_DEV_PM_OP()
3, Need update
.pm = pm_ptr(&hantro_pm_ops),
Frank
> static int hantro_runtime_resume(struct device *dev)
> {
> struct hantro_dev *vpu = dev_get_drvdata(dev);
> + int ret;
>
> - if (vpu->variant->runtime_resume)
> - return vpu->variant->runtime_resume(vpu);
> + ret = clk_bulk_enable(vpu->variant->num_clocks, vpu->clocks);
> + if (ret)
> + return ret;
> +
> + if (vpu->variant->runtime_resume) {
> + ret = vpu->variant->runtime_resume(vpu);
> + if (ret)
> + goto err_disable_clocks;
> + }
>
> return 0;
> +
> +err_disable_clocks:
> + clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> + return ret;
> +}
> +
> +static int hantro_runtime_suspend(struct device *dev)
> +{
> + struct hantro_dev *vpu = dev_get_drvdata(dev);
> +
> + clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> + return 0;
> }
> -#endif
>
> static const struct dev_pm_ops hantro_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> pm_runtime_force_resume)
> - SET_RUNTIME_PM_OPS(NULL, hantro_runtime_resume, NULL)
> + SET_RUNTIME_PM_OPS(hantro_runtime_suspend, hantro_runtime_resume, NULL)
> };
>
> static struct platform_driver hantro_driver = {
> --
> 2.47.3
>
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v3] media: hantro: release runtime resources when device_run fails
@ 2026-07-28 15:53 ` Frank Li
0 siblings, 0 replies; 26+ messages in thread
From: Frank Li @ 2026-07-28 15:53 UTC (permalink / raw)
To: Tharit Tangkijwanichakul
Cc: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel,
Mauro Carvalho Chehab, Ezequiel Garcia, Hans Verkuil, linux-media,
linux-rockchip, linux-kernel, linux-kernel-mentees, skhan, me,
jkoolstra
On Sun, Jul 26, 2026 at 02:42:43AM +0000, Tharit Tangkijwanichakul wrote:
> [You don't often get email from tharitt97@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> device_run() acquires a runtime PM reference and enables the VPU clocks
> before invoking the codec-specific run callback.
>
> If clk_bulk_enable() fails, the runtime PM reference is left held. If
> the codec-specific run callback fails, both the enabled clocks and the
> runtime PM reference are left held.
>
> Make the clocks part of the device's runtime PM state by enabling them
> from the runtime-resume callback and disabling them from the
> runtime-suspend callback.
>
> Fixes: 775fec69008d ("media: add Rockchip VPU JPEG encoder driver")
> Signed-off-by: Tharit Tangkijwanichakul <tharitt97@gmail.com>
> ---
> Changes in v3:
> - Move clock enable and disable operations into the runtime PM
> callbacks, as suggested by Frank Li.
> - Remove vpu from argument in hantro_job_finish() and
> hantro_job_finish_no_pm(). It is accessible by ctx.
>
> Changes in v2:
> - Fix the codec run failure path to disable the clocks before dropping
> the runtime PM reference.
> - Use a local vpu variable in device_run().
>
> Tested on a ROCK 5B (RK3588) using the Hantro G1 decoder and Fluster:
>
> H.264 (JVT-AVC_V1): 129/135, unchanged
> MPEG-2 (MPEG2_VIDEO-MAIN): 23/43, unchanged
> VP8 (VP8-TEST-VECTORS): 61/61, unchanged
>
> .../media/platform/verisilicon/hantro_drv.c | 68 +++++++++++--------
> 1 file changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index 2e81877f640f..7978bb72569e 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -59,8 +59,7 @@ static const struct v4l2_event hantro_eos_event = {
> .type = V4L2_EVENT_EOS
> };
>
> -static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> - struct hantro_ctx *ctx,
> +static void hantro_job_finish_no_pm(struct hantro_ctx *ctx,
> enum vb2_buffer_state result)
> {
> struct vb2_v4l2_buffer *src, *dst;
> @@ -86,15 +85,13 @@ static void hantro_job_finish_no_pm(struct hantro_dev *vpu,
> result);
> }
>
> -static void hantro_job_finish(struct hantro_dev *vpu,
> - struct hantro_ctx *ctx,
> +static void hantro_job_finish(struct hantro_ctx *ctx,
> enum vb2_buffer_state result)
> {
> - pm_runtime_put_autosuspend(vpu->dev);
> -
> - clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> + struct hantro_dev *vpu = ctx->dev;
>
> - hantro_job_finish_no_pm(vpu, ctx, result);
> + pm_runtime_put_autosuspend(vpu->dev);
> + hantro_job_finish_no_pm(ctx, result);
> }
>
> void hantro_irq_done(struct hantro_dev *vpu,
> @@ -111,7 +108,7 @@ void hantro_irq_done(struct hantro_dev *vpu,
> if (cancel_delayed_work(&vpu->watchdog_work)) {
> if (result == VB2_BUF_STATE_DONE && ctx->codec_ops->done)
> ctx->codec_ops->done(ctx);
> - hantro_job_finish(vpu, ctx, result);
> + hantro_job_finish(ctx, result);
> }
> }
>
> @@ -127,7 +124,7 @@ void hantro_watchdog(struct work_struct *work)
> vpu_err("frame processing timed out!\n");
> if (ctx->codec_ops->reset)
> ctx->codec_ops->reset(ctx);
> - hantro_job_finish(vpu, ctx, VB2_BUF_STATE_ERROR);
> + hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
> }
> }
>
> @@ -170,29 +167,24 @@ void hantro_end_prepare_run(struct hantro_ctx *ctx)
> static void device_run(void *priv)
> {
> struct hantro_ctx *ctx = priv;
> + struct hantro_dev *vpu = ctx->dev;
> struct vb2_v4l2_buffer *src, *dst;
> int ret;
>
> src = hantro_get_src_buf(ctx);
> dst = hantro_get_dst_buf(ctx);
>
> - ret = pm_runtime_resume_and_get(ctx->dev->dev);
> - if (ret < 0)
> - goto err_cancel_job;
> -
> - ret = clk_bulk_enable(ctx->dev->variant->num_clocks, ctx->dev->clocks);
> - if (ret)
> - goto err_cancel_job;
> + ret = pm_runtime_resume_and_get(vpu->dev);
> + if (ret < 0) {
> + hantro_job_finish_no_pm(ctx, VB2_BUF_STATE_ERROR);
> + return;
> + }
>
> v4l2_m2m_buf_copy_metadata(src, dst);
>
> - if (ctx->codec_ops->run(ctx))
> - goto err_cancel_job;
> -
> - return;
> -
> -err_cancel_job:
> - hantro_job_finish_no_pm(ctx->dev, ctx, VB2_BUF_STATE_ERROR);
> + ret = ctx->codec_ops->run(ctx);
> + if (ret)
> + hantro_job_finish(ctx, VB2_BUF_STATE_ERROR);
> }
>
> static const struct v4l2_m2m_ops vpu_m2m_ops = {
> @@ -1292,22 +1284,40 @@ static void hantro_remove(struct platform_device *pdev)
> pm_runtime_disable(vpu->dev);
> }
>
> -#ifdef CONFIG_PM
You need sperate patch to remove this. simple remove this here may cause
build error or warning at NO CONFIG_PM config
I suggest use following patch do below thing
1. Remove CONFIG_PM
2. Use DEFINE_RUNTIME_DEV_PM_OP()
3, Need update
.pm = pm_ptr(&hantro_pm_ops),
Frank
> static int hantro_runtime_resume(struct device *dev)
> {
> struct hantro_dev *vpu = dev_get_drvdata(dev);
> + int ret;
>
> - if (vpu->variant->runtime_resume)
> - return vpu->variant->runtime_resume(vpu);
> + ret = clk_bulk_enable(vpu->variant->num_clocks, vpu->clocks);
> + if (ret)
> + return ret;
> +
> + if (vpu->variant->runtime_resume) {
> + ret = vpu->variant->runtime_resume(vpu);
> + if (ret)
> + goto err_disable_clocks;
> + }
>
> return 0;
> +
> +err_disable_clocks:
> + clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> + return ret;
> +}
> +
> +static int hantro_runtime_suspend(struct device *dev)
> +{
> + struct hantro_dev *vpu = dev_get_drvdata(dev);
> +
> + clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
> + return 0;
> }
> -#endif
>
> static const struct dev_pm_ops hantro_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> pm_runtime_force_resume)
> - SET_RUNTIME_PM_OPS(NULL, hantro_runtime_resume, NULL)
> + SET_RUNTIME_PM_OPS(hantro_runtime_suspend, hantro_runtime_resume, NULL)
> };
>
> static struct platform_driver hantro_driver = {
> --
> 2.47.3
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v3] media: hantro: release runtime resources when device_run fails
2026-07-28 15:53 ` Frank Li
@ 2026-07-29 5:59 ` Tharit Tangkijwanichakul
-1 siblings, 0 replies; 26+ messages in thread
From: Tharit Tangkijwanichakul @ 2026-07-29 5:59 UTC (permalink / raw)
To: Frank Li
Cc: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel,
Mauro Carvalho Chehab, Ezequiel Garcia, Hans Verkuil, linux-media,
linux-rockchip, linux-kernel, linux-kernel-mentees, skhan, me,
jkoolstra
> You need sperate patch to remove this. simple remove this here may cause
> build error or warning at NO CONFIG_PM config
>
> I suggest use following patch do below thing
>
> 1. Remove CONFIG_PM
>
> 2. Use DEFINE_RUNTIME_DEV_PM_OP()
>
> 3, Need update
>
> .pm = pm_ptr(&hantro_pm_ops),
>
> Frank
Thanks for the suggestion and the patience walking me through. I will
send the v5 series as suggested.
Best,
Tharit
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3] media: hantro: release runtime resources when device_run fails
@ 2026-07-29 5:59 ` Tharit Tangkijwanichakul
0 siblings, 0 replies; 26+ messages in thread
From: Tharit Tangkijwanichakul @ 2026-07-29 5:59 UTC (permalink / raw)
To: Frank Li
Cc: Nicolas Dufresne, Benjamin Gaignard, Philipp Zabel,
Mauro Carvalho Chehab, Ezequiel Garcia, Hans Verkuil, linux-media,
linux-rockchip, linux-kernel, linux-kernel-mentees, skhan, me,
jkoolstra
> You need sperate patch to remove this. simple remove this here may cause
> build error or warning at NO CONFIG_PM config
>
> I suggest use following patch do below thing
>
> 1. Remove CONFIG_PM
>
> 2. Use DEFINE_RUNTIME_DEV_PM_OP()
>
> 3, Need update
>
> .pm = pm_ptr(&hantro_pm_ops),
>
> Frank
Thanks for the suggestion and the patience walking me through. I will
send the v5 series as suggested.
Best,
Tharit
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3] media: hantro: release runtime resources when device_run fails
2026-07-26 2:42 ` Tharit Tangkijwanichakul
@ 2026-07-29 20:37 ` kernel test robot
-1 siblings, 0 replies; 26+ messages in thread
From: kernel test robot @ 2026-07-29 20:37 UTC (permalink / raw)
To: Tharit Tangkijwanichakul, Nicolas Dufresne, Benjamin Gaignard,
Philipp Zabel, Mauro Carvalho Chehab
Cc: oe-kbuild-all, linux-media, Ezequiel Garcia, Hans Verkuil,
linux-rockchip, linux-kernel, linux-kernel-mentees, skhan, me,
jkoolstra, Frank.li, Tharit Tangkijwanichakul
Hi Tharit,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on pza/reset/next v7.2-rc5 next-20260729]
[cannot apply to pza/imx-drm/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Tharit-Tangkijwanichakul/media-hantro-release-runtime-resources-when-device_run-fails/20260726-104549
base: linus/master
patch link: https://lore.kernel.org/r/20260726024243.730-1-tharitt97%40gmail.com
patch subject: [PATCH v3] media: hantro: release runtime resources when device_run fails
config: nios2-allmodconfig (https://download.01.org/0day-ci/archive/20260730/202607300319.RuKrwvI7-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 11.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260730/202607300319.RuKrwvI7-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202607300319.RuKrwvI7-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/media/platform/verisilicon/hantro_drv.c:1309:12: warning: 'hantro_runtime_suspend' defined but not used [-Wunused-function]
1309 | static int hantro_runtime_suspend(struct device *dev)
| ^~~~~~~~~~~~~~~~~~~~~~
>> drivers/media/platform/verisilicon/hantro_drv.c:1287:12: warning: 'hantro_runtime_resume' defined but not used [-Wunused-function]
1287 | static int hantro_runtime_resume(struct device *dev)
| ^~~~~~~~~~~~~~~~~~~~~
vim +/hantro_runtime_suspend +1309 drivers/media/platform/verisilicon/hantro_drv.c
1286
> 1287 static int hantro_runtime_resume(struct device *dev)
1288 {
1289 struct hantro_dev *vpu = dev_get_drvdata(dev);
1290 int ret;
1291
1292 ret = clk_bulk_enable(vpu->variant->num_clocks, vpu->clocks);
1293 if (ret)
1294 return ret;
1295
1296 if (vpu->variant->runtime_resume) {
1297 ret = vpu->variant->runtime_resume(vpu);
1298 if (ret)
1299 goto err_disable_clocks;
1300 }
1301
1302 return 0;
1303
1304 err_disable_clocks:
1305 clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
1306 return ret;
1307 }
1308
> 1309 static int hantro_runtime_suspend(struct device *dev)
1310 {
1311 struct hantro_dev *vpu = dev_get_drvdata(dev);
1312
1313 clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
1314 return 0;
1315 }
1316
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v3] media: hantro: release runtime resources when device_run fails
@ 2026-07-29 20:37 ` kernel test robot
0 siblings, 0 replies; 26+ messages in thread
From: kernel test robot @ 2026-07-29 20:37 UTC (permalink / raw)
To: Tharit Tangkijwanichakul, Nicolas Dufresne, Benjamin Gaignard,
Philipp Zabel, Mauro Carvalho Chehab
Cc: oe-kbuild-all, linux-media, Ezequiel Garcia, Hans Verkuil,
linux-rockchip, linux-kernel, linux-kernel-mentees, skhan, me,
jkoolstra, Frank.li, Tharit Tangkijwanichakul
Hi Tharit,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on pza/reset/next v7.2-rc5 next-20260729]
[cannot apply to pza/imx-drm/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Tharit-Tangkijwanichakul/media-hantro-release-runtime-resources-when-device_run-fails/20260726-104549
base: linus/master
patch link: https://lore.kernel.org/r/20260726024243.730-1-tharitt97%40gmail.com
patch subject: [PATCH v3] media: hantro: release runtime resources when device_run fails
config: nios2-allmodconfig (https://download.01.org/0day-ci/archive/20260730/202607300319.RuKrwvI7-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 11.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260730/202607300319.RuKrwvI7-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202607300319.RuKrwvI7-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/media/platform/verisilicon/hantro_drv.c:1309:12: warning: 'hantro_runtime_suspend' defined but not used [-Wunused-function]
1309 | static int hantro_runtime_suspend(struct device *dev)
| ^~~~~~~~~~~~~~~~~~~~~~
>> drivers/media/platform/verisilicon/hantro_drv.c:1287:12: warning: 'hantro_runtime_resume' defined but not used [-Wunused-function]
1287 | static int hantro_runtime_resume(struct device *dev)
| ^~~~~~~~~~~~~~~~~~~~~
vim +/hantro_runtime_suspend +1309 drivers/media/platform/verisilicon/hantro_drv.c
1286
> 1287 static int hantro_runtime_resume(struct device *dev)
1288 {
1289 struct hantro_dev *vpu = dev_get_drvdata(dev);
1290 int ret;
1291
1292 ret = clk_bulk_enable(vpu->variant->num_clocks, vpu->clocks);
1293 if (ret)
1294 return ret;
1295
1296 if (vpu->variant->runtime_resume) {
1297 ret = vpu->variant->runtime_resume(vpu);
1298 if (ret)
1299 goto err_disable_clocks;
1300 }
1301
1302 return 0;
1303
1304 err_disable_clocks:
1305 clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
1306 return ret;
1307 }
1308
> 1309 static int hantro_runtime_suspend(struct device *dev)
1310 {
1311 struct hantro_dev *vpu = dev_get_drvdata(dev);
1312
1313 clk_bulk_disable(vpu->variant->num_clocks, vpu->clocks);
1314 return 0;
1315 }
1316
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 26+ messages in thread