Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200928164431.21884-1-stanimir.varbanov@linaro.org> <20200928164431.21884-2-stanimir.varbanov@linaro.org>
In-Reply-To: <20200928164431.21884-2-stanimir.varbanov@linaro.org>
From:   Alexandre Courbot <acourbot@chromium.org>
Date:   Fri, 9 Oct 2020 21:45:27 +0900
Message-ID: <CAPBb6MUMFnzd6zZE4C4T+bX5p=3iKPZp+S_hrTLECvHTHYXrtQ@mail.gmail.com>
Subject: Re: [PATCH 1/3] venus: vdec: Fix non reliable setting of LAST flag
To:     Stanimir Varbanov <stanimir.varbanov@linaro.org>
Cc:     Linux Media Mailing List <linux-media@vger.kernel.org>,
        linux-arm-msm@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
        Vikash Garodia <vgarodia@codeaurora.org>,
        Mansur Alisha Shaik <mansur@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Sep 29, 2020 at 1:44 AM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> In real use of dynamic-resolution-change it is observed that the
> LAST buffer flag (which marks the last decoded buffer with the
> resolution before the resolution-change event) is not reliably set.
>
> Fix this by set the LAST buffer flag on next queued capture buffer
> after the resolution-change event.
>
> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/core.h    |  5 +-
>  drivers/media/platform/qcom/venus/helpers.c |  6 +++
>  drivers/media/platform/qcom/venus/vdec.c    | 52 ++++++++++++---------
>  3 files changed, 38 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> index 7b79a33dc9d6..e30eeaf0ada9 100644
> --- a/drivers/media/platform/qcom/venus/core.h
> +++ b/drivers/media/platform/qcom/venus/core.h
> @@ -274,7 +274,6 @@ enum venus_dec_state {
>         VENUS_DEC_STATE_DRAIN           = 5,
>         VENUS_DEC_STATE_DECODING        = 6,
>         VENUS_DEC_STATE_DRC             = 7,
> -       VENUS_DEC_STATE_DRC_FLUSH_DONE  = 8,
>  };
>
>  struct venus_ts_metadata {
> @@ -339,7 +338,7 @@ struct venus_ts_metadata {
>   * @priv:      a private for HFI operations callbacks
>   * @session_type:      the type of the session (decoder or encoder)
>   * @hprop:     a union used as a holder by get property
> - * @last_buf:  last capture buffer for dynamic-resoluton-change
> + * @next_buf_last: a flag to mark next queued capture buffer as last
>   */
>  struct venus_inst {
>         struct list_head list;
> @@ -401,7 +400,7 @@ struct venus_inst {
>         union hfi_get_property hprop;
>         unsigned int core_acquired: 1;
>         unsigned int bit_depth;
> -       struct vb2_buffer *last_buf;
> +       bool next_buf_last;
>  };
>
>  #define IS_V1(core)    ((core)->res->hfi_version == HFI_VERSION_1XX)
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 50439eb1ffea..5ca3920237c5 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -1347,6 +1347,12 @@ void venus_helper_vb2_buf_queue(struct vb2_buffer *vb)
>
>         v4l2_m2m_buf_queue(m2m_ctx, vbuf);
>
> +       /* Skip processing queued capture buffers after LAST flag */
> +       if (inst->session_type == VIDC_SESSION_TYPE_DEC &&
> +           V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type) &&
> +           inst->codec_state == VENUS_DEC_STATE_DRC)
> +               goto unlock;
> +
>         cache_payload(inst, vb);
>
>         if (inst->session_type == VIDC_SESSION_TYPE_ENC &&
> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> index ea13170a6a2c..c11bdf3ca21b 100644
> --- a/drivers/media/platform/qcom/venus/vdec.c
> +++ b/drivers/media/platform/qcom/venus/vdec.c
> @@ -914,10 +914,6 @@ static int vdec_start_capture(struct venus_inst *inst)
>                 return 0;
>
>  reconfigure:
> -       ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, true);
> -       if (ret)
> -               return ret;
> -
>         ret = vdec_output_conf(inst);
>         if (ret)
>                 return ret;
> @@ -954,6 +950,7 @@ static int vdec_start_capture(struct venus_inst *inst)
>         inst->streamon_cap = 1;
>         inst->sequence_cap = 0;
>         inst->reconfig = false;
> +       inst->next_buf_last = false;

Is this needed? Whether a resolution change occurs should only be
dependent on what the OUTPUT queue receives, so even if the CAPTURE
queue is stopped and resumed for some reason, pending resolution
change events and their associated LAST buffer should still be
emitted. With this statement we are taking the risk of sending a
change resolution event without a corresponding LAST buffer (so the
following LAST buffer might be misinterpreted).

>
>         return 0;
>
> @@ -985,6 +982,7 @@ static int vdec_start_output(struct venus_inst *inst)
>         venus_helper_init_instance(inst);
>         inst->sequence_out = 0;
>         inst->reconfig = false;
> +       inst->next_buf_last = false;

This one I understand better - if the client seeks, it should probably
check for pending events before resuming.

>
>         ret = vdec_set_properties(inst);
>         if (ret)
> @@ -1078,9 +1076,7 @@ static int vdec_stop_capture(struct venus_inst *inst)
>                 inst->codec_state = VENUS_DEC_STATE_STOPPED;
>                 break;
>         case VENUS_DEC_STATE_DRC:
> -               WARN_ON(1);
> -               fallthrough;
> -       case VENUS_DEC_STATE_DRC_FLUSH_DONE:
> +               ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, true);
>                 inst->codec_state = VENUS_DEC_STATE_CAPTURE_SETUP;
>                 venus_helper_free_dpb_bufs(inst);
>                 break;
> @@ -1204,9 +1200,28 @@ static void vdec_buf_cleanup(struct vb2_buffer *vb)
>  static void vdec_vb2_buf_queue(struct vb2_buffer *vb)
>  {
>         struct venus_inst *inst = vb2_get_drv_priv(vb->vb2_queue);
> +       struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +       static const struct v4l2_event eos = { .type = V4L2_EVENT_EOS };
>
>         vdec_pm_get_put(inst);
>
> +       mutex_lock(&inst->lock);
> +
> +       if (inst->next_buf_last && V4L2_TYPE_IS_CAPTURE(vb->vb2_queue->type) &&
> +           inst->codec_state == VENUS_DEC_STATE_DRC) {
> +               vbuf->flags |= V4L2_BUF_FLAG_LAST;
> +               vbuf->sequence = inst->sequence_cap++;
> +               vbuf->field = V4L2_FIELD_NONE;
> +               vb2_set_plane_payload(vb, 0, 0);
> +               v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE);
> +               v4l2_event_queue_fh(&inst->fh, &eos);

I don't think publishing an EOS event here is correct. As the spec says:

"
... Last of the buffers will have the V4L2_BUF_FLAG_LAST flag set. To
determine the sequence to follow, the client must check if there is
any pending event and:

* if a V4L2_EVENT_SOURCE_CHANGE event with changes set to
V4L2_EVENT_SRC_CH_RESOLUTION is pending, the Dynamic Resolution Change
sequence needs to be followed,
* if a V4L2_EVENT_EOS event is pending, the End of Stream sequence
needs to be followed.
"

With this we will have *both* resolution change and EOS events pending
when the client checks the event queue after receiving the LAST
buffer.


> +               inst->next_buf_last = false;
> +               mutex_unlock(&inst->lock);
> +               return;
> +       }
> +
> +       mutex_unlock(&inst->lock);
> +
>         venus_helper_vb2_buf_queue(vb);
>  }
>
> @@ -1250,13 +1265,6 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
>                 vb->timestamp = timestamp_us * NSEC_PER_USEC;
>                 vbuf->sequence = inst->sequence_cap++;
>
> -               if (inst->last_buf == vb) {
> -                       inst->last_buf = NULL;
> -                       vbuf->flags |= V4L2_BUF_FLAG_LAST;
> -                       vb2_set_plane_payload(vb, 0, 0);
> -                       vb->timestamp = 0;
> -               }
> -
>                 if (vbuf->flags & V4L2_BUF_FLAG_LAST) {
>                         const struct v4l2_event ev = { .type = V4L2_EVENT_EOS };
>
> @@ -1344,13 +1352,14 @@ static void vdec_event_change(struct venus_inst *inst,
>                 struct vb2_v4l2_buffer *last;
>                 int ret;
>
> -               last = v4l2_m2m_last_dst_buf(inst->m2m_ctx);
> -               if (last)
> -                       inst->last_buf = &last->vb2_buf;
> +               inst->next_buf_last = true;
>
> -               ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, false);
> -               if (ret)
> -                       dev_dbg(dev, VDBGH "flush output error %d\n", ret);
> +               last = v4l2_m2m_last_dst_buf(inst->m2m_ctx);
> +               if (last) {
> +                       ret = hfi_session_flush(inst, HFI_FLUSH_OUTPUT, false);
> +                       if (ret)
> +                               dev_dbg(dev, VDBGH "flush output error %d\n", ret);
> +               }
>         }
>
>         inst->reconfig = true;
> @@ -1395,8 +1404,7 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event,
>
>  static void vdec_flush_done(struct venus_inst *inst)
>  {
> -       if (inst->codec_state == VENUS_DEC_STATE_DRC)
> -               inst->codec_state = VENUS_DEC_STATE_DRC_FLUSH_DONE;
> +       dev_dbg(inst->core->dev_dec, VDBGH "flush done\n");
>  }
>
>  static const struct hfi_inst_ops vdec_hfi_ops = {
> --
> 2.17.1
>