Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <1540389162-30358-1-git-send-email-mgottam@codeaurora.org>
In-Reply-To: <1540389162-30358-1-git-send-email-mgottam@codeaurora.org>
From:   Tomasz Figa <tfiga@chromium.org>
Date:   Wed, 24 Oct 2018 23:32:19 +0900
Message-ID: <CAAFQd5CaYH-kxj+9cquObTHiRyA1VoEYHQmiQAGjdZm6J1ACfg@mail.gmail.com>
Subject: Re: [PATCH v2] media: venus: add support for key frame
To:     mgottam@codeaurora.org
Cc:     Stanimir Varbanov <stanimir.varbanov@linaro.org>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Linux Media Mailing List <linux-media@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Alexandre Courbot <acourbot@chromium.org>,
        vgarodia@codeaurora.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Oct 24, 2018 at 10:52 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 | 16 +++++++++++++++-
>  1 file changed, 15 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..6c2655d 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,15 @@ static int venc_op_s_ctrl(struct v4l2_ctrl *ctrl)
>
>                 ctr->num_b_frames = bframes;
>                 break;
> +       case V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME:
> +               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)
> +                               return ret;
> +               }
> +               break;

This is still not the right way to handle this.

Please see the documentation of this control [1]:

"V4L2_CID_MPEG_VIDEO_FORCE_KEY_FRAME (button)
Force a key frame for the next queued buffer. Applicable to encoders.
This is a general, codec-agnostic keyframe control."

Even if the driver is not streaming, it must remember that the
keyframe was requested for next buffer. The next time userspace QBUFs
an OUTPUT buffer, it should ask the hardware to encode that OUTPUT
buffer into a keyframe.

[1] https://www.kernel.org/doc/html/latest/media/uapi/v4l/extended-controls.html?highlight=v4l2_cid_mpeg_video_force_key_frame

But generally, the proper modern way for the userspace to request a
keyframe is to set the V4L2_BUF_FLAG_KEYFRAME flag in the
vb2_buffer_flag when queuing an OUTPUT buffer. It's the only
guaranteed way to ensure that the keyframe will be encoded exactly for
the selected frame. (The V4L2 control API doesn't guarantee any
synchronization between controls and buffers itself.)

Best regards,
Tomasz