[PATCHv4 00/11] Implement V4L2_BUF_FLAG_NO_CACHE_* flags

[PATCHv4 00/11] Implement V4L2_BUF_FLAG_NO_CACHE_* flags

[Sergey Senozhatsky]

Hello,

	v4 of the series. Typos and grammar fixes.

Previous series:
v3 link: https://lore.kernel.org/lkml/20200226111529.180197-1-senozhatsky@chromium.org
v2 link: https://lore.kernel.org/lkml/20200204025641.218376-1-senozhatsky@chromium.org/
v1 link: https://lore.kernel.org/lkml/20191217032034.54897-1-senozhatsky@chromium.org/

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/linux-media@vger.kernel.org/msg112459.html

Sergey Senozhatsky (11):
  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

 Documentation/media/uapi/v4l/buffer.rst       |  29 +++++
 .../media/uapi/v4l/vidioc-create-bufs.rst     |   7 +-
 .../media/uapi/v4l/vidioc-reqbufs.rst         |  18 ++-
 .../media/common/videobuf2/videobuf2-core.c   | 110 +++++++++++++-----
 .../common/videobuf2/videobuf2-dma-contig.c   |  39 ++++++-
 .../media/common/videobuf2/videobuf2-dma-sg.c |  36 ++++--
 .../media/common/videobuf2/videobuf2-v4l2.c   |  82 ++++++++++++-
 drivers/media/dvb-core/dvb_vb2.c              |   2 +-
 drivers/media/v4l2-core/v4l2-ioctl.c          |   5 +-
 include/media/videobuf2-core.h                |  28 ++++-
 include/uapi/linux/videodev2.h                |  11 +-
 11 files changed, 310 insertions(+), 57 deletions(-)

-- 
2.25.0.265.gbab2e86ba0-goog

[PATCHv4 01/11] videobuf2: add cache management members

[Sergey Senozhatsky]

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.

Only drivers that set ->allow_cache_hints during queue initialisation
will handle user-space cache management hints. Otherwise hints
will be ignored.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 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 a2b2208b02da..4a19170672ac 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
@@ -564,6 +573,7 @@ struct vb2_queue {
 	unsigned			requires_requests:1;
 	unsigned			uses_qbuf:1;
 	unsigned			uses_requests:1;
+	unsigned			allow_cache_hints:1;
 
 	struct mutex			*lock;
 	void				*owner;
-- 
2.25.0.265.gbab2e86ba0-goog

[PATCHv4 02/11] videobuf2: handle V4L2 buffer cache flags

[Sergey Senozhatsky]

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 <senozhatsky@chromium.org>
---
 .../media/common/videobuf2/videobuf2-v4l2.c   | 49 +++++++++++++++++++
 include/media/videobuf2-core.h                | 11 +++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index eb5d5db96552..2a604bd7793a 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -199,6 +199,15 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
 	vbuf->request_fd = -1;
 	vbuf->is_held = false;
 
+	/*
+	 * Clear buffer cache flags if queue does not support user space hints.
+	 * That's to indicate to userspace that these flags won't work.
+	 */
+	if (!vb2_queue_allows_cache_hints(q)) {
+		b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
+		b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
+	}
+
 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
 		switch (b->memory) {
 		case VB2_MEMORY_USERPTR:
@@ -337,6 +346,45 @@ 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))
+		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 +429,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 4a19170672ac..731fd9fbd506 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -632,6 +632,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.25.0.265.gbab2e86ba0-goog

[PATCHv4 03/11] videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag

[Sergey Senozhatsky]

By setting or clearing V4L2_FLAG_MEMORY_NON_CONSISTENT flag
user-space should be able to set or clear queue's NON_CONSISTENT
->dma_attrs. Queue's ->dma_attrs are passed to the underlying
allocator in __vb2_buf_mem_alloc(), so thus user-space is able
to request vb2 buffer's memory to be either consistent (coherent)
or non-consistent.

The patch set also adds a corresponding capability flag:
fill_buf_caps() reports V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS when
queue supports user-space cache management hints.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 Documentation/media/uapi/v4l/buffer.rst       | 29 +++++++++++++++++++
 .../media/uapi/v4l/vidioc-reqbufs.rst         |  7 +++++
 .../media/common/videobuf2/videobuf2-v4l2.c   |  2 ++
 include/uapi/linux/videodev2.h                |  3 ++
 4 files changed, 41 insertions(+)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index 3112300c2fa0..8084e3f2a58d 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -681,6 +681,35 @@ Buffer Flags
 
     \normalsize
 
+.. _memory-flags:
+
+Memory Consistency Flags
+========================
+
+.. tabularcolumns:: |p{7.0cm}|p{2.2cm}|p{8.3cm}|
+
+.. cssclass:: longtable
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       3 1 4
+
+    * .. _`V4L2_FLAG_MEMORY_NON_CONSISTENT`:
+
+      - ``V4L2_FLAG_MEMORY_NON_CONSISTENT``
+      - 0x00000001
+      - A buffer is allocated either in consistent (it will be automatically
+	coherent between the CPU and the bus) or non-consistent memory. The
+	latter can provide performance gains, for instance the CPU cache
+	sync/flush operations can be avoided if the buffer is accessed by the
+	corresponding device only and the CPU does not read/write to/from that
+	buffer. However, this requires extra care from the driver -- it must
+	guarantee memory consistency by issuing a cache flush/sync when
+	consistency is needed. If this flag is set V4L2 will attempt to
+	allocate the buffer in non-consistent memory. The flag takes effect
+	only if the buffer is used for :ref:`memory mapping <mmap>` I/O and the
+	queue reports the :ref:`V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS` capability.
 
 .. c:type:: v4l2_memory
 
diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
index d0c643db477a..21ecacc72487 100644
--- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
+++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
@@ -156,6 +156,13 @@ aborting or finishing any DMA in progress, an implicit
       - Only valid for stateless decoders. If set, then userspace can set the
         ``V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF`` flag to hold off on returning the
 	capture buffer until the OUTPUT timestamp changes.
+    * - ``V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS``
+      - 0x00000040
+      - Set when the queue/buffer supports memory consistency and cache
+        management hints. See :ref:`V4L2_FLAG_MEMORY_NON_CONSISTENT`,
+        :ref:`V4L2_BUF_FLAG_NO_CACHE_INVALIDATE` and
+        :ref:`V4L2_BUF_FLAG_NO_CACHE_CLEAN`.
+
 
 Return Value
 ============
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 2a604bd7793a..c847bcea6e95 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -711,6 +711,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
 		*caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
 	if (q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)
 		*caps |= V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
+	if (q->allow_cache_hints && (q->io_modes & VB2_MMAP))
+		*caps |= V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS;
 #ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
 	if (q->supports_requests)
 		*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 5f9357dcb060..e92c29864730 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -189,6 +189,8 @@ enum v4l2_memory {
 	V4L2_MEMORY_DMABUF           = 4,
 };
 
+#define V4L2_FLAG_MEMORY_NON_CONSISTENT		(1 << 0)
+
 /* see also http://vektor.theorem.ca/graphics/ycbcr/ */
 enum v4l2_colorspace {
 	/*
@@ -946,6 +948,7 @@ struct v4l2_requestbuffers {
 #define V4L2_BUF_CAP_SUPPORTS_REQUESTS			(1 << 3)
 #define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS		(1 << 4)
 #define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF	(1 << 5)
+#define V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS		(1 << 6)
 
 /**
  * struct v4l2_plane - plane info for multi-planar buffers
-- 
2.25.0.265.gbab2e86ba0-goog

[PATCHv4 04/11] videobuf2: add queue memory consistency parameter

[Sergey Senozhatsky]

Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support.

Extend vb2_core_reqbufs() with queue memory consistency flag
that is applied to the newly allocated buffers.

An attempt to allocate a buffer with consistency requirements
which don't match queue's consistency model will fail.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 .../media/common/videobuf2/videobuf2-core.c   | 47 +++++++++++++++----
 .../media/common/videobuf2/videobuf2-v4l2.c   |  6 +--
 drivers/media/dvb-core/dvb_vb2.c              |  2 +-
 include/media/videobuf2-core.h                |  7 ++-
 4 files changed, 47 insertions(+), 15 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 4489744fbbd9..3ca0545db7ee 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -664,8 +664,19 @@ 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;
+}
+
 int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
-		unsigned int *count)
+		bool consistent_mem, unsigned int *count)
 {
 	unsigned int num_buffers, allocated_buffers, num_planes = 0;
 	unsigned plane_sizes[VB2_MAX_PLANES] = { };
@@ -720,6 +731,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.
@@ -803,9 +815,21 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
 }
 EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
 
+static bool verify_consistency_attr(struct vb2_queue *q, bool consistent_mem)
+{
+	bool queue_attr = q->dma_attrs & DMA_ATTR_NON_CONSISTENT;
+
+	if (consistent_mem != queue_attr) {
+		dprintk(1, "memory consistency model mismatch\n");
+		return false;
+	}
+	return true;
+}
+
 int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
-		unsigned int *count, unsigned requested_planes,
-		const unsigned requested_sizes[])
+			 bool consistent_mem, unsigned int *count,
+			 unsigned requested_planes,
+			 const unsigned requested_sizes[])
 {
 	unsigned int num_planes = 0, num_buffers, allocated_buffers;
 	unsigned plane_sizes[VB2_MAX_PLANES] = { };
@@ -823,10 +847,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 +2527,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, true, &fileio->count);
 	if (ret)
 		goto err_kfree;
 
@@ -2555,7 +2584,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, true, &fileio->count);
 
 err_kfree:
 	q->fileio = NULL;
@@ -2575,7 +2604,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, true, &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 c847bcea6e95..6111d74f68c9 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -724,7 +724,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, true, &req->count);
 }
 EXPORT_SYMBOL_GPL(vb2_reqbufs);
 
@@ -798,7 +798,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, true,
 		&create->count, requested_planes, requested_sizes);
 }
 EXPORT_SYMBOL_GPL(vb2_create_bufs);
@@ -974,7 +974,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, true, &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..e60063652164 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, true, &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 731fd9fbd506..ba83ac754c21 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -737,6 +737,7 @@ 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.
+ * @consistent_mem:	memory consistency model.
  * @count:	requested buffer count.
  *
  * Videobuf2 core helper to implement VIDIOC_REQBUF() operation. It is called
@@ -761,12 +762,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);
+		bool consistent_mem, 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.
+ * @consistent_mem: memory consistency model.
  * @count: requested buffer count.
  * @requested_planes: number of planes requested.
  * @requested_sizes: array with the size of the planes.
@@ -784,7 +786,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,
+			 bool consistent_mem, unsigned int *count,
+			 unsigned int requested_planes,
 			 const unsigned int requested_sizes[]);
 
 /**
-- 
2.25.0.265.gbab2e86ba0-goog

[PATCHv4 05/11] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT flag

[Sergey Senozhatsky]

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.

= 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 <senozhatsky@chromium.org>
---
 .../media/uapi/v4l/vidioc-create-bufs.rst     |  7 ++++-
 .../media/uapi/v4l/vidioc-reqbufs.rst         | 11 +++++--
 .../media/common/videobuf2/videobuf2-v4l2.c   | 31 +++++++++++++++++--
 drivers/media/v4l2-core/v4l2-ioctl.c          |  5 +--
 include/uapi/linux/videodev2.h                |  8 +++--
 5 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
index bd08e4f77ae4..a9c6c89f5098 100644
--- a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
+++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
index 21ecacc72487..faf0df4f9bb6 100644
--- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
+++ b/Documentation/media/uapi/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-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 6111d74f68c9..b4b379f3bf98 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -722,9 +722,18 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
 int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
 {
 	int ret = vb2_verify_memory_type(q, req->memory, req->type);
+	bool consistent = true;
+
+	if (!vb2_queue_allows_cache_hints(q))
+		req->flags &= ~V4L2_FLAG_MEMORY_NON_CONSISTENT;
+
+	if (req->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT)
+		consistent = false;
 
 	fill_buf_caps(q, &req->capabilities);
-	return ret ? ret : vb2_core_reqbufs(q, req->memory, true, &req->count);
+	if (ret)
+		return ret;
+	return vb2_core_reqbufs(q, req->memory, consistent, &req->count);
 }
 EXPORT_SYMBOL_GPL(vb2_reqbufs);
 
@@ -753,6 +762,7 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
 	unsigned requested_sizes[VIDEO_MAX_PLANES];
 	struct v4l2_format *f = &create->format;
 	int ret = vb2_verify_memory_type(q, create->memory, f->type);
+	bool consistent = true;
 	unsigned i;
 
 	fill_buf_caps(q, &create->capabilities);
@@ -798,7 +808,14 @@ 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, true,
+
+	if (!vb2_queue_allows_cache_hints(q))
+		create->flags &= ~V4L2_FLAG_MEMORY_NON_CONSISTENT;
+
+	if (create->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT)
+		consistent = false;
+
+	return ret ? ret : vb2_core_create_bufs(q, create->memory, consistent,
 		&create->count, requested_planes, requested_sizes);
 }
 EXPORT_SYMBOL_GPL(vb2_create_bufs);
@@ -968,13 +985,21 @@ 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);
+	bool consistent = true;
 
 	fill_buf_caps(vdev->queue, &p->capabilities);
 	if (res)
 		return res;
 	if (vb2_queue_is_busy(vdev, file))
 		return -EBUSY;
-	res = vb2_core_reqbufs(vdev->queue, p->memory, true, &p->count);
+
+	if (!vb2_queue_allows_cache_hints(vdev->queue))
+		p->flags &= ~V4L2_FLAG_MEMORY_NON_CONSISTENT;
+
+	if (p->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT)
+		consistent = false;
+
+	res = vb2_core_reqbufs(vdev->queue, p->memory, consistent, &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/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index fbcc7a20eedf..53b87bfd50d7 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1973,9 +1973,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);
 }
 
@@ -2015,7 +2012,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 e92c29864730..12b1bd220347 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -938,7 +938,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 */
@@ -2446,7 +2449,8 @@ struct v4l2_create_buffers {
 	__u32			memory;
 	struct v4l2_format	format;
 	__u32			capabilities;
-	__u32			reserved[7];
+	__u32			flags;
+	__u32			reserved[6];
 };
 
 /*
-- 
2.25.0.265.gbab2e86ba0-goog

[PATCHv4 06/11] videobuf2: factor out planes prepare/finish functions

[Sergey Senozhatsky]

Factor out the code, no functional changes.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 .../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 3ca0545db7ee..c2a1eadb26cf 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.
@@ -951,7 +977,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;
@@ -971,12 +996,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) {
@@ -1301,7 +1322,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) {
@@ -1345,11 +1365,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;
 
@@ -1969,14 +1985,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.25.0.265.gbab2e86ba0-goog

[PATCHv4 07/11] videobuf2: do not sync caches when we are allowed not to

[Sergey Senozhatsky]

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 <senozhatsky@chromium.org>
---
 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 c2a1eadb26cf..988e8796de4f 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.25.0.265.gbab2e86ba0-goog

[PATCHv4 08/11] videobuf2: check ->synced flag in prepare() and finish()

[Sergey Senozhatsky]

This simplifies the code a tiny bit and let's us to avoid
unneeded ->prepare()/->finish() calls.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 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 988e8796de4f..7f637e3a50ab 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,
@@ -1991,8 +1997,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.25.0.265.gbab2e86ba0-goog

[PATCHv4 09/11] videobuf2: add begin/end cpu_access callbacks to dma-contig

[Sergey Senozhatsky]

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 <senozhatsky@chromium.org>
---
 .../common/videobuf2/videobuf2-dma-contig.c   | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index d0c9dffe49e5..a387260fb321 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,32 @@ 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 +384,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.25.0.265.gbab2e86ba0-goog

[PATCHv4 10/11] videobuf2: add begin/end cpu_access callbacks to dma-sg

[Sergey Senozhatsky]

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 <senozhatsky@chromium.org>
---
 .../media/common/videobuf2/videobuf2-dma-sg.c | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 6db60e9d5183..ddc67c9aaedb 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)
@@ -470,6 +476,26 @@ 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;
@@ -488,6 +514,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.25.0.265.gbab2e86ba0-goog

[PATCHv4 11/11] videobuf2: don't test db_attach in dma-contig prepare and finish

[Sergey Senozhatsky]

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 <senozhatsky@chromium.org>
---
 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 a387260fb321..6ea0961149d7 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 ddc67c9aaedb..2a01bc567321 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.25.0.265.gbab2e86ba0-goog

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Hans Verkuil]

On 02/03/2020 05:12, Sergey Senozhatsky wrote:
> 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.
> 
> Only drivers that set ->allow_cache_hints during queue initialisation
> will handle user-space cache management hints. Otherwise hints
> will be ignored.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  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 a2b2208b02da..4a19170672ac 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().

checkpatch complains about a space before a tab in the two lines above.

Regards,

	Hans

>   * @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
> @@ -564,6 +573,7 @@ struct vb2_queue {
>  	unsigned			requires_requests:1;
>  	unsigned			uses_qbuf:1;
>  	unsigned			uses_requests:1;
> +	unsigned			allow_cache_hints:1;
>  
>  	struct mutex			*lock;
>  	void				*owner;
> 

Re: [PATCHv4 03/11] videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag

[Hans Verkuil]

On 02/03/2020 05:12, Sergey Senozhatsky wrote:
> By setting or clearing V4L2_FLAG_MEMORY_NON_CONSISTENT flag
> user-space should be able to set or clear queue's NON_CONSISTENT
> ->dma_attrs. Queue's ->dma_attrs are passed to the underlying
> allocator in __vb2_buf_mem_alloc(), so thus user-space is able
> to request vb2 buffer's memory to be either consistent (coherent)
> or non-consistent.
> 
> The patch set also adds a corresponding capability flag:
> fill_buf_caps() reports V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS when
> queue supports user-space cache management hints.

When building the docs I got a bunch of reference errors. This patch fixes those:

-----------------------------------------------
diff --git b/Documentation/media/uapi/v4l/buffer.rst a/Documentation/media/uapi/v4l/buffer.rst
index 8084e3f2a58d..82bad4dbec16 100644
--- b/Documentation/media/uapi/v4l/buffer.rst
+++ a/Documentation/media/uapi/v4l/buffer.rst
@@ -695,7 +695,7 @@ Memory Consistency Flags
     :stub-columns: 0
     :widths:       3 1 4

-    * .. _`V4L2_FLAG_MEMORY_NON_CONSISTENT`:
+    * .. _`V4L2-FLAG-MEMORY-NON-CONSISTENT`:

       - ``V4L2_FLAG_MEMORY_NON_CONSISTENT``
       - 0x00000001
@@ -709,7 +709,9 @@ Memory Consistency Flags
 	consistency is needed. If this flag is set V4L2 will attempt to
 	allocate the buffer in non-consistent memory. The flag takes effect
 	only if the buffer is used for :ref:`memory mapping <mmap>` I/O and the
-	queue reports the :ref:`V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS` capability.
+	queue reports the
+	:ref:`V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS <V4L2-BUF-CAP-SUPPORTS-CACHE-HINTS>`
+	capability.

 .. c:type:: v4l2_memory

diff --git b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
index faf0df4f9bb6..1c0299424fbc 100644
--- b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
+++ a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
@@ -133,6 +133,7 @@ aborting or finishing any DMA in progress, an implicit
 .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS:
 .. _V4L2-BUF-CAP-SUPPORTS-ORPHANED-BUFS:
 .. _V4L2-BUF-CAP-SUPPORTS-M2M-HOLD-CAPTURE-BUF:
+.. _V4L2-BUF-CAP-SUPPORTS-CACHE-HINTS:

 .. cssclass:: longtable

@@ -166,9 +167,10 @@ aborting or finishing any DMA in progress, an implicit
     * - ``V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS``
       - 0x00000040
       - Set when the queue/buffer supports memory consistency and cache
-        management hints. See :ref:`V4L2_FLAG_MEMORY_NON_CONSISTENT`,
-        :ref:`V4L2_BUF_FLAG_NO_CACHE_INVALIDATE` and
-        :ref:`V4L2_BUF_FLAG_NO_CACHE_CLEAN`.
+        management hints. See
+	:ref:`V4L2_FLAG_MEMORY_NON_CONSISTENT <V4L2-FLAG-MEMORY-NON-CONSISTENT>`,
+        :ref:`V4L2_BUF_FLAG_NO_CACHE_INVALIDATE <V4L2-BUF-FLAG-NO-CACHE-INVALIDATE>` and
+        :ref:`V4L2_BUF_FLAG_NO_CACHE_CLEAN <V4L2-BUF-FLAG-NO-CACHE-CLEAN>`.


 Return Value
-----------------------------------------------

Please fold that in this patch.

Regards,

	Hans

> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  Documentation/media/uapi/v4l/buffer.rst       | 29 +++++++++++++++++++
>  .../media/uapi/v4l/vidioc-reqbufs.rst         |  7 +++++
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  2 ++
>  include/uapi/linux/videodev2.h                |  3 ++
>  4 files changed, 41 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index 3112300c2fa0..8084e3f2a58d 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -681,6 +681,35 @@ Buffer Flags
>  
>      \normalsize
>  
> +.. _memory-flags:
> +
> +Memory Consistency Flags
> +========================
> +
> +.. tabularcolumns:: |p{7.0cm}|p{2.2cm}|p{8.3cm}|
> +
> +.. cssclass:: longtable
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths:       3 1 4
> +
> +    * .. _`V4L2_FLAG_MEMORY_NON_CONSISTENT`:
> +
> +      - ``V4L2_FLAG_MEMORY_NON_CONSISTENT``
> +      - 0x00000001
> +      - A buffer is allocated either in consistent (it will be automatically
> +	coherent between the CPU and the bus) or non-consistent memory. The
> +	latter can provide performance gains, for instance the CPU cache
> +	sync/flush operations can be avoided if the buffer is accessed by the
> +	corresponding device only and the CPU does not read/write to/from that
> +	buffer. However, this requires extra care from the driver -- it must
> +	guarantee memory consistency by issuing a cache flush/sync when
> +	consistency is needed. If this flag is set V4L2 will attempt to
> +	allocate the buffer in non-consistent memory. The flag takes effect
> +	only if the buffer is used for :ref:`memory mapping <mmap>` I/O and the
> +	queue reports the :ref:`V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS` capability.
>  
>  .. c:type:: v4l2_memory
>  
> diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> index d0c643db477a..21ecacc72487 100644
> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> @@ -156,6 +156,13 @@ aborting or finishing any DMA in progress, an implicit
>        - Only valid for stateless decoders. If set, then userspace can set the
>          ``V4L2_BUF_FLAG_M2M_HOLD_CAPTURE_BUF`` flag to hold off on returning the
>  	capture buffer until the OUTPUT timestamp changes.
> +    * - ``V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS``
> +      - 0x00000040
> +      - Set when the queue/buffer supports memory consistency and cache
> +        management hints. See :ref:`V4L2_FLAG_MEMORY_NON_CONSISTENT`,
> +        :ref:`V4L2_BUF_FLAG_NO_CACHE_INVALIDATE` and
> +        :ref:`V4L2_BUF_FLAG_NO_CACHE_CLEAN`.
> +
>  
>  Return Value
>  ============
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 2a604bd7793a..c847bcea6e95 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -711,6 +711,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
>  		*caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
>  	if (q->subsystem_flags & VB2_V4L2_FL_SUPPORTS_M2M_HOLD_CAPTURE_BUF)
>  		*caps |= V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF;
> +	if (q->allow_cache_hints && (q->io_modes & VB2_MMAP))
> +		*caps |= V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS;
>  #ifdef CONFIG_MEDIA_CONTROLLER_REQUEST_API
>  	if (q->supports_requests)
>  		*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 5f9357dcb060..e92c29864730 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -189,6 +189,8 @@ enum v4l2_memory {
>  	V4L2_MEMORY_DMABUF           = 4,
>  };
>  
> +#define V4L2_FLAG_MEMORY_NON_CONSISTENT		(1 << 0)
> +
>  /* see also http://vektor.theorem.ca/graphics/ycbcr/ */
>  enum v4l2_colorspace {
>  	/*
> @@ -946,6 +948,7 @@ struct v4l2_requestbuffers {
>  #define V4L2_BUF_CAP_SUPPORTS_REQUESTS			(1 << 3)
>  #define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS		(1 << 4)
>  #define V4L2_BUF_CAP_SUPPORTS_M2M_HOLD_CAPTURE_BUF	(1 << 5)
> +#define V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS		(1 << 6)
>  
>  /**
>   * struct v4l2_plane - plane info for multi-planar buffers
> 

Re: [PATCHv4 10/11] videobuf2: add begin/end cpu_access callbacks to dma-sg

[Hans Verkuil]

On 02/03/2020 05:12, Sergey Senozhatsky wrote:
> 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 <senozhatsky@chromium.org>
> ---
>  .../media/common/videobuf2/videobuf2-dma-sg.c | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> index 6db60e9d5183..ddc67c9aaedb 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)
> @@ -470,6 +476,26 @@ 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)

I suggest you use this style to avoid checkpatch warnings:

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)

Ditto.

Regards,

	Hans

> +{
> +	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;
> @@ -488,6 +514,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,
> 

Re: [PATCHv4 04/11] videobuf2: add queue memory consistency parameter

[Hans Verkuil]

On 02/03/2020 05:12, Sergey Senozhatsky wrote:
> Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support.
> 
> Extend vb2_core_reqbufs() with queue memory consistency flag
> that is applied to the newly allocated buffers.
> 
> An attempt to allocate a buffer with consistency requirements
> which don't match queue's consistency model will fail.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 47 +++++++++++++++----
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  6 +--
>  drivers/media/dvb-core/dvb_vb2.c              |  2 +-
>  include/media/videobuf2-core.h                |  7 ++-
>  4 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 4489744fbbd9..3ca0545db7ee 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -664,8 +664,19 @@ 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;
> +}
> +
>  int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> -		unsigned int *count)
> +		bool consistent_mem, unsigned int *count)
>  {
>  	unsigned int num_buffers, allocated_buffers, num_planes = 0;
>  	unsigned plane_sizes[VB2_MAX_PLANES] = { };
> @@ -720,6 +731,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.
> @@ -803,9 +815,21 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
>  
> +static bool verify_consistency_attr(struct vb2_queue *q, bool consistent_mem)
> +{
> +	bool queue_attr = q->dma_attrs & DMA_ATTR_NON_CONSISTENT;
> +
> +	if (consistent_mem != queue_attr) {

This is the wrong way around!

It's much better to write it like this:

       bool queue_is_consistent = !(q->dma_attrs & DMA_ATTR_NON_CONSISTENT);

       if (consistent_mem != queue_is_consistent) {

What concerns me more is that this means that this series has not been
tested properly. I found this when testing with v4l2-compliance and vivid.

I'll reply to the cover letter with more information about what needs to be
done before I merge this.

> +		dprintk(1, "memory consistency model mismatch\n");
> +		return false;
> +	}
> +	return true;
> +}
> +
>  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> -		unsigned int *count, unsigned requested_planes,
> -		const unsigned requested_sizes[])
> +			 bool consistent_mem, unsigned int *count,
> +			 unsigned requested_planes,
> +			 const unsigned requested_sizes[])

Use 'unsigned int' in the two lines above, as per checkpatch suggestion.

Regards,

	Hans

>  {
>  	unsigned int num_planes = 0, num_buffers, allocated_buffers;
>  	unsigned plane_sizes[VB2_MAX_PLANES] = { };
> @@ -823,10 +847,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 +2527,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, true, &fileio->count);
>  	if (ret)
>  		goto err_kfree;
>  
> @@ -2555,7 +2584,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, true, &fileio->count);
>  
>  err_kfree:
>  	q->fileio = NULL;
> @@ -2575,7 +2604,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, true, &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 c847bcea6e95..6111d74f68c9 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -724,7 +724,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, true, &req->count);
>  }
>  EXPORT_SYMBOL_GPL(vb2_reqbufs);
>  
> @@ -798,7 +798,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, true,
>  		&create->count, requested_planes, requested_sizes);
>  }
>  EXPORT_SYMBOL_GPL(vb2_create_bufs);
> @@ -974,7 +974,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, true, &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..e60063652164 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, true, &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 731fd9fbd506..ba83ac754c21 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -737,6 +737,7 @@ 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.
> + * @consistent_mem:	memory consistency model.
>   * @count:	requested buffer count.
>   *
>   * Videobuf2 core helper to implement VIDIOC_REQBUF() operation. It is called
> @@ -761,12 +762,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);
> +		bool consistent_mem, 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.
> + * @consistent_mem: memory consistency model.
>   * @count: requested buffer count.
>   * @requested_planes: number of planes requested.
>   * @requested_sizes: array with the size of the planes.
> @@ -784,7 +786,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,
> +			 bool consistent_mem, unsigned int *count,
> +			 unsigned int requested_planes,
>  			 const unsigned int requested_sizes[]);
>  
>  /**
> 

Re: [PATCHv4 00/11] Implement V4L2_BUF_FLAG_NO_CACHE_* flags

[Hans Verkuil]

On 02/03/2020 05:12, Sergey Senozhatsky wrote:
> Hello,
> 
> 	v4 of the series. Typos and grammar fixes.
> 
> Previous series:
> v3 link: https://lore.kernel.org/lkml/20200226111529.180197-1-senozhatsky@chromium.org
> v2 link: https://lore.kernel.org/lkml/20200204025641.218376-1-senozhatsky@chromium.org/
> v1 link: https://lore.kernel.org/lkml/20191217032034.54897-1-senozhatsky@chromium.org/
> 
> 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.

As mentioned in my v4 review I found a serious bug when testing with
v4l2-compliance. That meant that this series was not tested properly,
which is a requirement for something that touches the core framework.

I've posted an RFC patch with my v4l-utils changes (assumes you've run
'make sync-with-kernel' first), but that's just very basic testing. You
can use it as your starting point.

It needs to be expanded to test the various combinations of flags and
capabilities. I don't think there is a reliable way of actually testing
the cache hint functionality, so that can be skipped, but the compliance
test should at least test the basic behavior depending on whether or not
the cache hints capability is set.

I also would like to see a patch adding cache hint support to an existing
driver (more than one if possible) and the compliance output when tested
against that driver.

You should also test with the test-media script in contrib/test: run as
'sudo test-media mc' to test with all the virtual drivers. If it all passes,
then that's a good indication that there are at least no regressions.

Sorry, but this bug scared me a little, it suggests that not much testing
has been done.

Regards,

	Hans

> 
> [0] https://www.mail-archive.com/linux-media@vger.kernel.org/msg112459.html
> 
> Sergey Senozhatsky (11):
>   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
> 
>  Documentation/media/uapi/v4l/buffer.rst       |  29 +++++
>  .../media/uapi/v4l/vidioc-create-bufs.rst     |   7 +-
>  .../media/uapi/v4l/vidioc-reqbufs.rst         |  18 ++-
>  .../media/common/videobuf2/videobuf2-core.c   | 110 +++++++++++++-----
>  .../common/videobuf2/videobuf2-dma-contig.c   |  39 ++++++-
>  .../media/common/videobuf2/videobuf2-dma-sg.c |  36 ++++--
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  82 ++++++++++++-
>  drivers/media/dvb-core/dvb_vb2.c              |   2 +-
>  drivers/media/v4l2-core/v4l2-ioctl.c          |   5 +-
>  include/media/videobuf2-core.h                |  28 ++++-
>  include/uapi/linux/videodev2.h                |  11 +-
>  11 files changed, 310 insertions(+), 57 deletions(-)
> 

Re: [PATCHv4 05/11] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT flag

[Hans Verkuil]

On 02/03/2020 05:12, Sergey Senozhatsky wrote:
> 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.
> 
> = 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 <senozhatsky@chromium.org>
> ---
>  .../media/uapi/v4l/vidioc-create-bufs.rst     |  7 ++++-
>  .../media/uapi/v4l/vidioc-reqbufs.rst         | 11 +++++--
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 31 +++++++++++++++++--
>  drivers/media/v4l2-core/v4l2-ioctl.c          |  5 +--
>  include/uapi/linux/videodev2.h                |  8 +++--
>  5 files changed, 50 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
> index bd08e4f77ae4..a9c6c89f5098 100644
> --- a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
> +++ b/Documentation/media/uapi/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/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> index 21ecacc72487..faf0df4f9bb6 100644
> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> +++ b/Documentation/media/uapi/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-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 6111d74f68c9..b4b379f3bf98 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -722,9 +722,18 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
>  int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
>  {
>  	int ret = vb2_verify_memory_type(q, req->memory, req->type);
> +	bool consistent = true;
> +
> +	if (!vb2_queue_allows_cache_hints(q))
> +		req->flags &= ~V4L2_FLAG_MEMORY_NON_CONSISTENT;
> +
> +	if (req->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT)
> +		consistent = false;
>  
>  	fill_buf_caps(q, &req->capabilities);
> -	return ret ? ret : vb2_core_reqbufs(q, req->memory, true, &req->count);
> +	if (ret)
> +		return ret;
> +	return vb2_core_reqbufs(q, req->memory, consistent, &req->count);
>  }
>  EXPORT_SYMBOL_GPL(vb2_reqbufs);
>  
> @@ -753,6 +762,7 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>  	unsigned requested_sizes[VIDEO_MAX_PLANES];
>  	struct v4l2_format *f = &create->format;
>  	int ret = vb2_verify_memory_type(q, create->memory, f->type);
> +	bool consistent = true;
>  	unsigned i;
>  
>  	fill_buf_caps(q, &create->capabilities);
> @@ -798,7 +808,14 @@ 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, true,
> +
> +	if (!vb2_queue_allows_cache_hints(q))
> +		create->flags &= ~V4L2_FLAG_MEMORY_NON_CONSISTENT;
> +
> +	if (create->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT)
> +		consistent = false;
> +
> +	return ret ? ret : vb2_core_create_bufs(q, create->memory, consistent,
>  		&create->count, requested_planes, requested_sizes);
>  }
>  EXPORT_SYMBOL_GPL(vb2_create_bufs);
> @@ -968,13 +985,21 @@ 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);
> +	bool consistent = true;
>  
>  	fill_buf_caps(vdev->queue, &p->capabilities);
>  	if (res)
>  		return res;
>  	if (vb2_queue_is_busy(vdev, file))
>  		return -EBUSY;
> -	res = vb2_core_reqbufs(vdev->queue, p->memory, true, &p->count);
> +
> +	if (!vb2_queue_allows_cache_hints(vdev->queue))
> +		p->flags &= ~V4L2_FLAG_MEMORY_NON_CONSISTENT;
> +
> +	if (p->flags & V4L2_FLAG_MEMORY_NON_CONSISTENT)
> +		consistent = false;
> +
> +	res = vb2_core_reqbufs(vdev->queue, p->memory, consistent, &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/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index fbcc7a20eedf..53b87bfd50d7 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1973,9 +1973,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);
>  }
>  
> @@ -2015,7 +2012,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 e92c29864730..12b1bd220347 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -938,7 +938,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 */
> @@ -2446,7 +2449,8 @@ struct v4l2_create_buffers {
>  	__u32			memory;
>  	struct v4l2_format	format;
>  	__u32			capabilities;
> -	__u32			reserved[7];
> +	__u32			flags;

The new flags argument needs to be documented in the command for struct v4l2_create_buffers.

Regards,

	Hans

> +	__u32			reserved[6];
>  };
>  
>  /*
> 

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Sergey Senozhatsky]

On (20/03/06 14:57), Hans Verkuil wrote:
[..]
> > @@ -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().
> 
> checkpatch complains about a space before a tab in the two lines above.

I see them. Sorry. Fixed now.

	-ss

Re: [PATCHv4 10/11] videobuf2: add begin/end cpu_access callbacks to dma-sg

[Sergey Senozhatsky]

On (20/03/06 15:04), Hans Verkuil wrote:
[..]
> > +	/*
> > +	 * 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)
> > @@ -470,6 +476,26 @@ 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)
> 
> I suggest you use this style to avoid checkpatch warnings:
> 
> static int
> vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf,
> 				       enum dma_data_direction direction)

OK, will do.

Just for information, my checkpatch doesn't warn me:

$ ./scripts/checkpatch.pl outgoing/0010-videobuf2-add-begin-end-cpu_access-callbacks-to-dma-.patch
total: 0 errors, 0 warnings, 46 lines checked

outgoing/0010-videobuf2-add-begin-end-cpu_access-callbacks-to-dma-.patch has no obvious style problems and is ready for submission.

	-ss

Re: [PATCHv4 04/11] videobuf2: add queue memory consistency parameter

[Sergey Senozhatsky]

On (20/03/06 15:04), Hans Verkuil wrote:
[..]
> > +static bool verify_consistency_attr(struct vb2_queue *q, bool consistent_mem)
> > +{
> > +	bool queue_attr = q->dma_attrs & DMA_ATTR_NON_CONSISTENT;
> > +
> > +	if (consistent_mem != queue_attr) {
> 
> This is the wrong way around!
> 
> It's much better to write it like this:
> 
>        bool queue_is_consistent = !(q->dma_attrs & DMA_ATTR_NON_CONSISTENT);
> 
>        if (consistent_mem != queue_is_consistent) {

Hmm... That's a great catch. Thanks for spotting this.
Puzzled, how come I've never seen problems.

> What concerns me more is that this means that this series has not been
> tested properly. I found this when testing with v4l2-compliance and vivid.

I fully understand your concerns. Give me a moment to figure
out what's going on...


OK.

Apparently, the user-space I'm using for tests, utilizes different
call path. vb2_core_create_bufs() is never even invoked. Hence queue
consistency vs. request consistency checks are not performed.

What happens, instead, is v4l_reqbufs()->vb2_core_reqbufs() path.
It orphans existing buffers (if any), sets queue memory model, sets
queue consistency model (DMA attr), then allocates buffers.

On my test environment, I see that vb2_core_reqbufs() orphans the
buffers, but it's always due to "*count == 0 || q->num_buffers != 0"
conditions. The user-space I'm using does not twist queue ->memory
or consistency attr, so the tests I'm running are limited in scenarios.

verify_consistency_attr() is not on the list of reasons to orphan
allocated buffer. It probably should be, tho.

===
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index afb3c21a5902..d6b1d32bef3f 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -730,7 +730,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.
===

> > +		dprintk(1, "memory consistency model mismatch\n");
> > +		return false;
> > +	}
> > +	return true;
> > +}
> > +
> >  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> > -		unsigned int *count, unsigned requested_planes,
> > -		const unsigned requested_sizes[])
> > +			 bool consistent_mem, unsigned int *count,
> > +			 unsigned requested_planes,
> > +			 const unsigned requested_sizes[])
> 
> Use 'unsigned int' in the two lines above, as per checkpatch suggestion.

OK, will do.

This comes from the original code. There are 'unsigned'-s in the
existing code, I saw it and didn't want to modify, in order to keep
diffstats shorter.

	-ss

Re: [PATCHv4 05/11] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT flag

[Sergey Senozhatsky]

On (20/03/06 16:30), Hans Verkuil wrote:
[..]
> >  
> >  /* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
> > @@ -2446,7 +2449,8 @@ struct v4l2_create_buffers {
> >  	__u32			memory;
> >  	struct v4l2_format	format;
> >  	__u32			capabilities;
> > -	__u32			reserved[7];
> > +	__u32			flags;
> 
> The new flags argument needs to be documented in the command for struct v4l2_create_buffers.
> 

Will add.

	-ss

Re: [PATCHv4 00/11] Implement V4L2_BUF_FLAG_NO_CACHE_* flags

[Sergey Senozhatsky]

On (20/03/06 15:18), Hans Verkuil wrote:
[..]
> As mentioned in my v4 review I found a serious bug when testing with
> v4l2-compliance. That meant that this series was not tested properly,
> which is a requirement for something that touches the core framework.

I run tests locally on my board, but the scenarios are rather limited.

> I've posted an RFC patch with my v4l-utils changes (assumes you've run
> 'make sync-with-kernel' first), but that's just very basic testing. You
> can use it as your starting point.

Thanks. I'll try to use it as a starting point and run more "diverse"
tests cases.

> It needs to be expanded to test the various combinations of flags and
> capabilities. I don't think there is a reliable way of actually testing
> the cache hint functionality, so that can be skipped, but the compliance
> test should at least test the basic behavior depending on whether or not
> the cache hints capability is set.

I'll take a look.

> I also would like to see a patch adding cache hint support to an existing
> driver (more than one if possible) and the compliance output when tested
> against that driver.

Need to talk to Tomasz and Pawel first.

> You should also test with the test-media script in contrib/test: run as
> 'sudo test-media mc' to test with all the virtual drivers. If it all passes,
> then that's a good indication that there are at least no regressions.

OK, let me try.

	-ss

Re: [PATCHv4 03/11] videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag

[Sergey Senozhatsky]

On (20/03/06 14:58), Hans Verkuil wrote:
> 
> Please fold that in this patch.
> 

Thanks!

	-ss

Re: [PATCHv4 10/11] videobuf2: add begin/end cpu_access callbacks to dma-sg

[Hans Verkuil]

On 07/03/2020 06:26, Sergey Senozhatsky wrote:
> On (20/03/06 15:04), Hans Verkuil wrote:
> [..]
>>> +	/*
>>> +	 * 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)
>>> @@ -470,6 +476,26 @@ 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)
>>
>> I suggest you use this style to avoid checkpatch warnings:
>>
>> static int
>> vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf,
>> 				       enum dma_data_direction direction)
> 
> OK, will do.
> 
> Just for information, my checkpatch doesn't warn me:
> 
> $ ./scripts/checkpatch.pl outgoing/0010-videobuf2-add-begin-end-cpu_access-callbacks-to-dma-.patch

We use the --strict option to checkpatch.

Regards,

	Hans

> total: 0 errors, 0 warnings, 46 lines checked
> 
> outgoing/0010-videobuf2-add-begin-end-cpu_access-callbacks-to-dma-.patch has no obvious style problems and is ready for submission.
> 
> 	-ss
> 

Re: [PATCHv4 05/11] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT flag

[Sergey Senozhatsky]

On (20/03/07 16:51), Sergey Senozhatsky wrote:
> On (20/03/06 16:30), Hans Verkuil wrote:
> [..]
> > >  
> > >  /* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
> > > @@ -2446,7 +2449,8 @@ struct v4l2_create_buffers {
> > >  	__u32			memory;
> > >  	struct v4l2_format	format;
> > >  	__u32			capabilities;
> > > -	__u32			reserved[7];
> > > +	__u32			flags;
> > 
> > The new flags argument needs to be documented in the command for struct v4l2_create_buffers.
> > 

Hans, what does "command for struct v4l2_create_buffers" mean?

BTW, I added v4l2_create_buffers::flags comment:

---

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 12b1bd220347..c6c1cccbb5c1 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2441,6 +2441,8 @@ 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 if queue
+ *		does not have V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS capability).
  * @reserved:	future extensions
  */
 struct v4l2_create_buffers {

Re: [PATCHv4 10/11] videobuf2: add begin/end cpu_access callbacks to dma-sg

[Sergey Senozhatsky]

On (20/03/07 10:32), Hans Verkuil wrote:
> >>> +static int vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf,
> >>> +					enum dma_data_direction direction)
> >>
> >> I suggest you use this style to avoid checkpatch warnings:
> >>
> >> static int
> >> vb2_dma_sg_dmabuf_ops_begin_cpu_access(struct dma_buf *dbuf,
> >> 				       enum dma_data_direction direction)
> > 
> > OK, will do.
> > 
> > Just for information, my checkpatch doesn't warn me:
> > 
> > $ ./scripts/checkpatch.pl outgoing/0010-videobuf2-add-begin-end-cpu_access-callbacks-to-dma-.patch
> 
> We use the --strict option to checkpatch.

Got it.

	-ss

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Sergey Senozhatsky]

On (20/03/06 14:57), Hans Verkuil wrote:
[..]
> >   * @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
> > @@ -564,6 +573,7 @@ struct vb2_queue {
> >  	unsigned			requires_requests:1;
> >  	unsigned			uses_qbuf:1;
> >  	unsigned			uses_requests:1;
> > +	unsigned			allow_cache_hints:1;

Shall I use "unsigned int" here instead of "unsigned"?

	-ss

Re: [PATCHv4 04/11] videobuf2: add queue memory consistency parameter

[Hans Verkuil]

On 07/03/2020 08:50, Sergey Senozhatsky wrote:
> On (20/03/06 15:04), Hans Verkuil wrote:
> [..]
>>> +static bool verify_consistency_attr(struct vb2_queue *q, bool consistent_mem)
>>> +{
>>> +	bool queue_attr = q->dma_attrs & DMA_ATTR_NON_CONSISTENT;
>>> +
>>> +	if (consistent_mem != queue_attr) {
>>
>> This is the wrong way around!
>>
>> It's much better to write it like this:
>>
>>        bool queue_is_consistent = !(q->dma_attrs & DMA_ATTR_NON_CONSISTENT);
>>
>>        if (consistent_mem != queue_is_consistent) {
> 
> Hmm... That's a great catch. Thanks for spotting this.
> Puzzled, how come I've never seen problems.
> 
>> What concerns me more is that this means that this series has not been
>> tested properly. I found this when testing with v4l2-compliance and vivid.
> 
> I fully understand your concerns. Give me a moment to figure
> out what's going on...
> 
> 
> OK.
> 
> Apparently, the user-space I'm using for tests, utilizes different
> call path. vb2_core_create_bufs() is never even invoked. Hence queue
> consistency vs. request consistency checks are not performed.
> 
> What happens, instead, is v4l_reqbufs()->vb2_core_reqbufs() path.
> It orphans existing buffers (if any), sets queue memory model, sets
> queue consistency model (DMA attr), then allocates buffers.
> 
> On my test environment, I see that vb2_core_reqbufs() orphans the
> buffers, but it's always due to "*count == 0 || q->num_buffers != 0"
> conditions. The user-space I'm using does not twist queue ->memory
> or consistency attr, so the tests I'm running are limited in scenarios.

That's why v4l2-compliance is so important: it tests 'twisty code' for
correct handling.

> 
> verify_consistency_attr() is not on the list of reasons to orphan
> allocated buffer. It probably should be, tho.
> 
> ===
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index afb3c21a5902..d6b1d32bef3f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -730,7 +730,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.
> ===
> 
>>> +		dprintk(1, "memory consistency model mismatch\n");
>>> +		return false;
>>> +	}
>>> +	return true;
>>> +}
>>> +
>>>  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>>> -		unsigned int *count, unsigned requested_planes,
>>> -		const unsigned requested_sizes[])
>>> +			 bool consistent_mem, unsigned int *count,
>>> +			 unsigned requested_planes,
>>> +			 const unsigned requested_sizes[])
>>
>> Use 'unsigned int' in the two lines above, as per checkpatch suggestion.
> 
> OK, will do.
> 
> This comes from the original code. There are 'unsigned'-s in the
> existing code, I saw it and didn't want to modify, in order to keep
> diffstats shorter.

Yeah, but the prototype was already inconsistent (count is an unsigned int *),
so it makes sense to fix this.

Regards,

	Hans

> 
> 	-ss
> 

Re: [PATCHv4 05/11] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT flag

[Hans Verkuil]

On 07/03/2020 10:38, Sergey Senozhatsky wrote:
> On (20/03/07 16:51), Sergey Senozhatsky wrote:
>> On (20/03/06 16:30), Hans Verkuil wrote:
>> [..]
>>>>  
>>>>  /* capabilities for struct v4l2_requestbuffers and v4l2_create_buffers */
>>>> @@ -2446,7 +2449,8 @@ struct v4l2_create_buffers {
>>>>  	__u32			memory;
>>>>  	struct v4l2_format	format;
>>>>  	__u32			capabilities;
>>>> -	__u32			reserved[7];
>>>> +	__u32			flags;
>>>
>>> The new flags argument needs to be documented in the command for struct v4l2_create_buffers.
>>>
> 
> Hans, what does "command for struct v4l2_create_buffers" mean?

Oops: command -> comment

> 
> BTW, I added v4l2_create_buffers::flags comment:

Good, that's what I meant :-)

Regards,

	Hans

> 
> ---
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 12b1bd220347..c6c1cccbb5c1 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2441,6 +2441,8 @@ 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 if queue
> + *		does not have V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS capability).
>   * @reserved:	future extensions
>   */
>  struct v4l2_create_buffers {
> 

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Hans Verkuil]

On 07/03/2020 10:46, Sergey Senozhatsky wrote:
> On (20/03/06 14:57), Hans Verkuil wrote:
> [..]
>>>   * @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
>>> @@ -564,6 +573,7 @@ struct vb2_queue {
>>>  	unsigned			requires_requests:1;
>>>  	unsigned			uses_qbuf:1;
>>>  	unsigned			uses_requests:1;
>>> +	unsigned			allow_cache_hints:1;
> 
> Shall I use "unsigned int" here instead of "unsigned"?

The vb2_queue bitfields are the only places in that header were 'unsigned' is
used. I think that that should be fixed in a separate patch. It's nice to have
it consistent.

Put that patch in the beginning of the series, that way I can pick it up in the
next pull request.

Regards,

	Hans

> 
> 	-ss
> 

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Sergey Senozhatsky]

On (20/03/07 11:10), Hans Verkuil wrote:
[..]
> >>> @@ -564,6 +573,7 @@ struct vb2_queue {
> >>>  	unsigned			requires_requests:1;
> >>>  	unsigned			uses_qbuf:1;
> >>>  	unsigned			uses_requests:1;
> >>> +	unsigned			allow_cache_hints:1;
> > 
> > Shall I use "unsigned int" here instead of "unsigned"?
> 
> The vb2_queue bitfields are the only places in that header were 'unsigned' is
> used. I think that that should be fixed in a separate patch. It's nice to have
> it consistent.
> 
> Put that patch in the beginning of the series, that way I can pick it up in the
> next pull request.

OK, done.

For the time being the series has moved to github public repo [0],
I'll try to run more 'twisty' cases and re-submit once it survives
beating.

[0] https://github.com/sergey-senozhatsky/v4l2-mmap-cache-flags

	-ss

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Hans Verkuil]

On 07/03/2020 12:28, Sergey Senozhatsky wrote:
> On (20/03/07 11:10), Hans Verkuil wrote:
> [..]
>>>>> @@ -564,6 +573,7 @@ struct vb2_queue {
>>>>>  	unsigned			requires_requests:1;
>>>>>  	unsigned			uses_qbuf:1;
>>>>>  	unsigned			uses_requests:1;
>>>>> +	unsigned			allow_cache_hints:1;
>>>
>>> Shall I use "unsigned int" here instead of "unsigned"?
>>
>> The vb2_queue bitfields are the only places in that header were 'unsigned' is
>> used. I think that that should be fixed in a separate patch. It's nice to have
>> it consistent.
>>
>> Put that patch in the beginning of the series, that way I can pick it up in the
>> next pull request.
> 
> OK, done.
> 
> For the time being the series has moved to github public repo [0],
> I'll try to run more 'twisty' cases and re-submit once it survives
> beating.

Create those tests in v4l2-compliance: that's where they belong.

You need these tests:

For non-MMAP modes:

1) test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is never set.

If V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is not set, then:

1) attempting to use V4L2_FLAG_MEMORY_NON_CONSISTENT will clear the flag
   upon return (test with both reqbufs and create_bufs).
2) attempting to use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
   will clear those flags upon return (do we actually do that in the patch series?).

If V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is set, then:

1) set V4L2_FLAG_MEMORY_NON_CONSISTENT in reqbufs, but clear in create_bufs:
   this should fail.
2) clear V4L2_FLAG_MEMORY_NON_CONSISTENT in reqbufs, but set in create_bufs:
   this should fail.
3) set V4L2_FLAG_MEMORY_NON_CONSISTENT in both reqbufs and create_bufs: this should
   work.
4) clear V4L2_FLAG_MEMORY_NON_CONSISTENT in both reqbufs and create_bufs: this should
   work.
5) you can use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
   without these flags being cleared in v4l2_buffer.

All these tests can be done in testReqBufs().

Regards,

	Hans

> 
> [0] https://github.com/sergey-senozhatsky/v4l2-mmap-cache-flags
> 
> 	-ss
> 

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Sergey Senozhatsky]

On (20/03/07 12:47), Hans Verkuil wrote:
> 
> Create those tests in v4l2-compliance: that's where they belong.
>
> You need these tests:
> 
> For non-MMAP modes:
> 
> 1) test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is never set.
> 
> If V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is not set, then:
> 
> 1) attempting to use V4L2_FLAG_MEMORY_NON_CONSISTENT will clear the flag
>    upon return (test with both reqbufs and create_bufs).
> 2) attempting to use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
>    will clear those flags upon return (do we actually do that in the patch series?).

NO_CACHE_INVALIDATE/NO_CACHE_CLEAN are cleared in vb2_fill_vb2_v4l2_buffer()
[as was suggested], then the struct is copied back to user. But I think it
would be better to clear those flags when we clear
V4L2_FLAG_MEMORY_NON_CONSISTENT. We have 4 places which do something
like
	"if !vb2_queue_allows_cache_hints(q) then clear flags bit".

Besides, vb2_fill_vb2_v4l2_buffer() is called only for !prepared
buffers, so the flags won't be cleared if the buffer is already
prepared.

Another thing is that, it seems, I need to patch compat32 code. It
copies to user structs member by member so I need to add ->flags.

> If V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is set, then:
> 
> 1) set V4L2_FLAG_MEMORY_NON_CONSISTENT in reqbufs, but clear in create_bufs:
>    this should fail.
> 2) clear V4L2_FLAG_MEMORY_NON_CONSISTENT in reqbufs, but set in create_bufs:
>    this should fail.
> 3) set V4L2_FLAG_MEMORY_NON_CONSISTENT in both reqbufs and create_bufs: this should
>    work.
> 4) clear V4L2_FLAG_MEMORY_NON_CONSISTENT in both reqbufs and create_bufs: this should
>    work.
> 5) you can use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
>    without these flags being cleared in v4l2_buffer.
> 
> All these tests can be done in testReqBufs().

I'm looking into it. Will it work if I patch the vivid test driver to
enable/disable ->allow_cache_hints bit per-node and include the patch
into the series. So then v4l2 tests can create some nodes with
->allow_cache_hints.  Something like this:

---
 drivers/media/platform/vivid/vivid-core.c     | 6 +++++-
 drivers/media/platform/vivid/vivid-core.h     | 1 +
 drivers/media/platform/vivid/vivid-meta-cap.c | 3 +++
 drivers/media/platform/vivid/vivid-meta-out.c | 3 +++
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
index c62c068b6b60..9acbb59d240c 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -129,7 +129,8 @@ MODULE_PARM_DESC(node_types, " node types, default is 0xe1d3d. Bitmask with the
 			     "\t\t    bit 16: Framebuffer for testing overlays\n"
 			     "\t\t    bit 17: Metadata Capture node\n"
 			     "\t\t    bit 18: Metadata Output node\n"
-			     "\t\t    bit 19: Touch Capture node\n");
+			     "\t\t    bit 19: Touch Capture node\n"
+			     "\t\t    bit 20: Node supports cache-hints\n");
 
 /* Default: 4 inputs */
 static unsigned num_inputs[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS - 1)] = 4 };
@@ -977,6 +978,9 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		return -EINVAL;
 	}
 
+	/* do we enable user-space cache management hints */
+	dev->allow_cache_hints = node_type & 0x100000;
+
 	/* do we create a radio receiver device? */
 	dev->has_radio_rx = node_type & 0x0010;
 
diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
index 99e69b8f770f..2d311fc33619 100644
--- a/drivers/media/platform/vivid/vivid-core.h
+++ b/drivers/media/platform/vivid/vivid-core.h
@@ -206,6 +206,7 @@ struct vivid_dev {
 	bool				has_meta_out;
 	bool				has_tv_tuner;
 	bool				has_touch_cap;
+	bool				allow_cache_hints;
 
 	bool				can_loop_video;
 
diff --git a/drivers/media/platform/vivid/vivid-meta-cap.c b/drivers/media/platform/vivid/vivid-meta-cap.c
index 780f96860a6d..6c28034d3d58 100644
--- a/drivers/media/platform/vivid/vivid-meta-cap.c
+++ b/drivers/media/platform/vivid/vivid-meta-cap.c
@@ -33,6 +33,9 @@ static int meta_cap_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
 	if (vq->num_buffers + *nbuffers < 2)
 		*nbuffers = 2 - vq->num_buffers;
 
+	if (dev->allow_cache_hints)
+		vq->allow_cache_hints = true;
+
 	*nplanes = 1;
 	return 0;
 }
diff --git a/drivers/media/platform/vivid/vivid-meta-out.c b/drivers/media/platform/vivid/vivid-meta-out.c
index ff8a039aba72..d7b808aa5f6d 100644
--- a/drivers/media/platform/vivid/vivid-meta-out.c
+++ b/drivers/media/platform/vivid/vivid-meta-out.c
@@ -33,6 +33,9 @@ static int meta_out_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
 	if (vq->num_buffers + *nbuffers < 2)
 		*nbuffers = 2 - vq->num_buffers;
 
+	if (dev->allow_cache_hints)
+		vq->allow_cache_hints = true;
+
 	*nplanes = 1;
 	return 0;
 }
-- 
2.25.1

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Sergey Senozhatsky]

On (20/03/09 12:27), Sergey Senozhatsky wrote:
> On (20/03/07 12:47), Hans Verkuil wrote:
> > 
> > Create those tests in v4l2-compliance: that's where they belong.
> >
> > You need these tests:
> > 
> > For non-MMAP modes:
> > 
> > 1) test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is never set.
> > 
> > If V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is not set, then:
> > 
> > 1) attempting to use V4L2_FLAG_MEMORY_NON_CONSISTENT will clear the flag
> >    upon return (test with both reqbufs and create_bufs).
> > 2) attempting to use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
> >    will clear those flags upon return (do we actually do that in the patch series?).
> 
> NO_CACHE_INVALIDATE/NO_CACHE_CLEAN are cleared in vb2_fill_vb2_v4l2_buffer()
> [as was suggested], then the struct is copied back to user. But I think it
> would be better to clear those flags when we clear
> V4L2_FLAG_MEMORY_NON_CONSISTENT. We have 4 places which do something
> like
> 	"if !vb2_queue_allows_cache_hints(q) then clear flags bit".

No. I'll just move NO_CACHE_INVALIDATE/NO_CACHE_CLEAN handling to
set_buffer_cache_hints(). This is where we take care of q->memory
and q->allow_cache_hints, this is where we set/clear the
->need_cache_sync flags, this is where we also can clear v4l2_buffer
->flags. Seems like a better place than vb2_fill_vb2_v4l2_buffer().

---
 .../media/common/videobuf2/videobuf2-v4l2.c   | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index b4b379f3bf98..0a6bd4a1f58e 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -199,15 +199,6 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
 	vbuf->request_fd = -1;
 	vbuf->is_held = false;
 
-	/*
-	 * Clear buffer cache flags if queue does not support user space hints.
-	 * That's to indicate to userspace that these flags won't work.
-	 */
-	if (!vb2_queue_allows_cache_hints(q)) {
-		b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_INVALIDATE;
-		b->flags &= ~V4L2_BUF_FLAG_NO_CACHE_CLEAN;
-	}
-
 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
 		switch (b->memory) {
 		case VB2_MEMORY_USERPTR:
@@ -368,8 +359,16 @@ static void set_buffer_cache_hints(struct vb2_queue *q,
 	vb->need_cache_sync_on_prepare = 1;
 	vb->need_cache_sync_on_finish = 1;
 
-	if (!vb2_queue_allows_cache_hints(q))
+	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
-- 
2.25.1

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Sergey Senozhatsky]

On (20/03/07 12:47), Hans Verkuil wrote:
> 
> 1) attempting to use V4L2_FLAG_MEMORY_NON_CONSISTENT will clear the flag
>    upon return (test with both reqbufs and create_bufs).
> 2) attempting to use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
>
[..]
>
> All these tests can be done in testReqBufs().

MEMORY_NON_CONSISTENT is a queue property, we set it during queue setup.
NO_CACHE_INVALIDATE/FLAG_NO_CACHE_CLEAN is a buffer property, we set it
when we qbuf. I'm not sure if all of these can be done in testReqBufts().

	-ss

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Hans Verkuil]

On 3/9/20 5:03 AM, Sergey Senozhatsky wrote:
> On (20/03/07 12:47), Hans Verkuil wrote:
>>
>> 1) attempting to use V4L2_FLAG_MEMORY_NON_CONSISTENT will clear the flag
>>    upon return (test with both reqbufs and create_bufs).
>> 2) attempting to use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
>>
> [..]
>>
>> All these tests can be done in testReqBufs().
> 
> MEMORY_NON_CONSISTENT is a queue property, we set it during queue setup.
> NO_CACHE_INVALIDATE/FLAG_NO_CACHE_CLEAN is a buffer property, we set it
> when we qbuf. I'm not sure if all of these can be done in testReqBufts().

testReqBufs can call qbuf, but not streamon. Since you don't need streaming
for this, testReqBufs should be fine.

Regards,

	Hans

> 
> 	-ss
> 

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Hans Verkuil]

On 3/9/20 4:27 AM, Sergey Senozhatsky wrote:
> On (20/03/07 12:47), Hans Verkuil wrote:
>>
>> Create those tests in v4l2-compliance: that's where they belong.
>>
>> You need these tests:
>>
>> For non-MMAP modes:
>>
>> 1) test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is never set.
>>
>> If V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is not set, then:
>>
>> 1) attempting to use V4L2_FLAG_MEMORY_NON_CONSISTENT will clear the flag
>>    upon return (test with both reqbufs and create_bufs).
>> 2) attempting to use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
>>    will clear those flags upon return (do we actually do that in the patch series?).
> 
> NO_CACHE_INVALIDATE/NO_CACHE_CLEAN are cleared in vb2_fill_vb2_v4l2_buffer()
> [as was suggested], then the struct is copied back to user. But I think it
> would be better to clear those flags when we clear
> V4L2_FLAG_MEMORY_NON_CONSISTENT. We have 4 places which do something
> like
> 	"if !vb2_queue_allows_cache_hints(q) then clear flags bit".
> 
> Besides, vb2_fill_vb2_v4l2_buffer() is called only for !prepared
> buffers, so the flags won't be cleared if the buffer is already
> prepared.
> 
> Another thing is that, it seems, I need to patch compat32 code. It
> copies to user structs member by member so I need to add ->flags.
> 
>> If V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is set, then:
>>
>> 1) set V4L2_FLAG_MEMORY_NON_CONSISTENT in reqbufs, but clear in create_bufs:
>>    this should fail.
>> 2) clear V4L2_FLAG_MEMORY_NON_CONSISTENT in reqbufs, but set in create_bufs:
>>    this should fail.
>> 3) set V4L2_FLAG_MEMORY_NON_CONSISTENT in both reqbufs and create_bufs: this should
>>    work.
>> 4) clear V4L2_FLAG_MEMORY_NON_CONSISTENT in both reqbufs and create_bufs: this should
>>    work.
>> 5) you can use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
>>    without these flags being cleared in v4l2_buffer.
>>
>> All these tests can be done in testReqBufs().
> 
> I'm looking into it. Will it work if I patch the vivid test driver to
> enable/disable ->allow_cache_hints bit per-node and include the patch
> into the series. So then v4l2 tests can create some nodes with
> ->allow_cache_hints.  Something like this:

I would add a 'cache_hints' module parameter (array of bool) to tell vivid
whether cache hints should be set or not for each instance. It would be useful
to have this in vivid. Don't forget to update Documentation/media/v4l-drivers/vivid.rst
as well.

Regards,

	Hans

> 
> ---
>  drivers/media/platform/vivid/vivid-core.c     | 6 +++++-
>  drivers/media/platform/vivid/vivid-core.h     | 1 +
>  drivers/media/platform/vivid/vivid-meta-cap.c | 3 +++
>  drivers/media/platform/vivid/vivid-meta-out.c | 3 +++
>  4 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
> index c62c068b6b60..9acbb59d240c 100644
> --- a/drivers/media/platform/vivid/vivid-core.c
> +++ b/drivers/media/platform/vivid/vivid-core.c
> @@ -129,7 +129,8 @@ MODULE_PARM_DESC(node_types, " node types, default is 0xe1d3d. Bitmask with the
>  			     "\t\t    bit 16: Framebuffer for testing overlays\n"
>  			     "\t\t    bit 17: Metadata Capture node\n"
>  			     "\t\t    bit 18: Metadata Output node\n"
> -			     "\t\t    bit 19: Touch Capture node\n");
> +			     "\t\t    bit 19: Touch Capture node\n"
> +			     "\t\t    bit 20: Node supports cache-hints\n");
>  
>  /* Default: 4 inputs */
>  static unsigned num_inputs[VIVID_MAX_DEVS] = { [0 ... (VIVID_MAX_DEVS - 1)] = 4 };
> @@ -977,6 +978,9 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
>  		return -EINVAL;
>  	}
>  
> +	/* do we enable user-space cache management hints */
> +	dev->allow_cache_hints = node_type & 0x100000;
> +
>  	/* do we create a radio receiver device? */
>  	dev->has_radio_rx = node_type & 0x0010;
>  
> diff --git a/drivers/media/platform/vivid/vivid-core.h b/drivers/media/platform/vivid/vivid-core.h
> index 99e69b8f770f..2d311fc33619 100644
> --- a/drivers/media/platform/vivid/vivid-core.h
> +++ b/drivers/media/platform/vivid/vivid-core.h
> @@ -206,6 +206,7 @@ struct vivid_dev {
>  	bool				has_meta_out;
>  	bool				has_tv_tuner;
>  	bool				has_touch_cap;
> +	bool				allow_cache_hints;
>  
>  	bool				can_loop_video;
>  
> diff --git a/drivers/media/platform/vivid/vivid-meta-cap.c b/drivers/media/platform/vivid/vivid-meta-cap.c
> index 780f96860a6d..6c28034d3d58 100644
> --- a/drivers/media/platform/vivid/vivid-meta-cap.c
> +++ b/drivers/media/platform/vivid/vivid-meta-cap.c
> @@ -33,6 +33,9 @@ static int meta_cap_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>  	if (vq->num_buffers + *nbuffers < 2)
>  		*nbuffers = 2 - vq->num_buffers;
>  
> +	if (dev->allow_cache_hints)
> +		vq->allow_cache_hints = true;
> +
>  	*nplanes = 1;
>  	return 0;
>  }
> diff --git a/drivers/media/platform/vivid/vivid-meta-out.c b/drivers/media/platform/vivid/vivid-meta-out.c
> index ff8a039aba72..d7b808aa5f6d 100644
> --- a/drivers/media/platform/vivid/vivid-meta-out.c
> +++ b/drivers/media/platform/vivid/vivid-meta-out.c
> @@ -33,6 +33,9 @@ static int meta_out_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
>  	if (vq->num_buffers + *nbuffers < 2)
>  		*nbuffers = 2 - vq->num_buffers;
>  
> +	if (dev->allow_cache_hints)
> +		vq->allow_cache_hints = true;
> +
>  	*nplanes = 1;
>  	return 0;
>  }
> 

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Sergey Senozhatsky]

On (20/03/09 08:21), Hans Verkuil wrote:
> On 3/9/20 4:27 AM, Sergey Senozhatsky wrote:
> > On (20/03/07 12:47), Hans Verkuil wrote:
> >>
> >> Create those tests in v4l2-compliance: that's where they belong.
> >>
> >> You need these tests:
> >>
> >> For non-MMAP modes:
> >>
> >> 1) test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is never set.
> >>
> >> If V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is not set, then:
> >>
> >> 1) attempting to use V4L2_FLAG_MEMORY_NON_CONSISTENT will clear the flag
> >>    upon return (test with both reqbufs and create_bufs).
> >> 2) attempting to use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
> >>    will clear those flags upon return (do we actually do that in the patch series?).

[..]

> > I'm looking into it. Will it work if I patch the vivid test driver to
> > enable/disable ->allow_cache_hints bit per-node and include the patch
> > into the series. So then v4l2 tests can create some nodes with
> > ->allow_cache_hints.  Something like this:
> 
> I would add a 'cache_hints' module parameter (array of bool) to tell vivid
> whether cache hints should be set or not for each instance. It would be useful
> to have this in vivid. Don't forget to update Documentation/media/v4l-drivers/vivid.rst
> as well.

I see. Hmm, how do I do "test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS
is never set" then?

	-ss

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Hans Verkuil]

On 3/9/20 8:25 AM, Sergey Senozhatsky wrote:
> On (20/03/09 08:21), Hans Verkuil wrote:
>> On 3/9/20 4:27 AM, Sergey Senozhatsky wrote:
>>> On (20/03/07 12:47), Hans Verkuil wrote:
>>>>
>>>> Create those tests in v4l2-compliance: that's where they belong.
>>>>
>>>> You need these tests:
>>>>
>>>> For non-MMAP modes:
>>>>
>>>> 1) test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is never set.
>>>>
>>>> If V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is not set, then:
>>>>
>>>> 1) attempting to use V4L2_FLAG_MEMORY_NON_CONSISTENT will clear the flag
>>>>    upon return (test with both reqbufs and create_bufs).
>>>> 2) attempting to use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
>>>>    will clear those flags upon return (do we actually do that in the patch series?).
> 
> [..]
> 
>>> I'm looking into it. Will it work if I patch the vivid test driver to
>>> enable/disable ->allow_cache_hints bit per-node and include the patch
>>> into the series. So then v4l2 tests can create some nodes with
>>> ->allow_cache_hints.  Something like this:
>>
>> I would add a 'cache_hints' module parameter (array of bool) to tell vivid
>> whether cache hints should be set or not for each instance. It would be useful
>> to have this in vivid. Don't forget to update Documentation/media/v4l-drivers/vivid.rst
>> as well.
> 
> I see. Hmm, how do I do "test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS
> is never set" then?

Not sure I understand your question. When requesting buffers for non-MMAP memory,
this capability must never be returned. That has nothing to do with a cache_hints
module option.

Regards,

	Hans

> 
> 	-ss
> 

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Sergey Senozhatsky]

On (20/03/09 08:28), Hans Verkuil wrote:
> >>> I'm looking into it. Will it work if I patch the vivid test driver to
> >>> enable/disable ->allow_cache_hints bit per-node and include the patch
> >>> into the series. So then v4l2 tests can create some nodes with
> >>> ->allow_cache_hints.  Something like this:
> >>
> >> I would add a 'cache_hints' module parameter (array of bool) to tell vivid
> >> whether cache hints should be set or not for each instance. It would be useful
> >> to have this in vivid. Don't forget to update Documentation/media/v4l-drivers/vivid.rst
> >> as well.
> > 
> > I see. Hmm, how do I do "test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS
> > is never set" then?
> 
> Not sure I understand your question. When requesting buffers for non-MMAP memory,
> this capability must never be returned. That has nothing to do with a cache_hints
> module option.

OK. What I thought was that not every MMAP memory node can have cache
hints enabled, so it should be verified that only MMAP nodes which were
configured to allow_cache_hints should report that CAP, the rest of MMAP
nodes (if any) should have that CAP bit clear.

	-ss

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Tomasz Figa]

On Mon, Mar 9, 2020 at 4:28 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 3/9/20 8:25 AM, Sergey Senozhatsky wrote:
> > On (20/03/09 08:21), Hans Verkuil wrote:
> >> On 3/9/20 4:27 AM, Sergey Senozhatsky wrote:
> >>> On (20/03/07 12:47), Hans Verkuil wrote:
> >>>>
> >>>> Create those tests in v4l2-compliance: that's where they belong.
> >>>>
> >>>> You need these tests:
> >>>>
> >>>> For non-MMAP modes:
> >>>>
> >>>> 1) test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is never set.
> >>>>
> >>>> If V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS is not set, then:
> >>>>
> >>>> 1) attempting to use V4L2_FLAG_MEMORY_NON_CONSISTENT will clear the flag
> >>>>    upon return (test with both reqbufs and create_bufs).
> >>>> 2) attempting to use V4L2_BUF_FLAG_NO_CACHE_INVALIDATE or V4L2_BUF_FLAG_NO_CACHE_CLEAN
> >>>>    will clear those flags upon return (do we actually do that in the patch series?).
> >
> > [..]
> >
> >>> I'm looking into it. Will it work if I patch the vivid test driver to
> >>> enable/disable ->allow_cache_hints bit per-node and include the patch
> >>> into the series. So then v4l2 tests can create some nodes with
> >>> ->allow_cache_hints.  Something like this:
> >>
> >> I would add a 'cache_hints' module parameter (array of bool) to tell vivid
> >> whether cache hints should be set or not for each instance. It would be useful
> >> to have this in vivid. Don't forget to update Documentation/media/v4l-drivers/vivid.rst
> >> as well.
> >
> > I see. Hmm, how do I do "test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS
> > is never set" then?
>
> Not sure I understand your question. When requesting buffers for non-MMAP memory,
> this capability must never be returned. That has nothing to do with a cache_hints
> module option.

Have we decided that we explicitly don't want to support this for
USERPTR memory, even though technically possible and without much
extra code needed?

Best regards,
Tomasz

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Sergey Senozhatsky]

On (20/03/09 17:58), Tomasz Figa wrote:
[..]
> > > I see. Hmm, how do I do "test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS
> > > is never set" then?
> >
> > Not sure I understand your question. When requesting buffers for non-MMAP memory,
> > this capability must never be returned. That has nothing to do with a cache_hints
> > module option.
>
> Have we decided that we explicitly don't want to support this for
> USERPTR memory, even though technically possible and without much
> extra code needed?

My irrelevant 5 cents (sorry), I'd probably prefer to land MMAP
first + test drivers patches + v4l-util patches. The effort
required to land this is getting bigger.

	-ss

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Tomasz Figa]

On Mon, Mar 9, 2020 at 6:08 PM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (20/03/09 17:58), Tomasz Figa wrote:
> [..]
> > > > I see. Hmm, how do I do "test that V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS
> > > > is never set" then?
> > >
> > > Not sure I understand your question. When requesting buffers for non-MMAP memory,
> > > this capability must never be returned. That has nothing to do with a cache_hints
> > > module option.
> >
> > Have we decided that we explicitly don't want to support this for
> > USERPTR memory, even though technically possible and without much
> > extra code needed?
>
> My irrelevant 5 cents (sorry), I'd probably prefer to land MMAP
> first + test drivers patches + v4l-util patches. The effort
> required to land this is getting bigger.

I think that's fine, but we need to make it explicit. :)

Best regards,
Tomasz

Re: [PATCHv4 01/11] videobuf2: add cache management members

[Sergey Senozhatsky]

On (20/03/09 18:17), Tomasz Figa wrote:
> 
> I think that's fine, but we need to make it explicit. :)
> 

I'll improve Docs.

	-ss

Re: [PATCHv4 04/11] videobuf2: add queue memory consistency parameter

[Dafna Hirschfeld]

Hi

On 02.03.20 05:12, Sergey Senozhatsky wrote:
> Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support.
> 
> Extend vb2_core_reqbufs() with queue memory consistency flag
> that is applied to the newly allocated buffers.
> 
> An attempt to allocate a buffer with consistency requirements
> which don't match queue's consistency model will fail.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>   .../media/common/videobuf2/videobuf2-core.c   | 47 +++++++++++++++----
>   .../media/common/videobuf2/videobuf2-v4l2.c   |  6 +--
>   drivers/media/dvb-core/dvb_vb2.c              |  2 +-
>   include/media/videobuf2-core.h                |  7 ++-
>   4 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 4489744fbbd9..3ca0545db7ee 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -664,8 +664,19 @@ 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;
> +}
> +
>   int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> -		unsigned int *count)
> +		bool consistent_mem, unsigned int *count)
You extended the vb2_core_reqbufs accepting a new boolean that
is decided according to the setting of the V4L2_FLAG_MEMORY_NON_CONSISTENT
but in the future some other flags might be added, and so I think it
is better to replace the boolean with a u32 consisting of all the flags.

Thanks,
Dafna

>   {
>   	unsigned int num_buffers, allocated_buffers, num_planes = 0;
>   	unsigned plane_sizes[VB2_MAX_PLANES] = { };
> @@ -720,6 +731,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.
> @@ -803,9 +815,21 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>   }
>   EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
>   
> +static bool verify_consistency_attr(struct vb2_queue *q, bool consistent_mem)
> +{
> +	bool queue_attr = q->dma_attrs & DMA_ATTR_NON_CONSISTENT;
> +
> +	if (consistent_mem != queue_attr) {
> +		dprintk(1, "memory consistency model mismatch\n");
> +		return false;
> +	}
> +	return true;
> +}
> +
>   int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> -		unsigned int *count, unsigned requested_planes,
> -		const unsigned requested_sizes[])
> +			 bool consistent_mem, unsigned int *count,
> +			 unsigned requested_planes,
> +			 const unsigned requested_sizes[])
>   {
>   	unsigned int num_planes = 0, num_buffers, allocated_buffers;
>   	unsigned plane_sizes[VB2_MAX_PLANES] = { };
> @@ -823,10 +847,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 +2527,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, true, &fileio->count);
>   	if (ret)
>   		goto err_kfree;
>   
> @@ -2555,7 +2584,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, true, &fileio->count);
>   
>   err_kfree:
>   	q->fileio = NULL;
> @@ -2575,7 +2604,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, true, &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 c847bcea6e95..6111d74f68c9 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -724,7 +724,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, true, &req->count);
>   }
>   EXPORT_SYMBOL_GPL(vb2_reqbufs);
>   
> @@ -798,7 +798,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, true,
>   		&create->count, requested_planes, requested_sizes);
>   }
>   EXPORT_SYMBOL_GPL(vb2_create_bufs);
> @@ -974,7 +974,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, true, &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..e60063652164 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, true, &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 731fd9fbd506..ba83ac754c21 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -737,6 +737,7 @@ 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.
> + * @consistent_mem:	memory consistency model.
>    * @count:	requested buffer count.
>    *
>    * Videobuf2 core helper to implement VIDIOC_REQBUF() operation. It is called
> @@ -761,12 +762,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);
> +		bool consistent_mem, 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.
> + * @consistent_mem: memory consistency model.
>    * @count: requested buffer count.
>    * @requested_planes: number of planes requested.
>    * @requested_sizes: array with the size of the planes.
> @@ -784,7 +786,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,
> +			 bool consistent_mem, unsigned int *count,
> +			 unsigned int requested_planes,
>   			 const unsigned int requested_sizes[]);
>   
>   /**
> 

Re: [PATCHv4 04/11] videobuf2: add queue memory consistency parameter

[Sergey Senozhatsky]

On (20/03/20 14:46), Dafna Hirschfeld wrote:
[..]
> > +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;
> > +}
> > +
> >   int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> > -		unsigned int *count)
> > +		bool consistent_mem, unsigned int *count)
> You extended the vb2_core_reqbufs accepting a new boolean that
> is decided according to the setting of the V4L2_FLAG_MEMORY_NON_CONSISTENT
> but in the future some other flags might be added, and so I think it
> is better to replace the boolean with a u32 consisting of all the flags.

Don't have any objections. Can change the `bool' to `u32'.

	-ss

Re: [PATCHv4 04/11] videobuf2: add queue memory consistency parameter

[Ezequiel Garcia]

On Tue, 2020-03-24 at 11:39 +0900, Sergey Senozhatsky wrote:
> On (20/03/20 14:46), Dafna Hirschfeld wrote:
> [..]
> > > +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;
> > > +}
> > > +
> > >   int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> > > -		unsigned int *count)
> > > +		bool consistent_mem, unsigned int *count)
> > You extended the vb2_core_reqbufs accepting a new boolean that
> > is decided according to the setting of the V4L2_FLAG_MEMORY_NON_CONSISTENT
> > but in the future some other flags might be added, and so I think it
> > is better to replace the boolean with a u32 consisting of all the flags.
> 
> Don't have any objections. Can change the `bool' to `u32'.
> 

or maybe an enum instead? That would help get a cleaner
interface.

Thanks,
Ezequiel

Re: [PATCHv4 04/11] videobuf2: add queue memory consistency parameter

[Sergey Senozhatsky]

On (20/03/24 07:17), Ezequiel Garcia wrote:
[..]
> > > > +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;
> > > > +}
> > > > +
> > > >   int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> > > > -		unsigned int *count)
> > > > +		bool consistent_mem, unsigned int *count)
> > > You extended the vb2_core_reqbufs accepting a new boolean that
> > > is decided according to the setting of the V4L2_FLAG_MEMORY_NON_CONSISTENT
> > > but in the future some other flags might be added, and so I think it
> > > is better to replace the boolean with a u32 consisting of all the flags.
> > 
> > Don't have any objections. Can change the `bool' to `u32'.
> > 
> 
> or maybe an enum instead? That would help get a cleaner
> interface.

Hmm, interesting.

The flags in question can be from different, unrelated groups
(types), controlling various parts of the stack. Not necessarily
all of them are memory_consistency related. We can, for instance,
pass some additional flags to underlying memory allocators (contig,
sg), etc.

E.g.

	enum MEMORY_ATTR {
		MEM_NON_CONSISTENT,
		...
	};

	enum VMALLOC_ALLOCATOR_ATTR {
		DO_A_BARREL_ROLL,
		...
	};

	enum DMA_SG_ALLOCATOR_ATTR {
		WRITEBACK_TO_TAPE_DEVICE,
		...
	};

	enum DMA_CONTIG_ALLOCATOR_ATTR {
		PREFER_HTTPS,
		...
	};

and so on. We maybe can keep all of those in one enum (umm, what should
be the name?), and then either make sure that all of them are proper powers
of two

	enum AUX_FLAGS {
		MEM_NON_CONSISTENT		= (1 << 0),
		DO_A_BARREL_ROLL		= (1 << 1),
		WRITEBACK_TO_TAPE_DEVICE	= (1 << 2),
		PREFER_HTTPS			= (1 << 3),
	};

or
	enum AUX_FLAGS {
		MEM_NON_CONSISTENT		= 0,
		DO_A_BARREL_ROLL,
		WRITEBACK_TO_TAPE_DEVICE,
		PREFER_HTTPS,
	};

and make sure that those are not flags, but bits.
IOW, if (flags & BIT(MEM_NON_CONSISTENT)).


What do others think?

	-ss

Re: [PATCHv4 04/11] videobuf2: add queue memory consistency parameter

[Tomasz Figa]

On Wed, Mar 25, 2020 at 3:32 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (20/03/24 07:17), Ezequiel Garcia wrote:
> [..]
> > > > > +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;
> > > > > +}
> > > > > +
> > > > >   int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> > > > > -               unsigned int *count)
> > > > > +               bool consistent_mem, unsigned int *count)
> > > > You extended the vb2_core_reqbufs accepting a new boolean that
> > > > is decided according to the setting of the V4L2_FLAG_MEMORY_NON_CONSISTENT
> > > > but in the future some other flags might be added, and so I think it
> > > > is better to replace the boolean with a u32 consisting of all the flags.
> > >
> > > Don't have any objections. Can change the `bool' to `u32'.
> > >
> >
> > or maybe an enum instead? That would help get a cleaner
> > interface.
>
> Hmm, interesting.
>
> The flags in question can be from different, unrelated groups
> (types), controlling various parts of the stack. Not necessarily
> all of them are memory_consistency related. We can, for instance,
> pass some additional flags to underlying memory allocators (contig,
> sg), etc.
>
> E.g.
>
>         enum MEMORY_ATTR {
>                 MEM_NON_CONSISTENT,
>                 ...
>         };
>
>         enum VMALLOC_ALLOCATOR_ATTR {
>                 DO_A_BARREL_ROLL,
>                 ...
>         };
>
>         enum DMA_SG_ALLOCATOR_ATTR {
>                 WRITEBACK_TO_TAPE_DEVICE,
>                 ...
>         };
>
>         enum DMA_CONTIG_ALLOCATOR_ATTR {
>                 PREFER_HTTPS,
>                 ...
>         };
>
> and so on. We maybe can keep all of those in one enum (umm, what should
> be the name?), and then either make sure that all of them are proper powers
> of two
>
>         enum AUX_FLAGS {
>                 MEM_NON_CONSISTENT              = (1 << 0),
>                 DO_A_BARREL_ROLL                = (1 << 1),
>                 WRITEBACK_TO_TAPE_DEVICE        = (1 << 2),
>                 PREFER_HTTPS                    = (1 << 3),
>         };
>
> or
>         enum AUX_FLAGS {
>                 MEM_NON_CONSISTENT              = 0,
>                 DO_A_BARREL_ROLL,
>                 WRITEBACK_TO_TAPE_DEVICE,
>                 PREFER_HTTPS,
>         };
>
> and make sure that those are not flags, but bits.
> IOW, if (flags & BIT(MEM_NON_CONSISTENT)).
>
>
> What do others think?

My feeling is that there it's a bit of an abuse of the language
construct. Enum is expected to be an enumeration type, where the value
is always one and only one of the defined values at the same time.
Making a bitwise OR of several values makes the resulting value
outside of the enum specification.

AFAICT, the typical approach in the kernel is to just have a block of
#define statements to define the flags and have the flag names
prefixed with some consistent prefix, e.g. VB2_QUEUE_FLAG_. The flags
itself would be defined using BIT() so they can be used directly in
the bitwise arithmetics.

Best regards,
Tomasz