Thread (37 messages) 37 messages, 4 authors, 13d ago

RE: [PATCH v7 3/9] media: chips-media: wave6: Add Wave6 VPU interface

From: Nas Chung <nas.chung@chipsnmedia.com>
Date: 2026-09-14 04:37:50
Also in: linux-arm-kernel, linux-media, lkml 

Hi, Frank.

-----Original Message-----
From: Frank Li <redacted>
Sent: Friday, September 11, 2026 4:51 AM
To: Nas Chung <nas.chung@chipsnmedia.com>
Cc: mchehab@kernel.org; hverkuil@xs4all.nl; robh@kernel.org;
krzk+dt@kernel.org; conor+dt@kernel.org; shawnguo@kernel.org;
s.hauer@pengutronix.de; linux-media@vger.kernel.org;
devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-imx@nxp.com;
linux-arm-kernel@lists.infradead.org; jackson.lee
[off-list ref]; lafley.kim [off-list ref];
marek.vasut@mailbox.org; Ming Qian [off-list ref]
Subject: Re: [PATCH v7 3/9] media: chips-media: wave6: Add Wave6 VPU
interface

On Fri, Sep 04, 2026 at 03:46:29PM +0900, Nas Chung wrote:
quoted
Add an interface layer to manage hardware register configuration
and communication with the Chips&Media Wave6 video codec IP.

The interface provides low-level helper functions used by the
Wave6 core driver to implement video encoding and decoding operations.
It handles command submission to the firmware via MMIO registers,
and waits for a response by polling the firmware busy flag.

Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
Tested-by: Ming Qian <redacted>
Tested-by: Marek Vasut <redacted>
---
 MAINTAINERS                                   |    1 +
 .../platform/chips-media/wave6/wave6-hw.c     | 2030 +++++++++++++++++
 .../platform/chips-media/wave6/wave6-hw.h     |   56 +
 .../chips-media/wave6/wave6-regdefine.h       |  649 ++++++
 .../platform/chips-media/wave6/wave6-vdi.h    |   92 +
 .../platform/chips-media/wave6/wave6-vpuapi.c |  336 +++
 .../platform/chips-media/wave6/wave6-vpuapi.h | 1007 ++++++++
 .../chips-media/wave6/wave6-vpuconfig.h       |   72 +
 .../chips-media/wave6/wave6-vpuerror.h        |  262 +++
You need make this patch built-able. Add Makefile and Kconfig part.
OK. I'll move Makefile and Kconfig in v8.

quoted
 9 files changed, 4505 insertions(+)
 create mode 100644 drivers/media/platform/chips-media/wave6/wave6-hw.c
 create mode 100644 drivers/media/platform/chips-media/wave6/wave6-hw.h
 create mode 100644 drivers/media/platform/chips-media/wave6/wave6-
regdefine.h
quoted
 create mode 100644 drivers/media/platform/chips-media/wave6/wave6-vdi.h
 create mode 100644 drivers/media/platform/chips-media/wave6/wave6-
vpuapi.c
quoted
 create mode 100644 drivers/media/platform/chips-media/wave6/wave6-
vpuapi.h
quoted
 create mode 100644 drivers/media/platform/chips-media/wave6/wave6-
vpuconfig.h
quoted
 create mode 100644 drivers/media/platform/chips-media/wave6/wave6-
vpuerror.h
quoted
diff --git a/MAINTAINERS b/MAINTAINERS
index 7387a11facbe..e29018c2546b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -29239,6 +29239,7 @@ M:	Jackson Lee <jackson.lee@chipsnmedia.com>
 L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/media/nxp,imx95-vpu.yaml
+F:	drivers/media/platform/chips-media/wave6/

 WHISKEYCOVE PMIC GPIO DRIVER
 M:	Kuppuswamy Sathyanarayanan
[off-list ref]
quoted
diff --git a/drivers/media/platform/chips-media/wave6/wave6-hw.c
b/drivers/media/platform/chips-media/wave6/wave6-hw.c
quoted
new file mode 100644
index 000000000000..a91b582af023
--- /dev/null
+++ b/drivers/media/platform/chips-media/wave6/wave6-hw.c
@@ -0,0 +1,2030 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+/*
+ * Wave6 series multi-standard codec IP - wave6 backend interface
+ *
+ * Copyright (C) 2025 CHIPS&MEDIA INC
+ */
+
+#include <linux/iopoll.h>
+#include "wave6-vpu-core.h"
+#include "wave6-hw.h"
+#include "wave6-regdefine.h"
+#include "wave6-trace.h"
Add trace support later, add with patch6.
OK.

quoted
+
+void wave6_vpu_writel(struct vpu_core_device *core, u32 addr, u32 data)
+{
+	wave6_vdi_writel(core->reg_base, addr, data);
+	trace_wave6_vpu_writel(core->dev, addr, data);
Add trace support at later patches.
OK.

quoted
+}
+
+u32 wave6_vpu_readl(struct vpu_core_device *core, u32 addr)
+{
+	u32 data;
+
+	data = wave6_vdi_readl(core->reg_base, addr);
+	trace_wave6_vpu_readl(core->dev, addr, data);
+
+	return data;
+}
+
+static void wave6_print_reg_err(struct vpu_core_device *core, u32
fail_reason)
quoted
+{
+	void *caller = __builtin_return_address(0);
+	struct device *dev = core->dev;
+
+	switch (fail_reason) {
+	case WAVE6_SYSERR_QUEUEING_FAIL:
you define it as bit mask, is it possible more than 1 bits to set?
No, the firmware reports one reason at a time in W6_RET_FAIL_REASON, so
only one bit is ever set.

quoted
+		dev_dbg(dev, "%pS: queueing failure 0x%x\n", caller,
fail_reason);
quoted
+		break;
+	case WAVE6_SYSERR_RESULT_NOT_READY:
+		dev_err(dev, "%pS: result not ready 0x%x\n", caller,
fail_reason);
quoted
+		break;
+	case WAVE6_SYSERR_ACCESS_VIOLATION_HW:
+		dev_err(dev, "%pS: access violation 0x%x\n", caller,
fail_reason);
quoted
+		break;
+	case WAVE6_SYSERR_WATCHDOG_TIMEOUT:
+		dev_err(dev, "%pS: watchdog timeout 0x%x\n", caller,
fail_reason);
quoted
+		break;
+	case WAVE6_SYSERR_BUS_ERROR:
+		dev_err(dev, "%pS: bus error 0x%x\n", caller, fail_reason);
+		break;
+	case WAVE6_SYSERR_DOUBLE_FAULT:
+		dev_err(dev, "%pS: double fault 0x%x\n", caller, fail_reason);
+		break;
+	case WAVE6_SYSERR_VPU_STILL_RUNNING:
+		dev_err(dev, "%pS: still running 0x%x\n", caller,
fail_reason);
quoted
+		break;
+	default:
+		dev_err(dev, "%pS: failure: 0x%x\n", caller, fail_reason);
+		break;
+	}
+}
+
+static void wave6_dec_set_display_buffer(struct vpu_instance *inst,
struct frame_buffer fb)
quoted
+
+	struct dec_info *p_dec_info = &inst->codec_info->dec_info;
+	int index;
+
+	for (index = 0; index < WAVE6_MAX_FBS; index++) {
+		if (!p_dec_info->disp_buf[index].buf_y) {
+			p_dec_info->disp_buf[index] = fb;
+			p_dec_info->disp_buf[index].index = index;
index is the same as nature array index p_dec_info->disp_buf, why need it.
Agreed. I'll remove the index field.

quoted
+			break;
where update .buf_y?
p_dec_info->disp_buf[index] = fb; assigns the whole struct,
buf_y included.

quoted
+		}
+	}
+}
+
+static struct frame_buffer wave6_dec_get_display_buffer(struct
vpu_instance *inst,
quoted
+							dma_addr_t addr)
+{
+	struct dec_info *p_dec_info = &inst->codec_info->dec_info;
+	int i;
+	struct frame_buffer fb;
+
+	for (i = 0; i < WAVE6_MAX_FBS; i++) {
+		if (p_dec_info->disp_buf[i].buf_y == addr)
+			return p_dec_info->disp_buf[i];
+	}
+
+	memset(&fb, 0, sizeof(struct frame_buffer));
+	fb.index = -1;
suppose this error case,  did caller tolerent fb.index -1, I have not
see check this value.
In wave6_vpu_dec_get_output_info(), decoded_idx and disp_idx start at -1
and every dec_out_info[] access checks both bounds.

I'll remove the index and handle this in wave6_vpu_dec_get_output_info().

quoted
+
+	return fb;
+}
+
+static void wave6_dec_remove_display_buffer(struct vpu_instance *inst,
+					    dma_addr_t addr)
+{
+	struct dec_info *p_dec_info = &inst->codec_info->dec_info;
+	int i;
+
+	for (i = 0; i < WAVE6_MAX_FBS; i++) {
+		if (p_dec_info->disp_buf[i].buf_y == addr) {
+			memset(&p_dec_info->disp_buf[i], 0, sizeof(struct
frame_buffer));

you suppose need set p_dec_info->disp_buf[i].buf_y to 0?
Agreed. I'll drop the memset and just set buf_y to 0.

quoted
+			break;
+		}
+	}
+}
+
+static enum chroma_format_idc get_chroma_format_idc(enum
frame_buffer_format fmt)
quoted
+{
+	switch (fmt) {
+	case FORMAT_420:
+	case FORMAT_420_P10_16BIT_MSB:
+	case FORMAT_420_P10_16BIT_LSB:
+	case FORMAT_420_P10_32BIT_MSB:
+	case FORMAT_420_P10_32BIT_LSB:
+		return C_FMT_IDC_YUV420;
+	case FORMAT_422:
+	case FORMAT_422_P10_16BIT_MSB:
+	case FORMAT_422_P10_16BIT_LSB:
+	case FORMAT_422_P10_32BIT_MSB:
+	case FORMAT_422_P10_32BIT_LSB:
+	case FORMAT_YUYV:
+	case FORMAT_YVYU:
+	case FORMAT_UYVY:
+	case FORMAT_VYUY:
+	case FORMAT_YUYV_P10_16BIT_MSB:
+	case FORMAT_YVYU_P10_16BIT_MSB:
+	case FORMAT_UYVY_P10_16BIT_MSB:
+	case FORMAT_VYUY_P10_16BIT_MSB:
+	case FORMAT_YUYV_P10_16BIT_LSB:
+	case FORMAT_YVYU_P10_16BIT_LSB:
+	case FORMAT_UYVY_P10_16BIT_LSB:
+	case FORMAT_VYUY_P10_16BIT_LSB:
+	case FORMAT_YUYV_P10_32BIT_MSB:
+	case FORMAT_YVYU_P10_32BIT_MSB:
+	case FORMAT_UYVY_P10_32BIT_MSB:
+	case FORMAT_VYUY_P10_32BIT_MSB:
+	case FORMAT_YUYV_P10_32BIT_LSB:
+	case FORMAT_YVYU_P10_32BIT_LSB:
+	case FORMAT_UYVY_P10_32BIT_LSB:
+	case FORMAT_VYUY_P10_32BIT_LSB:
+		return C_FMT_IDC_YUV422;
+	case FORMAT_444:
+	case FORMAT_444_P10_16BIT_MSB:
+	case FORMAT_444_P10_16BIT_LSB:
+	case FORMAT_444_P10_32BIT_MSB:
+	case FORMAT_444_P10_32BIT_LSB:
+		return C_FMT_IDC_YUV444;
+	case FORMAT_400:
+	case FORMAT_400_P10_16BIT_MSB:
+	case FORMAT_400_P10_16BIT_LSB:
+	case FORMAT_400_P10_32BIT_MSB:
+	case FORMAT_400_P10_32BIT_LSB:
+	case FORMAT_YUV444_24BIT:
+		return C_FMT_IDC_YUV400;
+	case FORMAT_RGB_24BIT_PACKED:
+	case FORMAT_YUV444_24BIT_PACKED:
+	case FORMAT_RGB_32BIT_PACKED:
+	case FORMAT_RGB_P10_32BIT_PACKED:
+	case FORMAT_YUV444_32BIT_PACKED:
+	case FORMAT_YUV444_P10_32BIT_PACKED:
+		return C_FMT_IDC_RGB;
+	default:
+		return C_FMT_IDC_YUV400;
+	}
+}
+
+static int wave6_wait_vpu_busy(struct vpu_core_device *core, unsigned
int addr)
quoted
+{
+	u32 data;
+
+	lockdep_assert_held(&core->hw_lock);
+
+	return read_poll_timeout(vpu_read_reg, data, !data,
+				 W6_VPU_POLL_DELAY_US, W6_VPU_POLL_TIMEOUT,
+				 false, core, addr);
+}
+
+void wave6_vpu_enable_interrupt(struct vpu_core_device *core)
+{
+	u32 data;
+
+	lockdep_assert_held(&core->hw_lock);
+
+	data = BIT(W6_INT_BIT_ENC_SET_PARAM);
+	data |= BIT(W6_INT_BIT_ENC_PIC);
+	data |= BIT(W6_INT_BIT_INIT_SEQ);
+	data |= BIT(W6_INT_BIT_DEC_PIC);
+	data |= BIT(W6_INT_BIT_BSBUF_ERROR);
+	data |= BIT(W6_INT_BIT_REQ_WORK_BUF);
+	vpu_write_reg(core, W6_VPU_VINT_ENABLE, data);
+}
+
+bool wave6_vpu_is_init(struct vpu_core_device *core)
+{
+	lockdep_assert_held(&core->hw_lock);
+
+	return vpu_read_reg(core, W6_VPU_VCPU_CUR_PC) != 0;
+}
+
+static u32 wave6_vpu_get_product_id(struct vpu_core_device *core)
+{
+	u32 product_id = PRODUCT_ID_NONE;
+	u32 val;
+
+	lockdep_assert_held(&core->hw_lock);
+
+	val = vpu_read_reg(core, W6_VPU_RET_PRODUCT_CODE);
+
+	switch (val) {
+	case WAVE617_CODE:
+		product_id = PRODUCT_ID_617;
return PRODUCT_ID_617;

quoted
+		break;
+	case WAVE627_CODE:
+		product_id = PRODUCT_ID_627;
+		break;
return PRODUCT_ID_627;

quoted
+	case WAVE633_CODE:
+	case WAVE637_CODE:
+	case WAVE663_CODE:
+	case WAVE677_CODE:
+		product_id = PRODUCT_ID_637;
+		break;
return PRODUCT_ID_637;
quoted
+	default:
+		dev_err(core->dev, "Invalid product (%x)\n", val);
+		break;
+	}
+
+	return product_id;
return PRODUCT_ID_NONE; needn't variable product_id
OK.

quoted
+}
+
+static void wave6_send_command(struct vpu_core_device *core, u32 id, u32
std, u32 cmd)
quoted
+{
+	u32 reg_val;
+
+	lockdep_assert_held(&core->hw_lock);
+
+	if (cmd == W6_CMD_CREATE_INSTANCE)
+		reg_val = INSTANCE_INFO_CODEC_STD(std);
+	else
+		reg_val = INSTANCE_INFO_CODEC_STD(std) | INSTANCE_INFO_ID(id);
reg_val = INSTANCE_INFO_CODEC_STD(std);
if (cmd != W6_CMD_CREATE_INSTANCE)
	reg_val |= INSTANCE_INFO_ID(id);
OK.

quoted
+	vpu_write_reg(core, W6_CMD_INSTANCE_INFO, reg_val);
+
+	vpu_write_reg(core, W6_VPU_BUSY_STATUS, BUSY_STATUS_SET);
+	vpu_write_reg(core, W6_COMMAND, cmd);
+	vpu_write_reg(core, W6_VPU_HOST_INT_REQ, HOST_INT_REQ_ON);
+
+	trace_wave6_vpu_send_command(core, id, std, cmd);
+}
+
+static int wave6_send_query(struct vpu_core_device *core, u32 id, u32
std,
quoted
+			    enum wave6_query_option query_opt)
+{
+	int ret;
+	u32 reg_val;
+
+	lockdep_assert_held(&core->hw_lock);
+
+	vpu_write_reg(core, W6_QUERY_OPTION, query_opt);
+	wave6_send_command(core, id, std, W6_CMD_QUERY);
+
+	ret = wave6_wait_vpu_busy(core, W6_VPU_BUSY_STATUS);
+	if (ret) {
+		dev_err(core->dev, "query timed out opt=0x%x\n", query_opt);
+		return ret;
+	}
+
+	if (!vpu_read_reg(core, W6_RET_SUCCESS)) {
+		reg_val = vpu_read_reg(core, W6_RET_FAIL_REASON);
+		wave6_print_reg_err(core, reg_val);
You can add such debug helper function later to reduce each patch's change.
OK. I'll split the debug part to other patch.

quoted
+		return -EIO;
+	}
+
+	return 0;
+}
...

quoted
+static void wave6_get_dec_seq_result(struct vpu_instance *inst, struct
dec_seq_info *info)
quoted
+{
+	u32 reg_val;
+	u32 profile, profile_comp;
+
+	lockdep_assert_held(&inst->dev->hw_lock);
+
+	info->rd_ptr = wave6_vpu_dec_get_rd_ptr(inst);
+
+	reg_val = vpu_read_reg(inst->dev, W6_RET_DEC_PIC_SIZE);
+	info->pic_width = DEC_PIC_SIZE_WIDTH(reg_val);
+	info->pic_height = DEC_PIC_SIZE_HEIGHT(reg_val);
+
+	info->min_frame_buffer_count = vpu_read_reg(inst->dev,
W6_RET_DEC_NUM_REQUIRED_FBC_FB);
quoted
+	info->frame_buf_delay = vpu_read_reg(inst->dev,
W6_RET_DEC_NUM_REORDER_DELAY);
quoted
+	info->req_mv_buffer_count = vpu_read_reg(inst->dev,
W6_RET_DEC_NUM_REQUIRED_COL_BUF);
quoted
+
+	reg_val = vpu_read_reg(inst->dev, W6_RET_DEC_CROP_TOP_BOTTOM);
+	info->pic_crop_rect.top = DEC_CROP_TOP(reg_val);
+	info->pic_crop_rect.bottom = info->pic_height -
DEC_CROP_BOTTOM(reg_val);
quoted
+
+	reg_val = vpu_read_reg(inst->dev, W6_RET_DEC_CROP_LEFT_RIGHT);
+	info->pic_crop_rect.left = DEC_CROP_LEFT(reg_val);
+	info->pic_crop_rect.right = info->pic_width -
DEC_CROP_RIGHT(reg_val);
quoted
+
+	info->f_rate_numerator = vpu_read_reg(inst->dev,
W6_RET_DEC_FRAME_RATE_NR);
quoted
+	info->f_rate_denominator = vpu_read_reg(inst->dev,
W6_RET_DEC_FRAME_RATE_DR);
quoted
+
+	reg_val = vpu_read_reg(inst->dev, W6_RET_DEC_COLOR_SAMPLE);
+	info->luma_bit_depth = DEC_COLOR_SAMPLE_L_BIT_DEPTH(reg_val);
+	info->chroma_bit_depth = DEC_COLOR_SAMPLE_C_BIT_DEPTH(reg_val);
+	info->c_fmt_idc = DEC_COLOR_SAMPLE_C_FMT_IDC(reg_val);
+	info->aspect_rate_info = DEC_COLOR_SAMPLE_ASPECT_RATIO_IDC(reg_val);
+	info->is_ext_sar = info->aspect_rate_info ==
H264_VUI_SAR_IDC_EXTENDED;
quoted
+	if (info->is_ext_sar)
+		info->aspect_rate_info = vpu_read_reg(inst->dev,
W6_RET_DEC_ASPECT_RATIO);
quoted
+	info->bitrate = vpu_read_reg(inst->dev, W6_RET_DEC_BIT_RATE);
+
+	reg_val = vpu_read_reg(inst->dev, W6_RET_DEC_SEQ_PARAM);
+	info->level = DEC_SEQ_PARAM_LEVEL(reg_val);
+	info->tier = DEC_SEQ_PARAM_TIER(reg_val);
+
+	profile = DEC_SEQ_PARAM_PROFILE(reg_val);
+	profile_comp = DEC_SEQ_PARAM_PROFILE_COMPATIBILITY(reg_val);
+	if (inst->std == W_HEVC_DEC) {
+		info->profile = profile;
+		if (!info->profile) {
+			if ((profile_comp & PROFILE_COMPATIBILITY_MAIN) &&
+			    (profile_comp & PROFILE_COMPATIBILITY_MAIN10))
+				info->profile = HEVC_PROFILE_MAIN;
+			else if (profile_comp & PROFILE_COMPATIBILITY_MAIN10)
+				info->profile = HEVC_PROFILE_MAIN10;
+			else if (profile_comp &
PROFILE_COMPATIBILITY_STILL_PICTURE)
quoted
+				info->profile = HEVC_PROFILE_STILLPICTURE;
+			else
+				info->profile = HEVC_PROFILE_MAIN;
+		}
+	} else if (inst->std == W_AVC_DEC) {
+		if (profile == PROFILE_H264_BP)
+			info->profile = H264_PROFILE_BP;
+		else if (profile == PROFILE_H264_HP)
+			info->profile = H264_PROFILE_HP;
+		else if (profile == PROFILE_H264_MP)
+			info->profile = H264_PROFILE_MP;
+		else if (profile == PROFILE_H264_HIGH10)
+			info->profile = H264_PROFILE_HIGH10;
+		else if (profile == PROFILE_H264_EXTENDED)
+			info->profile = H264_PROFILE_EXTENDED;
+		else
+			info->profile = H264_PROFILE_BP;
use small helper function for such convertion, you can use switch case for
this maping or static const array.
OK. I'll address this in v8.

quoted
+	}
+
+	reg_val = vpu_read_reg(inst->dev, W6_RET_DEC_COLOR_CONFIG);
+	if (reg_val) {
+		info->color.video_signal_type_present = true;
+		info->color.color_description_present =
DEC_COLOR_CONFIG_COLOR_PRESENT(reg_val);
quoted
+		info->color.color_primaries =
DEC_COLOR_CONFIG_COLOR_PRIMARIES(reg_val);
quoted
+		info->color.transfer_characteristics =
DEC_COLOR_CONFIG_TRANS_CHAR(reg_val);
quoted
+		info->color.matrix_coefficients =
DEC_COLOR_CONFIG_MATRIX_COEFF(reg_val);
quoted
+		info->color.color_range =
DEC_COLOR_CONFIG_COLOR_RANGE(reg_val);
quoted
+	} else {
+		info->color.video_signal_type_present = false;
+	}
+}
+
+int wave6_vpu_dec_get_seq_info(struct vpu_instance *inst, struct
dec_seq_info *info)
quoted
+{
+	int ret;
+
+	lockdep_assert_held(&inst->dev->hw_lock);
+
+	ret = wave6_send_query(inst->dev, inst->id, inst->std,
W6_QUERY_OPT_GET_RESULT);
quoted
+	if (ret)
+		return ret;
+
+	if (vpu_read_reg(inst->dev, W6_RET_DEC_DECODING_SUCCESS) != 1) {
+		info->err_reason = vpu_read_reg(inst->dev,
W6_RET_DEC_ERR_INFO);
quoted
+		ret = -EIO;
+	} else {
+		info->warn_info = vpu_read_reg(inst->dev,
W6_RET_DEC_WARN_INFO);
quoted
+	}
+
+	wave6_get_dec_seq_result(inst, info);
+
+	return ret;
+}
+
+int wave6_vpu_dec_register_frame_buffer(struct vpu_instance *inst)
+{
+	struct dec_info *p_dec_info;
+	struct frame_buffer *fb_arr;
+	size_t fbc_remain, mv_remain, fbc_idx = 0, mv_idx = 0;
+	size_t i, k, group_num, fbc_count, mv_count;
+	dma_addr_t fbc_cr_tbl_addr;
+	u32 reg_val;
+	int ret;
+
+	guard(mutex)(&inst->dev->hw_lock);
+
+	p_dec_info = &inst->codec_info->dec_info;
+
+	fb_arr = &inst->frame_buf[0];
+	fbc_count = p_dec_info->seq_info.min_frame_buffer_count;
+	mv_count = p_dec_info->seq_info.req_mv_buffer_count;
+
+	for (i = 0; i < fbc_count; i++) {
+		if (!inst->aux_vbuf[AUX_BUF_FBC_Y_TBL][i].daddr)
+			return -EINVAL;
+		if (!inst->aux_vbuf[AUX_BUF_FBC_C_TBL][i].daddr)
+			return -EINVAL;
+	}
+	for (i = 0; i < mv_count; i++) {
+		if (!inst->aux_vbuf[AUX_BUF_MV_COL][i].daddr)
+			return -EINVAL;
+	}
use small helper function, like check_aux_vbuf().
OK.

quoted
+
+	reg_val = SET_FB_PIC_SIZE_WIDTH(p_dec_info->seq_info.pic_width) |
+		  SET_FB_PIC_SIZE_HEIGHT(p_dec_info->seq_info.pic_height);
+	vpu_write_reg(inst->dev, W6_CMD_SET_FB_PIC_SIZE, reg_val);
+	reg_val = SET_FB_PIC_INFO_C_FMT_IDC(p_dec_info->seq_info.c_fmt_idc)
|
quoted
+		  SET_FB_PIC_INFO_L_BIT_DEPTH(p_dec_info-
seq_info.luma_bit_depth) |
+		  SET_FB_PIC_INFO_C_BIT_DEPTH(p_dec_info-
seq_info.chroma_bit_depth);
+	vpu_write_reg(inst->dev, W6_CMD_SET_FB_PIC_INFO, reg_val);
+	vpu_write_reg(inst->dev, W6_CMD_SET_FB_DEFAULT_CDF, 0);
+	vpu_write_reg(inst->dev, W6_CMD_SET_FB_SEGMAP, 0);
+	vpu_write_reg(inst->dev, W6_CMD_SET_FB_MV_COL_PRE_ENT, 0);
+
+	fbc_remain = fbc_count;
+	mv_remain = mv_count;
+	group_num = DIV_ROUND_UP(max(fbc_count, mv_count), 16);
Add empty line here.
OK.

quoted
+	for (i = 0; i < group_num; i++) {
+		bool first_group = i == 0;
+		bool last_group = i == group_num - 1;
+		u32 set_fbc_num = (fbc_remain >= 16) ? 16 : fbc_remain;
+		u32 set_mv_num = (mv_remain >= 16) ? 16 : mv_remain;
+		u32 fbc_start_no = i * 16;
+		u32 fbc_end_no = fbc_start_no + set_fbc_num - 1;
+		u32 mv_start_no = i * 16;
+		u32 mv_end_no = mv_start_no + set_mv_num - 1;
+
+		reg_val = SET_FB_OPTION_ENDIAN(VDI_128BIT_BIG_ENDIAN);
+		if (first_group)
+			reg_val |= SET_FB_OPTION_START;
+		if (last_group)
+			reg_val |= SET_FB_OPTION_END;
+		vpu_write_reg(inst->dev, W6_CMD_SET_FB_OPTION, reg_val);
+
+		reg_val = SET_FB_NUM_FBC_START_IDX(fbc_start_no) |
+			  SET_FB_NUM_FBC_END_IDX(fbc_end_no) |
+			  SET_FB_NUM_MV_COL_START_IDX(mv_start_no) |
+			  SET_FB_NUM_MV_COL_END_IDX(mv_end_no);
+		vpu_write_reg(inst->dev, W6_CMD_SET_FB_NUM, reg_val);
+
+		for (k = 0; k < set_fbc_num; k++) {
+			vpu_write_reg(inst->dev, W6_CMD_SET_FB_FBC_Y(k),
+				      fb_arr[fbc_idx].buf_y);
+			vpu_write_reg(inst->dev, W6_CMD_SET_FB_FBC_C(k),
+				      fb_arr[fbc_idx].buf_cb);
+			vpu_write_reg(inst->dev, W6_CMD_SET_FB_FBC_CR(k),
+				      fb_arr[fbc_idx].buf_cr);
+			vpu_write_reg(inst->dev, W6_CMD_SET_FB_FBC_Y_OFFSET(k),
+				      inst-
aux_vbuf[AUX_BUF_FBC_Y_TBL][fbc_idx].daddr);
+			vpu_write_reg(inst->dev, W6_CMD_SET_FB_FBC_C_OFFSET(k),
+				      inst-
aux_vbuf[AUX_BUF_FBC_C_TBL][fbc_idx].daddr);
+			fbc_cr_tbl_addr = inst-
aux_vbuf[AUX_BUF_FBC_C_TBL][fbc_idx].daddr +
+					  (inst-
aux_vbuf[AUX_BUF_FBC_C_TBL][fbc_idx].size >> 1);
+			vpu_write_reg(inst->dev, W6_CMD_SET_FB_FBC_CR_OFFSET(k),
+				      fbc_cr_tbl_addr);
I am not sure how big set_fbc_num, I suggest change to use writel_relax().

because you wave6_wait_vpu_busy() to check status, extra memory barry in
writel() is not neccessary.
OK, as I answered on your earlier mail. The barriered writel() stays in
wave6_send_command().

quoted
+			fbc_idx++;
+		}
+		fbc_remain -= k;
+
+		for (k = 0; k < set_mv_num; k++) {
+			vpu_write_reg(inst->dev, W6_CMD_SET_FB_MV_COL(k),
+				      inst-
aux_vbuf[AUX_BUF_MV_COL][mv_idx].daddr);
+			mv_idx++;
+		}
+		mv_remain -= k;
+
+		wave6_send_command(inst->dev, inst->id, inst->std,
W6_CMD_SET_FB);
quoted
+		ret = wave6_wait_vpu_busy(inst->dev, W6_VPU_BUSY_STATUS);
+		if (ret) {
+			dev_err(inst->dev->dev, "%s: timeout\n", __func__);
+			return ret;
+		}
+
+		if (!vpu_read_reg(inst->dev, W6_RET_SUCCESS))
+			return -EIO;
+	}
+
+	return 0;
+}
+
+int wave6_vpu_dec_register_display_buffer(struct vpu_instance *inst,
struct frame_buffer fb)
quoted
+{
+	int ret;
+	struct dec_info *p_dec_info;
+	u32 reg_val;
+	u32 c_fmt_idc, out_fmt, out_mode;
+
+	guard(mutex)(&inst->dev->hw_lock);
+
+	p_dec_info = &inst->codec_info->dec_info;
+
+	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_SCL_PARAM,
+		      inst->scaler_info.enable);
+	reg_val = SET_DISP_SCL_PIC_SIZE_WIDTH(inst->scaler_info.width) |
+		  SET_DISP_SCL_PIC_SIZE_HEIGHT(inst->scaler_info.height);
+	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_SCL_PIC_SIZE, reg_val);
+	reg_val = SET_DISP_PIC_SIZE_WIDTH(p_dec_info->seq_info.pic_width) |
+		  SET_DISP_PIC_SIZE_HEIGHT(p_dec_info->seq_info.pic_height);
+	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_PIC_SIZE, reg_val);
+
+	c_fmt_idc = get_chroma_format_idc(p_dec_info->wtl_format);
+	switch (p_dec_info->wtl_format) {
+	case FORMAT_420_P10_16BIT_MSB:
+	case FORMAT_422_P10_16BIT_MSB:
+	case FORMAT_444_P10_16BIT_MSB:
+	case FORMAT_400_P10_16BIT_MSB:
+		out_mode = (WTL_RIGHT_JUSTIFIED << 2) | WTL_PIXEL_16BIT;
+		break;
+	case FORMAT_420_P10_16BIT_LSB:
+	case FORMAT_422_P10_16BIT_LSB:
+	case FORMAT_444_P10_16BIT_LSB:
+	case FORMAT_400_P10_16BIT_LSB:
+		out_mode = (WTL_LEFT_JUSTIFIED << 2) | WTL_PIXEL_16BIT;
+		break;
+	case FORMAT_420_P10_32BIT_MSB:
+	case FORMAT_422_P10_32BIT_MSB:
+	case FORMAT_444_P10_32BIT_MSB:
+	case FORMAT_400_P10_32BIT_MSB:
+		out_mode = (WTL_RIGHT_JUSTIFIED << 2) | WTL_PIXEL_32BIT;
+		break;
+	case FORMAT_420_P10_32BIT_LSB:
+	case FORMAT_422_P10_32BIT_LSB:
+	case FORMAT_444_P10_32BIT_LSB:
+	case FORMAT_400_P10_32BIT_LSB:
+		out_mode = (WTL_LEFT_JUSTIFIED << 2) | WTL_PIXEL_32BIT;
+		break;
+	default:
+		out_mode = (WTL_RIGHT_JUSTIFIED << 2) | WTL_PIXEL_8BIT;
+		break;
+	}
move such long convert to small helper function
OK.

quoted
+	out_fmt = (inst->nv21 << 1) | inst->cbcr_interleave;
+
+	reg_val = SET_DISP_COMMON_PIC_INFO_BWB_ON |
+		  SET_DISP_COMMON_PIC_INFO_C_FMT_IDC(c_fmt_idc) |
+		  SET_DISP_COMMON_PIC_INFO_PIXEL_ORDER(PIXEL_ORDER_INCREASING)
|
quoted
+		  SET_DISP_COMMON_PIC_INFO_OUT_MODE(out_mode) |
+		  SET_DISP_COMMON_PIC_INFO_OUT_FMT(out_fmt) |
+		  SET_DISP_COMMON_PIC_INFO_STRIDE(fb.stride);
+	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_COMMON_PIC_INFO,
reg_val);
quoted
+	reg_val = SET_DISP_OPTION_ENDIAN(VDI_128BIT_BIG_ENDIAN);
+	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_OPTION, reg_val);
+	reg_val = SET_DISP_PIC_INFO_L_BIT_DEPTH(fb.luma_bit_depth) |
+		  SET_DISP_PIC_INFO_C_BIT_DEPTH(fb.chroma_bit_depth) |
+		  SET_DISP_PIC_INFO_C_FMT_IDC(fb.c_fmt_idc);
+	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_PIC_INFO, reg_val);
+	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_Y_BASE, fb.buf_y);
+	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_CB_BASE, fb.buf_cb);
+	vpu_write_reg(inst->dev, W6_CMD_DEC_SET_DISP_CR_BASE, fb.buf_cr);
+
+	wave6_send_command(inst->dev, inst->id, inst->std,
W6_CMD_DEC_SET_DISP);
quoted
+	ret = wave6_wait_vpu_busy(inst->dev, W6_VPU_BUSY_STATUS);
+	if (ret) {
+		dev_err(inst->dev->dev, "%s: timeout\n", __func__);
+		return ret;
+	}
+
+	if (!vpu_read_reg(inst->dev, W6_RET_SUCCESS))
+		return -EIO;
+
+	wave6_dec_set_display_buffer(inst, fb);
+
+	return 0;
+}
...

quoted
+static int wave6_set_enc_crop_info(u32 codec, struct enc_codec_param
*param,
quoted
+				   u32 rot_mir, int width, int height)
+{
+	int aligned_width = (codec == W_HEVC_ENC) ? ALIGN(width, 32) :
ALIGN(width, 16);
quoted
+	int aligned_height = (codec == W_HEVC_ENC) ? ALIGN(height, 32) :
ALIGN(height, 16);
quoted
+	int pad_right, pad_bot;
+	int crop_right, crop_left, crop_top, crop_bot;
+
+	if (codec == W_HEVC_ENC &&
+	    (!rot_mir ||
+	     rot_mir == (SET_PARAM_ROT_MIRH | SET_PARAM_ROT_MIRV |
SET_PARAM_ROT_180)))
quoted
+		return 0;
+
+	pad_right = aligned_width - width;
+	pad_bot = aligned_height - height;
+
+	if (param->conf_win.right > 0)
+		crop_right = param->conf_win.right + pad_right;
+	else
+		crop_right = pad_right;
+
+	if (param->conf_win.bottom > 0)
+		crop_bot = param->conf_win.bottom + pad_bot;
+	else
+		crop_bot = pad_bot;
+
+	crop_top = param->conf_win.top;
+	crop_left = param->conf_win.left;
+
+	param->conf_win.top = crop_top;
+	param->conf_win.left = crop_left;
+	param->conf_win.bottom = crop_bot;
+	param->conf_win.right = crop_right;
+
+	if (rot_mir == SET_PARAM_ROT_90 ||
+	    rot_mir == (SET_PARAM_ROT_MIRH | SET_PARAM_ROT_MIRV |
SET_PARAM_ROT_270)) {
quoted
+		param->conf_win.top = crop_right;
+		param->conf_win.left = crop_top;
+		param->conf_win.bottom = crop_left;
+		param->conf_win.right = crop_bot;
+	} else if (rot_mir == SET_PARAM_ROT_180 ||
+		   rot_mir == (SET_PARAM_ROT_MIRH | SET_PARAM_ROT_MIRV)) {
+		param->conf_win.top = crop_bot;
+		param->conf_win.left = crop_right;
+		param->conf_win.bottom = crop_top;
+		param->conf_win.right = crop_left;
+	} else if (rot_mir == SET_PARAM_ROT_270 ||
+		   rot_mir == (SET_PARAM_ROT_MIRH | SET_PARAM_ROT_MIRV |
SET_PARAM_ROT_90)) {
quoted
+		param->conf_win.top = crop_left;
+		param->conf_win.left = crop_bot;
+		param->conf_win.bottom = crop_right;
+		param->conf_win.right = crop_top;
+	} else if (rot_mir == SET_PARAM_ROT_MIRV ||
+		   rot_mir == (SET_PARAM_ROT_MIRH | SET_PARAM_ROT_180)) {
+		param->conf_win.top = crop_bot;
+		param->conf_win.bottom = crop_top;
put bottom/right here to make code consistent.
OK. I'll fix it.

quoted
+	} else if (rot_mir == SET_PARAM_ROT_MIRH ||
+		   rot_mir == (SET_PARAM_ROT_MIRV | SET_PARAM_ROT_180)) {
+		param->conf_win.left = crop_right;
+		param->conf_win.right = crop_left;
+	} else if (rot_mir == (SET_PARAM_ROT_MIRV | SET_PARAM_ROT_90) ||
+		   rot_mir == (SET_PARAM_ROT_MIRH | SET_PARAM_ROT_270)) {
+		param->conf_win.top = crop_left;
+		param->conf_win.left = crop_top;
+		param->conf_win.bottom = crop_right;
+		param->conf_win.right = crop_bot;
+	} else if (rot_mir == (SET_PARAM_ROT_MIRV | SET_PARAM_ROT_270) ||
+		   rot_mir == (SET_PARAM_ROT_MIRH | SET_PARAM_ROT_90)) {
+		param->conf_win.top = crop_right;
+		param->conf_win.left = crop_bot;
+		param->conf_win.bottom = crop_left;
+		param->conf_win.right = crop_top;
+	}
else {
	param->conf_win.top = crop_top;
	param->conf_win.left = crop_left;
	param->conf_win.bottom = crop_bot;
	param->conf_win.right = crop_right;
}

Avoid overwrite it again at if-else branch.
OK. I'll fix it.

quoted
+
+	return 0;
+}
+
+static void wave6_update_enc_info(struct enc_info *p_enc_info)
+{
+	struct enc_open_param op = p_enc_info->open_param;
+
+	p_enc_info->width = op.pic_width;
+	p_enc_info->height = op.pic_height;
+	p_enc_info->c_fmt_idc = get_chroma_format_idc(op.output_format);
+}
+
+static void wave6_gen_set_param_reg_common(struct enc_info *p_enc_info,
enum codec_std std,
quoted
+					   struct enc_cmd_set_param_reg *reg)
+{
+	struct enc_open_param *p_open_param = &p_enc_info->open_param;
+	struct enc_codec_param *p_param = &p_open_param->codec_param;
+	unsigned int i;
+	u32 rot_mir = 0;
+
+	switch (p_enc_info->rot_angle) {
+	case ROT_0:
+		break;
+	case ROT_90:
+		rot_mir |= SET_PARAM_ROT_90;
+		break;
+	case ROT_180:
+		rot_mir |= SET_PARAM_ROT_180;
+		break;
+	case ROT_270:
+		rot_mir |= SET_PARAM_ROT_270;
+		break;
+	}
+
+	switch (p_enc_info->mir_dir) {
+	case MIR_NONE:
+		break;
+	case MIR_VER:
+		rot_mir |= SET_PARAM_ROT_MIRV;
+		break;
+	case MIR_HOR:
+		rot_mir |= SET_PARAM_ROT_MIRH;
+		break;
+	case MIR_HOR_VER:
+		rot_mir |= SET_PARAM_ROT_MIRH | SET_PARAM_ROT_MIRV;
+		break;
+	}
use small convert helper to make this function shorter
OK. I'll address this in v8.

...

quoted
+static void wave6_gen_enc_pic_reg(struct enc_info *p_enc_info, bool
cbcr_interleave, bool nv21,
quoted
+				  struct enc_param *opt, struct
enc_cmd_enc_pic_reg *reg)
quoted
+{
+	struct enc_open_param open = p_enc_info->open_param;
+	struct enc_codec_param param = open.codec_param;
+	bool is_lsb = false;
+	bool is_10bit = false;
+	bool is_3p4b = false;
+	bool is_cr_first = nv21;
+	u32 c_fmt_idc = get_chroma_format_idc(open.src_format);
+	u32 stride_c = opt->source_frame->stride_c;
+	bool is_ayuv = false;
+	bool is_csc_format = false;
+	bool is_24bit = false;
+	bool is_packed = false;
+	bool is_packed_uv_first = false;
+
+	if ((c_fmt_idc == C_FMT_IDC_YUV422 || c_fmt_idc == C_FMT_IDC_YUV444)
&&
quoted
+	    p_enc_info->c_fmt_idc == C_FMT_IDC_YUV420)
+		stride_c *= 2;
+
+	switch (open.src_format) {
+	case FORMAT_420:
+	case FORMAT_422:
+	case FORMAT_444:
+	case FORMAT_400:
+	case FORMAT_YUYV:
+	case FORMAT_YVYU:
+	case FORMAT_UYVY:
+	case FORMAT_VYUY:
+		is_lsb = false;
+		is_3p4b = false;
+		break;
+	case FORMAT_420_P10_16BIT_MSB:
+	case FORMAT_422_P10_16BIT_MSB:
+	case FORMAT_444_P10_16BIT_MSB:
+	case FORMAT_400_P10_16BIT_MSB:
+	case FORMAT_YUYV_P10_16BIT_MSB:
+	case FORMAT_YVYU_P10_16BIT_MSB:
+	case FORMAT_UYVY_P10_16BIT_MSB:
+	case FORMAT_VYUY_P10_16BIT_MSB:
+		is_lsb = false;
+		is_10bit = true;
+		is_3p4b = false;
+		break;
+	case FORMAT_420_P10_16BIT_LSB:
+	case FORMAT_422_P10_16BIT_LSB:
+	case FORMAT_444_P10_16BIT_LSB:
+	case FORMAT_400_P10_16BIT_LSB:
+	case FORMAT_YUYV_P10_16BIT_LSB:
+	case FORMAT_YVYU_P10_16BIT_LSB:
+	case FORMAT_UYVY_P10_16BIT_LSB:
+	case FORMAT_VYUY_P10_16BIT_LSB:
+		is_lsb = true;
+		is_10bit = true;
+		is_3p4b = false;
+		break;
+	case FORMAT_420_P10_32BIT_MSB:
+	case FORMAT_422_P10_32BIT_MSB:
+	case FORMAT_444_P10_32BIT_MSB:
+	case FORMAT_400_P10_32BIT_MSB:
+	case FORMAT_YUYV_P10_32BIT_MSB:
+	case FORMAT_YVYU_P10_32BIT_MSB:
+	case FORMAT_UYVY_P10_32BIT_MSB:
+	case FORMAT_VYUY_P10_32BIT_MSB:
+		is_lsb = false;
+		is_10bit = true;
+		is_3p4b = true;
+		break;
+	case FORMAT_420_P10_32BIT_LSB:
+	case FORMAT_422_P10_32BIT_LSB:
+	case FORMAT_444_P10_32BIT_LSB:
+	case FORMAT_400_P10_32BIT_LSB:
+	case FORMAT_YUYV_P10_32BIT_LSB:
+	case FORMAT_YVYU_P10_32BIT_LSB:
+	case FORMAT_UYVY_P10_32BIT_LSB:
+	case FORMAT_VYUY_P10_32BIT_LSB:
+		is_lsb = true;
+		is_10bit = true;
+		is_3p4b = true;
+		break;
+	case FORMAT_RGB_32BIT_PACKED:
+		is_ayuv = false;
+		is_csc_format = true;
+		break;
+	case FORMAT_RGB_P10_32BIT_PACKED:
+		is_ayuv = false;
+		is_csc_format = true;
+		is_10bit = true;
+		break;
+	case FORMAT_YUV444_32BIT_PACKED:
+		is_ayuv = true;
+		is_csc_format = true;
+		break;
+	case FORMAT_YUV444_P10_32BIT_PACKED:
+		is_ayuv = true;
+		is_csc_format = true;
+		is_10bit = true;
+		break;
+	case FORMAT_RGB_24BIT_PACKED:
+		is_ayuv = false;
+		is_csc_format = true;
+		is_24bit = true;
+		break;
+	case FORMAT_YUV444_24BIT_PACKED:
+		is_ayuv = true;
+		is_csc_format = true;
+		is_24bit = true;
+		break;
+	case FORMAT_YUV444_24BIT:
+		is_ayuv = true;
+		break;
+	default:
+		break;
+	}
move to small helper to this kinds of convert functions
OK. I'll address this in v8.

...

quoted
+static inline void wave6_vdi_writel(void __iomem *base, u32 addr, u32
data)
quoted
+{
+	writel(data, base + addr);
consider big amount reg read write, suggest use writel_relaxed() and
readl_relaxed()
Same as above - I'll add a relaxed variant rather than change
wave6_vdi_writel() itself, so the HOST_INT_REQ and the firmware start
keep the barrier.

quoted
+}
+
+static inline unsigned int wave6_vdi_readl(void __iomem *base, u32 addr)
+{
+	return readl(base + addr);
+}
...

quoted
+struct dec_open_param {
+	enum display_mode disp_mode;
+	u32 ext_addr_vcpu: 8;
+	bool is_secure_inst;
you use bit field:
here
is_secure_inst: 1
OK. enc_open_param has the same pattern, I'll change both.

quoted
+	u32 inst_priority: 5;
+	struct instance_buffer inst_buffer;
+};
...

quoted
+struct vpu_performance_info {
+	ktime_t ts_start;
+	ktime_t ts_first;
+	ktime_t ts_last;
+	s64 latency_first;
+	s64 latency_max;
+	s64 min_process_time;
+	s64 max_process_time;
+	u64 total_sw_time;
+	u64 total_hw_time;
+};
You can add performance_info later, like trace and dbg.
OK. I'll address this in v8.

Thanks.
Nas.

  



    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help