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

[Wangao Wang <wangao.wang@oss.qualcomm.com>]

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