linux-kernel - Re: [PATCH v3] media: docs-rst: Document m2m stateless video decoder interface

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 13 Feb 2019 10:57:10 +0100
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        Alexandre Courbot <acourbot@...omium.org>,
        Tomasz Figa <tfiga@...omium.org>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Pawel Osciak <posciak@...omium.org>,
        Linux Media Mailing List <linux-media@...r.kernel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] media: docs-rst: Document m2m stateless video decoder
 interface

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@...omium.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
>