[PATCH] media: venus: add support for key frame

[PATCH] media: venus: add support for key frame

[Malathi Gottam]

When client requests for a keyframe, set the property
to hardware to generate the sync frame.

Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
---
 drivers/media/platform/qcom/venus/venc_ctrls.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 45910172..f332c8e 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -81,6 +81,8 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 	struct venc_controls *ctr = &inst->controls.enc;
 	u32 bframes;
 	int ret;
+	void *ptr;
+	u32 ptype;
 
 	switch (ctrl->id) {
 	case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
@@ -173,6 +175,14 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 
 		ctr->num_b_frames = bframes;
 		break;
+	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
+		ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
+		ret = hfi_session_set_property(inst, ptype, ptr);
+
+		if (ret)
+			return ret;
+
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -309,6 +319,9 @@ int venc_ctrl_init(struct venus_inst *inst)
 	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
 		V4L2_CID_MPEG_VIDEO_H264_I_PERIOD, 0, (1 << 16) - 1, 1, 0);
 
+	v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
+			  V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME, 0, 0, 0, 0);
+
 	ret = inst->ctrl_handler.error;
 	if (ret)
 		goto err;
-- 
1.9.1

Re: [PATCH] media: venus: add support for key frame

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5285 bytes --]

Hi Malathi,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.19-rc7 next-20181009]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Malathi-Gottam/media-venus-add-support-for-key-frame/20181009-214918
base:   git://linuxtv.org/media_tree.git master
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=arm 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/media//platform/qcom/venus/venc_ctrls.c: In function 'venc_op_s_ctrl':
>> drivers/media//platform/qcom/venus/venc_ctrls.c:180:7: warning: 'ptr' may be used uninitialized in this function [-Wmaybe-uninitialized]
      ret = hfi_session_set_property(inst, ptype, ptr);
      ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/ptr +180 drivers/media//platform/qcom/venus/venc_ctrls.c

    77	
    78	static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
    79	{
    80		struct venus_inst *inst = ctrl_to_inst(ctrl);
    81		struct venc_controls *ctr = &inst->controls.enc;
    82		u32 bframes;
    83		int ret;
    84		void *ptr;
    85		u32 ptype;
    86	
    87		switch (ctrl->id) {
    88		case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
    89			ctr->bitrate_mode = ctrl->val;
    90			break;
    91		case V4L2_CID_MPEG_VIDEO_BITRATE:
    92			ctr->bitrate = ctrl->val;
    93			break;
    94		case V4L2_CID_MPEG_VIDEO_BITRATE_PEAK:
    95			ctr->bitrate_peak = ctrl->val;
    96			break;
    97		case V4L2_CID_MPEG_VIDEO_H264_ENTROPY_MODE:
    98			ctr->h264_entropy_mode = ctrl->val;
    99			break;
   100		case V4L2_CID_MPEG_VIDEO_MPEG4_PROFILE:
   101			ctr->profile.mpeg4 = ctrl->val;
   102			break;
   103		case V4L2_CID_MPEG_VIDEO_H264_PROFILE:
   104			ctr->profile.h264 = ctrl->val;
   105			break;
   106		case V4L2_CID_MPEG_VIDEO_VP8_PROFILE:
   107			ctr->profile.vpx = ctrl->val;
   108			break;
   109		case V4L2_CID_MPEG_VIDEO_MPEG4_LEVEL:
   110			ctr->level.mpeg4 = ctrl->val;
   111			break;
   112		case V4L2_CID_MPEG_VIDEO_H264_LEVEL:
   113			ctr->level.h264 = ctrl->val;
   114			break;
   115		case V4L2_CID_MPEG_VIDEO_H264_I_FRAME_QP:
   116			ctr->h264_i_qp = ctrl->val;
   117			break;
   118		case V4L2_CID_MPEG_VIDEO_H264_P_FRAME_QP:
   119			ctr->h264_p_qp = ctrl->val;
   120			break;
   121		case V4L2_CID_MPEG_VIDEO_H264_B_FRAME_QP:
   122			ctr->h264_b_qp = ctrl->val;
   123			break;
   124		case V4L2_CID_MPEG_VIDEO_H264_MIN_QP:
   125			ctr->h264_min_qp = ctrl->val;
   126			break;
   127		case V4L2_CID_MPEG_VIDEO_H264_MAX_QP:
   128			ctr->h264_max_qp = ctrl->val;
   129			break;
   130		case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MODE:
   131			ctr->multi_slice_mode = ctrl->val;
   132			break;
   133		case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MAX_BYTES:
   134			ctr->multi_slice_max_bytes = ctrl->val;
   135			break;
   136		case V4L2_CID_MPEG_VIDEO_MULTI_SLICE_MAX_MB:
   137			ctr->multi_slice_max_mb = ctrl->val;
   138			break;
   139		case V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_ALPHA:
   140			ctr->h264_loop_filter_alpha = ctrl->val;
   141			break;
   142		case V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_BETA:
   143			ctr->h264_loop_filter_beta = ctrl->val;
   144			break;
   145		case V4L2_CID_MPEG_VIDEO_H264_LOOP_FILTER_MODE:
   146			ctr->h264_loop_filter_mode = ctrl->val;
   147			break;
   148		case V4L2_CID_MPEG_VIDEO_HEADER_MODE:
   149			ctr->header_mode = ctrl->val;
   150			break;
   151		case V4L2_CID_MPEG_VIDEO_CYCLIC_INTRA_REFRESH_MB:
   152			break;
   153		case V4L2_CID_MPEG_VIDEO_GOP_SIZE:
   154			ret = venc_calc_bpframes(ctrl->val, ctr->num_b_frames, &bframes,
   155						 &ctr->num_p_frames);
   156			if (ret)
   157				return ret;
   158	
   159			ctr->gop_size = ctrl->val;
   160			break;
   161		case V4L2_CID_MPEG_VIDEO_H264_I_PERIOD:
   162			ctr->h264_i_period = ctrl->val;
   163			break;
   164		case V4L2_CID_MPEG_VIDEO_VPX_MIN_QP:
   165			ctr->vp8_min_qp = ctrl->val;
   166			break;
   167		case V4L2_CID_MPEG_VIDEO_VPX_MAX_QP:
   168			ctr->vp8_max_qp = ctrl->val;
   169			break;
   170		case V4L2_CID_MPEG_VIDEO_B_FRAMES:
   171			ret = venc_calc_bpframes(ctr->gop_size, ctrl->val, &bframes,
   172						 &ctr->num_p_frames);
   173			if (ret)
   174				return ret;
   175	
   176			ctr->num_b_frames = bframes;
   177			break;
   178		case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
   179			ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
 > 180			ret = hfi_session_set_property(inst, ptype, ptr);
   181	
   182			if (ret)
   183				return ret;
   184	
   185			break;
   186		default:
   187			return -EINVAL;
   188		}
   189	
   190		return 0;
   191	}
   192	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 67059 bytes --]

Re: [PATCH] media: venus: add support for key frame

[Alexandre Courbot]

On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <mgottam@codeaurora.org> wrote:
>
> When client requests for a keyframe, set the property
> to hardware to generate the sync frame.
>
> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/venc_ctrls.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> index 45910172..f332c8e 100644
> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> @@ -81,6 +81,8 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>         struct venc_controls *ctr = &inst->controls.enc;
>         u32 bframes;
>         int ret;
> +       void *ptr;
> +       u32 ptype;
>
>         switch (ctrl->id) {
>         case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
> @@ -173,6 +175,14 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>
>                 ctr->num_b_frames = bframes;
>                 break;
> +       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
> +               ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
> +               ret = hfi_session_set_property(inst, ptype, ptr);

The test bot already said it, but ptr is passed to
hfi_session_set_property() uninitialized. And as can be expected the
call returns -EINVAL on my board.

Looking at other uses of HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME I
see that the packet sent to the firmware does not have room for an
argument, so I tried to pass NULL but got the same result.

> +

This newline is unnecessary.

> +               if (ret)
> +                       return ret;
> +
> +               break;
>         default:
>                 return -EINVAL;
>         }
> @@ -309,6 +319,9 @@ int venc_ctrl_init(struct venus_inst *inst)
>         v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
>                 V4L2_CID_MPEG_VIDEO_H264_I_PERIOD, 0, (1 << 16) - 1, 1, 0);
>
> +       v4l2_ctrl_new_std(&inst->ctrl_handler, &venc_ctrl_ops,
> +                         V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME, 0, 0, 0, 0);
> +
>         ret = inst->ctrl_handler.error;
>         if (ret)
>                 goto err;
> --
> 1.9.1
>

Re: [PATCH] media: venus: add support for key frame

[Stanimir Varbanov]

Hi Alex,

On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
> On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <mgottam@codeaurora.org> wrote:
>>
>> When client requests for a keyframe, set the property
>> to hardware to generate the sync frame.
>>
>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>> ---
>>  drivers/media/platform/qcom/venus/venc_ctrls.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> index 45910172..f332c8e 100644
>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> @@ -81,6 +81,8 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>         struct venc_controls *ctr = &inst->controls.enc;
>>         u32 bframes;
>>         int ret;
>> +       void *ptr;
>> +       u32 ptype;
>>
>>         switch (ctrl->id) {
>>         case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
>> @@ -173,6 +175,14 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>
>>                 ctr->num_b_frames = bframes;
>>                 break;
>> +       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
>> +               ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
>> +               ret = hfi_session_set_property(inst, ptype, ptr);
> 
> The test bot already said it, but ptr is passed to
> hfi_session_set_property() uninitialized. And as can be expected the
> call returns -EINVAL on my board.
> 
> Looking at other uses of HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME I
> see that the packet sent to the firmware does not have room for an
> argument, so I tried to pass NULL but got the same result.

yes, because pdata cannot be NULL. I'd suggest to make a pointer to
struct hfi_enable and pass it to the set_property function.

-- 
regards,
Stan

Re: [PATCH] media: venus: add support for key frame

[Alexandre Courbot]

On Fri, Oct 12, 2018 at 4:37 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Alex,
>
> On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
> > On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <mgottam@codeaurora.org> wrote:
> >>
> >> When client requests for a keyframe, set the property
> >> to hardware to generate the sync frame.
> >>
> >> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> >> ---
> >>  drivers/media/platform/qcom/venus/venc_ctrls.c | 13 +++++++++++++
> >>  1 file changed, 13 insertions(+)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >> index 45910172..f332c8e 100644
> >> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> >> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >> @@ -81,6 +81,8 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> >>         struct venc_controls *ctr = &inst->controls.enc;
> >>         u32 bframes;
> >>         int ret;
> >> +       void *ptr;
> >> +       u32 ptype;
> >>
> >>         switch (ctrl->id) {
> >>         case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
> >> @@ -173,6 +175,14 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> >>
> >>                 ctr->num_b_frames = bframes;
> >>                 break;
> >> +       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
> >> +               ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
> >> +               ret = hfi_session_set_property(inst, ptype, ptr);
> >
> > The test bot already said it, but ptr is passed to
> > hfi_session_set_property() uninitialized. And as can be expected the
> > call returns -EINVAL on my board.
> >
> > Looking at other uses of HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME I
> > see that the packet sent to the firmware does not have room for an
> > argument, so I tried to pass NULL but got the same result.
>
> yes, because pdata cannot be NULL. I'd suggest to make a pointer to
> struct hfi_enable and pass it to the set_property function.

FWIW I also tried doing this and got the same error, strange...

Re: [PATCH] media: venus: add support for key frame

[Stanimir Varbanov]

On 10/12/2018 11:06 AM, Alexandre Courbot wrote:
> On Fri, Oct 12, 2018 at 4:37 PM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> Hi Alex,
>>
>> On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
>>> On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <mgottam@codeaurora.org> wrote:
>>>>
>>>> When client requests for a keyframe, set the property
>>>> to hardware to generate the sync frame.
>>>>
>>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>>>> ---
>>>>  drivers/media/platform/qcom/venus/venc_ctrls.c | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
>>>> index 45910172..f332c8e 100644
>>>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
>>>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
>>>> @@ -81,6 +81,8 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>         struct venc_controls *ctr = &inst->controls.enc;
>>>>         u32 bframes;
>>>>         int ret;
>>>> +       void *ptr;
>>>> +       u32 ptype;
>>>>
>>>>         switch (ctrl->id) {
>>>>         case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
>>>> @@ -173,6 +175,14 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>
>>>>                 ctr->num_b_frames = bframes;
>>>>                 break;
>>>> +       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
>>>> +               ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
>>>> +               ret = hfi_session_set_property(inst, ptype, ptr);
>>>
>>> The test bot already said it, but ptr is passed to
>>> hfi_session_set_property() uninitialized. And as can be expected the
>>> call returns -EINVAL on my board.
>>>
>>> Looking at other uses of HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME I
>>> see that the packet sent to the firmware does not have room for an
>>> argument, so I tried to pass NULL but got the same result.
>>
>> yes, because pdata cannot be NULL. I'd suggest to make a pointer to
>> struct hfi_enable and pass it to the set_property function.
> 
> FWIW I also tried doing this and got the same error, strange...
> 

OK, when you calling the v4l control? It makes sense when you calling
it, because set_property checks does the session is on START state (i.e.
streamon on both queues).

-- 
regards,
Stan

Re: [PATCH] media: venus: add support for key frame

[Alexandre Courbot]

On Fri, Oct 12, 2018 at 5:10 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
>
>
> On 10/12/2018 11:06 AM, Alexandre Courbot wrote:
> > On Fri, Oct 12, 2018 at 4:37 PM Stanimir Varbanov
> > <stanimir.varbanov@linaro.org> wrote:
> >>
> >> Hi Alex,
> >>
> >> On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
> >>> On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <mgottam@codeaurora.org> wrote:
> >>>>
> >>>> When client requests for a keyframe, set the property
> >>>> to hardware to generate the sync frame.
> >>>>
> >>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> >>>> ---
> >>>>  drivers/media/platform/qcom/venus/venc_ctrls.c | 13 +++++++++++++
> >>>>  1 file changed, 13 insertions(+)
> >>>>
> >>>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>>> index 45910172..f332c8e 100644
> >>>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>>> @@ -81,6 +81,8 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> >>>>         struct venc_controls *ctr = &inst->controls.enc;
> >>>>         u32 bframes;
> >>>>         int ret;
> >>>> +       void *ptr;
> >>>> +       u32 ptype;
> >>>>
> >>>>         switch (ctrl->id) {
> >>>>         case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
> >>>> @@ -173,6 +175,14 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> >>>>
> >>>>                 ctr->num_b_frames = bframes;
> >>>>                 break;
> >>>> +       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
> >>>> +               ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
> >>>> +               ret = hfi_session_set_property(inst, ptype, ptr);
> >>>
> >>> The test bot already said it, but ptr is passed to
> >>> hfi_session_set_property() uninitialized. And as can be expected the
> >>> call returns -EINVAL on my board.
> >>>
> >>> Looking at other uses of HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME I
> >>> see that the packet sent to the firmware does not have room for an
> >>> argument, so I tried to pass NULL but got the same result.
> >>
> >> yes, because pdata cannot be NULL. I'd suggest to make a pointer to
> >> struct hfi_enable and pass it to the set_property function.
> >
> > FWIW I also tried doing this and got the same error, strange...
> >
>
> OK, when you calling the v4l control? It makes sense when you calling
> it, because set_property checks does the session is on START state (i.e.
> streamon on both queues).

Do you mean that the property won't be actually applied unless both
queues are streaming? In that case maybe it would make sense for the
driver to save controls set before that and apply them when the
conditions allow them to be effective?

Re: [PATCH] media: venus: add support for key frame

[Tomasz Figa]

On Mon, Oct 22, 2018 at 3:15 PM Alexandre Courbot <acourbot@chromium.org> wrote:
>
> On Fri, Oct 12, 2018 at 5:10 PM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
> >
> >
> >
> > On 10/12/2018 11:06 AM, Alexandre Courbot wrote:
> > > On Fri, Oct 12, 2018 at 4:37 PM Stanimir Varbanov
> > > <stanimir.varbanov@linaro.org> wrote:
> > >>
> > >> Hi Alex,
> > >>
> > >> On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
> > >>> On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <mgottam@codeaurora.org> wrote:
> > >>>>
> > >>>> When client requests for a keyframe, set the property
> > >>>> to hardware to generate the sync frame.
> > >>>>
> > >>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> > >>>> ---
> > >>>>  drivers/media/platform/qcom/venus/venc_ctrls.c | 13 +++++++++++++
> > >>>>  1 file changed, 13 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> > >>>> index 45910172..f332c8e 100644
> > >>>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> > >>>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> > >>>> @@ -81,6 +81,8 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> > >>>>         struct venc_controls *ctr = &inst->controls.enc;
> > >>>>         u32 bframes;
> > >>>>         int ret;
> > >>>> +       void *ptr;
> > >>>> +       u32 ptype;
> > >>>>
> > >>>>         switch (ctrl->id) {
> > >>>>         case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
> > >>>> @@ -173,6 +175,14 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
> > >>>>
> > >>>>                 ctr->num_b_frames = bframes;
> > >>>>                 break;
> > >>>> +       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
> > >>>> +               ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
> > >>>> +               ret = hfi_session_set_property(inst, ptype, ptr);
> > >>>
> > >>> The test bot already said it, but ptr is passed to
> > >>> hfi_session_set_property() uninitialized. And as can be expected the
> > >>> call returns -EINVAL on my board.
> > >>>
> > >>> Looking at other uses of HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME I
> > >>> see that the packet sent to the firmware does not have room for an
> > >>> argument, so I tried to pass NULL but got the same result.
> > >>
> > >> yes, because pdata cannot be NULL. I'd suggest to make a pointer to
> > >> struct hfi_enable and pass it to the set_property function.
> > >
> > > FWIW I also tried doing this and got the same error, strange...
> > >
> >
> > OK, when you calling the v4l control? It makes sense when you calling
> > it, because set_property checks does the session is on START state (i.e.
> > streamon on both queues).
>
> Do you mean that the property won't be actually applied unless both
> queues are streaming? In that case maybe it would make sense for the
> driver to save controls set before that and apply them when the
> conditions allow them to be effective?

Right. The driver cannot just drop a control setting on the floor if
it's not ready to apply it.

However, the V4L2 control framework already provides a tool to handle this:
 - the driver can ignore any .s_ctrl() calls when it can't apply the controls,
 - the driver must call v4l2_ctrl_handler_setup() when it initialized
the hardware, so that all the control values are applied in one go.

Best regards,
Tomasz

Re: [PATCH] media: venus: add support for key frame

[mgottam]

On 2018-10-23 08:37, Tomasz Figa wrote:
> On Mon, Oct 22, 2018 at 3:15 PM Alexandre Courbot 
> <acourbot@chromium.org> wrote:
>> 
>> On Fri, Oct 12, 2018 at 5:10 PM Stanimir Varbanov
>> <stanimir.varbanov@linaro.org> wrote:
>> >
>> >
>> >
>> > On 10/12/2018 11:06 AM, Alexandre Courbot wrote:
>> > > On Fri, Oct 12, 2018 at 4:37 PM Stanimir Varbanov
>> > > <stanimir.varbanov@linaro.org> wrote:
>> > >>
>> > >> Hi Alex,
>> > >>
>> > >> On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
>> > >>> On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <mgottam@codeaurora.org> wrote:
>> > >>>>
>> > >>>> When client requests for a keyframe, set the property
>> > >>>> to hardware to generate the sync frame.
>> > >>>>
>> > >>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>> > >>>> ---
>> > >>>>  drivers/media/platform/qcom/venus/venc_ctrls.c | 13 +++++++++++++
>> > >>>>  1 file changed, 13 insertions(+)
>> > >>>>
>> > >>>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> > >>>> index 45910172..f332c8e 100644
>> > >>>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
>> > >>>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> > >>>> @@ -81,6 +81,8 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>> > >>>>         struct venc_controls *ctr = &inst->controls.enc;
>> > >>>>         u32 bframes;
>> > >>>>         int ret;
>> > >>>> +       void *ptr;
>> > >>>> +       u32 ptype;
>> > >>>>
>> > >>>>         switch (ctrl->id) {
>> > >>>>         case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:
>> > >>>> @@ -173,6 +175,14 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>> > >>>>
>> > >>>>                 ctr->num_b_frames = bframes;
>> > >>>>                 break;
>> > >>>> +       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
>> > >>>> +               ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
>> > >>>> +               ret = hfi_session_set_property(inst, ptype, ptr);
>> > >>>
>> > >>> The test bot already said it, but ptr is passed to
>> > >>> hfi_session_set_property() uninitialized. And as can be expected the
>> > >>> call returns -EINVAL on my board.
>> > >>>
>> > >>> Looking at other uses of HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME I
>> > >>> see that the packet sent to the firmware does not have room for an
>> > >>> argument, so I tried to pass NULL but got the same result.
>> > >>
>> > >> yes, because pdata cannot be NULL. I'd suggest to make a pointer to
>> > >> struct hfi_enable and pass it to the set_property function.
>> > >
>> > > FWIW I also tried doing this and got the same error, strange...
>> > >
>> >
>> > OK, when you calling the v4l control? It makes sense when you calling
>> > it, because set_property checks does the session is on START state (i.e.
>> > streamon on both queues).
>> 
>> Do you mean that the property won't be actually applied unless both
>> queues are streaming? In that case maybe it would make sense for the
>> driver to save controls set before that and apply them when the
>> conditions allow them to be effective?
> 
> Right. The driver cannot just drop a control setting on the floor if
> it's not ready to apply it.
> 
> However, the V4L2 control framework already provides a tool to handle 
> this:
>  - the driver can ignore any .s_ctrl() calls when it can't apply the 
> controls,
>  - the driver must call v4l2_ctrl_handler_setup() when it initialized
> the hardware, so that all the control values are applied in one go.
> 
> Best regards,
> Tomasz
Yes V4L2 control framework validates the ctrls before being set.

But these controls are initialized before session initialization. So 
s_ctrl tries to set property 
"HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME" as a part of initializing 
all controls(session still in UNINIT state). But this is not any client 
request.

So we can keep a check to
1. ensure that its client requesting sync frame and
2. session in START state (both planes are streaming)

I will update the patch with these changes.