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: <20190306080019.159676-1-acourbot@chromium.org>
 <371df0e4ec9e38d83d11171cbd98f19954cbf787.camel@ndufresne.ca>
 <da5aa9ab957923a72e30b169a3c242c4fe6260f1.camel@bootlin.com>
 <c07703459c7e48904bd7fd83be1e675c70eee83c.camel@ndufresne.ca>
 <d81d0112b99feba0a1899f3722077a4aeec9860c.camel@bootlin.com>
 <439b7f57aa3ba2b2ed5b043f961ef87cb83912af.camel@ndufresne.ca>
 <59e23c5ca5bfbadf9441ea06da2e9b9b5898c6d7.camel@bootlin.com>
 <0b495143bb260cf9f8927ee541e7f001842ac5c3.camel@ndufresne.ca>
 <CAPBb6MVG+3jQcw3AuhYDYCZ0YJ0aX=TmEuM5izh12GLw9V6B8Q@mail.gmail.com> <0f4775b1bb04e136e3a425df0ba6a201594acdbd.camel@bootlin.com>
In-Reply-To: <0f4775b1bb04e136e3a425df0ba6a201594acdbd.camel@bootlin.com>
From:   Alexandre Courbot <acourbot@chromium.org>
Date:   Wed, 17 Apr 2019 14:39:50 +0900
Message-ID: <CAPBb6MU4yJVJq_AasQ0p9PPxtUyt73yB-10=ATCudUTsiwKOSg@mail.gmail.com>
Subject: Re: [PATCH v4] media: docs-rst: Document m2m stateless video decoder interface
To:     Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Cc:     Nicolas Dufresne <nicolas@ndufresne.ca>,
        Tomasz Figa <tfiga@chromium.org>,
        Maxime Ripard <maxime.ripard@bootlin.com>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Dafna Hirschfeld <dafna3@gmail.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Linux Media Mailing List <linux-media@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Paul,

On Tue, Apr 16, 2019 at 4:55 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> Le mardi 16 avril 2019 =C3=A0 16:22 +0900, Alexandre Courbot a =C3=A9crit=
 :
>
> [...]
>
> > Thanks for this great discussion. Let me try to summarize the status
> > of this thread + the IRC discussion and add my own thoughts:
> >
> > Proper support for multiple decoding units (e.g. H.264 slices) per
> > frame should not be an afterthought ; compliance to encoded formats
> > depend on it, and the benefit of lower latency is a significant
> > consideration for vendors.
> >
> > m2m, which we use for all stateless codecs, has a strong assumption
> > that one OUTPUT buffer consumed results in one CAPTURE buffer being
> > produced. This assumption can however be overruled: at least the venus
> > driver does it to implement the stateful specification.
> >
> > So we need a way to specify frame boundaries when submitting encoded
> > content to the driver. One request should contain a single OUTPUT
> > buffer, containing a single decoding unit, but we need a way to
> > specify whether the driver should directly produce a CAPTURE buffer
> > from this request, or keep using the same CAPTURE buffer with
> > subsequent requests.
> >
> > I can think of 2 ways this can be expressed:
> > 1) We keep the current m2m behavior as the default (a CAPTURE buffer
> > is produced), and add a flag to ask the driver to change that behavior
> > and hold on the CAPTURE buffer and reuse it with the next request(s) ;
>
> That would kind of break the stateless idea. I think we need requests
> to be fully independent of eachother and have some entity that
> coordinates requests for this kind of things.

Side note: the idea that stateless decoders are entirely stateless is
not completely accurate anyway. When we specify a resolution on the
OUTPUT queue, we already store some state. What matters IIUC is that
the *hardware* behaves in a stateless manner. I don't think we should
refrain from storing some internal driver state if it makes sense.

Back to the topic: the effect of this flag would just be that the
first buffer is the CAPTURE queue is not removed, i.e. the next
request will work on the same buffer. It doesn't really preserve any
state - if the next request is the beginning of a different frame,
then the previous work will be discarded and the driver will behave as
it should, not considering any previous state.

>
> > 2) We specify that no CAPTURE buffer is produced by default, unless a
> > flag asking so is specified.
> >
> > The flag could be specified in one of two ways:
> > a) As a new v4l2_buffer.flag for the OUTPUT buffer ;
> > b) As a dedicated control, either format-specific or more common to all=
 codecs.
>
> I think we must aim for a generic solution that would be at least
> common to all codecs, and if possible common to requests regardless of
> whether they concern video decoding or not.
>
> I really like the idea of introducing a requests batch/group/queue,
> which groups requests together and allows marking them done when the
> whole group is done being decoded. For that, we explicitly mark one of
> the requests as the final one, so that we can continue adding requests
> to the batch even when it's already being processed. When all the
> requests are done being decoded, we can mark them done.

I'd need to see this idea more developed (with maybe an example of the
sequence of IOCTLs) to form an opinion about it. Also would need to be
given a few examples of where this could be used outside of stateless
codecs. Then we will have to address what this means for requests:
your argument against using a "release CAPTURE buffer" flag was that
requests won't be fully independent from each other anymore, but I
don't see that situation changing with batches. Then, does the end of
a batch only means that a CAPTURE buffer should be released, or are
other actions required for non-codec use-cases? There are lots and
lots of questions like this one lurking.

>
> With that, we also need some tweaking in the core to look for an
> available capture buffer that matches the output buffer's timestamp
> before trying to dequeue the next available capture buffer

I don't think that would be strictly necessary, unless we want to be
able to decode slices from different frames before the first one is
completed?

> This way,
> the first request of the batch will get any queued capture buffer, but
> subsequent requests will find the matchung capture buffer by timestamp.
>
> I think that's basically all we need to handle that and the two aspects
> (picking by timestamp and requests groups) are rather independent and
> the latter could probably be used in other situations than video
> decoding.
>
> What do you think?

At the current point I'd like to avoid over-engineering things.
Introducing a request batch mechanism would mean more months spent
before we can set the stateless codec API in stone, and at some point
we need to settle and release something that people can use. We don't
even have clear idea of what batches would look like and in which
cases they would be used. The idea of an extra flag is simple and
AFAICT would do the job nicely, so why not proceed with this for the
time being?

Cheers,
Alex.