Received-SPF: pass (google.com: domain of linux-kernel+bounces-30877-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <b3b01a1c-4946-531f-3dbe-9ba5cebbd93f@quicinc.com>
Date: Fri, 19 Jan 2024 13:35:04 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 2/2] media: venus: add new rate control type MBR for
 encoder
Content-Language: en-US
To: Konrad Dybcio <konrad.dybcio@linaro.org>,
        Sachin Kumar Garg
	<quic_sachinku@quicinc.com>,
        <hverkuil-cisco@xs4all.nl>,
        "Mauro Carvalho
 Chehab" <mchehab@kernel.org>,
        Stanimir Varbanov
	<stanimir.k.varbanov@gmail.com>,
        Andy Gross <agross@kernel.org>,
        "Bjorn
 Andersson" <andersson@kernel.org>
CC: Bryan O'Donoghue <bryan.odonoghue@linaro.org>,
        <linux-media@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>
References: <20240118105934.137919-1-quic_sachinku@quicinc.com>
 <20240118105934.137919-3-quic_sachinku@quicinc.com>
 <04a364e8-534c-40a4-a031-b9f9d2304c39@linaro.org>
From: Vikash Garodia <quic_vgarodia@quicinc.com>
In-Reply-To: <04a364e8-534c-40a4-a031-b9f9d2304c39@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit

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