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: <20201201042322.3346817-1-frkoenig@chromium.org> <57cc5999-e54c-219c-812b-71b214dbe760@linaro.org>
In-Reply-To: <57cc5999-e54c-219c-812b-71b214dbe760@linaro.org>
From:   Alexandre Courbot <acourbot@chromium.org>
Date:   Wed, 2 Dec 2020 14:58:29 +0900
Message-ID: <CAPBb6MUU0TayGLq-Jifh1O2RfhoTjimY7GEQcqdoxQ6itybbfw@mail.gmail.com>
Subject: Re: [PATCH v2] venus: vdec: Handle DRC after drain
To:     Stanimir Varbanov <stanimir.varbanov@linaro.org>
Cc:     Fritz Koenig <frkoenig@chromium.org>,
        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>,
        Dikshita Agarwal <dikshita@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Dec 2, 2020 at 7:34 AM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Fritz,
>
> On 12/1/20 6:23 AM, Fritz Koenig wrote:
> > If the DRC is near the end of the stream the client
> > may send a V4L2_DEC_CMD_STOP before the DRC occurs.
> > V4L2_DEC_CMD_STOP puts the driver into the
> > VENUS_DEC_STATE_DRAIN state.  DRC must be aware so
> > that after the DRC event the state can be restored
> > correctly.
> >
> > Signed-off-by: Fritz Koenig <frkoenig@chromium.org>
> > ---
> >
> > v2: remove TODO
> >
> >  drivers/media/platform/qcom/venus/core.h |  1 +
> >  drivers/media/platform/qcom/venus/vdec.c | 17 +++++++++++++++--
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/qcom/venus/core.h b/drivers/media/platform/qcom/venus/core.h
> > index 2b0899bf5b05f..1680c936c06fb 100644
> > --- a/drivers/media/platform/qcom/venus/core.h
> > +++ b/drivers/media/platform/qcom/venus/core.h
> > @@ -406,6 +406,7 @@ struct venus_inst {
> >       unsigned int core_acquired: 1;
> >       unsigned int bit_depth;
> >       bool next_buf_last;
> > +     bool drain_active;
>
> Could you introduce a new codec state instead of adding a flag for such
> corner case.
>
> I think that we will need to handle at least one more corner case (DRC
> during seek operation),

Just happen to have posted a patch for that. :)

https://lkml.org/lkml/2020/12/2/24

> so then we have to introduce another flag, and
> this is not a good solution to me. These additional flags will
> complicate the state handling and will make the code readability worst
>
> I'd introduce: VENUS_DEC_STATE_DRC_DURING_DRAIN or something similar.

I'm wondering what is the best approach here. As you see in my patch,
I did not have to introduce a new state but relied instead on the
state of next_buf_last (which may or may not be correct - maybe we can
think of a way to merge both patches into one?). Flushes, either
explicit or implicitly triggered by a DRC, are more than a state by
themselves but rather an extra dimension from which other states can
still apply. I'm afraid we already have many states as it is and
adding more might add complexity.

A lot of the recent issues we had came from that number of states,
notably from the fact that not all states are always tested when they
should (and fall back to the default: branch of a switch case that
does nothing). I think we could improve the robustness of this driver
if we mandate that each state check must be done using a switch
statement without a default: branch. That would force us to ensure
that each newly introduced state is considered in every situation
where it might be relevant.

>
> >  };
> >
> >  #define IS_V1(core)  ((core)->res->hfi_version == HFI_VERSION_1XX)
> > diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> > index 5671cf3458a68..1d551b4d651a8 100644
> > --- a/drivers/media/platform/qcom/venus/vdec.c
> > +++ b/drivers/media/platform/qcom/venus/vdec.c
> > @@ -523,8 +523,10 @@ vdec_decoder_cmd(struct file *file, void *fh, struct v4l2_decoder_cmd *cmd)
> >
> >               ret = hfi_session_process_buf(inst, &fdata);
> >
> > -             if (!ret && inst->codec_state == VENUS_DEC_STATE_DECODING)
> > +             if (!ret && inst->codec_state == VENUS_DEC_STATE_DECODING) {
> >                       inst->codec_state = VENUS_DEC_STATE_DRAIN;
> > +                     inst->drain_active = true;
> > +             }
> >       }
> >
> >  unlock:
> > @@ -976,10 +978,14 @@ static int vdec_start_capture(struct venus_inst *inst)
> >
> >       inst->codec_state = VENUS_DEC_STATE_DECODING;
> >
> > +     if (inst->drain_active)
> > +             inst->codec_state = VENUS_DEC_STATE_DRAIN;
> > +
> >       inst->streamon_cap = 1;
> >       inst->sequence_cap = 0;
> >       inst->reconfig = false;
> >       inst->next_buf_last = false;
> > +     inst->drain_active = false;
> >
> >       return 0;
> >
> > @@ -1105,6 +1111,7 @@ static int vdec_stop_capture(struct venus_inst *inst)
> >               /* fallthrough */
> >       case VENUS_DEC_STATE_DRAIN:
> >               inst->codec_state = VENUS_DEC_STATE_STOPPED;
> > +             inst->drain_active = false;
> >               /* fallthrough */
> >       case VENUS_DEC_STATE_SEEK:
> >               vdec_cancel_dst_buffers(inst);
> > @@ -1304,8 +1311,10 @@ static void vdec_buf_done(struct venus_inst *inst, unsigned int buf_type,
> >
> >                       v4l2_event_queue_fh(&inst->fh, &ev);
> >
> > -                     if (inst->codec_state == VENUS_DEC_STATE_DRAIN)
> > +                     if (inst->codec_state == VENUS_DEC_STATE_DRAIN) {
> > +                             inst->drain_active = false;
> >                               inst->codec_state = VENUS_DEC_STATE_STOPPED;
> > +                     }
> >               }
> >
> >               if (!bytesused)
> > @@ -1429,9 +1438,13 @@ static void vdec_event_notify(struct venus_inst *inst, u32 event,
> >       case EVT_SYS_EVENT_CHANGE:
> >               switch (data->event_type) {
> >               case HFI_EVENT_DATA_SEQUENCE_CHANGED_SUFFICIENT_BUF_RESOURCES:
> > +                     if (inst->codec_state == VENUS_DEC_STATE_DRAIN)
> > +                             inst->codec_state = VENUS_DEC_STATE_DECODING;
>
> Could you move this state transition into vdec_event_change(). That way
> we will do it only once.
>
> >                       vdec_event_change(inst, data, true);
> >                       break;
> >               case HFI_EVENT_DATA_SEQUENCE_CHANGED_INSUFFICIENT_BUF_RESOURCES:
> > +                     if (inst->codec_state == VENUS_DEC_STATE_DRAIN)
> > +                             inst->codec_state = VENUS_DEC_STATE_DECODING;
>
> ditto
>
> >                       vdec_event_change(inst, data, false);
> >                       break;
> >               case HFI_EVENT_RELEASE_BUFFER_REFERENCE:
> >
>
> --
> regards,
> Stan