Re: [PATCH v8 14/18] media: iris: implement support for the Agatti platform

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Mismatch between statically calculated vb2 buffer size and the firmware's requested buffer size causes a potential buffer overflow when the firmware writes to external buffers.
- [High] Querying buffer requirements for AR50Lt overwrites the DPB buffer count (fw_min_count) with the display output buffer count.
- [High] The AR50LT platform uses `iris_vpu2_calculate_frequency` but provides capability fields intended for `iris_vpu3x_vpu4x_calculate_frequency`, resulting in severely under-calculated clock scaling.
- [Medium] The hierarchical layer bitrates (LAYER0_BITRATE_H264 to LAYER5_BITRATE_H264) in `inst_fw_cap_gen1_ar50lt_enc` use the global `BITRATE_MAX` (160Mbps) instead of the AR50LT-specific limit.
--

--- Patch [14]: [PATCH v8 14/18] media: iris: implement support for the Agatti platform ---
commit 50e74e4ea9f26a2d05a3a8800b3cde717ea582a2
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

media: iris: implement support for the Agatti platform

Port support for the AR50Lt video codec core (present for example on the
Agatti platform) to the Iris driver. Unlike more recent cores this
generation doesn't have the PIPE property (as it always has only one
pipe). Also, unlike newer platforms, buffer sizes are requested from the
firmware instead of being calculated by the driver.

> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1.c
> index ca1545d28b531..020f55092737d 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1.c

[ ... ]

> +	{
> +		.cap_id = LAYER0_BITRATE_H264,
> +		.min = 1,
> +		.max = BITRATE_MAX,
> +		.step_or_mask = 1,
> +		.value = BITRATE_DEFAULT,
> +		.hfi_id = HFI_PROPERTY_CONFIG_VENC_TARGET_BITRATE,
> +		.flags = CAP_FLAG_OUTPUT_PORT | CAP_FLAG_INPUT_PORT |
> +			CAP_FLAG_DYNAMIC_ALLOWED,
> +		.set = iris_set_bitrate_gen1,
> +	},

[Severity: Medium]
Should these hierarchical layers use BITRATE_MAX_AR50LT instead of
BITRATE_MAX?

The base BITRATE capability uses BITRATE_MAX_AR50LT (100M), but the
hierarchical layers are using BITRATE_MAX (160M), which might allow
userspace to request a configuration that exceeds the platform's hardware
limits.

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> index 99e82e5510abe..329a9ef2fdc5f 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c

[ ... ]

> @@ -970,6 +970,13 @@ static int iris_hfi_gen1_set_bufsize(struct iris_inst *inst, u32 plane)
>  	struct hfi_buffer_size_actual bufsz;
>  	int ret;
>  
> +	if (inst->core->iris_firmware_data->query_buffer_requirements) {
> +		ret = inst->hfi_session_ops->session_get_property(inst,
> +								  HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS);
> +		if (ret)
> +			return ret;
> +	}
> +

[Severity: High]
Could querying buffer requirements here overwrite the DPB buffer count
with the display buffer count?

When this queries HFI_PROPERTY_CONFIG_BUFFER_REQUIREMENTS,
iris_hfi_gen1_session_buffer_requirements() processes the requirement.
For BUF_OUTPUT (which maps to the display buffer in split mode), it sets:

    if (type == BUF_OUTPUT)
        inst->fw_min_count = req->count_actual;

Since iris_vpu_dpb_count() relies on inst->fw_min_count for the DPB
count, does this result in allocating the wrong number of DPB buffers?

[Severity: Critical]
Could this lead to a buffer overflow when the firmware writes to
the external buffers?

When the buffer requirements are queried,
iris_hfi_gen1_session_buffer_requirements() overwrites the internal tracker:

    inst->buffers[type].size = req->size;

This firmware-requested size is then sent back to the firmware. However,
when physical memory is prepared in iris_vb2_buf_prepare(), it validates the
size against the statically calculated iris_get_buffer_size():

    if (vb2_plane_size(vb, 0) < iris_get_buffer_size(inst, BUF_OUTPUT))
        return -EINVAL;

If the firmware requires a larger size than the static calculation, it
might write beyond the statically allocated vb2_buffer bounds.

[ ... ]

> diff --git a/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c b/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c
> new file mode 100644
> index 0000000000000..92f7e25465399
> --- /dev/null
> +++ b/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c

[ ... ]

> +static struct platform_inst_caps platform_inst_cap_ar50lt = {
> +	.min_frame_width = 128,
> +	.max_frame_width = 1920,
> +	.min_frame_height = 128,
> +	.max_frame_height = 1920,
> +	.max_mbpf = (1920 * 1088) / 256,
> +	.mb_cycles_vpp = 440,
> +	.mb_cycles_fw = 733003,
> +	.mb_cycles_fw_vpp = 225975,
> +	.max_frame_rate = 120,
> +	.max_operating_rate = 120,
> +};

[Severity: High]
Does this platform need mb_cycles_vsp defined to scale the clock properly?

This struct provides mb_cycles_fw and mb_cycles_fw_vpp, which are used by
iris_vpu3x_vpu4x_calculate_frequency(). However, qcm2290_data assigns
iris_vpu_ar50lt_ops, which uses iris_vpu2_calculate_frequency():

    vsp_freq = mbs_per_second * caps->mb_cycles_vsp;

Since mb_cycles_vsp is omitted here, it defaults to 0. Will this cause the
calculated vsp_freq to be 0 and severely under-calculate the required clock
frequency?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-iris-ar50lt-v8-0-6597c08cea6f@oss.qualcomm.com?part=14