Hi Jackson,
sorry for the the delay, my reply below.
Le lundi 26 février 2024 à 01:33 +0000, jackson.lee a écrit :
> Hello Nicolas
>
[...]
>
> I have added the autosuspend feature, can you please review the below code ?
> If there is no problem at your side, I will make a patch for upstream.
>
>
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-hw.c b/drivers/media/platform/chips-media/wave5/wave5-hw.c
> index 0b6386f31..2ba9e0f0e 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-hw.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-hw.c
> @@ -1102,8 +1102,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 8b1417ece..4aea66483 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);
> }
> }
> @@ -1382,6 +1385,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);
>
> @@ -1389,7 +1393,6 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
> struct dec_open_param open_param;
>
> memset(&open_param, 0, sizeof(struct dec_open_param));
> -
I'd refrain from mixing style changes with the function changes. Just leave the
empty line, is it harmless. There is other cases in your RFC.
> ret = wave5_vpu_dec_allocate_ring_buffer(inst);
> if (ret)
> goto return_buffers;
> @@ -1425,13 +1428,14 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
> }
> }
> }
> -
There is a case were userspace may start streaming but never populate the
queues. You should call pm_runtime_put_autosuspend() here for this reason. In
most cases it won't suspend as there device_run() get called very shortly after,
before the timeout expires.
> 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_mark_last_busy(inst->dev->dev);
For fast operations or synchronous errors, I believe its fine to skip the
"mark_last_busy()" calls. This is resetting the timeout start time, but if the
elapse time is really short, I'm pretty sure there is very little values in
pushing it forward. Removing that call from error case, stream_start/stop should
make the patch a little lighter.
I think overall your RFC is in the right direction, and this is good material
for a V2.
regards,
Nicolas
> + pm_runtime_put_autosuspend(inst->dev->dev);
> return ret;
> }
>
> @@ -1517,6 +1521,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;
> @@ -1540,6 +1545,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 = {
> @@ -1626,7 +1634,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");
> @@ -1709,6 +1717,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);
> }
>
> @@ -1866,7 +1876,6 @@ static int wave5_vpu_open_dec(struct file *filp)
> }
>
> wave5_vdi_allocate_sram(inst->dev);
> -
> return 0;
>
> cleanup_inst:
> 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 f04baa93a..14fd26204 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);
> }
>
> @@ -1735,7 +1750,6 @@ static int wave5_vpu_open_enc(struct file *filp)
> }
>
> wave5_vdi_allocate_sram(inst->dev);
> -
> return 0;
>
> cleanup_inst:
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
> index 2323dba75..6b4794e0e 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 "wave5-vpu.h"
> #include "wave5-regdefine.h"
> #include "wave5-vpuconfig.h"
> @@ -117,6 +118,38 @@ static int wave5_vpu_load_firmware(struct device *dev, const char *fw_name,
> return 0;
> }
>
> +static int wave5_pm_suspend(struct device *dev)
> +{
> + struct vpu_device *vpu = dev_get_drvdata(dev);
> +
> + if (pm_runtime_suspended(dev))
> + return 0;
> +
> + wave5_vpu_sleep_wake(dev, true, NULL, 0);
> + clk_bulk_disable_unprepare(vpu->num_clks, vpu->clks);
> +
> + return 0;
> +}
> +
> +static 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;
> + }
> +
> + return ret;
> +}
> +
> +static const struct dev_pm_ops wave5_pm_ops = {
> + SET_RUNTIME_PM_OPS(wave5_pm_suspend, wave5_pm_resume, NULL)
> +};
> +
> static int wave5_vpu_probe(struct platform_device *pdev)
> {
> int ret;
> @@ -233,6 +266,12 @@ static int wave5_vpu_probe(struct platform_device *pdev)
> (match_data->flags & WAVE5_IS_DEC) ? "'DECODE'" : "");
> dev_info(&pdev->dev, "Product Code: 0x%x\n", dev->product_code);
> dev_info(&pdev->dev, "Firmware Revision: %u\n", fw_revision);
> +
> + pm_runtime_set_autosuspend_delay(&pdev->dev, 5000);
> + pm_runtime_use_autosuspend(&pdev->dev);
> + pm_runtime_enable(&pdev->dev);
> + wave5_vpu_sleep_wake(&pdev->dev, true, NULL, 0);
> +
> return 0;
>
> err_enc_unreg:
> @@ -255,6 +294,9 @@ static int wave5_vpu_remove(struct platform_device *pdev)
> {
> struct vpu_device *dev = dev_get_drvdata(&pdev->dev);
>
> + pm_runtime_put_sync(&pdev->dev);
> + pm_runtime_disable(&pdev->dev);
> +
> mutex_destroy(&dev->dev_lock);
> mutex_destroy(&dev->hw_lock);
> clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
> @@ -282,6 +324,7 @@ static struct platform_driver wave5_vpu_driver = {
> .driver = {
> .name = VPU_PLATFORM_DEVICE_NAME,
> .of_match_table = of_match_ptr(wave5_dt_ids),
> + .pm = &wave5_pm_ops,
> },
> .probe = wave5_vpu_probe,
> .remove = wave5_vpu_remove,
> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> index 86b399372..b2357d1b5 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
> @@ -6,6 +6,8 @@
> */
>
> #include <linux/bug.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/delay.h>
> #include "wave5-vpuapi.h"
> #include "wave5-regdefine.h"
> #include "wave5.h"
> @@ -200,6 +202,8 @@ int wave5_vpu_dec_close(struct vpu_instance *inst, u32 *fail_res)
> if (!inst->codec_info)
> return -EINVAL;
>
> + pm_runtime_resume_and_get(inst->dev->dev);
> +
> ret = mutex_lock_interruptible(&vpu_dev->hw_lock);
> if (ret)
> return ret;
> @@ -234,7 +238,7 @@ int wave5_vpu_dec_close(struct vpu_instance *inst, u32 *fail_res)
>
> unlock_and_return:
> mutex_unlock(&vpu_dev->hw_lock);
> -
> + pm_runtime_put_autosuspend(inst->dev->dev);
> return ret;
> }
>
> @@ -702,6 +706,8 @@ int wave5_vpu_enc_close(struct vpu_instance *inst, u32 *fail_res)
> if (!inst->codec_info)
> return -EINVAL;
>
> + pm_runtime_resume_and_get(inst->dev->dev);
> +
> ret = mutex_lock_interruptible(&vpu_dev->hw_lock);
> if (ret)
> return ret;
> @@ -733,9 +739,10 @@ int wave5_vpu_enc_close(struct vpu_instance *inst, u32 *fail_res)
> }
>
> wave5_vdi_free_dma_memory(vpu_dev, &p_enc_info->vb_task);
> -
> mutex_unlock(&vpu_dev->hw_lock);
>
> + pm_runtime_put_autosuspend(inst->dev->dev);
> +
> return 0;
> }
>
>
>
>
>
>
>
>
>
>
>
> > >
> > >
> > > Thanks.
> > > Jackson
> > >
> > > >
> > > > The delay means a timer, so there is no input for 5 secs, then
> > > > timeout callback is called, And suspend is set, if new activity
> > > > comes, the device is resumed again ?
> > > > My understanding is correct ?
> > > >
> > >
> > >
> > > >
> > > >
> > > > > Nicolas
> > > > >
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > >
> > > > > > > There is of course other places where you'll have to make sure
> > > > > > > the hardware is resumed, like on close, as you want to remove
> > > > > > > the
> > > > instance.
> > > > > > > There is also small queries here and there that need to be
> > > > > > > surrounded with resume/put, but with the redesign, most of the
> > > > > > > HW access now take place inside device_run() only.
> > > > > > >
> > > > > > > Open/Close is not invalid, but it has a lot of issues, as any
> > > > > > > random application can endup disabling the kernel ability to
> > > > > > > save
> > > > power.
> > > > > > > Personally, I think we should at least give it a try, and
> > > > > > > document valid reason not to do so if we find hardware issues.
> > > > > > > Otherwise, this sounds like all we care is ticking the box
> > > > > > > "this driver has runtime PM" without actually caring about
> > effective power saving.
> > > > > > >
> > > > > > > Nicolas
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks.
> > > > > > > >
> > > > > > > > > Le lundi 19 février 2024 à 04:04 +0000, jackson.lee a écrit :
> > > > > > > > > > Hi Nicolas
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > This seems very unnatural. We do the get() in
> > > > > > > > > > > "start_streaming()", but the
> > > > > > > > > > > put() is only done when the device is closed, or when
> > > > > > > > > > > the driver is removed. As this is not balanced, you
> > > > > > > > > > > seem to have to check the suspended condition all over
> > the place.
> > > > > > > > > > >
> > > > > > > > > > > I think we could aim for
> > > > > > > > > > > start_streaming()/stop_streaming()
> > > > > > > > > > > for your get/put placement. At least they will be
> > > > > > > > > > > bound to an entirely balanced
> > > > > > > > > API.
> > > > > > > > > > > But then, a media player in paused sate will prevent
> > > > > > > > > > > that device from being suspended.
> > > > > > > > > > >
> > > > > > > > > > > If the HW is capable of preserving enough state, and
> > > > > > > > > > > From the short doc I have it gives me the impression
> > > > > > > > > > > it can preserve that, I'd suggest to follow what
> > > > > > > > > > > hantro driver is doing. What is does is that it will
> > > > > > > > > > > do get() in device_run(), and put() whenever the job
> > > > > > > > > > > completes. This driver has been designed so when there
> > > > > > > > > > > is no job, it means the firmware is currently idle and
> > looking for more work.
> > > > > > > > > > > So it seems like the perfect moment to
> > > > > > > do suspend it.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Thanks your comment,
> > > > > > > > > >
> > > > > > > > > > Currently they are not balanced, If we puts "the put
> > functon"
> > > > > > > > > > into the stop_streaming, our hw is stalled
> > > > > > > > > until doing wake-up command, so our v4l2 device become block.
> > > > > > > > > > so I'd like to update the below instead of calling get
> > > > > > > > > > at the
> > > > > > > > > start_streaming function.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > @@ -1867,6 +1868,13 @@ static int
> > > > > > > > > > wave5_vpu_open_dec(struct file
> > > > > > > > > > *filp)
> > > > > > > > > >
> > > > > > > > > > wave5_vdi_allocate_sram(inst->dev);
> > > > > > > > > >
> > > > > > > > > > + err = pm_runtime_resume_and_get(inst->dev->dev);
> > > > > > > > > > + if (err) {
> > > > > > > > > > + dev_err(inst->dev->dev, "decoder runtime
> > > > > > > > > > + resume
> > > > > > > > > failed %d\n", err);
> > > > > > > > > > + ret = -EINVAL;
> > > > > > > > > > + goto cleanup_inst;
> > > > > > > > > > + }
> > > > > > > > > > +
> > > > > > > > > > return 0;
> > > > > > > > >
> > > > > > > > > I guess we need to discuss the power management strategy
> > > > > > > > > for this
> > > > > > > device.
> > > > > > > > > If you do resume_and_get() in open(), and then put in
> > > > > > > > > close(), that seems balanced. But in term of power saving,
> > > > > > > > > it might not be very strong. If you have a media player
> > > > > > > > > that is set to pause and then placed in the background,
> > > > > > > > > you still keep the IP running. This is extremely common,
> > > > > > > > > since application cannot close their device without
> > > > > > > > > loosing the reference frames, and thus having to do extra
> > > > > > > > > work on resume to seek back to the previous sync point and
> > > > > > > > > drop
> > > > > > > unneeded frames.
> > > > > > > > >
> > > > > > > > > It seems like the whole point of asking the firmware to
> > > > > > > > > save the state and suspend is to be able to do so while
> > > > > > > > > there is meaningful sate in the firt place.
> > > > > > > > > If we are to suspend only when there is no meaningful
> > > > > > > > > state, we could just free all resources and power it off
> > completely.
> > > > > > > > > (This is just for illustration, its probably to slow to
> > > > > > > > > boot the firmware at
> > > > > > > > > runtime)
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > I understand you suffered lockup with a start_streaming()
> > > > > > > > > for resume_and_get(), and stop_streaming() for put(), this
> > > > > > > > > may simply indicate that some hardware access are needed
> > > > > > > > > between these two. Can you write down a power management
> > > > > > > > > plan that would effectively save power in common use cases
> > > > > > > > > ? We can certainly help in refactoring the
> > > > > > > code to make that happen.
> > > > > > > > >
> > > > > > > > > Nicolas
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > Le mercredi 31 janvier 2024 à 10:30 +0900, jackson.lee
> > > > > > > > > > > a
> > > > > écrit :
> > > > > > > > > > > > There are two device run-time PM callbacks defined
> > > > > > > > > > > > in 'struct
> > > > > > > > > > > dev_pm_ops'
> > > > > > > > > > > > int (*runtime_suspend)(struct device *dev); int
> > > > > > > > > > > > (*runtime_resume)(struct device *dev);
> > > > > > > > > > >
> > > > > > > > > > > I wonder how useful is it to teach everyone what the
> > > > > > > > > > > generic 'struct dev_pm_ops'
> > > > > > > > > > > contains. Perhaps you simply wanted that this patch
> > > > > > > > > > > implement both suspend and resume ops ?
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Jackson Lee
> > > > > > > > > > > > <jackson.lee@xxxxxxxxxxxxxxx>
> > > > > > > > > > > > Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
> > > > > > > > > > > > ---
> > > > > > > > > > > > .../platform/chips-media/wave5/wave5-hw.c | 5 +-
> > > > > > > > > > > > .../chips-media/wave5/wave5-vpu-dec.c | 9 +++
> > > > > > > > > > > > .../chips-media/wave5/wave5-vpu-enc.c | 9 +++
> > > > > > > > > > > > .../platform/chips-media/wave5/wave5-vpu.c | 68
> > > > > > > > > +++++++++++++++++++
> > > > > > > > > > > > .../platform/chips-media/wave5/wave5-vpuapi.c | 7
> > > > > > > > > > > > ++ .../media/platform/chips-media/wave5/wave5.h |
> > > > > > > > > > > > 3 +
> > > > > > > > > > > > 6 files changed, 99 insertions(+), 2 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git
> > > > > > > > > > > > a/drivers/media/platform/chips-media/wave5/wave5-hw.
> > > > > > > > > > > > c
> > > > > > > > > > > > b/drivers/media/platform/chips-media/wave5/wave5-hw.
> > > > > > > > > > > > c index 8ad7f3a28ae1..8aade5a38439 100644
> > > > > > > > > > > > ---
> > > > > > > > > > > > a/drivers/media/platform/chips-media/wave5/wave5-hw.
> > > > > > > > > > > > c
> > > > > > > > > > > > +++ b/drivers/media/platform/chips-media/wave5/wave5-
> > hw.
> > > > > > > > > > > > +++ c
> > > > > > > > > > > > @@ -503,6 +503,7 @@ int
> > > > > > > > > > > > wave5_vpu_build_up_dec_param(struct
> > > > > > > > > > > > vpu_instance
> > > > > > > > > > > *inst,
> > > > > > > > > > > > /* This register must be reset explicitly */
> > > > > > > > > > > > vpu_write_reg(inst->dev, W5_CMD_EXT_ADDR, 0);
> > > > > > > > > > > > vpu_write_reg(inst->dev, W5_CMD_NUM_CQ_DEPTH_M1,
> > > > > > > > > > > > (COMMAND_QUEUE_DEPTH - 1));
> > > > > > > > > > > > + vpu_write_reg(inst->dev, W5_CMD_ERR_CONCEAL, 0);
> > > > > > > > > > >
> > > > > > > > > > > In some way, the relation between suspend and this
> > > > > > > > > > > register write is not obvious. If its not related,
> > > > > > > > > > > please do this in its
> > > > > > > own patch.
> > > > > > > > > > > Otherwise you want to explain why you needed this
> > > > > > > > > > > (possibly just in the commit message).
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > ret = send_firmware_command(inst,
> > > > > > > > > > > > W5_CREATE_INSTANCE, true, NULL,
> > > > > > > > > > > NULL);
> > > > > > > > > > > > if (ret) {
> > > > > > > > > > > > @@ -1075,8 +1076,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 ef227af72348..328a7a8f26c5 100644
> > > > > > > > > > > > ---
> > > > > > > > > > > > a/drivers/media/platform/chips-media/wave5/wave5-vpu
> > > > > > > > > > > > -dec
> > > > > > > > > > > > .c
> > > > > > > > > > > > +++ b/drivers/media/platform/chips-media/wave5/wave5
> > > > > > > > > > > > +++ -vpu
> > > > > > > > > > > > +++ -d
> > > > > > > > > > > > +++ ec.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"
> > > > > > > > > > > > @@ -1387,9 +1388,17 @@ static int
> > > > > > > > > > > > wave5_vpu_dec_start_streaming(struct
> > > > > > > > > > > > vb2_queue *q, unsigned int count
> > > > > > > > > > > >
> > > > > > > > > > > > if (q->type ==
> > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE
> > > > > > > > > > > > &&
> > > > > > > > > > > > inst-
> > > > > > > > > > state
> > > > > > > > > > > > ==
> > > > > > > > > > > VPU_INST_STATE_NONE) {
> > > > > > > > > > > > struct dec_open_param open_param;
> > > > > > > > > > > > + int err = 0;
> > > > > > > > > > > >
> > > > > > > > > > > > memset(&open_param, 0, sizeof(struct
> > > > > dec_open_param));
> > > > > > > > > > > >
> > > > > > > > > > > > + err = pm_runtime_resume_and_get(inst-
> > > dev->dev);
> > > > > > > > > > > > + if (err) {
> > > > > > > > > > > > + dev_err(inst->dev->dev, "decoder
> > runtime
> > > > > resume
> > > > > > > > > > > failed %d\n", err);
> > > > > > > > > > > > + ret = -EINVAL;
> > > > > > > > > > > > + goto return_buffers;
> > > > > > > > > > > > + }
> > > > > > > > > > > > +
> > > > > > > > > > > > ret =
> > wave5_vpu_dec_allocate_ring_buffer(inst);
> > > > > > > > > > > > if (ret)
> > > > > > > > > > > > goto return_buffers; 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 761775216cd4..ff73d69de41c 100644
> > > > > > > > > > > > ---
> > > > > > > > > > > > a/drivers/media/platform/chips-media/wave5/wave5-vpu
> > > > > > > > > > > > -enc
> > > > > > > > > > > > .c
> > > > > > > > > > > > +++ b/drivers/media/platform/chips-media/wave5/wave5
> > > > > > > > > > > > +++ -vpu
> > > > > > > > > > > > +++ -e
> > > > > > > > > > > > +++ nc.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"
> > > > > > > > > > > > @@ -1387,9 +1388,17 @@ static int
> > > > > > > > > > > > wave5_vpu_enc_start_streaming(struct
> > > > > > > > > > > > vb2_queue *q, unsigned int count
> > > > > > > > > > > >
> > > > > > > > > > > > if (inst->state == VPU_INST_STATE_NONE && q-
> > > type
> > > > > > > > > > > > ==
> > > > > > > > > > > V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> > > > > > > > > > > > struct enc_open_param open_param;
> > > > > > > > > > > > + int err = 0;
> > > > > > > > > > > >
> > > > > > > > > > > > memset(&open_param, 0, sizeof(struct
> > > > > enc_open_param));
> > > > > > > > > > > >
> > > > > > > > > > > > + err = pm_runtime_resume_and_get(inst-
> > > dev->dev);
> > > > > > > > > > > > + if (err) {
> > > > > > > > > > > > + dev_err(inst->dev->dev, "encoder
> > runtime
> > > > > resume
> > > > > > > > > > > failed %d\n", err);
> > > > > > > > > > > > + ret = -EINVAL;
> > > > > > > > > > > > + goto return_buffers;
> > > > > > > > > > > > + }
> > > > > > > > > > > > +
> > > > > > > > > > > > wave5_set_enc_openparam(&open_param,
> > inst);
> > > > > > > > > > > >
> > > > > > > > > > > > ret = wave5_vpu_enc_open(inst,
> > &open_param);
> > > > > diff --
> > > > > > > > > git
> > > > > > > > > > > > a/drivers/media/platform/chips-media/wave5/wave5-vpu
> > > > > > > > > > > > .c
> > > > > > > > > > > > b/drivers/media/platform/chips-media/wave5/wave5-vpu
> > > > > > > > > > > > .c index 0d90b5820bef..f81409740a56 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 "wave5-vpu.h"
> > > > > > > > > > > > #include "wave5-regdefine.h"
> > > > > > > > > > > > #include "wave5-vpuconfig.h"
> > > > > > > > > > > > @@ -117,6 +118,65 @@ 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;
> > > > > > > > > > > > +
> > > > > > > > > > > > + 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;
> > > > > > > > > > > > + }
> > > > > > > > > > > > +
> > > > > > > > > > > > + return ret;
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > +static __maybe_unused int wave5_suspend(struct
> > > > > > > > > > > > +device
> > > > > > > > > > > > +*dev)
> > > > > {
> > > > > > > > > > > > + struct vpu_device *vpu = dev_get_drvdata(dev);
> > > > > > > > > > > > + struct vpu_instance *inst;
> > > > > > > > > > > > +
> > > > > > > > > > > > + list_for_each_entry(inst, &vpu->instances, list)
> > > > > > > > > > > > + v4l2_m2m_suspend(inst->v4l2_m2m_dev);
> > > > > > > > > > > > +
> > > > > > > > > > > > + return pm_runtime_force_suspend(dev); }
> > > > > > > > > > > > +
> > > > > > > > > > > > +static __maybe_unused int wave5_resume(struct
> > > > > > > > > > > > +device
> > > > > > > > > > > > +*dev)
> > > > > {
> > > > > > > > > > > > + struct vpu_device *vpu = dev_get_drvdata(dev);
> > > > > > > > > > > > + struct vpu_instance *inst;
> > > > > > > > > > > > + int ret = 0;
> > > > > > > > > > > > +
> > > > > > > > > > > > + ret = pm_runtime_force_resume(dev);
> > > > > > > > > > > > + if (ret < 0)
> > > > > > > > > > > > + return ret;
> > > > > > > > > > > > +
> > > > > > > > > > > > + list_for_each_entry(inst, &vpu->instances, list)
> > > > > > > > > > > > + v4l2_m2m_resume(inst->v4l2_m2m_dev);
> > > > > > > > > > > > +
> > > > > > > > > > > > + return ret;
> > > > > > > > > > > > +}
> > > > > > > > > > >
> > > > > > > > > > > The functions wave5_suspend() and wave5_resume() are
> > > > > > > > > > > not just "maybe_unsued" but actually never used. What
> > > > > > > > > > > was the
> > > > > intention ?
> > > > > > > > > > > Considering the usage of __maybe_unused has been such
> > > > > > > > > > > a bad friend for this one, could you instead bracket
> > > > > > > > > > > the functions with an
> > > > > > > > > explicit ?
> > > > > > > > > > >
> > > > > > > > > > > #ifdef CONFIG_PM
> > > > > > > > > > > #endif
> > > > > > > > > > >
> > > > > > > > > > > This way the compiler will have a word if you
> > > > > > > > > > > implement something that you forgot to actually use.
> > > > > > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > > +static const struct dev_pm_ops wave5_pm_ops = {
> > > > > > > > > > > > + SET_RUNTIME_PM_OPS(wave5_pm_suspend,
> > > > > > > > > > > > +wave5_pm_resume,
> > > > > > > > > NULL) };
> > > > > > > > > > > > +
> > > > > > > > > > > > static int wave5_vpu_probe(struct platform_device
> > > > > > > > > > > > *pdev)
> > > > {
> > > > > > > > > > > > int ret;
> > > > > > > > > > > > @@ -232,6 +292,10 @@ static int
> > > > > > > > > > > > wave5_vpu_probe(struct platform_device
> > > > > > > > > > > *pdev)
> > > > > > > > > > > > (match_data->flags & WAVE5_IS_DEC) ?
> > > > > "'DECODE'" : "");
> > > > > > > > > > > > dev_info(&pdev->dev, "Product Code: 0x%x\n",
> > dev-
> > > > > > > > > > product_code);
> > > > > > > > > > > > dev_info(&pdev->dev, "Firmware Revision: %u\n",
> > > > > > > > > > > > fw_revision);
> > > > > > > > > > > > +
> > > > > > > > > > > > + pm_runtime_enable(&pdev->dev);
> > > > > > > > > > > > + wave5_vpu_sleep_wake(&pdev->dev, true, NULL, 0);
> > > > > > > > > > > > +
> > > > > > > > > > > > return 0;
> > > > > > > > > > > >
> > > > > > > > > > > > err_enc_unreg:
> > > > > > > > > > > > @@ -254,6 +318,9 @@ static int
> > > > > > > > > > > > wave5_vpu_remove(struct platform_device
> > > > > > > > > > > > *pdev) {
> > > > > > > > > > > > struct vpu_device *dev =
> > > > > > > > > > > > dev_get_drvdata(&pdev->dev);
> > > > > > > > > > > >
> > > > > > > > > > > > + pm_runtime_put_sync(&pdev->dev);
> > > > > > > > > > > > + pm_runtime_disable(&pdev->dev);
> > > > > > > > > > > > +
> > > > > > > > > > > > mutex_destroy(&dev->dev_lock);
> > > > > > > > > > > > mutex_destroy(&dev->hw_lock);
> > > > > > > > > > > > clk_bulk_disable_unprepare(dev->num_clks,
> > > > > > > > > > > > dev->clks);
> > > > > @@
> > > > > > > > > > > > -
> > > > > > > > > 281,6
> > > > > > > > > > > > +348,7 @@ static struct platform_driver
> > > > > > > > > > > > +wave5_vpu_driver = {
> > > > > > > > > > > > .driver = {
> > > > > > > > > > > > .name = VPU_PLATFORM_DEVICE_NAME,
> > > > > > > > > > > > .of_match_table =
> > of_match_ptr(wave5_dt_ids),
> > > > > > > > > > > > + .pm = &wave5_pm_ops,
> > > > > > > > > > > > },
> > > > > > > > > > > > .probe = wave5_vpu_probe,
> > > > > > > > > > > > .remove = wave5_vpu_remove, diff --git
> > > > > > > > > > > > a/drivers/media/platform/chips-media/wave5/wave5-
> > vpuapi.
> > > > > > > > > > > > c
> > > > > > > > > > > > b/drivers/media/platform/chips-media/wave5/wave5-
> > vpuapi.
> > > > > > > > > > > > c index 1a3efb638dde..f1f8e4fc8474 100644
> > > > > > > > > > > > ---
> > > > > > > > > > > > a/drivers/media/platform/chips-media/wave5/wave5-
> > vpuapi.
> > > > > > > > > > > > c
> > > > > > > > > > > > +++ b/drivers/media/platform/chips-media/wave5/wave5
> > > > > > > > > > > > +++ -vpu
> > > > > > > > > > > > +++ ap
> > > > > > > > > > > > +++ i.c
> > > > > > > > > > > > @@ -6,6 +6,7 @@
> > > > > > > > > > > > */
> > > > > > > > > > > >
> > > > > > > > > > > > #include <linux/bug.h>
> > > > > > > > > > > > +#include <linux/pm_runtime.h>
> > > > > > > > > > > > #include "wave5-vpuapi.h"
> > > > > > > > > > > > #include "wave5-regdefine.h"
> > > > > > > > > > > > #include "wave5.h"
> > > > > > > > > > > > @@ -232,6 +233,9 @@ int wave5_vpu_dec_close(struct
> > > > > > > > > > > > vpu_instance *inst,
> > > > > > > > > > > > u32 *fail_res)
> > > > > > > > > > > >
> > > > > > > > > > > > wave5_vdi_free_dma_memory(vpu_dev,
> > > > > > > > > > > > &p_dec_info->vb_task);
> > > > > > > > > > > >
> > > > > > > > > > > > + if (!pm_runtime_suspended(inst->dev->dev))
> > > > > > > > > > > > + pm_runtime_put_sync(inst->dev->dev);
> > > > > > > > > > > > +
> > > > > > > > > > > > unlock_and_return:
> > > > > > > > > > > > mutex_unlock(&vpu_dev->hw_lock);
> > > > > > > > > > > >
> > > > > > > > > > > > @@ -734,6 +738,9 @@ int wave5_vpu_enc_close(struct
> > > > > > > > > > > > vpu_instance *inst,
> > > > > > > > > > > > u32 *fail_res)
> > > > > > > > > > > >
> > > > > > > > > > > > wave5_vdi_free_dma_memory(vpu_dev,
> > > > > > > > > > > > &p_enc_info->vb_task);
> > > > > > > > > > > >
> > > > > > > > > > > > + if (!pm_runtime_suspended(inst->dev->dev))
> > > > > > > > > > > > + pm_runtime_put_sync(inst->dev->dev);
> > > > > > > > > > >
> > > > > > > > > > > This seems very unnatural. We do the get() in
> > > > > > > > > > > "start_streaming()", but the
> > > > > > > > > > > put() is only done when the device is closed, or when
> > > > > > > > > > > the driver is removed. As this is not balanced, you
> > > > > > > > > > > seem to have to check the suspended condition all over
> > the place.
> > > > > > > > > > >
> > > > > > > > > > > I think we could aim for
> > > > > > > > > > > start_streaming()/stop_streaming()
> > > > > > > > > > > for your get/put placement. At least they will be
> > > > > > > > > > > bound to an entirely balanced
> > > > > > > > > API.
> > > > > > > > > > > But then, a media player in paused sate will prevent
> > > > > > > > > > > that device from being suspended.
> > > > > > > > > > >
> > > > > > > > > > > If the HW is capable of preserving enough state, and
> > > > > > > > > > > From the short doc I have it gives me the impression
> > > > > > > > > > > it can preserve that, I'd suggest to follow what
> > > > > > > > > > > hantro driver is doing. What is does is that it will
> > > > > > > > > > > do get() in device_run(), and put() whenever the job
> > > > > > > > > > > completes. This driver has been designed so when there
> > > > > > > > > > > is no job, it means the firmware is currently idle and
> > looking for more work.
> > > > > > > > > > > So it seems like the perfect moment to
> > > > > > > do suspend it.
> > > > > > > > > > >
> > > > > > > > > > > Nicolas
> > > > > > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > > mutex_unlock(&vpu_dev->hw_lock);
> > > > > > > > > > > >
> > > > > > > > > > > > return 0;
> > > > > > > > > > > > diff --git
> > > > > > > > > > > > a/drivers/media/platform/chips-media/wave5/wave5.h
> > > > > > > > > > > > b/drivers/media/platform/chips-media/wave5/wave5.h
> > > > > > > > > > > > index 063028eccd3b..6125eff938a8 100644
> > > > > > > > > > > > ---
> > > > > > > > > > > > a/drivers/media/platform/chips-media/wave5/wave5.h
> > > > > > > > > > > > +++ b/drivers/media/platform/chips-media/wave5/wave5
> > > > > > > > > > > > +++ .h
> > > > > > > > > > > > @@ -56,6 +56,9 @@ int wave5_vpu_get_version(struct
> > > > > > > > > > > > vpu_device *vpu_dev, u32 *revision);
> > > > > > > > > > > >
> > > > > > > > > > > > int wave5_vpu_init(struct device *dev, u8 *fw,
> > > > > > > > > > > > size_t size);
> > > > > > > > > > > >
> > > > > > > > > > > > +int wave5_vpu_sleep_wake(struct device *dev, bool
> > > > > > > > > > > > +i_sleep_wake, const
> > > > > > > > > > > uint16_t *code,
> > > > > > > > > > > > + size_t size);
> > > > > > > > > > > > +
> > > > > > > > > > > > int wave5_vpu_reset(struct device *dev, enum
> > > > > > > > > > > > sw_reset_mode reset_mode);
> > > > > > > > > > > >
> > > > > > > > > > > > int wave5_vpu_build_up_dec_param(struct
> > > > > > > > > > > > vpu_instance *inst, struct dec_open_param *param);
> > > > > > > > > >
> > > > > > > >
> > > > > >
> > >
>
