Re: [PATCH v2 2/2] media: qcom: iris: Add request key frame support for encoder

[sashiko-bot@kernel.org]

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

New issues:
- [High] In Gen1, the payload for `HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME` is discarded and the packet size is not incremented, resulting in a truncated firmware command packet.
- [Medium] The driver unconditionally requests a sync frame on stream initialization, bypassing V4L2 button control semantics.

Pre-existing issues:
- [High] Concurrent modification of the shared `inst_hfi_gen2->packet` buffer causes HFI command packet corruption.
--

commit 69576d77651c69239afd97ac72082948d7e40043
Author: Wangao Wang <wangao.wang@oss.qualcomm.com>

media: qcom: iris: Add request key frame support for encoder

This commit adds support for requesting key frames for both gen1 and gen2
encoders by enabling the V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME control.

> diff --git a/drivers/media/platform/qcom/iris/iris_ctrls.c b/drivers/media/platform/qcom/iris/iris_ctrls.c
> index 391e1fc5f6e2e..2918a7df0b403 100644
> --- a/drivers/media/platform/qcom/iris/iris_ctrls.c
> +++ b/drivers/media/platform/qcom/iris/iris_ctrls.c
[ ... ]
> @@ -1477,6 +1481,24 @@ int iris_set_layer_bitrate(struct iris_inst *inst, enum platform_inst_fw_cap_typ
>  					     &bitrate, sizeof(u32));
>  }
>  
> +int iris_set_req_sync_frame(struct iris_inst *inst, enum platform_inst_fw_cap_type cap_id)
> +{
> +	const struct iris_hfi_session_ops *hfi_ops = inst->hfi_session_ops;
> +	u32 hfi_id = inst->fw_caps[cap_id].hfi_id;
> +	u32 hfi_val = 0;
> +
> +	if (inst->fw_caps[PREPEND_SPSPPS_TO_IDR].value)
> +		hfi_val = HFI_SYNC_FRAME_REQUEST_WITH_PREFIX_SEQ_HDR;
> +	else
> +		hfi_val = HFI_SYNC_FRAME_REQUEST_WITHOUT_SEQ_HDR;
> +
> +	return hfi_ops->session_set_property(inst, hfi_id,
> +					     HFI_HOST_FLAGS_NONE,
> +					     iris_get_port_info(inst, cap_id),
> +					     HFI_PAYLOAD_U32_ENUM,
> +					     &hfi_val, sizeof(u32));
> +}

[Severity: Medium]
Will this unconditionally request a sync frame on stream initialization?

During stream initialization, iris_set_properties() iterates over all fw_caps
and invokes their .set callbacks. Because this function lacks a check for
inst->fw_caps[cap_id].flags & CAP_FLAG_CLIENT_SET (like bitrate does), it
seems it will unconditionally issue a REQUEST_SYNC_FRAME command to the
firmware on every stream start, bypassing V4L2 button control semantics.

> +
>  int iris_set_properties(struct iris_inst *inst, u32 plane)
[ ... ]
> 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 7674b47ad6c49..d7fd867cf6c98 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> @@ -741,6 +741,9 @@ iris_hfi_gen1_packet_session_set_property(struct hfi_session_set_property_pkt *p
>  		packet->shdr.hdr.size += sizeof(u32);
>  		break;
>  	}
> +	case HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME: {
> +		break;
> +	}

[Severity: High]
Does this result in a truncated HFI packet?

In iris_hfi_gen1_packet_session_set_property(), the switch statement for
HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME simply breaks without writing
the 4-byte payload or incrementing packet->shdr.hdr.size by sizeof(u32).
This omits the payload entirely, which may cause the firmware to reject the
request with a parameter error.

>  	default:
>  		return -EINVAL;
>  	}
[ ... ]
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2.c b/drivers/media/platform/qcom/iris/iris_hfi_gen2.c
> index acc0ed8adda1a..df8e70b133a12 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2.c
> @@ -937,7 +937,17 @@ static const struct platform_inst_fw_cap inst_fw_cap_sm8550_enc[] = {
>  		.flags = CAP_FLAG_OUTPUT_PORT | CAP_FLAG_INPUT_PORT |
>  			CAP_FLAG_DYNAMIC_ALLOWED,
>  		.set = iris_set_layer_bitrate,
> -	}
> +	},
> +	{
> +		.cap_id = REQUEST_SYNC_FRAME,
> +		.min = 0,
> +		.max = 0,
> +		.step_or_mask = 0,
> +		.value = 0,
> +		.hfi_id = HFI_PROP_REQUEST_SYNC_FRAME,
> +		.flags = CAP_FLAG_INPUT_PORT | CAP_FLAG_DYNAMIC_ALLOWED,
> +		.set = iris_set_req_sync_frame,
> +	},

[Severity: High]
This is a pre-existing issue, but does adding this dynamic control expose a
race condition where the shared packet buffer could be corrupted?

In Gen2, properties and commands are built using a single, per-instance
dynamically allocated buffer inst_hfi_gen2->packet. The V4L2 control
framework allows dynamic controls, such as the newly added
V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME, to be set during streaming via
VIDIOC_S_CTRL.

The s_ctrl handler is protected only by the control handler's internal lock
(ctrl_handler.lock), while queue-related ioctls like VIDIOC_QBUF and
VIDIOC_STREAMON are protected by the queue lock (inst->ctx_q_lock). 

When s_ctrl and qbuf are executed concurrently, they can both invoke Gen2 HFI
functions which write headers and payload data directly into the shared
inst_hfi_gen2->packet. Since there is no common lock protecting
inst_hfi_gen2->packet prior to its submission in iris_hfi_queue_cmd_write(),
this could result in data races and corrupted HFI packets sent to the firmware.

>  };

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