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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Integer underflow when calculating end index for frame buffer registration causes out-of-bounds hardware configuration.
- [High] Encoder frame buffer registration fails to group configuration commands, allowing array index overflow into adjacent MMIO registers.
- [Medium] Silent truncation of RGB chroma format due to mismatched bitmask.
- [Medium] Display buffers are silently ignored when the metadata array is full.
- [Medium] Hardcoded little-endian memory layout breaks compatibility with big-endian host architectures.
--

commit 5d00a062981a74f9de3d4d2750b7f43dba64822e
Author: Nas Chung <nas.chung@chipsnmedia.com>

media: chips-media: wave6: Add Wave6 VPU interface

Add an interface layer to manage hardware register configuration and
communication with the Chips&Media Wave6 video codec IP.

> diff --git a/drivers/media/platform/chips-media/wave6/wave6-hw.c b/drivers/media/platform/chips-media/wave6/wave6-hw.c
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-hw.c
> @@ -0,0 +1,2030 @@
[ ... ]
> +static void wave6_dec_set_display_buffer(struct vpu_instance *inst, struct frame_buffer fb)
> +{
> +	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;
> +			break;
> +		}
> +	}
> +}

[Severity: Medium]
WAVE6_MAX_FBS is defined as 31. If userspace requests and registers 32
buffers, what happens when this loop completes without finding an empty
slot?

Does this silently drop the tracking metadata for the buffer, leading to
failures later in wave6_vpu_dec_get_output_info() when it tries to match
the display buffer address?

[ ... ]
> +int wave6_vpu_dec_register_frame_buffer(struct vpu_instance *inst)
> +{
[ ... ]
> +	fbc_remain = fbc_count;
> +	mv_remain = mv_count;
> +	group_num = DIV_ROUND_UP(max(fbc_count, mv_count), 16);
> +	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;

[Severity: High]
If either fbc_count or mv_count is initially 0, or if they are uneven
and one depletes before the other in subsequent loop iterations,
set_fbc_num or set_mv_num becomes 0.

Does this cause fbc_end_no or mv_end_no to underflow when subtracting 1,
sending invalid boundaries to the hardware?

[ ... ]
> +int wave6_vpu_dec_register_display_buffer(struct vpu_instance *inst, struct frame_buffer fb)
> +{
[ ... ]
> +	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) |

[Severity: Medium]
When the format is RGB, get_chroma_format_idc() returns C_FMT_IDC_RGB
which evaluates to 4 (binary 100).

Since SET_DISP_COMMON_PIC_INFO_C_FMT_IDC uses a 2-bit mask, does this
silently truncate the value to 0 (C_FMT_IDC_YUV400) and misconfigure
the hardware for RGB displays?

[ ... ]
> +int wave6_vpu_enc_register_frame_buffer(struct vpu_instance *inst)
> +{
[ ... ]
> +	for (idx = 0; idx < fbc_count; idx++) {
> +		vpu_write_reg(inst->dev, W6_CMD_SET_FB_FBC_Y(idx), fb_arr[idx].buf_y);
> +		vpu_write_reg(inst->dev, W6_CMD_SET_FB_FBC_C(idx), fb_arr[idx].buf_cb);
> +		vpu_write_reg(inst->dev, W6_CMD_SET_FB_FBC_CR(idx), fb_arr[idx].buf_cr);

[Severity: High]
Unlike the decoder buffer registration which batches commands into groups
of 16, this loop writes sequentially up to fbc_count.

If fbc_count exceeds 16, could this overflow past the 16 available MMIO
slots for W6_CMD_SET_FB_FBC_Y (and others) and corrupt adjacent hardware
registers like W6_CMD_SET_FB_DEFAULT_CDF?

[ ... ]
> diff --git a/drivers/media/platform/chips-media/wave6/wave6-regdefine.h b/drivers/media/platform/chips-media/wave6/wave6-regdefine.h
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-regdefine.h
> @@ -0,0 +1,649 @@
[ ... ]
> +#define		SET_DISP_COMMON_PIC_INFO_C_FMT_IDC(x)	FIELD_PREP(GENMASK(25, 24), (x))

[Severity: Medium]
As noted in wave6_vpu_dec_register_display_buffer(), this mask is only
2 bits wide. Could this be extended to correctly support C_FMT_IDC_RGB
(which requires at least 3 bits to store value 4)?

[ ... ]
> diff --git a/drivers/media/platform/chips-media/wave6/wave6-vpuconfig.h b/drivers/media/platform/chips-media/wave6/wave6-vpuconfig.h
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-vpuconfig.h
> @@ -0,0 +1,72 @@
[ ... ]
> +#define HOST_ENDIAN			VDI_128BIT_LITTLE_ENDIAN
> +#define VPU_STREAM_ENDIAN		HOST_ENDIAN
> +#define VPU_USER_DATA_ENDIAN		HOST_ENDIAN
> +#define VPU_SOURCE_ENDIAN		HOST_ENDIAN

[Severity: Medium]
Does hardcoding the host endianness to VDI_128BIT_LITTLE_ENDIAN break
stream decoding and encoding on big-endian architectures?

Could this be set conditionally using standard kernel endianness macros
to support big-endian hosts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788496816.git.nas.chung@chipsnmedia.com?part=3