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 <[email protected]>
---
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) {
+ 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,
--
2.34.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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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