[PATCHv2 00/10] Post-v18: Request API updates

[PATCHv2 00/10] Post-v18: Request API updates

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Hi all,

This patch series sits on top of my v18 series for the Request API.
It makes some final (?) changes as discussed in:

https://www.mail-archive.com/linux-media@vger.kernel.org/msg134419.html

and:

https://www.spinics.net/lists/linux-media/msg138596.html

The combined v18 patches + this series is available here:

https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv18-1

Updated v4l-utils for this is available here:

https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=request

Userspace visible changes:

- Invalid request_fd values now return -EINVAL instead of -ENOENT.
- It is no longer possible to use VIDIOC_G_EXT_CTRLS for requests
  that are not completed. -EACCES is returned in that case.
- Attempting to use requests if requests are not supported by the driver
  will result in -EACCES instead of -EPERM.

Driver visible changes (important for the cedrus driver!):

Drivers should set the new vb2_queue 'supports_request' bitfield to 1
if a vb2_queue can support requests. Otherwise the queue cannot be
used with requests.

This bitfield is also used to fill in the new capabilities field
in struct v4l2_requestbuffers and v4l2_create_buffers.

Changes since v1:

- Updated patch 4/10 to explain how to query the capabilities
  with REQBUFS/CREATE_BUFS with a minimum of side-effects
  (requested by Tomasz).
- Added patches 6-10:
  6: Sakari found a corner case: when accessing a request the
     request has to be protected from being re-inited. New
     media_request_(un)lock_for_access helpers are added for this.
  7: use these helpers in g_ext_ctrls.
  8: make s/try_ext_ctrls more robust by keeping the request
     references until we're fully done setting/trying the controls.
  9: Change two more EPERM's to EACCES. EPERM suggests that you can
     fix it by changing permissions somehow, but in this case the
     driver simply doesn't support requests at all.
  10: Update the request documentation based on Laurent's comments:
      https://www.spinics.net/lists/linux-media/msg139152.html
      To do: split off the V4L2 specifics into a V4L2 specific
      rst file. But this will take more time and is for later.

Regards,

	Hans

Hans Verkuil (10):
  media-request: return -EINVAL for invalid request_fds
  v4l2-ctrls: return -EACCES if request wasn't completed
  buffer.rst: only set V4L2_BUF_FLAG_REQUEST_FD for QBUF
  videodev2.h: add new capabilities for buffer types
  vb2: set reqbufs/create_bufs capabilities
  media-request: add media_request_(un)lock_for_access
  v4l2-ctrls: use media_request_(un)lock_for_access
  v4l2-ctrls: improve media_request_(un)lock_for_update
  media-request: EPERM -> EACCES
  media-request: update documentation

 .../uapi/mediactl/media-ioc-request-alloc.rst |  3 +-
 .../uapi/mediactl/media-request-ioc-queue.rst |  7 +--
 .../media/uapi/mediactl/request-api.rst       | 51 +++++++++++--------
 .../uapi/mediactl/request-func-close.rst      |  1 +
 .../media/uapi/mediactl/request-func-poll.rst |  2 +-
 Documentation/media/uapi/v4l/buffer.rst       | 22 +++++---
 .../media/uapi/v4l/vidioc-create-bufs.rst     | 14 ++++-
 .../media/uapi/v4l/vidioc-g-ext-ctrls.rst     | 48 +++++++++--------
 Documentation/media/uapi/v4l/vidioc-qbuf.rst  | 32 +++++++-----
 .../media/uapi/v4l/vidioc-reqbufs.rst         | 42 ++++++++++++++-
 .../media/common/videobuf2/videobuf2-v4l2.c   | 19 ++++++-
 drivers/media/media-request.c                 | 20 ++++++--
 drivers/media/platform/vim2m.c                |  1 +
 drivers/media/platform/vivid/vivid-core.c     |  5 ++
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c |  4 +-
 drivers/media/v4l2-core/v4l2-ctrls.c          | 32 +++++++-----
 drivers/media/v4l2-core/v4l2-ioctl.c          |  4 +-
 include/media/media-request.h                 | 46 +++++++++++++++++
 include/media/videobuf2-core.h                |  2 +
 include/uapi/linux/videodev2.h                | 13 ++++-
 20 files changed, 269 insertions(+), 99 deletions(-)

-- 
2.18.0

[PATCHv2 01/10] media-request: return -EINVAL for invalid request_fds

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

Instead of returning -ENOENT when a request_fd was not found (VIDIOC_QBUF
and VIDIOC_G/S/TRY_EXT_CTRLS), we now return -EINVAL. This is in line
with what we do when invalid dmabuf fds are passed to e.g. VIDIOC_QBUF.

Also document that EINVAL is returned for invalid m.fd values, we never
documented that.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
---
 Documentation/media/uapi/v4l/buffer.rst        |  4 ++--
 .../media/uapi/v4l/vidioc-g-ext-ctrls.rst      | 18 ++++++++----------
 Documentation/media/uapi/v4l/vidioc-qbuf.rst   | 12 +++++-------
 drivers/media/media-request.c                  |  6 ++++--
 4 files changed, 19 insertions(+), 21 deletions(-)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index dd0065a95ea0..35c2fadd10de 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -313,8 +313,8 @@ struct v4l2_buffer
 	queued to that request. This is set by the user when calling
 	:ref:`ioctl VIDIOC_QBUF <VIDIOC_QBUF>` and ignored by other ioctls.
 	If the device does not support requests, then ``EPERM`` will be returned.
-	If requests are supported but an invalid request FD is given, then
-	``ENOENT`` will be returned.
+	If requests are supported but an invalid request file descriptor is
+	given, then ``EINVAL`` will be returned.
 
 
 
diff --git a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
index 771fd1161277..9c56a9b6e98a 100644
--- a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
+++ b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
@@ -101,8 +101,8 @@ then the controls are not applied immediately when calling
 :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>`, but instead are applied by
 the driver for the buffer associated with the same request.
 If the device does not support requests, then ``EPERM`` will be returned.
-If requests are supported but an invalid request FD is given, then
-``ENOENT`` will be returned.
+If requests are supported but an invalid request file descriptor is given,
+then ``EINVAL`` will be returned.
 
 An attempt to call :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>` for a
 request that has already been queued will result in an ``EBUSY`` error.
@@ -301,8 +301,8 @@ still cause this situation.
       - File descriptor of the request to be used by this operation. Only
 	valid if ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``.
 	If the device does not support requests, then ``EPERM`` will be returned.
-	If requests are supported but an invalid request FD is given, then
-	``ENOENT`` will be returned.
+	If requests are supported but an invalid request file descriptor is
+	given, then ``EINVAL`` will be returned.
     * - __u32
       - ``reserved``\ [1]
       - Reserved for future extensions.
@@ -378,11 +378,13 @@ appropriately. The generic error codes are described at the
 
 EINVAL
     The struct :c:type:`v4l2_ext_control` ``id`` is
-    invalid, the struct :c:type:`v4l2_ext_controls`
+    invalid, or the struct :c:type:`v4l2_ext_controls`
     ``which`` is invalid, or the struct
     :c:type:`v4l2_ext_control` ``value`` was
     inappropriate (e.g. the given menu index is not supported by the
-    driver). This error code is also returned by the
+    driver), or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL``
+    but the given ``request_fd`` was invalid.
+    This error code is also returned by the
     :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>` and :ref:`VIDIOC_TRY_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>` ioctls if two or
     more control values are in conflict.
 
@@ -409,7 +411,3 @@ EACCES
 EPERM
     The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
     device does not support requests.
-
-ENOENT
-    The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
-    the given ``request_fd`` was invalid.
diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
index 0e415f2551b2..7bff69c15452 100644
--- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
+++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
@@ -105,8 +105,8 @@ until the request itself is queued. Also, the driver will apply any
 settings associated with the request for this buffer. This field will
 be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set.
 If the device does not support requests, then ``EPERM`` will be returned.
-If requests are supported but an invalid request FD is given, then
-``ENOENT`` will be returned.
+If requests are supported but an invalid request file descriptor is given,
+then ``EINVAL`` will be returned.
 
 .. caution::
    It is not allowed to mix queuing requests with queuing buffers directly.
@@ -152,7 +152,9 @@ EAGAIN
 EINVAL
     The buffer ``type`` is not supported, or the ``index`` is out of
     bounds, or no buffers have been allocated yet, or the ``userptr`` or
-    ``length`` are invalid.
+    ``length`` are invalid, or the ``V4L2_BUF_FLAG_REQUEST_FD`` flag was
+    set but the the given ``request_fd`` was invalid, or ``m.fd`` was
+    an invalid DMABUF file descriptor.
 
 EIO
     ``VIDIOC_DQBUF`` failed due to an internal error. Can also indicate
@@ -179,7 +181,3 @@ EPERM
     the application now tries to queue it directly, or vice versa (it is
     not permitted to mix the two APIs). Or an attempt is made to queue a
     CAPTURE buffer to a request for a :ref:`memory-to-memory device <codec>`.
-
-ENOENT
-    The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the the given
-    ``request_fd`` was invalid.
diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
index 4b0ce8fde7c9..4cee67e6657e 100644
--- a/drivers/media/media-request.c
+++ b/drivers/media/media-request.c
@@ -244,7 +244,7 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd)
 
 	filp = fget(request_fd);
 	if (!filp)
-		return ERR_PTR(-ENOENT);
+		goto err_no_req_fd;
 
 	if (filp->f_op != &request_fops)
 		goto err_fput;
@@ -268,7 +268,9 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd)
 err_fput:
 	fput(filp);
 
-	return ERR_PTR(-ENOENT);
+err_no_req_fd:
+	dev_dbg(mdev->dev, "cannot find request_fd %d\n", request_fd);
+	return ERR_PTR(-EINVAL);
 }
 EXPORT_SYMBOL_GPL(media_request_get_by_fd);
 
-- 
2.18.0

[PATCHv2 02/10] v4l2-ctrls: return -EACCES if request wasn't completed

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

For now (this might be relaxed in the future) we do not allow getting
controls from a request that isn't completed. In that case we return
-EACCES. Update the documentation accordingly.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
---
 .../media/uapi/v4l/vidioc-g-ext-ctrls.rst      | 18 +++++++++---------
 drivers/media/v4l2-core/v4l2-ctrls.c           |  5 ++---
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
index 9c56a9b6e98a..ad8908ce3095 100644
--- a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
+++ b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
@@ -107,13 +107,12 @@ then ``EINVAL`` will be returned.
 An attempt to call :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>` for a
 request that has already been queued will result in an ``EBUSY`` error.
 
-If ``request_fd`` is specified and ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``
-during a call to :ref:`VIDIOC_G_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>`, then the
-returned values will be the values currently set for the request (or the
-hardware value if none is set) if the request has not yet been queued, or the
-values of the controls at the time of request completion if it has already
-completed. Attempting to get controls while the request has been queued but
-not yet completed will result in an ``EBUSY`` error.
+If ``request_fd`` is specified and ``which`` is set to
+``V4L2_CTRL_WHICH_REQUEST_VAL`` during a call to
+:ref:`VIDIOC_G_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>`, then it will return the
+values of the controls at the time of request completion.
+If the request is not yet completed, then this will result in an
+``EACCES`` error.
 
 The driver will only set/get these controls if all control values are
 correct. This prevents the situation where only some of the controls
@@ -405,8 +404,9 @@ ENOSPC
     and this error code is returned.
 
 EACCES
-    Attempt to try or set a read-only control or to get a write-only
-    control.
+    Attempt to try or set a read-only control, or to get a write-only
+    control, or to get a control from a request that has not yet been
+    completed.
 
 EPERM
     The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index a197b60183f5..ccaf3068de6d 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -3301,10 +3301,9 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
 		if (IS_ERR(req))
 			return PTR_ERR(req);
 
-		if (req->state != MEDIA_REQUEST_STATE_IDLE &&
-		    req->state != MEDIA_REQUEST_STATE_COMPLETE) {
+		if (req->state != MEDIA_REQUEST_STATE_COMPLETE) {
 			media_request_put(req);
-			return -EBUSY;
+			return -EACCES;
 		}
 
 		obj = v4l2_ctrls_find_req_obj(hdl, req, false);
-- 
2.18.0

[PATCHv2 03/10] buffer.rst: only set V4L2_BUF_FLAG_REQUEST_FD for QBUF

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

Document that V4L2_BUF_FLAG_REQUEST_FD should only be used with
VIDIOC_QBUF and cleared otherwise.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
---
 Documentation/media/uapi/v4l/buffer.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index 35c2fadd10de..1865cd5b9d3c 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -312,6 +312,8 @@ struct v4l2_buffer
         and flag ``V4L2_BUF_FLAG_REQUEST_FD`` is set, then the buffer will be
 	queued to that request. This is set by the user when calling
 	:ref:`ioctl VIDIOC_QBUF <VIDIOC_QBUF>` and ignored by other ioctls.
+	Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls
+	other than :ref:`VIDIOC_QBUF <VIDIOC_QBUF>`.
 	If the device does not support requests, then ``EPERM`` will be returned.
 	If requests are supported but an invalid request file descriptor is
 	given, then ``EINVAL`` will be returned.
-- 
2.18.0

[PATCHv2 04/10] videodev2.h: add new capabilities for buffer types

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

VIDIOC_REQBUFS and VIDIOC_CREATE_BUFFERS will return capabilities
telling userspace what the given buffer type is capable of.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 .../media/uapi/v4l/vidioc-create-bufs.rst     | 14 ++++++-
 .../media/uapi/v4l/vidioc-reqbufs.rst         | 42 ++++++++++++++++++-
 include/uapi/linux/videodev2.h                | 13 +++++-
 3 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
index a39e18d69511..eadf6f757fbf 100644
--- a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
+++ b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
@@ -102,7 +102,19 @@ than the number requested.
       - ``format``
       - Filled in by the application, preserved by the driver.
     * - __u32
-      - ``reserved``\ [8]
+      - ``capabilities``
+      - Set by the driver. If 0, then the driver doesn't support
+        capabilities. In that case all you know is that the driver is
+	guaranteed to support ``V4L2_MEMORY_MMAP`` and *might* support
+	other :c:type:`v4l2_memory` types. It will not support any others
+	capabilities. See :ref:`here <v4l2-buf-capabilities>` for a list of the
+	capabilities.
+
+	If you want to just query the capabilities without making any
+	other changes, then set ``count`` to 0, ``memory`` to
+	``V4L2_MEMORY_MMAP`` and ``format.type`` to the buffer type.
+    * - __u32
+      - ``reserved``\ [7]
       - A place holder for future extensions. Drivers and applications
 	must set the array to zero.
 
diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
index 316f52c8a310..d4bbbb0c60e8 100644
--- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
+++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
@@ -88,10 +88,50 @@ any DMA in progress, an implicit
 	``V4L2_MEMORY_DMABUF`` or ``V4L2_MEMORY_USERPTR``. See
 	:c:type:`v4l2_memory`.
     * - __u32
-      - ``reserved``\ [2]
+      - ``capabilities``
+      - Set by the driver. If 0, then the driver doesn't support
+        capabilities. In that case all you know is that the driver is
+	guaranteed to support ``V4L2_MEMORY_MMAP`` and *might* support
+	other :c:type:`v4l2_memory` types. It will not support any others
+	capabilities.
+
+	If you want to query the capabilities with a minimum of side-effects,
+	then this can be called with ``count`` set to 0, ``memory`` set to
+	``V4L2_MEMORY_MMAP`` and ``type`` set to the buffer type. This will
+	free any previously allocated buffers, so this is typically something
+	that will be done at the start of the application.
+    * - __u32
+      - ``reserved``\ [1]
       - A place holder for future extensions. Drivers and applications
 	must set the array to zero.
 
+.. tabularcolumns:: |p{6.1cm}|p{2.2cm}|p{8.7cm}|
+
+.. _v4l2-buf-capabilities:
+.. _V4L2-BUF-CAP-SUPPORTS-MMAP:
+.. _V4L2-BUF-CAP-SUPPORTS-USERPTR:
+.. _V4L2-BUF-CAP-SUPPORTS-DMABUF:
+.. _V4L2-BUF-CAP-SUPPORTS-REQUESTS:
+
+.. cssclass:: longtable
+
+.. flat-table:: V4L2 Buffer Capabilities Flags
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       3 1 4
+
+    * - ``V4L2_BUF_CAP_SUPPORTS_MMAP``
+      - 0x00000001
+      - This buffer type supports the ``V4L2_MEMORY_MMAP`` streaming mode.
+    * - ``V4L2_BUF_CAP_SUPPORTS_USERPTR``
+      - 0x00000002
+      - This buffer type supports the ``V4L2_MEMORY_USERPTR`` streaming mode.
+    * - ``V4L2_BUF_CAP_SUPPORTS_DMABUF``
+      - 0x00000004
+      - This buffer type supports the ``V4L2_MEMORY_DMABUF`` streaming mode.
+    * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``
+      - 0x00000008
+      - This buffer type supports :ref:`requests <media-request-api>`.
 
 Return Value
 ============
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 91126b7312f8..490fc9964d97 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -856,9 +856,16 @@ struct v4l2_requestbuffers {
 	__u32			count;
 	__u32			type;		/* enum v4l2_buf_type */
 	__u32			memory;		/* enum v4l2_memory */
-	__u32			reserved[2];
+	__u32			capabilities;
+	__u32			reserved[1];
 };
 
+/* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
+#define V4L2_BUF_CAP_SUPPORTS_MMAP	(1 << 0)
+#define V4L2_BUF_CAP_SUPPORTS_USERPTR	(1 << 1)
+#define V4L2_BUF_CAP_SUPPORTS_DMABUF	(1 << 2)
+#define V4L2_BUF_CAP_SUPPORTS_REQUESTS	(1 << 3)
+
 /**
  * struct v4l2_plane - plane info for multi-planar buffers
  * @bytesused:		number of bytes occupied by data in the plane (payload)
@@ -2312,6 +2319,7 @@ struct v4l2_dbg_chip_info {
  *		return: number of created buffers
  * @memory:	enum v4l2_memory; buffer memory type
  * @format:	frame format, for which buffers are requested
+ * @capabilities: capabilities of this buffer type.
  * @reserved:	future extensions
  */
 struct v4l2_create_buffers {
@@ -2319,7 +2327,8 @@ struct v4l2_create_buffers {
 	__u32			count;
 	__u32			memory;
 	struct v4l2_format	format;
-	__u32			reserved[8];
+	__u32			capabilities;
+	__u32			reserved[7];
 };
 
 /*
-- 
2.18.0

[PATCHv2 05/10] vb2: set reqbufs/create_bufs capabilities

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

Set the capabilities field of v4l2_requestbuffers and v4l2_create_buffers.

The various mapping modes were easy, but for signaling the request capability
a new 'supports_requests' bitfield was added to videobuf2-core.h (and set in
vim2m and vivid). Drivers have to set this bitfield for any queue where
requests are supported.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
---
 .../media/common/videobuf2/videobuf2-v4l2.c   | 19 ++++++++++++++++++-
 drivers/media/platform/vim2m.c                |  1 +
 drivers/media/platform/vivid/vivid-core.c     |  5 +++++
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c |  4 +++-
 drivers/media/v4l2-core/v4l2-ioctl.c          |  4 ++--
 include/media/videobuf2-core.h                |  2 ++
 6 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index a70df16d68f1..2caaabd50532 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -384,7 +384,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
 			return -EPERM;
 		}
 		return 0;
-	} else if (q->uses_qbuf) {
+	} else if (q->uses_qbuf || !q->supports_requests) {
 		dprintk(1, "%s: queue does not use requests\n", opname);
 		return -EPERM;
 	}
@@ -619,10 +619,24 @@ int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b)
 }
 EXPORT_SYMBOL(vb2_querybuf);
 
+static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
+{
+	*caps = 0;
+	if (q->io_modes & VB2_MMAP)
+		*caps |= V4L2_BUF_CAP_SUPPORTS_MMAP;
+	if (q->io_modes & VB2_USERPTR)
+		*caps |= V4L2_BUF_CAP_SUPPORTS_USERPTR;
+	if (q->io_modes & VB2_DMABUF)
+		*caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
+	if (q->supports_requests)
+		*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
+}
+
 int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 {
 	int ret = vb2_verify_memory_type(q, req->memory, req->type);
 
+	fill_buf_caps(q, &req->capabilities);
 	return ret ? ret : vb2_core_reqbufs(q, req->memory, &req->count);
 }
 EXPORT_SYMBOL_GPL(vb2_reqbufs);
@@ -654,6 +668,7 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
 	int ret = vb2_verify_memory_type(q, create->memory, f->type);
 	unsigned i;
 
+	fill_buf_caps(q, &create->capabilities);
 	create->index = q->num_buffers;
 	if (create->count == 0)
 		return ret != -EBUSY ? ret : 0;
@@ -861,6 +876,7 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv,
 	struct video_device *vdev = video_devdata(file);
 	int res = vb2_verify_memory_type(vdev->queue, p->memory, p->type);
 
+	fill_buf_caps(vdev->queue, &p->capabilities);
 	if (res)
 		return res;
 	if (vb2_queue_is_busy(vdev, file))
@@ -882,6 +898,7 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
 			p->format.type);
 
 	p->index = vdev->queue->num_buffers;
+	fill_buf_caps(vdev->queue, &p->capabilities);
 	/*
 	 * If count == 0, then just check if memory and type are valid.
 	 * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0.
diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index 5423f0dd0821..40fbb1e429af 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -855,6 +855,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *ds
 	src_vq->mem_ops = &vb2_vmalloc_memops;
 	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	src_vq->lock = &ctx->dev->dev_mutex;
+	src_vq->supports_requests = true;
 
 	ret = vb2_queue_init(src_vq);
 	if (ret)
diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
index 3f6f5cbe1b60..e7f1394832fe 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -1077,6 +1077,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->min_buffers_needed = 2;
 		q->lock = &dev->mutex;
 		q->dev = dev->v4l2_dev.dev;
+		q->supports_requests = true;
 
 		ret = vb2_queue_init(q);
 		if (ret)
@@ -1097,6 +1098,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->min_buffers_needed = 2;
 		q->lock = &dev->mutex;
 		q->dev = dev->v4l2_dev.dev;
+		q->supports_requests = true;
 
 		ret = vb2_queue_init(q);
 		if (ret)
@@ -1117,6 +1119,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->min_buffers_needed = 2;
 		q->lock = &dev->mutex;
 		q->dev = dev->v4l2_dev.dev;
+		q->supports_requests = true;
 
 		ret = vb2_queue_init(q);
 		if (ret)
@@ -1137,6 +1140,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->min_buffers_needed = 2;
 		q->lock = &dev->mutex;
 		q->dev = dev->v4l2_dev.dev;
+		q->supports_requests = true;
 
 		ret = vb2_queue_init(q);
 		if (ret)
@@ -1156,6 +1160,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->min_buffers_needed = 8;
 		q->lock = &dev->mutex;
 		q->dev = dev->v4l2_dev.dev;
+		q->supports_requests = true;
 
 		ret = vb2_queue_init(q);
 		if (ret)
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 633465d21d04..0028e0be6b5b 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -251,7 +251,8 @@ struct v4l2_create_buffers32 {
 	__u32			count;
 	__u32			memory;	/* enum v4l2_memory */
 	struct v4l2_format32	format;
-	__u32			reserved[8];
+	__u32			capabilities;
+	__u32			reserved[7];
 };
 
 static int __bufsize_v4l2_format(struct v4l2_format32 __user *p32, u32 *size)
@@ -411,6 +412,7 @@ static int put_v4l2_create32(struct v4l2_create_buffers __user *p64,
 	if (!access_ok(VERIFY_WRITE, p32, sizeof(*p32)) ||
 	    copy_in_user(p32, p64,
 			 offsetof(struct v4l2_create_buffers32, format)) ||
+	    assign_in_user(&p32->capabilities, &p64->capabilities) ||
 	    copy_in_user(p32->reserved, p64->reserved, sizeof(p64->reserved)))
 		return -EFAULT;
 	return __put_v4l2_format32(&p64->format, &p32->format);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 2a84ca9e328a..87dba0b9c0a7 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1877,7 +1877,7 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
 	if (ret)
 		return ret;
 
-	CLEAR_AFTER_FIELD(p, memory);
+	CLEAR_AFTER_FIELD(p, capabilities);
 
 	return ops->vidioc_reqbufs(file, fh, p);
 }
@@ -1918,7 +1918,7 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
 	if (ret)
 		return ret;
 
-	CLEAR_AFTER_FIELD(create, format);
+	CLEAR_AFTER_FIELD(create, capabilities);
 
 	v4l_sanitize_format(&create->format);
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 881f53b38b26..6c76b9802589 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -472,6 +472,7 @@ struct vb2_buf_ops {
  * @quirk_poll_must_check_waiting_for_buffers: Return %EPOLLERR at poll when QBUF
  *              has not been called. This is a vb1 idiom that has been adopted
  *              also by vb2.
+ * @supports_requests: this queue supports the Request API.
  * @uses_qbuf:	qbuf was used directly for this queue. Set to 1 the first
  *		time this is called. Set to 0 when the queue is canceled.
  *		If this is 1, then you cannot queue buffers from a request.
@@ -545,6 +546,7 @@ struct vb2_queue {
 	unsigned			fileio_write_immediately:1;
 	unsigned			allow_zero_bytesused:1;
 	unsigned		   quirk_poll_must_check_waiting_for_buffers:1;
+	unsigned			supports_requests:1;
 	unsigned			uses_qbuf:1;
 	unsigned			uses_requests:1;
 
-- 
2.18.0

[PATCHv2 06/10] media-request: add media_request_(un)lock_for_access

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add helper functions to prevent a completed request from being
re-inited while it is being accessed.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-request.c | 10 ++++++++
 include/media/media-request.h | 46 +++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
index 4cee67e6657e..414197645e09 100644
--- a/drivers/media/media-request.c
+++ b/drivers/media/media-request.c
@@ -43,6 +43,7 @@ static void media_request_clean(struct media_request *req)
 	/* Just a sanity check. No other code path is allowed to change this. */
 	WARN_ON(req->state != MEDIA_REQUEST_STATE_CLEANING);
 	WARN_ON(req->updating_count);
+	WARN_ON(req->access_count);
 
 	list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
 		media_request_object_unbind(obj);
@@ -50,6 +51,7 @@ static void media_request_clean(struct media_request *req)
 	}
 
 	req->updating_count = 0;
+	req->access_count = 0;
 	WARN_ON(req->num_incomplete_objects);
 	req->num_incomplete_objects = 0;
 	wake_up_interruptible_all(&req->poll_wait);
@@ -198,6 +200,13 @@ static long media_request_ioctl_reinit(struct media_request *req)
 		spin_unlock_irqrestore(&req->lock, flags);
 		return -EBUSY;
 	}
+	if (req->access_count) {
+		dev_dbg(mdev->dev,
+			"request: %s is being accessed, cannot reinit\n",
+			req->debug_str);
+		spin_unlock_irqrestore(&req->lock, flags);
+		return -EBUSY;
+	}
 	req->state = MEDIA_REQUEST_STATE_CLEANING;
 	spin_unlock_irqrestore(&req->lock, flags);
 
@@ -313,6 +322,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd)
 	spin_lock_init(&req->lock);
 	init_waitqueue_head(&req->poll_wait);
 	req->updating_count = 0;
+	req->access_count = 0;
 
 	*alloc_fd = fd;
 
diff --git a/include/media/media-request.h b/include/media/media-request.h
index ac02019c1d77..707c7577f46d 100644
--- a/include/media/media-request.h
+++ b/include/media/media-request.h
@@ -53,6 +53,7 @@ struct media_request_object;
  * @debug_str: Prefix for debug messages (process name:fd)
  * @state: The state of the request
  * @updating_count: count the number of request updates that are in progress
+ * @access_count: count the number of request accesses that are in progress
  * @objects: List of @struct media_request_object request objects
  * @num_incomplete_objects: The number of incomplete objects in the request
  * @poll_wait: Wait queue for poll
@@ -64,6 +65,7 @@ struct media_request {
 	char debug_str[TASK_COMM_LEN + 11];
 	enum media_request_state state;
 	unsigned int updating_count;
+	unsigned int access_count;
 	struct list_head objects;
 	unsigned int num_incomplete_objects;
 	struct wait_queue_head poll_wait;
@@ -72,6 +74,50 @@ struct media_request {
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 
+/**
+ * media_request_lock_for_access - Lock the request to access its objects
+ *
+ * @req: The media request
+ *
+ * Use before accessing a completed request. A reference to the request must
+ * be held during the access. This usually takes place automatically through
+ * a file handle. Use @media_request_unlock_for_access when done.
+ */
+static inline int __must_check
+media_request_lock_for_access(struct media_request *req)
+{
+	unsigned long flags;
+	int ret = -EBUSY;
+
+	spin_lock_irqsave(&req->lock, flags);
+	if (req->state == MEDIA_REQUEST_STATE_COMPLETE) {
+		req->access_count++;
+		ret = 0;
+	}
+	spin_unlock_irqrestore(&req->lock, flags);
+
+	return ret;
+}
+
+/**
+ * media_request_unlock_for_access - Unlock a request previously locked for
+ *				     access
+ *
+ * @req: The media request
+ *
+ * Unlock a request that has previously been locked using
+ * @media_request_lock_for_access.
+ */
+static inline void media_request_unlock_for_access(struct media_request *req)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&req->lock, flags);
+	if (!WARN_ON(!req->access_count))
+		req->access_count--;
+	spin_unlock_irqrestore(&req->lock, flags);
+}
+
 /**
  * media_request_lock_for_update - Lock the request for updating its objects
  *
-- 
2.18.0

[PATCHv2 07/10] v4l2-ctrls: use media_request_(un)lock_for_access

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

When getting control values from a completed request, we have
to protect the request against being re-inited why it is
being accessed by calling media_request_(un)lock_for_access.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index ccaf3068de6d..cc266a4a6e88 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -3289,11 +3289,10 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
 		     struct v4l2_ext_controls *cs)
 {
 	struct media_request_object *obj = NULL;
+	struct media_request *req = NULL;
 	int ret;
 
 	if (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL) {
-		struct media_request *req;
-
 		if (!mdev || cs->request_fd < 0)
 			return -EINVAL;
 
@@ -3306,11 +3305,18 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
 			return -EACCES;
 		}
 
+		ret = media_request_lock_for_access(req);
+		if (ret) {
+			media_request_put(req);
+			return ret;
+		}
+
 		obj = v4l2_ctrls_find_req_obj(hdl, req, false);
-		/* Reference to the request held through obj */
-		media_request_put(req);
-		if (IS_ERR(obj))
+		if (IS_ERR(obj)) {
+			media_request_unlock_for_access(req);
+			media_request_put(req);
 			return PTR_ERR(obj);
+		}
 
 		hdl = container_of(obj, struct v4l2_ctrl_handler,
 				   req_obj);
@@ -3318,8 +3324,11 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
 
 	ret = v4l2_g_ext_ctrls_common(hdl, cs);
 
-	if (obj)
+	if (obj) {
+		media_request_unlock_for_access(req);
 		media_request_object_put(obj);
+		media_request_put(req);
+	}
 	return ret;
 }
 EXPORT_SYMBOL(v4l2_g_ext_ctrls);
-- 
2.18.0

[PATCHv2 08/10] v4l2-ctrls: improve media_request_(un)lock_for_update

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The request reference count was decreased again once a reference to the
request object was taken. Postpone this until we finished using the object.

In theory I think it is possible that the request_fd can be closed by
the application from another thread. In that case when request_put is
called the whole request would be freed.

It's highly unlikely, but let's just be safe and fix this potential
race condition.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index cc266a4a6e88..95d065d54308 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -3657,10 +3657,9 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 		}
 
 		obj = v4l2_ctrls_find_req_obj(hdl, req, set);
-		/* Reference to the request held through obj */
-		media_request_put(req);
 		if (IS_ERR(obj)) {
 			media_request_unlock_for_update(req);
+			media_request_put(req);
 			return PTR_ERR(obj);
 		}
 		hdl = container_of(obj, struct v4l2_ctrl_handler,
@@ -3670,8 +3669,9 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
 	ret = try_set_ext_ctrls_common(fh, hdl, cs, set);
 
 	if (obj) {
-		media_request_unlock_for_update(obj->req);
+		media_request_unlock_for_update(req);
 		media_request_object_put(obj);
+		media_request_put(req);
 	}
 
 	return ret;
-- 
2.18.0

[PATCHv2 09/10] media-request: EPERM -> EACCES

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

If requests are not supported by the driver, then return EACCES, not
EPERM. This is consistent with the error that an invalid request_fd will
give, and if requests are not supported, then all request_fd values are
invalid.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/media/uapi/v4l/buffer.rst           |  2 +-
 .../media/uapi/v4l/vidioc-g-ext-ctrls.rst         |  9 ++++-----
 Documentation/media/uapi/v4l/vidioc-qbuf.rst      | 15 +++++++++------
 drivers/media/media-request.c                     |  4 ++--
 4 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index 1865cd5b9d3c..58a6d7d336e6 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -314,7 +314,7 @@ struct v4l2_buffer
 	:ref:`ioctl VIDIOC_QBUF <VIDIOC_QBUF>` and ignored by other ioctls.
 	Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls
 	other than :ref:`VIDIOC_QBUF <VIDIOC_QBUF>`.
-	If the device does not support requests, then ``EPERM`` will be returned.
+	If the device does not support requests, then ``EACCES`` will be returned.
 	If requests are supported but an invalid request file descriptor is
 	given, then ``EINVAL`` will be returned.
 
diff --git a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
index ad8908ce3095..54a999df5aec 100644
--- a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
+++ b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
@@ -100,7 +100,7 @@ file descriptor and ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``,
 then the controls are not applied immediately when calling
 :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>`, but instead are applied by
 the driver for the buffer associated with the same request.
-If the device does not support requests, then ``EPERM`` will be returned.
+If the device does not support requests, then ``EACCES`` will be returned.
 If requests are supported but an invalid request file descriptor is given,
 then ``EINVAL`` will be returned.
 
@@ -233,7 +233,7 @@ still cause this situation.
 	these controls have to be retrieved from a request or tried/set for
 	a request. In the latter case the ``request_fd`` field contains the
 	file descriptor of the request that should be used. If the device
-	does not support requests, then ``EPERM`` will be returned.
+	does not support requests, then ``EACCES`` will be returned.
 
 	.. note::
 
@@ -299,7 +299,7 @@ still cause this situation.
       - ``request_fd``
       - File descriptor of the request to be used by this operation. Only
 	valid if ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``.
-	If the device does not support requests, then ``EPERM`` will be returned.
+	If the device does not support requests, then ``EACCES`` will be returned.
 	If requests are supported but an invalid request file descriptor is
 	given, then ``EINVAL`` will be returned.
     * - __u32
@@ -408,6 +408,5 @@ EACCES
     control, or to get a control from a request that has not yet been
     completed.
 
-EPERM
-    The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
+    Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
     device does not support requests.
diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
index 7bff69c15452..a2f4ac0b0ba1 100644
--- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
+++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
@@ -104,7 +104,7 @@ in use. Setting it means that the buffer will not be passed to the driver
 until the request itself is queued. Also, the driver will apply any
 settings associated with the request for this buffer. This field will
 be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set.
-If the device does not support requests, then ``EPERM`` will be returned.
+If the device does not support requests, then ``EACCES`` will be returned.
 If requests are supported but an invalid request file descriptor is given,
 then ``EINVAL`` will be returned.
 
@@ -175,9 +175,12 @@ EPIPE
     codecs if a buffer with the ``V4L2_BUF_FLAG_LAST`` was already
     dequeued and no new buffers are expected to become available.
 
-EPERM
+EACCES
     The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not
-    support requests. Or the first buffer was queued via a request, but
-    the application now tries to queue it directly, or vice versa (it is
-    not permitted to mix the two APIs). Or an attempt is made to queue a
-    CAPTURE buffer to a request for a :ref:`memory-to-memory device <codec>`.
+    support requests.
+
+EPERM
+    The first buffer was queued via a request, but the application now tries
+    to queue it directly, or vice versa (it is not permitted to mix the two
+    APIs). Or an attempt is made to queue a CAPTURE buffer to a request for a
+    :ref:`memory-to-memory device <codec>`.
diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
index 414197645e09..4e9db1fed697 100644
--- a/drivers/media/media-request.c
+++ b/drivers/media/media-request.c
@@ -249,7 +249,7 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd)
 
 	if (!mdev || !mdev->ops ||
 	    !mdev->ops->req_validate || !mdev->ops->req_queue)
-		return ERR_PTR(-EPERM);
+		return ERR_PTR(-EACCES);
 
 	filp = fget(request_fd);
 	if (!filp)
@@ -405,7 +405,7 @@ int media_request_object_bind(struct media_request *req,
 	int ret = -EBUSY;
 
 	if (WARN_ON(!ops->release))
-		return -EPERM;
+		return -EACCES;
 
 	spin_lock_irqsave(&req->lock, flags);
 
-- 
2.18.0

[PATCHv2 10/10] media-request: update documentation

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Various clarifications and readability improvements based on
Laurent Pinchart's review of the documentation.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 .../uapi/mediactl/media-ioc-request-alloc.rst |  3 +-
 .../uapi/mediactl/media-request-ioc-queue.rst |  7 +--
 .../media/uapi/mediactl/request-api.rst       | 51 +++++++++++--------
 .../uapi/mediactl/request-func-close.rst      |  1 +
 .../media/uapi/mediactl/request-func-poll.rst |  2 +-
 Documentation/media/uapi/v4l/buffer.rst       | 14 +++--
 .../media/uapi/v4l/vidioc-g-ext-ctrls.rst     |  5 +-
 Documentation/media/uapi/v4l/vidioc-qbuf.rst  |  5 +-
 8 files changed, 52 insertions(+), 36 deletions(-)

diff --git a/Documentation/media/uapi/mediactl/media-ioc-request-alloc.rst b/Documentation/media/uapi/mediactl/media-ioc-request-alloc.rst
index 34434e2b3918..0f8b31874002 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-request-alloc.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-request-alloc.rst
@@ -52,7 +52,8 @@ for the request to complete.
 
 The request will remain allocated until all the file descriptors associated
 with it are closed by :ref:`close() <request-func-close>` and the driver no
-longer uses the request internally.
+longer uses the request internally. See also
+:ref:`here <media-request-life-time>` for more information.
 
 Return Value
 ============
diff --git a/Documentation/media/uapi/mediactl/media-request-ioc-queue.rst b/Documentation/media/uapi/mediactl/media-request-ioc-queue.rst
index d4f8119e0643..3bf1c2e492eb 100644
--- a/Documentation/media/uapi/mediactl/media-request-ioc-queue.rst
+++ b/Documentation/media/uapi/mediactl/media-request-ioc-queue.rst
@@ -40,9 +40,6 @@ Other errors can be returned if the contents of the request contained
 invalid or inconsistent data, see the next section for a list of
 common error codes. On error both the request and driver state are unchanged.
 
-Typically if you get an error here, then that means that the application
-did something wrong and you have to fix the application.
-
 Once a request is queued, then the driver is required to gracefully handle
 errors that occur when the request is applied to the hardware. The
 exception is the ``EIO`` error which signals a fatal error that requires
@@ -69,8 +66,8 @@ EPERM
     to use a request. It is not permitted to mix the two APIs.
 ENOENT
     The request did not contain any buffers. All requests are required
-    to have at least one buffer. This can also be returned if required
-    controls are missing.
+    to have at least one buffer. This can also be returned if some required
+    configuration is missing in the request.
 ENOMEM
     Out of memory when allocating internal data structures for this
     request.
diff --git a/Documentation/media/uapi/mediactl/request-api.rst b/Documentation/media/uapi/mediactl/request-api.rst
index 0b9da58b01e3..1ac42749e564 100644
--- a/Documentation/media/uapi/mediactl/request-api.rst
+++ b/Documentation/media/uapi/mediactl/request-api.rst
@@ -12,6 +12,9 @@ the same pipeline to reconfigure and collaborate closely on a per-frame basis.
 Another is support of stateless codecs, which require controls to be applied
 to specific frames (aka 'per-frame controls') in order to be used efficiently.
 
+While the initial use-case was V4L2, it can be extended to other subsystems
+as well, as long as they use the media controller.
+
 Supporting these features without the Request API is not always possible and if
 it is, it is terribly inefficient: user-space would have to flush all activity
 on the media pipeline, reconfigure it for the next frame, queue the buffers to
@@ -20,19 +23,23 @@ dequeuing before considering the next frame. This defeats the purpose of having
 buffer queues since in practice only one buffer would be queued at a time.
 
 The Request API allows a specific configuration of the pipeline (media
-controller topology + controls for each media entity) to be associated with
-specific buffers. The parameters are applied by each participating device as
-buffers associated to a request flow in. This allows user-space to schedule
-several tasks ("requests") with different parameters in advance, knowing that
-the parameters will be applied when needed to get the expected result. Control
-values at the time of request completion are also available for reading.
+controller topology + configuration for each media entity) to be associated with
+specific buffers. This allows user-space to schedule several tasks ("requests")
+with different configurations in advance, knowing that the configuration will be
+applied when needed to get the expected result. Configuration values at the time
+of request completion are also available for reading.
 
 Usage
 =====
 
-The Request API is used on top of standard media controller and V4L2 calls,
-which are augmented with an extra ``request_fd`` parameter. Requests themselves
-are allocated from the supporting media controller node.
+The Request API extends the Media Controller API and cooperates with
+subsystem-specific APIs to support request usage. At the Media Controller
+level, requests are allocated from the supporting Media Controller device
+node. Their life cycle is then managed through the request file descriptors in
+an opaque way. Configuration data, buffer handles and processing results
+stored in requests are accessed through subsystem-specific APIs extended for
+request support, such as V4L2 APIs that take an explicit ``request_fd``
+parameter.
 
 Request Allocation
 ------------------
@@ -47,29 +54,27 @@ Request Preparation
 Standard V4L2 ioctls can then receive a request file descriptor to express the
 fact that the ioctl is part of said request, and is not to be applied
 immediately. See :ref:`MEDIA_IOC_REQUEST_ALLOC` for a list of ioctls that
-support this. Controls set with a ``request_fd`` parameter are stored instead
-of being immediately applied, and buffers queued to a request do not enter the
-regular buffer queue until the request itself is queued.
+support this. Configurations set with a ``request_fd`` parameter are stored
+instead of being immediately applied, and buffers queued to a request do not
+enter the regular buffer queue until the request itself is queued.
 
 Request Submission
 ------------------
 
-Once the parameters and buffers of the request are specified, it can be
+Once the configuration and buffers of the request are specified, it can be
 queued by calling :ref:`MEDIA_REQUEST_IOC_QUEUE` on the request file descriptor.
 A request must contain at least one buffer, otherwise ``ENOENT`` is returned.
-This will make the buffers associated to the request available to their driver,
-which can then apply the associated controls as buffers are processed. A queued
-request cannot be modified anymore.
+A queued request cannot be modified anymore.
 
 .. caution::
    For :ref:`memory-to-memory devices <codec>` you can use requests only for
    output buffers, not for capture buffers. Attempting to add a capture buffer
    to a request will result in an ``EPERM`` error.
 
-If the request contains parameters for multiple entities, individual drivers may
-synchronize so the requested pipeline's topology is applied before the buffers
-are processed. Media controller drivers do a best effort implementation since
-perfect atomicity may not be possible due to hardware limitations.
+If the request contains configurations for multiple entities, individual drivers
+may synchronize so the requested pipeline's topology is applied before the
+buffers are processed. Media controller drivers do a best effort implementation
+since perfect atomicity may not be possible due to hardware limitations.
 
 .. caution::
 
@@ -96,14 +101,16 @@ Note that user-space does not need to wait for the request to complete to
 dequeue its buffers: buffers that are available halfway through a request can
 be dequeued independently of the request's state.
 
-A completed request contains the state of the request at the time of the
-request completion. User-space can query that state by calling
+A completed request contains the state of the device after the request was
+executed. User-space can query that state by calling
 :ref:`ioctl VIDIOC_G_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>` with the request file
 descriptor. Calling :ref:`ioctl VIDIOC_G_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>` for a
 request that has been queued but not yet completed will return ``EBUSY``
 since the control values might be changed at any time by the driver while the
 request is in flight.
 
+.. _media-request-life-time:
+
 Recycling and Destruction
 -------------------------
 
diff --git a/Documentation/media/uapi/mediactl/request-func-close.rst b/Documentation/media/uapi/mediactl/request-func-close.rst
index b5c78683840b..098d7f2b9548 100644
--- a/Documentation/media/uapi/mediactl/request-func-close.rst
+++ b/Documentation/media/uapi/mediactl/request-func-close.rst
@@ -36,6 +36,7 @@ Description
 Closes the request file descriptor. Resources associated with the request
 are freed once all file descriptors associated with the request are closed
 and the driver has completed the request.
+See :ref:`here <media-request-life-time>` for more information.
 
 
 Return Value
diff --git a/Documentation/media/uapi/mediactl/request-func-poll.rst b/Documentation/media/uapi/mediactl/request-func-poll.rst
index 70cc9d406a9f..85191254f381 100644
--- a/Documentation/media/uapi/mediactl/request-func-poll.rst
+++ b/Documentation/media/uapi/mediactl/request-func-poll.rst
@@ -50,7 +50,7 @@ when the request was completed.  When the function times out it returns
 a value of zero, on failure it returns -1 and the ``errno`` variable is
 set appropriately.
 
-Attempting to poll for a request that is completed or not yet queued will
+Attempting to poll for a request that is not yet queued will
 set the ``POLLERR`` flag in ``revents``.
 
 
diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index 58a6d7d336e6..2e266d32470a 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -308,12 +308,18 @@ struct v4l2_buffer
     * - __u32
       - ``request_fd``
       -
-      - The file descriptor of the request to queue the buffer to. If specified
-        and flag ``V4L2_BUF_FLAG_REQUEST_FD`` is set, then the buffer will be
-	queued to that request. This is set by the user when calling
-	:ref:`ioctl VIDIOC_QBUF <VIDIOC_QBUF>` and ignored by other ioctls.
+      - The file descriptor of the request to queue the buffer to. If the flag
+        ``V4L2_BUF_FLAG_REQUEST_FD`` is set, then the buffer will be
+	queued to this request. If the flag is not set, then this field will
+	be ignored.
+
+	The ``V4L2_BUF_FLAG_REQUEST_FD`` flag and this field are only used by
+	:ref:`ioctl VIDIOC_QBUF <VIDIOC_QBUF>` and ignored by other ioctls that
+	take a :c:type:`v4l2_buffer` as argument.
+
 	Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls
 	other than :ref:`VIDIOC_QBUF <VIDIOC_QBUF>`.
+
 	If the device does not support requests, then ``EACCES`` will be returned.
 	If requests are supported but an invalid request file descriptor is
 	given, then ``EINVAL`` will be returned.
diff --git a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
index 54a999df5aec..d9930fe776cf 100644
--- a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
+++ b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
@@ -237,7 +237,7 @@ still cause this situation.
 
 	.. note::
 
-	   When using ``V4L2_CTRL_WHICH_DEF_VAL`` note that You can only
+	   When using ``V4L2_CTRL_WHICH_DEF_VAL`` be aware that you can only
 	   get the default value of the control, you cannot set or try it.
 
 	For backwards compatibility you can also use a control class here
@@ -382,7 +382,8 @@ EINVAL
     :c:type:`v4l2_ext_control` ``value`` was
     inappropriate (e.g. the given menu index is not supported by the
     driver), or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL``
-    but the given ``request_fd`` was invalid.
+    but the given ``request_fd`` was invalid or ``V4L2_CTRL_WHICH_REQUEST_VAL``
+    is not supported by the kernel.
     This error code is also returned by the
     :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>` and :ref:`VIDIOC_TRY_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>` ioctls if two or
     more control values are in conflict.
diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
index a2f4ac0b0ba1..b4d5e62aa0c7 100644
--- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
+++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
@@ -111,7 +111,10 @@ then ``EINVAL`` will be returned.
 .. caution::
    It is not allowed to mix queuing requests with queuing buffers directly.
    ``EPERM`` will be returned if the first buffer was queued directly and
-   then the application tries to queue a request, or vice versa.
+   then the application tries to queue a request, or vice versa. After
+   closing the file descriptor, calling
+   :ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>` or calling :ref:`VIDIOC_REQBUFS`
+   the check for this will be reset.
 
    For :ref:`memory-to-memory devices <codec>` you can specify the
    ``request_fd`` only for output buffers, not for capture buffers. Attempting
-- 
2.18.0

Re: [PATCHv2 06/10] media-request: add media_request_(un)lock_for_access

[Hans Verkuil]

On 28/08/18 15:49, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Add helper functions to prevent a completed request from being
> re-inited while it is being accessed.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/media-request.c | 10 ++++++++
>  include/media/media-request.h | 46 +++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> index 4cee67e6657e..414197645e09 100644
> --- a/drivers/media/media-request.c
> +++ b/drivers/media/media-request.c
> @@ -43,6 +43,7 @@ static void media_request_clean(struct media_request *req)
>  	/* Just a sanity check. No other code path is allowed to change this. */
>  	WARN_ON(req->state != MEDIA_REQUEST_STATE_CLEANING);
>  	WARN_ON(req->updating_count);
> +	WARN_ON(req->access_count);
>  
>  	list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
>  		media_request_object_unbind(obj);
> @@ -50,6 +51,7 @@ static void media_request_clean(struct media_request *req)
>  	}
>  
>  	req->updating_count = 0;
> +	req->access_count = 0;
>  	WARN_ON(req->num_incomplete_objects);
>  	req->num_incomplete_objects = 0;
>  	wake_up_interruptible_all(&req->poll_wait);
> @@ -198,6 +200,13 @@ static long media_request_ioctl_reinit(struct media_request *req)
>  		spin_unlock_irqrestore(&req->lock, flags);
>  		return -EBUSY;
>  	}
> +	if (req->access_count) {
> +		dev_dbg(mdev->dev,
> +			"request: %s is being accessed, cannot reinit\n",
> +			req->debug_str);
> +		spin_unlock_irqrestore(&req->lock, flags);
> +		return -EBUSY;
> +	}
>  	req->state = MEDIA_REQUEST_STATE_CLEANING;
>  	spin_unlock_irqrestore(&req->lock, flags);
>  
> @@ -313,6 +322,7 @@ int media_request_alloc(struct media_device *mdev, int *alloc_fd)
>  	spin_lock_init(&req->lock);
>  	init_waitqueue_head(&req->poll_wait);
>  	req->updating_count = 0;
> +	req->access_count = 0;
>  
>  	*alloc_fd = fd;
>  
> diff --git a/include/media/media-request.h b/include/media/media-request.h
> index ac02019c1d77..707c7577f46d 100644
> --- a/include/media/media-request.h
> +++ b/include/media/media-request.h
> @@ -53,6 +53,7 @@ struct media_request_object;
>   * @debug_str: Prefix for debug messages (process name:fd)
>   * @state: The state of the request
>   * @updating_count: count the number of request updates that are in progress
> + * @access_count: count the number of request accesses that are in progress
>   * @objects: List of @struct media_request_object request objects
>   * @num_incomplete_objects: The number of incomplete objects in the request
>   * @poll_wait: Wait queue for poll
> @@ -64,6 +65,7 @@ struct media_request {
>  	char debug_str[TASK_COMM_LEN + 11];
>  	enum media_request_state state;
>  	unsigned int updating_count;
> +	unsigned int access_count;
>  	struct list_head objects;
>  	unsigned int num_incomplete_objects;
>  	struct wait_queue_head poll_wait;
> @@ -72,6 +74,50 @@ struct media_request {
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  
> +/**
> + * media_request_lock_for_access - Lock the request to access its objects
> + *
> + * @req: The media request
> + *
> + * Use before accessing a completed request. A reference to the request must
> + * be held during the access. This usually takes place automatically through
> + * a file handle. Use @media_request_unlock_for_access when done.
> + */
> +static inline int __must_check
> +media_request_lock_for_access(struct media_request *req)
> +{
> +	unsigned long flags;
> +	int ret = -EBUSY;
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	if (req->state == MEDIA_REQUEST_STATE_COMPLETE) {
> +		req->access_count++;
> +		ret = 0;
> +	}
> +	spin_unlock_irqrestore(&req->lock, flags);
> +
> +	return ret;
> +}
> +
> +/**
> + * media_request_unlock_for_access - Unlock a request previously locked for
> + *				     access
> + *
> + * @req: The media request
> + *
> + * Unlock a request that has previously been locked using
> + * @media_request_lock_for_access.
> + */
> +static inline void media_request_unlock_for_access(struct media_request *req)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	if (!WARN_ON(!req->access_count))
> +		req->access_count--;
> +	spin_unlock_irqrestore(&req->lock, flags);
> +}
> +
>  /**
>   * media_request_lock_for_update - Lock the request for updating its objects
>   *
> 

I also need to add *_for_access() stub functions that are used when the MEDIA_CONTROLLER
is not set in the kernel config.

I've fixed this in my tree.

Regards,

	Hans

Re: [PATCHv2 09/10] media-request: EPERM -> EACCES

[Sakari Ailus]

Hi Hans,

Thanks a lot for working on this!

On Tue, Aug 28, 2018 at 03:49:10PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> If requests are not supported by the driver, then return EACCES, not
> EPERM. This is consistent with the error that an invalid request_fd will
> give, and if requests are not supported, then all request_fd values are
> invalid.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  Documentation/media/uapi/v4l/buffer.rst           |  2 +-
>  .../media/uapi/v4l/vidioc-g-ext-ctrls.rst         |  9 ++++-----
>  Documentation/media/uapi/v4l/vidioc-qbuf.rst      | 15 +++++++++------
>  drivers/media/media-request.c                     |  4 ++--
>  4 files changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index 1865cd5b9d3c..58a6d7d336e6 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -314,7 +314,7 @@ struct v4l2_buffer
>  	:ref:`ioctl VIDIOC_QBUF <VIDIOC_QBUF>` and ignored by other ioctls.
>  	Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls
>  	other than :ref:`VIDIOC_QBUF <VIDIOC_QBUF>`.
> -	If the device does not support requests, then ``EPERM`` will be returned.
> +	If the device does not support requests, then ``EACCES`` will be returned.
>  	If requests are supported but an invalid request file descriptor is
>  	given, then ``EINVAL`` will be returned.
>  
> diff --git a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
> index ad8908ce3095..54a999df5aec 100644
> --- a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
> @@ -100,7 +100,7 @@ file descriptor and ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``,
>  then the controls are not applied immediately when calling
>  :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>`, but instead are applied by
>  the driver for the buffer associated with the same request.
> -If the device does not support requests, then ``EPERM`` will be returned.
> +If the device does not support requests, then ``EACCES`` will be returned.
>  If requests are supported but an invalid request file descriptor is given,
>  then ``EINVAL`` will be returned.
>  
> @@ -233,7 +233,7 @@ still cause this situation.
>  	these controls have to be retrieved from a request or tried/set for
>  	a request. In the latter case the ``request_fd`` field contains the
>  	file descriptor of the request that should be used. If the device
> -	does not support requests, then ``EPERM`` will be returned.
> +	does not support requests, then ``EACCES`` will be returned.
>  
>  	.. note::
>  
> @@ -299,7 +299,7 @@ still cause this situation.
>        - ``request_fd``
>        - File descriptor of the request to be used by this operation. Only
>  	valid if ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``.
> -	If the device does not support requests, then ``EPERM`` will be returned.
> +	If the device does not support requests, then ``EACCES`` will be returned.
>  	If requests are supported but an invalid request file descriptor is
>  	given, then ``EINVAL`` will be returned.
>      * - __u32
> @@ -408,6 +408,5 @@ EACCES
>      control, or to get a control from a request that has not yet been
>      completed.
>  
> -EPERM

-EACCES here, too?

> -    The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
> +    Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
>      device does not support requests.
> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> index 7bff69c15452..a2f4ac0b0ba1 100644
> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> @@ -104,7 +104,7 @@ in use. Setting it means that the buffer will not be passed to the driver
>  until the request itself is queued. Also, the driver will apply any
>  settings associated with the request for this buffer. This field will
>  be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set.
> -If the device does not support requests, then ``EPERM`` will be returned.
> +If the device does not support requests, then ``EACCES`` will be returned.
>  If requests are supported but an invalid request file descriptor is given,
>  then ``EINVAL`` will be returned.
>  
> @@ -175,9 +175,12 @@ EPIPE
>      codecs if a buffer with the ``V4L2_BUF_FLAG_LAST`` was already
>      dequeued and no new buffers are expected to become available.
>  
> -EPERM
> +EACCES
>      The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not
> -    support requests. Or the first buffer was queued via a request, but
> -    the application now tries to queue it directly, or vice versa (it is
> -    not permitted to mix the two APIs). Or an attempt is made to queue a
> -    CAPTURE buffer to a request for a :ref:`memory-to-memory device <codec>`.
> +    support requests.
> +
> +EPERM
> +    The first buffer was queued via a request, but the application now tries
> +    to queue it directly, or vice versa (it is not permitted to mix the two
> +    APIs). Or an attempt is made to queue a CAPTURE buffer to a request for a
> +    :ref:`memory-to-memory device <codec>`.

This is still apparently not quite the error code it should be --- EPERM is
about lacking permissions, not that the user did something that isn't
possible. We should not use an error code that has a well established
meaning everywhere else in uAPI already for a purpose that is very
different.

If you think this needs to be something else than EACCES (which I think is
perfectly fine), then how about EDOM or EBUSY?

> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> index 414197645e09..4e9db1fed697 100644
> --- a/drivers/media/media-request.c
> +++ b/drivers/media/media-request.c
> @@ -249,7 +249,7 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd)
>  
>  	if (!mdev || !mdev->ops ||
>  	    !mdev->ops->req_validate || !mdev->ops->req_queue)
> -		return ERR_PTR(-EPERM);
> +		return ERR_PTR(-EACCES);
>  
>  	filp = fget(request_fd);
>  	if (!filp)
> @@ -405,7 +405,7 @@ int media_request_object_bind(struct media_request *req,
>  	int ret = -EBUSY;
>  
>  	if (WARN_ON(!ops->release))
> -		return -EPERM;
> +		return -EACCES;
>  
>  	spin_lock_irqsave(&req->lock, flags);
>  

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 09/10] media-request: EPERM -> EACCES

[Hans Verkuil]

On 08/30/2018 12:15 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks a lot for working on this!
> 
> On Tue, Aug 28, 2018 at 03:49:10PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> If requests are not supported by the driver, then return EACCES, not
>> EPERM. This is consistent with the error that an invalid request_fd will
>> give, and if requests are not supported, then all request_fd values are
>> invalid.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  Documentation/media/uapi/v4l/buffer.rst           |  2 +-
>>  .../media/uapi/v4l/vidioc-g-ext-ctrls.rst         |  9 ++++-----
>>  Documentation/media/uapi/v4l/vidioc-qbuf.rst      | 15 +++++++++------
>>  drivers/media/media-request.c                     |  4 ++--
>>  4 files changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
>> index 1865cd5b9d3c..58a6d7d336e6 100644
>> --- a/Documentation/media/uapi/v4l/buffer.rst
>> +++ b/Documentation/media/uapi/v4l/buffer.rst
>> @@ -314,7 +314,7 @@ struct v4l2_buffer
>>  	:ref:`ioctl VIDIOC_QBUF <VIDIOC_QBUF>` and ignored by other ioctls.
>>  	Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls
>>  	other than :ref:`VIDIOC_QBUF <VIDIOC_QBUF>`.
>> -	If the device does not support requests, then ``EPERM`` will be returned.
>> +	If the device does not support requests, then ``EACCES`` will be returned.
>>  	If requests are supported but an invalid request file descriptor is
>>  	given, then ``EINVAL`` will be returned.
>>  
>> diff --git a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
>> index ad8908ce3095..54a999df5aec 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
>> @@ -100,7 +100,7 @@ file descriptor and ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``,
>>  then the controls are not applied immediately when calling
>>  :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>`, but instead are applied by
>>  the driver for the buffer associated with the same request.
>> -If the device does not support requests, then ``EPERM`` will be returned.
>> +If the device does not support requests, then ``EACCES`` will be returned.
>>  If requests are supported but an invalid request file descriptor is given,
>>  then ``EINVAL`` will be returned.
>>  
>> @@ -233,7 +233,7 @@ still cause this situation.
>>  	these controls have to be retrieved from a request or tried/set for
>>  	a request. In the latter case the ``request_fd`` field contains the
>>  	file descriptor of the request that should be used. If the device
>> -	does not support requests, then ``EPERM`` will be returned.
>> +	does not support requests, then ``EACCES`` will be returned.
>>  
>>  	.. note::
>>  
>> @@ -299,7 +299,7 @@ still cause this situation.
>>        - ``request_fd``
>>        - File descriptor of the request to be used by this operation. Only
>>  	valid if ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``.
>> -	If the device does not support requests, then ``EPERM`` will be returned.
>> +	If the device does not support requests, then ``EACCES`` will be returned.
>>  	If requests are supported but an invalid request file descriptor is
>>  	given, then ``EINVAL`` will be returned.
>>      * - __u32
>> @@ -408,6 +408,5 @@ EACCES
>>      control, or to get a control from a request that has not yet been
>>      completed.
>>  
>> -EPERM
> 
> -EACCES here, too?

The '-' in -EPERM is from diff, meaning: delete this line :-)

> 
>> -    The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
>> +    Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
>>      device does not support requests.
>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> index 7bff69c15452..a2f4ac0b0ba1 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> @@ -104,7 +104,7 @@ in use. Setting it means that the buffer will not be passed to the driver
>>  until the request itself is queued. Also, the driver will apply any
>>  settings associated with the request for this buffer. This field will
>>  be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set.
>> -If the device does not support requests, then ``EPERM`` will be returned.
>> +If the device does not support requests, then ``EACCES`` will be returned.
>>  If requests are supported but an invalid request file descriptor is given,
>>  then ``EINVAL`` will be returned.
>>  
>> @@ -175,9 +175,12 @@ EPIPE
>>      codecs if a buffer with the ``V4L2_BUF_FLAG_LAST`` was already
>>      dequeued and no new buffers are expected to become available.
>>  
>> -EPERM
>> +EACCES
>>      The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not
>> -    support requests. Or the first buffer was queued via a request, but
>> -    the application now tries to queue it directly, or vice versa (it is
>> -    not permitted to mix the two APIs). Or an attempt is made to queue a
>> -    CAPTURE buffer to a request for a :ref:`memory-to-memory device <codec>`.
>> +    support requests.
>> +
>> +EPERM
>> +    The first buffer was queued via a request, but the application now tries
>> +    to queue it directly, or vice versa (it is not permitted to mix the two
>> +    APIs). Or an attempt is made to queue a CAPTURE buffer to a request for a
>> +    :ref:`memory-to-memory device <codec>`.
> 
> This is still apparently not quite the error code it should be --- EPERM is
> about lacking permissions, not that the user did something that isn't
> possible. We should not use an error code that has a well established
> meaning everywhere else in uAPI already for a purpose that is very
> different.
> 
> If you think this needs to be something else than EACCES (which I think is
> perfectly fine), then how about EDOM or EBUSY?

Hmm. EPERM is returned for two reasons:

- attempting to queue a buffer when you need to use requests, or vice versa.
  EBUSY would be a much better error code since the device is busy streaming
  in a different mode than you requested. And after you stop streaming you
  can use the requested mode again.

- attempting to queue a request for a vb2 queue that doesn't support requests.
  This should return -EACCES.

What do you think?

Regards,

	Hans

> 
>> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
>> index 414197645e09..4e9db1fed697 100644
>> --- a/drivers/media/media-request.c
>> +++ b/drivers/media/media-request.c
>> @@ -249,7 +249,7 @@ media_request_get_by_fd(struct media_device *mdev, int request_fd)
>>  
>>  	if (!mdev || !mdev->ops ||
>>  	    !mdev->ops->req_validate || !mdev->ops->req_queue)
>> -		return ERR_PTR(-EPERM);
>> +		return ERR_PTR(-EACCES);
>>  
>>  	filp = fget(request_fd);
>>  	if (!filp)
>> @@ -405,7 +405,7 @@ int media_request_object_bind(struct media_request *req,
>>  	int ret = -EBUSY;
>>  
>>  	if (WARN_ON(!ops->release))
>> -		return -EPERM;
>> +		return -EACCES;
>>  
>>  	spin_lock_irqsave(&req->lock, flags);
>>  
> 

Re: [PATCHv2 09/10] media-request: EPERM -> EACCES

[Sakari Ailus]

On Thu, Aug 30, 2018 at 01:51:39PM +0200, Hans Verkuil wrote:
> On 08/30/2018 12:15 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > Thanks a lot for working on this!
> > 
> > On Tue, Aug 28, 2018 at 03:49:10PM +0200, Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>
> >> If requests are not supported by the driver, then return EACCES, not
> >> EPERM. This is consistent with the error that an invalid request_fd will
> >> give, and if requests are not supported, then all request_fd values are
> >> invalid.
> >>
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >>  Documentation/media/uapi/v4l/buffer.rst           |  2 +-
> >>  .../media/uapi/v4l/vidioc-g-ext-ctrls.rst         |  9 ++++-----
> >>  Documentation/media/uapi/v4l/vidioc-qbuf.rst      | 15 +++++++++------
> >>  drivers/media/media-request.c                     |  4 ++--
> >>  4 files changed, 16 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> >> index 1865cd5b9d3c..58a6d7d336e6 100644
> >> --- a/Documentation/media/uapi/v4l/buffer.rst
> >> +++ b/Documentation/media/uapi/v4l/buffer.rst
> >> @@ -314,7 +314,7 @@ struct v4l2_buffer
> >>  	:ref:`ioctl VIDIOC_QBUF <VIDIOC_QBUF>` and ignored by other ioctls.
> >>  	Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls
> >>  	other than :ref:`VIDIOC_QBUF <VIDIOC_QBUF>`.
> >> -	If the device does not support requests, then ``EPERM`` will be returned.
> >> +	If the device does not support requests, then ``EACCES`` will be returned.
> >>  	If requests are supported but an invalid request file descriptor is
> >>  	given, then ``EINVAL`` will be returned.
> >>  
> >> diff --git a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
> >> index ad8908ce3095..54a999df5aec 100644
> >> --- a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
> >> +++ b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
> >> @@ -100,7 +100,7 @@ file descriptor and ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``,
> >>  then the controls are not applied immediately when calling
> >>  :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>`, but instead are applied by
> >>  the driver for the buffer associated with the same request.
> >> -If the device does not support requests, then ``EPERM`` will be returned.
> >> +If the device does not support requests, then ``EACCES`` will be returned.
> >>  If requests are supported but an invalid request file descriptor is given,
> >>  then ``EINVAL`` will be returned.
> >>  
> >> @@ -233,7 +233,7 @@ still cause this situation.
> >>  	these controls have to be retrieved from a request or tried/set for
> >>  	a request. In the latter case the ``request_fd`` field contains the
> >>  	file descriptor of the request that should be used. If the device
> >> -	does not support requests, then ``EPERM`` will be returned.
> >> +	does not support requests, then ``EACCES`` will be returned.
> >>  
> >>  	.. note::
> >>  
> >> @@ -299,7 +299,7 @@ still cause this situation.
> >>        - ``request_fd``
> >>        - File descriptor of the request to be used by this operation. Only
> >>  	valid if ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``.
> >> -	If the device does not support requests, then ``EPERM`` will be returned.
> >> +	If the device does not support requests, then ``EACCES`` will be returned.
> >>  	If requests are supported but an invalid request file descriptor is
> >>  	given, then ``EINVAL`` will be returned.
> >>      * - __u32
> >> @@ -408,6 +408,5 @@ EACCES
> >>      control, or to get a control from a request that has not yet been
> >>      completed.
> >>  
> >> -EPERM
> > 
> > -EACCES here, too?
> 
> The '-' in -EPERM is from diff, meaning: delete this line :-)

Oh, I missed that while reading the quoted message. X-)

> 
> > 
> >> -    The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
> >> +    Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
> >>      device does not support requests.
> >> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >> index 7bff69c15452..a2f4ac0b0ba1 100644
> >> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >> @@ -104,7 +104,7 @@ in use. Setting it means that the buffer will not be passed to the driver
> >>  until the request itself is queued. Also, the driver will apply any
> >>  settings associated with the request for this buffer. This field will
> >>  be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set.
> >> -If the device does not support requests, then ``EPERM`` will be returned.
> >> +If the device does not support requests, then ``EACCES`` will be returned.
> >>  If requests are supported but an invalid request file descriptor is given,
> >>  then ``EINVAL`` will be returned.
> >>  
> >> @@ -175,9 +175,12 @@ EPIPE
> >>      codecs if a buffer with the ``V4L2_BUF_FLAG_LAST`` was already
> >>      dequeued and no new buffers are expected to become available.
> >>  
> >> -EPERM
> >> +EACCES
> >>      The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not
> >> -    support requests. Or the first buffer was queued via a request, but
> >> -    the application now tries to queue it directly, or vice versa (it is
> >> -    not permitted to mix the two APIs). Or an attempt is made to queue a
> >> -    CAPTURE buffer to a request for a :ref:`memory-to-memory device <codec>`.
> >> +    support requests.
> >> +
> >> +EPERM
> >> +    The first buffer was queued via a request, but the application now tries
> >> +    to queue it directly, or vice versa (it is not permitted to mix the two
> >> +    APIs). Or an attempt is made to queue a CAPTURE buffer to a request for a
> >> +    :ref:`memory-to-memory device <codec>`.
> > 
> > This is still apparently not quite the error code it should be --- EPERM is
> > about lacking permissions, not that the user did something that isn't
> > possible. We should not use an error code that has a well established
> > meaning everywhere else in uAPI already for a purpose that is very
> > different.
> > 
> > If you think this needs to be something else than EACCES (which I think is
> > perfectly fine), then how about EDOM or EBUSY?
> 
> Hmm. EPERM is returned for two reasons:
> 
> - attempting to queue a buffer when you need to use requests, or vice versa.
>   EBUSY would be a much better error code since the device is busy streaming
>   in a different mode than you requested. And after you stop streaming you
>   can use the requested mode again.
> 
> - attempting to queue a request for a vb2 queue that doesn't support requests.
>   This should return -EACCES.

I think we've actually used EINVAL in most cases the user tries to use
functionality (fine grainer than IOCTL command) which isn't supported such
as buffer or memory types not supported by the driver. EINVAL would be just
a little less specific but better in line with the rest of the API.
> 
> What do you think?

I agree on EBUSY.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCHv2 07/10] v4l2-ctrls: use media_request_(un)lock_for_access

[Tomasz Figa]

Hi Hans,

On Tue, Aug 28, 2018 at 10:49 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> When getting control values from a completed request, we have
> to protect the request against being re-inited why it is
> being accessed by calling media_request_(un)lock_for_access.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index ccaf3068de6d..cc266a4a6e88 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -3289,11 +3289,10 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
>                      struct v4l2_ext_controls *cs)
>  {
>         struct media_request_object *obj = NULL;
> +       struct media_request *req = NULL;
>         int ret;
>
>         if (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL) {
> -               struct media_request *req;
> -
>                 if (!mdev || cs->request_fd < 0)
>                         return -EINVAL;
>
> @@ -3306,11 +3305,18 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
>                         return -EACCES;
>                 }
>
> +               ret = media_request_lock_for_access(req);
> +               if (ret) {
> +                       media_request_put(req);
> +                       return ret;
> +               }
> +
>                 obj = v4l2_ctrls_find_req_obj(hdl, req, false);
> -               /* Reference to the request held through obj */
> -               media_request_put(req);
> -               if (IS_ERR(obj))
> +               if (IS_ERR(obj)) {
> +                       media_request_unlock_for_access(req);
> +                       media_request_put(req);
>                         return PTR_ERR(obj);
> +               }
>
>                 hdl = container_of(obj, struct v4l2_ctrl_handler,
>                                    req_obj);
> @@ -3318,8 +3324,11 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
>
>         ret = v4l2_g_ext_ctrls_common(hdl, cs);
>
> -       if (obj)
> +       if (obj) {
> +               media_request_unlock_for_access(req);

We called media_request_lock_for_access() before looking up obj. Don't
we also need to  call media_request_unlock_for_access() regardless of
whether obj is non-NULL?

>                 media_request_object_put(obj);
> +               media_request_put(req);
> +       }
>         return ret;
>  }

Best regards,
Tomasz

Re: [PATCHv2 07/10] v4l2-ctrls: use media_request_(un)lock_for_access

[Tomasz Figa]

On Fri, Aug 31, 2018 at 11:55 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> Hi Hans,
>
> On Tue, Aug 28, 2018 at 10:49 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> >
> > When getting control values from a completed request, we have
> > to protect the request against being re-inited why it is
> > being accessed by calling media_request_(un)lock_for_access.
> >
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-ctrls.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index ccaf3068de6d..cc266a4a6e88 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -3289,11 +3289,10 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
> >                      struct v4l2_ext_controls *cs)
> >  {
> >         struct media_request_object *obj = NULL;
> > +       struct media_request *req = NULL;
> >         int ret;
> >
> >         if (cs->which == V4L2_CTRL_WHICH_REQUEST_VAL) {
> > -               struct media_request *req;
> > -
> >                 if (!mdev || cs->request_fd < 0)
> >                         return -EINVAL;
> >
> > @@ -3306,11 +3305,18 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
> >                         return -EACCES;
> >                 }
> >
> > +               ret = media_request_lock_for_access(req);
> > +               if (ret) {
> > +                       media_request_put(req);
> > +                       return ret;
> > +               }
> > +
> >                 obj = v4l2_ctrls_find_req_obj(hdl, req, false);
> > -               /* Reference to the request held through obj */
> > -               media_request_put(req);
> > -               if (IS_ERR(obj))
> > +               if (IS_ERR(obj)) {
> > +                       media_request_unlock_for_access(req);
> > +                       media_request_put(req);
> >                         return PTR_ERR(obj);
> > +               }
> >
> >                 hdl = container_of(obj, struct v4l2_ctrl_handler,
> >                                    req_obj);
> > @@ -3318,8 +3324,11 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
> >
> >         ret = v4l2_g_ext_ctrls_common(hdl, cs);
> >
> > -       if (obj)
> > +       if (obj) {
> > +               media_request_unlock_for_access(req);
>
> We called media_request_lock_for_access() before looking up obj. Don't
> we also need to  call media_request_unlock_for_access() regardless of
> whether obj is non-NULL?

Aha, never mind. I checked the full context and we can't have !obj
here if we operated on a request. Sorry for the noise.

Best regards,
Tomasz

Re: [PATCHv2 09/10] media-request: EPERM -> EACCES

[Tomasz Figa]

On Thu, Aug 30, 2018 at 10:04 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> On Thu, Aug 30, 2018 at 01:51:39PM +0200, Hans Verkuil wrote:
> > On 08/30/2018 12:15 PM, Sakari Ailus wrote:
> > > Hi Hans,
> > >
> > > Thanks a lot for working on this!
> > >
> > > On Tue, Aug 28, 2018 at 03:49:10PM +0200, Hans Verkuil wrote:
> > >> From: Hans Verkuil <hans.verkuil@cisco.com>
> > >>
> > >> If requests are not supported by the driver, then return EACCES, not
> > >> EPERM. This is consistent with the error that an invalid request_fd will
> > >> give, and if requests are not supported, then all request_fd values are
> > >> invalid.
> > >>
> > >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > >> ---
> > >>  Documentation/media/uapi/v4l/buffer.rst           |  2 +-
> > >>  .../media/uapi/v4l/vidioc-g-ext-ctrls.rst         |  9 ++++-----
> > >>  Documentation/media/uapi/v4l/vidioc-qbuf.rst      | 15 +++++++++------
> > >>  drivers/media/media-request.c                     |  4 ++--
> > >>  4 files changed, 16 insertions(+), 14 deletions(-)
> > >>
> > >> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> > >> index 1865cd5b9d3c..58a6d7d336e6 100644
> > >> --- a/Documentation/media/uapi/v4l/buffer.rst
> > >> +++ b/Documentation/media/uapi/v4l/buffer.rst
> > >> @@ -314,7 +314,7 @@ struct v4l2_buffer
> > >>    :ref:`ioctl VIDIOC_QBUF <VIDIOC_QBUF>` and ignored by other ioctls.
> > >>    Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls
> > >>    other than :ref:`VIDIOC_QBUF <VIDIOC_QBUF>`.
> > >> -  If the device does not support requests, then ``EPERM`` will be returned.
> > >> +  If the device does not support requests, then ``EACCES`` will be returned.
> > >>    If requests are supported but an invalid request file descriptor is
> > >>    given, then ``EINVAL`` will be returned.
> > >>
> > >> diff --git a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
> > >> index ad8908ce3095..54a999df5aec 100644
> > >> --- a/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
> > >> +++ b/Documentation/media/uapi/v4l/vidioc-g-ext-ctrls.rst
> > >> @@ -100,7 +100,7 @@ file descriptor and ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``,
> > >>  then the controls are not applied immediately when calling
> > >>  :ref:`VIDIOC_S_EXT_CTRLS <VIDIOC_G_EXT_CTRLS>`, but instead are applied by
> > >>  the driver for the buffer associated with the same request.
> > >> -If the device does not support requests, then ``EPERM`` will be returned.
> > >> +If the device does not support requests, then ``EACCES`` will be returned.
> > >>  If requests are supported but an invalid request file descriptor is given,
> > >>  then ``EINVAL`` will be returned.
> > >>
> > >> @@ -233,7 +233,7 @@ still cause this situation.
> > >>    these controls have to be retrieved from a request or tried/set for
> > >>    a request. In the latter case the ``request_fd`` field contains the
> > >>    file descriptor of the request that should be used. If the device
> > >> -  does not support requests, then ``EPERM`` will be returned.
> > >> +  does not support requests, then ``EACCES`` will be returned.
> > >>
> > >>    .. note::
> > >>
> > >> @@ -299,7 +299,7 @@ still cause this situation.
> > >>        - ``request_fd``
> > >>        - File descriptor of the request to be used by this operation. Only
> > >>    valid if ``which`` is set to ``V4L2_CTRL_WHICH_REQUEST_VAL``.
> > >> -  If the device does not support requests, then ``EPERM`` will be returned.
> > >> +  If the device does not support requests, then ``EACCES`` will be returned.
> > >>    If requests are supported but an invalid request file descriptor is
> > >>    given, then ``EINVAL`` will be returned.
> > >>      * - __u32
> > >> @@ -408,6 +408,5 @@ EACCES
> > >>      control, or to get a control from a request that has not yet been
> > >>      completed.
> > >>
> > >> -EPERM
> > >
> > > -EACCES here, too?
> >
> > The '-' in -EPERM is from diff, meaning: delete this line :-)
>
> Oh, I missed that while reading the quoted message. X-)
>
> >
> > >
> > >> -    The ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
> > >> +    Or the ``which`` field was set to ``V4L2_CTRL_WHICH_REQUEST_VAL`` but the
> > >>      device does not support requests.
> > >> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> > >> index 7bff69c15452..a2f4ac0b0ba1 100644
> > >> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> > >> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> > >> @@ -104,7 +104,7 @@ in use. Setting it means that the buffer will not be passed to the driver
> > >>  until the request itself is queued. Also, the driver will apply any
> > >>  settings associated with the request for this buffer. This field will
> > >>  be ignored unless the ``V4L2_BUF_FLAG_REQUEST_FD`` flag is set.
> > >> -If the device does not support requests, then ``EPERM`` will be returned.
> > >> +If the device does not support requests, then ``EACCES`` will be returned.
> > >>  If requests are supported but an invalid request file descriptor is given,
> > >>  then ``EINVAL`` will be returned.
> > >>
> > >> @@ -175,9 +175,12 @@ EPIPE
> > >>      codecs if a buffer with the ``V4L2_BUF_FLAG_LAST`` was already
> > >>      dequeued and no new buffers are expected to become available.
> > >>
> > >> -EPERM
> > >> +EACCES
> > >>      The ``V4L2_BUF_FLAG_REQUEST_FD`` flag was set but the device does not
> > >> -    support requests. Or the first buffer was queued via a request, but
> > >> -    the application now tries to queue it directly, or vice versa (it is
> > >> -    not permitted to mix the two APIs). Or an attempt is made to queue a
> > >> -    CAPTURE buffer to a request for a :ref:`memory-to-memory device <codec>`.
> > >> +    support requests.
> > >> +
> > >> +EPERM
> > >> +    The first buffer was queued via a request, but the application now tries
> > >> +    to queue it directly, or vice versa (it is not permitted to mix the two
> > >> +    APIs). Or an attempt is made to queue a CAPTURE buffer to a request for a
> > >> +    :ref:`memory-to-memory device <codec>`.
> > >
> > > This is still apparently not quite the error code it should be --- EPERM is
> > > about lacking permissions, not that the user did something that isn't
> > > possible. We should not use an error code that has a well established
> > > meaning everywhere else in uAPI already for a purpose that is very
> > > different.
> > >
> > > If you think this needs to be something else than EACCES (which I think is
> > > perfectly fine), then how about EDOM or EBUSY?
> >
> > Hmm. EPERM is returned for two reasons:
> >
> > - attempting to queue a buffer when you need to use requests, or vice versa.
> >   EBUSY would be a much better error code since the device is busy streaming
> >   in a different mode than you requested. And after you stop streaming you
> >   can use the requested mode again.

EBUSY sounds good here indeed.

> >
> > - attempting to queue a request for a vb2 queue that doesn't support requests.
> >   This should return -EACCES.
>
> I think we've actually used EINVAL in most cases the user tries to use
> functionality (fine grainer than IOCTL command) which isn't supported such
> as buffer or memory types not supported by the driver. EINVAL would be just
> a little less specific but better in line with the rest of the API.

Buffer or memory type is a value inside the ioctl arguments, so EINVAL
matches there.

I tend to side with Hans on EACCES.

Best regards,
Tomasz

Re: [PATCHv2 00/10] Post-v18: Request API updates

[Tomasz Figa]

On Tue, Aug 28, 2018 at 10:49 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> From: Hans Verkuil <hans.verkuil@cisco.com>
>
> Hi all,
>
> This patch series sits on top of my v18 series for the Request API.
> It makes some final (?) changes as discussed in:
>
> https://www.mail-archive.com/linux-media@vger.kernel.org/msg134419.html
>
> and:
>
> https://www.spinics.net/lists/linux-media/msg138596.html
>
> The combined v18 patches + this series is available here:
>
> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv18-1
>
> Updated v4l-utils for this is available here:
>
> https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=request
>
> Userspace visible changes:
>
> - Invalid request_fd values now return -EINVAL instead of -ENOENT.
> - It is no longer possible to use VIDIOC_G_EXT_CTRLS for requests
>   that are not completed. -EACCES is returned in that case.
> - Attempting to use requests if requests are not supported by the driver
>   will result in -EACCES instead of -EPERM.
>
> Driver visible changes (important for the cedrus driver!):
>
> Drivers should set the new vb2_queue 'supports_request' bitfield to 1
> if a vb2_queue can support requests. Otherwise the queue cannot be
> used with requests.
>
> This bitfield is also used to fill in the new capabilities field
> in struct v4l2_requestbuffers and v4l2_create_buffers.
>
> Changes since v1:
>
> - Updated patch 4/10 to explain how to query the capabilities
>   with REQBUFS/CREATE_BUFS with a minimum of side-effects
>   (requested by Tomasz).
> - Added patches 6-10:
>   6: Sakari found a corner case: when accessing a request the
>      request has to be protected from being re-inited. New
>      media_request_(un)lock_for_access helpers are added for this.
>   7: use these helpers in g_ext_ctrls.
>   8: make s/try_ext_ctrls more robust by keeping the request
>      references until we're fully done setting/trying the controls.
>   9: Change two more EPERM's to EACCES. EPERM suggests that you can
>      fix it by changing permissions somehow, but in this case the
>      driver simply doesn't support requests at all.
>   10: Update the request documentation based on Laurent's comments:
>       https://www.spinics.net/lists/linux-media/msg139152.html
>       To do: split off the V4L2 specifics into a V4L2 specific
>       rst file. But this will take more time and is for later.

For all the patches which still don't have my Reviewed-by, except
patch 9/10 ("media-request: EPERM -> EACCES"):

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Thanks a lot for this great work!

Best regards,
Tomasz