Skip to contentSkip to conversation

media: venus: add support for key frame

PATCH In review Malathi Gottam 3 participants 7 messages Nov 2, 2018 – Dec 7, 2018

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

Malathi Gottam

author
#0 lore raw

PATCH v3 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 | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/media/platform/qcom/venus/venc_ctrls.c +19 -1
index 45910172..59fe7fc 100644
@@ -79,8 +79,10 @@ 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;
+	struct hfi_enable en = { .enable = 1 };
 	u32 bframes;
 	int ret;
+	u32 ptype;
 
 	switch (ctrl->id) {
 	case V4L2_CID_MPEG_VIDEO_BITRATE_MODE:

@@ -173,6 +175,19 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 
 		ctr->num_b_frames = bframes;
 		break;
+	case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
+		mutex_lock(&inst->lock);
+		if (inst->streamon_out && inst->streamon_cap) {
+			ptype = HFI_PROPERTY_CONFIG_VENC_REQUEST_SYNC_FRAME;
+			ret = hfi_session_set_property(inst, ptype, &en);
+
+			if (ret) {
+				mutex_unlock(&inst->lock);
+				return ret;
+			}
+		}
+		mutex_unlock(&inst->lock);
+		break;
 	default:
 		return -EINVAL;
 	}

@@ -188,7 +203,7 @@ int venc_ctrl_init(struct venus_inst *inst)
 {
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 27);
+	ret = v4l2_ctrl_handler_init(&inst->ctrl_handler, 28);
 	if (ret)
 		return ret;
 

@@ -309,6 +324,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
#1 lore raw

Hi Malathi,

Quoted 25 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 | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..59fe7fc 100644
@@ -79,8 +79,10 @@ 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;
+       struct hfi_enable en = { .enable = 1 };
        u32 bframes;
        int ret;
+       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,19 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 
                ctr->num_b_frames = bframes;
                break;
+       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
+               mutex_lock(&inst->lock);
+               if (inst->streamon_out && inst->streamon_cap) {

We had a discussion on this in v2. I don't remember seeing any conclusion.

Obviously the hardware should generate a keyframe naturally when the CAPTURE streaming starts, which is where the encoding starts, but the state of the OUTPUT queue should not affect this.

The application is free to stop and start streaming on the OUTPUT queue as it goes and it shouldn't imply any side effects in the encoded bitstream (e.g. a keyframe inserted). So:
- a sequence of STREAMOFF(OUTPUT), S_CTRL(V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME), STREAMON(OUTPUT) should explicitly generate a keyframe,
- a sequence of STREAMOFF(OUTPUT), STREAMON(OUTPUT) should _not_ explicitly generate a keyframe (the hardware may generate one, if the periodic keyframe counter is active or a scene detection algorithm decides so).

Please refer to the specification (v2 is the latest for the time being -> https://lore.kernel.org/patchwork/patch/1002476/) for further details and feel free to leave any comment for it.

Best regards,
Tomasz

author
#2 lore raw
Quoted 53 lines · Tomasz Figa

Hi Malathi,

Quoted 26 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 | 20 
+++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..59fe7fc 100644
@@ -79,8 +79,10 @@ 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;
+       struct hfi_enable en = { .enable = 1 };
        u32 bframes;
        int ret;
+       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,19 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 
                ctr->num_b_frames = bframes;
                break;
+       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
+               mutex_lock(&inst->lock);
+               if (inst->streamon_out && inst->streamon_cap) {

We had a discussion on this in v2. I don't remember seeing any conclusion.

Obviously the hardware should generate a keyframe naturally when the CAPTURE streaming starts, which is where the encoding starts, but the state of the OUTPUT queue should not affect this.

The application is free to stop and start streaming on the OUTPUT queue as it goes and it shouldn't imply any side effects in the encoded bitstream (e.g. a keyframe inserted). So:
- a sequence of STREAMOFF(OUTPUT), S_CTRL(V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME), STREAMON(OUTPUT) should explicitly generate a keyframe,
- a sequence of STREAMOFF(OUTPUT), STREAMON(OUTPUT) should _not_ explicitly generate a keyframe (the hardware may generate one, if the periodic keyframe counter is active or a scene detection algorithm decides so).

Quoted 6 lines

Please refer to the specification (v2 is the latest for the time being -> https://lore.kernel.org/patchwork/patch/1002476/) for further details and feel free to leave any comment for it.

Best regards,
Tomasz

Gentle reminder!!

Stan, please provide your view on this.

Thanks,
Malathi.

#3 lore raw
In reply to Tomasz Figa

Hi Tomasz,

Quoted 41 lines · Tomasz Figa

Hi Malathi,

Quoted 25 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 | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..59fe7fc 100644
@@ -79,8 +79,10 @@ 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;
+       struct hfi_enable en = { .enable = 1 };
        u32 bframes;
        int ret;
+       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,19 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
 
                ctr->num_b_frames = bframes;
                break;
+       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
+               mutex_lock(&inst->lock);
+               if (inst->streamon_out && inst->streamon_cap) {

We had a discussion on this in v2. I don't remember seeing any conclusion.

Obviously the hardware should generate a keyframe naturally when the CAPTURE streaming starts, which is where the encoding starts, but the state of the OUTPUT queue should not affect this.

Quoted 6 lines

The application is free to stop and start streaming on the OUTPUT queue as it goes and it shouldn't imply any side effects in the encoded bitstream (e.g. a keyframe inserted). So:
- a sequence of STREAMOFF(OUTPUT), S_CTRL(V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME), STREAMON(OUTPUT) should explicitly generate a keyframe,

I agree with you, but presently we don't follow strictly the stateful encoder spec. In this spirit I think proposed patch is applicable to the current state of the encoder driver, and your comment should be addressed in the follow-up patches where we have to re-factor a bit start/stop_streaming according to the encoder documentation.

But until then we have to get that patch.

Quoted 12 lines

- a sequence of STREAMOFF(OUTPUT), STREAMON(OUTPUT) should _not_ explicitly generate a keyframe (the hardware may generate one, if the periodic keyframe counter is active or a scene detection algorithm decides so).

Please refer to the specification (v2 is the latest for the time being -> https://lore.kernel.org/patchwork/patch/1002476/) for further details and feel free to leave any comment for it.

Best regards,
Tomasz

Signature · 2 lines
regards,
Stan
author
#4 lore raw

Hi Stan,

Quoted 53 lines · Stanimir Varbanov

Hi Tomasz,

Quoted 43 lines · Tomasz Figa

Hi Malathi,

Quoted 26 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 | 20 
+++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..59fe7fc 100644
@@ -79,8 +79,10 @@ 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;
+       struct hfi_enable en = { .enable = 1 };
        u32 bframes;
        int ret;
+       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 · line 181

Quoted 7 lines
drivers/media/platform/qcom/venus/venc_ctrls.c +3 -0
@@ -173,6 +175,19 @@ static int venc_op_s_ctrl(struct v4l2_ctrl 
 *ctrl)
 
                ctr->num_b_frames = bframes;
                break;
+       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
+               mutex_lock(&inst->lock);
+               if (inst->streamon_out && inst->streamon_cap) {

We had a discussion on this in v2. I don't remember seeing any conclusion.

Obviously the hardware should generate a keyframe naturally when the CAPTURE streaming starts, which is where the encoding starts, but the state of the OUTPUT queue should not affect this.

Quoted 6 lines

The application is free to stop and start streaming on the OUTPUT queue as it goes and it shouldn't imply any side effects in the encoded bitstream (e.g. a keyframe inserted). So:
- a sequence of STREAMOFF(OUTPUT), S_CTRL(V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME), STREAMON(OUTPUT) should explicitly generate a keyframe,

Quoted 7 lines

I agree with you, but presently we don't follow strictly the stateful encoder spec. In this spirit I think proposed patch is applicable to the current state of the encoder driver, and your comment should be addressed in the follow-up patches where we have to re-factor a bit start/stop_streaming according to the encoder documentation.

But until then we have to get that patch.

So I can see that this patch is good implementation of forcing sync frame under current encoder state.

Can you please ack the same.

Thanks,
Malathi.

Quoted 13 lines
Quoted 13 lines

- a sequence of STREAMOFF(OUTPUT), STREAMON(OUTPUT) should _not_ explicitly generate a keyframe (the hardware may generate one, if the periodic keyframe counter is active or a scene detection algorithm decides so).

Please refer to the specification (v2 is the latest for the time being -> https://lore.kernel.org/patchwork/patch/1002476/) for further details and feel free to leave any comment for it.

Best regards,
Tomasz

Acked-by
#5 lore raw
Quoted 68 lines · On Thu, Nov 29, 2018 at 3:10 AM <[email protected]> wrote:

Hi Stan,

Quoted 63 lines · Stanimir Varbanov

Hi Tomasz,

Quoted 46 lines · Tomasz Figa

Hi Malathi,

Quoted 27 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 | 20
+++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..59fe7fc 100644
@@ -79,8 +79,10 @@ 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;
+       struct hfi_enable en = { .enable = 1 };
        u32 bframes;
        int ret;
+       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 · line 181

Quoted 7 lines
drivers/media/platform/qcom/venus/venc_ctrls.c +3 -0
@@ -173,6 +175,19 @@ static int venc_op_s_ctrl(struct v4l2_ctrl
 *ctrl)
 
                ctr->num_b_frames = bframes;
                break;
+       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
+               mutex_lock(&inst->lock);
+               if (inst->streamon_out && inst->streamon_cap) {

We had a discussion on this in v2. I don't remember seeing any conclusion.

Obviously the hardware should generate a keyframe naturally when the CAPTURE streaming starts, which is where the encoding starts, but the state of the OUTPUT queue should not affect this.

Quoted 6 lines

The application is free to stop and start streaming on the OUTPUT queue as it goes and it shouldn't imply any side effects in the encoded bitstream (e.g. a keyframe inserted). So:
- a sequence of STREAMOFF(OUTPUT), S_CTRL(V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME), STREAMON(OUTPUT) should explicitly generate a keyframe,

I agree with you, but presently we don't follow strictly the stateful encoder spec. In this spirit I think proposed patch is applicable to the
current state of the encoder driver, and your comment should be addressed in the follow-up patches where we have to re-factor a bit start/stop_streaming according to the encoder documentation.

Quoted 7 lines
Quoted 1 line

But until then we have to get that patch.

So I can see that this patch is good implementation of forcing sync frame
under current encoder state.

Can you please ack the same.

Okay, assuming that when you start streaming you naturally get a keyframe, I'm okay with this patch, since it actually fixes the missing key frame request, so from the general encoder interface point of view:

Acked-by Tomasz Figa <[email protected]>

Best regards,
Tomasz

Acked-by
#6 lore raw

Hi Hans,

Quoted 75 lines · Tomasz Figa
Quoted 74 lines · On Thu, Nov 29, 2018 at 3:10 AM <[email protected]> wrote:

Hi Stan,

Quoted 56 lines · Stanimir Varbanov

Hi Tomasz,

Quoted 46 lines · Tomasz Figa

Hi Malathi,

Quoted 27 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 | 20
+++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)
drivers/media/platform/qcom/venus/venc_ctrls.c +2 -0
index 45910172..59fe7fc 100644
@@ -79,8 +79,10 @@ 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;
+       struct hfi_enable en = { .enable = 1 };
        u32 bframes;
        int ret;
+       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 · line 181

Quoted 7 lines
drivers/media/platform/qcom/venus/venc_ctrls.c +3 -0
@@ -173,6 +175,19 @@ static int venc_op_s_ctrl(struct v4l2_ctrl
 *ctrl)
 
                ctr->num_b_frames = bframes;
                break;
+       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
+               mutex_lock(&inst->lock);
+               if (inst->streamon_out && inst->streamon_cap) {

We had a discussion on this in v2. I don't remember seeing any conclusion.

Obviously the hardware should generate a keyframe naturally when the CAPTURE streaming starts, which is where the encoding starts, but the state of the OUTPUT queue should not affect this.

Quoted 6 lines

The application is free to stop and start streaming on the OUTPUT queue as it goes and it shouldn't imply any side effects in the encoded bitstream (e.g. a keyframe inserted). So:
- a sequence of STREAMOFF(OUTPUT), S_CTRL(V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME), STREAMON(OUTPUT) should explicitly generate a keyframe,

Quoted 8 lines

I agree with you, but presently we don't follow strictly the stateful encoder spec. In this spirit I think proposed patch is applicable to the
current state of the encoder driver, and your comment should be addressed in the follow-up patches where we have to re-factor a bit start/stop_streaming according to the encoder documentation.

But until then we have to get that patch.

So I can see that this patch is good implementation of forcing sync frame
under current encoder state.

Quoted 8 lines
Quoted 1 line

Can you please ack the same.

Okay, assuming that when you start streaming you naturally get a keyframe, I'm okay with this patch, since it actually fixes the missing key frame request, so from the general encoder interface point of view:

Acked-by Tomasz Figa <[email protected]>

Thanks Tomasz!

Acked-by Stanimir Varbanov <[email protected]>
Signature · 2 lines
regards,
Stan