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

2017-12-01 Thread Hans Verkuil 
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 
> ---
>  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, );
>   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

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

2017-11-29 Thread Stanimir Varbanov 
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 
---
 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, );
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);
+
+   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 = >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, ,
+>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, ,
+>num_p_frames);
+   if (ret)
+   return ret;
+
+   ctr->num_b_frames = bframes;
break;
default:
return -EINVAL;
-- 
2.11.0