diff mbox

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

Message ID 1541163476-23249-1-git-send-email-mgottam@codeaurora.org (mailing list archive)
State Accepted, archived
Delegated to: Hans Verkuil
Headers

Commit Message

Malathi Gottam Nov. 2, 2018, 12:57 p.m. UTC 
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 | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

Tomasz Figa Nov. 3, 2018, 3:01 a.m. UTC | #1 
Hi Malathi,

On Fri, Nov 2, 2018 at 9:58 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 | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
> index 45910172..59fe7fc 100644
> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> @@ -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) {

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
Malathi Gottam Nov. 28, 2018, 12:39 p.m. UTC | #2 
On 2018-11-03 08:31, Tomasz Figa wrote:
> Hi Malathi,
> 
> On Fri, Nov 2, 2018 at 9:58 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 | 20 
>> +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c 
>> b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> index 45910172..59fe7fc 100644
>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> @@ -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) {
> 
> 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

Gentle reminder!!

Stan, please provide your view on this.

Thanks,
Malathi.
Stanimir Varbanov Nov. 29, 2018, 10:31 a.m. UTC | #3 
Hi Tomasz,

On 11/3/18 5:01 AM, Tomasz Figa wrote:
> Hi Malathi,
> 
> On Fri, Nov 2, 2018 at 9:58 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 | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> index 45910172..59fe7fc 100644
>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
>> @@ -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) {
> 
> 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,

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.

> - 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
>
Malathi Gottam Nov. 29, 2018, 11:10 a.m. UTC | #4 
Hi Stan,

On 2018-11-29 16:01, Stanimir Varbanov wrote:
> Hi Tomasz,
> 
> On 11/3/18 5:01 AM, Tomasz Figa wrote:
>> Hi Malathi,
>> 
>> On Fri, Nov 2, 2018 at 9:58 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 | 20 
>>> +++++++++++++++++++-
>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c 
>>> b/drivers/media/platform/qcom/venus/venc_ctrls.c
>>> index 45910172..59fe7fc 100644
>>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
>>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
>>> @@ -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) {
>> 
>> 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,
> 
> 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.

> 
>> - 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
>>
Tomasz Figa Nov. 29, 2018, 7:40 p.m. UTC | #5 
On Thu, Nov 29, 2018 at 3:10 AM <mgottam@codeaurora.org> wrote:
>
>
> Hi Stan,
>
> On 2018-11-29 16:01, Stanimir Varbanov wrote:
> > Hi Tomasz,
> >
> > On 11/3/18 5:01 AM, Tomasz Figa wrote:
> >> Hi Malathi,
> >>
> >> On Fri, Nov 2, 2018 at 9:58 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 | 20
> >>> +++++++++++++++++++-
> >>>  1 file changed, 19 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>> b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>> index 45910172..59fe7fc 100644
> >>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
> >>> @@ -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) {
> >>
> >> 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,
> >
> > 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.

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 <tfiga@chromium.org>

Best regards,
Tomasz
Stanimir Varbanov Dec. 7, 2018, 9:33 a.m. UTC | #6 
Hi Hans,

On 11/29/18 9:40 PM, Tomasz Figa wrote:
> On Thu, Nov 29, 2018 at 3:10 AM <mgottam@codeaurora.org> wrote:
>>
>>
>> Hi Stan,
>>
>> On 2018-11-29 16:01, Stanimir Varbanov wrote:
>>> Hi Tomasz,
>>>
>>> On 11/3/18 5:01 AM, Tomasz Figa wrote:
>>>> Hi Malathi,
>>>>
>>>> On Fri, Nov 2, 2018 at 9:58 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 | 20
>>>>> +++++++++++++++++++-
>>>>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c
>>>>> b/drivers/media/platform/qcom/venus/venc_ctrls.c
>>>>> index 45910172..59fe7fc 100644
>>>>> --- a/drivers/media/platform/qcom/venus/venc_ctrls.c
>>>>> +++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
>>>>> @@ -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) {
>>>>
>>>> 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,
>>>
>>> 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.
> 
> 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 <tfiga@chromium.org>

Thanks Tomasz!

Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
diff mbox

Patch

diff --git a/drivers/media/platform/qcom/venus/venc_ctrls.c b/drivers/media/platform/qcom/venus/venc_ctrls.c
index 45910172..59fe7fc 100644
--- a/drivers/media/platform/qcom/venus/venc_ctrls.c
+++ b/drivers/media/platform/qcom/venus/venc_ctrls.c
@@ -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;