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.varba...@linaro.org> > --- > 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; >