Re: [RFC] Request API and V4L2 capabilities
On 08/23/2018 07:37 PM, Nicolas Dufresne wrote: > Le jeudi 23 août 2018 à 16:31 +0200, Hans Verkuil a écrit : >>> I propose adding these capabilities: >>> >>> #define V4L2_BUF_CAP_HAS_REQUESTS 0x0001 >>> #define V4L2_BUF_CAP_REQUIRES_REQUESTS0x0002 >>> #define V4L2_BUF_CAP_HAS_MMAP 0x0100 >>> #define V4L2_BUF_CAP_HAS_USERPTR 0x0200 >>> #define V4L2_BUF_CAP_HAS_DMABUF 0x0400 >> >> I substituted SUPPORTS for HAS and dropped the REQUIRES_REQUESTS capability. >> As Tomasz mentioned, technically (at least for stateless codecs) you could >> handle just one frame at a time without using requests. It's very >> inefficient, >> but it would work. > > I thought the request was providing a data structure to refer back to > the frames, so each codec don't have to implement one. Do you mean that > the framework will implicitly request requests in that mode ? or simply > that there is no such helper ? Yes, that's done through controls as well. The idea would be that you set the necessary controls, queue a buffer to the output queue, dequeue a buffer from the output queue, read back any new state information and repeat the process. That said, I'm not sure if the cedrus driver for example can handle this at the moment. It is also inefficient and it won't work if codecs require more than one buffer in the queue for whatever reason. Tomasz, Paul, please correct me if I am wrong. In any case, I think we can do without this proposed capability since it is simply a requirement when implementing the pixelformat for the stateless codec that the Request API will be available and it should be documented as such in the spec. Regards, Hans
[PATCH 1/5] media-request: return -EINVAL for invalid request_fds
From: Hans Verkuil 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 --- 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 ` 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 `, 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 ` 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 ` and :ref:`VIDIOC_TRY_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 inv
[PATCH 3/5] buffer.rst: only set V4L2_BUF_FLAG_REQUEST_FD for QBUF
From: Hans Verkuil Document that V4L2_BUF_FLAG_REQUEST_FD should only be used with VIDIOC_QBUF and cleared otherwise. Signed-off-by: Hans Verkuil --- 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 ` and ignored by other ioctls. + Applications should not set ``V4L2_BUF_FLAG_REQUEST_FD`` for any ioctls + other than :ref:`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
[PATCH 4/5] videodev2.h: add new capabilities for buffer types
From: Hans Verkuil
VIDIOC_REQBUFS and VIDIOC_CREATE_BUFFERS will return capabilities
telling userspace what the given buffer type is capable of.
Signed-off-by: Hans Verkuil
---
.../media/uapi/v4l/vidioc-create-bufs.rst | 10 +-
.../media/uapi/v4l/vidioc-reqbufs.rst | 36 ++-
include/uapi/linux/videodev2.h| 13 +--
3 files changed, 55 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..fd34d3f236c9 100644
--- a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
+++ b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
@@ -102,7 +102,15 @@ 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 ` for a list of the
+ capabilities.
+* - __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..20a4bd4f2b74 100644
--- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
+++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
@@ -88,10 +88,44 @@ 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.
+* - __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``
+ - 0x0001
+ - This buffer type supports the ``V4L2_MEMORY_MMAP`` streaming mode.
+* - ``V4L2_BUF_CAP_SUPPORTS_USERPTR``
+ - 0x0002
+ - This buffer type supports the ``V4L2_MEMORY_USERPTR`` streaming mode.
+* - ``V4L2_BUF_CAP_SUPPORTS_DMABUF``
+ - 0x0004
+ - This buffer type supports the ``V4L2_MEMORY_DMABUF`` streaming mode.
+* - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``
+ - 0x0008
+ - This buffer type supports :ref:`requests `.
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
[PATCH 5/5] vb2: set reqbufs/create_bufs capabilities
From: Hans Verkuil
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
---
.../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;
[PATCH 0/5] Post-v18: Request API updates
From: Hans Verkuil 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. 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. There will be a follow-up documentation patch incorporating Laurent's comments, but that doesn't change any APIs. Regards, Hans Hans Verkuil (5): 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 Documentation/media/uapi/v4l/buffer.rst | 6 ++-- .../media/uapi/v4l/vidioc-create-bufs.rst | 10 +- .../media/uapi/v4l/vidioc-g-ext-ctrls.rst | 36 +-- Documentation/media/uapi/v4l/vidioc-qbuf.rst | 12 +++ .../media/uapi/v4l/vidioc-reqbufs.rst | 36 ++- .../media/common/videobuf2/videobuf2-v4l2.c | 19 +- drivers/media/media-request.c | 6 ++-- 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 | 5 ++- drivers/media/v4l2-core/v4l2-ioctl.c | 4 +-- include/media/videobuf2-core.h| 2 ++ include/uapi/linux/videodev2.h| 13 +-- 14 files changed, 118 insertions(+), 41 deletions(-) -- 2.18.0
[PATCH 2/5] v4l2-ctrls: return -EACCES if request wasn't completed
From: Hans Verkuil
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
---
.../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 ` 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 `, 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 `, 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
Re: [RFC] Request API and V4L2 capabilities
Hi, On Fri, 2018-08-24 at 09:29 +0200, Hans Verkuil wrote: > On 08/23/2018 07:37 PM, Nicolas Dufresne wrote: > > Le jeudi 23 août 2018 à 16:31 +0200, Hans Verkuil a écrit : > > > > I propose adding these capabilities: > > > > > > > > #define V4L2_BUF_CAP_HAS_REQUESTS 0x0001 > > > > #define V4L2_BUF_CAP_REQUIRES_REQUESTS0x0002 > > > > #define V4L2_BUF_CAP_HAS_MMAP 0x0100 > > > > #define V4L2_BUF_CAP_HAS_USERPTR 0x0200 > > > > #define V4L2_BUF_CAP_HAS_DMABUF 0x0400 > > > > > > I substituted SUPPORTS for HAS and dropped the REQUIRES_REQUESTS > > > capability. > > > As Tomasz mentioned, technically (at least for stateless codecs) you could > > > handle just one frame at a time without using requests. It's very > > > inefficient, > > > but it would work. > > > > I thought the request was providing a data structure to refer back to > > the frames, so each codec don't have to implement one. Do you mean that > > the framework will implicitly request requests in that mode ? or simply > > that there is no such helper ? > > Yes, that's done through controls as well. > > The idea would be that you set the necessary controls, queue a buffer to > the output queue, dequeue a buffer from the output queue, read back any > new state information and repeat the process. > > That said, I'm not sure if the cedrus driver for example can handle this > at the moment. It is also inefficient and it won't work if codecs require > more than one buffer in the queue for whatever reason. > > Tomasz, Paul, please correct me if I am wrong. I haven't tested decoding without requests, but I suppose it should work when submitting only one source buffer at a time. By the way, note that our VAAPI backend for the request API gets the slice data and metadata for each frame sequentially and we are not yet starting to fill the next request before the current one was completed (fences would probably help implement that). > In any case, I think we can do without this proposed capability since it is > simply a requirement when implementing the pixelformat for the stateless > codec that the Request API will be available and it should be documented > as such in the spec. Agreed. Cheers, Paul -- Paul Kocialkowski, Bootlin (formerly Free Electrons) Embedded Linux and kernel engineering https://bootlin.com signature.asc Description: This is a digitally signed message part
Re: [PATCHv18 07/35] v4l2-device.h: add v4l2_device_supports_requests() helper
On Tue, Aug 14, 2018 at 04:20:19PM +0200, Hans Verkuil wrote: > From: Hans Verkuil > > Add a simple helper function that tests if the driver supports > the request API. > > Signed-off-by: Hans Verkuil > Reviewed-by: Mauro Carvalho Chehab Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH] media: aptina-pll: allow approximating the requested pix_clock
Hi Sakari, Thank you for taking the time to look into the issue. On Thu, Aug 23, 2018 at 01:30:12PM +0200, Sakari Ailus wrote: > Knowing the formula, the limits as well as the external clock frequency, it > should be relatively straightforward to come up with a functional pixel > clock value. Was there a practical problem in coming up with such a value? I've added a concrete example to my other reply and should have done that with posting the initial question. I'm sorry for not having done it earlier. > You can't pick a value at random; it needs to be one that actually works. > The purpose of having an exact frequency is that the typical systems where > these devices are being used, as I explained earlier, is that having a > random frequency, albeit with which the sensor could possibly work, the > other devices in the system may not do so. We already refuted the concept of an "exact frequency". In nature, it simply isn't exact. Every board will have a slightly different frequency no matter how precise you calculate it. I'm not after random frequencies in any case. The goal of course is to approximate the requested frequency as good as possible. In particular, when the requested pix_clock allows using a parameter set that attains the frequency exactly, that parameter set will be emitted rather than some other approximation. Only for parameters where the old algorithm returns -EINVAL there should be an observable difference. Using an exact frequency is difficult here. How am I supposed to write e.g. 74242424.24242424... Hz into the device tree? Helmut
Re: [PATCH] media: aptina-pll: allow approximating the requested pix_clock
Hi Laurent,
Thank you for taking the time to reply to my patch and to my earlier
questions.
On Thu, Aug 23, 2018 at 01:12:15PM +0200, Laurent Pinchart wrote:
> Could you please share numbers, ideally when run in kernel space ?
Can you explain the benefits of profiling this inside the kernel rather
than outside? Doing it outside is much simpler, so I'd like to
understand your reasons. The next question is what kind of system to
profile on. I guess doing it on an X86 will not help. Typical users will
use some arm CPU and integer division on arm is slower than on x86. Is a
Cortex A9 ok?
If you can provide more relevant inputs, that'd also help. I was able to
derive an example input from the dt bindings for mt9p031 (ext=6MHz,
pix=96MHz) and also used random inputs for testing. Getting more
real-world inputs would help in producing a useful benchmark.
> This patch is very hard to review as you rewrite the whole, removing all the
> documentation for the existing algorithm, without documenting the new one.
That's not entirely fair. Unlike the original algorithm, I've split it
to multiple functions and unlike the original algorithm, I've documented
pre- and post-conditions. In a sense, that's actually more
documentation. At least, you now see what the functions are supposed to
be doing.
Inside the alogrithm, I've often writting which precondition
(in)equation I used in which particular step.
The variable naming choice makes it very clear that the first step is
reducing the value ranges for n, m, and p1 as much as possible before
proceeding to an actual parameter computation.
There was little choice in removing much of the old algorithm as the
approach of using gcd is numerically unstable. I actually kept
everything until the first gcd computation.
> There's also no example of a failing case with the existing code that works
> with yours.
That is an unfortunate omission. I should have thought of this and
should have given it upfront. I'm sorry.
Take for instance MT9M024. The data sheet
(http://www.mouser.com/ds/2/308/MT9M024-D-606228.pdf) allows deducing
the following limits:
const struct aptina_pll_limits mt9m024_limits = {
.ext_clock_min = 600,
.ext_clock_max = 5000,
.int_clock_min = 200,
.int_clock_max = 2400,
.out_clock_min = 38400,
.out_clock_max = 76800,
.pix_clock_max = 7425,
.n_min = 1,
.n_max = 63,
.m_min = 32,
.m_max = 255,
.p1_min = 4,
.p1_max = 16,
};
Now if you choose ext_clock and pix_clock maximal within the given
limits, the existing aptina_pll_calculate gives up. Lowering the
pix_clock does not help either. Even down to 73 MHz, it is unable to
find any pll configuration.
The new algorithm finds a solution (n=11, m=98, p1=6) with 7.5 KHz
error. Incidentally, that solution is close to the one given by the
vendor tool (n=22, m=196, p1=6).
> Could you please document the algorithm in details (especially explaining the
> background ideas), and share at least one use case, with with numbers for all
> the input and output parameters ?
I'll try to improve the documentation in the next version. Summarizing
the idea is something I can do, but beyond that I don't see much to add
beyond prose.
Ahead of posting a V2, let me suggest this:
/* The first part of the algorithm reduces the given aptina_pll_limits
* for n, m and p1 using the (in)equalities and the concrete values for
* ext_clock and pix_clock in order to reduce the search space.
*
* The main loop iterates over all remaining possible p1 values and
* computes the necessary out_clock frequency. The ext_clock / out_clock
* ratio is then approximated with n / m within their respective bounds.
* For each parameter choice, the preconditions must be rechecked,
* because integer rounding errors may result in violating some of the
* preconditions. The parameter set with the least frequency error is
* returned.
*/
Is this what you are looking for?
Helmut
Re: [PATCH 4/5] videodev2.h: add new capabilities for buffer types
Hi Hans, On Fri, Aug 24, 2018 at 5:22 PM Hans Verkuil wrote: > > From: Hans Verkuil > > VIDIOC_REQBUFS and VIDIOC_CREATE_BUFFERS will return capabilities > telling userspace what the given buffer type is capable of. > Please see my comments below. > Signed-off-by: Hans Verkuil > --- > .../media/uapi/v4l/vidioc-create-bufs.rst | 10 +- > .../media/uapi/v4l/vidioc-reqbufs.rst | 36 ++- > include/uapi/linux/videodev2.h| 13 +-- > 3 files changed, 55 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..fd34d3f236c9 100644 > --- a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst > +++ b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst > @@ -102,7 +102,15 @@ 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 ` for a list of > the > + capabilities. Perhaps it would make sense to document how the application is expected to query for these capabilities? Right now, the application is expected to fill in the "memory" field in this struct (and reqbufs counterpart), but it sounds a bit strange that one needs to know what "memory" value to write there to query what set of "memory" values is supported. In theory, MMAP is expected to be always supported, but it sounds strange anyway. Also, is there a way to call REQBUFS without altering the buffer allocation? Best regards, Tomasz
Re: [PATCH 0/5] Post-v18: Request API updates
Hi Hans, On Fri, Aug 24, 2018 at 5:22 PM Hans Verkuil wrote: > > From: Hans Verkuil > > 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 > For all patches except "[PATCH 4/5] videodev2.h: add new capabilities for buffer types": Reviewed-by: Tomasz Figa Best regards, Tomasz
cron job: media_tree daily build: OK
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Sat Aug 25 05:00:16 CEST 2018 media-tree git hash:da2048b7348a0be92f706ac019e022139e29495e media_build git hash: baf45935ffad914f33faf751ad9f4d0dd276c021 v4l-utils git hash: 015ca7524748fa7cef296102c68b631b078b63c6 edid-decode git hash: b2da1516df3cc2756bfe8d1fa06d7bf2562ba1f4 gcc version:i686-linux-gcc (GCC) 8.1.0 sparse version: 0.5.2 smatch version: 0.5.1 host hardware: x86_64 host os:4.17.0-1-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-multi: OK linux-git-arm-pxa: OK linux-git-arm-stm32: OK linux-git-arm64: OK linux-git-i686: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK Check COMPILE_TEST: OK linux-2.6.36.4-i686: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-i686: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-i686: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-i686: OK linux-2.6.39.4-x86_64: OK linux-3.0.101-i686: OK linux-3.0.101-x86_64: OK linux-3.1.10-i686: OK linux-3.1.10-x86_64: OK linux-3.2.102-i686: OK linux-3.2.102-x86_64: OK linux-3.3.8-i686: OK linux-3.3.8-x86_64: OK linux-3.4.113-i686: OK linux-3.4.113-x86_64: OK linux-3.5.7-i686: OK linux-3.5.7-x86_64: OK linux-3.6.11-i686: OK linux-3.6.11-x86_64: OK linux-3.7.10-i686: OK linux-3.7.10-x86_64: OK linux-3.8.13-i686: OK linux-3.8.13-x86_64: OK linux-3.9.11-i686: OK linux-3.9.11-x86_64: OK linux-3.10.108-i686: OK linux-3.10.108-x86_64: OK linux-3.11.10-i686: OK linux-3.11.10-x86_64: OK linux-3.12.74-i686: OK linux-3.12.74-x86_64: OK linux-3.13.11-i686: OK linux-3.13.11-x86_64: OK linux-3.14.79-i686: OK linux-3.14.79-x86_64: OK linux-3.15.10-i686: OK linux-3.15.10-x86_64: OK linux-3.16.57-i686: OK linux-3.16.57-x86_64: OK linux-3.17.8-i686: OK linux-3.17.8-x86_64: OK linux-3.18.115-i686: OK linux-3.18.115-x86_64: OK linux-3.19.8-i686: OK linux-3.19.8-x86_64: OK linux-4.18-i686: OK linux-4.18-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/index.html
Re: [PATCH v2 2/2] uvcvideo: add a D4M camera description
Hi Guennadi,
Thank you for the patch.
Overall this looks good to me, I only have small comments. Please see below,
with a summary at the end.
On Friday, 3 August 2018 14:37:08 EEST Guennadi Liakhovetski wrote:
> D4M is a mobile model from the D4XX family of Intel RealSense cameras.
> This patch adds a descriptor for it, which enables reading per-frame
> metadata from it.
>
> Signed-off-by: Guennadi Liakhovetski
> ---
> Documentation/media/uapi/v4l/meta-formats.rst | 1 +
> Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 204 +++
> drivers/media/usb/uvc/uvc_driver.c| 11 ++
> include/uapi/linux/videodev2.h| 1 +
> 4 files changed, 217 insertions(+)
> create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
[snip]
> diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst new file mode 100644
> index 000..57ecfd9
> --- /dev/null
> +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst
> @@ -0,0 +1,204 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _v4l2-meta-fmt-d4xx:
> +
> +***
> +V4L2_META_FMT_D4XX ('D4XX')
> +***
> +
> +Intel D4xx UVC Cameras Metadata
> +
> +
> +Description
> +===
> +
> +Intel D4xx (D435 and other) cameras include per-frame metadata in their UVC
> +payload headers, following the Microsoft(R) UVC extension proposal [1_].
> That +means, that the private D4XX metadata, following the standard UVC
> header, is +organised in blocks. D4XX cameras implement several standard
> block types, +proposed by Microsoft, and several proprietary ones.
> Supported standard metadata +types are MetadataId_CaptureStats (ID 3),
> MetadataId_CameraExtrinsics (ID 4), +and MetadataId_CameraIntrinsics (ID
> 5). For their description see [1_]. This +document describes proprietary
> metadata types, used by D4xx cameras.
> +
> +V4L2_META_FMT_D4XX buffers follow the metadata buffer layout of
> +V4L2_META_FMT_UVC with the only difference, that it also includes
> proprietary +payload header data. D4xx cameras use bulk transfers and only
> send one payload +per frame, therefore their headers cannot be larger than
> 255 bytes.
> +
> +Below are proprietary Microsoft style metadata types, used by D4xx cameras,
> +where all fields are in little endian order:
> +
> +.. flat-table:: D4xx metadata
> +:widths: 1 4
> +:header-rows: 1
> +:stub-columns: 0
> +
> +* - Field
> + - Description
> +* - :cspan:`1` *Depth Control*
Does this mean that all fields in this structure apply to the depth image only
? If so, do you mind if I mention that explicitly ?
> +* - __u32 ID
> + - 0x8000
> +* - __u32 Size
> + - Size in bytes (currently 56)
> +* - __u32 Version
> + - Version of the struct
I would elaborate a bit here, how about the following ?
"Version of this structure. The documentation herein corresponds to version
xxx. The version number will be incremented when new fields are added."
If you can provide me with the current version number I can update this when
applying the patch (I would get it myself, but I'm about to board a plane and
haven't taken the camera with me :-/).
> +* - __u32 Flags
> + - A bitmask of flags: see [2_] below
> +* - __u32 Gain
> + - Gain value in internal units, same as the V4L2_CID_GAIN control,
> used to
> +capture the frame
> +* - __u32 Exposure
> + - Exposure time (in microseconds) used to capture the frame
> +* - __u32 Laser power
> + - Power of the laser LED 0-360, used for depth measurement
> +* - __u32 AE mode
> + - 0: manual; 1: automatic exposure
> +* - __u32 Exposure priority
> + - Exposure priority value: 0 - constant frame rate
> +* - __u32 AE ROI left
> + - Left border of the AE Region of Interest (all ROI values are in
> pixels
> +and lie between 0 and maximum width or height respectively)
> +* - __u32 AE ROI right
> + - Right border of the AE Region of Interest
> +* - __u32 AE ROI top
> + - Top border of the AE Region of Interest
> +* - __u32 AE ROI bottom
> + - Bottom border of the AE Region of Interest
> +* - __u32 Preset
> + - Preset selector value, default: 0, unless changed by the user
I won't block this patch for this, but could we get the documentation of the
corresponding XU control ? Even better, if Intel could publish documentation
of the full XUs, that would be great :-)
> +* - __u32 Laser mode
> + - 0: off, 1: on
> +* - :cspan:`1` *Capture Timing*
Similarly, what does this apply to ? The left sensor, the fish eye camera, or
both (or something else) ?
> +* - __u32 ID
> + - 0x8001
> +* - __u32 Size
> + - Size in bytes (currently 40)
> +* - __u32 Version
> + - Version of the struct
> +* - __u32 Flags
> + - A bitmask of flags: see [3_]
Re: [PATCH] uvcvideo: add a D4M camera description
Hi Guennadi, On Friday, 3 August 2018 14:07:12 EEST Guennadi Liakhovetski wrote: > Hi Laurent, > > Thanks for the review. A general note: I think you're requesting a rather > detailed information about many parameters. That isn't a problem by > itself, however, it is difficult to obtain some of that information. I'll > address whatever comments I can in an updated version, just answering some > questions here. I directed youor questions, that I couldn't answer myself > to respective people, but I have no idea if and when I get replies. So, > it's up to you whether to wait for that additional information or to take > at least what we have now. I've replied to v2, and apart from a few minor points, I think we can apply the current version. There are a few small questions I would still like to have answers to, but if it takes to long to obtain that, let's not miss v4.20. > On Sun, 29 Jul 2018, Laurent Pinchart wrote: > > On Saturday, 23 December 2017 13:11:00 EEST Guennadi Liakhovetski wrote: > >> From: Guennadi Liakhovetski > >> > >> D4M is a mobile model from the D4XX family of Intel RealSense cameras. > >> This patch adds a descriptor for it, which enables reading per-frame > >> metadata from it. > >> > >> Signed-off-by: Guennadi Liakhovetski > >> --- > >> > >> Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst | 202 > >> drivers/media/usb/uvc/uvc_driver.c| 11 ++ > >> include/uapi/linux/videodev2.h| 1 + > >> 3 files changed, 214 insertions(+) > >> create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst > >> > >> diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst > >> b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst new file mode 100644 > >> index 000..950780d > >> --- /dev/null > >> +++ b/Documentation/media/uapi/v4l/pixfmt-meta-d4xx.rst [snip] > >> +* - :cspan:`1` *Configuration* > >> +* - __u32 ID > >> + - 0x8002 > >> +* - __u32 Size > >> + - Size in bytes (currently 40) > >> +* - __u32 Version > >> + - Version of the struct > >> +* - __u32 Flags > >> + - A bitmask of flags: see [4_] below > >> +* - __u8 Hardware type > >> + - Camera hardware version [5_] > >> +* - __u8 SKU ID > >> + - Camera hardware configuration [6_] > >> +* - __u32 Cookie > >> + - Internal synchronisation > > > > Internal synchronisation with what ? :-) This is still something I'd like to understand (and I understand it may still take time to receive an answer from the right person). > >> +* - __u16 Format > >> + - Image format code [7_] > >> +* - __u16 Width > >> + - Width in pixels > >> +* - __u16 Height > >> + - Height in pixels > >> +* - __u16 Framerate > >> + - Requested framerate > > > > What's the unit of this value ? > > Is anything other than frames per second used in V4L? V4L2 expresses the frame rate as a fraction, hence my question, to know whether this field contained the number of frames per second as an integer, or used a different representation (such as a fixed point decimal value for instance). > >> +* - __u16 Trigger > >> + - Byte 0: bit 0: depth and RGB are synchronised, bit 1: external > >> trigger > >> + > >> +.. _1: > >> + > >> +[1] > >> https://docs.microsoft.com/en-us/windows-hardware/drivers/stream/uvc-ext > >> ensions-1-5 > > > > Should we at some point replicate that documentation in the V4L2 spec ? > > Without copying it of course, as that would be a copyright violation. > > Well, we don't replicate the UVC itself or any other standards, do we? Of > course, that document doesn't have the same status as an official > vendor-neutral standard, but still, we don't replicate data sheets either. > Besides, I think there are cameras that use this, and windows supports > this, so, don't think it will disappear overnight... Probably not overnight, you're right. I'm a bit worried about the link becoming invalid though. In any case that's not a blocker, but I might at some point decide to replicate the documentation. [snip] -- Regards, Laurent Pinchart
