LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Brian Starkey <brian.starkey@arm.com>
To: Ezequiel Garcia <ezequiel@collabora.com>
Cc: linux-media@vger.kernel.org, kernel@collabora.com,
	Hans Verkuil <hverkuil@xs4all.nl>,
	Mauro Carvalho Chehab <mchehab@osg.samsung.com>,
	Shuah Khan <shuahkh@osg.samsung.com>,
	Pawel Osciak <pawel@osciak.com>,
	Alexandre Courbot <acourbot@chromium.org>,
	Sakari Ailus <sakari.ailus@iki.fi>,
	linux-kernel@vger.kernel.org,
	Gustavo Padovan <gustavo.padovan@collabora.com>
Subject: Re: [PATCH v9 10/15] vb2: add explicit fence user API
Date: Wed, 9 May 2018 17:33:01 +0100	[thread overview]
Message-ID: <20180509163300.GA23664@e107564-lin.cambridge.arm.com> (raw)
In-Reply-To: <e52f72ea1fdf491dd10a9a40bbced6d3bad66f7b.camel@collabora.com>

On Wed, May 09, 2018 at 12:52:26PM -0300, Ezequiel Garcia wrote:
>On Wed, 2018-05-09 at 11:33 +0100, Brian Starkey wrote:
>> Hi Ezequiel,
>>
>> On Fri, May 04, 2018 at 05:06:07PM -0300, Ezequiel Garcia wrote:
>> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
>> >
>> > Turn the reserved2 field into fence_fd that we will use to send
>> > an in-fence to the kernel or return an out-fence from the kernel to
>> > userspace.
>> >
>> > Two new flags were added, V4L2_BUF_FLAG_IN_FENCE, that should be used
>> > when sending an in-fence to the kernel to be waited on, and
>> > V4L2_BUF_FLAG_OUT_FENCE, to ask the kernel to give back an out-fence.
>> >
>> > v7: minor fixes on the Documentation (Hans Verkuil)
>> >
>> > v6: big improvement on doc (Hans Verkuil)
>> >
>> > v5: - keep using reserved2 field for cpia2
>> >    - set fence_fd to 0 for now, for compat with userspace(Mauro)
>> >
>> > v4: make it a union with reserved2 and fence_fd (Hans Verkuil)
>> >
>> > v3: make the out_fence refer to the current buffer (Hans Verkuil)
>> >
>> > v2: add documentation
>> >
>> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>> > ---
>> > Documentation/media/uapi/v4l/buffer.rst         | 45 +++++++++++++++++++++++--
>> > drivers/media/common/videobuf2/videobuf2-v4l2.c |  2 +-
>> > drivers/media/v4l2-core/v4l2-compat-ioctl32.c   |  4 +--
>> > include/uapi/linux/videodev2.h                  |  8 ++++-
>> > 4 files changed, 52 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
>> > index e2c85ddc990b..be9719cf5745 100644
>> > --- a/Documentation/media/uapi/v4l/buffer.rst
>> > +++ b/Documentation/media/uapi/v4l/buffer.rst
>> > @@ -301,10 +301,22 @@ struct v4l2_buffer
>> > 	elements in the ``planes`` array. The driver will fill in the
>> > 	actual number of valid elements in that array.
>> >     * - __u32
>> > -      - ``reserved2``
>> > +      - ``fence_fd``
>> >       -
>> > -      - A place holder for future extensions. Drivers and applications
>> > -	must set this to 0.
>> > +      - Used to communicate a fence file descriptors from userspace to kernel
>> > +	and vice-versa. On :ref:`VIDIOC_QBUF <VIDIOC_QBUF>` when sending
>> > +	an in-fence for V4L2 to wait on, the ``V4L2_BUF_FLAG_IN_FENCE`` flag must
>> > +	be used and this field set to the fence file descriptor of the in-fence.
>> > +	If the in-fence is not valid ` VIDIOC_QBUF`` returns an error.
>> > +
>> > +        To get an out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE``
>> > +	must be set, the kernel will return the out-fence file descriptor in
>> > +	this field. If it fails to create the out-fence ``VIDIOC_QBUF` returns
>> > +        an error.
>> > +
>> > +	For all other ioctls V4L2 sets this field to -1 if
>> > +	``V4L2_BUF_FLAG_IN_FENCE`` and/or ``V4L2_BUF_FLAG_OUT_FENCE`` are set,
>> > +	otherwise this field is set to 0 for backward compatibility.
>> >     * - __u32
>> >       - ``reserved``
>> >       -
>> > @@ -648,6 +660,33 @@ Buffer Flags
>> >       - Start Of Exposure. The buffer timestamp has been taken when the
>> > 	exposure of the frame has begun. This is only valid for the
>> > 	``V4L2_BUF_TYPE_VIDEO_CAPTURE`` buffer type.
>> > +    * .. _`V4L2-BUF-FLAG-IN-FENCE`:
>> > +
>> > +      - ``V4L2_BUF_FLAG_IN_FENCE``
>> > +      - 0x00200000
>> > +      - Ask V4L2 to wait on the fence passed in the ``fence_fd`` field. The
>> > +	buffer won't be queued to the driver until the fence signals. The order
>> > +	in which buffers are queued is guaranteed to be preserved, so any
>> > +	buffers queued after this buffer will also be blocked until this fence
>> > +	signals. This flag must be set before calling ``VIDIOC_QBUF``. For
>> > +	other ioctls the driver just reports the value of the flag.
>> > +
>> > +        If the fence signals the flag is cleared and not reported anymore.
>> > +	If the fence is not valid ``VIDIOC_QBUF`` returns an error.
>> > +
>> > +
>> > +    * .. _`V4L2-BUF-FLAG-OUT-FENCE`:
>> > +
>> > +      - ``V4L2_BUF_FLAG_OUT_FENCE``
>> > +      - 0x00400000
>> > +      - Request for a fence to be attached to the buffer. The driver will fill
>> > +	in the out-fence fd in the ``fence_fd`` field when :ref:`VIDIOC_QBUF
>> > +	<VIDIOC_QBUF>` returns. This flag must be set before calling
>> > +	``VIDIOC_QBUF``. For other ioctls the driver just reports the value of
>> > +	the flag.
>> > +
>> > +        If the creation of the out-fence fails ``VIDIOC_QBUF`` returns an
>> > +	error.
>> >
>>
>> I commented similarly on some of the old patch-sets, and it's a minor
>> thing, but I still think the ordering of this series is off. It's
>> strange/wrong to me document all this behaviour, and expose the flags
>> to userspace, when the functionality isn't implemented yet.
>>
>> If I apply this patch to the kernel, then the kernel doesn't do what
>> the (newly added) kernel-doc says it will.
>>
>
>This has never been a problem, and it has always been the canonical
>way of doing things.
>
>First the required macros, stubs, documentation and interfaces are added,
>and then they are implemented.

If you say so, I don't know what sets the standard but that seems
kinda backwards.

I'd expect the "flick the switch, expose to userspace" to always be
the last thing, but I'm happy to be shown examples to the contrary.

>
>I see no reason to go berserk here, unless you see an actual problem?
>Or something actually broken?
>

The only "broken" thing, is as I said - I can apply this patch to a
kernel (any kernel, because there's no dependencies in the code), and
it won't do what the kernel-doc says it will.

Maybe I'm crazy, but shouldn't comments at least be correct at the
point where they are added, even if they become incorrect later
through neglect?

Cheers,
-Brian

>The only thing I can think of is that we should return fence_fd -1
>if the flags are set. We could do it on this patch, and be consistent
>with userspace.
>
>Regards,
>Eze

  reply	other threads:[~2018-05-09 16:33 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-04 20:05 [PATCH v9 00/15] V4L2 Explicit Synchronization Ezequiel Garcia
2018-05-04 20:05 ` [PATCH v9 01/15] xilinx: regroup caps on querycap Ezequiel Garcia
2018-05-04 20:05 ` [PATCH v9 02/15] hackrf: group device capabilities Ezequiel Garcia
2018-05-04 20:06 ` [PATCH v9 03/15] omap3isp: " Ezequiel Garcia
2018-05-04 20:06 ` [PATCH v9 04/15] vb2: move vb2_ops functions to videobuf2-core.[ch] Ezequiel Garcia
2018-05-04 20:06 ` [PATCH v9 05/15] vb2: add unordered vb2_queue property for drivers Ezequiel Garcia
2018-05-07 11:03   ` Hans Verkuil
2018-05-04 20:06 ` [PATCH v9 06/15] v4l: add unordered flag to format description ioctl Ezequiel Garcia
2018-05-04 20:06 ` [PATCH v9 07/15] v4l: mark unordered formats Ezequiel Garcia
2018-05-07 13:45   ` Hans Verkuil
2018-05-04 20:06 ` [PATCH v9 08/15] cobalt: set queue as unordered Ezequiel Garcia
2018-05-07 11:04   ` Hans Verkuil
2018-05-04 20:06 ` [PATCH v9 09/15] vb2: mark codec drivers " Ezequiel Garcia
2018-05-07 11:02   ` Hans Verkuil
2018-05-04 20:06 ` [PATCH v9 10/15] vb2: add explicit fence user API Ezequiel Garcia
2018-05-07 11:30   ` Hans Verkuil
2018-05-09 10:33   ` Brian Starkey
2018-05-09 15:52     ` Ezequiel Garcia
2018-05-09 16:33       ` Brian Starkey [this message]
2018-05-09 19:10         ` Ezequiel Garcia
2018-05-09 19:40         ` Ezequiel Garcia
2018-05-04 20:06 ` [PATCH v9 11/15] vb2: add in-fence support to QBUF Ezequiel Garcia
2018-05-07 12:07   ` Hans Verkuil
2018-05-08 19:16     ` Ezequiel Garcia
2018-05-09  7:04       ` Hans Verkuil
2018-05-08 23:18     ` Gustavo Padovan
2018-05-09 10:35       ` Brian Starkey
2018-05-09 10:36   ` Brian Starkey
2018-05-09 10:52     ` Hans Verkuil
2018-05-09 16:03     ` Ezequiel Garcia
2018-05-09 16:45       ` Brian Starkey
2018-05-04 20:06 ` [PATCH v9 12/15] vb2: add out-fence " Ezequiel Garcia
2018-05-07 12:29   ` Hans Verkuil
2018-05-09 10:37   ` Brian Starkey
2018-05-04 20:06 ` [PATCH v9 13/15] v4l: introduce the fences capability Ezequiel Garcia
2018-05-04 20:06 ` [PATCH v9 14/15] v4l: Add V4L2_CAP_FENCES to drivers Ezequiel Garcia
2018-05-07 12:42   ` Hans Verkuil
2018-05-04 20:06 ` [PATCH v9 15/15] v4l: Document explicit synchronization behavior Ezequiel Garcia
2018-05-07 12:51   ` 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=20180509163300.GA23664@e107564-lin.cambridge.arm.com \
    --to=brian.starkey@arm.com \
    --cc=acourbot@chromium.org \
    --cc=ezequiel@collabora.com \
    --cc=gustavo.padovan@collabora.com \
    --cc=hverkuil@xs4all.nl \
    --cc=kernel@collabora.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=mchehab@osg.samsung.com \
    --cc=pawel@osciak.com \
    --cc=sakari.ailus@iki.fi \
    --cc=shuahkh@osg.samsung.com \
    --subject='Re: [PATCH v9 10/15] vb2: add explicit fence user API' \
    /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

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).