When client requests for a keyframe, set the property
to hardware to generate the sync frame.
Signed-off-by: Malathi Gottam <[email protected]>
---
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
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
On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <[email protected]> wrote:
>
> When client requests for a keyframe, set the property
> to hardware to generate the sync frame.
>
> Signed-off-by: Malathi Gottam <[email protected]>
> ---
> 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
>
Hi Alex,
On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
> On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <[email protected]> wrote:
>>
>> When client requests for a keyframe, set the property
>> to hardware to generate the sync frame.
>>
>> Signed-off-by: Malathi Gottam <[email protected]>
>> ---
>> 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
On Fri, Oct 12, 2018 at 4:37 PM Stanimir Varbanov
<[email protected]> wrote:
>
> Hi Alex,
>
> On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
> > On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <[email protected]> wrote:
> >>
> >> When client requests for a keyframe, set the property
> >> to hardware to generate the sync frame.
> >>
> >> Signed-off-by: Malathi Gottam <[email protected]>
> >> ---
> >> 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...
On 10/12/2018 11:06 AM, Alexandre Courbot wrote:
> On Fri, Oct 12, 2018 at 4:37 PM Stanimir Varbanov
> <[email protected]> wrote:
>>
>> Hi Alex,
>>
>> On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
>>> On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <[email protected]> wrote:
>>>>
>>>> When client requests for a keyframe, set the property
>>>> to hardware to generate the sync frame.
>>>>
>>>> Signed-off-by: Malathi Gottam <[email protected]>
>>>> ---
>>>> 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
On Fri, Oct 12, 2018 at 5:10 PM Stanimir Varbanov
<[email protected]> wrote:
>
>
>
> On 10/12/2018 11:06 AM, Alexandre Courbot wrote:
> > On Fri, Oct 12, 2018 at 4:37 PM Stanimir Varbanov
> > <[email protected]> wrote:
> >>
> >> Hi Alex,
> >>
> >> On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
> >>> On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <[email protected]> wrote:
> >>>>
> >>>> When client requests for a keyframe, set the property
> >>>> to hardware to generate the sync frame.
> >>>>
> >>>> Signed-off-by: Malathi Gottam <[email protected]>
> >>>> ---
> >>>> 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?
On Mon, Oct 22, 2018 at 3:15 PM Alexandre Courbot <[email protected]> wrote:
>
> On Fri, Oct 12, 2018 at 5:10 PM Stanimir Varbanov
> <[email protected]> wrote:
> >
> >
> >
> > On 10/12/2018 11:06 AM, Alexandre Courbot wrote:
> > > On Fri, Oct 12, 2018 at 4:37 PM Stanimir Varbanov
> > > <[email protected]> wrote:
> > >>
> > >> Hi Alex,
> > >>
> > >> On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
> > >>> On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <[email protected]> wrote:
> > >>>>
> > >>>> When client requests for a keyframe, set the property
> > >>>> to hardware to generate the sync frame.
> > >>>>
> > >>>> Signed-off-by: Malathi Gottam <[email protected]>
> > >>>> ---
> > >>>> 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
On 2018-10-23 08:37, Tomasz Figa wrote:
> On Mon, Oct 22, 2018 at 3:15 PM Alexandre Courbot
> <[email protected]> wrote:
>>
>> On Fri, Oct 12, 2018 at 5:10 PM Stanimir Varbanov
>> <[email protected]> wrote:
>> >
>> >
>> >
>> > On 10/12/2018 11:06 AM, Alexandre Courbot wrote:
>> > > On Fri, Oct 12, 2018 at 4:37 PM Stanimir Varbanov
>> > > <[email protected]> wrote:
>> > >>
>> > >> Hi Alex,
>> > >>
>> > >> On 10/12/2018 08:26 AM, Alexandre Courbot wrote:
>> > >>> On Tue, Oct 9, 2018 at 4:54 PM Malathi Gottam <[email protected]> wrote:
>> > >>>>
>> > >>>> When client requests for a keyframe, set the property
>> > >>>> to hardware to generate the sync frame.
>> > >>>>
>> > >>>> Signed-off-by: Malathi Gottam <[email protected]>
>> > >>>> ---
>> > >>>> 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.