Re: [RESEND PATCH v7 2/4] media: chips-media: wave5: Support runtime suspend/resume

From: sebastian.fricke@xxxxxxxxxxxxx
Date: Mon Sep 09 2024 - 01:50:29 EST

Next message: Tony Lindgren: "Re: ERROR: modpost: "match_devname_and_update_preferred_console" [drivers/tty/serial/serial_base.ko] undefined!"
Previous message: Joel Stanley: "Re: [PATCH] net: ftgmac100: Ensure tx descriptor updates are visible"
In reply to: jackson . lee: "RE: [RESEND PATCH v7 2/4] media: chips-media: wave5: Support runtime suspend/resume"
Next in thread: jackson . lee: "RE: [RESEND PATCH v7 2/4] media: chips-media: wave5: Support runtime suspend/resume"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hey Jackson,

On 09.09.2024 01:21, jackson.lee wrote:

Hi Nicolas

-----Original Message-----
From: Nicolas Dufresne <nicolas@xxxxxxxxxxxx>
Sent: Saturday, September 7, 2024 4:27 AM
To: jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx;
sebastian.fricke@xxxxxxxxxxxxx
Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
hverkuil@xxxxxxxxx; Nas Chung <nas.chung@xxxxxxxxxxxxxxx>; lafley.kim
<lafley.kim@xxxxxxxxxxxxxxx>; b-brnich@xxxxxx
Subject: Re: [RESEND PATCH v7 2/4] media: chips-media: wave5: Support
runtime suspend/resume

Hi Again,

Le vendredi 06 septembre 2024 à 13:08 -0400, Nicolas Dufresne a écrit :
> Hi,
>
> Le lundi 12 août 2024 à 16:08 +0900, Jackson.lee a écrit :
> > Add support for runtime suspend/resume in the encoder and decoder.
> > This is achieved by saving the VPU state and powering it off while the
VPU idle.
>
> If you don't, I'll edit to "while the VPU *is* idle".
>
> regards,
> Nicolas
>
> >
> > Signed-off-by: Jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>
> > Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx>
> > ---
> > .../platform/chips-media/wave5/wave5-helper.c | 13 -----
> > .../platform/chips-media/wave5/wave5-hw.c | 4 +-
> > .../chips-media/wave5/wave5-vpu-dec.c | 21 ++++++--
> > .../chips-media/wave5/wave5-vpu-enc.c | 20 ++++++--
> > .../platform/chips-media/wave5/wave5-vpu.c | 50 +++++++++++++++++++
> > .../platform/chips-media/wave5/wave5-vpuapi.c | 33 ++++++++++--
> > .../media/platform/chips-media/wave5/wave5.h | 3 ++
> > 7 files changed, 118 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > index d60841c54a80..a20d84dbe3aa 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
> > @@ -58,7 +58,6 @@ int wave5_vpu_release_device(struct file *filp,
> > char *name)
> > {
> > struct vpu_instance *inst = wave5_to_vpu_inst(filp->private_data);
> > - struct vpu_device *dev = inst->dev;
> > int ret = 0;
> >
> > v4l2_m2m_ctx_release(inst->v4l2_fh.m2m_ctx);
> > @@ -78,18 +77,6 @@ int wave5_vpu_release_device(struct file *filp,
> > }
> >
> > wave5_cleanup_instance(inst);
> > - if (dev->irq < 0) {
> > - ret = mutex_lock_interruptible(&dev->dev_lock);
> > - if (ret)
> > - return ret;
> > -
> > - if (list_empty(&dev->instances)) {
> > - dev_dbg(dev->dev, "Disabling the hrtimer\n");
> > - hrtimer_cancel(&dev->hrtimer);
> > - }
> > -
> > - mutex_unlock(&dev->dev_lock);
> > - }
> >
> > return ret;
> > }
> > diff --git a/drivers/media/platform/chips-media/wave5/wave5-hw.c
> > b/drivers/media/platform/chips-media/wave5/wave5-hw.c
> > index 2a98bab446d0..c8a905994109 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-hw.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-hw.c
> > @@ -1228,8 +1228,8 @@ int wave5_vpu_re_init(struct device *dev, u8 *fw,
size_t size)
> > return setup_wave5_properties(dev); }
> >
> > -static int wave5_vpu_sleep_wake(struct device *dev, bool i_sleep_wake,
const uint16_t *code,
> > - size_t size)
> > +int wave5_vpu_sleep_wake(struct device *dev, bool i_sleep_wake, const
uint16_t *code,
> > + size_t size)
> > {
> > u32 reg_val;
> > struct vpu_buf *common_vb;
> > diff --git
> > a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > index 0c5c9a8de91f..698c83e3082e 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> > @@ -5,6 +5,7 @@
> > * Copyright (C) 2021-2023 CHIPS&MEDIA INC
> > */
> >
> > +#include <linux/pm_runtime.h>
> > #include "wave5-helper.h"
> >
> > #define VPU_DEC_DEV_NAME "C&M Wave5 VPU decoder"
> > @@ -518,6 +519,8 @@ static void wave5_vpu_dec_finish_decode(struct
vpu_instance *inst)
> > if (q_status.report_queue_count == 0 &&
> > (q_status.instance_queue_count == 0 ||
dec_info.sequence_changed)) {
> > dev_dbg(inst->dev->dev, "%s: finishing job.\n", __func__);
> > + pm_runtime_mark_last_busy(inst->dev->dev);
> > + pm_runtime_put_autosuspend(inst->dev->dev);
> > v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx);
> > }
> > }
> > @@ -1398,6 +1401,7 @@ static int wave5_vpu_dec_start_streaming(struct
vb2_queue *q, unsigned int count
> > int ret = 0;
> >
> > dev_dbg(inst->dev->dev, "%s: type: %u\n", __func__, q->type);
> > + pm_runtime_resume_and_get(inst->dev->dev);
> >
> > v4l2_m2m_update_start_streaming_state(m2m_ctx, q);
> >
> > @@ -1429,13 +1433,15 @@ static int
wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
> > if (ret)
> > goto return_buffers;
> > }
> > -
> > + pm_runtime_mark_last_busy(inst->dev->dev);
> > + pm_runtime_put_autosuspend(inst->dev->dev);
> > return ret;
> >
> > free_bitstream_vbuf:
> > wave5_vdi_free_dma_memory(inst->dev, &inst->bitstream_vbuf);
> > return_buffers:
> > wave5_return_bufs(q, VB2_BUF_STATE_QUEUED);
> > + pm_runtime_put_autosuspend(inst->dev->dev);
> > return ret;
> > }
> >
> > @@ -1521,6 +1527,7 @@ static void wave5_vpu_dec_stop_streaming(struct
vb2_queue *q)
> > bool check_cmd = TRUE;
> >
> > dev_dbg(inst->dev->dev, "%s: type: %u\n", __func__, q->type);
> > + pm_runtime_resume_and_get(inst->dev->dev);
> >
> > while (check_cmd) {
> > struct queue_status_info q_status; @@ -1544,6 +1551,9 @@
static
> > void wave5_vpu_dec_stop_streaming(struct vb2_queue *q)
> > streamoff_output(q);
> > else
> > streamoff_capture(q);
> > +
> > + pm_runtime_mark_last_busy(inst->dev->dev);
> > + pm_runtime_put_autosuspend(inst->dev->dev);
> > }
> >
> > static const struct vb2_ops wave5_vpu_dec_vb2_ops = { @@ -1630,7
> > +1640,7 @@ static void wave5_vpu_dec_device_run(void *priv)
> > int ret = 0;
> >
> > dev_dbg(inst->dev->dev, "%s: Fill the ring buffer with new
> > bitstream data", __func__);
> > -
> > + pm_runtime_resume_and_get(inst->dev->dev);
> > ret = fill_ringbuffer(inst);
> > if (ret) {
> > dev_warn(inst->dev->dev, "Filling ring buffer failed\n"); @@
> > -1713,6 +1723,8 @@ static void wave5_vpu_dec_device_run(void *priv)
> >
> > finish_job_and_return:
> > dev_dbg(inst->dev->dev, "%s: leave and finish job", __func__);
> > + pm_runtime_mark_last_busy(inst->dev->dev);
> > + pm_runtime_put_autosuspend(inst->dev->dev);
> > v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx); }
> >
> > @@ -1879,9 +1891,8 @@ static int wave5_vpu_open_dec(struct file *filp)
> > if (ret)
> > goto cleanup_inst;
> >
> > - if (dev->irq < 0 && !hrtimer_active(&dev->hrtimer) &&
list_empty(&dev->instances))
> > - hrtimer_start(&dev->hrtimer, ns_to_ktime(dev-
>vpu_poll_interval * NSEC_PER_MSEC),
> > - HRTIMER_MODE_REL_PINNED);
> > + if (list_empty(&dev->instances))
> > + pm_runtime_use_autosuspend(inst->dev->dev);
> >
> > list_add_tail(&inst->list, &dev->instances);
> >
> > diff --git
> > a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > index b731decbfda6..985de0c293e2 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> > @@ -5,6 +5,7 @@
> > * Copyright (C) 2021-2023 CHIPS&MEDIA INC
> > */
> >
> > +#include <linux/pm_runtime.h>
> > #include "wave5-helper.h"
> >
> > #define VPU_ENC_DEV_NAME "C&M Wave5 VPU encoder"
> > @@ -1310,6 +1311,7 @@ static int wave5_vpu_enc_start_streaming(struct
vb2_queue *q, unsigned int count
> > struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
> > int ret = 0;
> >
> > + pm_runtime_resume_and_get(inst->dev->dev);
> > v4l2_m2m_update_start_streaming_state(m2m_ctx, q);
> >
> > if (inst->state == VPU_INST_STATE_NONE && q->type ==
> > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) { @@ -1364,9 +1366,13 @@ static int
wave5_vpu_enc_start_streaming(struct vb2_queue *q, unsigned int count
> > if (ret)
> > goto return_buffers;
> >
> > + pm_runtime_mark_last_busy(inst->dev->dev);
> > + pm_runtime_put_autosuspend(inst->dev->dev);
> > return 0;
> > return_buffers:
> > wave5_return_bufs(q, VB2_BUF_STATE_QUEUED);
> > + pm_runtime_mark_last_busy(inst->dev->dev);
> > + pm_runtime_put_autosuspend(inst->dev->dev);
> > return ret;
> > }
> >
> > @@ -1408,6 +1414,7 @@ static void wave5_vpu_enc_stop_streaming(struct
vb2_queue *q)
> > */
> >
> > dev_dbg(inst->dev->dev, "%s: type: %u\n", __func__, q->type);
> > + pm_runtime_resume_and_get(inst->dev->dev);
> >
> > if (wave5_vpu_both_queues_are_streaming(inst))
> > switch_state(inst, VPU_INST_STATE_STOP); @@ -1432,6 +1439,9
@@
> > static void wave5_vpu_enc_stop_streaming(struct vb2_queue *q)
> > streamoff_output(inst, q);
> > else
> > streamoff_capture(inst, q);
> > +
> > + pm_runtime_mark_last_busy(inst->dev->dev);
> > + pm_runtime_put_autosuspend(inst->dev->dev);
> > }
> >
> > static const struct vb2_ops wave5_vpu_enc_vb2_ops = { @@ -1478,6
> > +1488,7 @@ static void wave5_vpu_enc_device_run(void *priv)
> > u32 fail_res = 0;
> > int ret = 0;
> >
> > + pm_runtime_resume_and_get(inst->dev->dev);
> > switch (inst->state) {
> > case VPU_INST_STATE_PIC_RUN:
> > ret = start_encode(inst, &fail_res); @@ -1491,6 +1502,8 @@
static
> > void wave5_vpu_enc_device_run(void *priv)
> > break;
> > }
> > dev_dbg(inst->dev->dev, "%s: leave with active job",
__func__);
> > + pm_runtime_mark_last_busy(inst->dev->dev);
> > + pm_runtime_put_autosuspend(inst->dev->dev);
> > return;
> > default:
> > WARN(1, "Execution of a job in state %s is invalid.\n", @@
> > -1498,6 +1511,8 @@ static void wave5_vpu_enc_device_run(void *priv)
> > break;
> > }
> > dev_dbg(inst->dev->dev, "%s: leave and finish job", __func__);
> > + pm_runtime_mark_last_busy(inst->dev->dev);
> > + pm_runtime_put_autosuspend(inst->dev->dev);
> > v4l2_m2m_job_finish(inst->v4l2_m2m_dev, m2m_ctx); }
> >
> > @@ -1739,9 +1754,8 @@ static int wave5_vpu_open_enc(struct file *filp)
> > if (ret)
> > goto cleanup_inst;
> >
> > - if (dev->irq < 0 && !hrtimer_active(&dev->hrtimer) &&
list_empty(&dev->instances))
> > - hrtimer_start(&dev->hrtimer, ns_to_ktime(dev-
>vpu_poll_interval * NSEC_PER_MSEC),
> > - HRTIMER_MODE_REL_PINNED);
> > + if (list_empty(&dev->instances))
> > + pm_runtime_use_autosuspend(inst->dev->dev);
> >
> > list_add_tail(&inst->list, &dev->instances);
> >
> > diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > index 7273254ecb03..41c4bf64f27d 100644
> > --- a/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> > @@ -10,6 +10,7 @@
> > #include <linux/clk.h>
> > #include <linux/firmware.h>
> > #include <linux/interrupt.h>
> > +#include <linux/pm_runtime.h>
> > #include <linux/reset.h>
> > #include "wave5-vpu.h"
> > #include "wave5-regdefine.h"
> > @@ -153,6 +154,45 @@ static int wave5_vpu_load_firmware(struct device
*dev, const char *fw_name,
> > return 0;
> > }
> >
> > +static __maybe_unused int wave5_pm_suspend(struct device *dev) {
> > + struct vpu_device *vpu = dev_get_drvdata(dev);
> > +
> > + if (pm_runtime_suspended(dev))
> > + return 0;
> > +
> > + if (vpu->irq < 0)
> > + hrtimer_cancel(&vpu->hrtimer);
> > +
> > + wave5_vpu_sleep_wake(dev, true, NULL, 0);
> > + clk_bulk_disable_unprepare(vpu->num_clks, vpu->clks);
> > +
> > + return 0;
> > +}
> > +
> > +static __maybe_unused int wave5_pm_resume(struct device *dev) {
> > + struct vpu_device *vpu = dev_get_drvdata(dev);
> > + int ret = 0;
> > +
> > + wave5_vpu_sleep_wake(dev, false, NULL, 0);
> > + ret = clk_bulk_prepare_enable(vpu->num_clks, vpu->clks);
> > + if (ret) {
> > + dev_err(dev, "Enabling clocks, fail: %d\n", ret);
> > + return ret;
> > + }
> > +
> > + if (vpu->irq < 0 && !hrtimer_active(&vpu->hrtimer))
> > + hrtimer_start(&vpu->hrtimer, ns_to_ktime(vpu-
>vpu_poll_interval * NSEC_PER_MSEC),
> > + HRTIMER_MODE_REL_PINNED);

I have fixed locally this style error. Alignment should match open
parenthesis.
Checkpatch caught that, it saves times if you run checkpatch before your
submissions.

https://gitlab.freedesktop.org/ndufresne/media-staging/-/jobs/63283654

Sorry for that, thanks for your feedback.
Should I make v8 for that ?

That is not required in this case, when he says "I have fixed locally",
that means that the required change is applied by the maintainer before
merging the changes upstream.

Regards,
Sebastian

Next message: Tony Lindgren: "Re: ERROR: modpost: "match_devname_and_update_preferred_console" [drivers/tty/serial/serial_base.ko] undefined!"
Previous message: Joel Stanley: "Re: [PATCH] net: ftgmac100: Ensure tx descriptor updates are visible"
In reply to: jackson . lee: "RE: [RESEND PATCH v7 2/4] media: chips-media: wave5: Support runtime suspend/resume"
Next in thread: jackson . lee: "RE: [RESEND PATCH v7 2/4] media: chips-media: wave5: Support runtime suspend/resume"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]