Re: [PATCH v3 4/6] media: qcom: iris: Add hierarchical coding support for encoder

[Wangao Wang]

On 2026/1/22 17:38, Dikshita Agarwal wrote:
> > @@ -116,6 +116,40 @@ static enum platform_inst_fw_cap_type iris_get_cap_id(u32 id)
> >   		return MARK_LTR;
> >   	case V4L2_CID_MPEG_VIDEO_B_FRAMES:
> >   		return B_FRAME;
> > +	case V4L2_CID_MPEG_VIDEO_H264_HIERARCHICAL_CODING:
> > +		return LAYER_ENABLE;
>
> Will the same control be used for HEVC as well? I think this is applicable
> for only H264 encoders.

H264 flow:
V4L2_CID_MPEG_VIDEO_H264_HIERARCHICAL_CODING,
V4L2_CID_MPEG_VIDEO_H264_HIERARCHICAL_CODING_TYPE,
V4L2_CID_MPEG_VIDEO_H264_HIERARCHICAL_CODING_LAYER
HEVC flow:
V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_TYPE,
V4L2_CID_MPEG_VIDEO_HEVC_HIER_CODING_LAYER

LAYER_ENABLE is used for H.264. In the HEVC flow, this flag is 
considered redundant—once the type is set, layer encoding is implicitly 
enabled.

> > +int iris_set_bitrate_gen1(struct iris_inst *inst, enum platform_inst_fw_cap_type cap_id)
> > +{
> > +	const struct iris_hfi_command_ops *hfi_ops = inst->core->hfi_ops;
> > +	u32 entropy_mode = inst->fw_caps[ENTROPY_MODE].value;
> > +	u32 bitrate = inst->fw_caps[cap_id].value;
> > +	u32 hfi_id = inst->fw_caps[cap_id].hfi_id;
> > +	struct hfi_bitrate hfi_val;
> > +	u32 max_bitrate;
> > +
> > +	if (!(inst->fw_caps[cap_id].flags & CAP_FLAG_CLIENT_SET) && cap_id != BITRATE)
> > +		return -EINVAL;
>
> Can you pls explain what is this check for?
>
> The layer bitrate should only be set if layer encoding is enabled, isn't it?

This flag is used to confirm whether the corresponding ctrl has been 
invoked. A check should also be added to determine whether layer 
encoding is enabled, while excluding bitrate configuration in 
non–layer‑encoding scenarios.

> > +	if (inst->codec == V4L2_PIX_FMT_H264) {
> > +		if (!layer_enable || !inst->fw_caps[LAYER_COUNT_H264].value)
> > +			return -EINVAL;
> > +
> > +		if (inst->fw_caps[LAYER_TYPE_H264].value ==
> > +			V4L2_MPEG_VIDEO_H264_HIERARCHICAL_CODING_P) {
> > +			if (inst->hfi_rc_type == HFI_RC_VBR_CFR)
> > +				layer_type = HFI_HIER_P_HYBRID_LTR;
> > +			else
> > +				layer_type = HFI_HIER_P_SLIDING_WINDOW;
> > +		} else if (inst->fw_caps[LAYER_TYPE_HEVC].value ==
> > +			V4L2_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_B) {
>
> why are you checking HEVC layer type for H264 codec? seems like a bug.

This is a bug and will be fixed in v4.

> > +int iris_set_layer_count_gen1(struct iris_inst *inst, enum platform_inst_fw_cap_type cap_id)
> > +{
> > +	const struct iris_hfi_command_ops *hfi_ops = inst->core->hfi_ops;
> > +	struct vb2_queue *sq = v4l2_m2m_get_src_vq(inst->m2m_ctx);
> > +	struct vb2_queue *dq = v4l2_m2m_get_dst_vq(inst->m2m_ctx);
> > +	u32 layer_enable = inst->fw_caps[LAYER_ENABLE].value;
> > +	u32 layer_count = inst->fw_caps[cap_id].value;
> > +	u32 hfi_id, ret;
> > +
> > +	if (!layer_enable || !layer_count)
> > +		return -EINVAL;
> > +
> > +	inst->hfi_layer_count = layer_count;
> > +
> > +	if (!vb2_is_streaming(sq) && !vb2_is_streaming(dq)) {
> > +		hfi_id = HFI_PROPERTY_PARAM_VENC_HIER_P_MAX_NUM_ENH_LAYER;
>
> why the streaming check? and what's the significance of this setting? why
> this prop is set under streaming check?

This property needs to be set to the firmware before streaming. It 
represents the maximum layer count and is static; any dynamically 
configured layer count later must not exceed this maximum.

> > +
> > +int iris_set_layer_bitrate(struct iris_inst *inst, enum platform_inst_fw_cap_type cap_id)
> > +{
> > +	const struct iris_hfi_command_ops *hfi_ops = inst->core->hfi_ops;
> > +	u32 hfi_id = inst->fw_caps[cap_id].hfi_id;
> > +	u32 bitrate = inst->fw_caps[cap_id].value;
> > +
> > +	/* ignore layer bitrate when total bitrate is set */
> > +	if (inst->fw_caps[BITRATE].flags & CAP_FLAG_CLIENT_SET)
> > +		return 0;
> > +
>
> any streaming check required here?

A streaming check will be added here.

> > +	{
> > +		.cap_id = LAYER_TYPE_HEVC,
> > +		.min = V4L2_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_B,
> > +		.max = V4L2_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_P,
> > +		.step_or_mask = BIT(V4L2_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_B) |
> > +				BIT(V4L2_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_P),
> > +		.value = V4L2_MPEG_VIDEO_HEVC_HIERARCHICAL_CODING_P,
> > +		.hfi_id = HFI_PROP_LAYER_ENCODING_TYPE,
> > +		.flags = CAP_FLAG_OUTPUT_PORT | CAP_FLAG_MENU,
> > +		.set = iris_set_layer_type,
>
> layer type depends on layer count so shouldn't you have layer count before
> layer type in caps? or handle both in same set API?

In the iris_set_layer_type API, there is a check for the layer count. If 
the count is 0, layer type will not be set to the firmware.

> > +	/*
> > +	 * The expression (1 << layers - 2) + 1 accounts for the number of reference
> > +	 * frames in the Adaptive Hierarchical B-frame encoding case. In this scheme,
> > +	 * the number of frames in a sub-GOP is related to (2^(number of layers) - 1),
> > +	 * hence the use of the shift operation.
> > +	 */
> > +	if (layer_type == HFI_HIER_B) {
> > +		if (inst->codec == V4L2_PIX_FMT_HEVC)
> > +			num_ref	= layer_count;
> > +		else
> > +			num_ref = (1 << (layer_count - 2)) + 1;
> > +	}
>
> were you able to test these different scenarios?

Okay, I will test the other scenarios.

--
Best Regards,
Wangao