lkml.org 
[lkml]   [2022]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v11 2/6] media: chips-media: wave5: Add vpuapi layer
From
Il 07/12/22 13:13, Sebastian Fricke ha scritto:
> From: Nas Chung <nas.chung@chipsnmedia.com>
>
> 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@collabora.com>
> Signed-off-by: Robert Beckett <bob.beckett@collabora.com>
> Signed-off-by: Sebastian Fricke <sebastian.fricke@collabora.com>
> Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>
> ---
> .../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

\
 
 \ /
  Last update: 2022-12-07 14:07    [W:0.070 / U:0.736 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site