Re: [PATCH 2/2] venus: venc: set correctly GOP size and number of B-frames

From: Hans Verkuil
Date: Fri Dec 01 2017 - 08:14:31 EST

Next message: Adrian Hunter: "Re: [PATCH V15 00/22] mmc: Add Command Queue support"
Previous message: Vitaly Kuznetsov: "[PATCH RFC 6/6] x86/kvm: support Hyper-V reenlightenment"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 11/29/17 14:25, Stanimir Varbanov wrote:
> This change fixes the calculation of B-frames and GOP size by
> adopt v4l2 controls with the firmware interface expectations.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@xxxxxxxxxx>
> ---
> drivers/media/platform/qcom/venus/venc.c | 15 ++++---
> drivers/media/platform/qcom/venus/venc_ctrls.c | 60 +++++++++++++++++++++++++-
> 2 files changed, 69 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index 36d31540c59d..e3a10a852cad 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -685,8 +685,13 @@ static int venc_set_properties(struct venus_inst *inst)
> return ret;
> }
>
> + /* IDR periodicity, n:
> + * n = 0 - only the first I-frame is IDR frame
> + * n = 1 - all I-frames will be IDR frames
> + * n > 1 - every n-th I-frame will be IDR frame
> + */
> ptype = HFI_PROPERTY_CONFIG_VENC_IDR_PERIOD;
> - idrp.idr_period = ctr->gop_size;
> + idrp.idr_period = 0;
> ret = hfi_session_set_property(inst, ptype, &idrp);
> if (ret)
> return ret;
> @@ -700,10 +705,6 @@ static int venc_set_properties(struct venus_inst *inst)
> return ret;
> }
>
> - /* intra_period = pframes + bframes + 1 */
> - if (!ctr->num_p_frames)
> - ctr->num_p_frames = 2 * 15 - 1,
> -
> ptype = HFI_PROPERTY_CONFIG_VENC_INTRA_PERIOD;
> intra_period.pframes = ctr->num_p_frames;
> intra_period.bframes = ctr->num_b_frames;
> @@ -800,6 +801,10 @@ static int venc_init_session(struct venus_inst *inst)
> if (ret)
> goto deinit;
>
> + ret = venc_set_properties(inst);
> + if (ret)
> + goto deinit;
> +
> return 0;
> deinit:
> hfi_session_deinit(inst);
> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> index ab0fe51ff0f7..53130d4774d3 100644
> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> @@ -30,10 +30,58 @@
> #define AT_SLICE_BOUNDARY \
> V4L2_MPEG_VIDEO_H264_LOOP_FILTER_MODE_DISABLED_AT_SLICE_BOUNDARY
>
> +static int venc_calc_bpframes(u32 gop_size, u32 conseq_b, u32 *bf, u32 *pf)
> +{
> + uint32_t half = (gop_size - 1) >> 1;
> + uint32_t b, p, ratio;
> + bool found = false;
> +
> + if (!gop_size)
> + return -EINVAL;
> +
> + *bf = *pf = 0;
> +
> + if (!conseq_b) {
> + *pf = gop_size - 1;
> + return 0;
> + }
> +
> + b = p = half;
> +
> + for (; b <= gop_size - 1; b++, p--) {
> + if (b % p)
> + continue;
> +
> + ratio = b;
> + do_div(ratio, p);

Huh? Why use do_div when both arguments are uint32_t?

This gives a pile of compile warnings:

In file included from /home/hans/work/build/media-git/arch/arm/include/asm/div64.h:127:0,
from /home/hans/work/build/media-git/include/linux/kernel.h:173,
from /home/hans/work/build/media-git/include/linux/list.h:9,
from /home/hans/work/build/media-git/include/media/v4l2-ctrls.h:20,
from /home/hans/work/build/media-git/drivers/media/platform/qcom/venus/venc_ctrls.c:16:
/home/hans/work/build/media-git/drivers/media/platform/qcom/venus/venc_ctrls.c: In function 'venc_calc_bpframes':
/home/hans/work/build/media-git/include/asm-generic/div64.h:222:28: warning: comparison of distinct pointer types lacks a cast
(void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
^
/home/hans/work/build/media-git/drivers/media/platform/qcom/venus/venc_ctrls.c:56:3: note: in expansion of macro 'do_div'
do_div(ratio, p);
^~~~~~
In file included from /home/hans/work/build/media-git/include/linux/kernel.h:10:0,
from /home/hans/work/build/media-git/include/linux/list.h:9,
from /home/hans/work/build/media-git/include/media/v4l2-ctrls.h:20,
from /home/hans/work/build/media-git/drivers/media/platform/qcom/venus/venc_ctrls.c:16:
/home/hans/work/build/media-git/include/asm-generic/div64.h:235:25: warning: right shift count >= width of type [-Wshift-count-overflow]
} else if (likely(((n) >> 32) == 0)) { \
^
/home/hans/work/build/media-git/include/linux/compiler.h:76:40: note: in definition of macro 'likely'
# define likely(x) __builtin_expect(!!(x), 1)
^
/home/hans/work/build/media-git/drivers/media/platform/qcom/venus/venc_ctrls.c:56:3: note: in expansion of macro 'do_div'
do_div(ratio, p);
^~~~~~
In file included from /home/hans/work/build/media-git/arch/arm/include/asm/div64.h:127:0,
from /home/hans/work/build/media-git/include/linux/kernel.h:173,
from /home/hans/work/build/media-git/include/linux/list.h:9,
from /home/hans/work/build/media-git/include/media/v4l2-ctrls.h:20,
from /home/hans/work/build/media-git/drivers/media/platform/qcom/venus/venc_ctrls.c:16:
/home/hans/work/build/media-git/include/asm-generic/div64.h:239:22: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
__rem = __div64_32(&(n), __base); \
^
/home/hans/work/build/media-git/drivers/media/platform/qcom/venus/venc_ctrls.c:56:3: note: in expansion of macro 'do_div'
do_div(ratio, p);
^~~~~~
In file included from /home/hans/work/build/media-git/include/linux/kernel.h:173:0,
from /home/hans/work/build/media-git/include/linux/list.h:9,
from /home/hans/work/build/media-git/include/media/v4l2-ctrls.h:20,
from /home/hans/work/build/media-git/drivers/media/platform/qcom/venus/venc_ctrls.c:16:
/home/hans/work/build/media-git/arch/arm/include/asm/div64.h:33:24: note: expected 'uint64_t * {aka long long unsigned int *}' but argument is of type 'uint32_t * {aka unsigned int *}'
static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
^~~~~~~~~~

Please post a v2.

Regards,

Hans

> +
> + if (ratio == conseq_b) {
> + found = true;
> + break;
> + }
> +
> + if (ratio > conseq_b)
> + break;
> + }
> +
> + if (found == false)
> + return -EINVAL;
> +
> + if (b + p + 1 != gop_size)
> + return -EINVAL;
> +
> + *bf = b;
> + *pf = p;
> +
> + return 0;
> +}
> +
> static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> struct venus_inst *inst = ctrl_to_inst(ctrl);
> struct venc_controls *ctr = &inst->controls.enc;
> + u32 bframes;
> + int ret;
>
> switch (ctrl->id) {
> case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
> @@ -102,6 +150,11 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> case V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB:
> break;
> case V4L2_CID_MPEG_VIDEO_GOP_SIZE:
> + ret = venc_calc_bpframes(ctrl->val, ctr->num_b_frames, &bframes,
> + &ctr->num_p_frames);
> + if (ret)
> + return ret;
> +
> ctr->gop_size = ctrl->val;
> break;
> case V4L2_CID_MPEG_VIDEO_H264_I_PERIOD:
> @@ -114,7 +167,12 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> ctr->vp8_max_qp = ctrl->val;
> break;
> case V4L2_CID_MPEG_VIDEO_B_FRAMES:
> - ctr->num_b_frames = ctrl->val;
> + ret = venc_calc_bpframes(ctr->gop_size, ctrl->val, &bframes,
> + &ctr->num_p_frames);
> + if (ret)
> + return ret;
> +
> + ctr->num_b_frames = bframes;
> break;
> default:
> return -EINVAL;
>

Next message: Adrian Hunter: "Re: [PATCH V15 00/22] mmc: Add Command Queue support"
Previous message: Vitaly Kuznetsov: "[PATCH RFC 6/6] x86/kvm: support Hyper-V reenlightenment"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]