Hello
v6 changes:
The design has been slightly reworked. The cache-hints capability has
been renamed to SUPPORTS_MMAP_CACHE_HINTS and is reported for all queues
that support MMAP and allow cache hints. However, the actual hints and
memory consistency are ignored unless the queue is used for the MMAP
streaming I/O. Plus some cleanups, documentation updates, and so on.
Previous versions:
v5 link: https://lore.kernel.org/lkml/[email protected]
v4 link: https://lore.kernel.org/lkml/[email protected]/
v3 link: https://lore.kernel.org/lkml/[email protected]
v2 link: https://lore.kernel.org/lkml/[email protected]/
v1 link: https://lore.kernel.org/lkml/[email protected]/
Series Intro
========================================================================
This is a reworked version of the vb2 cache hints
(V4L2_BUF_FLAG_NO_CACHE_INVALIDATE / V4L2_BUF_FLAG_NO_CACHE_CLEAN)
support patch series which previsouly was developed by Sakari and
Laurent [0].
The patch set attempts to preserve the existing behvaiour - cache
sync is performed in ->prepare() and ->finish() (unless the buffer
is DMA exported). User space can request “default behavior” override
with cache management hints, which are handled on a per-buffer basis
and should be supplied with v4l2_buffer ->flags during buffer
preparation. There are two possible hints:
- V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
No cache sync on ->finish()
- V4L2_BUF_FLAG_NO_CACHE_CLEAN
No cache sync on ->prepare()
In order to keep things on the safe side, we also require driver
to explicitly state which of its queues (if any) support user space
cache management hints (such queues should have ->allow_cache_hints
bit set).
The patch set also (to some extent) simplifies allocators' ->prepare()
and ->finish() callbacks. Namely, we move cache management decision
making to the upper - core - layer. For example, if, previously, we
would have something like this
vb2_buffer_done()
vb2_dc_finish()
if (buf->db_attach)
return;
where each allocators' ->finish() callback would either bail
out (DMA exported buffer, for instance) or sync, now that "bail
out or sync" decision is made before we call into the allocator.
Along with cache management hints, user space is also able to
adjust queue's memory consistency attributes. Memory consistency
attribute (dma_attrs) is per-queue, yet it plays its role on the
allocator level, when we allocate buffers’ private memory (planes).
For the time being, only one consistency attribute is supported:
DMA_ATTR_NON_CONSISTENT.
[0] https://www.mail-archive.com/[email protected]/msg112459.html
Sergey Senozhatsky (14):
videobuf2: use explicit unsigned int in vb2_queue
videobuf2: add cache management members
videobuf2: handle V4L2 buffer cache flags
videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag
videobuf2: add queue memory consistency parameter
videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT flag
videobuf2: factor out planes prepare/finish functions
videobuf2: do not sync caches when we are allowed not to
videobuf2: check ->synced flag in prepare() and finish()
videobuf2: add begin/end cpu_access callbacks to dma-contig
videobuf2: add begin/end cpu_access callbacks to dma-sg
videobuf2: don't test db_attach in dma-contig prepare and finish
videobuf2: remove redundant if-statement
media: vivid: add cache_hints module param
Documentation/admin-guide/media/vivid.rst | 9 ++
.../userspace-api/media/v4l/buffer.rst | 40 +++++-
.../media/v4l/vidioc-create-bufs.rst | 7 +-
.../media/v4l/vidioc-reqbufs.rst | 21 ++-
.../media/common/videobuf2/videobuf2-core.c | 121 +++++++++++++-----
.../common/videobuf2/videobuf2-dma-contig.c | 44 ++++++-
.../media/common/videobuf2/videobuf2-dma-sg.c | 38 ++++--
.../media/common/videobuf2/videobuf2-v4l2.c | 72 ++++++++++-
drivers/media/dvb-core/dvb_vb2.c | 2 +-
drivers/media/test-drivers/vivid/vivid-core.c | 9 ++
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 10 +-
drivers/media/v4l2-core/v4l2-ioctl.c | 5 +-
include/media/videobuf2-core.h | 47 +++++--
include/uapi/linux/videodev2.h | 14 +-
14 files changed, 366 insertions(+), 73 deletions(-)
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
Switch from 'unsigned' to 'unsigned int' so that checkpatch doesn't
complain.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
include/media/videobuf2-core.h | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index f11b96514cf7..9e522bd2acc7 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -558,15 +558,15 @@ struct vb2_queue {
unsigned int io_modes;
struct device *dev;
unsigned long dma_attrs;
- unsigned bidirectional:1;
- unsigned fileio_read_once:1;
- unsigned fileio_write_immediately:1;
- unsigned allow_zero_bytesused:1;
- unsigned quirk_poll_must_check_waiting_for_buffers:1;
- unsigned supports_requests:1;
- unsigned requires_requests:1;
- unsigned uses_qbuf:1;
- unsigned uses_requests:1;
+ unsigned int bidirectional:1;
+ unsigned int fileio_read_once:1;
+ unsigned int fileio_write_immediately:1;
+ unsigned int allow_zero_bytesused:1;
+ unsigned int quirk_poll_must_check_waiting_for_buffers:1;
+ unsigned int supports_requests:1;
+ unsigned int requires_requests:1;
+ unsigned int uses_qbuf:1;
+ unsigned int uses_requests:1;
struct mutex *lock;
void *owner;
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
Extend vb2_buffer and vb2_queue structs with cache management
members.
V4L2 UAPI already contains two buffer flags which user-space,
supposedly, can use to control buffer cache sync:
- V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
- V4L2_BUF_FLAG_NO_CACHE_CLEAN
None of these, however, do anything at the moment. This patch
set is intended to change it.
Since user-space cache management hints are supposed to be
implemented on a per-buffer basis we need to extend vb2_buffer
struct with two new members ->need_cache_sync_on_prepare and
->need_cache_sync_on_finish, which will store corresponding
user-space hints.
In order to preserve the existing behaviour, user-space cache
managements flags will be handled only by those drivers that
permit user-space cache hints. That's the purpose of vb2_queue
->allow_cache_hints member. Driver must set ->allow_cache_hints
during queue initialisation to enable cache management hints
mechanism.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
include/media/videobuf2-core.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 9e522bd2acc7..7f39d9fffc8c 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -263,6 +263,10 @@ struct vb2_buffer {
* after the 'buf_finish' op is called.
* copied_timestamp: the timestamp of this capture buffer was copied
* from an output buffer.
+ * need_cache_sync_on_prepare: when set buffer's ->prepare() function
+ * performs cache sync/invalidation.
+ * need_cache_sync_on_finish: when set buffer's ->finish() function
+ * performs cache sync/invalidation.
* queued_entry: entry on the queued buffers list, which holds
* all buffers queued from userspace
* done_entry: entry on the list that stores all buffers ready
@@ -273,6 +277,8 @@ struct vb2_buffer {
unsigned int synced:1;
unsigned int prepared:1;
unsigned int copied_timestamp:1;
+ unsigned int need_cache_sync_on_prepare:1;
+ unsigned int need_cache_sync_on_finish:1;
struct vb2_plane planes[VB2_MAX_PLANES];
struct list_head queued_entry;
@@ -491,6 +497,9 @@ struct vb2_buf_ops {
* @uses_requests: requests are used for this queue. Set to 1 the first time
* a request is queued. Set to 0 when the queue is canceled.
* If this is 1, then you cannot queue buffers directly.
+ * @allow_cache_hints: when set user-space can pass cache management hints in
+ * order to skip cache flush/invalidation on ->prepare() or/and
+ * ->finish().
* @lock: pointer to a mutex that protects the &struct vb2_queue. The
* driver can set this to a mutex to let the v4l2 core serialize
* the queuing ioctls. If the driver wants to handle locking
@@ -567,6 +576,7 @@ struct vb2_queue {
unsigned int requires_requests:1;
unsigned int uses_qbuf:1;
unsigned int uses_requests:1;
+ unsigned int allow_cache_hints:1;
struct mutex *lock;
void *owner;
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
This patch lets user-space to request a non-consistent memory
allocation during CREATE_BUFS and REQBUFS ioctl calls.
= CREATE_BUFS
struct v4l2_create_buffers has seven 4-byte reserved areas,
so reserved[0] is renamed to ->flags. The struct, thus, now
has six reserved 4-byte regions.
= CREATE_BUFS32
struct v4l2_create_buffers32 has seven 4-byte reserved areas,
so reserved[0] is renamed to ->flags. The struct, thus, now
has six reserved 4-byte regions.
= REQBUFS
We use one bit of a ->reserved[1] member of struct v4l2_requestbuffers,
which is now renamed to ->flags. Unlike v4l2_create_buffers, struct
v4l2_requestbuffers does not have enough reserved room. Therefore for
backward compatibility ->reserved and ->flags were put into anonymous
union.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
.../media/v4l/vidioc-create-bufs.rst | 7 +++++-
.../media/v4l/vidioc-reqbufs.rst | 11 +++++++--
.../media/common/videobuf2/videobuf2-core.c | 6 +++++
.../media/common/videobuf2/videobuf2-v4l2.c | 24 +++++++++++++++----
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 10 ++++++--
drivers/media/v4l2-core/v4l2-ioctl.c | 5 +---
include/uapi/linux/videodev2.h | 11 +++++++--
7 files changed, 59 insertions(+), 15 deletions(-)
diff --git a/Documentation/userspace-api/media/v4l/vidioc-create-bufs.rst b/Documentation/userspace-api/media/v4l/vidioc-create-bufs.rst
index e1afc5b504c2..f2a702870fad 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-create-bufs.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-create-bufs.rst
@@ -121,7 +121,12 @@ than the number requested.
other changes, then set ``count`` to 0, ``memory`` to
``V4L2_MEMORY_MMAP`` and ``format.type`` to the buffer type.
* - __u32
- - ``reserved``\ [7]
+ - ``flags``
+ - Specifies additional buffer management attributes.
+ See :ref:`memory-flags`.
+
+ * - __u32
+ - ``reserved``\ [6]
- A place holder for future extensions. Drivers and applications
must set the array to zero.
diff --git a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
index 96a59793d857..75d894d9c36c 100644
--- a/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
+++ b/Documentation/userspace-api/media/v4l/vidioc-reqbufs.rst
@@ -112,10 +112,17 @@ aborting or finishing any DMA in progress, an implicit
``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.
+ * - union {
+ - (anonymous)
+ * - __u32
+ - ``flags``
+ - Specifies additional buffer management attributes.
+ See :ref:`memory-flags`.
* - __u32
- ``reserved``\ [1]
- - A place holder for future extensions. Drivers and applications
- must set the array to zero.
+ - Kept for backwards compatibility. Use ``flags`` instead.
+ * - }
+ -
.. tabularcolumns:: |p{6.1cm}|p{2.2cm}|p{8.7cm}|
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index b1332f7f1aad..6efac531006f 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -695,6 +695,9 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
unsigned int i;
int ret;
+ if (flags & V4L2_FLAG_MEMORY_NON_CONSISTENT)
+ consistent_mem = false;
+
if (q->streaming) {
dprintk(1, "streaming active\n");
return -EBUSY;
@@ -838,6 +841,9 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
bool consistent_mem = true;
int ret;
+ if (flags & V4L2_FLAG_MEMORY_NON_CONSISTENT)
+ consistent_mem = false;
+
if (q->num_buffers == VB2_MAX_FRAME) {
dprintk(1, "maximum number of buffers already allocated\n");
return -ENOBUFS;
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 26a3ec333bb7..559a229cac41 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -718,12 +718,22 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
#endif
}
+static void clear_consistency_attr(struct vb2_queue *q,
+ int memory,
+ unsigned int *flags)
+{
+ if (!q->allow_cache_hints || memory != V4L2_MEMORY_MMAP)
+ *flags &= ~V4L2_FLAG_MEMORY_NON_CONSISTENT;
+}
+
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, 0, &req->count);
+ clear_consistency_attr(q, req->memory, &req->flags);
+ return ret ? ret : vb2_core_reqbufs(q, req->memory,
+ req->flags, &req->count);
}
EXPORT_SYMBOL_GPL(vb2_reqbufs);
@@ -755,6 +765,7 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
unsigned i;
fill_buf_caps(q, &create->capabilities);
+ clear_consistency_attr(q, create->memory, &create->flags);
create->index = q->num_buffers;
if (create->count == 0)
return ret != -EBUSY ? ret : 0;
@@ -797,8 +808,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
for (i = 0; i < requested_planes; i++)
if (requested_sizes[i] == 0)
return -EINVAL;
- return ret ? ret : vb2_core_create_bufs(q, create->memory, 0,
- &create->count, requested_planes, requested_sizes);
+ return ret ? ret : vb2_core_create_bufs(q, create->memory,
+ create->flags,
+ &create->count,
+ requested_planes,
+ requested_sizes);
}
EXPORT_SYMBOL_GPL(vb2_create_bufs);
@@ -969,11 +983,12 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv,
int res = vb2_verify_memory_type(vdev->queue, p->memory, p->type);
fill_buf_caps(vdev->queue, &p->capabilities);
+ clear_consistency_attr(vdev->queue, p->memory, &p->flags);
if (res)
return res;
if (vb2_queue_is_busy(vdev, file))
return -EBUSY;
- res = vb2_core_reqbufs(vdev->queue, p->memory, 0, &p->count);
+ res = vb2_core_reqbufs(vdev->queue, p->memory, p->flags, &p->count);
/* If count == 0, then the owner has released all buffers and he
is no longer owner of the queue. Otherwise we have a new owner. */
if (res == 0)
@@ -991,6 +1006,7 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
p->index = vdev->queue->num_buffers;
fill_buf_caps(vdev->queue, &p->capabilities);
+ clear_consistency_attr(vdev->queue, p->memory, &p->flags);
/*
* 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/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index a99e82ec9ab6..593bcf6c3735 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -246,6 +246,9 @@ struct v4l2_format32 {
* @memory: buffer memory type
* @format: frame format, for which buffers are requested
* @capabilities: capabilities of this buffer type.
+ * @flags: additional buffer management attributes (ignored unless the
+ * queue has V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS capability and
+ * configured for MMAP streaming I/O).
* @reserved: future extensions
*/
struct v4l2_create_buffers32 {
@@ -254,7 +257,8 @@ struct v4l2_create_buffers32 {
__u32 memory; /* enum v4l2_memory */
struct v4l2_format32 format;
__u32 capabilities;
- __u32 reserved[7];
+ __u32 flags;
+ __u32 reserved[6];
};
static int __bufsize_v4l2_format(struct v4l2_format32 __user *p32, u32 *size)
@@ -355,7 +359,8 @@ static int get_v4l2_create32(struct v4l2_create_buffers __user *p64,
{
if (!access_ok(p32, sizeof(*p32)) ||
copy_in_user(p64, p32,
- offsetof(struct v4l2_create_buffers32, format)))
+ offsetof(struct v4l2_create_buffers32, format)) ||
+ assign_in_user(&p64->flags, &p32->flags))
return -EFAULT;
return __get_v4l2_format32(&p64->format, &p32->format,
aux_buf, aux_space);
@@ -417,6 +422,7 @@ static int put_v4l2_create32(struct v4l2_create_buffers __user *p64,
copy_in_user(p32, p64,
offsetof(struct v4l2_create_buffers32, format)) ||
assign_in_user(&p32->capabilities, &p64->capabilities) ||
+ assign_in_user(&p32->flags, &p64->flags) ||
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 2322f08a98be..02bfef0da76d 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2038,9 +2038,6 @@ static int v4l_reqbufs(const struct v4l2_ioctl_ops *ops,
if (ret)
return ret;
-
- CLEAR_AFTER_FIELD(p, capabilities);
-
return ops->vidioc_reqbufs(file, fh, p);
}
@@ -2080,7 +2077,7 @@ static int v4l_create_bufs(const struct v4l2_ioctl_ops *ops,
if (ret)
return ret;
- CLEAR_AFTER_FIELD(create, capabilities);
+ CLEAR_AFTER_FIELD(create, flags);
v4l_sanitize_format(&create->format);
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 34ba1017b89b..fec2607a07e3 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -946,7 +946,10 @@ struct v4l2_requestbuffers {
__u32 type; /* enum v4l2_buf_type */
__u32 memory; /* enum v4l2_memory */
__u32 capabilities;
- __u32 reserved[1];
+ union {
+ __u32 flags;
+ __u32 reserved[1];
+ };
};
/* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
@@ -2450,6 +2453,9 @@ struct v4l2_dbg_chip_info {
* @memory: enum v4l2_memory; buffer memory type
* @format: frame format, for which buffers are requested
* @capabilities: capabilities of this buffer type.
+ * @flags: additional buffer management attributes (ignored unless the
+ * queue has V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS capability
+ * and configured for MMAP streaming I/O).
* @reserved: future extensions
*/
struct v4l2_create_buffers {
@@ -2458,7 +2464,8 @@ struct v4l2_create_buffers {
__u32 memory;
struct v4l2_format format;
__u32 capabilities;
- __u32 reserved[7];
+ __u32 flags;
+ __u32 reserved[6];
};
/*
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
Factor out the code, no functional changes.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
.../media/common/videobuf2/videobuf2-core.c | 52 +++++++++++--------
1 file changed, 31 insertions(+), 21 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 6efac531006f..6a95663afcd1 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -296,6 +296,32 @@ static void __vb2_buf_dmabuf_put(struct vb2_buffer *vb)
__vb2_plane_dmabuf_put(vb, &vb->planes[plane]);
}
+/*
+ * __vb2_buf_mem_prepare() - call ->prepare() on buffer's private memory
+ * to sync caches
+ */
+static void __vb2_buf_mem_prepare(struct vb2_buffer *vb)
+{
+ unsigned int plane;
+
+ for (plane = 0; plane < vb->num_planes; ++plane)
+ call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
+ vb->synced = 1;
+}
+
+/*
+ * __vb2_buf_mem_finish() - call ->finish on buffer's private memory
+ * to sync caches
+ */
+static void __vb2_buf_mem_finish(struct vb2_buffer *vb)
+{
+ unsigned int plane;
+
+ for (plane = 0; plane < vb->num_planes; ++plane)
+ call_void_memop(vb, finish, vb->planes[plane].mem_priv);
+ vb->synced = 0;
+}
+
/*
* __setup_offsets() - setup unique offsets ("cookies") for every plane in
* the buffer.
@@ -960,7 +986,6 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
{
struct vb2_queue *q = vb->vb2_queue;
unsigned long flags;
- unsigned int plane;
if (WARN_ON(vb->state != VB2_BUF_STATE_ACTIVE))
return;
@@ -980,12 +1005,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
dprintk(4, "done processing on buffer %d, state: %d\n",
vb->index, state);
- if (state != VB2_BUF_STATE_QUEUED) {
- /* sync buffers */
- for (plane = 0; plane < vb->num_planes; ++plane)
- call_void_memop(vb, finish, vb->planes[plane].mem_priv);
- vb->synced = 0;
- }
+ if (state != VB2_BUF_STATE_QUEUED)
+ __vb2_buf_mem_finish(vb);
spin_lock_irqsave(&q->done_lock, flags);
if (state == VB2_BUF_STATE_QUEUED) {
@@ -1310,7 +1331,6 @@ static int __buf_prepare(struct vb2_buffer *vb)
{
struct vb2_queue *q = vb->vb2_queue;
enum vb2_buffer_state orig_state = vb->state;
- unsigned int plane;
int ret;
if (q->error) {
@@ -1354,11 +1374,7 @@ static int __buf_prepare(struct vb2_buffer *vb)
return ret;
}
- /* sync buffers */
- for (plane = 0; plane < vb->num_planes; ++plane)
- call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
-
- vb->synced = 1;
+ __vb2_buf_mem_prepare(vb);
vb->prepared = 1;
vb->state = orig_state;
@@ -1978,14 +1994,8 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
call_void_vb_qop(vb, buf_request_complete, vb);
}
- if (vb->synced) {
- unsigned int plane;
-
- for (plane = 0; plane < vb->num_planes; ++plane)
- call_void_memop(vb, finish,
- vb->planes[plane].mem_priv);
- vb->synced = 0;
- }
+ if (vb->synced)
+ __vb2_buf_mem_finish(vb);
if (vb->prepared) {
call_void_vb_qop(vb, buf_finish, vb);
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support.
Extend vb2_core_reqbufs() parameters list to accept requests'
->flags, which will be used for memory consistency configuration.
An attempt to allocate a buffer with consistency requirements
which don't match queue's consistency model will fail.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
.../media/common/videobuf2/videobuf2-core.c | 52 +++++++++++++++----
.../media/common/videobuf2/videobuf2-v4l2.c | 6 +--
drivers/media/dvb-core/dvb_vb2.c | 2 +-
include/media/videobuf2-core.h | 8 ++-
4 files changed, 52 insertions(+), 16 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 44d65f5be845..b1332f7f1aad 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -664,11 +664,34 @@ int vb2_verify_memory_type(struct vb2_queue *q,
}
EXPORT_SYMBOL(vb2_verify_memory_type);
+static void set_queue_consistency(struct vb2_queue *q, bool consistent_mem)
+{
+ if (!vb2_queue_allows_cache_hints(q))
+ return;
+
+ if (consistent_mem)
+ q->dma_attrs &= ~DMA_ATTR_NON_CONSISTENT;
+ else
+ q->dma_attrs |= DMA_ATTR_NON_CONSISTENT;
+}
+
+static bool verify_consistency_attr(struct vb2_queue *q, bool consistent_mem)
+{
+ bool queue_is_consistent = !(q->dma_attrs & DMA_ATTR_NON_CONSISTENT);
+
+ if (consistent_mem != queue_is_consistent) {
+ dprintk(1, "memory consistency model mismatch\n");
+ return false;
+ }
+ return true;
+}
+
int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
- unsigned int *count)
+ unsigned int flags, unsigned int *count)
{
unsigned int num_buffers, allocated_buffers, num_planes = 0;
unsigned plane_sizes[VB2_MAX_PLANES] = { };
+ bool consistent_mem = true;
unsigned int i;
int ret;
@@ -683,7 +706,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
}
if (*count == 0 || q->num_buffers != 0 ||
- (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
+ (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory) ||
+ !verify_consistency_attr(q, consistent_mem)) {
/*
* We already have buffers allocated, so first check if they
* are not in use and can be freed.
@@ -720,6 +744,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
num_buffers = min_t(unsigned int, num_buffers, VB2_MAX_FRAME);
memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
q->memory = memory;
+ set_queue_consistency(q, consistent_mem);
/*
* Ask the driver how many buffers and planes per buffer it requires.
@@ -804,11 +829,13 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
- unsigned int *count, unsigned requested_planes,
- const unsigned requested_sizes[])
+ unsigned int flags, unsigned int *count,
+ unsigned int requested_planes,
+ const unsigned int requested_sizes[])
{
unsigned int num_planes = 0, num_buffers, allocated_buffers;
unsigned plane_sizes[VB2_MAX_PLANES] = { };
+ bool consistent_mem = true;
int ret;
if (q->num_buffers == VB2_MAX_FRAME) {
@@ -823,10 +850,15 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
}
memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
q->memory = memory;
+ set_queue_consistency(q, consistent_mem);
q->waiting_for_buffers = !q->is_output;
- } else if (q->memory != memory) {
- dprintk(1, "memory model mismatch\n");
- return -EINVAL;
+ } else {
+ if (q->memory != memory) {
+ dprintk(1, "memory model mismatch\n");
+ return -EINVAL;
+ }
+ if (!verify_consistency_attr(q, consistent_mem))
+ return -EINVAL;
}
num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
@@ -2498,7 +2530,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
fileio->memory = VB2_MEMORY_MMAP;
fileio->type = q->type;
q->fileio = fileio;
- ret = vb2_core_reqbufs(q, fileio->memory, &fileio->count);
+ ret = vb2_core_reqbufs(q, fileio->memory, 0, &fileio->count);
if (ret)
goto err_kfree;
@@ -2555,7 +2587,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
err_reqbufs:
fileio->count = 0;
- vb2_core_reqbufs(q, fileio->memory, &fileio->count);
+ vb2_core_reqbufs(q, fileio->memory, 0, &fileio->count);
err_kfree:
q->fileio = NULL;
@@ -2575,7 +2607,7 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q)
vb2_core_streamoff(q, q->type);
q->fileio = NULL;
fileio->count = 0;
- vb2_core_reqbufs(q, fileio->memory, &fileio->count);
+ vb2_core_reqbufs(q, fileio->memory, 0, &fileio->count);
kfree(fileio);
dprintk(3, "file io emulator closed\n");
}
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index e4b4354b42b8..26a3ec333bb7 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -723,7 +723,7 @@ 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);
+ return ret ? ret : vb2_core_reqbufs(q, req->memory, 0, &req->count);
}
EXPORT_SYMBOL_GPL(vb2_reqbufs);
@@ -797,7 +797,7 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
for (i = 0; i < requested_planes; i++)
if (requested_sizes[i] == 0)
return -EINVAL;
- return ret ? ret : vb2_core_create_bufs(q, create->memory,
+ return ret ? ret : vb2_core_create_bufs(q, create->memory, 0,
&create->count, requested_planes, requested_sizes);
}
EXPORT_SYMBOL_GPL(vb2_create_bufs);
@@ -973,7 +973,7 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv,
return res;
if (vb2_queue_is_busy(vdev, file))
return -EBUSY;
- res = vb2_core_reqbufs(vdev->queue, p->memory, &p->count);
+ res = vb2_core_reqbufs(vdev->queue, p->memory, 0, &p->count);
/* If count == 0, then the owner has released all buffers and he
is no longer owner of the queue. Otherwise we have a new owner. */
if (res == 0)
diff --git a/drivers/media/dvb-core/dvb_vb2.c b/drivers/media/dvb-core/dvb_vb2.c
index 6974f1731529..959d110407a4 100644
--- a/drivers/media/dvb-core/dvb_vb2.c
+++ b/drivers/media/dvb-core/dvb_vb2.c
@@ -342,7 +342,7 @@ int dvb_vb2_reqbufs(struct dvb_vb2_ctx *ctx, struct dmx_requestbuffers *req)
ctx->buf_siz = req->size;
ctx->buf_cnt = req->count;
- ret = vb2_core_reqbufs(&ctx->vb_q, VB2_MEMORY_MMAP, &req->count);
+ ret = vb2_core_reqbufs(&ctx->vb_q, VB2_MEMORY_MMAP, 0, &req->count);
if (ret) {
ctx->state = DVB_VB2_STATE_NONE;
dprintk(1, "[%s] count=%d size=%d errno=%d\n", ctx->name,
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index ccc5c498d3e3..9e68fe043a6c 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -740,6 +740,8 @@ void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb);
* vb2_core_reqbufs() - Initiate streaming.
* @q: pointer to &struct vb2_queue with videobuf2 queue.
* @memory: memory type, as defined by &enum vb2_memory.
+ * @flags: auxiliary queue/buffer management flags. Currently, the only
+ * used flag is %V4L2_FLAG_MEMORY_NON_CONSISTENT.
* @count: requested buffer count.
*
* Videobuf2 core helper to implement VIDIOC_REQBUF() operation. It is called
@@ -764,12 +766,13 @@ void vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb);
* Return: returns zero on success; an error code otherwise.
*/
int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
- unsigned int *count);
+ unsigned int flags, unsigned int *count);
/**
* vb2_core_create_bufs() - Allocate buffers and any required auxiliary structs
* @q: pointer to &struct vb2_queue with videobuf2 queue.
* @memory: memory type, as defined by &enum vb2_memory.
+ * @flags: auxiliary queue/buffer management flags.
* @count: requested buffer count.
* @requested_planes: number of planes requested.
* @requested_sizes: array with the size of the planes.
@@ -787,7 +790,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
* Return: returns zero on success; an error code otherwise.
*/
int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
- unsigned int *count, unsigned int requested_planes,
+ unsigned int flags, unsigned int *count,
+ unsigned int requested_planes,
const unsigned int requested_sizes[]);
/**
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
Skip ->prepare() or/and ->finish() cache synchronisation if
user-space requested us to do so (or when queue dma direction
permits us to skip cache syncs).
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
drivers/media/common/videobuf2/videobuf2-core.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 6a95663afcd1..1a55ea19160b 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -304,8 +304,11 @@ static void __vb2_buf_mem_prepare(struct vb2_buffer *vb)
{
unsigned int plane;
- for (plane = 0; plane < vb->num_planes; ++plane)
- call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
+ if (vb->need_cache_sync_on_prepare) {
+ for (plane = 0; plane < vb->num_planes; ++plane)
+ call_void_memop(vb, prepare,
+ vb->planes[plane].mem_priv);
+ }
vb->synced = 1;
}
@@ -317,8 +320,11 @@ static void __vb2_buf_mem_finish(struct vb2_buffer *vb)
{
unsigned int plane;
- for (plane = 0; plane < vb->num_planes; ++plane)
- call_void_memop(vb, finish, vb->planes[plane].mem_priv);
+ if (vb->need_cache_sync_on_finish) {
+ for (plane = 0; plane < vb->num_planes; ++plane)
+ call_void_memop(vb, finish,
+ vb->planes[plane].mem_priv);
+ }
vb->synced = 0;
}
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
This simplifies the code a tiny bit and let's us to avoid
unneeded ->prepare()/->finish() calls.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
drivers/media/common/videobuf2/videobuf2-core.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 1a55ea19160b..7e081716b8da 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -304,6 +304,9 @@ static void __vb2_buf_mem_prepare(struct vb2_buffer *vb)
{
unsigned int plane;
+ if (vb->synced)
+ return;
+
if (vb->need_cache_sync_on_prepare) {
for (plane = 0; plane < vb->num_planes; ++plane)
call_void_memop(vb, prepare,
@@ -320,6 +323,9 @@ static void __vb2_buf_mem_finish(struct vb2_buffer *vb)
{
unsigned int plane;
+ if (!vb->synced)
+ return;
+
if (vb->need_cache_sync_on_finish) {
for (plane = 0; plane < vb->num_planes; ++plane)
call_void_memop(vb, finish,
@@ -2000,8 +2006,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
call_void_vb_qop(vb, buf_request_complete, vb);
}
- if (vb->synced)
- __vb2_buf_mem_finish(vb);
+ __vb2_buf_mem_finish(vb);
if (vb->prepared) {
call_void_vb_qop(vb, buf_finish, vb);
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
That if-statement seems to be unneeded.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
drivers/media/common/videobuf2/videobuf2-dma-contig.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index 5a3e1c3b556f..6ac0822e1bf0 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -152,8 +152,7 @@ static void *vb2_dc_alloc(struct device *dev, unsigned long attrs,
if (!buf)
return ERR_PTR(-ENOMEM);
- if (attrs)
- buf->attrs = attrs;
+ buf->attrs = attrs;
buf->cookie = dma_alloc_attrs(dev, size, &buf->dma_addr,
GFP_KERNEL | gfp_flags, buf->attrs);
if (!buf->cookie) {
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
Add a cache_hints module param to control per-queue user space cache
hints support.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
Documentation/admin-guide/media/vivid.rst | 9 +++++++++
drivers/media/test-drivers/vivid/vivid-core.c | 9 +++++++++
2 files changed, 18 insertions(+)
diff --git a/Documentation/admin-guide/media/vivid.rst b/Documentation/admin-guide/media/vivid.rst
index 52e57b773f07..6d7175f96f74 100644
--- a/Documentation/admin-guide/media/vivid.rst
+++ b/Documentation/admin-guide/media/vivid.rst
@@ -293,6 +293,15 @@ all configurable using the following module options:
- 0: vmalloc
- 1: dma-contig
+- cache_hints:
+
+ specifies if the device should set queues' user-space cache and memory
+ consistency hint capability (V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS).
+ The hints are valid only when using MMAP streaming I/O. Default is 0.
+
+ - 0: forbid hints
+ - 1: allow hints
+
Taken together, all these module options allow you to precisely customize
the driver behavior and test your application with all sorts of permutations.
It is also very suitable to emulate hardware that is not yet available, e.g.
diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
index 6c740e3e6999..5c986df4a8d4 100644
--- a/drivers/media/test-drivers/vivid/vivid-core.c
+++ b/drivers/media/test-drivers/vivid/vivid-core.c
@@ -169,6 +169,14 @@ MODULE_PARM_DESC(allocators, " memory allocator selection, default is 0.\n"
"\t\t 0 == vmalloc\n"
"\t\t 1 == dma-contig");
+static unsigned int cache_hints[VIVID_MAX_DEVS] = {
+ [0 ... (VIVID_MAX_DEVS - 1)] = 0
+};
+module_param_array(cache_hints, uint, NULL, 0444);
+MODULE_PARM_DESC(cache_hints, " user-space cache hints, default is 0.\n"
+ "\t\t 0 == forbid\n"
+ "\t\t 1 == allow");
+
static struct vivid_dev *vivid_devs[VIVID_MAX_DEVS];
const struct v4l2_rect vivid_min_rect = {
@@ -819,6 +827,7 @@ static int vivid_create_queue(struct vivid_dev *dev,
q->lock = &dev->mutex;
q->dev = dev->v4l2_dev.dev;
q->supports_requests = true;
+ q->allow_cache_hints = (cache_hints[dev->inst] == 1);
return vb2_queue_init(q);
}
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
Provide begin_cpu_access() and end_cpu_access() callbacks for
cache synchronisation on exported buffers.
The patch also adds a new helper function - vb2_dc_buffer_consistent(),
which returns true is if the buffer is consistent (DMA_ATTR_NON_CONSISTENT
bit cleared), so then we don't need to sync anything.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
.../common/videobuf2/videobuf2-dma-contig.c | 35 +++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index d3a3ee5b597b..6787e2cb905e 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -42,6 +42,11 @@ struct vb2_dc_buf {
struct dma_buf_attachment *db_attach;
};
+static inline bool vb2_dc_buffer_consistent(unsigned long attr)
+{
+ return !(attr & DMA_ATTR_NON_CONSISTENT);
+}
+
/*********************************************/
/* scatterlist table functions */
/*********************************************/
@@ -335,6 +340,34 @@ static void vb2_dc_dmabuf_ops_release(struct dma_buf *dbuf)
vb2_dc_put(dbuf->priv);
}
+static int
+vb2_dc_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf,
+ enum dma_data_direction direction)
+{
+ struct vb2_dc_buf *buf = dbuf->priv;
+ struct sg_table *sgt = buf->dma_sgt;
+
+ if (vb2_dc_buffer_consistent(buf->attrs))
+ return 0;
+
+ dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
+ return 0;
+}
+
+static int
+vb2_dc_dmabuf_ops_end_cpu_access(struct dma_buf *dbuf,
+ enum dma_data_direction direction)
+{
+ struct vb2_dc_buf *buf = dbuf->priv;
+ struct sg_table *sgt = buf->dma_sgt;
+
+ if (vb2_dc_buffer_consistent(buf->attrs))
+ return 0;
+
+ dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
+ return 0;
+}
+
static void *vb2_dc_dmabuf_ops_vmap(struct dma_buf *dbuf)
{
struct vb2_dc_buf *buf = dbuf->priv;
@@ -353,6 +386,8 @@ static const struct dma_buf_ops vb2_dc_dmabuf_ops = {
.detach = vb2_dc_dmabuf_ops_detach,
.map_dma_buf = vb2_dc_dmabuf_ops_map,
.unmap_dma_buf = vb2_dc_dmabuf_ops_unmap,
+ .begin_cpu_access = vb2_dc_dmabuf_ops_begin_cpu_access,
+ .end_cpu_access = vb2_dc_dmabuf_ops_end_cpu_access,
.vmap = vb2_dc_dmabuf_ops_vmap,
.mmap = vb2_dc_dmabuf_ops_mmap,
.release = vb2_dc_dmabuf_ops_release,
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
We moved cache management decision making to the upper layer and
rely on buffer's need_cache_sync flags and videobuf2 core. If the
upper layer (core) has decided to invoke ->prepare() or ->finish()
then we must sync.
For DMABUF ->need_cache_sync_on_prepare and ->need_cache_sync_on_flush
are always false so videobuf core does not call ->prepare() and
->finish() on such buffers.
Additionally, scratch the DMABUF comment.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
drivers/media/common/videobuf2/videobuf2-dma-contig.c | 6 ++----
drivers/media/common/videobuf2/videobuf2-dma-sg.c | 8 --------
2 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index 6787e2cb905e..5a3e1c3b556f 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -100,8 +100,7 @@ static void vb2_dc_prepare(void *buf_priv)
struct vb2_dc_buf *buf = buf_priv;
struct sg_table *sgt = buf->dma_sgt;
- /* DMABUF exporter will flush the cache for us */
- if (!sgt || buf->db_attach)
+ if (!sgt)
return;
dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,
@@ -113,8 +112,7 @@ static void vb2_dc_finish(void *buf_priv)
struct vb2_dc_buf *buf = buf_priv;
struct sg_table *sgt = buf->dma_sgt;
- /* DMABUF exporter will flush the cache for us */
- if (!sgt || buf->db_attach)
+ if (!sgt)
return;
dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 595137e358e7..0a40e00f0d7e 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -204,10 +204,6 @@ static void vb2_dma_sg_prepare(void *buf_priv)
struct vb2_dma_sg_buf *buf = buf_priv;
struct sg_table *sgt = buf->dma_sgt;
- /* DMABUF exporter will flush the cache for us */
- if (buf->db_attach)
- return;
-
dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,
buf->dma_dir);
}
@@ -217,10 +213,6 @@ static void vb2_dma_sg_finish(void *buf_priv)
struct vb2_dma_sg_buf *buf = buf_priv;
struct sg_table *sgt = buf->dma_sgt;
- /* DMABUF exporter will flush the cache for us */
- if (buf->db_attach)
- return;
-
dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
}
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
Provide begin_cpu_access() and end_cpu_access() dma_buf_ops
callbacks for cache synchronisation on exported buffers.
V4L2_FLAG_MEMORY_NON_CONSISTENT has no effect on dma-sg buffers.
dma-sg allocates memory using the page allocator directly, so
there is no memory consistency guarantee.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
.../media/common/videobuf2/videobuf2-dma-sg.c | 30 +++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 92072a08af25..595137e358e7 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -120,6 +120,12 @@ static void *vb2_dma_sg_alloc(struct device *dev, unsigned long dma_attrs,
buf->num_pages = size >> PAGE_SHIFT;
buf->dma_sgt = &buf->sg_table;
+ /*
+ * NOTE: dma-sg allocates memory using the page allocator directly, so
+ * there is no memory consistency guarantee, hence dma-sg ignores DMA
+ * attributes passed from the upper layer. That means that
+ * V4L2_FLAG_MEMORY_NON_CONSISTENT has no effect on dma-sg buffers.
+ */
buf->pages = kvmalloc_array(buf->num_pages, sizeof(struct page *),
GFP_KERNEL | __GFP_ZERO);
if (!buf->pages)
@@ -469,6 +475,28 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf)
vb2_dma_sg_put(dbuf->priv);
}
+static int
+vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf,
+ enum dma_data_direction direction)
+{
+ struct vb2_dma_sg_buf *buf = dbuf->priv;
+ struct sg_table *sgt = buf->dma_sgt;
+
+ dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
+ return 0;
+}
+
+static int
+vb2_dma_sg_dmabuf_ops_end_cpu_access(struct dma_buf *dbuf,
+ enum dma_data_direction direction)
+{
+ struct vb2_dma_sg_buf *buf = dbuf->priv;
+ struct sg_table *sgt = buf->dma_sgt;
+
+ dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->nents, buf->dma_dir);
+ return 0;
+}
+
static void *vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf)
{
struct vb2_dma_sg_buf *buf = dbuf->priv;
@@ -487,6 +515,8 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = {
.detach = vb2_dma_sg_dmabuf_ops_detach,
.map_dma_buf = vb2_dma_sg_dmabuf_ops_map,
.unmap_dma_buf = vb2_dma_sg_dmabuf_ops_unmap,
+ .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access,
+ .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access,
.vmap = vb2_dma_sg_dmabuf_ops_vmap,
.mmap = vb2_dma_sg_dmabuf_ops_mmap,
.release = vb2_dma_sg_dmabuf_ops_release,
--
2.26.2
From: Sergey Senozhatsky <[email protected]>
Set video buffer cache management flags corresponding to V4L2 cache
flags.
Both ->prepare() and ->finish() cache management hints should be
passed during this stage (buffer preparation), because there is
no other way for user-space to tell V4L2 to avoid ->finish() cache
flush.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
.../media/common/videobuf2/videobuf2-v4l2.c | 48 +++++++++++++++++++
include/media/videobuf2-core.h | 11 +++++
2 files changed, 59 insertions(+)
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index eb5d5db96552..f13851212cc8 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -337,6 +337,53 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
return 0;
}
+static void set_buffer_cache_hints(struct vb2_queue *q,
+ struct vb2_buffer *vb,
+ struct v4l2_buffer *b)
+{
+ /*
+ * DMA exporter should take care of cache syncs, so we can avoid
+ * explicit ->prepare()/->finish() syncs. For other ->memory types
+ * we always need ->prepare() or/and ->finish() cache sync.
+ */
+ if (q->memory == VB2_MEMORY_DMABUF) {
+ vb->need_cache_sync_on_finish = 0;
+ vb->need_cache_sync_on_prepare = 0;
+ return;
+ }
+
+ /*
+ * Cache sync/invalidation flags are set by default in order to
+ * preserve existing behaviour for old apps/drivers.
+ */
+ vb->need_cache_sync_on_prepare = 1;
+ vb->need_cache_sync_on_finish = 1;
+
+ if (!vb2_queue_allows_cache_hints(q)) {
+ /*
+ * Clear buffer cache flags if queue does not support user
+ * space hints. That's to indicate to userspace that these
+ * flags won't work.
+ */
+ b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
+ b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
+ return;
+ }
+
+ /*
+ * ->finish() cache sync can be avoided when queue direction is
+ * TO_DEVICE.
+ */
+ if (q->dma_dir == DMA_TO_DEVICE)
+ vb->need_cache_sync_on_finish = 0;
+
+ if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE)
+ vb->need_cache_sync_on_finish = 0;
+
+ if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN)
+ vb->need_cache_sync_on_prepare = 0;
+}
+
static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
struct v4l2_buffer *b, bool is_prepare,
struct media_request **p_req)
@@ -381,6 +428,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
}
if (!vb->prepared) {
+ set_buffer_cache_hints(q, vb, b);
/* Copy relevant information provided by the userspace */
memset(vbuf->planes, 0,
sizeof(vbuf->planes[0]) * vb->num_planes);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 7f39d9fffc8c..ccc5c498d3e3 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -635,6 +635,17 @@ struct vb2_queue {
#endif
};
+/**
+ * vb2_queue_allows_cache_hints() - Return true if the queue allows cache
+ * and memory consistency hints.
+ *
+ * @q: pointer to &struct vb2_queue with videobuf2 queue
+ */
+static inline bool vb2_queue_allows_cache_hints(struct vb2_queue *q)
+{
+ return q->allow_cache_hints && q->memory == VB2_MEMORY_MMAP;
+}
+
/**
* vb2_plane_vaddr() - Return a kernel virtual address of a given plane.
* @vb: pointer to &struct vb2_buffer to which the plane in
--
2.26.2
Hi Sergey,
On 14/05/2020 18:01, Sergey Senozhatsky wrote:
> Hello
>
> v6 changes:
> The design has been slightly reworked. The cache-hints capability has
> been renamed to SUPPORTS_MMAP_CACHE_HINTS and is reported for all queues
> that support MMAP and allow cache hints. However, the actual hints and
> memory consistency are ignored unless the queue is used for the MMAP
> streaming I/O. Plus some cleanups, documentation updates, and so on.
This looks good. If there are no new comments then I plan to make a PR for 5.9 in
two weeks.
Thank you for all your work on this!
Regards,
Hans
>
> Previous versions:
> v5 link: https://lore.kernel.org/lkml/[email protected]
> v4 link: https://lore.kernel.org/lkml/[email protected]/
> v3 link: https://lore.kernel.org/lkml/[email protected]
> v2 link: https://lore.kernel.org/lkml/[email protected]/
> v1 link: https://lore.kernel.org/lkml/[email protected]/
>
> Series Intro
> ========================================================================
>
> This is a reworked version of the vb2 cache hints
> (V4L2_BUF_FLAG_NO_CACHE_INVALIDATE / V4L2_BUF_FLAG_NO_CACHE_CLEAN)
> support patch series which previsouly was developed by Sakari and
> Laurent [0].
>
> The patch set attempts to preserve the existing behvaiour - cache
> sync is performed in ->prepare() and ->finish() (unless the buffer
> is DMA exported). User space can request “default behavior” override
> with cache management hints, which are handled on a per-buffer basis
> and should be supplied with v4l2_buffer ->flags during buffer
> preparation. There are two possible hints:
>
> - V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
> No cache sync on ->finish()
>
> - V4L2_BUF_FLAG_NO_CACHE_CLEAN
> No cache sync on ->prepare()
>
> In order to keep things on the safe side, we also require driver
> to explicitly state which of its queues (if any) support user space
> cache management hints (such queues should have ->allow_cache_hints
> bit set).
>
> The patch set also (to some extent) simplifies allocators' ->prepare()
> and ->finish() callbacks. Namely, we move cache management decision
> making to the upper - core - layer. For example, if, previously, we
> would have something like this
>
> vb2_buffer_done()
> vb2_dc_finish()
> if (buf->db_attach)
> return;
>
> where each allocators' ->finish() callback would either bail
> out (DMA exported buffer, for instance) or sync, now that "bail
> out or sync" decision is made before we call into the allocator.
>
> Along with cache management hints, user space is also able to
> adjust queue's memory consistency attributes. Memory consistency
> attribute (dma_attrs) is per-queue, yet it plays its role on the
> allocator level, when we allocate buffers’ private memory (planes).
> For the time being, only one consistency attribute is supported:
> DMA_ATTR_NON_CONSISTENT.
>
> [0] https://www.mail-archive.com/[email protected]/msg112459.html
>
> Sergey Senozhatsky (14):
> videobuf2: use explicit unsigned int in vb2_queue
> videobuf2: add cache management members
> videobuf2: handle V4L2 buffer cache flags
> videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag
> videobuf2: add queue memory consistency parameter
> videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT flag
> videobuf2: factor out planes prepare/finish functions
> videobuf2: do not sync caches when we are allowed not to
> videobuf2: check ->synced flag in prepare() and finish()
> videobuf2: add begin/end cpu_access callbacks to dma-contig
> videobuf2: add begin/end cpu_access callbacks to dma-sg
> videobuf2: don't test db_attach in dma-contig prepare and finish
> videobuf2: remove redundant if-statement
> media: vivid: add cache_hints module param
>
> Documentation/admin-guide/media/vivid.rst | 9 ++
> .../userspace-api/media/v4l/buffer.rst | 40 +++++-
> .../media/v4l/vidioc-create-bufs.rst | 7 +-
> .../media/v4l/vidioc-reqbufs.rst | 21 ++-
> .../media/common/videobuf2/videobuf2-core.c | 121 +++++++++++++-----
> .../common/videobuf2/videobuf2-dma-contig.c | 44 ++++++-
> .../media/common/videobuf2/videobuf2-dma-sg.c | 38 ++++--
> .../media/common/videobuf2/videobuf2-v4l2.c | 72 ++++++++++-
> drivers/media/dvb-core/dvb_vb2.c | 2 +-
> drivers/media/test-drivers/vivid/vivid-core.c | 9 ++
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 10 +-
> drivers/media/v4l2-core/v4l2-ioctl.c | 5 +-
> include/media/videobuf2-core.h | 47 +++++--
> include/uapi/linux/videodev2.h | 14 +-
> 14 files changed, 366 insertions(+), 73 deletions(-)
>
Hi Hans,
On (20/05/18 17:18), Hans Verkuil wrote:
> Hi Sergey,
>
> On 14/05/2020 18:01, Sergey Senozhatsky wrote:
> > Hello
> >
> > v6 changes:
> > The design has been slightly reworked. The cache-hints capability has
> > been renamed to SUPPORTS_MMAP_CACHE_HINTS and is reported for all queues
> > that support MMAP and allow cache hints. However, the actual hints and
> > memory consistency are ignored unless the queue is used for the MMAP
> > streaming I/O. Plus some cleanups, documentation updates, and so on.
>
> This looks good. If there are no new comments then I plan to make a PR for 5.9 in
> two weeks.
>
> Thank you for all your work on this!
Hans, Tomasz, Ezequiel, thanks for all the help and guidance.
-ss
Hi Sergey,
While doing final testing for this patch series (together with the v4l-utils patch)
I found one remaining issue:
On 14/05/2020 18:01, Sergey Senozhatsky wrote:
> From: Sergey Senozhatsky <[email protected]>
>
> Set video buffer cache management flags corresponding to V4L2 cache
> flags.
>
> Both ->prepare() and ->finish() cache management hints should be
> passed during this stage (buffer preparation), because there is
> no other way for user-space to tell V4L2 to avoid ->finish() cache
> flush.
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
> ---
> .../media/common/videobuf2/videobuf2-v4l2.c | 48 +++++++++++++++++++
> include/media/videobuf2-core.h | 11 +++++
> 2 files changed, 59 insertions(+)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index eb5d5db96552..f13851212cc8 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -337,6 +337,53 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
> return 0;
> }
>
> +static void set_buffer_cache_hints(struct vb2_queue *q,
> + struct vb2_buffer *vb,
> + struct v4l2_buffer *b)
> +{
> + /*
> + * DMA exporter should take care of cache syncs, so we can avoid
> + * explicit ->prepare()/->finish() syncs. For other ->memory types
> + * we always need ->prepare() or/and ->finish() cache sync.
> + */
> + if (q->memory == VB2_MEMORY_DMABUF) {
> + vb->need_cache_sync_on_finish = 0;
> + vb->need_cache_sync_on_prepare = 0;
> + return;
> + }
> +
> + /*
> + * Cache sync/invalidation flags are set by default in order to
> + * preserve existing behaviour for old apps/drivers.
> + */
> + vb->need_cache_sync_on_prepare = 1;
> + vb->need_cache_sync_on_finish = 1;
> +
> + if (!vb2_queue_allows_cache_hints(q)) {
> + /*
> + * Clear buffer cache flags if queue does not support user
> + * space hints. That's to indicate to userspace that these
> + * flags won't work.
> + */
> + b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
> + b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
> + return;
> + }
These two flags need to be cleared for VB2_MEMORY_DMABUF as well in the test above.
This bug is causing v4l2-compliance failures (use the test-media script in contrib/test
in v4l-utils: 'sudo test-media vim2m').
It's enough to post a v6.1 for this patch, everything else is fine.
Regards,
Hans
> +
> + /*
> + * ->finish() cache sync can be avoided when queue direction is
> + * TO_DEVICE.
> + */
> + if (q->dma_dir == DMA_TO_DEVICE)
> + vb->need_cache_sync_on_finish = 0;
> +
> + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE)
> + vb->need_cache_sync_on_finish = 0;
> +
> + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN)
> + vb->need_cache_sync_on_prepare = 0;
> +}
> +
> static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
> struct v4l2_buffer *b, bool is_prepare,
> struct media_request **p_req)
> @@ -381,6 +428,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
> }
>
> if (!vb->prepared) {
> + set_buffer_cache_hints(q, vb, b);
> /* Copy relevant information provided by the userspace */
> memset(vbuf->planes, 0,
> sizeof(vbuf->planes[0]) * vb->num_planes);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 7f39d9fffc8c..ccc5c498d3e3 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -635,6 +635,17 @@ struct vb2_queue {
> #endif
> };
>
> +/**
> + * vb2_queue_allows_cache_hints() - Return true if the queue allows cache
> + * and memory consistency hints.
> + *
> + * @q: pointer to &struct vb2_queue with videobuf2 queue
> + */
> +static inline bool vb2_queue_allows_cache_hints(struct vb2_queue *q)
> +{
> + return q->allow_cache_hints && q->memory == VB2_MEMORY_MMAP;
> +}
> +
> /**
> * vb2_plane_vaddr() - Return a kernel virtual address of a given plane.
> * @vb: pointer to &struct vb2_buffer to which the plane in
>
Hi Hans,
On (20/06/02 11:51), Hans Verkuil wrote:
> Hi Sergey,
>
> While doing final testing for this patch series (together with the v4l-utils patch)
> I found one remaining issue:
Thanks for the report.
> > +static void set_buffer_cache_hints(struct vb2_queue *q,
> > + struct vb2_buffer *vb,
> > + struct v4l2_buffer *b)
> > +{
> > + /*
> > + * DMA exporter should take care of cache syncs, so we can avoid
> > + * explicit ->prepare()/->finish() syncs. For other ->memory types
> > + * we always need ->prepare() or/and ->finish() cache sync.
> > + */
> > + if (q->memory == VB2_MEMORY_DMABUF) {
> > + vb->need_cache_sync_on_finish = 0;
> > + vb->need_cache_sync_on_prepare = 0;
> > + return;
> > + }
> > +
> > + /*
> > + * Cache sync/invalidation flags are set by default in order to
> > + * preserve existing behaviour for old apps/drivers.
> > + */
> > + vb->need_cache_sync_on_prepare = 1;
> > + vb->need_cache_sync_on_finish = 1;
> > +
> > + if (!vb2_queue_allows_cache_hints(q)) {
> > + /*
> > + * Clear buffer cache flags if queue does not support user
> > + * space hints. That's to indicate to userspace that these
> > + * flags won't work.
> > + */
> > + b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
> > + b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
> > + return;
> > + }
>
> These two flags need to be cleared for VB2_MEMORY_DMABUF as well in the test above.
> This bug is causing v4l2-compliance failures (use the test-media script in contrib/test
> in v4l-utils: 'sudo test-media vim2m').
Sorry, Hans, do you suggest to have something like this:
if (q->memory == VB2_MEMORY_DMABUF) {
vb->need_cache_sync_on_finish = 0;
vb->need_cache_sync_on_prepare = 0;
b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
return;
}
I didn't clear the ->flags there because we clear the vb flush/sync
flags: ->need_cache_sync_on_finish/prepare are zeros for DMABUF memory
type. Which is equivalent to passing V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
V4L2_BUF_FLAG_NO_CACHE_CLEAN. IOW we would clearing both "vb's do cache
sync" and request's "do not cache sync".
> It's enough to post a v6.1 for this patch, everything else is fine.
Thanks!
-ss
On 02/06/2020 12:18, Sergey Senozhatsky wrote:
> Hi Hans,
>
> On (20/06/02 11:51), Hans Verkuil wrote:
>> Hi Sergey,
>>
>> While doing final testing for this patch series (together with the v4l-utils patch)
>> I found one remaining issue:
>
> Thanks for the report.
>
>>> +static void set_buffer_cache_hints(struct vb2_queue *q,
>>> + struct vb2_buffer *vb,
>>> + struct v4l2_buffer *b)
>>> +{
>>> + /*
>>> + * DMA exporter should take care of cache syncs, so we can avoid
>>> + * explicit ->prepare()/->finish() syncs. For other ->memory types
>>> + * we always need ->prepare() or/and ->finish() cache sync.
>>> + */
>>> + if (q->memory == VB2_MEMORY_DMABUF) {
>>> + vb->need_cache_sync_on_finish = 0;
>>> + vb->need_cache_sync_on_prepare = 0;
>>> + return;
>>> + }
>>> +
>>> + /*
>>> + * Cache sync/invalidation flags are set by default in order to
>>> + * preserve existing behaviour for old apps/drivers.
>>> + */
>>> + vb->need_cache_sync_on_prepare = 1;
>>> + vb->need_cache_sync_on_finish = 1;
>>> +
>>> + if (!vb2_queue_allows_cache_hints(q)) {
>>> + /*
>>> + * Clear buffer cache flags if queue does not support user
>>> + * space hints. That's to indicate to userspace that these
>>> + * flags won't work.
>>> + */
>>> + b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
>>> + b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
>>> + return;
>>> + }
>>
>> These two flags need to be cleared for VB2_MEMORY_DMABUF as well in the test above.
>> This bug is causing v4l2-compliance failures (use the test-media script in contrib/test
>> in v4l-utils: 'sudo test-media vim2m').
>
> Sorry, Hans, do you suggest to have something like this:
>
> if (q->memory == VB2_MEMORY_DMABUF) {
> vb->need_cache_sync_on_finish = 0;
> vb->need_cache_sync_on_prepare = 0;
> b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
> b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
> return;
> }
>
> I didn't clear the ->flags there because we clear the vb flush/sync
> flags: ->need_cache_sync_on_finish/prepare are zeros for DMABUF memory
> type. Which is equivalent to passing V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
> V4L2_BUF_FLAG_NO_CACHE_CLEAN. IOW we would clearing both "vb's do cache
> sync" and request's "do not cache sync".
Ah, yes. In that case the v4l-utils patch is likely wrong. Can you take a
look at that patch?
In any case, *something* is wrong.
Regards,
Hans
>
>> It's enough to post a v6.1 for this patch, everything else is fine.
>
> Thanks!
>
> -ss
>
On (20/06/02 12:27), Hans Verkuil wrote:
[..]
> > Sorry, Hans, do you suggest to have something like this:
> >
> > if (q->memory == VB2_MEMORY_DMABUF) {
> > vb->need_cache_sync_on_finish = 0;
> > vb->need_cache_sync_on_prepare = 0;
> > b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
> > b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
> > return;
> > }
> >
> > I didn't clear the ->flags there because we clear the vb flush/sync
> > flags: ->need_cache_sync_on_finish/prepare are zeros for DMABUF memory
> > type. Which is equivalent to passing V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
> > V4L2_BUF_FLAG_NO_CACHE_CLEAN. IOW we would clearing both "vb's do cache
> > sync" and request's "do not cache sync".
>
> Ah, yes. In that case the v4l-utils patch is likely wrong.
> Can you take a look at that patch?
Hans, are we talking about "v4l2-utils: test cache_hints for MMAP queues"
patch? I can take a look, yes.
-ss
On 02/06/2020 14:10, Sergey Senozhatsky wrote:
> On (20/06/02 12:27), Hans Verkuil wrote:
> [..]
>>> Sorry, Hans, do you suggest to have something like this:
>>>
>>> if (q->memory == VB2_MEMORY_DMABUF) {
>>> vb->need_cache_sync_on_finish = 0;
>>> vb->need_cache_sync_on_prepare = 0;
>>> b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
>>> b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
>>> return;
>>> }
>>>
>>> I didn't clear the ->flags there because we clear the vb flush/sync
>>> flags: ->need_cache_sync_on_finish/prepare are zeros for DMABUF memory
>>> type. Which is equivalent to passing V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
>>> V4L2_BUF_FLAG_NO_CACHE_CLEAN. IOW we would clearing both "vb's do cache
>>> sync" and request's "do not cache sync".
>>
>> Ah, yes. In that case the v4l-utils patch is likely wrong.
>> Can you take a look at that patch?
>
> Hans, are we talking about "v4l2-utils: test cache_hints for MMAP queues"
> patch? I can take a look, yes.
Yes, that's the one.
Hans
On 02/06/2020 14:22, Tomasz Figa wrote:
> Hi Hans,
>
> On Tue, Jun 2, 2020 at 11:51 AM Hans Verkuil <[email protected]> wrote:
>>
>> Hi Sergey,
>>
>> While doing final testing for this patch series (together with the v4l-utils patch)
>> I found one remaining issue:
>>
>> On 14/05/2020 18:01, Sergey Senozhatsky wrote:
>>> From: Sergey Senozhatsky <[email protected]>
>>>
>>> Set video buffer cache management flags corresponding to V4L2 cache
>>> flags.
>>>
>>> Both ->prepare() and ->finish() cache management hints should be
>>> passed during this stage (buffer preparation), because there is
>>> no other way for user-space to tell V4L2 to avoid ->finish() cache
>>> flush.
>>>
>>> Signed-off-by: Sergey Senozhatsky <[email protected]>
>>> ---
>>> .../media/common/videobuf2/videobuf2-v4l2.c | 48 +++++++++++++++++++
>>> include/media/videobuf2-core.h | 11 +++++
>>> 2 files changed, 59 insertions(+)
>>>
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> index eb5d5db96552..f13851212cc8 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> @@ -337,6 +337,53 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
>>> return 0;
>>> }
>>>
>>> +static void set_buffer_cache_hints(struct vb2_queue *q,
>>> + struct vb2_buffer *vb,
>>> + struct v4l2_buffer *b)
>>> +{
>>> + /*
>>> + * DMA exporter should take care of cache syncs, so we can avoid
>>> + * explicit ->prepare()/->finish() syncs. For other ->memory types
>>> + * we always need ->prepare() or/and ->finish() cache sync.
>>> + */
>>> + if (q->memory == VB2_MEMORY_DMABUF) {
>>> + vb->need_cache_sync_on_finish = 0;
>>> + vb->need_cache_sync_on_prepare = 0;
>>> + return;
>>> + }
>>> +
>>> + /*
>>> + * Cache sync/invalidation flags are set by default in order to
>>> + * preserve existing behaviour for old apps/drivers.
>>> + */
>>> + vb->need_cache_sync_on_prepare = 1;
>>> + vb->need_cache_sync_on_finish = 1;
>>> +
>>> + if (!vb2_queue_allows_cache_hints(q)) {
>>> + /*
>>> + * Clear buffer cache flags if queue does not support user
>>> + * space hints. That's to indicate to userspace that these
>>> + * flags won't work.
>>> + */
>>> + b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
>>> + b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
>>> + return;
>>> + }
>>
>> These two flags need to be cleared for VB2_MEMORY_DMABUF as well in the test above.
>> This bug is causing v4l2-compliance failures (use the test-media script in contrib/test
>> in v4l-utils: 'sudo test-media vim2m').
>
> Would you be able to paste the failures, so that we know that we
> reproduce the same problems? Thanks!
For vim2m (but looks the same for vivid/vimc/vicodec):
Streaming ioctls:
test read/write: OK (Not Supported)
test blocking wait: OK
Video Capture: Captured 8 buffers
test MMAP (no poll): OK
Video Capture: Captured 8 buffers
test MMAP (select): OK
Video Capture: Captured 8 buffers
test MMAP (epoll): OK
Video Capture: Captured 8 buffers
test USERPTR (no poll): OK
Video Capture: Captured 8 buffers
test USERPTR (select): OK
fail: v4l2-test-buffers.cpp(1874): flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
fail: v4l2-test-buffers.cpp(1937): setupDmaBuf(expbuf_node, node, q, exp_q)
test DMABUF (no poll): FAIL
fail: v4l2-test-buffers.cpp(1874): flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
fail: v4l2-test-buffers.cpp(1937): setupDmaBuf(expbuf_node, node, q, exp_q)
test DMABUF (select): FAIL
Regards,
Hans
>
> Best regards,
> Tomasz
>
>>
>> It's enough to post a v6.1 for this patch, everything else is fine.
>>
>> Regards,
>>
>> Hans
>>
>>> +
>>> + /*
>>> + * ->finish() cache sync can be avoided when queue direction is
>>> + * TO_DEVICE.
>>> + */
>>> + if (q->dma_dir == DMA_TO_DEVICE)
>>> + vb->need_cache_sync_on_finish = 0;
>>> +
>>> + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE)
>>> + vb->need_cache_sync_on_finish = 0;
>>> +
>>> + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN)
>>> + vb->need_cache_sync_on_prepare = 0;
>>> +}
>>> +
>>> static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
>>> struct v4l2_buffer *b, bool is_prepare,
>>> struct media_request **p_req)
>>> @@ -381,6 +428,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
>>> }
>>>
>>> if (!vb->prepared) {
>>> + set_buffer_cache_hints(q, vb, b);
>>> /* Copy relevant information provided by the userspace */
>>> memset(vbuf->planes, 0,
>>> sizeof(vbuf->planes[0]) * vb->num_planes);
>>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>>> index 7f39d9fffc8c..ccc5c498d3e3 100644
>>> --- a/include/media/videobuf2-core.h
>>> +++ b/include/media/videobuf2-core.h
>>> @@ -635,6 +635,17 @@ struct vb2_queue {
>>> #endif
>>> };
>>>
>>> +/**
>>> + * vb2_queue_allows_cache_hints() - Return true if the queue allows cache
>>> + * and memory consistency hints.
>>> + *
>>> + * @q: pointer to &struct vb2_queue with videobuf2 queue
>>> + */
>>> +static inline bool vb2_queue_allows_cache_hints(struct vb2_queue *q)
>>> +{
>>> + return q->allow_cache_hints && q->memory == VB2_MEMORY_MMAP;
>>> +}
>>> +
>>> /**
>>> * vb2_plane_vaddr() - Return a kernel virtual address of a given plane.
>>> * @vb: pointer to &struct vb2_buffer to which the plane in
>>>
>>
Hi Hans,
On Tue, Jun 2, 2020 at 11:51 AM Hans Verkuil <[email protected]> wrote:
>
> Hi Sergey,
>
> While doing final testing for this patch series (together with the v4l-utils patch)
> I found one remaining issue:
>
> On 14/05/2020 18:01, Sergey Senozhatsky wrote:
> > From: Sergey Senozhatsky <[email protected]>
> >
> > Set video buffer cache management flags corresponding to V4L2 cache
> > flags.
> >
> > Both ->prepare() and ->finish() cache management hints should be
> > passed during this stage (buffer preparation), because there is
> > no other way for user-space to tell V4L2 to avoid ->finish() cache
> > flush.
> >
> > Signed-off-by: Sergey Senozhatsky <[email protected]>
> > ---
> > .../media/common/videobuf2/videobuf2-v4l2.c | 48 +++++++++++++++++++
> > include/media/videobuf2-core.h | 11 +++++
> > 2 files changed, 59 insertions(+)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > index eb5d5db96552..f13851212cc8 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > @@ -337,6 +337,53 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
> > return 0;
> > }
> >
> > +static void set_buffer_cache_hints(struct vb2_queue *q,
> > + struct vb2_buffer *vb,
> > + struct v4l2_buffer *b)
> > +{
> > + /*
> > + * DMA exporter should take care of cache syncs, so we can avoid
> > + * explicit ->prepare()/->finish() syncs. For other ->memory types
> > + * we always need ->prepare() or/and ->finish() cache sync.
> > + */
> > + if (q->memory == VB2_MEMORY_DMABUF) {
> > + vb->need_cache_sync_on_finish = 0;
> > + vb->need_cache_sync_on_prepare = 0;
> > + return;
> > + }
> > +
> > + /*
> > + * Cache sync/invalidation flags are set by default in order to
> > + * preserve existing behaviour for old apps/drivers.
> > + */
> > + vb->need_cache_sync_on_prepare = 1;
> > + vb->need_cache_sync_on_finish = 1;
> > +
> > + if (!vb2_queue_allows_cache_hints(q)) {
> > + /*
> > + * Clear buffer cache flags if queue does not support user
> > + * space hints. That's to indicate to userspace that these
> > + * flags won't work.
> > + */
> > + b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
> > + b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
> > + return;
> > + }
>
> These two flags need to be cleared for VB2_MEMORY_DMABUF as well in the test above.
> This bug is causing v4l2-compliance failures (use the test-media script in contrib/test
> in v4l-utils: 'sudo test-media vim2m').
Would you be able to paste the failures, so that we know that we
reproduce the same problems? Thanks!
Best regards,
Tomasz
>
> It's enough to post a v6.1 for this patch, everything else is fine.
>
> Regards,
>
> Hans
>
> > +
> > + /*
> > + * ->finish() cache sync can be avoided when queue direction is
> > + * TO_DEVICE.
> > + */
> > + if (q->dma_dir == DMA_TO_DEVICE)
> > + vb->need_cache_sync_on_finish = 0;
> > +
> > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE)
> > + vb->need_cache_sync_on_finish = 0;
> > +
> > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN)
> > + vb->need_cache_sync_on_prepare = 0;
> > +}
> > +
> > static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *mdev,
> > struct v4l2_buffer *b, bool is_prepare,
> > struct media_request **p_req)
> > @@ -381,6 +428,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md
> > }
> >
> > if (!vb->prepared) {
> > + set_buffer_cache_hints(q, vb, b);
> > /* Copy relevant information provided by the userspace */
> > memset(vbuf->planes, 0,
> > sizeof(vbuf->planes[0]) * vb->num_planes);
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index 7f39d9fffc8c..ccc5c498d3e3 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -635,6 +635,17 @@ struct vb2_queue {
> > #endif
> > };
> >
> > +/**
> > + * vb2_queue_allows_cache_hints() - Return true if the queue allows cache
> > + * and memory consistency hints.
> > + *
> > + * @q: pointer to &struct vb2_queue with videobuf2 queue
> > + */
> > +static inline bool vb2_queue_allows_cache_hints(struct vb2_queue *q)
> > +{
> > + return q->allow_cache_hints && q->memory == VB2_MEMORY_MMAP;
> > +}
> > +
> > /**
> > * vb2_plane_vaddr() - Return a kernel virtual address of a given plane.
> > * @vb: pointer to &struct vb2_buffer to which the plane in
> >
>
On (20/06/02 14:24), Hans Verkuil wrote:
[..]
> For vim2m (but looks the same for vivid/vimc/vicodec):
>
> Streaming ioctls:
> test read/write: OK (Not Supported)
> test blocking wait: OK
> Video Capture: Captured 8 buffers
> test MMAP (no poll): OK
> Video Capture: Captured 8 buffers
> test MMAP (select): OK
> Video Capture: Captured 8 buffers
> test MMAP (epoll): OK
> Video Capture: Captured 8 buffers
> test USERPTR (no poll): OK
> Video Capture: Captured 8 buffers
> test USERPTR (select): OK
> fail: v4l2-test-buffers.cpp(1874): flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
> fail: v4l2-test-buffers.cpp(1937): setupDmaBuf(expbuf_node, node, q, exp_q)
> test DMABUF (no poll): FAIL
> fail: v4l2-test-buffers.cpp(1874): flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE
> fail: v4l2-test-buffers.cpp(1937): setupDmaBuf(expbuf_node, node, q, exp_q)
> test DMABUF (select): FAIL
This helps. I'm probably "holding v4l2-compliance wrong", but I have
never seen that assertion triggering. The fix should be easy enough
---
diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp
index 79b74e96..1ee12f96 100644
--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -1871,8 +1871,8 @@ static int setupDmaBuf(struct node *expbuf_node, struct node *node,
fail_on_test(!buf.g_bytesused(p));
}
flags = buf.g_flags();
- fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE);
- fail_on_test(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN);
+ fail_on_test(!(flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE));
+ fail_on_test(!(flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN));
fail_on_test(flags & V4L2_BUF_FLAG_DONE);
fail_on_test(buf.querybuf(node, i));
fail_on_test(buf.check(q, Queued, i));