Re: [PATCH v11 2/6] media: chips-media: wave5: Add vpuapi layer

From: AngeloGioacchino Del Regno
Date: Wed Dec 07 2022 - 08:06:07 EST

Il 07/12/22 13:13, Sebastian Fricke ha scritto:
From: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>

Add the vpuapi layer of the wave5 codec driver.
This layer is used to configure the hardware according
to the parameters.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
Signed-off-by: Robert Beckett <bob.beckett@xxxxxxxxxxxxx>
Signed-off-by: Sebastian Fricke <sebastian.fricke@xxxxxxxxxxxxx>
Signed-off-by: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>
---
.../platform/chips-media/wave5/wave5-hw.c | 3359 +++++++++++++++++
.../chips-media/wave5/wave5-regdefine.h | 743 ++++
.../platform/chips-media/wave5/wave5-vdi.c | 245 ++
.../platform/chips-media/wave5/wave5-vdi.h | 67 +
.../platform/chips-media/wave5/wave5-vpuapi.c | 1040 +++++
.../platform/chips-media/wave5/wave5-vpuapi.h | 1136 ++++++
.../chips-media/wave5/wave5-vpuconfig.h | 90 +
.../chips-media/wave5/wave5-vpuerror.h | 454 +++
.../media/platform/chips-media/wave5/wave5.h | 94 +
9 files changed, 7228 insertions(+)
create mode 100644 drivers/media/platform/chips-media/wave5/wave5-hw.c
create mode 100644 drivers/media/platform/chips-media/wave5/wave5-regdefine.h
create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vdi.c
create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vdi.h
create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpuapi.c
create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpuconfig.h
create mode 100644 drivers/media/platform/chips-media/wave5/wave5-vpuerror.h
create mode 100644 drivers/media/platform/chips-media/wave5/wave5.h

diff --git a/drivers/media/platform/chips-media/wave5/wave5-hw.c b/drivers/media/platform/chips-media/wave5/wave5-hw.c
new file mode 100644
index 000000000000..25705e61cdb3
--- /dev/null
+++ b/drivers/media/platform/chips-media/wave5/wave5-hw.c
@@ -0,0 +1,3359 @@
+// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
+/*
+ * Wave5 series multi-standard codec IP - wave5 backend logic
+ *
+ * Copyright (C) 2021 CHIPS&MEDIA INC
+ */
+
+#include <linux/iopoll.h>
+#include "wave5-vpu.h"
+#include "wave5.h"
+#include "wave5-regdefine.h"
+
+#define FIO_TIMEOUT 10000000

FIO_TIMEOUT_US looks better :-)

+#define FIO_CTRL_READY BIT(31)
+#define FIO_CTRL_WRITE BIT(16)
+#define VPU_BUSY_CHECK_TIMEOUT 10000000
+#define QUEUE_REPORT_MASK 0xffff
+
+static void wave5_print_reg_err(struct vpu_device *vpu_dev, u32 reg_fail_reason)
+{
+ char *caller = __builtin_return_address(0);
+ struct device *dev = vpu_dev->dev;
+ u32 reg_val;
+
+ switch (reg_fail_reason) {
+ case WAVE5_SYSERR_QUEUEING_FAIL:
+ reg_val = vpu_read_reg(vpu_dev, W5_RET_QUEUE_FAIL_REASON);
+ dev_dbg(dev, "%s: queueing failure: 0x%x\n", caller, reg_val);
+ break;
+ case WAVE5_SYSERR_RESULT_NOT_READY:
+ dev_err(dev, "%s: result not ready: 0x%x\n", caller, reg_fail_reason);
+ break;
+ case WAVE5_SYSERR_ACCESS_VIOLATION_HW:
+ dev_err(dev, "%s: access violation: 0x%x\n", caller, reg_fail_reason);
+ break;
+ case WAVE5_SYSERR_WATCHDOG_TIMEOUT:
+ dev_err(dev, "%s: watchdog timeout: 0x%x\n", caller, reg_fail_reason);
+ break;
+ case WAVE5_SYSERR_BUS_ERROR:
+ dev_err(dev, "%s: bus error: 0x%x\n", caller, reg_fail_reason);
+ break;
+ case WAVE5_SYSERR_DOUBLE_FAULT:
+ dev_err(dev, "%s: double fault: 0x%x\n", caller, reg_fail_reason);
+ break;
+ case WAVE5_SYSERR_VPU_STILL_RUNNING:
+ dev_err(dev, "%s: still running: 0x%x\n", caller, reg_fail_reason);
+ break;
+ case WAVE5_SYSERR_VLC_BUF_FULL:
+ dev_err(dev, "%s: vlc buf full: 0x%x\n", caller, reg_fail_reason);
+ break;
+ default:
+ dev_err(dev, "%s: failure:: 0x%x\n", caller, reg_fail_reason);
+ break;
+ }
+}
+
+static int wave5_wait_fio_readl(struct vpu_device *vpu_dev, u32 addr, u32 val)
+{
+ u32 ctrl;
+ int ret;
+
+ ctrl = addr & 0xffff;
+ wave5_vdi_write_register(vpu_dev, W5_VPU_FIO_CTRL_ADDR, ctrl);
+ ret = read_poll_timeout(wave5_vdi_readl, ctrl, ctrl & FIO_CTRL_READY,
+ 0, FIO_TIMEOUT, false, vpu_dev, W5_VPU_FIO_CTRL_ADDR);
+ if (ret)
+ return ret;
+ if (wave5_vdi_readl(vpu_dev, W5_VPU_FIO_DATA) != val)
+ return -ETIMEDOUT;

Are you sure that this is a timeout?
if (read_data != expected_data) => invalid data => return -EINVAL ?

+ return 0;
+}
+

..snip..

+
+static int wave5_wait_bus_busy(struct vpu_device *vpu_dev, unsigned int addr)
+{
+ u32 gdi_status_check_value = 0x3f;
+
+ if (vpu_dev->product_code == WAVE521C_CODE ||
+ vpu_dev->product_code == WAVE521_CODE ||
+ vpu_dev->product_code == WAVE521E1_CODE)
+ gdi_status_check_value = 0x00ff1f3f;

#define SOME_VALUE 0x3f
#define ANOTHER_VALUE 0xff1f3f

+
+ return wave5_wait_fio_readl(vpu_dev, addr, gdi_status_check_value);
+}
+

..snip..

+
+static int setup_wave5_properties(struct device *dev)
+{
+ struct vpu_device *vpu_dev = dev_get_drvdata(dev);
+ struct vpu_attr *p_attr = &vpu_dev->attr;
+ u32 reg_val;
+ u8 *str;
+ int ret;
+ u32 hw_config_def0, hw_config_def1, hw_config_feature, hw_config_rev;
+
+ vpu_write_reg(vpu_dev, W5_QUERY_OPTION, GET_VPU_INFO);
+ vpu_write_reg(vpu_dev, W5_VPU_BUSY_STATUS, 1);
+ vpu_write_reg(vpu_dev, W5_COMMAND, W5_QUERY);
+ vpu_write_reg(vpu_dev, W5_VPU_HOST_INT_REQ, 1);
+ ret = wave5_wait_vpu_busy(vpu_dev, W5_VPU_BUSY_STATUS);
+ if (ret)
+ return ret;
+
+ if (!vpu_read_reg(vpu_dev, W5_RET_SUCCESS))
+ return -EIO;
+
+ reg_val = vpu_read_reg(vpu_dev, W5_RET_PRODUCT_NAME);
+ str = (u8 *)&reg_val;
+ p_attr->product_name[0] = str[3];
+ p_attr->product_name[1] = str[2];
+ p_attr->product_name[2] = str[1];
+ p_attr->product_name[3] = str[0];
+ p_attr->product_name[4] = 0;
+
+ p_attr->product_id = wave5_vpu_get_product_id(vpu_dev);
+ p_attr->product_version = vpu_read_reg(vpu_dev, W5_RET_PRODUCT_VERSION);
+ p_attr->fw_version = vpu_read_reg(vpu_dev, W5_RET_FW_VERSION);
+ p_attr->customer_id = vpu_read_reg(vpu_dev, W5_RET_CUSTOMER_ID);
+ hw_config_def0 = vpu_read_reg(vpu_dev, W5_RET_STD_DEF0);
+ hw_config_def1 = vpu_read_reg(vpu_dev, W5_RET_STD_DEF1);
+ hw_config_feature = vpu_read_reg(vpu_dev, W5_RET_CONF_FEATURE);
+ hw_config_rev = vpu_read_reg(vpu_dev, W5_RET_CONF_REVISION);
+
+ p_attr->support_hevc10bit_enc = (hw_config_feature >> 3) & 1;

This looks like being BIT(3), and the latter is BIT(11)...

#define W5_CONF_FEATURE_HEVC10_ENC BIT(3)
#define W5_CONF_FEATURE_AVC10_ENC BIT(11)

p_attr->support_hevc10bit_enc = FIELD_GET(W5_CONF_FEATURE_HEVC10_ENC, hw_config_feature);

if (hw_config_rev > W5_REVISION_SOMETHING)
p_attr->support_avc10bit_enc = FIELD_GET(W5_CONF_FEATURE_AVC10_ENC,
hw_config_feature);
else
p_attr->support_avc10bit_enc = p_attr->support_hevc10bit_enc;


+ if (hw_config_rev > 167455) //20190321
+ p_attr->support_avc10bit_enc = (hw_config_feature >> 11) & 1;
+ else
+ p_attr->support_avc10bit_enc = p_attr->support_hevc10bit_enc;
+
+ p_attr->support_decoders = 0;
+ p_attr->support_encoders = 0;
+ if (p_attr->product_id == PRODUCT_ID_521) {
+ p_attr->support_dual_core = ((hw_config_def1 >> 26) & 0x01);

p_attr->support_dual_core = FIELD_GET(W5_CONF_DEF_HW_DUAL_CORE, hw_config_def1);

....and there are others below, but I think I gave enough examples... :-)

+ if (p_attr->support_dual_core || hw_config_rev < 206116) {
+ p_attr->support_decoders = BIT(STD_AVC);
+ p_attr->support_decoders |= BIT(STD_HEVC);
+ p_attr->support_encoders = BIT(STD_AVC);
+ p_attr->support_encoders |= BIT(STD_HEVC);
+ } else {
+ p_attr->support_decoders |= (((hw_config_def1 >> 3) & 0x01) << STD_AVC);
+ p_attr->support_decoders |= (((hw_config_def1 >> 2) & 0x01) << STD_HEVC);
+ p_attr->support_encoders = (((hw_config_def1 >> 1) & 0x01) << STD_AVC);
+ p_attr->support_encoders |= ((hw_config_def1 & 0x01) << STD_HEVC);
+ }
+ } else if (p_attr->product_id == PRODUCT_ID_511) {
+ p_attr->support_decoders = BIT(STD_HEVC);
+ p_attr->support_decoders |= BIT(STD_AVC);
+ } else if (p_attr->product_id == PRODUCT_ID_517) {
+ p_attr->support_decoders = (((hw_config_def1 >> 4) & 0x01) << STD_AV1);
+ p_attr->support_decoders |= (((hw_config_def1 >> 3) & 0x01) << STD_AVS2);
+ p_attr->support_decoders |= (((hw_config_def1 >> 2) & 0x01) << STD_AVC);
+ p_attr->support_decoders |= (((hw_config_def1 >> 1) & 0x01) << STD_VP9);
+ p_attr->support_decoders |= ((hw_config_def1 & 0x01) << STD_HEVC);
+ }
+
+ p_attr->support_backbone = (hw_config_def0 >> 16) & 0x01;
+ p_attr->support_vcpu_backbone = (hw_config_def0 >> 28) & 0x01;
+ p_attr->support_vcore_backbone = (hw_config_def0 >> 22) & 0x01;
+ p_attr->support_dual_core = (hw_config_def1 >> 26) & 0x01;
+ p_attr->support_endian_mask = BIT(VDI_LITTLE_ENDIAN) |
+ BIT(VDI_BIG_ENDIAN) |
+ BIT(VDI_32BIT_LITTLE_ENDIAN) |
+ BIT(VDI_32BIT_BIG_ENDIAN) |
+ (0xffffUL << 16);
+ p_attr->support_bitstream_mode = BIT(BS_MODE_INTERRUPT) |
+ BIT(BS_MODE_PIC_END);
+
+ return 0;
+}
+
+int wave5_vpu_get_version(struct vpu_device *vpu_dev, u32 *revision)
+{
+ u32 reg_val;
+ int ret;
+
+ vpu_write_reg(vpu_dev, W5_QUERY_OPTION, GET_VPU_INFO);
+ vpu_write_reg(vpu_dev, W5_VPU_BUSY_STATUS, 1);
+ vpu_write_reg(vpu_dev, W5_COMMAND, W5_QUERY);
+ vpu_write_reg(vpu_dev, W5_VPU_HOST_INT_REQ, 1);
+ ret = wave5_wait_vpu_busy(vpu_dev, W5_VPU_BUSY_STATUS);
+ if (ret) {
+ dev_err(vpu_dev->dev, "%s: timeout\n", __func__);
+ return ret;
+ }
+
+ if (!vpu_read_reg(vpu_dev, W5_RET_SUCCESS)) {
+ dev_err(vpu_dev->dev, "%s: failed\n", __func__);
+ return -EIO;
+ }
+
+ reg_val = vpu_read_reg(vpu_dev, W5_RET_FW_VERSION);
+ if (revision)

Move the revision pointer null check at the beginning and return an error
if that happens to be null: it doesn't make a lot of sense to read many
registers before the check as the whole point of this function is to get
the version and return it to that variable.


+ *revision = reg_val;
+
+ return 0;
+}
+
+static void remap_page(struct vpu_device *vpu_dev, dma_addr_t code_base, u32 index)
+{
+ u32 remap_size = (W5_REMAP_MAX_SIZE >> 12) & 0x1ff;
+ u32 reg_val = 0x80000000 | (WAVE5_UPPER_PROC_AXI_ID << 20) | (index << 12) | BIT(11)
+ | remap_size;
+
+ vpu_write_reg(vpu_dev, W5_VPU_REMAP_CTRL, reg_val);
+ vpu_write_reg(vpu_dev, W5_VPU_REMAP_VADDR, index * W5_REMAP_MAX_SIZE);
+ vpu_write_reg(vpu_dev, W5_VPU_REMAP_PADDR, code_base + index * W5_REMAP_MAX_SIZE);
+}
+

..snip..

+
+int wave5_vpu_build_up_dec_param(struct vpu_instance *inst,
+ struct dec_open_param *param)
+{
+ int ret;
+ struct dec_info *p_dec_info = &inst->codec_info->dec_info;
+ u32 bs_endian;
+ struct dma_vpu_buf *sram_vb;
+ struct vpu_device *vpu_dev = inst->dev;
+
+ p_dec_info->cycle_per_tick = 256;
+ switch (inst->std) {
+ case W_HEVC_DEC:
+ p_dec_info->seq_change_mask = SEQ_CHANGE_ENABLE_ALL_HEVC;
+ break;
+ case W_VP9_DEC:
+ p_dec_info->seq_change_mask = SEQ_CHANGE_ENABLE_ALL_VP9;
+ break;
+ case W_AVS2_DEC:
+ p_dec_info->seq_change_mask = SEQ_CHANGE_ENABLE_ALL_AVS2;
+ break;
+ case W_AVC_DEC:
+ p_dec_info->seq_change_mask = SEQ_CHANGE_ENABLE_ALL_AVC;
+ break;
+ case W_AV1_DEC:
+ p_dec_info->seq_change_mask = SEQ_CHANGE_ENABLE_ALL_AV1;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (vpu_dev->product == PRODUCT_ID_517)

Another switch would be good here.

+ p_dec_info->vb_work.size = WAVE517_WORKBUF_SIZE;
+ else if (vpu_dev->product == PRODUCT_ID_521)
+ p_dec_info->vb_work.size = WAVE521DEC_WORKBUF_SIZE;
+ else if (vpu_dev->product == PRODUCT_ID_511)
+ p_dec_info->vb_work.size = WAVE521DEC_WORKBUF_SIZE;
+
+ ret = wave5_vdi_allocate_dma_memory(inst->dev, &p_dec_info->vb_work);
+ if (ret)
+ return ret;
+
+ vpu_write_reg(inst->dev, W5_CMD_DEC_VCORE_INFO, 1);
+
+ sram_vb = &vpu_dev->sram_buf;
+ p_dec_info->sec_axi_info.buf_base = sram_vb->daddr;
+ p_dec_info->sec_axi_info.buf_size = sram_vb->size;
+
+ wave5_vdi_clear_memory(inst->dev, &p_dec_info->vb_work);
+
+ vpu_write_reg(inst->dev, W5_ADDR_WORK_BASE, p_dec_info->vb_work.daddr);
+ vpu_write_reg(inst->dev, W5_WORK_SIZE, p_dec_info->vb_work.size);
+
+ vpu_write_reg(inst->dev, W5_CMD_DEC_BS_START_ADDR, p_dec_info->stream_buf_start_addr);
+ vpu_write_reg(inst->dev, W5_CMD_DEC_BS_SIZE, p_dec_info->stream_buf_size);
+
+ /* NOTE: when endian mode is 0, SDMA reads MSB first */
+ bs_endian = wave5_vdi_convert_endian(inst->dev, param->stream_endian);
+ bs_endian = (~bs_endian & VDI_128BIT_ENDIAN_MASK);
+ vpu_write_reg(inst->dev, W5_CMD_BS_PARAM, bs_endian);
+ vpu_write_reg(inst->dev, W5_CMD_EXT_ADDR, (param->pri_axprot << 20) |
+ (param->pri_axcache << 16) | param->pri_ext_addr);
+ vpu_write_reg(inst->dev, W5_CMD_NUM_CQ_DEPTH_M1, (COMMAND_QUEUE_DEPTH - 1));
+ vpu_write_reg(inst->dev, W5_CMD_ERR_CONCEAL, (param->error_conceal_unit << 2) |
+ (param->error_conceal_mode));
+
+ wave5_bit_issue_command(inst, W5_CREATE_INSTANCE);
+ // check QUEUE_DONE

Please be consistent in comments format. Use C-style comments.

+ ret = wave5_wait_vpu_busy(inst->dev, W5_VPU_BUSY_STATUS);
+ if (ret) {
+ dev_warn(inst->dev->dev, "command: 'W5_CREATE_INSTANCE' timed out\n");
+ goto free_vb_work;
+ }
+
+ // Check if we were able to add the parameters into the VCPU QUEUE
+ if (!vpu_read_reg(inst->dev, W5_RET_SUCCESS)) {
+ ret = -EIO;
+ goto free_vb_work;
+ }
+
+ p_dec_info->product_code = vpu_read_reg(inst->dev, W5_PRODUCT_NUMBER);
+
+ return 0;
+free_vb_work:
+ wave5_vdi_free_dma_memory(vpu_dev, &p_dec_info->vb_work);
+ return ret;
+}
+

..snip..

+
+static void wave5_get_dec_seq_result(struct vpu_instance *inst, struct dec_initial_info *info)
+{
+ u32 reg_val, sub_layer_info;
+ u32 profile_compatibility_flag;
+ u32 output_bit_depth_minus8;
+ struct dec_info *p_dec_info = &inst->codec_info->dec_info;
+
+ p_dec_info->stream_rd_ptr = wave5_vpu_dec_get_rd_ptr(inst);
+ info->rd_ptr = p_dec_info->stream_rd_ptr;
+
+ p_dec_info->frame_display_flag = vpu_read_reg(inst->dev, W5_RET_DEC_DISP_IDC);
+
+ reg_val = vpu_read_reg(inst->dev, W5_RET_DEC_PIC_SIZE);
+ info->pic_width = ((reg_val >> 16) & 0xffff);
+ info->pic_height = (reg_val & 0xffff);
+ info->min_frame_buffer_count = vpu_read_reg(inst->dev, W5_RET_DEC_NUM_REQUIRED_FB);
+ info->frame_buf_delay = vpu_read_reg(inst->dev, W5_RET_DEC_NUM_REORDER_DELAY);
+
+ reg_val = vpu_read_reg(inst->dev, W5_RET_DEC_CROP_LEFT_RIGHT);
+ info->pic_crop_rect.left = (reg_val >> 16) & 0xffff;
+ info->pic_crop_rect.right = reg_val & 0xffff;
+ reg_val = vpu_read_reg(inst->dev, W5_RET_DEC_CROP_TOP_BOTTOM);
+ info->pic_crop_rect.top = (reg_val >> 16) & 0xffff;
+ info->pic_crop_rect.bottom = reg_val & 0xffff;
+
+ info->f_rate_numerator = vpu_read_reg(inst->dev, W5_RET_DEC_FRAME_RATE_NR);
+ info->f_rate_denominator = vpu_read_reg(inst->dev, W5_RET_DEC_FRAME_RATE_DR);
+
+ reg_val = vpu_read_reg(inst->dev, W5_RET_DEC_COLOR_SAMPLE_INFO);
+ info->luma_bitdepth = reg_val & 0xf;
+ info->chroma_bitdepth = (reg_val >> 4) & 0xf;
+ info->chroma_format_idc = (reg_val >> 8) & 0xf;
+ info->aspect_rate_info = (reg_val >> 16) & 0xff;

Bitfield macros would make this way more readable.

+ info->is_ext_sar = ((info->aspect_rate_info == 255) ? true : false);
+ /* [0:15] - vertical size, [16:31] - horizontal size */
+ if (info->is_ext_sar)
+ info->aspect_rate_info = vpu_read_reg(inst->dev, W5_RET_DEC_ASPECT_RATIO);
+ info->bit_rate = vpu_read_reg(inst->dev, W5_RET_DEC_BIT_RATE);
+
+ sub_layer_info = vpu_read_reg(inst->dev, W5_RET_DEC_SUB_LAYER_INFO);
+ info->max_temporal_layers = (sub_layer_info >> 8) & 0x7;
+
+ reg_val = vpu_read_reg(inst->dev, W5_RET_DEC_SEQ_PARAM);
+ info->level = reg_val & 0xff;
+ profile_compatibility_flag = (reg_val >> 12) & 0xff;
+ info->profile = (reg_val >> 24) & 0x1f;
+ info->tier = (reg_val >> 29) & 0x01;
+ output_bit_depth_minus8 = (reg_val >> 30) & 0x03;
+
+ if (inst->std == W_HEVC_DEC) {
+ /* guessing profile */
+ if (!info->profile) {
+ if ((profile_compatibility_flag & 0x06) == 0x06)
+ info->profile = HEVC_PROFILE_MAIN; /* main profile */

main/main10 profile comments are stating the obvious, please remove them.

+ else if ((profile_compatibility_flag & 0x04) == 0x04)
+ info->profile = HEVC_PROFILE_MAIN10; /* main10 profile */
+ else if ((profile_compatibility_flag & 0x08) == 0x08)
+ /* main still picture profile */
+ info->profile = HEVC_PROFILE_STILLPICTURE;
+ else
+ info->profile = HEVC_PROFILE_MAIN; /* for old version HM */
+ }
+
+ } else if (inst->std == W_AVS2_DEC) {
+ if (info->luma_bitdepth == 10 && output_bit_depth_minus8 == 2)
+ info->output_bit_depth = 10;
+ else
+ info->output_bit_depth = 8;
+
+ } else if (inst->std == W_AVC_DEC) {
+ info->profile = (reg_val >> 24) & 0x7f;
+ }
+
+ info->vlc_buf_size = vpu_read_reg(inst->dev, W5_RET_VLC_BUF_SIZE);
+ info->param_buf_size = vpu_read_reg(inst->dev, W5_RET_PARAM_BUF_SIZE);
+ p_dec_info->vlc_buf_size = info->vlc_buf_size;
+ p_dec_info->param_buf_size = info->param_buf_size;
+}
+

..snip..

+
+static u32 calculate_table_size(u32 bit_depth, u32 frame_width, u32 frame_height, u32 ot_bg_width)
+{
+ u32 bgs_width = ((bit_depth > 8) ? 256 : 512);
+ u32 comp_frame_width = ALIGN(ALIGN(frame_width, 16) + 16, 16);
+ u32 ot_frame_width = ALIGN(comp_frame_width, ot_bg_width);
+
+ // sizeof_offset_table()
+ u32 ot_bg_height = 32;
+ u32 bgs_height = BIT(14) / bgs_width / ((bit_depth > 8) ? 2 : 1);

Please, no magic BIT(x) usage: add a definition for that bit.

+ u32 comp_frame_height = ALIGN(ALIGN(frame_height, 4) + 4, bgs_height);
+ u32 ot_frame_height = ALIGN(comp_frame_height, ot_bg_height);
+
+ return (ot_frame_width / 16) * (ot_frame_height / 4) * 2;
+}
+

..snip..

+
+int wave5_vpu_decode(struct vpu_instance *inst, struct dec_param *option, u32 *fail_res)
+{
+ u32 mode_option = DEC_PIC_NORMAL, bs_option, reg_val;
+ u32 force_latency = 0;
+ struct dec_info *p_dec_info = &inst->codec_info->dec_info;
+ struct dec_open_param *p_open_param = &p_dec_info->open_param;
+ int ret;
+

switch (option->skipframe_mode) {
case ...
...
default:
break;
};

if (p_dec_info->thumbnail_mode) {
mode_option = DEC_PIC_W_THUMBNAIL;
if (mode_option != DEC_PIC_NORMAL)
... do something: as I read the code, this is not a supported case.
}

^^^^ this should improve the flow.

+ if (p_dec_info->thumbnail_mode) {
+ mode_option = DEC_PIC_W_THUMBNAIL;
+ } else if (option->skipframe_mode) {
+ switch (option->skipframe_mode) {
+ case WAVE_SKIPMODE_NON_IRAP:
+ mode_option = SKIP_NON_IRAP;
+ force_latency = 1;
+ break;
+ case WAVE_SKIPMODE_NON_REF:
+ mode_option = SKIP_NON_REF_PIC;
+ break;
+ default:
+ // skip mode off
+ break;
+ }
+ }
+
+ // set disable reorder
+ if (!p_dec_info->reorder_enable)
+ force_latency = 1;
+
+ /* set attributes of bitstream buffer controller */
+ bs_option = 0;

You don't have to initialize this variable at all, as you're either writing twice
or failing.

+ switch (p_open_param->bitstream_mode) {
+ case BS_MODE_INTERRUPT:
+ bs_option = BSOPTION_ENABLE_EXPLICIT_END;
+ break;
+ case BS_MODE_PIC_END:
+ bs_option = BSOPTION_ENABLE_EXPLICIT_END;
+ break;
+ default:
+ return -EINVAL;
+ }
+

..snip..

+
+int wave5_vpu_re_init(struct device *dev, u8 *fw, size_t size)
+{
+ struct vpu_buf *common_vb;
+ dma_addr_t code_base, temp_base;
+ dma_addr_t old_code_base, temp_size;
+ u32 code_size;
+ u32 reg_val;
+ struct vpu_device *vpu_dev = dev_get_drvdata(dev);
+
+ common_vb = &vpu_dev->common_mem;
+
+ code_base = common_vb->daddr;
+ /* ALIGN TO 4KB */
+ code_size = (WAVE5_MAX_CODE_BUF_SIZE & ~0xfff);
+ if (code_size < size * 2)
+ return -EINVAL;
+ temp_base = common_vb->daddr + WAVE5_TEMPBUF_OFFSET;
+ temp_size = WAVE5_TEMPBUF_SIZE;
+
+ old_code_base = vpu_read_reg(vpu_dev, W5_VPU_REMAP_PADDR);
+
+ if (old_code_base != code_base + W5_REMAP_INDEX1 * W5_REMAP_MAX_SIZE) {

Put the contents of this branch into another function maybe?

if (old_code_base != code_base + W5_REMAP_INDEX1 * W5_REMAP_MAX_SIZE) {
ret = do_that_vpu_init_flow(things);
if (ret)
return ret;
}


return setup_wave5_properties(dev);
};

Alternatively, invert the conditional and use a goto, but I personally don't
really like using gotos unless it's *totally* necessary.

+ int ret;
+ struct dma_vpu_buf *sram_vb;
+
+ ret = wave5_vdi_write_memory(vpu_dev, common_vb, 0, fw, size,
+ VDI_128BIT_LITTLE_ENDIAN);
+ if (ret < 0) {
+ dev_err(vpu_dev->dev,
+ "VPU init, Writing firmware to common buffer, fail: %d\n", ret);
+ return ret;
+ }
+
+ vpu_write_reg(vpu_dev, W5_PO_CONF, 0);
+
+ ret = wave5_vpu_reset(dev, SW_RESET_ON_BOOT);
+ if (ret < 0) {
+ dev_err(vpu_dev->dev, "VPU init, Resetting the VPU, fail: %d\n", ret);
+ return ret;
+ }
+
+ remap_page(vpu_dev, code_base, W5_REMAP_INDEX0);
+ remap_page(vpu_dev, code_base, W5_REMAP_INDEX1);
+
+ vpu_write_reg(vpu_dev, W5_ADDR_CODE_BASE, code_base);
+ vpu_write_reg(vpu_dev, W5_CODE_SIZE, code_size);
+ vpu_write_reg(vpu_dev, W5_CODE_PARAM, (WAVE5_UPPER_PROC_AXI_ID << 4) | 0);
+ vpu_write_reg(vpu_dev, W5_ADDR_TEMP_BASE, temp_base);
+ vpu_write_reg(vpu_dev, W5_TEMP_SIZE, temp_size);
+
+ vpu_write_reg(vpu_dev, W5_HW_OPTION, 0);
+
+ reg_val = (WAVE5_PROC_AXI_EXT_ADDR & 0xFFFF);
+ wave5_fio_writel(vpu_dev, W5_BACKBONE_PROC_EXT_ADDR, reg_val);
+ reg_val = ((WAVE5_PROC_AXI_AXPROT & 0x7) << 4) |
+ (WAVE5_PROC_AXI_AXCACHE & 0xF);
+ wave5_fio_writel(vpu_dev, W5_BACKBONE_AXI_PARAM, reg_val);
+ reg_val = ((WAVE5_SEC_AXI_AXPROT & 0x7) << 20) |
+ ((WAVE5_SEC_AXI_AXCACHE & 0xF) << 16) |
+ (WAVE5_SEC_AXI_EXT_ADDR & 0xFFFF);
+ vpu_write_reg(vpu_dev, W5_SEC_AXI_PARAM, reg_val);
+
+ /* interrupt */
+ // encoder
+ reg_val = BIT(INT_WAVE5_ENC_SET_PARAM);
+ reg_val |= BIT(INT_WAVE5_ENC_PIC);
+ reg_val |= BIT(INT_WAVE5_BSBUF_FULL);
+ // decoder
+ reg_val |= BIT(INT_WAVE5_INIT_SEQ);
+ reg_val |= BIT(INT_WAVE5_DEC_PIC);
+ reg_val |= BIT(INT_WAVE5_BSBUF_EMPTY);
+ vpu_write_reg(vpu_dev, W5_VPU_VINT_ENABLE, reg_val);
+
+ reg_val = vpu_read_reg(vpu_dev, W5_VPU_RET_VPU_CONFIG0);
+ if ((reg_val >> 16) & 1) {
+ reg_val = ((WAVE5_PROC_AXI_ID << 28) |
+ (WAVE5_PRP_AXI_ID << 24) |
+ (WAVE5_FBD_Y_AXI_ID << 20) |
+ (WAVE5_FBC_Y_AXI_ID << 16) |
+ (WAVE5_FBD_C_AXI_ID << 12) |
+ (WAVE5_FBC_C_AXI_ID << 8) |
+ (WAVE5_PRI_AXI_ID << 4) |
+ WAVE5_SEC_AXI_ID);
+ wave5_fio_writel(vpu_dev, W5_BACKBONE_PROG_AXI_ID, reg_val);
+ }
+
+ sram_vb = &vpu_dev->sram_buf;
+
+ vpu_write_reg(vpu_dev, W5_ADDR_SEC_AXI, sram_vb->daddr);
+ vpu_write_reg(vpu_dev, W5_SEC_AXI_SIZE, sram_vb->size);
+ vpu_write_reg(vpu_dev, W5_VPU_BUSY_STATUS, 1);
+ vpu_write_reg(vpu_dev, W5_COMMAND, W5_INIT_VPU);
+ vpu_write_reg(vpu_dev, W5_VPU_REMAP_CORE_START, 1);
+
+ ret = wave5_wait_vpu_busy(vpu_dev, W5_VPU_BUSY_STATUS);
+ if (ret) {
+ dev_err(vpu_dev->dev, "VPU reinit(W5_VPU_REMAP_CORE_START) timeout\n");
+ return ret;
+ }
+
+ reg_val = vpu_read_reg(vpu_dev, W5_RET_SUCCESS);
+ if (!reg_val) {
+ u32 reason_code = vpu_read_reg(vpu_dev, W5_RET_FAIL_REASON);
+
+ wave5_print_reg_err(vpu_dev, reason_code);
+ return -EIO;
+ }
+ }
+
+ return setup_wave5_properties(dev);
+}
+

..snip..

+
+int wave5_vpu_reset(struct device *dev, enum sw_reset_mode reset_mode)
+{
+ u32 val = 0;
+ int ret = 0;
+ struct vpu_device *vpu_dev = dev_get_drvdata(dev);
+ struct vpu_attr *p_attr = &vpu_dev->attr;
+ // VPU doesn't send response. force to set BUSY flag to 0.
+ vpu_write_reg(vpu_dev, W5_VPU_BUSY_STATUS, 0);
+
+ if (reset_mode == SW_RESET_SAFETY) {
+ ret = wave5_vpu_sleep_wake(dev, true, NULL, 0);
+ if (ret)
+ return ret;
+ }
+
+ val = vpu_read_reg(vpu_dev, W5_VPU_RET_VPU_CONFIG0);
+ if ((val >> 16) & 0x1)
+ p_attr->support_backbone = true;

bitfield macros ftw.

+ if ((val >> 22) & 0x1)
+ p_attr->support_vcore_backbone = true;
+ if ((val >> 28) & 0x1)
+ p_attr->support_vcpu_backbone = true;
+
+ val = vpu_read_reg(vpu_dev, W5_VPU_RET_VPU_CONFIG1);
+ if ((val >> 26) & 0x1)
+ p_attr->support_dual_core = true;
+


..snip..

+static void wave5_set_enc_crop_info(u32 codec, struct enc_wave_param *param, int rot_mode,
+ int src_width, int src_height)
+{
+ int aligned_width = (codec == W_HEVC_ENC) ? ALIGN(src_width, 32) : ALIGN(src_width, 16);
+ int aligned_height = (codec == W_HEVC_ENC) ? ALIGN(src_height, 32) : ALIGN(src_height, 16);
+ int pad_right, pad_bot;
+ int crop_right, crop_left, crop_top, crop_bot;
+ int prp_mode = rot_mode >> 1; // remove prp_enable bit
+
+ if (codec == W_HEVC_ENC &&
+ (!rot_mode || prp_mode == 14)) // prp_mode 14 : hor_mir && ver_mir && rot_180
+ return;
+
+ pad_right = aligned_width - src_width;
+ pad_bot = aligned_height - src_height;
+
+ if (param->conf_win_right > 0)
+ crop_right = param->conf_win_right + pad_right;
+ else
+ crop_right = pad_right;
+
+ if (param->conf_win_bot > 0)
+ crop_bot = param->conf_win_bot + 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_bot = crop_bot;
+ param->conf_win_right = crop_right;
+
+ if (prp_mode == 1 || prp_mode == 15) {

#define PRP_MODE_SOMETHING 1
#define PRP_MODE_SOMETHING_ELSE 2

or use an enumeration... otherwise it's not really understandable...

+ param->conf_win_top = crop_right;
+ param->conf_win_left = crop_top;
+ param->conf_win_bot = crop_left;
+ param->conf_win_right = crop_bot;
+ } else if (prp_mode == 2 || prp_mode == 12) {
+ param->conf_win_top = crop_bot;
+ param->conf_win_left = crop_right;
+ param->conf_win_bot = crop_top;
+ param->conf_win_right = crop_left;
+ } else if (prp_mode == 3 || prp_mode == 13) {
+ param->conf_win_top = crop_left;
+ param->conf_win_left = crop_bot;
+ param->conf_win_bot = crop_right;
+ param->conf_win_right = crop_top;
+ } else if (prp_mode == 4 || prp_mode == 10) {
+ param->conf_win_top = crop_bot;
+ param->conf_win_bot = crop_top;
+ } else if (prp_mode == 8 || prp_mode == 6) {
+ param->conf_win_left = crop_right;
+ param->conf_win_right = crop_left;
+ } else if (prp_mode == 5 || prp_mode == 11) {
+ param->conf_win_top = crop_left;
+ param->conf_win_left = crop_top;
+ param->conf_win_bot = crop_right;
+ param->conf_win_right = crop_bot;
+ } else if (prp_mode == 7 || prp_mode == 9) {
+ param->conf_win_top = crop_right;
+ param->conf_win_left = crop_bot;
+ param->conf_win_bot = crop_left;
+ param->conf_win_right = crop_top;
+ }
+}
+
+int wave5_vpu_enc_init_seq(struct vpu_instance *inst)
+{
+ u32 reg_val = 0, rot_mir_mode, fixed_cu_size_mode = 0x7;
+ struct enc_info *p_enc_info = &inst->codec_info->enc_info;
+ struct enc_open_param *p_open_param = &p_enc_info->open_param;
+ struct enc_wave_param *p_param = &p_open_param->wave_param;
+ int ret;
+
+ if (inst->dev->product != PRODUCT_ID_521)
+ return -EINVAL;
+
+ /*==============================================*/
+ /* OPT_CUSTOM_GOP */
+ /*==============================================*/

Comments like these are usually like

/*
* OPT_CUSTOM_GOP
*
* SET_PARAM + CUSTOM_GOP
* only when... blah
*/

+ /*
+ * SET_PARAM + CUSTOM_GOP
+ * only when gop_preset_idx == custom_gop, custom_gop related registers should be set
+ */

..snip..

+}
diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
new file mode 100644
index 000000000000..1b3ffb737925
--- /dev/null
+++ b/drivers/media/platform/chips-media/wave5/wave5-vpuapi.h
@@ -0,0 +1,1136 @@
+/* SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause) */
+/*
+ * Wave5 series multi-standard codec IP - helper definitions
+ *
+ * Copyright (C) 2021 CHIPS&MEDIA INC
+ */
+
+#ifndef VPUAPI_H_INCLUDED
+#define VPUAPI_H_INCLUDED
+
+#include <linux/kfifo.h>
+#include <linux/idr.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-mem2mem.h>
+#include <media/v4l2-ctrls.h>
+#include "wave5-vpuerror.h"
+#include "wave5-vpuconfig.h"
+#include "wave5-vdi.h"
+
+enum product_id {
+ PRODUCT_ID_521,
+ PRODUCT_ID_511,
+ PRODUCT_ID_517,
+ PRODUCT_ID_NONE,
+};
+
+struct vpu_attr;
+
+enum vpu_instance_type {
+ VPU_INST_TYPE_DEC = 0,

The default for the first enum entry is always zero, and the next one will always
be 1, 2, 3, 4.....

.....so you don't need to assign any number.

+ VPU_INST_TYPE_ENC = 1
+};
+
+enum vpu_instance_state {
+ VPU_INST_STATE_NONE = 0,
+ VPU_INST_STATE_OPEN = 1,
+ VPU_INST_STATE_INIT_SEQ = 2,
+ VPU_INST_STATE_PIC_RUN = 3,
+ VPU_INST_STATE_STOP = 4

ditto

+};
+
+#define WAVE5_MAX_FBS 32
+
+#define MAX_REG_FRAME (WAVE5_MAX_FBS * 2)
+
+#define WAVE5_DEC_HEVC_BUF_SIZE(_w, _h) (DIV_ROUND_UP(_w, 64) * DIV_ROUND_UP(_h, 64) * 256 + 64)
+#define WAVE5_DEC_AVC_BUF_SIZE(_w, _h) ((((ALIGN(_w, 256) / 16) * (ALIGN(_h, 16) / 16)) + 16) * 80)
+#define WAVE5_DEC_VP9_BUF_SIZE(_w, _h) (((ALIGN(_w, 64) * ALIGN(_h, 64)) >> 2))
+#define WAVE5_DEC_AVS2_BUF_SIZE(_w, _h) (((ALIGN(_w, 64) * ALIGN(_h, 64)) >> 5))
+// AV1 BUF SIZE : MFMV + segment ID + CDF probs table + film grain param Y+ film graim param C
+#define WAVE5_DEC_AV1_BUF_SZ_1(_w, _h) \
+ (((ALIGN(_w, 64) / 64) * (ALIGN(_h, 64) / 64) * 512) + 41984 + 8192 + 4864)
+#define WAVE5_DEC_AV1_BUF_SZ_2(_w1, _w2, _h) \
+ (((ALIGN(_w1, 64) / 64) * 256 + (ALIGN(_w2, 256) / 64) * 128) * (ALIGN(_h, 64) / 64))
+
+#define WAVE5_FBC_LUMA_TABLE_SIZE(_w, _h) (ALIGN(_h, 64) * ALIGN(_w, 256) / 32)
+#define WAVE5_FBC_CHROMA_TABLE_SIZE(_w, _h) (ALIGN((_h), 64) * ALIGN((_w) / 2, 256) / 32)
+#define WAVE5_ENC_AVC_BUF_SIZE(_w, _h) (ALIGN(_w, 64) * ALIGN(_h, 64) / 32)
+#define WAVE5_ENC_HEVC_BUF_SIZE(_w, _h) (ALIGN(_w, 64) / 64 * ALIGN(_h, 64) / 64 * 128)
+
+/*
+ * common struct and definition
+ */
+enum cod_std {
+ STD_AVC = 0,
+ STD_VC1 = 1,
+ STD_MPEG2 = 2,
+ STD_MPEG4 = 3,
+ STD_H263 = 4,
+ STD_DIV3 = 5,
+ STD_RV = 6,
+ STD_AVS = 7,

and same here, so that becomes

.....
STD_AVS,
STD_RESERVED,
STD_THO,


+ STD_THO = 9 > + STD_VP3 = 10,
+ STD_VP8 = 11,
+ STD_HEVC = 12,
+ STD_VP9 = 13,
+ STD_AVS2 = 14,

STD_RESERVED2, (which will be 15)...

+ STD_AV1 = 16,
+ STD_MAX
+};
+
+enum wave_std {
+ W_HEVC_DEC = 0x00,
+ W_HEVC_ENC = 0x01,
+ W_AVC_DEC = 0x02,
+ W_AVC_ENC = 0x03,
+ W_VP9_DEC = 0x16,
+ W_AVS2_DEC = 0x18,
+ W_AV1_DEC = 0x1A,
+ STD_UNKNOWN = 0xFF
+};
+
+enum SET_PARAM_OPTION {

Lowercase names for enums please.

+ OPT_COMMON = 0, /* SET_PARAM command option for encoding sequence */
+ OPT_CUSTOM_GOP = 1, /* SET_PARAM command option for setting custom GOP */
+ OPT_CUSTOM_HEADER = 2, /* SET_PARAM command option for setting custom VPS/SPS/PPS */
+ OPT_VUI = 3, /* SET_PARAM command option for encoding VUI */
+ OPT_CHANGE_PARAM = 0x10,
+};
+
+enum DEC_PIC_HDR_OPTION {
+ INIT_SEQ_NORMAL = 0x01,
+ INIT_SEQ_W_THUMBNAIL = 0x11,
+};
+
+enum DEC_PIC_OPTION {
+ DEC_PIC_NORMAL = 0x00, /* it is normal mode of DEC_PIC command */
+ DEC_PIC_W_THUMBNAIL = 0x10, /* thumbnail mode (skip non-IRAP without reference reg) */
+ SKIP_NON_IRAP = 0x11, /* it skips to decode non-IRAP pictures */
+ SKIP_NON_REF_PIC = 0x13
+};
+


There's probably more, but starting with that is surely something :-)


Regards,
Angelo