Message ID | 20240118105934.137919-3-quic_sachinku@quicinc.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | add MBR type rate control for encoder | expand |
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 <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
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
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,
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(-)