Re: [PATCH v3 2/2] media: iris: add new rate control type MBR for encoder

[Nicolas Dufresne <nicolas@ndufresne.ca>]

[-- Attachment #1: Type: text/plain, Size: 12072 bytes --]

Hi,

Le vendredi 30 octobre 2026 à 12:02 +0530, Sachin Kumar Garg a écrit :
> Add support for the MBR rate-control mode on the encoder.
> MBR has no corresponding V4L2-spec definition, it is exposed as a
> Qualcomm IRIS vendor-specific control rather than a generic
> bitrate-mode value.
> 
> Add a new boolean control, V4L2_CID_QCOM_MBR_RATE_CONTROL, based on
> V4L2_CID_USER_QCOM_IRIS_BASE, to enable it. When set, the driver

Vendor specific does not mean undocumented. Please update the Documentation
folder with precise description. Upstreaming is about doing things right, if you
want to secretly add feature, do this in your BSP.

We use vendor control when we don't have enough data to generalize. But
documentation should be sufficient to understand what the feature does and be
able to later enhance the control and generalize for other vendor / hardware. It
is in no mean an excuse for a vendor to ignore publicly available information
from other vendors or to hide details strictly needed by final users.

In this state, this is a NACK.

Nicolas

> programs the firmware with the MBR rate-control mode, taking priority
> over V4L2_CID_MPEG_VIDEO_BITRATE_MODE when both are client-set.
> 
> MBR is currently supported only on sc7280. Rather than duplicating
> the encoder capability table per platform, add a mbr_rc_supported
> flag to struct iris_platform_data, and merge in a small
> platform-specific capability table containing only the MBR cap for
> platforms that support it.
> 
> Signed-off-by: Sachin Kumar Garg <sachin.garg@oss.qualcomm.com>
> ---
>  drivers/media/platform/qcom/iris/iris_ctrls.c      | 101 +++++++++++++++++----
>  drivers/media/platform/qcom/iris/iris_ctrls.h      |   4 +
>  .../platform/qcom/iris/iris_hfi_gen1_command.c     |   1 +
>  .../platform/qcom/iris/iris_hfi_gen1_defines.h     |   1 +
>  .../platform/qcom/iris/iris_hfi_gen2_defines.h     |   1 +
>  .../platform/qcom/iris/iris_platform_common.h      |   2 +
>  .../media/platform/qcom/iris/iris_platform_vpu2.c  |   1 +
>  7 files changed, 94 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/iris/iris_ctrls.c b/drivers/media/platform/qcom/iris/iris_ctrls.c
> index 4f2633bbb49f..626239952b96 100644
> --- a/drivers/media/platform/qcom/iris/iris_ctrls.c
> +++ b/drivers/media/platform/qcom/iris/iris_ctrls.c
> @@ -163,6 +163,8 @@ static enum platform_inst_fw_cap_type iris_get_cap_id(u32 id)
>  		return SLICE_MAX_BYTES;
>  	case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MAX_MB:
>  		return SLICE_MAX_MB;
> +	case V4L2_CID_QCOM_MBR_RATE_CONTROL:
> +		return MBR_RATE_CONTROL;
>  	default:
>  		return INST_FW_CAP_MAX;
>  	}
> @@ -314,6 +316,8 @@ static u32 iris_get_v4l2_id(enum platform_inst_fw_cap_type cap_id)
>  		return V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MAX_BYTES;
>  	case SLICE_MAX_MB:
>  		return V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MAX_MB;
> +	case MBR_RATE_CONTROL:
> +		return V4L2_CID_QCOM_MBR_RATE_CONTROL;
>  	default:
>  		return 0;
>  	}
> @@ -352,6 +356,17 @@ static const struct v4l2_ctrl_ops iris_ctrl_ops = {
>  	.s_ctrl = iris_op_s_ctrl,
>  };
>  
> +static const struct v4l2_ctrl_config iris_mbr_rate_control_ctrl_config = {
> +	.ops = &iris_ctrl_ops,
> +	.id = V4L2_CID_QCOM_MBR_RATE_CONTROL,
> +	.name = "MBR Rate Control",
> +	.type = V4L2_CTRL_TYPE_BOOLEAN,
> +	.min = 0,
> +	.max = 1,
> +	.step = 1,
> +	.def = 0,
> +};
> +
>  int iris_ctrls_init(struct iris_inst *inst)
>  {
>  	struct platform_inst_fw_cap *cap = &inst->fw_caps[0];
> @@ -385,7 +400,11 @@ int iris_ctrls_init(struct iris_inst *inst)
>  			goto error;
>  		}
>  
> -		if (cap[idx].flags & CAP_FLAG_MENU) {
> +		if (v4l2_id == V4L2_CID_QCOM_MBR_RATE_CONTROL) {
> +			ctrl = v4l2_ctrl_new_custom(&inst->ctrl_handler,
> +						    &iris_mbr_rate_control_ctrl_config,
> +						    NULL);
> +		} else if (cap[idx].flags & CAP_FLAG_MENU) {
>  			ctrl = v4l2_ctrl_new_std_menu(&inst->ctrl_handler,
>  						      &iris_ctrl_ops,
>  						      v4l2_id,
> @@ -428,6 +447,38 @@ int iris_ctrls_init(struct iris_inst *inst)
>  	return ret;
>  }
>  
> +static const struct platform_inst_fw_cap inst_fw_cap_sc7280_only_enc[] = {
> +	{
> +		.cap_id = MBR_RATE_CONTROL,
> +		.min = 0,
> +		.max = 1,
> +		.step_or_mask = 1,
> +		.value = 0,
> +		.flags = CAP_FLAG_OUTPUT_PORT,
> +	},
> +};
> +
> +static void iris_copy_enc_caps(struct iris_core *core, const struct platform_inst_fw_cap *caps,
> +			       u32 num_cap)
> +{
> +	u32 i, cap_id;
> +
> +	for (i = 0; i < num_cap; i++) {
> +		cap_id = caps[i].cap_id;
> +		if (!iris_valid_cap_id(cap_id))
> +			continue;
> +
> +		core->inst_fw_caps_enc[cap_id].cap_id = caps[i].cap_id;
> +		core->inst_fw_caps_enc[cap_id].min = caps[i].min;
> +		core->inst_fw_caps_enc[cap_id].max = caps[i].max;
> +		core->inst_fw_caps_enc[cap_id].step_or_mask = caps[i].step_or_mask;
> +		core->inst_fw_caps_enc[cap_id].value = caps[i].value;
> +		core->inst_fw_caps_enc[cap_id].flags = caps[i].flags;
> +		core->inst_fw_caps_enc[cap_id].hfi_id = caps[i].hfi_id;
> +		core->inst_fw_caps_enc[cap_id].set = caps[i].set;
> +	}
> +}
> +
>  void iris_session_init_caps(struct iris_core *core)
>  {
>  	const struct platform_inst_fw_cap *caps;
> @@ -461,23 +512,12 @@ void iris_session_init_caps(struct iris_core *core)
>  		}
>  	}
>  
> -	caps = core->iris_firmware_data->inst_fw_caps_enc;
> -	num_cap = core->iris_firmware_data->inst_fw_caps_enc_size;
> +	iris_copy_enc_caps(core, core->iris_firmware_data->inst_fw_caps_enc,
> +			   core->iris_firmware_data->inst_fw_caps_enc_size);
>  
> -	for (i = 0; i < num_cap; i++) {
> -		cap_id = caps[i].cap_id;
> -		if (!iris_valid_cap_id(cap_id))
> -			continue;
> -
> -		core->inst_fw_caps_enc[cap_id].cap_id = caps[i].cap_id;
> -		core->inst_fw_caps_enc[cap_id].min = caps[i].min;
> -		core->inst_fw_caps_enc[cap_id].max = caps[i].max;
> -		core->inst_fw_caps_enc[cap_id].step_or_mask = caps[i].step_or_mask;
> -		core->inst_fw_caps_enc[cap_id].value = caps[i].value;
> -		core->inst_fw_caps_enc[cap_id].flags = caps[i].flags;
> -		core->inst_fw_caps_enc[cap_id].hfi_id = caps[i].hfi_id;
> -		core->inst_fw_caps_enc[cap_id].set = caps[i].set;
> -	}
> +	if (core->iris_platform_data->mbr_rc_supported)
> +		iris_copy_enc_caps(core, inst_fw_cap_sc7280_only_enc,
> +				   ARRAY_SIZE(inst_fw_cap_sc7280_only_enc));
>  }
>  
>  static u32 iris_get_port_info(struct iris_inst *inst,
> @@ -777,6 +817,27 @@ int iris_set_peak_bitrate(struct iris_inst *inst, enum platform_inst_fw_cap_type
>  				     &peak_bitrate, sizeof(u32));
>  }
>  
> +static int iris_set_mbr_rate_control(struct iris_inst *inst, u32 rc_mode)
> +{
> +	const struct iris_hfi_session_ops *hfi_ops = inst->hfi_session_ops;
> +	u32 hfi_id = inst->fw_caps[BITRATE_MODE].hfi_id;
> +
> +	inst->hfi_rc_type = rc_mode;
> +
> +	return hfi_ops->session_set_property(inst, hfi_id,
> +					     HFI_HOST_FLAGS_NONE,
> +					     iris_get_port_info(inst, BITRATE_MODE),
> +					     HFI_PAYLOAD_U32_ENUM,
> +					     &rc_mode, sizeof(u32));
> +}
> +
> +static bool iris_mbr_rate_control_enabled(struct iris_inst *inst)
> +{
> +	/* MBR_RATE_CONTROL takes priority over BITRATE_MODE when both are client-set */
> +	return (inst->fw_caps[MBR_RATE_CONTROL].flags & CAP_FLAG_CLIENT_SET) &&
> +		inst->fw_caps[MBR_RATE_CONTROL].value;
> +}
> +
>  int iris_set_bitrate_mode_gen1(struct iris_inst *inst, enum platform_inst_fw_cap_type cap_id)
>  {
>  	const struct iris_hfi_session_ops *hfi_ops = inst->hfi_session_ops;
> @@ -786,6 +847,9 @@ int iris_set_bitrate_mode_gen1(struct iris_inst *inst, enum platform_inst_fw_cap
>  	u32 hfi_id = inst->fw_caps[cap_id].hfi_id;
>  	u32 rc_mode = 0;
>  
> +	if (iris_mbr_rate_control_enabled(inst))
> +		return iris_set_mbr_rate_control(inst, HFI_RATE_CONTROL_MBR);
> +
>  	if (!frame_rc)
>  		rc_mode = HFI_RATE_CONTROL_OFF;
>  	else if (bitrate_mode == V4L2_MPEG_VIDEO_BITRATE_MODE_VBR)
> @@ -813,6 +877,9 @@ int iris_set_bitrate_mode_gen2(struct iris_inst *inst, enum platform_inst_fw_cap
>  	u32 hfi_id = inst->fw_caps[cap_id].hfi_id;
>  	u32 rc_mode = 0;
>  
> +	if (iris_mbr_rate_control_enabled(inst))
> +		return iris_set_mbr_rate_control(inst, HFI_RC_MBR);
> +
>  	if (!frame_rc)
>  		rc_mode = HFI_RC_OFF;
>  	else if (bitrate_mode == V4L2_MPEG_VIDEO_BITRATE_MODE_VBR)
> diff --git a/drivers/media/platform/qcom/iris/iris_ctrls.h b/drivers/media/platform/qcom/iris/iris_ctrls.h
> index 08db80744420..a170ef33a0ac 100644
> --- a/drivers/media/platform/qcom/iris/iris_ctrls.h
> +++ b/drivers/media/platform/qcom/iris/iris_ctrls.h
> @@ -6,8 +6,12 @@
>  #ifndef __IRIS_CTRLS_H__
>  #define __IRIS_CTRLS_H__
>  
> +#include <linux/videodev2.h>
> +
>  #include "iris_platform_common.h"
>  
> +#define V4L2_CID_QCOM_MBR_RATE_CONTROL (V4L2_CID_USER_QCOM_IRIS_BASE + 0)
> +
>  struct iris_core;
>  struct iris_inst;
>  
> 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 47e9147a9b6d..50d38fe89792 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> @@ -621,6 +621,7 @@ iris_hfi_gen1_packet_session_set_property(struct hfi_session_set_property_pkt *p
>  		case HFI_RATE_CONTROL_VBR_CFR:
>  		case HFI_RATE_CONTROL_VBR_VFR:
>  		case HFI_RATE_CONTROL_CQ:
> +		case HFI_RATE_CONTROL_MBR:
>  			break;
>  		default:
>  			return -EINVAL;
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_defines.h b/drivers/media/platform/qcom/iris/iris_hfi_gen1_defines.h
> index 6981e6e84fd8..33c238a1c9bb 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_defines.h
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_defines.h
> @@ -132,6 +132,7 @@
>  #define HFI_RATE_CONTROL_VBR_CFR		0x1000003
>  #define HFI_RATE_CONTROL_CBR_VFR		0x1000004
>  #define HFI_RATE_CONTROL_CBR_CFR		0x1000005
> +#define HFI_RATE_CONTROL_MBR			0x1000006
>  #define HFI_RATE_CONTROL_CQ				0x1000008
>  
>  #define HFI_H264_ENTROPY_CAVLC			0x1
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h b/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h
> index 2b8b057d35a9..cac2f3225754 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_defines.h
> @@ -64,6 +64,7 @@ enum hfi_rate_control {
>  	HFI_RC_OFF		= 0x00000003,
>  	HFI_RC_CBR_VFR		= 0x00000004,
>  	HFI_RC_LOSSLESS		= 0x00000005,
> +	HFI_RC_MBR		= 0x00000006,
>  };
>  
>  #define HFI_PROP_RATE_CONTROL			0x0300012a
> diff --git a/drivers/media/platform/qcom/iris/iris_platform_common.h b/drivers/media/platform/qcom/iris/iris_platform_common.h
> index cddab9a9c375..c150dbb559f0 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_common.h
> +++ b/drivers/media/platform/qcom/iris/iris_platform_common.h
> @@ -211,6 +211,7 @@ enum platform_inst_fw_cap_type {
>  	SLICE_MODE,
>  	SLICE_MAX_BYTES,
>  	SLICE_MAX_MB,
> +	MBR_RATE_CONTROL,
>  	INST_FW_CAP_MAX,
>  };
>  
> @@ -348,6 +349,7 @@ struct iris_platform_data {
>  	u32 num_vpp_pipe;
>  	bool no_aon;
>  	bool no_rpmh;
> +	bool mbr_rc_supported;
>  	u32 wd_intr_mask;
>  	u32 icc_ib_multiplier;
>  	u32 max_session_count;
> diff --git a/drivers/media/platform/qcom/iris/iris_platform_vpu2.c b/drivers/media/platform/qcom/iris/iris_platform_vpu2.c
> index becbe3776544..35a5c42c6ab7 100644
> --- a/drivers/media/platform/qcom/iris/iris_platform_vpu2.c
> +++ b/drivers/media/platform/qcom/iris/iris_platform_vpu2.c
> @@ -132,6 +132,7 @@ const struct iris_platform_data sc7280_data = {
>  	.tz_cp_config_data_size = ARRAY_SIZE(tz_cp_config_vpu2),
>  	.num_vpp_pipe = 1,
>  	.no_aon = true,
> +	.mbr_rc_supported = true,
>  	.wd_intr_mask = WRAPPER_INTR_STATUS_A2HWD_BMSK,
>  	.icc_ib_multiplier = 1,
>  	.max_session_count = 16,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]