Re: [PATCH v7 15/18] media: iris: Introduce buffer size calculations for AR50LT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] AR50LT decoder line buffer calculations fail to allocate required VPSS (Video Processing Subsystem) line buffers when OPB (Output Picture Buffer) is enabled.
- [Medium] AR50LT VP9 line buffer size calculation omits key sub-buffer components (`fe_top_ctrl`, `recon_dma_metadata_wr`, and `qp`) present in other codecs.
--

--- Patch [15]: [PATCH v7 15/18] media: iris: Introduce buffer size calculations for AR50LT ---
commit 706f1cfb849daea9c7ee9eb5866f06341b85640c
Author: Dikshita Agarwal <dikshita.agarwal@oss.qualcomm.com>

media: iris: Introduce buffer size calculations for AR50LT

Introduces AR50LT buffer size calculation for both encoder and
decoder. Reuse the buffer size calculation which are common, while
adding the AR50LT specific ones separately.

diff --git a/drivers/media/platform/qcom/iris/iris_vpu_buffer.c b/drivers/media/platform/qcom/iris/iris_vpu_buffer.c
--- a/drivers/media/platform/qcom/iris/iris_vpu_buffer.c
+++ b/drivers/media/platform/qcom/iris/iris_vpu_buffer.c

[ ... ]

> @@ -571,6 +571,25 @@ u32 hfi_buffer_line_h265d(u32 frame_width, u32 frame_height, bool is_opb, u32 nu
>  	return ALIGN((_size + vpss_lb_size), DMA_ALIGNMENT);
>  }
>  
> +static inline
> +u32 hfi_buffer_line_h265d_ar50lt(u32 frame_width, u32 frame_height)
> +{
> +	u32 size;
> +
> +	size = ALIGN(size_h265d_lb_fe_top_data_ar50lt(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_fe_top_ctrl(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_fe_left_ctrl(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_se_left_ctrl_ar50lt(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_se_top_ctrl(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_pe_top_data_ar50lt(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_vsp_top(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_vsp_left(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_h265d_lb_recon_dma_metadata_wr(frame_width, frame_height), DMA_ALIGNMENT) * 4 +
> +		ALIGN(size_h265d_qp(frame_width, frame_height), DMA_ALIGNMENT);
> +
> +	return ALIGN(size, DMA_ALIGNMENT);
> +}
> +

[Severity: Medium]
Does this code need to handle the VPSS line buffer allocation when the
Output Picture Buffer (OPB) is enabled?

The baseline functions like hfi_buffer_line_h265d() check for is_opb
and add size_vpss_lb(frame_width, frame_height) to the total allocation.
Could the AR50LT hardware experience an out-of-bounds memory write
during decoding if the VPSS line buffer size is omitted here?

[ ... ]

> @@ -689,6 +689,17 @@ u32 hfi_iris3_vp9d_lb_size(u32 frame_width, u32 frame_height, u32 num_vpp_pipes)
>  		ALIGN(size_vp9d_qp(frame_width, frame_height), DMA_ALIGNMENT);
>  }
>  
> +static inline
> +u32 hfi_buffer_line_vp9d_ar50lt(u32 frame_width, u32 frame_height, u32 _yuv_bufcount_min)
> +{
> +	return ALIGN(size_vpxd_lb_fe_left_ctrl(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_vpxd_lb_se_left_ctrl_ar50lt(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_vp9d_lb_vsp_top(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_vpxd_lb_se_top_ctrl(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_vp9d_lb_pe_top_data(frame_width, frame_height), DMA_ALIGNMENT) +
> +		ALIGN(size_vp9d_lb_fe_top_data(frame_width, frame_height), DMA_ALIGNMENT);
> +}
> +

[Severity: Medium]
Are there some missing sub-buffer sizes in this VP9 line buffer calculation?

Looking at the baseline hfi_iris3_vp9d_lb_size(), as well as the AR50LT
H.264 and H.265 calculations, they seem to include sizes for fe_top_ctrl,
recon_dma_metadata_wr, and qp. If the AR50LT hardware requires these
buffers for VP9, could omitting them cause out-of-bounds memory writes?

[ ... ]

> @@ -1100,6 +1100,23 @@ static u32 iris_vpu_dec_line_size(struct iris_inst *inst)
>  	return 0;
>  }
>  
> +static u32 iris_vpu_ar50lt_dec_line_size(struct iris_inst *inst)
> +{
> +	struct v4l2_format *f = inst->fmt_src;
> +	u32 height = f->fmt.pix_mp.height;
> +	u32 width = f->fmt.pix_mp.width;
> +	u32 out_min_count = inst->buffers[BUF_OUTPUT].min_count;
> +
> +	if (inst->codec == V4L2_PIX_FMT_H264)
> +		return hfi_buffer_line_h264d_ar50lt(width, height);
> +	else if (inst->codec == V4L2_PIX_FMT_HEVC)
> +		return hfi_buffer_line_h265d_ar50lt(width, height);
> +	else if (inst->codec == V4L2_PIX_FMT_VP9)
> +		return hfi_buffer_line_vp9d_ar50lt(width, height, out_min_count);
> +
> +	return 0;
> +}
> +

[Severity: Medium]
Should this function check for iris_split_mode_enabled() to determine OPB
state, similar to how iris_vpu_dec_line_size() does?

If the VPSS line buffer size allocation is required for AR50LT when OPB
is enabled, it seems the is_opb flag would need to be passed down to the
buffer calculations here as well to prevent buffer under-allocation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709-iris-ar50lt-v7-0-76af9dd4d1f6@oss.qualcomm.com?part=15