Re: [PATCH v2 1/2] media: qcom: iris: Add gop size support for gen1 encoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Undocumented addition of dynamic allowed flag to `IR_PERIOD` capability.

Pre-existing issues:
- [High] The driver lacks a `.try_ctrl` implementation to validate dependencies between controls, resulting in silent hardware configuration failures for valid V4L2 inputs like `gop_size == 0`.
--

commit 0331fb3580ba43928b379274c9bd300bf884fc44
Author: Wangao Wang <wangao.wang@oss.qualcomm.com>

media: qcom: iris: Add gop size support for gen1 encoder

This commit adds support for GOP size configuration on gen1 encoders by
enabling V4L2_CID_MPEG_VIDEO_GOP_SIZE and routing it to the hardware handler.

> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1.c
> index ca1545d28b531..eff9216bb5014 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1.c
> @@ -171,7 +171,9 @@ static const struct platform_inst_fw_cap inst_fw_cap_sm8250_enc[] = {
>  		.max = (1 << 16) - 1,
>  		.step_or_mask = 1,
>  		.value = 30,
> -		.set = iris_set_u32
> +		.hfi_id = HFI_PROPERTY_CONFIG_VENC_INTRA_PERIOD,
> +		.flags = CAP_FLAG_OUTPUT_PORT,
> +		.set = iris_set_intra_period,
>  	},

[Severity: High]
This isn't a bug introduced by this patch, but this patch explicitly wires up
the GOP_SIZE control to the hardware for Gen1, bringing a pre-existing silent
failure path into active use.

The V4L2 specification defines V4L2_CID_MPEG_VIDEO_GOP_SIZE = 0 as meaning
"only I-frames are used". The driver exposes .min = 0 for this control,
allowing userspace to request it.

However, the hardware handler explicitly rejects a 0 gop_size:

iris_set_intra_period() {
    ...
    if (!gop_size || b_frame >= gop_size)
        return -EINVAL;
    ...
}

During streamon, the driver iterates over capabilities and applies them to
the firmware, but ignores any returned errors:

iris_set_properties() {
    ...
    if (cap->cap_id && cap->set)
        cap->set(inst, i);
    ...
}

Because the return value is ignored, the error is swallowed and user space
receives a success response, but the property is never dispatched to the
firmware. Could this cause the encoder hardware to operate with out-of-sync
default GOP settings instead of the requested all-I-frames configuration?

[ ... ]

> @@ -240,7 +242,7 @@ static const struct platform_inst_fw_cap inst_fw_cap_sm8250_enc[] = {
>  		.step_or_mask = 1,
>  		.value = 0,
>  		.hfi_id = HFI_PROPERTY_PARAM_VENC_INTRA_REFRESH,
> -		.flags = CAP_FLAG_OUTPUT_PORT,
> +		.flags = CAP_FLAG_OUTPUT_PORT | CAP_FLAG_DYNAMIC_ALLOWED,
>  		.set = iris_set_ir_period_gen1,
>  	},

[Severity: Medium]
Does this modification belong in this patch?

The commit message strictly states that it adds support for GOP size
configuration on gen1 encoders. However, this line modifies the IR_PERIOD
(Intra Refresh Period) capability to include the CAP_FLAG_DYNAMIC_ALLOWED
flag.

Modifying intra-refresh dynamic behavior appears to be a distinct functional
change that isn't mentioned in the commit message. Could this be split into
a separate commit or mentioned in the commit message?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-dynamic_encode-v2-0-6e97df032482@oss.qualcomm.com?part=1