Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: dikshita=codeaurora.org@mg.codeaurora.org
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Wed, 13 Jan 2021 09:58:18 +0530
From:   dikshita@codeaurora.org
To:     Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc:     linux-media@vger.kernel.org, stanimir.varbanov@linaro.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        vgarodia@codeaurora.org
Subject: Re: [PATCH v5 2/2] venus: venc: Add support for Long Term Reference
 (LTR) controls
In-Reply-To: <e11a4e78-8e41-c7f4-4947-76d27f0fa2ed@xs4all.nl>
References: <1609736971-14454-1-git-send-email-dikshita@codeaurora.org>
 <1609736971-14454-3-git-send-email-dikshita@codeaurora.org>
 <e11a4e78-8e41-c7f4-4947-76d27f0fa2ed@xs4all.nl>
Message-ID: <cf29047907ef104a233316a47c313d21@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Precedence: bulk

Hi Hans,

On 2021-01-12 14:34, Hans Verkuil wrote:
> On 04/01/2021 06:09, Dikshita Agarwal wrote:
>> Add support for below LTR controls in encoder:
>> - V4L2_CID_MPEG_VIDEO_LTR_COUNT
>> - V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX
>> - V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES
> 
> This patch does not add any Request API support to the venus driver,
> so it makes no sense to merge this series. It's incomplete.
> 

It Is not mandatory to use request API for LTR controls.
LTR controls can be exercised either in frame synchronized manner or
in the non-frame synchronized way as well.

Thanks,
Dikshita
> The other two series (priority ID and QP and layer bitrate) look good
> to be merged.
> 
> Regards,
> 
> 	Hans
> 
>> 
>> Signed-off-by: Dikshita Agarwal <dikshita@codeaurora.org>
>> ---
>>  drivers/media/platform/qcom/venus/venc_ctrls.c | 49 
>> +++++++++++++++++++++++++-
>>  1 file changed, 48 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c 
>> b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> index 496ad4d..7d010d8 100644
>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> @@ -20,6 +20,7 @@
>>  #define INTRA_REFRESH_MBS_MAX	300
>>  #define AT_SLICE_BOUNDARY	\
>>  	V4L2_MPEG_VIDEO_H264_LOOP_FILTER_MODE_DISABLED_AT_SLICE_BOUNDARY
>> +#define MAX_LTR_FRAME_COUNT 4
>> 
>>  static int venc_calc_bpframes(u32 gop_size, u32 conseq_b, u32 *bf, 
>> u32 *pf)
>>  {
>> @@ -72,6 +73,9 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>  	struct venc_controls *ctr = &inst->controls.enc;
>>  	struct hfi_enable en = { .enable = 1 };
>>  	struct hfi_bitrate brate;
>> +	struct hfi_ltr_use ltr_use;
>> +	struct hfi_ltr_mark ltr_mark;
>> +	struct hfi_ltr_mode ltr_mode;
>>  	u32 bframes;
>>  	u32 ptype;
>>  	int ret;
>> @@ -259,6 +263,37 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>  	case V4L2_CID_MPEG_VIDEO_FRAME_SKIP_MODE:
>>  		ctr->frame_skip_mode = ctrl->val;
>>  		break;
>> +	case V4L2_CID_MPEG_VIDEO_LTR_COUNT:
>> +		ptype = HFI_PROPERTY_PARAM_VENC_LTRMODE;
>> +		ltr_mode.ltr_count = ctrl->val;
>> +		ltr_mode.ltr_mode = HFI_LTR_MODE_MANUAL;
>> +		ltr_mode.trust_mode = 1;
>> +		ret = hfi_session_set_property(inst, ptype, &ltr_mode);
>> +		if (ret) {
>> +			mutex_unlock(&inst->lock);
>> +			return ret;
>> +		}
>> +		break;
>> +	case V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX:
>> +		ptype = HFI_PROPERTY_CONFIG_VENC_MARKLTRFRAME;
>> +		ltr_mark.mark_frame = ctrl->val;
>> +		ret = hfi_session_set_property(inst, ptype, &ltr_mark);
>> +		if (ret) {
>> +			mutex_unlock(&inst->lock);
>> +			return ret;
>> +		}
>> +		break;
>> +	case V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES:
>> +		ptype = HFI_PROPERTY_CONFIG_VENC_USELTRFRAME;
>> +		ltr_use.ref_ltr = ctrl->val;
>> +		ltr_use.use_constrnt = true;
>> +		ltr_use.frames = 0;
>> +		ret = hfi_session_set_property(inst, ptype, &ltr_use);
>> +		if (ret) {
>> +			mutex_unlock(&inst->lock);
>> +			return ret;
>> +		}
>> +		break;
>>  	default:
>>  		return -EINVAL;
>>  	}
>> @@ -274,7 +309,7 @@ int venc_ctrl_init(struct venus_inst *inst)
>>  {
>>  	int ret;
>> 
>> -	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 50);
>> +	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 53);
>>  	if (ret)
>>  		return ret;
>> 
>> @@ -476,6 +511,18 @@ int venc_ctrl_init(struct venus_inst *inst)
>>  			       (1 << V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT)),
>>  			       V4L2_MPEG_VIDEO_FRAME_SKIP_MODE_DISABLED);
>> 
>> +	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
>> +			  V4L2_CID_MPEG_VIDEO_USE_LTR_FRAMES, 0,
>> +			  (MAX_LTR_FRAME_COUNT - 1), 1, 0);
>> +
>> +	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
>> +			  V4L2_CID_MPEG_VIDEO_LTR_COUNT, 0,
>> +			  MAX_LTR_FRAME_COUNT, 1, 0);
>> +
>> +	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
>> +			  V4L2_CID_MPEG_VIDEO_FRAME_LTR_INDEX, 0,
>> +			  (MAX_LTR_FRAME_COUNT - 1), 1, 0);
>> +
>>  	ret = inst->ctrl_handler.error;
>>  	if (ret)
>>  		goto err;
>>