From: Hans Verkuil <hverkuil@xs4all.nl>
To: Paul Kocialkowski <paul.kocialkowski@bootlin.com>,
Alexandre Courbot <acourbot@chromium.org>,
Tomasz Figa <tfiga@chromium.org>,
Maxime Ripard <maxime.ripard@bootlin.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Pawel Osciak <posciak@chromium.org>,
Linux Media Mailing List <linux-media@vger.kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] media: docs-rst: Document m2m stateless video decoder interface
Date: Wed, 13 Feb 2019 10:57:10 +0100 [thread overview]
Message-ID: <7caf9381-e920-f5fc-e8f9-a54ac2733add@xs4all.nl> (raw)
In-Reply-To: <3de0825971b91ea0b8fd349f4ecf8164de14254a.camel@bootlin.com>
On 2/13/19 10:20 AM, Paul Kocialkowski wrote:
> Hi,
>
> On Wed, 2019-02-13 at 09:59 +0100, Hans Verkuil wrote:
>> On 2/13/19 6:58 AM, Alexandre Courbot wrote:
>>> On Wed, Feb 13, 2019 at 2:53 PM Alexandre Courbot <acourbot@chromium.org> wrote:
>>>> [snip]
>>>> +Buffers used as reference frames can be queued back to the ``CAPTURE`` queue as
>>>> +soon as all the frames they are affecting have been queued to the ``OUTPUT``
>>>> +queue. The driver will refrain from using the reference buffer as a decoding
>>>> +target until all the frames depending on it are decoded.
>>>
>>> Just want to highlight this part in order to make sure that this is
>>> indeed what we agreed on. The recent changes to vb2_find_timestamp()
>>> suggest this, but maybe I misunderstood the intent. It makes the
>>> kernel responsible for tracking referenced buffers and not using them
>>> until all the dependent frames are decoded, something the client could
>>> also do.
>>
>> I don't think this is quite right. Once this patch https://patchwork.linuxtv.org/patch/54275/
>> is in the vb2 core will track when a buffer can no longer be used as a
>> reference buffer because the underlying memory might have disappeared.
>>
>> The core does not check if it makes sense to use a buffer as a reference
>> frame, just that it is valid memory.
>>
>> So the driver has to check that the timestamp refers to an existing
>> buffer, but userspace has to check that it queues everything in the
>> right order and that the reference buffer won't be overwritten
>> before the last output buffer using that reference buffer has been
>> decoded.
>>
>> So I would say that the second sentence in your paragraph is wrong.
>>
>> The first sentence isn't quite right either, but I am not really sure how
>> to phrase it. It is possible to queue a reference buffer even if
>> not all output buffers referring to it have been decoded, provided
>> that by the time the driver starts to use this buffer this actually
>> has happened.
>
> Is there a way we can guarantee this? Looking at the rest of the spec,
> it says that capture buffers "are returned in decode order" but that
> doesn't imply that they are picked up in the order they are queued.
>
> It seems quite troublesome for the core to check whether each queued
> capture buffer is used as a reference for one of the queued requests to
> decide whether to pick it up or not.
The core only checks that the timestamp points to a valid buffer.
It is not up to the core or the driver to do anything else. If userspace
gives a reference to a wrong buffer or one that is already overwritten,
then you just get bad decoded video, but nothing crashes.
>
>> But this is an optimization and theoretically it can depend on the
>> driver behavior. It is always safe to only queue a reference frame
>> when all frames depending on it have been decoded. So I am leaning
>> towards not complicating matters and keeping your first sentence
>> as-is.
>
> Yes, I believe it would be much simpler to require userspace to only
> queue capture buffers once they are no longer needed as references.
I think that's what we should document, but in cases where you know
the hardware (i.e. an embedded system) it should be allowed to optimize
and have the application queue a capture buffer containing a reference
frame even if it is still in use by already queued output buffers.
That way you can achieve optimal speed and memory usage.
I think this is a desirable feature.
> This also means that the dmabuf fd can't be changed so we are
> guaranteed that memory will still be there.
This is easy enough to check for, so I rather have some checks in
the core for this than prohibiting optimizing the decoder memory
usage.
Regards,
Hans
>
> Cheers,
>
> Paul
>
next prev parent reply other threads:[~2019-02-13 9:57 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-13 5:53 [PATCH v3] media: docs-rst: Document m2m stateless video decoder interface Alexandre Courbot
2019-02-13 5:58 ` Alexandre Courbot
2019-02-13 8:59 ` Hans Verkuil
2019-02-13 9:20 ` Paul Kocialkowski
2019-02-13 9:57 ` Hans Verkuil [this message]
2019-02-13 11:04 ` Paul Kocialkowski
2019-02-26 3:33 ` Alexandre Courbot
2019-02-28 10:14 ` Hans Verkuil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7caf9381-e920-f5fc-e8f9-a54ac2733add@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=acourbot@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=maxime.ripard@bootlin.com \
--cc=mchehab@kernel.org \
--cc=paul.kocialkowski@bootlin.com \
--cc=posciak@chromium.org \
--cc=tfiga@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).