[RESEND PATCH v6 0/4] Add features to an existing driver

[RESEND PATCH v6 0/4] Add features to an existing driver

[Jackson.lee]

The wave5 codec driver is a stateful encoder/decoder.
The following patches is for supporting yuv422 inpuy format, supporting runtime suspend/resume feature and extra things.

v4l2-compliance results:
========================

v4l2-compliance 1.24.1, 64 bits, 64-bit time_t

Buffer ioctls:
       warn: v4l2-test-buffers.cpp(693): VIDIOC_CREATE_BUFS not supported
       warn: v4l2-test-buffers.cpp(693): VIDIOC_CREATE_BUFS not supported
    test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
    test VIDIOC_EXPBUF: OK
    test Requests: OK (Not Supported)

Total for wave5-dec device /dev/video0: 45, Succeeded: 45, Failed: 0, Warnings: 2 Total for wave5-enc device /dev/video1: 45, Succeeded: 45, Failed: 0, Warnings: 0

Fluster test results:
=====================

Running test suite JCT-VC-HEVC_V1 with decoder GStreamer-H.265-V4L2-Gst1.0 Using 1 parallel job(s)
Ran 132/147 tests successfully               in 88.745 secs

(1 test fails because of not supporting to parse multi frames, 1 test fails because of a missing frame and slight corruption,
 2 tests fail because of sizes which are incompatible with the IP, 11 tests fail because of unsupported 10 bit format)

Running test suite JVT-AVC_V1 with decoder GStreamer-H.264-V4L2-Gst1.0 Using 1 parallel job(s)
Ran 77/135 tests successfully               in 32.044 secs

(58 fail because the hardware is unable to decode  MBAFF / FMO / Field / Extended profile streams.)

Change since v5:
================
* For [PATCH v4 3/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage.
 - Fix v4l2-compliance error for the vidioc_enum_framesizes

* For [PATCH v4 1/4] media: chips-media: wave5: Support SPS/PPS generation for each IDR
 - Remove warning messages for the checkpatch.pl script

Change since v4:
================
* For [PATCH v4 2/4] media: chips-media: wave5: Support runtime suspend/resume
 - Fix warning message

* For [PATCH v4 3/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage.
 - Fix warning message
 - add Reviewed-By tag

* For [PATCH v4 4/4] media: chips-media: wave5: Support YUV422 raw pixel-formats on the encoder
 - add Reviewed-By tag

Change since v3:
=================

* For [PATCH v4 1/4] media: chips-media: wave5: Support SPS/PPS generation for each IDR
 - add Reviewed-By tag

* For [PATCH v4 2/4] media: chips-media: wave5: Support runtime suspend/resume
 - add Reviewed-By tag

* For [PATCH v4 3/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage.
 - modify the commit message
 - define three framesize structures for decoder

* For [PATCH v4 4/4] media: chips-media: wave5: Support YUV422 raw pixel-formats on the encoder
 - modify the commit message
 - use the v4l2_format_info to calculate luma, chroma size

Change since v2:
=================

* For [PATCH v3 0/4] media: chips-media: wave5: Support SPS/PPS generation for each IDR
 - add the suggested _SHIFT suffix

* For [PATCH v3 1/4] media: chips-media: wave5: Support runtime suspend/resume
 - change a commit message

* For [PATCH v3 2/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage
 - add pix_fmt_type parameter into wave5_update_pix_fmt function
 - add min/max width/height values into dec_fmt_list 

Change since v1:
=================

* For [PATCH v2 0/4] media: chips-media: wave5: Support SPS/PPS generation for each IDR
 - define a macro for register addresses

* For [PATCH v2 1/4] media: chips-media: wave5: Support runtime suspend/resume
 - add auto suspend/resume

* For [PATCH v2 2/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage
 - use helper functions to calculate bytesperline and sizeimage

* For [PATCH v2 3/4] media: chips-media: wave5: Support YUV422 raw pixel-formats on the encoder
 - remove unnecessary codes

Change since v0:
=================
The DEFAULT_SRC_SIZE macro was defined using multiple lines, To make a simple define, tab and multiple lines has been removed, The macro is defined using one line.


jackson.lee (4):
  media: chips-media: wave5: Support SPS/PPS generation for each IDR
  media: chips-media: wave5: Support runtime suspend/resume
  media: chips-media: wave5: Use helpers to calculate bytesperline and
    sizeimage.
  media: chips-media: wave5: Support YUV422 raw pixel-formats on the
    encoder.

 .../platform/chips-media/wave5/wave5-helper.c |  24 ++
 .../platform/chips-media/wave5/wave5-helper.h |   5 +
 .../platform/chips-media/wave5/wave5-hw.c     |  30 +-
 .../chips-media/wave5/wave5-vpu-dec.c         | 316 +++++++-----------
 .../chips-media/wave5/wave5-vpu-enc.c         | 308 +++++++++--------
 .../platform/chips-media/wave5/wave5-vpu.c    |  43 +++
 .../platform/chips-media/wave5/wave5-vpu.h    |   5 +-
 .../platform/chips-media/wave5/wave5-vpuapi.c |  14 +-
 .../platform/chips-media/wave5/wave5-vpuapi.h |   1 +
 .../chips-media/wave5/wave5-vpuconfig.h       |  27 +-
 .../media/platform/chips-media/wave5/wave5.h  |   3 +
 11 files changed, 430 insertions(+), 346 deletions(-)

-- 
2.43.0

[RESEND PATCH v6 1/4] media: chips-media: wave5: Support SPS/PPS generation for each IDR

[Jackson.lee]

From: "jackson.lee" <jackson.lee@chipsnmedia.com>

Provide a control to toggle (0 = off / 1 = on), whether the SPS and
PPS are generated for every IDR.

Signed-off-by: Jackson.lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 .../platform/chips-media/wave5/wave5-hw.c     | 26 +++++++++++++++----
 .../chips-media/wave5/wave5-vpu-enc.c         |  7 +++++
 .../platform/chips-media/wave5/wave5-vpuapi.h |  1 +
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-hw.c b/drivers/media/platform/chips-media/wave5/wave5-hw.c
index 2d82791f575e..6ef5bd5fb325 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-hw.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-hw.c
@@ -23,6 +23,15 @@
 #define FEATURE_AVC_ENCODER		BIT(1)
 #define FEATURE_HEVC_ENCODER		BIT(0)
 
+#define ENC_AVC_INTRA_IDR_PARAM_MASK	0x7ff
+#define ENC_AVC_INTRA_PERIOD_SHIFT		6
+#define ENC_AVC_IDR_PERIOD_SHIFT		17
+#define ENC_AVC_FORCED_IDR_HEADER_SHIFT		28
+
+#define ENC_HEVC_INTRA_QP_SHIFT			3
+#define ENC_HEVC_FORCED_IDR_HEADER_SHIFT	9
+#define ENC_HEVC_INTRA_PERIOD_SHIFT		16
+
 /* Decoder support fields */
 #define FEATURE_AVC_DECODER		BIT(3)
 #define FEATURE_HEVC_DECODER		BIT(2)
@@ -33,7 +42,7 @@
 
 #define REMAP_CTRL_MAX_SIZE_BITS	((W5_REMAP_MAX_SIZE >> 12) & 0x1ff)
 #define REMAP_CTRL_REGISTER_VALUE(index)	(			\
-	(BIT(31) | (index << 12) | BIT(11) | REMAP_CTRL_MAX_SIZE_BITS)	\
+	(BIT(31) | ((index) << 12) | BIT(11) | REMAP_CTRL_MAX_SIZE_BITS)\
 )
 
 #define FASTIO_ADDRESS_MASK		GENMASK(15, 0)
@@ -1601,12 +1610,19 @@ int wave5_vpu_enc_init_seq(struct vpu_instance *inst)
 
 	if (inst->std == W_AVC_ENC)
 		vpu_write_reg(inst->dev, W5_CMD_ENC_SEQ_INTRA_PARAM, p_param->intra_qp |
-				((p_param->intra_period & 0x7ff) << 6) |
-				((p_param->avc_idr_period & 0x7ff) << 17));
+			      ((p_param->intra_period & ENC_AVC_INTRA_IDR_PARAM_MASK)
+				<< ENC_AVC_INTRA_PERIOD_SHIFT) |
+			      ((p_param->avc_idr_period & ENC_AVC_INTRA_IDR_PARAM_MASK)
+				<< ENC_AVC_IDR_PERIOD_SHIFT) |
+			      (p_param->forced_idr_header_enable
+			       << ENC_AVC_FORCED_IDR_HEADER_SHIFT));
 	else if (inst->std == W_HEVC_ENC)
 		vpu_write_reg(inst->dev, W5_CMD_ENC_SEQ_INTRA_PARAM,
-			      p_param->decoding_refresh_type | (p_param->intra_qp << 3) |
-				(p_param->intra_period << 16));
+			      p_param->decoding_refresh_type |
+			      (p_param->intra_qp << ENC_HEVC_INTRA_QP_SHIFT) |
+			      (p_param->forced_idr_header_enable
+			       << ENC_HEVC_FORCED_IDR_HEADER_SHIFT) |
+			      (p_param->intra_period << ENC_HEVC_INTRA_PERIOD_SHIFT));
 
 	reg_val = (p_param->rdo_skip << 2) |
 		(p_param->lambda_scaling_enable << 3) |
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 a45a2f699000..a23908011a39 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
@@ -1061,6 +1061,9 @@ static int wave5_vpu_enc_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
 		inst->enc_param.entropy_coding_mode = ctrl->val;
 		break;
+	case V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR:
+		inst->enc_param.forced_idr_header_enable = ctrl->val;
+		break;
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
 		break;
 	default:
@@ -1219,6 +1222,7 @@ static void wave5_set_enc_openparam(struct enc_open_param *open_param,
 		else
 			open_param->wave_param.intra_refresh_arg = num_ctu_row;
 	}
+	open_param->wave_param.forced_idr_header_enable = input.forced_idr_header_enable;
 }
 
 static int initialize_sequence(struct vpu_instance *inst)
@@ -1701,6 +1705,9 @@ static int wave5_vpu_open_enc(struct file *filp)
 			  0, 1, 1, 0);
 	v4l2_ctrl_new_std(v4l2_ctrl_hdl, &wave5_vpu_enc_ctrl_ops,
 			  V4L2_CID_MIN_BUFFERS_FOR_OUTPUT, 1, 32, 1, 1);
+	v4l2_ctrl_new_std(v4l2_ctrl_hdl, &wave5_vpu_enc_ctrl_ops,
+			  V4L2_CID_MPEG_VIDEO_PREPEND_SPSPPS_TO_IDR,
+			  0, 1, 1, 0);
 
 	if (v4l2_ctrl_hdl->error) {
 		ret = -ENODEV;
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
index edc50450ddb8..554c40b2e002 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
@@ -566,6 +566,7 @@ struct enc_wave_param {
 	u32 lambda_scaling_enable: 1; /* enable lambda scaling using custom GOP */
 	u32 transform8x8_enable: 1; /* enable 8x8 intra prediction and 8x8 transform */
 	u32 mb_level_rc_enable: 1; /* enable MB-level rate control */
+	u32 forced_idr_header_enable: 1; /* enable header encoding before IDR frame */
 };
 
 struct enc_open_param {
-- 
2.43.0

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

[Jackson.lee]

From: "jackson.lee" <jackson.lee@chipsnmedia.com>

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.

Signed-off-by: Jackson.lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 .../platform/chips-media/wave5/wave5-hw.c     |  4 +-
 .../chips-media/wave5/wave5-vpu-dec.c         | 16 ++++++-
 .../chips-media/wave5/wave5-vpu-enc.c         | 15 +++++++
 .../platform/chips-media/wave5/wave5-vpu.c    | 43 +++++++++++++++++++
 .../platform/chips-media/wave5/wave5-vpuapi.c | 14 ++++--
 .../media/platform/chips-media/wave5/wave5.h  |  3 ++
 6 files changed, 88 insertions(+), 7 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-hw.c b/drivers/media/platform/chips-media/wave5/wave5-hw.c
index 6ef5bd5fb325..dcdb1eab0174 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-hw.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-hw.c
@@ -1084,8 +1084,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 c8624c681fa6..861a0664047c 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);
 
@@ -1425,13 +1429,15 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
 			}
 		}
 	}
-
+	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;
 }
 
@@ -1517,6 +1523,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 +1547,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 +1636,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 +1719,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);
 }
 
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 a23908011a39..703fd8d1c7da 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);
 }
 
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.c b/drivers/media/platform/chips-media/wave5/wave5-vpu.c
index 68a519ac412d..0e7c1c255563 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"
@@ -145,6 +146,38 @@ 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 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;
@@ -268,6 +301,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:
@@ -295,6 +334,9 @@ static void wave5_vpu_remove(struct platform_device *pdev)
 		hrtimer_cancel(&dev->hrtimer);
 	}
 
+	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);
@@ -320,6 +362,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_new = 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..b0911fef232f 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,9 +202,13 @@ 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)
+	if (ret) {
+		pm_runtime_put_sync(inst->dev->dev);
 		return ret;
+	}
 
 	do {
 		ret = wave5_vpu_dec_finish_seq(inst, fail_res);
@@ -234,7 +240,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_sync(inst->dev->dev);
 	return ret;
 }
 
@@ -702,6 +708,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 +741,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);
-
 	mutex_unlock(&vpu_dev->hw_lock);
 
+	pm_runtime_put_sync(inst->dev->dev);
 	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);
-- 
2.43.0

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

[Devarsh Thakkar]

Hi Jackson,

Thanks for the patch.
On 17/06/24 16:18, Jackson.lee wrote:
> From: "jackson.lee" <jackson.lee@chipsnmedia.com>
> 
> 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.
> 
> Signed-off-by: Jackson.lee <jackson.lee@chipsnmedia.com>
> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

[..]
>  static int wave5_vpu_probe(struct platform_device *pdev)
>  {
>  	int ret;
> @@ -268,6 +301,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);

Why are we putting 5s delay for autosuspend ? Without using auto-suspend delay
too, we can directly go to suspended state when last instance is closed and
resume back when first instance is open.

I don't think having an autosuspend delay (especially of 5s) bodes well with
low power-centric devices such as AM62A where we would prefer to go to suspend
state as soon as possible when the last instance is closed.

Also apologies for the delay in review, this didn't caught my eye earlier as
commit message did not mention it either.

Regards
Devarsh

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

[jackson.lee]

Hi Devarsh

If there is no feeding bitstreams during encoding and decoding frames, then driver's status is switched to suspended automatically by autosuspend.
And if we don’t use autosuspend, it is very difficult for us to catch if there is feeding or not while working a pipeline.
So it is very efficient for managing power status.

If the delay is very great value, we can adjust it.

Thanks
Jackson

> -----Original Message-----
> From: Devarsh Thakkar <devarsht@ti.com>
> Sent: Wednesday, June 19, 2024 10:00 PM
> To: jackson.lee <jackson.lee@chipsnmedia.com>; mchehab@kernel.org;
> nicolas@ndufresne.ca; sebastian.fricke@collabora.com
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> hverkuil@xs4all.nl; Nas Chung <nas.chung@chipsnmedia.com>; lafley.kim
> <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; Nicolas Dufresne
> <nicolas.dufresne@collabora.com>
> Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
> suspend/resume
> 
> Hi Jackson,
> 
> Thanks for the patch.
> On 17/06/24 16:18, Jackson.lee wrote:
> > From: "jackson.lee" <jackson.lee@chipsnmedia.com>
> >
> > 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.
> >
> > Signed-off-by: Jackson.lee <jackson.lee@chipsnmedia.com>
> > Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> 
> [..]
> >  static int wave5_vpu_probe(struct platform_device *pdev)  {
> >  	int ret;
> > @@ -268,6 +301,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);
> 
> Why are we putting 5s delay for autosuspend ? Without using auto-suspend
> delay too, we can directly go to suspended state when last instance is closed
> and resume back when first instance is open.
> 
> I don't think having an autosuspend delay (especially of 5s) bodes well with
> low power-centric devices such as AM62A where we would prefer to go to
> suspend state as soon as possible when the last instance is closed.
> 
> Also apologies for the delay in review, this didn't caught my eye earlier as
> commit message did not mention it either.
> 
> Regards
> Devarsh

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

[jackson.lee]

> -----Original Message-----
> From: jackson.lee
> Sent: Thursday, June 20, 2024 8:57 AM
> To: Devarsh Thakkar <devarsht@ti.com>; mchehab@kernel.org;
> nicolas@ndufresne.ca; sebastian.fricke@collabora.com
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> hverkuil@xs4all.nl; Nas Chung <nas.chung@chipsnmedia.com>; lafley.kim
> <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; Nicolas Dufresne
> <nicolas.dufresne@collabora.com>
> Subject: RE: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
> suspend/resume
> 
> Hi Devarsh
> 
> If there is no feeding bitstreams during encoding and decoding frames, then
> driver's status is switched to suspended automatically by autosuspend.
> And if we don’t use autosuspend, it is very difficult for us to catch if
> there is feeding or not while working a pipeline.
> So it is very efficient for managing power status.
> 
> If the delay is very great value, we can adjust it.
> 
> Thanks
> Jackson
> 

One more thing, 

When an instance is closed or started, we are currently putting a power status to suspend or resumed immediately.
The autospend feature is being only used when there is no feeding while working a pipeline.
I don’t think the delay is very great value.

Thanks


> > -----Original Message-----
> > From: Devarsh Thakkar <devarsht@ti.com>
> > Sent: Wednesday, June 19, 2024 10:00 PM
> > To: jackson.lee <jackson.lee@chipsnmedia.com>; mchehab@kernel.org;
> > nicolas@ndufresne.ca; sebastian.fricke@collabora.com
> > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > hverkuil@xs4all.nl; Nas Chung <nas.chung@chipsnmedia.com>; lafley.kim
> > <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; Nicolas Dufresne
> > <nicolas.dufresne@collabora.com>
> > Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support
> > runtime suspend/resume
> >
> > Hi Jackson,
> >
> > Thanks for the patch.
> > On 17/06/24 16:18, Jackson.lee wrote:
> > > From: "jackson.lee" <jackson.lee@chipsnmedia.com>
> > >
> > > 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.
> > >
> > > Signed-off-by: Jackson.lee <jackson.lee@chipsnmedia.com>
> > > Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> >
> > [..]
> > >  static int wave5_vpu_probe(struct platform_device *pdev)  {
> > >  	int ret;
> > > @@ -268,6 +301,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);
> >
> > Why are we putting 5s delay for autosuspend ? Without using
> > auto-suspend delay too, we can directly go to suspended state when
> > last instance is closed and resume back when first instance is open.
> >
> > I don't think having an autosuspend delay (especially of 5s) bodes
> > well with low power-centric devices such as AM62A where we would
> > prefer to go to suspend state as soon as possible when the last instance is
> closed.
> >
> > Also apologies for the delay in review, this didn't caught my eye
> > earlier as commit message did not mention it either.
> >
> > Regards
> > Devarsh

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

[Devarsh Thakkar]

Hi Jackson,

On 20/06/24 05:41, jackson.lee wrote:
> 
> 
>> -----Original Message-----
>> From: jackson.lee
>> Sent: Thursday, June 20, 2024 8:57 AM
>> To: Devarsh Thakkar <devarsht@ti.com>; mchehab@kernel.org;
>> nicolas@ndufresne.ca; sebastian.fricke@collabora.com
>> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
>> hverkuil@xs4all.nl; Nas Chung <nas.chung@chipsnmedia.com>; lafley.kim
>> <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; Nicolas Dufresne
>> <nicolas.dufresne@collabora.com>
>> Subject: RE: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
>> suspend/resume
>>
>> Hi Devarsh
>>
>> If there is no feeding bitstreams during encoding and decoding frames, then
>> driver's status is switched to suspended automatically by autosuspend.

I think the pm_runtime_*_autosuspend helpers are to schedule a delayed suspend
i.e. after the pm counter goes to 0, suspend the device after timeout period
which is set to 5s in this case.

Even without using the pm_runtime_*_autosuspend helpers, i.e if you use
pm_runtime_resume_and_get on start streaming and pm_runtime_put_sync on stop
streaming the device gets suspended automatically if not in use albeit
immediately after the pm counter goes to 0. And this is what many codec
devices drivers do today [1]. Ain't that suffice what we want ?

In my view the delayed suspend functionality is generally helpful for devices
where resume latencies are higher for e.g. this light sensor driver [2] uses
it because it takes 250ms to stabilize after resumption and I don't see this
being used in codec drivers generally since there is no such large resume
latency. Please let me know if I am missing something or there is a strong
reason to have delayed suspend for wave5.

>> And if we don’t use autosuspend, it is very difficult for us to catch if
>> there is feeding or not while working a pipeline.
>> So it is very efficient for managing power status.

As mentioned above, if you mean by autosuspend that device should
automatically suspend if not used then you don't require to use
pm_runtime_*_autosuspend helpers (as those are for delayed suspend actually)
and instead use the generic pm helpers pm_runtime_resume_and_get and
pm_runtime_put_sync and PM core will automatically suspend the device when pm
counter drops to 0 and resume it back when pm counter is incremented.

>>
>> If the delay is very great value, we can adjust it.
>>

As mentioned above, I feel we don't require to use pm_runtime_*_autosuspend
helpers at first place.

>> Thanks
>> Jackson
>>
> 
> One more thing, 
> 
> When an instance is closed or started, we are currently putting a power status to suspend or resumed immediately.
So I tested this series and see below issues :

1) I see it seems to break VPU operation on AM62A using upstream linux-next
colliding with the polling functionality there since that device does not have
an irq and relies on polling as I see below logs on bootup :

[2024-06-20 13:01:12] root@am62axx-evm:~# dmesg | tail
[2024-06-20 13:01:16] [   23.744372] x8 : ffff000804248a50 x7 :
ffff00087f6ba0c0 x6 : 0000000000000000
[2024-06-20 13:01:16] [   23.744381] x5 : 0000000000f42400 x4 :
0000000000000000 x3 : 0000000000000001
[2024-06-20 13:01:16] [   23.744390] x2 : ffff0008041ad808 x1 :
0000000000000044 x0 : ffff800082150044
[2024-06-20 13:01:16] [   23.744400] Call trace:
[2024-06-20 13:01:16] [   23.744404]  wave5_vdi_read_register+0x8/0x20 [wave5]
[2024-06-20 13:01:16] [   23.744420]  kthread_worker_fn+0xcc/0x184
[2024-06-20 13:01:16] [   23.744432]  kthread+0x118/0x11c
[2024-06-20 13:01:16] [   23.744440]  ret_from_fork+0x10/0x20
[2024-06-20 13:01:16] [   23.744452] Code: b9000022 d65f03c0 f940b000 8b214000
(b9400000)
[2024-06-20 13:01:16] [   23.744458] ---[ end trace 0000000000000000 ]---

I think care needs to be taken to make sure timer is started after device is
powered on and stopped before device gets powered off.


> The autospend feature is being only used when there is no feeding while working a pipeline.

2) I think above doesn't seem to work, Brandon had a hack patch on vendor tree
[3] for AM62A timer, with that I no longer see above crash issue but I observe
that there is a 5 second wait to power off device even after last instance is
closed as seen here [4], seems like power counter is not getting set to 0 on
instance close, you may try to reproduce the same on j721s2 evm too.


[1]:
https://gitlab.com/linux-kernel/linux-next/-/blob/master/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c?ref_type=heads#L1637
[2]:
https://gitlab.com/linux-kernel/linux-next/-/blob/next-20240619/drivers/iio/light/bh1780.c?ref_type=tags#L179
[3]:
https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=ti-linux-6.6.y-cicd&id=0be8de03825c2834a39af603b088cbf31e19d55d
[4]: https://gist.github.com/devarsht/009075d8706001f447733ed859152d90

Regards
Devarsh

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

[Nicolas Dufresne]

Hi Devarsh,

Le jeudi 20 juin 2024 à 15:05 +0530, Devarsh Thakkar a écrit :
> In my view the delayed suspend functionality is generally helpful for devices
> where resume latencies are higher for e.g. this light sensor driver [2] uses
> it because it takes 250ms to stabilize after resumption and I don't see this
> being used in codec drivers generally since there is no such large resume
> latency. Please let me know if I am missing something or there is a strong
> reason to have delayed suspend for wave5.

It sounds like you did proper scientific testing of the suspend results calls,
mind sharing the actual data ?

Nicolas

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

[Devarsh Thakkar]

Hi Nicolas,

On 20/06/24 19:35, Nicolas Dufresne wrote:
> Hi Devarsh,
> 
> Le jeudi 20 juin 2024 à 15:05 +0530, Devarsh Thakkar a écrit :
>> In my view the delayed suspend functionality is generally helpful for devices
>> where resume latencies are higher for e.g. this light sensor driver [2] uses
>> it because it takes 250ms to stabilize after resumption and I don't see this
>> being used in codec drivers generally since there is no such large resume
>> latency. Please let me know if I am missing something or there is a strong
>> reason to have delayed suspend for wave5.
> 
> It sounds like you did proper scientific testing of the suspend results calls,
> mind sharing the actual data ?

Nopes, I did not do that but yes I agree it is good to profile and evaluate
the trade-off but I am not expecting 250ms kind of latency. I would suggest
Jackson to do the profiling for the resume latencies.

But perhaps a separate issue, I did notice that intention of the patchset was
to suspend without waiting for the timeout if there is no application having a
handle to the wave5 device but even if I close the last instance I still see
the IP stays on for 5seconds as seen in this logs [1] and this perhaps could
be because extra pm counter references being hold.

[2024-06-20 12:32:50] Freeing pipeline ...

and after 5 seconds..

[2024-06-20 12:32:55] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_ON |
[2024-06-20 12:32:56] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_OFF

[1]: https://gist.github.com/devarsht/009075d8706001f447733ed859152d90

Regards
Devarsh

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

[Nicolas Dufresne]

Le jeudi 20 juin 2024 à 19:50 +0530, Devarsh Thakkar a écrit :
> Hi Nicolas,
> 
> On 20/06/24 19:35, Nicolas Dufresne wrote:
> > Hi Devarsh,
> > 
> > Le jeudi 20 juin 2024 à 15:05 +0530, Devarsh Thakkar a écrit :
> > > In my view the delayed suspend functionality is generally helpful for devices
> > > where resume latencies are higher for e.g. this light sensor driver [2] uses
> > > it because it takes 250ms to stabilize after resumption and I don't see this
> > > being used in codec drivers generally since there is no such large resume
> > > latency. Please let me know if I am missing something or there is a strong
> > > reason to have delayed suspend for wave5.
> > 
> > It sounds like you did proper scientific testing of the suspend results calls,
> > mind sharing the actual data ?
> 
> Nopes, I did not do that but yes I agree it is good to profile and evaluate
> the trade-off but I am not expecting 250ms kind of latency. I would suggest
> Jackson to do the profiling for the resume latencies.

I'd clearly like to see numbers before we proceed.

> 
> But perhaps a separate issue, I did notice that intention of the patchset was
> to suspend without waiting for the timeout if there is no application having a
> handle to the wave5 device but even if I close the last instance I still see
> the IP stays on for 5seconds as seen in this logs [1] and this perhaps could
> be because extra pm counter references being hold.

Not sure where this comes from, I'm not aware of drivers doing that with M2M
instances. Only 

> 
> [2024-06-20 12:32:50] Freeing pipeline ...
> 
> and after 5 seconds..
> 
> [2024-06-20 12:32:55] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_ON |
> [2024-06-20 12:32:56] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_OFF
> 
> [1]: https://gist.github.com/devarsht/009075d8706001f447733ed859152d90

Appart from the 5s being too long, that is expected. If it fails after that,
this is a bug, we we should hold on merging this until the problem has been
resolved.

Imagine that userspace is going gapless playback, if you have a lets say 30ms on
forced suspend cycle due to close/open of the decoder instance, it won't
actually endup gapless. The delay will ensure that we only suspend when needed.

There is other changes I have asked in this series, since we always have the
case where userspace just pause on streaming, and we want that prolonged paused
lead to suspend. Hopefully this has been strongly tested and is not just added
for "completeness".

Its important to note that has a reviewer only, my time is limited, and I
completely rely on the author judgment of delay tuning and actual testing.

Nicolas

> 
> Regards
> Devarsh

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

[jackson.lee]

Hi Nicolas / Devarsh


There are lots of mail thread in the loop, I have confusion.
I'd like to make check-up list for the "Support runtime suspend/resume" patch.

1. Profiling resume latency
2. after that, adjusting the time.

The patch set is okay except the above thing. ?

Thanks.
Jackson


> -----Original Message-----
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Sent: Friday, June 21, 2024 2:33 AM
> To: Devarsh Thakkar <devarsht@ti.com>; jackson.lee
> <jackson.lee@chipsnmedia.com>; mchehab@kernel.org;
> sebastian.fricke@collabora.com
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> hverkuil@xs4all.nl; Nas Chung <nas.chung@chipsnmedia.com>; lafley.kim
> <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; Luthra, Jai <j-luthra@ti.com>;
> Vibhore <vibhore@ti.com>; Dhruva Gole <d-gole@ti.com>; Aradhya <a-
> bhatia1@ti.com>; Raghavendra, Vignesh <vigneshr@ti.com>
> Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
> suspend/resume
> 
> Le jeudi 20 juin 2024 à 19:50 +0530, Devarsh Thakkar a écrit :
> > Hi Nicolas,
> >
> > On 20/06/24 19:35, Nicolas Dufresne wrote:
> > > Hi Devarsh,
> > >
> > > Le jeudi 20 juin 2024 à 15:05 +0530, Devarsh Thakkar a écrit :
> > > > In my view the delayed suspend functionality is generally helpful
> > > > for devices where resume latencies are higher for e.g. this light
> > > > sensor driver [2] uses it because it takes 250ms to stabilize
> > > > after resumption and I don't see this being used in codec drivers
> > > > generally since there is no such large resume latency. Please let
> > > > me know if I am missing something or there is a strong reason to have
> delayed suspend for wave5.
> > >
> > > It sounds like you did proper scientific testing of the suspend
> > > results calls, mind sharing the actual data ?
> >
> > Nopes, I did not do that but yes I agree it is good to profile and
> > evaluate the trade-off but I am not expecting 250ms kind of latency. I
> > would suggest Jackson to do the profiling for the resume latencies.
> 
> I'd clearly like to see numbers before we proceed.
> 
> >
> > But perhaps a separate issue, I did notice that intention of the
> > patchset was to suspend without waiting for the timeout if there is no
> > application having a handle to the wave5 device but even if I close
> > the last instance I still see the IP stays on for 5seconds as seen in
> > this logs [1] and this perhaps could be because extra pm counter references
> being hold.
> 
> Not sure where this comes from, I'm not aware of drivers doing that with M2M
> instances. Only
> 
> >
> > [2024-06-20 12:32:50] Freeing pipeline ...
> >
> > and after 5 seconds..
> >
> > [2024-06-20 12:32:55] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_ON |
> > [2024-06-20 12:32:56] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_OFF
> >
> > [1]: https://gist.github.com/devarsht/009075d8706001f447733ed859152d90
> 
> Appart from the 5s being too long, that is expected. If it fails after that,
> this is a bug, we we should hold on merging this until the problem has been
> resolved.
> 
> Imagine that userspace is going gapless playback, if you have a lets say 30ms
> on forced suspend cycle due to close/open of the decoder instance, it won't
> actually endup gapless. The delay will ensure that we only suspend when
> needed.
> 
> There is other changes I have asked in this series, since we always have the
> case where userspace just pause on streaming, and we want that prolonged
> paused lead to suspend. Hopefully this has been strongly tested and is not
> just added for "completeness".
> 
> Its important to note that has a reviewer only, my time is limited, and I
> completely rely on the author judgment of delay tuning and actual testing.
> 
> Nicolas
> 
> >
> > Regards
> > Devarsh

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

[Devarsh Thakkar]

Hi Jackson,

On 21/06/24 06:00, jackson.lee wrote:
> Hi Nicolas / Devarsh
> 
> 
> There are lots of mail thread in the loop, I have confusion.
> I'd like to make check-up list for the "Support runtime suspend/resume" patch.
> 
> 1. Profiling resume latency
> 2. after that, adjusting the time.
> 

Beyond above two points,

3. I think this patchset also breaks hrtimer polling and so the VPU operation
on AM62A which completely relies on polling, you can test with removing the
interrupt property from your dts file before/after this patch-set. With the
polling it needs to be taken care that polling is started only after device is
on power-on state and is stopped before device gets suspended.

4. There is some discussion going on between me and Nicholas on whether
delayed suspend is really required after last instance close or not. My
thought was that we should suspend immediately after last instance close, but
Nicolas mentioned some concerns w.r.t use-cases such as gapless playback so I
am following up with him.

Regards
Devarsh

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

[jackson.lee]

> -----Original Message-----
> From: Devarsh Thakkar <devarsht@ti.com>
> Sent: Friday, June 21, 2024 8:55 PM
> To: jackson.lee <jackson.lee@chipsnmedia.com>; Nicolas Dufresne
> <nicolas.dufresne@collabora.com>; mchehab@kernel.org;
> sebastian.fricke@collabora.com
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> hverkuil@xs4all.nl; Nas Chung <nas.chung@chipsnmedia.com>; lafley.kim
> <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; Luthra, Jai <j-luthra@ti.com>;
> Vibhore <vibhore@ti.com>; Dhruva Gole <d-gole@ti.com>; Aradhya <a-
> bhatia1@ti.com>; Raghavendra, Vignesh <vigneshr@ti.com>
> Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
> suspend/resume
> 
> Hi Jackson,
> 
> On 21/06/24 06:00, jackson.lee wrote:
> > Hi Nicolas / Devarsh
> >
> >
> > There are lots of mail thread in the loop, I have confusion.
> > I'd like to make check-up list for the "Support runtime suspend/resume"
> patch.
> >
> > 1. Profiling resume latency
> > 2. after that, adjusting the time.
> >
> 
> Beyond above two points,
> 

Hi Brandon

According to today meeting, should we take care of this ?

> 3. I think this patchset also breaks hrtimer polling and so the VPU operation
> on AM62A which completely relies on polling, you can test with removing the
> interrupt property from your dts file before/after this patch-set. With the
> polling it needs to be taken care that polling is started only after device
> is on power-on state and is stopped before device gets suspended.
> 

Hi Devarsh

I have already sent some changes to fix this in the previous e-mail. Please refer to the e-mail.

> 4. There is some discussion going on between me and Nicholas on whether
> delayed suspend is really required after last instance close or not. My
> thought was that we should suspend immediately after last instance close, but
> Nicolas mentioned some concerns w.r.t use-cases such as gapless playback so I
> am following up with him.
> 
> Regards
> Devarsh

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

[Devarsh Thakkar]

Hi Nicolas,

On 20/06/24 23:02, Nicolas Dufresne wrote:
> Le jeudi 20 juin 2024 à 19:50 +0530, Devarsh Thakkar a écrit :
[..]
 > Imagine that userspace is going gapless playback, if you have a lets say
30ms on
> forced suspend cycle due to close/open of the decoder instance, it won't
> actually endup gapless. The delay will ensure that we only suspend when needed.
> 

Shouldn't the applications doing gapless playback avoid frequent open/close of
the decoder instance too as it will add up re-instantiation (initializing hw,
allocating buffers) and cleanup (de-initialization and freeing up of buffers)
delay for each open/close respectively ? Even in case of scenario where
resolution of next stream is different than previous, I guess the application
can still hold up the file handle and do the necessary setup (stream
off/stream on/REQBUFS etc) required for re-initialization ?

Regards
Devarsh

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

[Nicolas Dufresne]

Hi,

Le vendredi 21 juin 2024 à 18:01 +0530, Devarsh Thakkar a écrit :
> Hi Nicolas,
> 
> On 20/06/24 23:02, Nicolas Dufresne wrote:
> > Le jeudi 20 juin 2024 à 19:50 +0530, Devarsh Thakkar a écrit :
> [..]
>  > Imagine that userspace is going gapless playback, if you have a lets say
> 30ms on
> > forced suspend cycle due to close/open of the decoder instance, it won't
> > actually endup gapless. The delay will ensure that we only suspend when needed.
> > 
> 
> Shouldn't the applications doing gapless playback avoid frequent open/close of
> the decoder instance too as it will add up re-instantiation (initializing hw,
> allocating buffers) and cleanup (de-initialization and freeing up of buffers)
> delay for each open/close respectively ? Even in case of scenario where
> resolution of next stream is different than previous, I guess the application
> can still hold up the file handle and do the necessary setup (stream
> off/stream on/REQBUFS etc) required for re-initialization ?

I don't have a very strong opinion here, I usually try to avoid optimizing for
what userspace should do. Best would be to build your opinion on your own
testing of existing userspace (perhaps not just GStreamer).

I think if you have good reason to force suspend when the last instance is
destroyed, please do so (e.g. stability issue, race conditions etc). So far, I
don't personally know what is the issue with leaving a small delay in order to
avoid a suspend / resume cycle if one quickly close the last instance and open
the next one immediately. A comment would be nice, so no one fall in such a trap
later.

Nicolas

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

[jackson.lee]

Hi Nicolas

> -----Original Message-----
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Sent: Friday, June 21, 2024 2:33 AM
> To: Devarsh Thakkar <devarsht@ti.com>; jackson.lee
> <jackson.lee@chipsnmedia.com>; mchehab@kernel.org;
> sebastian.fricke@collabora.com
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> hverkuil@xs4all.nl; Nas Chung <nas.chung@chipsnmedia.com>; lafley.kim
> <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; Luthra, Jai <j-luthra@ti.com>;
> Vibhore <vibhore@ti.com>; Dhruva Gole <d-gole@ti.com>; Aradhya <a-
> bhatia1@ti.com>; Raghavendra, Vignesh <vigneshr@ti.com>
> Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
> suspend/resume
> 
> Le jeudi 20 juin 2024 à 19:50 +0530, Devarsh Thakkar a écrit :
> > Hi Nicolas,
> >
> > On 20/06/24 19:35, Nicolas Dufresne wrote:
> > > Hi Devarsh,
> > >
> > > Le jeudi 20 juin 2024 à 15:05 +0530, Devarsh Thakkar a écrit :
> > > > In my view the delayed suspend functionality is generally helpful
> > > > for devices where resume latencies are higher for e.g. this light
> > > > sensor driver [2] uses it because it takes 250ms to stabilize
> > > > after resumption and I don't see this being used in codec drivers
> > > > generally since there is no such large resume latency. Please let
> > > > me know if I am missing something or there is a strong reason to have
> delayed suspend for wave5.
> > >
> > > It sounds like you did proper scientific testing of the suspend
> > > results calls, mind sharing the actual data ?
> >
> > Nopes, I did not do that but yes I agree it is good to profile and
> > evaluate the trade-off but I am not expecting 250ms kind of latency. I
> > would suggest Jackson to do the profiling for the resume latencies.
> 
> I'd clearly like to see numbers before we proceed.
> 

I measured latency for the resume and suspend of our hw block.

Resume : 124 microsecond
Suspend : 355 microsecond

I think if the delay is 100ms, it is enough.
How about this ?

> >
> > But perhaps a separate issue, I did notice that intention of the
> > patchset was to suspend without waiting for the timeout if there is no
> > application having a handle to the wave5 device but even if I close
> > the last instance I still see the IP stays on for 5seconds as seen in
> > this logs [1] and this perhaps could be because extra pm counter references
> being hold.
> 
> Not sure where this comes from, I'm not aware of drivers doing that with M2M
> instances. Only
> 
> >
> > [2024-06-20 12:32:50] Freeing pipeline ...
> >
> > and after 5 seconds..
> >
> > [2024-06-20 12:32:55] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_ON |
> > [2024-06-20 12:32:56] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_OFF
> >
> > [1]: https://gist.github.com/devarsht/009075d8706001f447733ed859152d90
> 
> Appart from the 5s being too long, that is expected. If it fails after that,
> this is a bug, we we should hold on merging this until the problem has been
> resolved.
> 

After 5sec, the hw goes to suspend. So there is no bug in the current patch-set.


Thanks


> Imagine that userspace is going gapless playback, if you have a lets say 30ms
> on forced suspend cycle due to close/open of the decoder instance, it won't
> actually endup gapless. The delay will ensure that we only suspend when
> needed.
> 
> There is other changes I have asked in this series, since we always have the
> case where userspace just pause on streaming, and we want that prolonged
> paused lead to suspend. Hopefully this has been strongly tested and is not
> just added for "completeness".
> 
> Its important to note that has a reviewer only, my time is limited, and I
> completely rely on the author judgment of delay tuning and actual testing.
> 
> Nicolas
> 
> >
> > Regards
> > Devarsh

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

[Nicolas Dufresne]

Hi Jackson,

Le vendredi 12 juillet 2024 à 06:10 +0000, jackson.lee a écrit :
> Hi Nicolas
> 
> > -----Original Message-----
> > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > Sent: Friday, June 21, 2024 2:33 AM
> > To: Devarsh Thakkar <devarsht@ti.com>; jackson.lee
> > <jackson.lee@chipsnmedia.com>; mchehab@kernel.org;
> > sebastian.fricke@collabora.com
> > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > hverkuil@xs4all.nl; Nas Chung <nas.chung@chipsnmedia.com>; lafley.kim
> > <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; Luthra, Jai <j-luthra@ti.com>;
> > Vibhore <vibhore@ti.com>; Dhruva Gole <d-gole@ti.com>; Aradhya <a-
> > bhatia1@ti.com>; Raghavendra, Vignesh <vigneshr@ti.com>
> > Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
> > suspend/resume
> > 
> > Le jeudi 20 juin 2024 à 19:50 +0530, Devarsh Thakkar a écrit :
> > > Hi Nicolas,
> > > 
> > > On 20/06/24 19:35, Nicolas Dufresne wrote:
> > > > Hi Devarsh,
> > > > 
> > > > Le jeudi 20 juin 2024 à 15:05 +0530, Devarsh Thakkar a écrit :
> > > > > In my view the delayed suspend functionality is generally helpful
> > > > > for devices where resume latencies are higher for e.g. this light
> > > > > sensor driver [2] uses it because it takes 250ms to stabilize
> > > > > after resumption and I don't see this being used in codec drivers
> > > > > generally since there is no such large resume latency. Please let
> > > > > me know if I am missing something or there is a strong reason to have
> > delayed suspend for wave5.
> > > > 
> > > > It sounds like you did proper scientific testing of the suspend
> > > > results calls, mind sharing the actual data ?
> > > 
> > > Nopes, I did not do that but yes I agree it is good to profile and
> > > evaluate the trade-off but I am not expecting 250ms kind of latency. I
> > > would suggest Jackson to do the profiling for the resume latencies.
> > 
> > I'd clearly like to see numbers before we proceed.
> > 
> 
> I measured latency for the resume and suspend of our hw block.
> 
> Resume : 124 microsecond
> Suspend : 355 microsecond
> 
> I think if the delay is 100ms, it is enough.
> How about this ?

Seem very fast operation indeed, so a very small delay seems logical. I believe
this is similar to what other drivers uses, so sounds good to me. 

**If** we decide to go lower or drop the delay, then I'd like see someones
benchmark that show that doing suspend during playback does improve power
efficiently without reducing throughput.

Nicolas

> 
> > > 
> > > But perhaps a separate issue, I did notice that intention of the
> > > patchset was to suspend without waiting for the timeout if there is no
> > > application having a handle to the wave5 device but even if I close
> > > the last instance I still see the IP stays on for 5seconds as seen in
> > > this logs [1] and this perhaps could be because extra pm counter references
> > being hold.
> > 
> > Not sure where this comes from, I'm not aware of drivers doing that with M2M
> > instances. Only
> > 
> > > 
> > > [2024-06-20 12:32:50] Freeing pipeline ...
> > > 
> > > and after 5 seconds..
> > > 
> > > [2024-06-20 12:32:55] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_ON |
> > > [2024-06-20 12:32:56] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_OFF
> > > 
> > > [1]: https://gist.github.com/devarsht/009075d8706001f447733ed859152d90
> > 
> > Appart from the 5s being too long, that is expected. If it fails after that,
> > this is a bug, we we should hold on merging this until the problem has been
> > resolved.
> > 
> 
> After 5sec, the hw goes to suspend. So there is no bug in the current patch-set.
> 
> 
> Thanks
> 
> 
> > Imagine that userspace is going gapless playback, if you have a lets say 30ms
> > on forced suspend cycle due to close/open of the decoder instance, it won't
> > actually endup gapless. The delay will ensure that we only suspend when
> > needed.
> > 
> > There is other changes I have asked in this series, since we always have the
> > case where userspace just pause on streaming, and we want that prolonged
> > paused lead to suspend. Hopefully this has been strongly tested and is not
> > just added for "completeness".
> > 
> > Its important to note that has a reviewer only, my time is limited, and I
> > completely rely on the author judgment of delay tuning and actual testing.
> > 
> > Nicolas
> > 
> > > 
> > > Regards
> > > Devarsh
> 

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

[jackson.lee]

Hi Nicolas

Thanks for your reply.

> -----Original Message-----
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Sent: Tuesday, July 16, 2024 2:06 AM
> To: jackson.lee <jackson.lee@chipsnmedia.com>; Devarsh Thakkar
> <devarsht@ti.com>; mchehab@kernel.org; sebastian.fricke@collabora.com
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> hverkuil@xs4all.nl; Nas Chung <nas.chung@chipsnmedia.com>; lafley.kim
> <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; Luthra, Jai <j-luthra@ti.com>;
> Vibhore <vibhore@ti.com>; Dhruva Gole <d-gole@ti.com>; Aradhya <a-
> bhatia1@ti.com>; Raghavendra, Vignesh <vigneshr@ti.com>
> Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
> suspend/resume
> 
> Hi Jackson,
> 
> Le vendredi 12 juillet 2024 à 06:10 +0000, jackson.lee a écrit :
> > Hi Nicolas
> >
> > > -----Original Message-----
> > > From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > > Sent: Friday, June 21, 2024 2:33 AM
> > > To: Devarsh Thakkar <devarsht@ti.com>; jackson.lee
> > > <jackson.lee@chipsnmedia.com>; mchehab@kernel.org;
> > > sebastian.fricke@collabora.com
> > > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > > hverkuil@xs4all.nl; Nas Chung <nas.chung@chipsnmedia.com>;
> > > lafley.kim <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; Luthra,
> > > Jai <j-luthra@ti.com>; Vibhore <vibhore@ti.com>; Dhruva Gole
> > > <d-gole@ti.com>; Aradhya <a- bhatia1@ti.com>; Raghavendra, Vignesh
> > > <vigneshr@ti.com>
> > > Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5:
> > > Support runtime suspend/resume
> > >
> > > Le jeudi 20 juin 2024 à 19:50 +0530, Devarsh Thakkar a écrit :
> > > > Hi Nicolas,
> > > >
> > > > On 20/06/24 19:35, Nicolas Dufresne wrote:
> > > > > Hi Devarsh,
> > > > >
> > > > > Le jeudi 20 juin 2024 à 15:05 +0530, Devarsh Thakkar a écrit :
> > > > > > In my view the delayed suspend functionality is generally
> > > > > > helpful for devices where resume latencies are higher for e.g.
> > > > > > this light sensor driver [2] uses it because it takes 250ms to
> > > > > > stabilize after resumption and I don't see this being used in
> > > > > > codec drivers generally since there is no such large resume
> > > > > > latency. Please let me know if I am missing something or there
> > > > > > is a strong reason to have
> > > delayed suspend for wave5.
> > > > >
> > > > > It sounds like you did proper scientific testing of the suspend
> > > > > results calls, mind sharing the actual data ?
> > > >
> > > > Nopes, I did not do that but yes I agree it is good to profile and
> > > > evaluate the trade-off but I am not expecting 250ms kind of
> > > > latency. I would suggest Jackson to do the profiling for the resume
> latencies.
> > >
> > > I'd clearly like to see numbers before we proceed.
> > >
> >
> > I measured latency for the resume and suspend of our hw block.
> >
> > Resume : 124 microsecond
> > Suspend : 355 microsecond
> >
> > I think if the delay is 100ms, it is enough.
> > How about this ?
> 
> Seem very fast operation indeed, so a very small delay seems logical. I
> believe this is similar to what other drivers uses, so sounds good to me.
> 
> **If** we decide to go lower or drop the delay, then I'd like see someones
> benchmark that show that doing suspend during playback does improve power
> efficiently without reducing throughput.
> 
> Nicolas
> 
> >
> > > >
> > > > But perhaps a separate issue, I did notice that intention of the
> > > > patchset was to suspend without waiting for the timeout if there
> > > > is no application having a handle to the wave5 device but even if
> > > > I close the last instance I still see the IP stays on for 5seconds
> > > > as seen in this logs [1] and this perhaps could be because extra
> > > > pm counter references
> > > being hold.
> > >
> > > Not sure where this comes from, I'm not aware of drivers doing that
> > > with M2M instances. Only
> > >
> > > >
> > > > [2024-06-20 12:32:50] Freeing pipeline ...
> > > >
> > > > and after 5 seconds..
> > > >
> > > > [2024-06-20 12:32:55] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_ON
> |
> > > > [2024-06-20 12:32:56] |   204     | AM62AX_DEV_CODEC0 | DEVICE_STATE_OFF
> > > >
> > > > [1]:
> > > > https://gist.github.com/devarsht/009075d8706001f447733ed859152d90
> > >
> > > Appart from the 5s being too long, that is expected. If it fails
> > > after that, this is a bug, we we should hold on merging this until
> > > the problem has been resolved.
> > >
> >
> > After 5sec, the hw goes to suspend. So there is no bug in the current
> patch-set.
> >
> >
> > Thanks
> >
> >
> > > Imagine that userspace is going gapless playback, if you have a lets
> > > say 30ms on forced suspend cycle due to close/open of the decoder
> > > instance, it won't actually endup gapless. The delay will ensure
> > > that we only suspend when needed.
> > >
> > > There is other changes I have asked in this series, since we always
> > > have the case where userspace just pause on streaming, and we want
> > > that prolonged paused lead to suspend. Hopefully this has been
> > > strongly tested and is not just added for "completeness".
> > >
> > > Its important to note that has a reviewer only, my time is limited,
> > > and I completely rely on the author judgment of delay tuning and actual
> testing.
> > >
> > > Nicolas
> > >
> > > >
> > > > Regards
> > > > Devarsh
> >

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

[Nicolas Dufresne]

Hi Jackson, Devarsh,

Le mercredi 19 juin 2024 à 23:56 +0000, jackson.lee a écrit :
> Hi Devarsh
> 
> If there is no feeding bitstreams during encoding and decoding frames, then driver's status is switched to suspended automatically by autosuspend.
> And if we don’t use autosuspend, it is very difficult for us to catch if there is feeding or not while working a pipeline.
> So it is very efficient for managing power status.
> 
> If the delay is very great value, we can adjust it.

One way to resolve this, would be if someone share measurement of the suspend /
resume cycle duration. With firmware (third party OS) like this, the cost and
duration is few order of magnitude higher then with more basic ASIC like Hantro
and other single function HW.

Yet, 5s might be to much (but clearly safe), but getting two low may means that
we suspect "between two frames", and if that happens, we may endup with various
range of side effect, like reduce throughput due to suspend collisions, or even
worse power footprint. Some lab testing to adjust the value will be needed, we
have very little of that happening at the moment as I understood.

Nicolas

> 
> Thanks
> Jackson
> 
> > -----Original Message-----
> > From: Devarsh Thakkar <devarsht@ti.com>
> > Sent: Wednesday, June 19, 2024 10:00 PM
> > To: jackson.lee <jackson.lee@chipsnmedia.com>; mchehab@kernel.org;
> > nicolas@ndufresne.ca; sebastian.fricke@collabora.com
> > Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > hverkuil@xs4all.nl; Nas Chung <nas.chung@chipsnmedia.com>; lafley.kim
> > <lafley.kim@chipsnmedia.com>; b-brnich@ti.com; Nicolas Dufresne
> > <nicolas.dufresne@collabora.com>
> > Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
> > suspend/resume
> > 
> > Hi Jackson,
> > 
> > Thanks for the patch.
> > On 17/06/24 16:18, Jackson.lee wrote:
> > > From: "jackson.lee" <jackson.lee@chipsnmedia.com>
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Jackson.lee <jackson.lee@chipsnmedia.com>
> > > Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> > > Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> > 
> > [..]
> > >  static int wave5_vpu_probe(struct platform_device *pdev)  {
> > >  	int ret;
> > > @@ -268,6 +301,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);
> > 
> > Why are we putting 5s delay for autosuspend ? Without using auto-suspend
> > delay too, we can directly go to suspended state when last instance is closed
> > and resume back when first instance is open.
> > 
> > I don't think having an autosuspend delay (especially of 5s) bodes well with
> > low power-centric devices such as AM62A where we would prefer to go to
> > suspend state as soon as possible when the last instance is closed.
> > 
> > Also apologies for the delay in review, this didn't caught my eye earlier as
> > commit message did not mention it either.
> > 
> > Regards
> > Devarsh

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

[Devarsh Thakkar]

Hi Jackson, Nicolas,

On 20/06/24 19:33, Nicolas Dufresne wrote:
> Hi Jackson, Devarsh,
> 
> Le mercredi 19 juin 2024 à 23:56 +0000, jackson.lee a écrit :
>> Hi Devarsh
>>
>> If there is no feeding bitstreams during encoding and decoding frames, then driver's status is switched to suspended automatically by autosuspend.
>> And if we don’t use autosuspend, it is very difficult for us to catch if there is feeding or not while working a pipeline.
>> So it is very efficient for managing power status.
>>
>> If the delay is very great value, we can adjust it.
> 
> One way to resolve this, would be if someone share measurement of the suspend /
> resume cycle duration. With firmware (third party OS) like this, the cost and
> duration is few order of magnitude higher then with more basic ASIC like Hantro
> and other single function HW.
> 
> Yet, 5s might be to much (but clearly safe), but getting two low may means that
> we suspect "between two frames", and if that happens, we may endup with various
> range of side effect, like reduce throughput due to suspend collisions, or even
> worse power footprint. Some lab testing to adjust the value will be needed, we
> have very little of that happening at the moment as I understood.
> 

Okay I see the intention here is that if there is a process holding the vpu
device handle and the input feed is stalled for some seconds due to network
delay or CPU throughput then after a specified timeout say 5 seconds we want
to suspend even if the process is still active and holding the vpu device
handle ? I agree then if we want to support this feature a safer/slightly
larger value is required to avoid frequent suspend/resume due to network
jitter or any other bottleneck and maybe 5s is a good value to start with.

But if last instance is closed/stops streaming and there is no process holding
the device handle anymore then I think we should suspend immediately without
any delay.

Regards
Devarsh

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

[Nicolas Dufresne]

Le jeudi 20 juin 2024 à 20:22 +0530, Devarsh Thakkar a écrit :
> Hi Jackson, Nicolas,
> 
> On 20/06/24 19:33, Nicolas Dufresne wrote:
> > Hi Jackson, Devarsh,
> > 
> > Le mercredi 19 juin 2024 à 23:56 +0000, jackson.lee a écrit :
> > > Hi Devarsh
> > > 
> > > If there is no feeding bitstreams during encoding and decoding frames, then driver's status is switched to suspended automatically by autosuspend.
> > > And if we don’t use autosuspend, it is very difficult for us to catch if there is feeding or not while working a pipeline.
> > > So it is very efficient for managing power status.
> > > 
> > > If the delay is very great value, we can adjust it.
> > 
> > One way to resolve this, would be if someone share measurement of the suspend /
> > resume cycle duration. With firmware (third party OS) like this, the cost and
> > duration is few order of magnitude higher then with more basic ASIC like Hantro
> > and other single function HW.
> > 
> > Yet, 5s might be to much (but clearly safe), but getting two low may means that
> > we suspect "between two frames", and if that happens, we may endup with various
> > range of side effect, like reduce throughput due to suspend collisions, or even
> > worse power footprint. Some lab testing to adjust the value will be needed, we
> > have very little of that happening at the moment as I understood.
> > 
> 
> Okay I see the intention here is that if there is a process holding the vpu
> device handle and the input feed is stalled for some seconds due to network
> delay or CPU throughput then after a specified timeout say 5 seconds we want
> to suspend even if the process is still active and holding the vpu device
> handle ? I agree then if we want to support this feature a safer/slightly
> larger value is required to avoid frequent suspend/resume due to network
> jitter or any other bottleneck and maybe 5s is a good value to start with.
> 
> But if last instance is closed/stops streaming and there is no process holding
> the device handle anymore then I think we should suspend immediately without
> any delay.

Our emails crossed each other, but see my explanation about gapless playback
transiton, were userspace may destroy and create a new video session. I believe
5s is way too long to be honest.

Nicolas

> 
> Regards
> Devarsh

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

[jackson.lee]

Hi Nicolas and Devarsh

> -----Original Message-----
> From: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> Sent: Friday, June 21, 2024 12:24 AM
> To: Devarsh Thakkar <devarsht@ti.com>; jackson.lee
> <jackson.lee@chipsnmedia.com>; mchehab@kernel.org;
> sebastian.fricke@collabora.com
> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> hverkuil@xs4all.nl; Nas Chung <nas.chung@chipsnmedia.com>; lafley.kim
> <lafley.kim@chipsnmedia.com>; b-brnich@ti.com
> Subject: Re: [RESEND PATCH v6 2/4] media: chips-media: wave5: Support runtime
> suspend/resume
> 
> Le jeudi 20 juin 2024 à 20:22 +0530, Devarsh Thakkar a écrit :
> > Hi Jackson, Nicolas,
> >
> > On 20/06/24 19:33, Nicolas Dufresne wrote:
> > > Hi Jackson, Devarsh,
> > >
> > > Le mercredi 19 juin 2024 à 23:56 +0000, jackson.lee a écrit :
> > > > Hi Devarsh
> > > >
> > > > If there is no feeding bitstreams during encoding and decoding frames,
> then driver's status is switched to suspended automatically by autosuspend.
> > > > And if we don’t use autosuspend, it is very difficult for us to catch
> if there is feeding or not while working a pipeline.
> > > > So it is very efficient for managing power status.
> > > >
> > > > If the delay is very great value, we can adjust it.
> > >
> > > One way to resolve this, would be if someone share measurement of
> > > the suspend / resume cycle duration. With firmware (third party OS)
> > > like this, the cost and duration is few order of magnitude higher
> > > then with more basic ASIC like Hantro and other single function HW.
> > >
> > > Yet, 5s might be to much (but clearly safe), but getting two low may
> > > means that we suspect "between two frames", and if that happens, we
> > > may endup with various range of side effect, like reduce throughput
> > > due to suspend collisions, or even worse power footprint. Some lab
> > > testing to adjust the value will be needed, we have very little of that
> happening at the moment as I understood.
> > >
> >
> > Okay I see the intention here is that if there is a process holding
> > the vpu device handle and the input feed is stalled for some seconds
> > due to network delay or CPU throughput then after a specified timeout
> > say 5 seconds we want to suspend even if the process is still active
> > and holding the vpu device handle ? I agree then if we want to support
> > this feature a safer/slightly larger value is required to avoid
> > frequent suspend/resume due to network jitter or any other bottleneck and
> maybe 5s is a good value to start with.
> >
> > But if last instance is closed/stops streaming and there is no process
> > holding the device handle anymore then I think we should suspend
> > immediately without any delay.
> 
> Our emails crossed each other, but see my explanation about gapless playback
> transiton, were userspace may destroy and create a new video session. I
> believe 5s is way too long to be honest.
> 

I investigated why it takes 5 sec to go to suspend even if the last instance is closed
The reason is if autosuspend is used, timeout should happen to go to suspend even though the power.usage count is 0 after an instance is closed.


So I made the below modification, when the last instance is closed, autosuspend turns off,  and when the first instance is opened, the autosuspend turns on, again.
When I tested with the change, it works well.

Can you review the below code?


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 1aa5b6788266..87932d1550ce 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -1714,6 +1714,8 @@ static int wave5_vpu_open_dec(struct file *filp)
        struct vpu_device *dev = video_drvdata(filp);
        struct vpu_instance *inst = NULL;
        struct v4l2_m2m_ctx *m2m_ctx;
+       int inst_count = 0;
+       struct vpu_instance *inst_elm;
        int ret = 0;

        inst = kzalloc(sizeof(*inst), GFP_KERNEL);
@@ -1799,6 +1801,12 @@ static int wave5_vpu_open_dec(struct file *filp)
                hrtimer_start(&dev->hrtimer, ns_to_ktime(dev->vpu_poll_interval * NSEC_PER_MSEC),
                              HRTIMER_MODE_REL_PINNED);

+       list_for_each_entry(inst_elm, &dev->instances, list)
+               inst_count++;
+
+       if (!inst_count)
+               pm_runtime_use_autosuspend(inst->dev->dev);
+
        list_add_tail(&inst->list, &dev->instances);

        mutex_unlock(&dev->dev_lock);


diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
index b0911fef232f..05b83445c650 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
@@ -197,6 +197,8 @@ int wave5_vpu_dec_close(struct vpu_instance *inst, u32 *fail_res)
        int retry = 0;
        struct vpu_device *vpu_dev = inst->dev;
        int i;
+       int inst_count = 0;
+       struct vpu_instance *inst_elm;

        *fail_res = 0;
        if (!inst->codec_info)
@@ -239,8 +241,14 @@ int wave5_vpu_dec_close(struct vpu_instance *inst, u32 *fail_res)
        wave5_vdi_free_dma_memory(vpu_dev, &p_dec_info->vb_task);

 unlock_and_return:
-       mutex_unlock(&vpu_dev->hw_lock);
+       list_for_each_entry(inst_elm, &vpu_dev->instances, list)
+               inst_count++;
+
+       if (inst_count == 1)
+               pm_runtime_dont_use_autosuspend(inst->dev->dev);
+
        pm_runtime_put_sync(inst->dev->dev);
+       mutex_unlock(&vpu_dev->hw_lock);
        return ret;
 }



> Nicolas
> 
> >
> > Regards
> > Devarsh

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

[Nicolas Dufresne]

Hi,

one more thing I notice below when comparing with Hantro ...

Le lundi 17 juin 2024 à 19:48 +0900, Jackson.lee a écrit :
> From: "jackson.lee" <jackson.lee@chipsnmedia.com>
> 

[...]

>  
>  err_enc_unreg:
> @@ -295,6 +334,9 @@ static void wave5_vpu_remove(struct platform_device *pdev)
>  		hrtimer_cancel(&dev->hrtimer);
>  	}
>  
> +	pm_runtime_put_sync(&pdev->dev);

I don't know if its strictly needed, but I noticed that Hantro calls
pm_runtime_dont_use_autosuspend() in its remove function. Can you check if this
is strictly needed, we don't want anything to call again later if we are
removing the module, so better check.

Nicolas

> +	pm_runtime_disable(&pdev->dev);
> +
>  	mutex_destroy(&dev->dev_lock);
>  	mutex_destroy(&dev->hw_lock);
>  	clk_bulk_disable_unprepare(dev->num_clks, dev->clks);
> @@ -320,6 +362,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_new = wave5_vpu_remove,

[RESEND PATCH v6 3/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage.

[Jackson.lee]

From: "jackson.lee" <jackson.lee@chipsnmedia.com>

Use v4l2-common helper functions to calculate bytesperline and sizeimage,
instead of calculating in a wave5 driver directly.

In case of raw(YUV) v4l2_pix_format, the wave5 driver updates
v4l2_pix_format_mplane struct through v4l2_fill_pixfmt_mp() function.

Encoder and Decoder need same bytesperline and sizeimage values
for same v4l2_pix_format.
So, a wave5_update_pix_fmt is refactored to support both together.

Signed-off-by: Jackson.lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 .../platform/chips-media/wave5/wave5-helper.c |  24 ++
 .../platform/chips-media/wave5/wave5-helper.h |   5 +
 .../chips-media/wave5/wave5-vpu-dec.c         | 300 +++++++-----------
 .../chips-media/wave5/wave5-vpu-enc.c         | 197 +++++-------
 .../platform/chips-media/wave5/wave5-vpu.h    |   5 +-
 .../chips-media/wave5/wave5-vpuconfig.h       |  27 +-
 6 files changed, 239 insertions(+), 319 deletions(-)

diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.c b/drivers/media/platform/chips-media/wave5/wave5-helper.c
index 7e0f34bfa5be..b20ab69cd341 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-helper.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-helper.c
@@ -7,6 +7,8 @@
 
 #include "wave5-helper.h"
 
+#define DEFAULT_BS_SIZE(width, height) ((width) * (height) / 8 * 3)
+
 const char *state_to_str(enum vpu_instance_state state)
 {
 	switch (state) {
@@ -224,3 +226,25 @@ void wave5_return_bufs(struct vb2_queue *q, u32 state)
 		v4l2_m2m_buf_done(vbuf, state);
 	}
 }
+
+void wave5_update_pix_fmt(struct v4l2_pix_format_mplane *pix_mp,
+			  int pix_fmt_type,
+			  unsigned int width,
+			  unsigned int height,
+			  const struct v4l2_frmsize_stepwise *frmsize)
+{
+	v4l2_apply_frmsize_constraints(&width, &height, frmsize);
+
+	if (pix_fmt_type == VPU_FMT_TYPE_CODEC) {
+		pix_mp->width = width;
+		pix_mp->height = height;
+		pix_mp->num_planes = 1;
+		pix_mp->plane_fmt[0].bytesperline = 0;
+		pix_mp->plane_fmt[0].sizeimage = max(DEFAULT_BS_SIZE(width, height),
+						     pix_mp->plane_fmt[0].sizeimage);
+	} else {
+		v4l2_fill_pixfmt_mp(pix_mp, pix_mp->pixelformat, width, height);
+	}
+	pix_mp->flags = 0;
+	pix_mp->field = V4L2_FIELD_NONE;
+}
diff --git a/drivers/media/platform/chips-media/wave5/wave5-helper.h b/drivers/media/platform/chips-media/wave5/wave5-helper.h
index 6cee1c14d3ce..9937fce553fc 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-helper.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-helper.h
@@ -28,4 +28,9 @@ const struct vpu_format *wave5_find_vpu_fmt_by_idx(unsigned int idx,
 						   const struct vpu_format fmt_list[MAX_FMTS]);
 enum wave_std wave5_to_vpu_std(unsigned int v4l2_pix_fmt, enum vpu_instance_type type);
 void wave5_return_bufs(struct vb2_queue *q, u32 state);
+void wave5_update_pix_fmt(struct v4l2_pix_format_mplane *pix_mp,
+			  int pix_fmt_type,
+			  unsigned int width,
+			  unsigned int height,
+			  const struct v4l2_frmsize_stepwise *frmsize);
 #endif
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 861a0664047c..5901a8bb0797 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
@@ -11,111 +11,92 @@
 #define VPU_DEC_DEV_NAME "C&M Wave5 VPU decoder"
 #define VPU_DEC_DRV_NAME "wave5-dec"
 
-#define DEFAULT_SRC_SIZE(width, height) ({			\
-	(width) * (height) / 8 * 3;					\
-})
+static const struct v4l2_frmsize_stepwise dec_hevc_frmsize = {
+	.min_width = W5_MIN_DEC_PIC_8_WIDTH,
+	.max_width = W5_MAX_DEC_PIC_WIDTH,
+	.step_width = W5_DEC_CODEC_STEP_WIDTH,
+	.min_height = W5_MIN_DEC_PIC_8_HEIGHT,
+	.max_height = W5_MAX_DEC_PIC_HEIGHT,
+	.step_height = W5_DEC_CODEC_STEP_HEIGHT,
+};
+
+static const struct v4l2_frmsize_stepwise dec_h264_frmsize = {
+	.min_width = W5_MIN_DEC_PIC_32_WIDTH,
+	.max_width = W5_MAX_DEC_PIC_WIDTH,
+	.step_width = W5_DEC_CODEC_STEP_WIDTH,
+	.min_height = W5_MIN_DEC_PIC_32_HEIGHT,
+	.max_height = W5_MAX_DEC_PIC_HEIGHT,
+	.step_height = W5_DEC_CODEC_STEP_HEIGHT,
+};
+
+static const struct v4l2_frmsize_stepwise dec_raw_frmsize = {
+	.min_width = W5_MIN_DEC_PIC_8_WIDTH,
+	.max_width = W5_MAX_DEC_PIC_WIDTH,
+	.step_width = W5_DEC_RAW_STEP_WIDTH,
+	.min_height = W5_MIN_DEC_PIC_8_HEIGHT,
+	.max_height = W5_MAX_DEC_PIC_HEIGHT,
+	.step_height = W5_DEC_RAW_STEP_HEIGHT,
+};
 
 static const struct vpu_format dec_fmt_list[FMT_TYPES][MAX_FMTS] = {
 	[VPU_FMT_TYPE_CODEC] = {
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_HEVC,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_hevc_frmsize,
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_H264,
-			.max_width = 8192,
-			.min_width = 32,
-			.max_height = 4320,
-			.min_height = 32,
+			.v4l2_frmsize = &dec_h264_frmsize,
 		},
 	},
 	[VPU_FMT_TYPE_RAW] = {
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_YUV420,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_raw_frmsize,
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV12,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_raw_frmsize,
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV21,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_raw_frmsize,
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_YUV422P,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_raw_frmsize,
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV16,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_raw_frmsize,
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV61,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_raw_frmsize,
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_YUV420M,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_raw_frmsize,
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV12M,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_raw_frmsize,
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV21M,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_raw_frmsize,
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_YUV422M,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_raw_frmsize,
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV16M,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_raw_frmsize,
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV61M,
-			.max_width = 8192,
-			.min_width = 8,
-			.max_height = 4320,
-			.min_height = 8,
+			.v4l2_frmsize = &dec_raw_frmsize,
 		},
 	}
 };
@@ -234,74 +215,6 @@ static void wave5_handle_src_buffer(struct vpu_instance *inst, dma_addr_t rd_ptr
 	inst->remaining_consumed_bytes = consumed_bytes;
 }
 
-static void wave5_update_pix_fmt(struct v4l2_pix_format_mplane *pix_mp, unsigned int width,
-				 unsigned int height)
-{
-	switch (pix_mp->pixelformat) {
-	case V4L2_PIX_FMT_YUV420:
-	case V4L2_PIX_FMT_NV12:
-	case V4L2_PIX_FMT_NV21:
-		pix_mp->width = round_up(width, 32);
-		pix_mp->height = round_up(height, 16);
-		pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
-		pix_mp->plane_fmt[0].sizeimage = width * height * 3 / 2;
-		break;
-	case V4L2_PIX_FMT_YUV422P:
-	case V4L2_PIX_FMT_NV16:
-	case V4L2_PIX_FMT_NV61:
-		pix_mp->width = round_up(width, 32);
-		pix_mp->height = round_up(height, 16);
-		pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
-		pix_mp->plane_fmt[0].sizeimage = width * height * 2;
-		break;
-	case V4L2_PIX_FMT_YUV420M:
-		pix_mp->width = round_up(width, 32);
-		pix_mp->height = round_up(height, 16);
-		pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
-		pix_mp->plane_fmt[0].sizeimage = width * height;
-		pix_mp->plane_fmt[1].bytesperline = round_up(width, 32) / 2;
-		pix_mp->plane_fmt[1].sizeimage = width * height / 4;
-		pix_mp->plane_fmt[2].bytesperline = round_up(width, 32) / 2;
-		pix_mp->plane_fmt[2].sizeimage = width * height / 4;
-		break;
-	case V4L2_PIX_FMT_NV12M:
-	case V4L2_PIX_FMT_NV21M:
-		pix_mp->width = round_up(width, 32);
-		pix_mp->height = round_up(height, 16);
-		pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
-		pix_mp->plane_fmt[0].sizeimage = width * height;
-		pix_mp->plane_fmt[1].bytesperline = round_up(width, 32);
-		pix_mp->plane_fmt[1].sizeimage = width * height / 2;
-		break;
-	case V4L2_PIX_FMT_YUV422M:
-		pix_mp->width = round_up(width, 32);
-		pix_mp->height = round_up(height, 16);
-		pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
-		pix_mp->plane_fmt[0].sizeimage = width * height;
-		pix_mp->plane_fmt[1].bytesperline = round_up(width, 32) / 2;
-		pix_mp->plane_fmt[1].sizeimage = width * height / 2;
-		pix_mp->plane_fmt[2].bytesperline = round_up(width, 32) / 2;
-		pix_mp->plane_fmt[2].sizeimage = width * height / 2;
-		break;
-	case V4L2_PIX_FMT_NV16M:
-	case V4L2_PIX_FMT_NV61M:
-		pix_mp->width = round_up(width, 32);
-		pix_mp->height = round_up(height, 16);
-		pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
-		pix_mp->plane_fmt[0].sizeimage = width * height;
-		pix_mp->plane_fmt[1].bytesperline = round_up(width, 32);
-		pix_mp->plane_fmt[1].sizeimage = width * height;
-		break;
-	default:
-		pix_mp->width = width;
-		pix_mp->height = height;
-		pix_mp->plane_fmt[0].bytesperline = 0;
-		pix_mp->plane_fmt[0].sizeimage = max(DEFAULT_SRC_SIZE(width, height),
-						     pix_mp->plane_fmt[0].sizeimage);
-		break;
-	}
-}
-
 static int start_decode(struct vpu_instance *inst, u32 *fail_res)
 {
 	struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
@@ -389,6 +302,8 @@ static int handle_dynamic_resolution_change(struct vpu_instance *inst)
 	}
 
 	if (p_dec_info->initial_info_obtained) {
+		const struct vpu_format *vpu_fmt;
+
 		inst->conf_win.left = initial_info->pic_crop_rect.left;
 		inst->conf_win.top = initial_info->pic_crop_rect.top;
 		inst->conf_win.width = initial_info->pic_width -
@@ -396,10 +311,27 @@ static int handle_dynamic_resolution_change(struct vpu_instance *inst)
 		inst->conf_win.height = initial_info->pic_height -
 			initial_info->pic_crop_rect.top - initial_info->pic_crop_rect.bottom;
 
-		wave5_update_pix_fmt(&inst->src_fmt, initial_info->pic_width,
-				     initial_info->pic_height);
-		wave5_update_pix_fmt(&inst->dst_fmt, initial_info->pic_width,
-				     initial_info->pic_height);
+		vpu_fmt = wave5_find_vpu_fmt(inst->src_fmt.pixelformat,
+					     dec_fmt_list[VPU_FMT_TYPE_CODEC]);
+		if (!vpu_fmt)
+			return -EINVAL;
+
+		wave5_update_pix_fmt(&inst->src_fmt,
+				     VPU_FMT_TYPE_CODEC,
+				     initial_info->pic_width,
+				     initial_info->pic_height,
+				     vpu_fmt->v4l2_frmsize);
+
+		vpu_fmt = wave5_find_vpu_fmt(inst->dst_fmt.pixelformat,
+					     dec_fmt_list[VPU_FMT_TYPE_RAW]);
+		if (!vpu_fmt)
+			return -EINVAL;
+
+		wave5_update_pix_fmt(&inst->dst_fmt,
+				     VPU_FMT_TYPE_RAW,
+				     initial_info->pic_width,
+				     initial_info->pic_height,
+				     vpu_fmt->v4l2_frmsize);
 	}
 
 	v4l2_event_queue_fh(fh, &vpu_event_src_ch);
@@ -548,12 +480,12 @@ static int wave5_vpu_dec_enum_framesizes(struct file *f, void *fh, struct v4l2_f
 	}
 
 	fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
-	fsize->stepwise.min_width = vpu_fmt->min_width;
-	fsize->stepwise.max_width = vpu_fmt->max_width;
-	fsize->stepwise.step_width = 1;
-	fsize->stepwise.min_height = vpu_fmt->min_height;
-	fsize->stepwise.max_height = vpu_fmt->max_height;
-	fsize->stepwise.step_height = 1;
+	fsize->stepwise.min_width = vpu_fmt->v4l2_frmsize->min_width;
+	fsize->stepwise.max_width = vpu_fmt->v4l2_frmsize->max_width;
+	fsize->stepwise.step_width = W5_DEC_CODEC_STEP_WIDTH;
+	fsize->stepwise.min_height = vpu_fmt->v4l2_frmsize->min_height;
+	fsize->stepwise.max_height = vpu_fmt->v4l2_frmsize->max_height;
+	fsize->stepwise.step_height = W5_DEC_CODEC_STEP_HEIGHT;
 
 	return 0;
 }
@@ -576,6 +508,7 @@ static int wave5_vpu_dec_try_fmt_cap(struct file *file, void *fh, struct v4l2_fo
 {
 	struct vpu_instance *inst = wave5_to_vpu_inst(fh);
 	struct dec_info *p_dec_info = &inst->codec_info->dec_info;
+	const struct v4l2_frmsize_stepwise *frmsize;
 	const struct vpu_format *vpu_fmt;
 	int width, height;
 
@@ -589,14 +522,12 @@ static int wave5_vpu_dec_try_fmt_cap(struct file *file, void *fh, struct v4l2_fo
 		width = inst->dst_fmt.width;
 		height = inst->dst_fmt.height;
 		f->fmt.pix_mp.pixelformat = inst->dst_fmt.pixelformat;
-		f->fmt.pix_mp.num_planes = inst->dst_fmt.num_planes;
+		frmsize = &dec_raw_frmsize;
 	} else {
-		const struct v4l2_format_info *info = v4l2_format_info(vpu_fmt->v4l2_pix_fmt);
-
-		width = clamp(f->fmt.pix_mp.width, vpu_fmt->min_width, vpu_fmt->max_width);
-		height = clamp(f->fmt.pix_mp.height, vpu_fmt->min_height, vpu_fmt->max_height);
+		width = f->fmt.pix_mp.width;
+		height = f->fmt.pix_mp.height;
 		f->fmt.pix_mp.pixelformat = vpu_fmt->v4l2_pix_fmt;
-		f->fmt.pix_mp.num_planes = info->mem_planes;
+		frmsize = vpu_fmt->v4l2_frmsize;
 	}
 
 	if (p_dec_info->initial_info_obtained) {
@@ -604,9 +535,8 @@ static int wave5_vpu_dec_try_fmt_cap(struct file *file, void *fh, struct v4l2_fo
 		height = inst->dst_fmt.height;
 	}
 
-	wave5_update_pix_fmt(&f->fmt.pix_mp, width, height);
-	f->fmt.pix_mp.flags = 0;
-	f->fmt.pix_mp.field = V4L2_FIELD_NONE;
+	wave5_update_pix_fmt(&f->fmt.pix_mp, VPU_FMT_TYPE_RAW,
+			     width, height, frmsize);
 	f->fmt.pix_mp.colorspace = inst->colorspace;
 	f->fmt.pix_mp.ycbcr_enc = inst->ycbcr_enc;
 	f->fmt.pix_mp.quantization = inst->quantization;
@@ -718,7 +648,9 @@ static int wave5_vpu_dec_enum_fmt_out(struct file *file, void *fh, struct v4l2_f
 static int wave5_vpu_dec_try_fmt_out(struct file *file, void *fh, struct v4l2_format *f)
 {
 	struct vpu_instance *inst = wave5_to_vpu_inst(fh);
+	const struct v4l2_frmsize_stepwise *frmsize;
 	const struct vpu_format *vpu_fmt;
+	int width, height;
 
 	dev_dbg(inst->dev->dev,
 		"%s: fourcc: %u width: %u height: %u num_planes: %u colorspace: %u field: %u\n",
@@ -727,20 +659,19 @@ static int wave5_vpu_dec_try_fmt_out(struct file *file, void *fh, struct v4l2_fo
 
 	vpu_fmt = wave5_find_vpu_fmt(f->fmt.pix_mp.pixelformat, dec_fmt_list[VPU_FMT_TYPE_CODEC]);
 	if (!vpu_fmt) {
+		width = inst->src_fmt.width;
+		height = inst->src_fmt.height;
 		f->fmt.pix_mp.pixelformat = inst->src_fmt.pixelformat;
-		f->fmt.pix_mp.num_planes = inst->src_fmt.num_planes;
-		wave5_update_pix_fmt(&f->fmt.pix_mp, inst->src_fmt.width, inst->src_fmt.height);
+		frmsize = &dec_hevc_frmsize;
 	} else {
-		int width = clamp(f->fmt.pix_mp.width, vpu_fmt->min_width, vpu_fmt->max_width);
-		int height = clamp(f->fmt.pix_mp.height, vpu_fmt->min_height, vpu_fmt->max_height);
-
+		width = f->fmt.pix_mp.width;
+		height = f->fmt.pix_mp.height;
 		f->fmt.pix_mp.pixelformat = vpu_fmt->v4l2_pix_fmt;
-		f->fmt.pix_mp.num_planes = 1;
-		wave5_update_pix_fmt(&f->fmt.pix_mp, width, height);
+		frmsize = vpu_fmt->v4l2_frmsize;
 	}
 
-	f->fmt.pix_mp.flags = 0;
-	f->fmt.pix_mp.field = V4L2_FIELD_NONE;
+	wave5_update_pix_fmt(&f->fmt.pix_mp, VPU_FMT_TYPE_CODEC,
+			     width, height, frmsize);
 
 	return 0;
 }
@@ -748,6 +679,7 @@ static int wave5_vpu_dec_try_fmt_out(struct file *file, void *fh, struct v4l2_fo
 static int wave5_vpu_dec_s_fmt_out(struct file *file, void *fh, struct v4l2_format *f)
 {
 	struct vpu_instance *inst = wave5_to_vpu_inst(fh);
+	const struct vpu_format *vpu_fmt;
 	int i, ret;
 
 	dev_dbg(inst->dev->dev,
@@ -782,7 +714,13 @@ static int wave5_vpu_dec_s_fmt_out(struct file *file, void *fh, struct v4l2_form
 	inst->quantization = f->fmt.pix_mp.quantization;
 	inst->xfer_func = f->fmt.pix_mp.xfer_func;
 
-	wave5_update_pix_fmt(&inst->dst_fmt, f->fmt.pix_mp.width, f->fmt.pix_mp.height);
+	vpu_fmt = wave5_find_vpu_fmt(inst->dst_fmt.pixelformat, dec_fmt_list[VPU_FMT_TYPE_RAW]);
+	if (!vpu_fmt)
+		return -EINVAL;
+
+	wave5_update_pix_fmt(&inst->dst_fmt, VPU_FMT_TYPE_RAW,
+			     f->fmt.pix_mp.width, f->fmt.pix_mp.height,
+			     vpu_fmt->v4l2_frmsize);
 
 	return 0;
 }
@@ -1005,6 +943,7 @@ static int wave5_vpu_dec_queue_setup(struct vb2_queue *q, unsigned int *num_buff
 	struct vpu_instance *inst = vb2_get_drv_priv(q);
 	struct v4l2_pix_format_mplane inst_format =
 		(q->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) ? inst->src_fmt : inst->dst_fmt;
+	unsigned int i;
 
 	dev_dbg(inst->dev->dev, "%s: num_buffers: %u | num_planes: %u | type: %u\n", __func__,
 		*num_buffers, *num_planes, q->type);
@@ -1018,31 +957,9 @@ static int wave5_vpu_dec_queue_setup(struct vb2_queue *q, unsigned int *num_buff
 		if (*num_buffers < inst->fbc_buf_count)
 			*num_buffers = inst->fbc_buf_count;
 
-		if (*num_planes == 1) {
-			if (inst->output_format == FORMAT_422)
-				sizes[0] = inst_format.width * inst_format.height * 2;
-			else
-				sizes[0] = inst_format.width * inst_format.height * 3 / 2;
-			dev_dbg(inst->dev->dev, "%s: size[0]: %u\n", __func__, sizes[0]);
-		} else if (*num_planes == 2) {
-			sizes[0] = inst_format.width * inst_format.height;
-			if (inst->output_format == FORMAT_422)
-				sizes[1] = inst_format.width * inst_format.height;
-			else
-				sizes[1] = inst_format.width * inst_format.height / 2;
-			dev_dbg(inst->dev->dev, "%s: size[0]: %u | size[1]: %u\n",
-				__func__, sizes[0], sizes[1]);
-		} else if (*num_planes == 3) {
-			sizes[0] = inst_format.width * inst_format.height;
-			if (inst->output_format == FORMAT_422) {
-				sizes[1] = inst_format.width * inst_format.height / 2;
-				sizes[2] = inst_format.width * inst_format.height / 2;
-			} else {
-				sizes[1] = inst_format.width * inst_format.height / 4;
-				sizes[2] = inst_format.width * inst_format.height / 4;
-			}
-			dev_dbg(inst->dev->dev, "%s: size[0]: %u | size[1]: %u | size[2]: %u\n",
-				__func__, sizes[0], sizes[1], sizes[2]);
+		for (i = 0; i < *num_planes; i++) {
+			sizes[i] = inst_format.plane_fmt[i].sizeimage;
+			dev_dbg(inst->dev->dev, "%s: size[%u]: %u\n", __func__, i, sizes[i]);
 		}
 	}
 
@@ -1564,20 +1481,15 @@ static const struct vb2_ops wave5_vpu_dec_vb2_ops = {
 static void wave5_set_default_format(struct v4l2_pix_format_mplane *src_fmt,
 				     struct v4l2_pix_format_mplane *dst_fmt)
 {
-	unsigned int dst_pix_fmt = dec_fmt_list[VPU_FMT_TYPE_RAW][0].v4l2_pix_fmt;
-	const struct v4l2_format_info *dst_fmt_info = v4l2_format_info(dst_pix_fmt);
-
 	src_fmt->pixelformat = dec_fmt_list[VPU_FMT_TYPE_CODEC][0].v4l2_pix_fmt;
-	src_fmt->field = V4L2_FIELD_NONE;
-	src_fmt->flags = 0;
-	src_fmt->num_planes = 1;
-	wave5_update_pix_fmt(src_fmt, 720, 480);
-
-	dst_fmt->pixelformat = dst_pix_fmt;
-	dst_fmt->field = V4L2_FIELD_NONE;
-	dst_fmt->flags = 0;
-	dst_fmt->num_planes = dst_fmt_info->mem_planes;
-	wave5_update_pix_fmt(dst_fmt, 736, 480);
+	wave5_update_pix_fmt(src_fmt, VPU_FMT_TYPE_CODEC,
+			     W5_DEF_DEC_PIC_WIDTH, W5_DEF_DEC_PIC_HEIGHT,
+			     &dec_hevc_frmsize);
+
+	dst_fmt->pixelformat = dec_fmt_list[VPU_FMT_TYPE_RAW][0].v4l2_pix_fmt;
+	wave5_update_pix_fmt(dst_fmt, VPU_FMT_TYPE_RAW,
+			     W5_DEF_DEC_PIC_WIDTH, W5_DEF_DEC_PIC_HEIGHT,
+			     &dec_raw_frmsize);
 }
 
 static int wave5_vpu_dec_queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
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 703fd8d1c7da..a470f24cbabe 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
@@ -11,65 +11,60 @@
 #define VPU_ENC_DEV_NAME "C&M Wave5 VPU encoder"
 #define VPU_ENC_DRV_NAME "wave5-enc"
 
+static const struct v4l2_frmsize_stepwise enc_frmsize[FMT_TYPES] = {
+	[VPU_FMT_TYPE_CODEC] = {
+		.min_width = W5_MIN_ENC_PIC_WIDTH,
+		.max_width = W5_MAX_ENC_PIC_WIDTH,
+		.step_width = W5_ENC_CODEC_STEP_WIDTH,
+		.min_height = W5_MIN_ENC_PIC_HEIGHT,
+		.max_height = W5_MAX_ENC_PIC_HEIGHT,
+		.step_height = W5_ENC_CODEC_STEP_HEIGHT,
+	},
+	[VPU_FMT_TYPE_RAW] = {
+		.min_width = W5_MIN_ENC_PIC_WIDTH,
+		.max_width = W5_MAX_ENC_PIC_WIDTH,
+		.step_width = W5_ENC_RAW_STEP_WIDTH,
+		.min_height = W5_MIN_ENC_PIC_HEIGHT,
+		.max_height = W5_MAX_ENC_PIC_HEIGHT,
+		.step_height = W5_ENC_RAW_STEP_HEIGHT,
+	},
+};
+
 static const struct vpu_format enc_fmt_list[FMT_TYPES][MAX_FMTS] = {
 	[VPU_FMT_TYPE_CODEC] = {
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_HEVC,
-			.max_width = W5_MAX_ENC_PIC_WIDTH,
-			.min_width = W5_MIN_ENC_PIC_WIDTH,
-			.max_height = W5_MAX_ENC_PIC_HEIGHT,
-			.min_height = W5_MIN_ENC_PIC_HEIGHT,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_CODEC],
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_H264,
-			.max_width = W5_MAX_ENC_PIC_WIDTH,
-			.min_width = W5_MIN_ENC_PIC_WIDTH,
-			.max_height = W5_MAX_ENC_PIC_HEIGHT,
-			.min_height = W5_MIN_ENC_PIC_HEIGHT,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_CODEC],
 		},
 	},
 	[VPU_FMT_TYPE_RAW] = {
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_YUV420,
-			.max_width = W5_MAX_ENC_PIC_WIDTH,
-			.min_width = W5_MIN_ENC_PIC_WIDTH,
-			.max_height = W5_MAX_ENC_PIC_HEIGHT,
-			.min_height = W5_MIN_ENC_PIC_HEIGHT,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV12,
-			.max_width = W5_MAX_ENC_PIC_WIDTH,
-			.min_width = W5_MIN_ENC_PIC_WIDTH,
-			.max_height = W5_MAX_ENC_PIC_HEIGHT,
-			.min_height = W5_MIN_ENC_PIC_HEIGHT,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV21,
-			.max_width = W5_MAX_ENC_PIC_WIDTH,
-			.min_width = W5_MIN_ENC_PIC_WIDTH,
-			.max_height = W5_MAX_ENC_PIC_HEIGHT,
-			.min_height = W5_MIN_ENC_PIC_HEIGHT,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_YUV420M,
-			.max_width = W5_MAX_ENC_PIC_WIDTH,
-			.min_width = W5_MIN_ENC_PIC_WIDTH,
-			.max_height = W5_MAX_ENC_PIC_HEIGHT,
-			.min_height = W5_MIN_ENC_PIC_HEIGHT,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV12M,
-			.max_width = W5_MAX_ENC_PIC_WIDTH,
-			.min_width = W5_MIN_ENC_PIC_WIDTH,
-			.max_height = W5_MAX_ENC_PIC_HEIGHT,
-			.min_height = W5_MIN_ENC_PIC_HEIGHT,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
 		},
 		{
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV21M,
-			.max_width = W5_MAX_ENC_PIC_WIDTH,
-			.min_width = W5_MIN_ENC_PIC_WIDTH,
-			.max_height = W5_MAX_ENC_PIC_HEIGHT,
-			.min_height = W5_MIN_ENC_PIC_HEIGHT,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
 		},
 	}
 };
@@ -106,46 +101,6 @@ static int switch_state(struct vpu_instance *inst, enum vpu_instance_state state
 	return -EINVAL;
 }
 
-static void wave5_update_pix_fmt(struct v4l2_pix_format_mplane *pix_mp, unsigned int width,
-				 unsigned int height)
-{
-	switch (pix_mp->pixelformat) {
-	case V4L2_PIX_FMT_YUV420:
-	case V4L2_PIX_FMT_NV12:
-	case V4L2_PIX_FMT_NV21:
-		pix_mp->width = width;
-		pix_mp->height = height;
-		pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
-		pix_mp->plane_fmt[0].sizeimage = round_up(width, 32) * height * 3 / 2;
-		break;
-	case V4L2_PIX_FMT_YUV420M:
-		pix_mp->width = width;
-		pix_mp->height = height;
-		pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
-		pix_mp->plane_fmt[0].sizeimage = round_up(width, 32) * height;
-		pix_mp->plane_fmt[1].bytesperline = round_up(width, 32) / 2;
-		pix_mp->plane_fmt[1].sizeimage = round_up(width, 32) * height / 4;
-		pix_mp->plane_fmt[2].bytesperline = round_up(width, 32) / 2;
-		pix_mp->plane_fmt[2].sizeimage = round_up(width, 32) * height / 4;
-		break;
-	case V4L2_PIX_FMT_NV12M:
-	case V4L2_PIX_FMT_NV21M:
-		pix_mp->width = width;
-		pix_mp->height = height;
-		pix_mp->plane_fmt[0].bytesperline = round_up(width, 32);
-		pix_mp->plane_fmt[0].sizeimage = round_up(width, 32) * height;
-		pix_mp->plane_fmt[1].bytesperline = round_up(width, 32);
-		pix_mp->plane_fmt[1].sizeimage = round_up(width, 32) * height / 2;
-		break;
-	default:
-		pix_mp->width = width;
-		pix_mp->height = height;
-		pix_mp->plane_fmt[0].bytesperline = 0;
-		pix_mp->plane_fmt[0].sizeimage = width * height / 8 * 3;
-		break;
-	}
-}
-
 static int start_encode(struct vpu_instance *inst, u32 *fail_res)
 {
 	struct v4l2_m2m_ctx *m2m_ctx = inst->v4l2_fh.m2m_ctx;
@@ -360,13 +315,8 @@ static int wave5_vpu_enc_enum_framesizes(struct file *f, void *fh, struct v4l2_f
 			return -EINVAL;
 	}
 
-	fsize->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
-	fsize->stepwise.min_width = vpu_fmt->min_width;
-	fsize->stepwise.max_width = vpu_fmt->max_width;
-	fsize->stepwise.step_width = 1;
-	fsize->stepwise.min_height = vpu_fmt->min_height;
-	fsize->stepwise.max_height = vpu_fmt->max_height;
-	fsize->stepwise.step_height = 1;
+	fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
+	fsize->stepwise = enc_frmsize[VPU_FMT_TYPE_CODEC];
 
 	return 0;
 }
@@ -391,7 +341,9 @@ static int wave5_vpu_enc_enum_fmt_cap(struct file *file, void *fh, struct v4l2_f
 static int wave5_vpu_enc_try_fmt_cap(struct file *file, void *fh, struct v4l2_format *f)
 {
 	struct vpu_instance *inst = wave5_to_vpu_inst(fh);
+	const struct v4l2_frmsize_stepwise *frmsize;
 	const struct vpu_format *vpu_fmt;
+	int width, height;
 
 	dev_dbg(inst->dev->dev, "%s: fourcc: %u width: %u height: %u num_planes: %u field: %u\n",
 		__func__, f->fmt.pix_mp.pixelformat, f->fmt.pix_mp.width, f->fmt.pix_mp.height,
@@ -399,20 +351,19 @@ static int wave5_vpu_enc_try_fmt_cap(struct file *file, void *fh, struct v4l2_fo
 
 	vpu_fmt = wave5_find_vpu_fmt(f->fmt.pix_mp.pixelformat, enc_fmt_list[VPU_FMT_TYPE_CODEC]);
 	if (!vpu_fmt) {
+		width = inst->dst_fmt.width;
+		height = inst->dst_fmt.height;
 		f->fmt.pix_mp.pixelformat = inst->dst_fmt.pixelformat;
-		f->fmt.pix_mp.num_planes = inst->dst_fmt.num_planes;
-		wave5_update_pix_fmt(&f->fmt.pix_mp, inst->dst_fmt.width, inst->dst_fmt.height);
+		frmsize = &enc_frmsize[VPU_FMT_TYPE_CODEC];
 	} else {
-		int width = clamp(f->fmt.pix_mp.width, vpu_fmt->min_width, vpu_fmt->max_width);
-		int height = clamp(f->fmt.pix_mp.height, vpu_fmt->min_height, vpu_fmt->max_height);
-
+		width = f->fmt.pix_mp.width;
+		height = f->fmt.pix_mp.height;
 		f->fmt.pix_mp.pixelformat = vpu_fmt->v4l2_pix_fmt;
-		f->fmt.pix_mp.num_planes = 1;
-		wave5_update_pix_fmt(&f->fmt.pix_mp, width, height);
+		frmsize = vpu_fmt->v4l2_frmsize;
 	}
 
-	f->fmt.pix_mp.flags = 0;
-	f->fmt.pix_mp.field = V4L2_FIELD_NONE;
+	wave5_update_pix_fmt(&f->fmt.pix_mp, VPU_FMT_TYPE_CODEC,
+			     width, height, frmsize);
 	f->fmt.pix_mp.colorspace = inst->colorspace;
 	f->fmt.pix_mp.ycbcr_enc = inst->ycbcr_enc;
 	f->fmt.pix_mp.quantization = inst->quantization;
@@ -499,7 +450,9 @@ static int wave5_vpu_enc_enum_fmt_out(struct file *file, void *fh, struct v4l2_f
 static int wave5_vpu_enc_try_fmt_out(struct file *file, void *fh, struct v4l2_format *f)
 {
 	struct vpu_instance *inst = wave5_to_vpu_inst(fh);
+	const struct v4l2_frmsize_stepwise *frmsize;
 	const struct vpu_format *vpu_fmt;
+	int width, height;
 
 	dev_dbg(inst->dev->dev, "%s: fourcc: %u width: %u height: %u num_planes: %u field: %u\n",
 		__func__, f->fmt.pix_mp.pixelformat, f->fmt.pix_mp.width, f->fmt.pix_mp.height,
@@ -507,28 +460,26 @@ static int wave5_vpu_enc_try_fmt_out(struct file *file, void *fh, struct v4l2_fo
 
 	vpu_fmt = wave5_find_vpu_fmt(f->fmt.pix_mp.pixelformat, enc_fmt_list[VPU_FMT_TYPE_RAW]);
 	if (!vpu_fmt) {
+		width = inst->src_fmt.width;
+		height = inst->src_fmt.height;
 		f->fmt.pix_mp.pixelformat = inst->src_fmt.pixelformat;
-		f->fmt.pix_mp.num_planes = inst->src_fmt.num_planes;
-		wave5_update_pix_fmt(&f->fmt.pix_mp, inst->src_fmt.width, inst->src_fmt.height);
+		frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW];
 	} else {
-		int width = clamp(f->fmt.pix_mp.width, vpu_fmt->min_width, vpu_fmt->max_width);
-		int height = clamp(f->fmt.pix_mp.height, vpu_fmt->min_height, vpu_fmt->max_height);
-		const struct v4l2_format_info *info = v4l2_format_info(vpu_fmt->v4l2_pix_fmt);
-
+		width = f->fmt.pix_mp.width;
+		height = f->fmt.pix_mp.height;
 		f->fmt.pix_mp.pixelformat = vpu_fmt->v4l2_pix_fmt;
-		f->fmt.pix_mp.num_planes = info->mem_planes;
-		wave5_update_pix_fmt(&f->fmt.pix_mp, width, height);
+		frmsize = vpu_fmt->v4l2_frmsize;
 	}
 
-	f->fmt.pix_mp.flags = 0;
-	f->fmt.pix_mp.field = V4L2_FIELD_NONE;
-
+	wave5_update_pix_fmt(&f->fmt.pix_mp, VPU_FMT_TYPE_RAW,
+			     width, height, frmsize);
 	return 0;
 }
 
 static int wave5_vpu_enc_s_fmt_out(struct file *file, void *fh, struct v4l2_format *f)
 {
 	struct vpu_instance *inst = wave5_to_vpu_inst(fh);
+	const struct vpu_format *vpu_fmt;
 	int i, ret;
 
 	dev_dbg(inst->dev->dev, "%s: fourcc: %u width: %u height: %u num_planes: %u field: %u\n",
@@ -568,7 +519,15 @@ static int wave5_vpu_enc_s_fmt_out(struct file *file, void *fh, struct v4l2_form
 	inst->quantization = f->fmt.pix_mp.quantization;
 	inst->xfer_func = f->fmt.pix_mp.xfer_func;
 
-	wave5_update_pix_fmt(&inst->dst_fmt, f->fmt.pix_mp.width, f->fmt.pix_mp.height);
+	vpu_fmt = wave5_find_vpu_fmt(inst->dst_fmt.pixelformat, enc_fmt_list[VPU_FMT_TYPE_CODEC]);
+	if (!vpu_fmt)
+		return -EINVAL;
+
+	wave5_update_pix_fmt(&inst->dst_fmt, VPU_FMT_TYPE_CODEC,
+			     f->fmt.pix_mp.width, f->fmt.pix_mp.height,
+			     vpu_fmt->v4l2_frmsize);
+	inst->conf_win.width = inst->dst_fmt.width;
+	inst->conf_win.height = inst->dst_fmt.height;
 
 	return 0;
 }
@@ -584,12 +543,17 @@ static int wave5_vpu_enc_g_selection(struct file *file, void *fh, struct v4l2_se
 	switch (s->target) {
 	case V4L2_SEL_TGT_CROP_DEFAULT:
 	case V4L2_SEL_TGT_CROP_BOUNDS:
-	case V4L2_SEL_TGT_CROP:
 		s->r.left = 0;
 		s->r.top = 0;
 		s->r.width = inst->dst_fmt.width;
 		s->r.height = inst->dst_fmt.height;
 		break;
+	case V4L2_SEL_TGT_CROP:
+		s->r.left = 0;
+		s->r.top = 0;
+		s->r.width = inst->conf_win.width;
+		s->r.height = inst->conf_win.height;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -612,8 +576,10 @@ static int wave5_vpu_enc_s_selection(struct file *file, void *fh, struct v4l2_se
 
 	s->r.left = 0;
 	s->r.top = 0;
-	s->r.width = inst->src_fmt.width;
-	s->r.height = inst->src_fmt.height;
+	s->r.width = min(s->r.width, inst->dst_fmt.width);
+	s->r.height = min(s->r.height, inst->dst_fmt.height);
+
+	inst->conf_win = s->r;
 
 	return 0;
 }
@@ -1151,8 +1117,8 @@ static void wave5_set_enc_openparam(struct enc_open_param *open_param,
 	open_param->wave_param.lambda_scaling_enable = 1;
 
 	open_param->line_buf_int_en = true;
-	open_param->pic_width = inst->dst_fmt.width;
-	open_param->pic_height = inst->dst_fmt.height;
+	open_param->pic_width = inst->conf_win.width;
+	open_param->pic_height = inst->conf_win.height;
 	open_param->frame_rate_info = inst->frame_rate;
 	open_param->rc_enable = inst->rc_enable;
 	if (inst->rc_enable) {
@@ -1456,20 +1422,15 @@ static const struct vb2_ops wave5_vpu_enc_vb2_ops = {
 static void wave5_set_default_format(struct v4l2_pix_format_mplane *src_fmt,
 				     struct v4l2_pix_format_mplane *dst_fmt)
 {
-	unsigned int src_pix_fmt = enc_fmt_list[VPU_FMT_TYPE_RAW][0].v4l2_pix_fmt;
-	const struct v4l2_format_info *src_fmt_info = v4l2_format_info(src_pix_fmt);
-
-	src_fmt->pixelformat = src_pix_fmt;
-	src_fmt->field = V4L2_FIELD_NONE;
-	src_fmt->flags = 0;
-	src_fmt->num_planes = src_fmt_info->mem_planes;
-	wave5_update_pix_fmt(src_fmt, 416, 240);
+	src_fmt->pixelformat = enc_fmt_list[VPU_FMT_TYPE_RAW][0].v4l2_pix_fmt;
+	wave5_update_pix_fmt(src_fmt, VPU_FMT_TYPE_RAW,
+			     W5_DEF_ENC_PIC_WIDTH, W5_DEF_ENC_PIC_HEIGHT,
+			     &enc_frmsize[VPU_FMT_TYPE_RAW]);
 
 	dst_fmt->pixelformat = enc_fmt_list[VPU_FMT_TYPE_CODEC][0].v4l2_pix_fmt;
-	dst_fmt->field = V4L2_FIELD_NONE;
-	dst_fmt->flags = 0;
-	dst_fmt->num_planes = 1;
-	wave5_update_pix_fmt(dst_fmt, 416, 240);
+	wave5_update_pix_fmt(dst_fmt, VPU_FMT_TYPE_CODEC,
+			     W5_DEF_ENC_PIC_WIDTH, W5_DEF_ENC_PIC_HEIGHT,
+			     &enc_frmsize[VPU_FMT_TYPE_CODEC]);
 }
 
 static int wave5_vpu_enc_queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *dst_vq)
@@ -1733,6 +1694,8 @@ static int wave5_vpu_open_enc(struct file *filp)
 	v4l2_ctrl_handler_setup(v4l2_ctrl_hdl);
 
 	wave5_set_default_format(&inst->src_fmt, &inst->dst_fmt);
+	inst->conf_win.width = inst->dst_fmt.width;
+	inst->conf_win.height = inst->dst_fmt.height;
 	inst->colorspace = V4L2_COLORSPACE_REC709;
 	inst->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
 	inst->quantization = V4L2_QUANTIZATION_DEFAULT;
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu.h b/drivers/media/platform/chips-media/wave5/wave5-vpu.h
index 32b7fd3730b5..3847332551fc 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu.h
@@ -38,10 +38,7 @@ enum vpu_fmt_type {
 
 struct vpu_format {
 	unsigned int v4l2_pix_fmt;
-	unsigned int max_width;
-	unsigned int min_width;
-	unsigned int max_height;
-	unsigned int min_height;
+	const struct v4l2_frmsize_stepwise *v4l2_frmsize;
 };
 
 static inline struct vpu_instance *wave5_to_vpu_inst(struct v4l2_fh *vfh)
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h b/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
index d9751eedb0f9..8e11d93ca38f 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
@@ -30,10 +30,29 @@
 
 #define MAX_NUM_INSTANCE                32
 
-#define W5_MIN_ENC_PIC_WIDTH            256
-#define W5_MIN_ENC_PIC_HEIGHT           128
-#define W5_MAX_ENC_PIC_WIDTH            8192
-#define W5_MAX_ENC_PIC_HEIGHT           8192
+#define W5_DEF_DEC_PIC_WIDTH            720U
+#define W5_DEF_DEC_PIC_HEIGHT           480U
+#define W5_MIN_DEC_PIC_8_WIDTH          8U
+#define W5_MIN_DEC_PIC_8_HEIGHT         8U
+#define W5_MIN_DEC_PIC_32_WIDTH         32U
+#define W5_MIN_DEC_PIC_32_HEIGHT        32U
+#define W5_MAX_DEC_PIC_WIDTH            8192U
+#define W5_MAX_DEC_PIC_HEIGHT           4320U
+#define W5_DEC_CODEC_STEP_WIDTH         1U
+#define W5_DEC_CODEC_STEP_HEIGHT        1U
+#define W5_DEC_RAW_STEP_WIDTH           32U
+#define W5_DEC_RAW_STEP_HEIGHT          16U
+
+#define W5_DEF_ENC_PIC_WIDTH            416U
+#define W5_DEF_ENC_PIC_HEIGHT           240U
+#define W5_MIN_ENC_PIC_WIDTH            256U
+#define W5_MIN_ENC_PIC_HEIGHT           128U
+#define W5_MAX_ENC_PIC_WIDTH            8192U
+#define W5_MAX_ENC_PIC_HEIGHT           8192U
+#define W5_ENC_CODEC_STEP_WIDTH         8U
+#define W5_ENC_CODEC_STEP_HEIGHT        8U
+#define W5_ENC_RAW_STEP_WIDTH           32U
+#define W5_ENC_RAW_STEP_HEIGHT          16U
 
 //  application specific configuration
 #define VPU_ENC_TIMEOUT                 60000
-- 
2.43.0

[RESEND PATCH v6 4/4] media: chips-media: wave5: Support YUV422 raw pixel-formats on the encoder.

[Jackson.lee]

From: "jackson.lee" <jackson.lee@chipsnmedia.com>

Add support for the YUV422P, NV16, NV61, YUV422M, NV16M,
NV61M raw pixel-formats to the Wave5 encoder.

All these formats have a chroma subsampling ratio of 4:2:2 and
therefore require a new image size calculation as the driver
previously only handled a ratio of 4:2:0.

Signed-off-by: Jackson.lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
Reviewed-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 .../chips-media/wave5/wave5-vpu-enc.c         | 89 +++++++++++++++----
 1 file changed, 74 insertions(+), 15 deletions(-)

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 a470f24cbabe..fee24b427fd1 100644
--- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
@@ -66,6 +66,30 @@ static const struct vpu_format enc_fmt_list[FMT_TYPES][MAX_FMTS] = {
 			.v4l2_pix_fmt = V4L2_PIX_FMT_NV21M,
 			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
 		},
+		{
+			.v4l2_pix_fmt = V4L2_PIX_FMT_YUV422P,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
+		},
+		{
+			.v4l2_pix_fmt = V4L2_PIX_FMT_NV16,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
+		},
+		{
+			.v4l2_pix_fmt = V4L2_PIX_FMT_NV61,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
+		},
+		{
+			.v4l2_pix_fmt = V4L2_PIX_FMT_YUV422M,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
+		},
+		{
+			.v4l2_pix_fmt = V4L2_PIX_FMT_NV16M,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
+		},
+		{
+			.v4l2_pix_fmt = V4L2_PIX_FMT_NV61M,
+			.v4l2_frmsize = &enc_frmsize[VPU_FMT_TYPE_RAW],
+		},
 	}
 };
 
@@ -109,13 +133,26 @@ static int start_encode(struct vpu_instance *inst, u32 *fail_res)
 	struct vb2_v4l2_buffer *dst_buf;
 	struct frame_buffer frame_buf;
 	struct enc_param pic_param;
-	u32 stride = ALIGN(inst->dst_fmt.width, 32);
-	u32 luma_size = (stride * inst->dst_fmt.height);
-	u32 chroma_size = ((stride / 2) * (inst->dst_fmt.height / 2));
+	const struct v4l2_format_info *info;
+	u32 stride = inst->src_fmt.plane_fmt[0].bytesperline;
+	u32 luma_size = 0;
+	u32 chroma_size = 0;
 
 	memset(&pic_param, 0, sizeof(struct enc_param));
 	memset(&frame_buf, 0, sizeof(struct frame_buffer));
 
+	info = v4l2_format_info(inst->src_fmt.pixelformat);
+	if (!info)
+		return -EINVAL;
+
+	if (info->mem_planes == 1) {
+		luma_size = stride * inst->dst_fmt.height;
+		chroma_size = luma_size / (info->hdiv * info->vdiv);
+	} else {
+		luma_size = inst->src_fmt.plane_fmt[0].sizeimage;
+		chroma_size = inst->src_fmt.plane_fmt[1].sizeimage;
+	}
+
 	dst_buf = v4l2_m2m_next_dst_buf(m2m_ctx);
 	if (!dst_buf) {
 		dev_dbg(inst->dev->dev, "%s: No destination buffer found\n", __func__);
@@ -480,6 +517,7 @@ static int wave5_vpu_enc_s_fmt_out(struct file *file, void *fh, struct v4l2_form
 {
 	struct vpu_instance *inst = wave5_to_vpu_inst(fh);
 	const struct vpu_format *vpu_fmt;
+	const struct v4l2_format_info *info;
 	int i, ret;
 
 	dev_dbg(inst->dev->dev, "%s: fourcc: %u width: %u height: %u num_planes: %u field: %u\n",
@@ -501,16 +539,20 @@ static int wave5_vpu_enc_s_fmt_out(struct file *file, void *fh, struct v4l2_form
 		inst->src_fmt.plane_fmt[i].sizeimage = f->fmt.pix_mp.plane_fmt[i].sizeimage;
 	}
 
-	if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12 ||
-	    inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV12M) {
-		inst->cbcr_interleave = true;
-		inst->nv21 = false;
-	} else if (inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21 ||
-		   inst->src_fmt.pixelformat == V4L2_PIX_FMT_NV21M) {
-		inst->cbcr_interleave = true;
+	info = v4l2_format_info(inst->src_fmt.pixelformat);
+	if (!info)
+		return -EINVAL;
+
+	inst->cbcr_interleave = (info->comp_planes == 2) ? true : false;
+
+	switch (inst->src_fmt.pixelformat) {
+	case V4L2_PIX_FMT_NV21:
+	case V4L2_PIX_FMT_NV21M:
+	case V4L2_PIX_FMT_NV61:
+	case V4L2_PIX_FMT_NV61M:
 		inst->nv21 = true;
-	} else {
-		inst->cbcr_interleave = false;
+		break;
+	default:
 		inst->nv21 = false;
 	}
 
@@ -1095,13 +1137,23 @@ static void wave5_vpu_enc_buf_queue(struct vb2_buffer *vb)
 	v4l2_m2m_buf_queue(m2m_ctx, vbuf);
 }
 
-static void wave5_set_enc_openparam(struct enc_open_param *open_param,
-				    struct vpu_instance *inst)
+static int wave5_set_enc_openparam(struct enc_open_param *open_param,
+				   struct vpu_instance *inst)
 {
 	struct enc_wave_param input = inst->enc_param;
+	const struct v4l2_format_info *info;
 	u32 num_ctu_row = ALIGN(inst->dst_fmt.height, 64) / 64;
 	u32 num_mb_row = ALIGN(inst->dst_fmt.height, 16) / 16;
 
+	info = v4l2_format_info(inst->src_fmt.pixelformat);
+	if (!info)
+		return -EINVAL;
+
+	if (info->hdiv == 2 && info->vdiv == 1)
+		open_param->src_format = FORMAT_422;
+	else
+		open_param->src_format = FORMAT_420;
+
 	open_param->wave_param.gop_preset_idx = PRESET_IDX_IPP_SINGLE;
 	open_param->wave_param.hvs_qp_scale = 2;
 	open_param->wave_param.hvs_max_delta_qp = 10;
@@ -1190,6 +1242,8 @@ static void wave5_set_enc_openparam(struct enc_open_param *open_param,
 			open_param->wave_param.intra_refresh_arg = num_ctu_row;
 	}
 	open_param->wave_param.forced_idr_header_enable = input.forced_idr_header_enable;
+
+	return 0;
 }
 
 static int initialize_sequence(struct vpu_instance *inst)
@@ -1285,7 +1339,12 @@ static int wave5_vpu_enc_start_streaming(struct vb2_queue *q, unsigned int count
 
 		memset(&open_param, 0, sizeof(struct enc_open_param));
 
-		wave5_set_enc_openparam(&open_param, inst);
+		ret = wave5_set_enc_openparam(&open_param, inst);
+		if (ret) {
+			dev_dbg(inst->dev->dev, "%s: wave5_set_enc_openparam, fail: %d\n",
+				__func__, ret);
+			goto return_buffers;
+		}
 
 		ret = wave5_vpu_enc_open(inst, &open_param);
 		if (ret) {
-- 
2.43.0

Re: [RESEND PATCH v6 0/4] Add features to an existing driver

[Sebastian Fricke]

Hey Jackson,

what is up with all the resends, I can see that you send V6 two times
without a RESEND tag and once with a RESEND tag?
Was that an error on your side or did you actually change something?
Does it matter for me which version to consider or are they all the same
content-wise?

Regards,
Sebastian

On 17.06.2024 19:48, Jackson.lee wrote:
>The wave5 codec driver is a stateful encoder/decoder.
>The following patches is for supporting yuv422 inpuy format, supporting runtime suspend/resume feature and extra things.
>
>v4l2-compliance results:
>========================
>
>v4l2-compliance 1.24.1, 64 bits, 64-bit time_t
>
>Buffer ioctls:
>       warn: v4l2-test-buffers.cpp(693): VIDIOC_CREATE_BUFS not supported
>       warn: v4l2-test-buffers.cpp(693): VIDIOC_CREATE_BUFS not supported
>    test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
>    test VIDIOC_EXPBUF: OK
>    test Requests: OK (Not Supported)
>
>Total for wave5-dec device /dev/video0: 45, Succeeded: 45, Failed: 0, Warnings: 2 Total for wave5-enc device /dev/video1: 45, Succeeded: 45, Failed: 0, Warnings: 0
>
>Fluster test results:
>=====================
>
>Running test suite JCT-VC-HEVC_V1 with decoder GStreamer-H.265-V4L2-Gst1.0 Using 1 parallel job(s)
>Ran 132/147 tests successfully               in 88.745 secs
>
>(1 test fails because of not supporting to parse multi frames, 1 test fails because of a missing frame and slight corruption,
> 2 tests fail because of sizes which are incompatible with the IP, 11 tests fail because of unsupported 10 bit format)
>
>Running test suite JVT-AVC_V1 with decoder GStreamer-H.264-V4L2-Gst1.0 Using 1 parallel job(s)
>Ran 77/135 tests successfully               in 32.044 secs
>
>(58 fail because the hardware is unable to decode  MBAFF / FMO / Field / Extended profile streams.)
>
>Change since v5:
>================
>* For [PATCH v4 3/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage.
> - Fix v4l2-compliance error for the vidioc_enum_framesizes
>
>* For [PATCH v4 1/4] media: chips-media: wave5: Support SPS/PPS generation for each IDR
> - Remove warning messages for the checkpatch.pl script
>
>Change since v4:
>================
>* For [PATCH v4 2/4] media: chips-media: wave5: Support runtime suspend/resume
> - Fix warning message
>
>* For [PATCH v4 3/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage.
> - Fix warning message
> - add Reviewed-By tag
>
>* For [PATCH v4 4/4] media: chips-media: wave5: Support YUV422 raw pixel-formats on the encoder
> - add Reviewed-By tag
>
>Change since v3:
>=================
>
>* For [PATCH v4 1/4] media: chips-media: wave5: Support SPS/PPS generation for each IDR
> - add Reviewed-By tag
>
>* For [PATCH v4 2/4] media: chips-media: wave5: Support runtime suspend/resume
> - add Reviewed-By tag
>
>* For [PATCH v4 3/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage.
> - modify the commit message
> - define three framesize structures for decoder
>
>* For [PATCH v4 4/4] media: chips-media: wave5: Support YUV422 raw pixel-formats on the encoder
> - modify the commit message
> - use the v4l2_format_info to calculate luma, chroma size
>
>Change since v2:
>=================
>
>* For [PATCH v3 0/4] media: chips-media: wave5: Support SPS/PPS generation for each IDR
> - add the suggested _SHIFT suffix
>
>* For [PATCH v3 1/4] media: chips-media: wave5: Support runtime suspend/resume
> - change a commit message
>
>* For [PATCH v3 2/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage
> - add pix_fmt_type parameter into wave5_update_pix_fmt function
> - add min/max width/height values into dec_fmt_list
>
>Change since v1:
>=================
>
>* For [PATCH v2 0/4] media: chips-media: wave5: Support SPS/PPS generation for each IDR
> - define a macro for register addresses
>
>* For [PATCH v2 1/4] media: chips-media: wave5: Support runtime suspend/resume
> - add auto suspend/resume
>
>* For [PATCH v2 2/4] media: chips-media: wave5: Use helpers to calculate bytesperline and sizeimage
> - use helper functions to calculate bytesperline and sizeimage
>
>* For [PATCH v2 3/4] media: chips-media: wave5: Support YUV422 raw pixel-formats on the encoder
> - remove unnecessary codes
>
>Change since v0:
>=================
>The DEFAULT_SRC_SIZE macro was defined using multiple lines, To make a simple define, tab and multiple lines has been removed, The macro is defined using one line.
>
>
>jackson.lee (4):
>  media: chips-media: wave5: Support SPS/PPS generation for each IDR
>  media: chips-media: wave5: Support runtime suspend/resume
>  media: chips-media: wave5: Use helpers to calculate bytesperline and
>    sizeimage.
>  media: chips-media: wave5: Support YUV422 raw pixel-formats on the
>    encoder.
>
> .../platform/chips-media/wave5/wave5-helper.c |  24 ++
> .../platform/chips-media/wave5/wave5-helper.h |   5 +
> .../platform/chips-media/wave5/wave5-hw.c     |  30 +-
> .../chips-media/wave5/wave5-vpu-dec.c         | 316 +++++++-----------
> .../chips-media/wave5/wave5-vpu-enc.c         | 308 +++++++++--------
> .../platform/chips-media/wave5/wave5-vpu.c    |  43 +++
> .../platform/chips-media/wave5/wave5-vpu.h    |   5 +-
> .../platform/chips-media/wave5/wave5-vpuapi.c |  14 +-
> .../platform/chips-media/wave5/wave5-vpuapi.h |   1 +
> .../chips-media/wave5/wave5-vpuconfig.h       |  27 +-
> .../media/platform/chips-media/wave5/wave5.h  |   3 +
> 11 files changed, 430 insertions(+), 346 deletions(-)
>
>-- 
>2.43.0
>

RE: [RESEND PATCH v6 0/4] Add features to an existing driver

[jackson.lee]

Hi Sebastian

> -----Original Message-----
> From: Sebastian Fricke <sebastian.fricke@collabora.com>
> Sent: Tuesday, June 18, 2024 6:30 PM
> To: jackson.lee <jackson.lee@chipsnmedia.com>
> Cc: mchehab@kernel.org; nicolas@ndufresne.ca; linux-media@vger.kernel.org;
> linux-kernel@vger.kernel.org; hverkuil@xs4all.nl; Nas Chung
> <nas.chung@chipsnmedia.com>; lafley.kim <lafley.kim@chipsnmedia.com>; b-
> brnich@ti.com
> Subject: Re: [RESEND PATCH v6 0/4] Add features to an existing driver
> 
> Hey Jackson,
> 
> what is up with all the resends, I can see that you send V6 two times without
> a RESEND tag and once with a RESEND tag?
> Was that an error on your side or did you actually change something?
> Does it matter for me which version to consider or are they all the same
> content-wise?
> 


Sorry for the confusion.

Please pick the v6 RESEND patch, and please ignore the v6 patches sent two times without the RESEND tag.
I missed to remove the "-ENOTTY" code in the v6 patch two times,  I sent the v6 patch with RESEND tag, again.

Thanks
Jackson

> Regards,
> Sebastian
> 
> On 17.06.2024 19:48, Jackson.lee wrote:
> >The wave5 codec driver is a stateful encoder/decoder.
> >The following patches is for supporting yuv422 inpuy format, supporting
> runtime suspend/resume feature and extra things.
> >
> >v4l2-compliance results:
> >========================
> >
> >v4l2-compliance 1.24.1, 64 bits, 64-bit time_t
> >
> >Buffer ioctls:
> >       warn: v4l2-test-buffers.cpp(693): VIDIOC_CREATE_BUFS not supported
> >       warn: v4l2-test-buffers.cpp(693): VIDIOC_CREATE_BUFS not supported
> >    test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> >    test VIDIOC_EXPBUF: OK
> >    test Requests: OK (Not Supported)
> >
> >Total for wave5-dec device /dev/video0: 45, Succeeded: 45, Failed: 0,
> >Warnings: 2 Total for wave5-enc device /dev/video1: 45, Succeeded: 45,
> >Failed: 0, Warnings: 0
> >
> >Fluster test results:
> >=====================
> >
> >Running test suite JCT-VC-HEVC_V1 with decoder GStreamer-H.265-V4L2-Gst1.0
> Using 1 parallel job(s)
> >Ran 132/147 tests successfully               in 88.745 secs
> >
> >(1 test fails because of not supporting to parse multi frames, 1 test
> >fails because of a missing frame and slight corruption,
> > 2 tests fail because of sizes which are incompatible with the IP, 11
> >tests fail because of unsupported 10 bit format)
> >
> >Running test suite JVT-AVC_V1 with decoder GStreamer-H.264-V4L2-Gst1.0 Using
> 1 parallel job(s)
> >Ran 77/135 tests successfully               in 32.044 secs
> >
> >(58 fail because the hardware is unable to decode  MBAFF / FMO / Field
> >/ Extended profile streams.)
> >
> >Change since v5:
> >================
> >* For [PATCH v4 3/4] media: chips-media: wave5: Use helpers to calculate
> bytesperline and sizeimage.
> > - Fix v4l2-compliance error for the vidioc_enum_framesizes
> >
> >* For [PATCH v4 1/4] media: chips-media: wave5: Support SPS/PPS
> >generation for each IDR
> > - Remove warning messages for the checkpatch.pl script
> >
> >Change since v4:
> >================
> >* For [PATCH v4 2/4] media: chips-media: wave5: Support runtime
> >suspend/resume
> > - Fix warning message
> >
> >* For [PATCH v4 3/4] media: chips-media: wave5: Use helpers to calculate
> bytesperline and sizeimage.
> > - Fix warning message
> > - add Reviewed-By tag
> >
> >* For [PATCH v4 4/4] media: chips-media: wave5: Support YUV422 raw
> >pixel-formats on the encoder
> > - add Reviewed-By tag
> >
> >Change since v3:
> >=================
> >
> >* For [PATCH v4 1/4] media: chips-media: wave5: Support SPS/PPS
> >generation for each IDR
> > - add Reviewed-By tag
> >
> >* For [PATCH v4 2/4] media: chips-media: wave5: Support runtime
> >suspend/resume
> > - add Reviewed-By tag
> >
> >* For [PATCH v4 3/4] media: chips-media: wave5: Use helpers to calculate
> bytesperline and sizeimage.
> > - modify the commit message
> > - define three framesize structures for decoder
> >
> >* For [PATCH v4 4/4] media: chips-media: wave5: Support YUV422 raw
> >pixel-formats on the encoder
> > - modify the commit message
> > - use the v4l2_format_info to calculate luma, chroma size
> >
> >Change since v2:
> >=================
> >
> >* For [PATCH v3 0/4] media: chips-media: wave5: Support SPS/PPS
> >generation for each IDR
> > - add the suggested _SHIFT suffix
> >
> >* For [PATCH v3 1/4] media: chips-media: wave5: Support runtime
> >suspend/resume
> > - change a commit message
> >
> >* For [PATCH v3 2/4] media: chips-media: wave5: Use helpers to
> >calculate bytesperline and sizeimage
> > - add pix_fmt_type parameter into wave5_update_pix_fmt function
> > - add min/max width/height values into dec_fmt_list
> >
> >Change since v1:
> >=================
> >
> >* For [PATCH v2 0/4] media: chips-media: wave5: Support SPS/PPS
> >generation for each IDR
> > - define a macro for register addresses
> >
> >* For [PATCH v2 1/4] media: chips-media: wave5: Support runtime
> >suspend/resume
> > - add auto suspend/resume
> >
> >* For [PATCH v2 2/4] media: chips-media: wave5: Use helpers to
> >calculate bytesperline and sizeimage
> > - use helper functions to calculate bytesperline and sizeimage
> >
> >* For [PATCH v2 3/4] media: chips-media: wave5: Support YUV422 raw
> >pixel-formats on the encoder
> > - remove unnecessary codes
> >
> >Change since v0:
> >=================
> >The DEFAULT_SRC_SIZE macro was defined using multiple lines, To make a
> simple define, tab and multiple lines has been removed, The macro is defined
> using one line.
> >
> >
> >jackson.lee (4):
> >  media: chips-media: wave5: Support SPS/PPS generation for each IDR
> >  media: chips-media: wave5: Support runtime suspend/resume
> >  media: chips-media: wave5: Use helpers to calculate bytesperline and
> >    sizeimage.
> >  media: chips-media: wave5: Support YUV422 raw pixel-formats on the
> >    encoder.
> >
> > .../platform/chips-media/wave5/wave5-helper.c |  24 ++
> > .../platform/chips-media/wave5/wave5-helper.h |   5 +
> > .../platform/chips-media/wave5/wave5-hw.c     |  30 +-
> > .../chips-media/wave5/wave5-vpu-dec.c         | 316 +++++++-----------
> > .../chips-media/wave5/wave5-vpu-enc.c         | 308 +++++++++--------
> > .../platform/chips-media/wave5/wave5-vpu.c    |  43 +++
> > .../platform/chips-media/wave5/wave5-vpu.h    |   5 +-
> > .../platform/chips-media/wave5/wave5-vpuapi.c |  14 +-
> > .../platform/chips-media/wave5/wave5-vpuapi.h |   1 +
> > .../chips-media/wave5/wave5-vpuconfig.h       |  27 +-
> > .../media/platform/chips-media/wave5/wave5.h  |   3 +
> > 11 files changed, 430 insertions(+), 346 deletions(-)
> >
> >--
> >2.43.0
> >