[2/2] media: venus: add new rate control type MBR for encoder

Message ID 20240118105934.137919-3-quic_sachinku@quicinc.com (mailing list archive)
State Superseded
Delegated to: Stanimir Varbanov
Headers
Series add MBR type rate control for encoder |

Commit Message

Sachin Kumar Garg Jan. 18, 2024, 10:59 a.m. UTC
  There is no limit on the maximum level of the bit rate with
the existing VBR rate control.
V4L2_MPEG_VIDEO_BITRATE_MODE_MBR rate control will limit the
frame maximum bit rate range to the +/- 10% of the configured
bit-rate value. Encoder will choose appropriate quantization
parameter and do the smart bit allocation to set the frame
maximum bitrate level.

Signed-off-by: Sachin Kumar Garg <quic_sachinku@quicinc.com>
---
 drivers/media/platform/qcom/venus/hfi_cmds.c  | 38 +++++++++++++------
 .../media/platform/qcom/venus/hfi_helper.h    |  1 +
 drivers/media/platform/qcom/venus/venc.c      |  2 +
 .../media/platform/qcom/venus/venc_ctrls.c    |  5 ++-
 4 files changed, 33 insertions(+), 13 deletions(-)
  

Comments

Bryan O'Donoghue Jan. 18, 2024, 1:34 p.m. UTC | #1
On 18/01/2024 10:59, Sachin Kumar Garg wrote:
>   
> -		switch (*in) {
> -		case HFI_RATE_CONTROL_OFF:
> -		case HFI_RATE_CONTROL_CBR_CFR:
> -		case HFI_RATE_CONTROL_CBR_VFR:
> -		case HFI_RATE_CONTROL_VBR_CFR:
> -		case HFI_RATE_CONTROL_VBR_VFR:
> -		case HFI_RATE_CONTROL_CQ:
> -			break;
> -		default:
> -			ret = -EINVAL;
> -			break;
> +		if (hfi_ver == HFI_VERSION_4XX) {
> +			switch (*in) {
> +			case HFI_RATE_CONTROL_OFF:
> +			case HFI_RATE_CONTROL_CBR_CFR:
> +			case HFI_RATE_CONTROL_CBR_VFR:
> +			case HFI_RATE_CONTROL_VBR_CFR:
> +			case HFI_RATE_CONTROL_VBR_VFR:
> +			case HFI_RATE_CONTROL_CQ:
> +			case HFI_RATE_CONTROL_MBR_CFR:
> +				break;
> +			default:
> +				ret = -EINVAL;
> +				break;
> +			}
> +		} else {
> +			switch (*in) {
> +			case HFI_RATE_CONTROL_OFF:
> +			case HFI_RATE_CONTROL_CBR_CFR:
> +			case HFI_RATE_CONTROL_CBR_VFR:
> +			case HFI_RATE_CONTROL_VBR_CFR:
> +			case HFI_RATE_CONTROL_VBR_VFR:
> +			case HFI_RATE_CONTROL_CQ:
> +				break;
> +			default:
> +				ret = -EINVAL;
> +				break;
> +			}

The if/else you have here seems like a needless replication

Just have =>

case HFI_RATE_CONTROL_MBR_CFR:
    if (hfi_ver == HFI_VERSION_4XX)

---
bod
  
Konrad Dybcio Jan. 18, 2024, 5:44 p.m. UTC | #2
On 1/18/24 11:59, Sachin Kumar Garg wrote:
> There is no limit on the maximum level of the bit rate with
> the existing VBR rate control.
> V4L2_MPEG_VIDEO_BITRATE_MODE_MBR rate control will limit the
> frame maximum bit rate range to the +/- 10% of the configured
> bit-rate value. Encoder will choose appropriate quantization
> parameter and do the smart bit allocation to set the frame
> maximum bitrate level.
> 
> Signed-off-by: Sachin Kumar Garg <quic_sachinku@quicinc.com>
> ---
>   drivers/media/platform/qcom/venus/hfi_cmds.c  | 38 +++++++++++++------
>   .../media/platform/qcom/venus/hfi_helper.h    |  1 +
>   drivers/media/platform/qcom/venus/venc.c      |  2 +
>   .../media/platform/qcom/venus/venc_ctrls.c    |  5 ++-
>   4 files changed, 33 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c
> index 3418d2dd9371..95fc27e0dc7d 100644
> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c
> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
> @@ -645,17 +645,33 @@ static int pkt_session_set_property_1x(struct hfi_session_set_property_pkt *pkt,
>   	case HFI_PROPERTY_PARAM_VENC_RATE_CONTROL: {
>   		u32 *in = pdata;
>   
> -		switch (*in) {
> -		case HFI_RATE_CONTROL_OFF:
> -		case HFI_RATE_CONTROL_CBR_CFR:
> -		case HFI_RATE_CONTROL_CBR_VFR:
> -		case HFI_RATE_CONTROL_VBR_CFR:
> -		case HFI_RATE_CONTROL_VBR_VFR:
> -		case HFI_RATE_CONTROL_CQ:
> -			break;
> -		default:
> -			ret = -EINVAL;
> -			break;
> +		if (hfi_ver == HFI_VERSION_4XX) {

So, only sdm845/sc7180 and friends support it, but the newer
SoCs (like 8250 don't)?

[...]

> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> @@ -387,10 +387,11 @@ int venc_ctrl_init(struct venus_inst *inst)
>   
>   	v4l2_ctrl_new_std_menu(&inst->ctrl_handler, &venc_ctrl_ops,
>   		V4L2_CID_MPEG_VIDEO_BITRATE_MODE,
> -		V4L2_MPEG_VIDEO_BITRATE_MODE_CBR,
> +		V4L2_MPEG_VIDEO_BITRATE_MODE_MBR,

Is this okay, since you're claiming only v4 supports it?

Konrad
  
Vikash Garodia Jan. 19, 2024, 8:05 a.m. UTC | #3
On 1/18/2024 11:14 PM, Konrad Dybcio wrote:
> 
> 
> On 1/18/24 11:59, Sachin Kumar Garg wrote:
>> There is no limit on the maximum level of the bit rate with
>> the existing VBR rate control.
>> V4L2_MPEG_VIDEO_BITRATE_MODE_MBR rate control will limit the
>> frame maximum bit rate range to the +/- 10% of the configured
>> bit-rate value. Encoder will choose appropriate quantization
>> parameter and do the smart bit allocation to set the frame
>> maximum bitrate level.
>>
>> Signed-off-by: Sachin Kumar Garg <quic_sachinku@quicinc.com>
>> ---
>>   drivers/media/platform/qcom/venus/hfi_cmds.c  | 38 +++++++++++++------
>>   .../media/platform/qcom/venus/hfi_helper.h    |  1 +
>>   drivers/media/platform/qcom/venus/venc.c      |  2 +
>>   .../media/platform/qcom/venus/venc_ctrls.c    |  5 ++-
>>   4 files changed, 33 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c
>> b/drivers/media/platform/qcom/venus/hfi_cmds.c
>> index 3418d2dd9371..95fc27e0dc7d 100644
>> --- a/drivers/media/platform/qcom/venus/hfi_cmds.c
>> +++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
>> @@ -645,17 +645,33 @@ static int pkt_session_set_property_1x(struct
>> hfi_session_set_property_pkt *pkt,
>>       case HFI_PROPERTY_PARAM_VENC_RATE_CONTROL: {
>>           u32 *in = pdata;
>>   -        switch (*in) {
>> -        case HFI_RATE_CONTROL_OFF:
>> -        case HFI_RATE_CONTROL_CBR_CFR:
>> -        case HFI_RATE_CONTROL_CBR_VFR:
>> -        case HFI_RATE_CONTROL_VBR_CFR:
>> -        case HFI_RATE_CONTROL_VBR_VFR:
>> -        case HFI_RATE_CONTROL_CQ:
>> -            break;
>> -        default:
>> -            ret = -EINVAL;
>> -            break;
>> +        if (hfi_ver == HFI_VERSION_4XX) {
> 
> So, only sdm845/sc7180 and friends support it, but the newer
> SoCs (like 8250 don't)?
Thats correct. Supported only in AR50 generations. Not available in 8250.

> 
> [...]
> 
>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> @@ -387,10 +387,11 @@ int venc_ctrl_init(struct venus_inst *inst)
>>         v4l2_ctrl_new_std_menu(&inst->ctrl_handler, &venc_ctrl_ops,
>>           V4L2_CID_MPEG_VIDEO_BITRATE_MODE,
>> -        V4L2_MPEG_VIDEO_BITRATE_MODE_CBR,
>> +        V4L2_MPEG_VIDEO_BITRATE_MODE_MBR,
> 
> Is this okay, since you're claiming only v4 supports it?
This looks okay to extend the support for new RC mode. I see an issue in
handling this new RC for non supported SOCs. This needs to be fixed in
hfi_cmds.c while preparing the packet. MBR for unsupported SOC should be treated
as -ENOTSUPP instead of -EINVAL which would terminate the session.

This need to be fixed.

Regards,
Vikash
  

Patch

diff --git a/drivers/media/platform/qcom/venus/hfi_cmds.c b/drivers/media/platform/qcom/venus/hfi_cmds.c
index 3418d2dd9371..95fc27e0dc7d 100644
--- a/drivers/media/platform/qcom/venus/hfi_cmds.c
+++ b/drivers/media/platform/qcom/venus/hfi_cmds.c
@@ -645,17 +645,33 @@  static int pkt_session_set_property_1x(struct hfi_session_set_property_pkt *pkt,
 	case HFI_PROPERTY_PARAM_VENC_RATE_CONTROL: {
 		u32 *in = pdata;
 
-		switch (*in) {
-		case HFI_RATE_CONTROL_OFF:
-		case HFI_RATE_CONTROL_CBR_CFR:
-		case HFI_RATE_CONTROL_CBR_VFR:
-		case HFI_RATE_CONTROL_VBR_CFR:
-		case HFI_RATE_CONTROL_VBR_VFR:
-		case HFI_RATE_CONTROL_CQ:
-			break;
-		default:
-			ret = -EINVAL;
-			break;
+		if (hfi_ver == HFI_VERSION_4XX) {
+			switch (*in) {
+			case HFI_RATE_CONTROL_OFF:
+			case HFI_RATE_CONTROL_CBR_CFR:
+			case HFI_RATE_CONTROL_CBR_VFR:
+			case HFI_RATE_CONTROL_VBR_CFR:
+			case HFI_RATE_CONTROL_VBR_VFR:
+			case HFI_RATE_CONTROL_CQ:
+			case HFI_RATE_CONTROL_MBR_CFR:
+				break;
+			default:
+				ret = -EINVAL;
+				break;
+			}
+		} else {
+			switch (*in) {
+			case HFI_RATE_CONTROL_OFF:
+			case HFI_RATE_CONTROL_CBR_CFR:
+			case HFI_RATE_CONTROL_CBR_VFR:
+			case HFI_RATE_CONTROL_VBR_CFR:
+			case HFI_RATE_CONTROL_VBR_VFR:
+			case HFI_RATE_CONTROL_CQ:
+				break;
+			default:
+				ret = -EINVAL;
+				break;
+			}
 		}
 
 		pkt->data[1] = *in;
diff --git a/drivers/media/platform/qcom/venus/hfi_helper.h b/drivers/media/platform/qcom/venus/hfi_helper.h
index e4c05d62cfc7..a0fd857f5c4b 100644
--- a/drivers/media/platform/qcom/venus/hfi_helper.h
+++ b/drivers/media/platform/qcom/venus/hfi_helper.h
@@ -232,6 +232,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_CFR		0x1000006
 #define HFI_RATE_CONTROL_CQ			0x1000008
 
 #define HFI_VIDEO_CODEC_H264			0x00000002
diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index 3ec2fb8d9fab..8acbb05f6ce8 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -807,6 +807,8 @@  static int venc_set_properties(struct venus_inst *inst)
 						      HFI_RATE_CONTROL_CBR_CFR;
 	else if (ctr->bitrate_mode == V4L2_MPEG_VIDEO_BITRATE_MODE_CQ)
 		rate_control = HFI_RATE_CONTROL_CQ;
+	else if (ctr->bitrate_mode == V4L2_MPEG_VIDEO_BITRATE_MODE_MBR)
+		rate_control = HFI_RATE_CONTROL_MBR_CFR;
 
 	ptype = HFI_PROPERTY_PARAM_VENC_RATE_CONTROL;
 	ret = hfi_session_set_property(inst, ptype, &rate_control);
diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index d9d2a293f3ef..c9c3b1b45525 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -387,10 +387,11 @@  int venc_ctrl_init(struct venus_inst *inst)
 
 	v4l2_ctrl_new_std_menu(&inst->ctrl_handler, &venc_ctrl_ops,
 		V4L2_CID_MPEG_VIDEO_BITRATE_MODE,
-		V4L2_MPEG_VIDEO_BITRATE_MODE_CBR,
+		V4L2_MPEG_VIDEO_BITRATE_MODE_MBR,
 		~((1 << V4L2_MPEG_VIDEO_BITRATE_MODE_VBR) |
 		  (1 << V4L2_MPEG_VIDEO_BITRATE_MODE_CBR) |
-		  (1 << V4L2_MPEG_VIDEO_BITRATE_MODE_CQ)),
+		  (1 << V4L2_MPEG_VIDEO_BITRATE_MODE_CQ)  |
+		  (1 << V4L2_MPEG_VIDEO_BITRATE_MODE_MBR)),
 		V4L2_MPEG_VIDEO_BITRATE_MODE_VBR);
 
 	v4l2_ctrl_new_std_menu(&inst->ctrl_handler, &venc_ctrl_ops,