Re: [PATCH v5 12/16] media: iris: implement support for the Agatti platform

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Logic Error: Invalid hardware frequency calculation due to missing `mb_cycles_vsp` parameter
- [High] Hardware Configuration: The watchdog interrupt is defined but never unmasked, breaking watchdog recovery
- [Low] Build Failure: Initialization of non-existent struct members in qcm2290_data
--

commit eabd242734902d122adf5442b6b1946c0a61380b
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_platform_vpu_ar50lt.c b/drivers/media/platform/qcom/iris/iris_platform_vpu_ar50lt.c
> new file mode 100644
> index 0000000000000..393256f39112b
> --- /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,

[Severity: High]
Does this code need to initialize .mb_cycles_vsp?

It looks like .mb_cycles_vsp is omitted here, so it will default to 0. If
the .calc_freq callback in iris_vpu_ar50lt_ops uses
iris_vpu2_calculate_frequency(), that function calculates vsp_freq by
multiplying the macroblocks-per-second by caps->mb_cycles_vsp. Because
mb_cycles_vsp is 0, the resulting hardware clock frequency might be severely
undercalculated, potentially causing performance degradation or hardware
timeouts.

> +	.max_frame_rate = 120,
> +	.max_operating_rate = 120,
> +};
> +
> +const struct iris_platform_data qcm2290_data = {
> +	.firmware_desc_gen1 = &iris_vpu_ar50lt_p1_gen1_s6_desc,

[Severity: Low]
Does struct iris_platform_data have a member named firmware_desc_gen1?

It appears that struct iris_platform_data (defined in
iris_platform_common.h) only contains a generic firmware_desc member. Using
.firmware_desc_gen1 here might result in a compilation error.

> +	.vpu_ops = &iris_vpu_ar50lt_ops,
> +	.icc_tbl = iris_icc_info_ar50lt,
> +	.icc_tbl_size = ARRAY_SIZE(iris_icc_info_ar50lt),

[ ... ]

> +	.tz_cp_config_data_size = ARRAY_SIZE(tz_cp_config_ar50lt),
> +	.num_vpp_pipe = 1,
> +	.no_rpmh = true,
> +	.wd_intr_mask = WRAPPER_INTR_STATUS_A2HWD_BMSK,

[Severity: High]
Will the watchdog interrupt actually be unmasked by the hardware?

This defines the platform's watchdog status bit mask as
WRAPPER_INTR_STATUS_A2HWD_BMSK (0x10). However, if
iris_vpu_ar50lt_interrupt_init() in iris_vpu_ar50lt.c overwrites
WRAPPER_INTR_MASK with a hardcoded mask that omits the watchdog bit, the
watchdog interrupt will not be unmasked, preventing the driver from
detecting and recovering from VPU firmware hangs.

> +	.icc_ib_multiplier = 2,
> +	.max_session_count = 8,
> +	.max_core_mbpf = ((1920 * 1088) / 256) * 4,
> +	/* Concurrency: 1080p@30 decode + 1080p@30 encode */
> +	/* Concurrency: 3 * 1080p@30 decode */
> +	.max_core_mbps = (((1920 * 1088) / 256) * 90),
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616-iris-ar50lt-v5-0-583b42770b6a@oss.qualcomm.com?part=12