Message-ID: <efe24b949a60034bf618eb3b8a8ba82e8a5dc99c.camel@ndufresne.ca>
Date: Wed, 07 Feb 2024 13:29:31 -0500
From: Nicolas Dufresne <nicolas@...fresne.ca>
To: "jackson.lee" <jackson.lee@...psnmedia.com>, mchehab@...nel.org, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, 
	nas.chung@...psnmedia.com
Cc: lafley.kim@...psnmedia.com, b-brnich@...com
Subject: Re: [RESEND PATCH v0 3/5] wave5 : Support runtime suspend/resume.

Hi Jackson,

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@...psnmedia.com>
> Signed-off-by: Nas Chung <nas.chung@...psnmedia.com>
> ---
>  .../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-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"
> @@ -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-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"
> @@ -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-vpuapi.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);

Powered by blists - more mailing lists