Skip to contentSkip to conversation

media: venus: add support for key frame

PATCH Superseded Malathi Gottam 5 participants 9 messages Oct 9, 2018 – Oct 24, 2018

Subsystems (4)
Media (V4L/DVB) Arm Qualcomm mailing list Qualcomm media platform Qualcomm venus video accelerator driver

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Malathi Gottam

author
#0 lore raw

PATCH 1/1 media: venus: add support for key frame

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(+)
drivers/media/platform/qcom/venus/venc_ctrls.c +13 -0
index 45910172..f332c8e 100644
@@ -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;
Signature · 1 line
1.9.1
build bot
#1 lore raw
Thank you for the patch! Perhaps something to improve:

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

#2 lore raw
In reply to Malathi Gottam
Quoted 23 lines · Malathi Gottam

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(+)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..f332c8e 100644
@@ -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:

drivers/media/platform/qcom/venus/venc_ctrls.c static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl) · line 180

Quoted 6 lines
drivers/media/platform/qcom/venus/venc_ctrls.c +3 -0
@@ -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.

drivers/media/platform/qcom/venus/venc_ctrls.c static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl) · line 181

Quoted 1 line
drivers/media/platform/qcom/venus/venc_ctrls.c +1 -0
+

This newline is unnecessary.

Quoted 20 lines
drivers/media/platform/qcom/venus/venc_ctrls.c +7 -1
+               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

#3 lore raw

Hi Alex,

Quoted 35 lines · Alexandre Courbot
Quoted 23 lines · Malathi Gottam

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(+)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..f332c8e 100644
@@ -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:

drivers/media/platform/qcom/venus/venc_ctrls.c static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl) · line 180

Quoted 6 lines
drivers/media/platform/qcom/venus/venc_ctrls.c +3 -0
@@ -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.

Quoted 3 lines

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.

Signature · 2 lines
regards,
Stan
#4 lore raw
Quoted 39 lines · Stanimir Varbanov

Hi Alex,

Quoted 35 lines · Alexandre Courbot
Quoted 23 lines · Malathi Gottam

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(+)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..f332c8e 100644
@@ -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:

drivers/media/platform/qcom/venus/venc_ctrls.c static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl) · line 180

Quoted 6 lines
drivers/media/platform/qcom/venus/venc_ctrls.c +3 -0
@@ -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.

Quoted 6 lines
Quoted 3 lines

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...

#5 lore raw
Quoted 41 lines · Alexandre Courbot
Quoted 39 lines · Stanimir Varbanov

Hi Alex,

Quoted 35 lines · Alexandre Courbot
Quoted 23 lines · Malathi Gottam

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(+)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..f332c8e 100644
@@ -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:

drivers/media/platform/qcom/venus/venc_ctrls.c static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl) · line 180

Quoted 6 lines
drivers/media/platform/qcom/venus/venc_ctrls.c +3 -0
@@ -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.

Quoted 9 lines
Quoted 6 lines
Quoted 3 lines

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).

Signature · 2 lines
regards,
Stan
#6 lore raw
Quoted 52 lines · Stanimir Varbanov
Quoted 48 lines · Alexandre Courbot
Quoted 45 lines · Stanimir Varbanov

Hi Alex,

Quoted 35 lines · Alexandre Courbot
Quoted 23 lines · Malathi Gottam

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(+)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..f332c8e 100644
@@ -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:

drivers/media/platform/qcom/venus/venc_ctrls.c static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl) · line 180

Quoted 6 lines
drivers/media/platform/qcom/venus/venc_ctrls.c +3 -0
@@ -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.

Quoted 3 lines

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.

Quoted 6 lines
Quoted 2 lines

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?

#7 lore raw
Quoted 58 lines · Alexandre Courbot
Quoted 55 lines · Stanimir Varbanov
Quoted 50 lines · Alexandre Courbot
Quoted 39 lines · Stanimir Varbanov

Hi Alex,

Quoted 35 lines · Alexandre Courbot
Quoted 23 lines · Malathi Gottam

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(+)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..f332c8e 100644
@@ -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:

drivers/media/platform/qcom/venus/venc_ctrls.c static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl) · line 180

Quoted 6 lines
drivers/media/platform/qcom/venus/venc_ctrls.c +3 -0
@@ -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.

Quoted 6 lines
Quoted 3 lines

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...

Quoted 8 lines
Quoted 3 lines

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

author
#8 lore raw
Quoted 71 lines · Tomasz Figa
Quoted 58 lines · Alexandre Courbot
Quoted 55 lines · Stanimir Varbanov
Quoted 50 lines · Alexandre Courbot
Quoted 39 lines · Stanimir Varbanov

Hi Alex,

Quoted 35 lines · Alexandre Courbot
Quoted 23 lines · Malathi Gottam

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(+)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..f332c8e 100644
@@ -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:

drivers/media/platform/qcom/venus/venc_ctrls.c static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl) · line 180

Quoted 6 lines
drivers/media/platform/qcom/venus/venc_ctrls.c +3 -0
@@ -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.

Quoted 6 lines
Quoted 3 lines

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...

Quoted 8 lines
Quoted 3 lines

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.

Quoted 8 lines

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.