linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags
@ 2019-12-17  3:20 Sergey Senozhatsky
  2019-12-17  3:20 ` [RFC][PATCH 01/15] videobuf2: add cache management members Sergey Senozhatsky
                   ` (15 more replies)
  0 siblings, 16 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, Sergey Senozhatsky

Hello,

	RFC

	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 (15):
  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 in REQBUFS
  videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE_BUFS
  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: let user-space know when driver supports cache hints
  videobuf2: add begin/end cpu_access callbacks to dma-contig
  videobuf2: add begin/end cpu_access callbacks to dma-sg
  videobuf2: do not sync buffers for DMABUF queues
  videobuf2: don't test db_attach in dma-contig prepare and finish
  videobuf2: don't test db_attach in dma-sg prepare and finish

 Documentation/media/uapi/v4l/buffer.rst       |  19 ++++
 .../media/uapi/v4l/vidioc-create-bufs.rst     |   8 +-
 .../media/uapi/v4l/vidioc-reqbufs.rst         |  19 +++-
 .../media/common/videobuf2/videobuf2-core.c   | 107 +++++++++++++-----
 .../common/videobuf2/videobuf2-dma-contig.c   |  39 ++++++-
 .../media/common/videobuf2/videobuf2-dma-sg.c |  30 +++--
 .../media/common/videobuf2/videobuf2-v4l2.c   |  59 +++++++++-
 drivers/media/dvb-core/dvb_vb2.c              |   2 +-
 drivers/media/v4l2-core/v4l2-ioctl.c          |   5 +-
 include/media/videobuf2-core.h                |  17 ++-
 include/uapi/linux/videodev2.h                |  11 +-
 11 files changed, 259 insertions(+), 57 deletions(-)

-- 
2.24.1.735.g03f4e72817-goog


^ permalink raw reply	[flat|nested] 56+ messages in thread

* [RFC][PATCH 01/15] videobuf2: add cache management members
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2019-12-17  3:20 ` [RFC][PATCH 02/15] videobuf2: handle V4L2 buffer cache flags Sergey Senozhatsky
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, 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 memebers ->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..026004180440 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: do not sync/invalidate cache from
+	 * 			buffer's ->prepare() callback.
+	 * need_cache_sync_on_finish: do not sync/invalidate cache from buffer's
+	 * 			->finish() callback.
 	 * 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.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 02/15] videobuf2: handle V4L2 buffer cache flags
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
  2019-12-17  3:20 ` [RFC][PATCH 01/15] videobuf2: add cache management members Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2020-01-10 10:24   ` Hans Verkuil
  2019-12-17  3:20 ` [RFC][PATCH 03/15] videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag Sergey Senozhatsky
                   ` (13 subsequent siblings)
  15 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, 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 skip ->finish() cache flush.

There are two possible alternative approaches:
- The first one is to move cache sync from ->finish() to dqbuf().
  But this breaks some drivers, that need to fix-up buffers before
  dequeueing them.

- The second one is to move ->finish() call from ->done() to dqbuf.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 .../media/common/videobuf2/videobuf2-v4l2.c   | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index e652f4318284..2fccfe2a57f8 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -337,6 +337,27 @@ 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)
+{
+	vb->need_cache_sync_on_prepare = 1;
+
+	if (q->dma_dir != DMA_TO_DEVICE)
+		vb->need_cache_sync_on_finish = 1;
+	else
+		vb->need_cache_sync_on_finish = 0;
+
+	if (!q->allow_cache_hints)
+		return;
+
+	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 +402,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);
-- 
2.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 03/15] videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
  2019-12-17  3:20 ` [RFC][PATCH 01/15] videobuf2: add cache management members Sergey Senozhatsky
  2019-12-17  3:20 ` [RFC][PATCH 02/15] videobuf2: handle V4L2 buffer cache flags Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2020-01-10  9:36   ` Hans Verkuil
  2019-12-17  3:20 ` [RFC][PATCH 04/15] videobuf2: add queue memory consistency parameter Sergey Senozhatsky
                   ` (12 subsequent siblings)
  15 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, 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 user-space will be able
to request consistent or non-consistent memory allocations.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 Documentation/media/uapi/v4l/buffer.rst | 19 +++++++++++++++++++
 include/uapi/linux/videodev2.h          |  2 ++
 2 files changed, 21 insertions(+)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index 9149b57728e5..b08b5609f5f3 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -705,6 +705,25 @@ Buffer Flags
 
 .. c:type:: v4l2_memory
 
+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
+      - Set DMA_ATTR_NON_CONSISTENT queue memory consistency bit,
+	so all queue buffers may be allocated in non-consistent memory.
+
 enum v4l2_memory
 ================
 
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 04481c717fee..d352997f2b62 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 {
 	/*
-- 
2.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 04/15] videobuf2: add queue memory consistency parameter
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (2 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 03/15] videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2020-01-10  9:47   ` Hans Verkuil
  2019-12-17  3:20 ` [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS Sergey Senozhatsky
                   ` (11 subsequent siblings)
  15 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, Sergey Senozhatsky

Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support.

Extend vb2_core_reqbufs() with queue memory consistency flag.
API permits queue's consistency attribute adjustment only if
the queue has no allocated buffers, not busy, and does not have
buffers waiting to be de-queued.

If user-space attempts to allocate a buffer with consistency
requirements which don't match queue's consistency model such
allocation requests will be failed.

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

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 4489744fbbd9..668c56df13f6 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -664,8 +664,16 @@ 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 (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 +728,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.
@@ -2498,7 +2507,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 +2564,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 +2584,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 2fccfe2a57f8..f1e88c9398c7 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -695,7 +695,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);
 
@@ -945,7 +945,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 026004180440..810af5cf5742 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -726,6 +726,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
@@ -750,7 +751,7 @@ 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
-- 
2.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (3 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 04/15] videobuf2: add queue memory consistency parameter Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2020-01-10  9:55   ` Hans Verkuil
  2019-12-17  3:20 ` [RFC][PATCH 06/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE_BUFS Sergey Senozhatsky
                   ` (10 subsequent siblings)
  15 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, Sergey Senozhatsky

This patch lets user-space to request a non-consistent memory
allocation during REQBUFS ioctl call. We use one bit of a
->reserved[1] member of struct v4l2_requestbuffers, which is
now renamed to ->flags.

There is just 1 four-byte reserved area in v4l2_requestbuffers
struct, therefore for backward compatibility ->reserved and
->flags were put into anonymous union.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 Documentation/media/uapi/v4l/vidioc-reqbufs.rst | 14 ++++++++++++--
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 14 ++++++++++++--
 drivers/media/v4l2-core/v4l2-ioctl.c            |  3 ---
 include/uapi/linux/videodev2.h                  |  5 ++++-
 4 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
index d0c643db477a..9b69a61d9fd4 100644
--- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
+++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
@@ -112,10 +112,20 @@ 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.
-    * - __u32
+    * - union
+      - (anonymous)
+    * -
+      - __u32
+      - ``flags``\ [1]
+      - Specifies additional buffer management attributes. E.g. when
+        ``V4L2_FLAG_MEMORY_NON_CONSISTENT`` set vb2 backends may be allocated
+        in non-consistent memory.
+    * -
+      - __u32
       - ``reserved``\ [1]
       - A place holder for future extensions. Drivers and applications
-	must set the array to zero.
+	must set the array to zero, unless application wants to specify
+        buffer management ``flags``.
 
 .. 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 f1e88c9398c7..0eabb589684f 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -693,9 +693,15 @@ 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 (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);
 
@@ -939,13 +945,17 @@ 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 (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 003b7422aeef..225d06819bce 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);
 }
 
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index d352997f2b62..73a4854f71bd 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -919,7 +919,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 */
-- 
2.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 06/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE_BUFS
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (4 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2020-01-10  9:59   ` Hans Verkuil
  2019-12-17  3:20 ` [RFC][PATCH 07/15] videobuf2: factor out planes prepare/finish functions Sergey Senozhatsky
                   ` (9 subsequent siblings)
  15 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, Sergey Senozhatsky

This patch lets user-space to request a non-consistent memory
allocation during CREATE_BUFS ioctl call. 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.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 .../media/uapi/v4l/vidioc-create-bufs.rst     |  8 +++++-
 .../media/common/videobuf2/videobuf2-core.c   | 27 +++++++++++++++----
 .../media/common/videobuf2/videobuf2-v4l2.c   |  7 ++++-
 drivers/media/v4l2-core/v4l2-ioctl.c          |  2 +-
 include/media/videobuf2-core.h                |  4 ++-
 include/uapi/linux/videodev2.h                |  3 ++-
 6 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
index bd08e4f77ae4..c56e80659b4a 100644
--- a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
+++ b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
@@ -121,7 +121,13 @@ 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. E.g. when
+        ``V4L2_FLAG_MEMORY_NON_CONSISTENT`` set vb2 backends may be allocated
+        in non-consistent memory.
+
+    * - __u32
+      - ``reserved``\ [6]
       - A place holder for future extensions. Drivers and applications
 	must set the array to zero.
 
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 668c56df13f6..d1012a24755d 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -812,9 +812,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] = { };
@@ -832,10 +844,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);
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 0eabb589684f..48d123a1ac2a 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -730,6 +730,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);
@@ -775,7 +776,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
 	for (i = 0; i < requested_planes; i++)
 		if (requested_sizes[i] == 0)
 			return -EINVAL;
-	return ret ? ret : vb2_core_create_bufs(q, create->memory,
+
+	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);
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 225d06819bce..793cb6534de4 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2012,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/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 810af5cf5742..5e5450bdabbd 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -757,6 +757,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
  * 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.
@@ -774,7 +775,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[]);
 
 /**
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 73a4854f71bd..82e2ded5a136 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2419,7 +2419,8 @@ struct v4l2_create_buffers {
 	__u32			memory;
 	struct v4l2_format	format;
 	__u32			capabilities;
-	__u32			reserved[7];
+	__u32			flags;
+	__u32			reserved[6];
 };
 
 /*
-- 
2.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 07/15] videobuf2: factor out planes prepare/finish functions
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (5 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 06/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE_BUFS Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2019-12-17  3:20 ` [RFC][PATCH 08/15] videobuf2: do not sync caches when we are allowed not to Sergey Senozhatsky
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, 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 d1012a24755d..492ed2577219 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.
@@ -948,7 +974,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;
@@ -968,12 +993,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) {
@@ -1298,7 +1319,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) {
@@ -1342,11 +1362,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;
 
@@ -1966,14 +1982,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.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 08/15] videobuf2: do not sync caches when we are allowed not to
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (6 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 07/15] videobuf2: factor out planes prepare/finish functions Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2019-12-17  3:20 ` [RFC][PATCH 09/15] videobuf2: check ->synced flag in prepare() and finish() Sergey Senozhatsky
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, 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 492ed2577219..4e81a8447472 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.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 09/15] videobuf2: check ->synced flag in prepare() and finish()
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (7 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 08/15] videobuf2: do not sync caches when we are allowed not to Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2019-12-17  3:20 ` [RFC][PATCH 10/15] videobuf2: let user-space know when driver supports cache hints Sergey Senozhatsky
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, 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 4e81a8447472..0ec10802e9f6 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,
@@ -1988,8 +1994,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.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 10/15] videobuf2: let user-space know when driver supports cache hints
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (8 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 09/15] videobuf2: check ->synced flag in prepare() and finish() Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2019-12-17  3:20 ` [RFC][PATCH 11/15] videobuf2: add begin/end cpu_access callbacks to dma-contig Sergey Senozhatsky
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, Sergey Senozhatsky

Add V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS to fill_buf_caps(), which
is set when queue supports user-space cache management hints.

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

diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
index 9b69a61d9fd4..13eab807fdfb 100644
--- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
+++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
@@ -166,6 +166,11 @@ 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
+      - This buffer supports :ref:`V4L2_BUF_FLAG_NO_CACHE_INVALIDATE` and
+        :ref:`V4L2_BUF_FLAG_NO_CACHE_CLEAN` user-space cache hints.
+
 
 Return Value
 ============
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 48d123a1ac2a..1762849288ae 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -684,6 +684,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)
+		*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 82e2ded5a136..b5d6b8d4ae93 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -932,6 +932,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.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 11/15] videobuf2: add begin/end cpu_access callbacks to dma-contig
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (9 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 10/15] videobuf2: let user-space know when driver supports cache hints Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2019-12-17  3:20 ` [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg Sergey Senozhatsky
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, 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.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (10 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 11/15] videobuf2: add begin/end cpu_access callbacks to dma-contig Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2020-01-10 10:13   ` Hans Verkuil
  2019-12-17  3:20 ` [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues Sergey Senozhatsky
                   ` (3 subsequent siblings)
  15 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, Sergey Senozhatsky

Provide begin_cpu_access() and end_cpu_access() dma_buf_ops
callbacks for cache synchronisation on exported buffers.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 6db60e9d5183..bfc99a0cb7b9 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -470,6 +470,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 +508,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.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (11 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2020-01-10 10:30   ` Hans Verkuil
  2019-12-17  3:20 ` [RFC][PATCH 14/15] videobuf2: don't test db_attach in dma-contig prepare and finish Sergey Senozhatsky
                   ` (2 subsequent siblings)
  15 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, Sergey Senozhatsky

DMA-exporter is supposed to handle cache syncs, so we can
avoid ->prepare()/->finish() syncs from videobuf2 core for
DMABUF buffers.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 1762849288ae..2b9d3318e6fb 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -341,8 +341,22 @@ static void set_buffer_cache_hints(struct vb2_queue *q,
 				   struct vb2_buffer *vb,
 				   struct v4l2_buffer *b)
 {
-	vb->need_cache_sync_on_prepare = 1;
+	/*
+	 * DMA exporter should take care of cache syncs, so we can avoid
+	 * explicit ->prepare()/->finish() syncs.
+	 */
+	if (q->memory == VB2_MEMORY_DMABUF) {
+		vb->need_cache_sync_on_finish = 0;
+		vb->need_cache_sync_on_prepare = 0;
+		return;
+	}
 
+	/*
+	 * For other ->memory types we always need ->prepare() cache
+	 * sync. ->finish() cache sync, however, can be avoided when queue
+	 * direction is TO_DEVICE.
+	 */
+	vb->need_cache_sync_on_prepare = 1;
 	if (q->dma_dir != DMA_TO_DEVICE)
 		vb->need_cache_sync_on_finish = 1;
 	else
-- 
2.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 14/15] videobuf2: don't test db_attach in dma-contig prepare and finish
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (12 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2020-01-10 10:32   ` Hans Verkuil
  2019-12-17  3:20 ` [RFC][PATCH 15/15] videobuf2: don't test db_attach in dma-sg " Sergey Senozhatsky
  2020-01-08  2:27 ` [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
  15 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, 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 ++----
 1 file changed, 2 insertions(+), 4 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);
-- 
2.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* [RFC][PATCH 15/15] videobuf2: don't test db_attach in dma-sg prepare and finish
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (13 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 14/15] videobuf2: don't test db_attach in dma-contig prepare and finish Sergey Senozhatsky
@ 2019-12-17  3:20 ` Sergey Senozhatsky
  2020-01-08  2:27 ` [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
  15 siblings, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2019-12-17  3:20 UTC (permalink / raw)
  To: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel, 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.

Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index bfc99a0cb7b9..1fd25eda0bf2 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -198,10 +198,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);
 }
@@ -211,10 +207,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.24.1.735.g03f4e72817-goog


^ permalink raw reply related	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags
  2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
                   ` (14 preceding siblings ...)
  2019-12-17  3:20 ` [RFC][PATCH 15/15] videobuf2: don't test db_attach in dma-sg " Sergey Senozhatsky
@ 2020-01-08  2:27 ` Sergey Senozhatsky
  15 siblings, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-08  2:27 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski, Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	linux-media, linux-kernel

On (19/12/17 12:20), Sergey Senozhatsky wrote:
> Hello,
> 
> 	RFC
> 
> 	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.

Gentle ping.

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 03/15] videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag
  2019-12-17  3:20 ` [RFC][PATCH 03/15] videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag Sergey Senozhatsky
@ 2020-01-10  9:36   ` Hans Verkuil
  2020-01-10  9:46     ` Sergey Senozhatsky
  0 siblings, 1 reply; 56+ messages in thread
From: Hans Verkuil @ 2020-01-10  9:36 UTC (permalink / raw)
  To: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media, linux-kernel

On 12/17/19 4:20 AM, 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 user-space will be able
> to request consistent or non-consistent memory allocations.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  Documentation/media/uapi/v4l/buffer.rst | 19 +++++++++++++++++++
>  include/uapi/linux/videodev2.h          |  2 ++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index 9149b57728e5..b08b5609f5f3 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -705,6 +705,25 @@ Buffer Flags
>  
>  .. c:type:: v4l2_memory
>  
> +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
> +      - Set DMA_ATTR_NON_CONSISTENT queue memory consistency bit,
> +	so all queue buffers may be allocated in non-consistent memory.

This needs much more extensive documentation. This is a userspace API,
and it shouldn't refer to a kernelspace API. Instead, explain what it
means from a user perspective. Also, how does this relate to the cache
buffer flags?

These things are tricky, so it is worth spending some time on writing
good documentation.

Regards,

	Hans

> +
>  enum v4l2_memory
>  ================
>  
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 04481c717fee..d352997f2b62 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 {
>  	/*
> 


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 03/15] videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag
  2020-01-10  9:36   ` Hans Verkuil
@ 2020-01-10  9:46     ` Sergey Senozhatsky
  0 siblings, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-10  9:46 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski,
	Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel

On (20/01/10 10:36), Hans Verkuil wrote:
[..]
> > diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> > index 9149b57728e5..b08b5609f5f3 100644
> > --- a/Documentation/media/uapi/v4l/buffer.rst
> > +++ b/Documentation/media/uapi/v4l/buffer.rst
> > @@ -705,6 +705,25 @@ Buffer Flags
> >  
> >  .. c:type:: v4l2_memory
> >  
> > +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
> > +      - Set DMA_ATTR_NON_CONSISTENT queue memory consistency bit,
> > +	so all queue buffers may be allocated in non-consistent memory.
> 
> This needs much more extensive documentation. This is a userspace API,
> and it shouldn't refer to a kernelspace API. Instead, explain what it
> means from a user perspective. Also, how does this relate to the cache
> buffer flags?
> 
> These things are tricky, so it is worth spending some time on writing
> good documentation.

Agreed. I'll give it a shot, will try to improve it.

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 04/15] videobuf2: add queue memory consistency parameter
  2019-12-17  3:20 ` [RFC][PATCH 04/15] videobuf2: add queue memory consistency parameter Sergey Senozhatsky
@ 2020-01-10  9:47   ` Hans Verkuil
  2020-01-22  2:05     ` Sergey Senozhatsky
  0 siblings, 1 reply; 56+ messages in thread
From: Hans Verkuil @ 2020-01-10  9:47 UTC (permalink / raw)
  To: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media, linux-kernel

On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support.
> 
> Extend vb2_core_reqbufs() with queue memory consistency flag.
> API permits queue's consistency attribute adjustment only if
> the queue has no allocated buffers, not busy, and does not have
> buffers waiting to be de-queued.

Actually, you can call vb2_core_reqbufs() when buffers are allocated:
it will free the old buffers, then allocate the new ones. So drop the
'has no allocated buffers' bit.

> 
> If user-space attempts to allocate a buffer with consistency
> requirements which don't match queue's consistency model such
> allocation requests will be failed.

Is this last paragraph right? I don't see any code for that.

BTW, a general comment about patches 4-6: I prefer if you changes
this to two patches: one that adds videobuf2-core.c support for
this for reqbufs and create_bufs, then another that wires up the
new V4L2 flag in videobuf2-v4l2.c.

It's easier to review that way.

Regards,

	Hans

> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 17 +++++++++++++----
>  drivers/media/common/videobuf2/videobuf2-v4l2.c |  4 ++--
>  drivers/media/dvb-core/dvb_vb2.c                |  2 +-
>  include/media/videobuf2-core.h                  |  3 ++-
>  4 files changed, 18 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 4489744fbbd9..668c56df13f6 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -664,8 +664,16 @@ 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 (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 +728,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.
> @@ -2498,7 +2507,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 +2564,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 +2584,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 2fccfe2a57f8..f1e88c9398c7 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -695,7 +695,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);
>  
> @@ -945,7 +945,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 026004180440..810af5cf5742 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -726,6 +726,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
> @@ -750,7 +751,7 @@ 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
> 


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS
  2019-12-17  3:20 ` [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS Sergey Senozhatsky
@ 2020-01-10  9:55   ` Hans Verkuil
  2020-01-22  2:18     ` Sergey Senozhatsky
  0 siblings, 1 reply; 56+ messages in thread
From: Hans Verkuil @ 2020-01-10  9:55 UTC (permalink / raw)
  To: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media, linux-kernel

On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> This patch lets user-space to request a non-consistent memory
> allocation during REQBUFS ioctl call. We use one bit of a
> ->reserved[1] member of struct v4l2_requestbuffers, which is
> now renamed to ->flags.
> 
> There is just 1 four-byte reserved area in v4l2_requestbuffers
> struct, therefore for backward compatibility ->reserved and
> ->flags were put into anonymous union.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  Documentation/media/uapi/v4l/vidioc-reqbufs.rst | 14 ++++++++++++--
>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 14 ++++++++++++--
>  drivers/media/v4l2-core/v4l2-ioctl.c            |  3 ---
>  include/uapi/linux/videodev2.h                  |  5 ++++-
>  4 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> index d0c643db477a..9b69a61d9fd4 100644
> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> @@ -112,10 +112,20 @@ 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.
> -    * - __u32
> +    * - union
> +      - (anonymous)
> +    * -
> +      - __u32
> +      - ``flags``\ [1]
> +      - Specifies additional buffer management attributes. E.g. when
> +        ``V4L2_FLAG_MEMORY_NON_CONSISTENT`` set vb2 backends may be allocated
> +        in non-consistent memory.

This should link to the table with these memory flags, rather than
effectively documenting V4L2_FLAG_MEMORY_NON_CONSISTENT again.

You also probably meant "vb2 buffers" rather than "vb2 backends".

> +    * -
> +      - __u32
>        - ``reserved``\ [1]
>        - A place holder for future extensions. Drivers and applications
> -	must set the array to zero.
> +	must set the array to zero, unless application wants to specify
> +        buffer management ``flags``.

I think support for this flag should be signaled as a V4L2_BUF_CAP capability.
If the capability is not set, then vb2 should set 'flags' to 0 to preserve the
old 'Drivers and applications must set the array to zero' behavior.

The documentation for 'reserved[1]' should be changed to something like:

	Kept for backwards compatibility. Use ``flags`` instead.

Regards,

	Hans
>  
>  .. 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 f1e88c9398c7..0eabb589684f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -693,9 +693,15 @@ 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 (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);
>  
> @@ -939,13 +945,17 @@ 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 (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 003b7422aeef..225d06819bce 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);
>  }
>  
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index d352997f2b62..73a4854f71bd 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -919,7 +919,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 */
> 


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 06/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE_BUFS
  2019-12-17  3:20 ` [RFC][PATCH 06/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE_BUFS Sergey Senozhatsky
@ 2020-01-10  9:59   ` Hans Verkuil
  2020-01-23  3:41     ` Sergey Senozhatsky
  0 siblings, 1 reply; 56+ messages in thread
From: Hans Verkuil @ 2020-01-10  9:59 UTC (permalink / raw)
  To: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media, linux-kernel

On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> This patch lets user-space to request a non-consistent memory
> allocation during CREATE_BUFS ioctl call. 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.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  .../media/uapi/v4l/vidioc-create-bufs.rst     |  8 +++++-
>  .../media/common/videobuf2/videobuf2-core.c   | 27 +++++++++++++++----
>  .../media/common/videobuf2/videobuf2-v4l2.c   |  7 ++++-
>  drivers/media/v4l2-core/v4l2-ioctl.c          |  2 +-
>  include/media/videobuf2-core.h                |  4 ++-
>  include/uapi/linux/videodev2.h                |  3 ++-
>  6 files changed, 41 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
> index bd08e4f77ae4..c56e80659b4a 100644
> --- a/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-create-bufs.rst
> @@ -121,7 +121,13 @@ 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. E.g. when
> +        ``V4L2_FLAG_MEMORY_NON_CONSISTENT`` set vb2 backends may be allocated
> +        in non-consistent memory.

Same comment as for patch 05/15.

> +
> +    * - __u32
> +      - ``reserved``\ [6]
>        - A place holder for future extensions. Drivers and applications
>  	must set the array to zero.
>  
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 668c56df13f6..d1012a24755d 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -812,9 +812,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;
> +}
> +

This belongs in patch 04/15. The commit log for that patch makes a lot
more sense if this code is moved there.

>  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] = { };
> @@ -832,10 +844,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;
>  	}

Ditto.

>  
>  	num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 0eabb589684f..48d123a1ac2a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -730,6 +730,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);
> @@ -775,7 +776,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>  	for (i = 0; i < requested_planes; i++)
>  		if (requested_sizes[i] == 0)
>  			return -EINVAL;
> -	return ret ? ret : vb2_core_create_bufs(q, create->memory,
> +
> +	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);

As mentioned before: we need a V4L2_BUF_CAP capability.

>  }
>  EXPORT_SYMBOL_GPL(vb2_create_bufs);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 225d06819bce..793cb6534de4 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -2012,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/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 810af5cf5742..5e5450bdabbd 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -757,6 +757,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>   * 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.
> @@ -774,7 +775,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[]);
>  
>  /**
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 73a4854f71bd..82e2ded5a136 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2419,7 +2419,8 @@ struct v4l2_create_buffers {
>  	__u32			memory;
>  	struct v4l2_format	format;
>  	__u32			capabilities;
> -	__u32			reserved[7];
> +	__u32			flags;
> +	__u32			reserved[6];
>  };
>  
>  /*
> 


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg
  2019-12-17  3:20 ` [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg Sergey Senozhatsky
@ 2020-01-10 10:13   ` Hans Verkuil
  2020-01-22  6:37     ` Sergey Senozhatsky
  2020-01-28  4:38     ` Tomasz Figa
  0 siblings, 2 replies; 56+ messages in thread
From: Hans Verkuil @ 2020-01-10 10:13 UTC (permalink / raw)
  To: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media, linux-kernel

On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> Provide begin_cpu_access() and end_cpu_access() dma_buf_ops
> callbacks for cache synchronisation on exported buffers.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> index 6db60e9d5183..bfc99a0cb7b9 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf)
>  	vb2_dma_sg_put(dbuf->priv);
>  }
>  

There is no corresponding vb2_sg_buffer_consistent function here.

Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs
argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no
effect on dma-sg buffers.

Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()?

I suspect it was just laziness in the past, and that it should be wired
up, just as for dma-contig.

Regards,

	Hans

> +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 +508,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,
> 


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 02/15] videobuf2: handle V4L2 buffer cache flags
  2019-12-17  3:20 ` [RFC][PATCH 02/15] videobuf2: handle V4L2 buffer cache flags Sergey Senozhatsky
@ 2020-01-10 10:24   ` Hans Verkuil
  2020-01-22  1:39     ` Sergey Senozhatsky
  0 siblings, 1 reply; 56+ messages in thread
From: Hans Verkuil @ 2020-01-10 10:24 UTC (permalink / raw)
  To: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media, linux-kernel

On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> 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 skip ->finish() cache flush.
> 
> There are two possible alternative approaches:
> - The first one is to move cache sync from ->finish() to dqbuf().
>   But this breaks some drivers, that need to fix-up buffers before
>   dequeueing them.
> 
> - The second one is to move ->finish() call from ->done() to dqbuf.

Please combine this patch with patch 13/15!

> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 22 +++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index e652f4318284..2fccfe2a57f8 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -337,6 +337,27 @@ 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)
> +{
> +	vb->need_cache_sync_on_prepare = 1;
> +
> +	if (q->dma_dir != DMA_TO_DEVICE)

What should be done when dma_dir == DMA_BIDIRECTIONAL?

> +		vb->need_cache_sync_on_finish = 1;
> +	else
> +		vb->need_cache_sync_on_finish = 0;
> +
> +	if (!q->allow_cache_hints)
> +		return;
> +
> +	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 +402,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);
> 

Regards,

	Hans

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues
  2019-12-17  3:20 ` [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues Sergey Senozhatsky
@ 2020-01-10 10:30   ` Hans Verkuil
  2020-01-22  5:05     ` Sergey Senozhatsky
  0 siblings, 1 reply; 56+ messages in thread
From: Hans Verkuil @ 2020-01-10 10:30 UTC (permalink / raw)
  To: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media, linux-kernel

On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> DMA-exporter is supposed to handle cache syncs, so we can
> avoid ->prepare()/->finish() syncs from videobuf2 core for
> DMABUF buffers.
> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 1762849288ae..2b9d3318e6fb 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -341,8 +341,22 @@ static void set_buffer_cache_hints(struct vb2_queue *q,
>  				   struct vb2_buffer *vb,
>  				   struct v4l2_buffer *b)
>  {
> -	vb->need_cache_sync_on_prepare = 1;
> +	/*
> +	 * DMA exporter should take care of cache syncs, so we can avoid
> +	 * explicit ->prepare()/->finish() syncs.
> +	 */
> +	if (q->memory == VB2_MEMORY_DMABUF) {
> +		vb->need_cache_sync_on_finish = 0;
> +		vb->need_cache_sync_on_prepare = 0;
> +		return;
> +	}
>  
> +	/*
> +	 * For other ->memory types we always need ->prepare() cache
> +	 * sync. ->finish() cache sync, however, can be avoided when queue
> +	 * direction is TO_DEVICE.
> +	 */
> +	vb->need_cache_sync_on_prepare = 1;

I'm trying to remember: what needs to be done in prepare()
for a capture buffer? I thought that for capture you only
needed to invalidate the cache in finish(), but nothing needs
to be done in the prepare().

Regards,

	Hans

>  	if (q->dma_dir != DMA_TO_DEVICE)
>  		vb->need_cache_sync_on_finish = 1;
>  	else
> 


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 14/15] videobuf2: don't test db_attach in dma-contig prepare and finish
  2019-12-17  3:20 ` [RFC][PATCH 14/15] videobuf2: don't test db_attach in dma-contig prepare and finish Sergey Senozhatsky
@ 2020-01-10 10:32   ` Hans Verkuil
  0 siblings, 0 replies; 56+ messages in thread
From: Hans Verkuil @ 2020-01-10 10:32 UTC (permalink / raw)
  To: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski
  Cc: Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media, linux-kernel

On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> 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.

Just combine this and the next patch into a single patch. No need to
split this up.

Regards,

	Hans

> 
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  drivers/media/common/videobuf2/videobuf2-dma-contig.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 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);
> 


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 02/15] videobuf2: handle V4L2 buffer cache flags
  2020-01-10 10:24   ` Hans Verkuil
@ 2020-01-22  1:39     ` Sergey Senozhatsky
  2020-01-22  2:53       ` Sergey Senozhatsky
  0 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-22  1:39 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski,
	Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel

Hi,

Sorry for the delayed.

On (20/01/10 11:24), Hans Verkuil wrote:
> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> > 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 skip ->finish() cache flush.
> > 
> > There are two possible alternative approaches:
> > - The first one is to move cache sync from ->finish() to dqbuf().
> >   But this breaks some drivers, that need to fix-up buffers before
> >   dequeueing them.
> > 
> > - The second one is to move ->finish() call from ->done() to dqbuf.
> 
> Please combine this patch with patch 13/15!

OK.

[..]
> >  }
> >  
> > +static void set_buffer_cache_hints(struct vb2_queue *q,
> > +				   struct vb2_buffer *vb,
> > +				   struct v4l2_buffer *b)
> > +{
> > +	vb->need_cache_sync_on_prepare = 1;
> > +
> > +	if (q->dma_dir != DMA_TO_DEVICE)
> 
> What should be done when dma_dir == DMA_BIDIRECTIONAL?

Well, ->need_cache_sync_on_prepare/->need_cache_sync_on_finish are set,
by default. If the queue supports user-space cache hints (driver has
set ->allow_cache_hints), then user-space can override cache behavior.
We probably cannot enforce any other behavior here. Am I missing
something?

> > +		vb->need_cache_sync_on_finish = 1;
> > +	else
> > +		vb->need_cache_sync_on_finish = 0;
> > +
> > +	if (!q->allow_cache_hints)
> > +		return;
> > +
> > +	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;
> > +}

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 04/15] videobuf2: add queue memory consistency parameter
  2020-01-10  9:47   ` Hans Verkuil
@ 2020-01-22  2:05     ` Sergey Senozhatsky
  2020-01-23 11:02       ` Hans Verkuil
  0 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-22  2:05 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski,
	Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel

On (20/01/10 10:47), Hans Verkuil wrote:
> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> > Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support.
> >
> > Extend vb2_core_reqbufs() with queue memory consistency flag.
> > API permits queue's consistency attribute adjustment only if
> > the queue has no allocated buffers, not busy, and does not have
> > buffers waiting to be de-queued.
>
> Actually, you can call vb2_core_reqbufs() when buffers are allocated:
> it will free the old buffers, then allocate the new ones.
> So drop the 'has no allocated buffers' bit.

Well, the wording, basically, follows the existing vb2_core_reqbufs()
behavior "queue memory type"-wise. What I'm trying to say:

[..]
int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
		bool consistent_mem, unsigned int *count)
{
	unsigned int num_buffers, allocated_buffers, num_planes = 0;
	unsigned plane_sizes[VB2_MAX_PLANES] = { };
	unsigned int i;
	int ret;

	if (q->streaming) {
		dprintk(1, "streaming active\n");
		return -EBUSY;
	}

	if (q->waiting_in_dqbuf && *count) {
		dprintk(1, "another dup()ped fd is waiting for a buffer\n");
		return -EBUSY;
	}

	if (*count == 0 || q->num_buffers != 0 ||
	    (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
		/*
		 * We already have buffers allocated, so first check if they
		 * are not in use and can be freed.
		 */
		mutex_lock(&q->mmap_lock);
		if (debug && q->memory == VB2_MEMORY_MMAP &&
		    __buffers_in_use(q))
			dprintk(1, "memory in use, orphaning buffers\n");

		/*
		 * Call queue_cancel to clean up any buffers in the
		 * QUEUED state which is possible if buffers were prepared or
		 * queued without ever calling STREAMON.
		 */
		__vb2_queue_cancel(q);
		ret = __vb2_queue_free(q, q->num_buffers);
		mutex_unlock(&q->mmap_lock);
		if (ret)
			return ret;

		/*
		 * In case of REQBUFS(0) return immediately without calling
		 * driver's queue_setup() callback and allocating resources.
		 */
		if (*count == 0)
			return 0;
	}

	/*
	 * Make sure the requested values and current defaults are sane.
	 */
	WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
	num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
	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);

[..]

So we set/change queue consistency attribute when we set/change
queue memory type. Is there a use case for more flexibility when
it comes to queue consistency?

> > If user-space attempts to allocate a buffer with consistency
> > requirements which don't match queue's consistency model such
> > allocation requests will be failed.
>
> Is this last paragraph right? I don't see any code for that.

Yeah, this was more about the general direction. The actual code
was added later in the series.

> BTW, a general comment about patches 4-6: I prefer if you changes
> this to two patches: one that adds videobuf2-core.c support for
> this for reqbufs and create_bufs, then another that wires up the
> new V4L2 flag in videobuf2-v4l2.c.

I'll take a look.

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS
  2020-01-10  9:55   ` Hans Verkuil
@ 2020-01-22  2:18     ` Sergey Senozhatsky
  2020-01-22  3:48       ` Sergey Senozhatsky
  0 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-22  2:18 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski,
	Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel

On (20/01/10 10:55), Hans Verkuil wrote:
> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> > This patch lets user-space to request a non-consistent memory
> > allocation during REQBUFS ioctl call. We use one bit of a
> > ->reserved[1] member of struct v4l2_requestbuffers, which is
> > now renamed to ->flags.
> > 
> > There is just 1 four-byte reserved area in v4l2_requestbuffers
> > struct, therefore for backward compatibility ->reserved and
> > ->flags were put into anonymous union.
> > 
> > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > ---
> >  Documentation/media/uapi/v4l/vidioc-reqbufs.rst | 14 ++++++++++++--
> >  drivers/media/common/videobuf2/videobuf2-v4l2.c | 14 ++++++++++++--
> >  drivers/media/v4l2-core/v4l2-ioctl.c            |  3 ---
> >  include/uapi/linux/videodev2.h                  |  5 ++++-
> >  4 files changed, 28 insertions(+), 8 deletions(-)
> > 
> > diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> > index d0c643db477a..9b69a61d9fd4 100644
> > --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> > +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> > @@ -112,10 +112,20 @@ 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.
> > -    * - __u32
> > +    * - union
> > +      - (anonymous)
> > +    * -
> > +      - __u32
> > +      - ``flags``\ [1]
> > +      - Specifies additional buffer management attributes. E.g. when
> > +        ``V4L2_FLAG_MEMORY_NON_CONSISTENT`` set vb2 backends may be allocated
> > +        in non-consistent memory.
> 
> This should link to the table with these memory flags, rather than
> effectively documenting V4L2_FLAG_MEMORY_NON_CONSISTENT again.

OK.

> You also probably meant "vb2 buffers" rather than "vb2 backends".

Thanks.

> 
> > +    * -
> > +      - __u32
> >        - ``reserved``\ [1]
> >        - A place holder for future extensions. Drivers and applications
> > -	must set the array to zero.
> > +	must set the array to zero, unless application wants to specify
> > +        buffer management ``flags``.
> 
> I think support for this flag should be signaled as a V4L2_BUF_CAP capability.
> If the capability is not set, then vb2 should set 'flags' to 0 to preserve the
> old 'Drivers and applications must set the array to zero' behavior.

The patch set adds V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS towards the end of the
series, I guess I can shuffle the patches and change the wording here.

> The documentation for 'reserved[1]' should be changed to something like:
> 
> 	Kept for backwards compatibility. Use ``flags`` instead.

OK.

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 02/15] videobuf2: handle V4L2 buffer cache flags
  2020-01-22  1:39     ` Sergey Senozhatsky
@ 2020-01-22  2:53       ` Sergey Senozhatsky
  2020-01-28  4:35         ` Tomasz Figa
  0 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-22  2:53 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski, Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	linux-media, linux-kernel, Sergey Senozhatsky

On (20/01/22 10:39), Sergey Senozhatsky wrote:
> [..]
> > >  }
> > >  
> > > +static void set_buffer_cache_hints(struct vb2_queue *q,
> > > +				   struct vb2_buffer *vb,
> > > +				   struct v4l2_buffer *b)
> > > +{
> > > +	vb->need_cache_sync_on_prepare = 1;
> > > +
> > > +	if (q->dma_dir != DMA_TO_DEVICE)
> > 
> > What should be done when dma_dir == DMA_BIDIRECTIONAL?
> 

[..]

> We probably cannot enforce any other behavior here. Am I missing
> something?

Never mind. I got your point.

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS
  2020-01-22  2:18     ` Sergey Senozhatsky
@ 2020-01-22  3:48       ` Sergey Senozhatsky
  2020-01-23 11:08         ` Hans Verkuil
  0 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-22  3:48 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski, Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	linux-media, linux-kernel, Sergey Senozhatsky

On (20/01/22 11:18), Sergey Senozhatsky wrote:
[..]
> > > +    * -
> > > +      - __u32
> > >        - ``reserved``\ [1]
> > >        - A place holder for future extensions. Drivers and applications
> > > -	must set the array to zero.
> > > +	must set the array to zero, unless application wants to specify
> > > +        buffer management ``flags``.
> > 
> > I think support for this flag should be signaled as a V4L2_BUF_CAP capability.
> > If the capability is not set, then vb2 should set 'flags' to 0 to preserve the
> > old 'Drivers and applications must set the array to zero' behavior.
> 
> The patch set adds V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS towards the end of the
> series, I guess I can shuffle the patches and change the wording here.

Or I can add separate queue flag and V4L2_BUF_CAP:

struct vb2_queue {
...
	allow_cache_hints:1
+	allow_consistency_hints:1
...
}

and then have CAP_SUPPORTS_CACHE_HINTS/CAP_SUPPORTS_CONSISTENCY_HINTS.

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues
  2020-01-10 10:30   ` Hans Verkuil
@ 2020-01-22  5:05     ` Sergey Senozhatsky
  2020-01-23 11:35       ` Hans Verkuil
  0 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-22  5:05 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski,
	Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel

On (20/01/10 11:30), Hans Verkuil wrote:
[..]
> > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > index 1762849288ae..2b9d3318e6fb 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > @@ -341,8 +341,22 @@ static void set_buffer_cache_hints(struct vb2_queue *q,
> >  				   struct vb2_buffer *vb,
> >  				   struct v4l2_buffer *b)
> >  {
> > -	vb->need_cache_sync_on_prepare = 1;
> > +	/*
> > +	 * DMA exporter should take care of cache syncs, so we can avoid
> > +	 * explicit ->prepare()/->finish() syncs.
> > +	 */
> > +	if (q->memory == VB2_MEMORY_DMABUF) {
> > +		vb->need_cache_sync_on_finish = 0;
> > +		vb->need_cache_sync_on_prepare = 0;
> > +		return;
> > +	}
> >  
> > +	/*
> > +	 * For other ->memory types we always need ->prepare() cache
> > +	 * sync. ->finish() cache sync, however, can be avoided when queue
> > +	 * direction is TO_DEVICE.
> > +	 */
> > +	vb->need_cache_sync_on_prepare = 1;
> 
> I'm trying to remember: what needs to be done in prepare()
> for a capture buffer? I thought that for capture you only
> needed to invalidate the cache in finish(), but nothing needs
> to be done in the prepare().

Hmm. Not sure. A precaution in case if user-space wrote to that buffer?

+	if (q->dma_dir == DMA_FROM_DEVICE)
+		q->need_cache_sync_on_prepare = 0;

?

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg
  2020-01-10 10:13   ` Hans Verkuil
@ 2020-01-22  6:37     ` Sergey Senozhatsky
  2020-01-28  4:38     ` Tomasz Figa
  1 sibling, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-22  6:37 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski,
	Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel

On (20/01/10 11:13), Hans Verkuil wrote:
> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> > Provide begin_cpu_access() and end_cpu_access() dma_buf_ops
> > callbacks for cache synchronisation on exported buffers.
> > 
> > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > ---
> >  .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > index 6db60e9d5183..bfc99a0cb7b9 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf)
> >  	vb2_dma_sg_put(dbuf->priv);
> >  }
> >  
> 
> There is no corresponding vb2_sg_buffer_consistent function here.
> 
> Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs
> argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no
> effect on dma-sg buffers.
> 
> Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()?

Laziness.

> I suspect it was just laziness in the past, and that it should be wired
> up, just as for dma-contig.

OK, I can include it into v2.

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 06/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE_BUFS
  2020-01-10  9:59   ` Hans Verkuil
@ 2020-01-23  3:41     ` Sergey Senozhatsky
  2020-01-23 11:41       ` Hans Verkuil
  0 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-23  3:41 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski,
	Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel

On (20/01/10 10:59), Hans Verkuil wrote:
[..]
> >  
> >  	num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
> > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > index 0eabb589684f..48d123a1ac2a 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > @@ -730,6 +730,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);
> > @@ -775,7 +776,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
> >  	for (i = 0; i < requested_planes; i++)
> >  		if (requested_sizes[i] == 0)
> >  			return -EINVAL;
> > -	return ret ? ret : vb2_core_create_bufs(q, create->memory,
> > +
> > +	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);
> 
> As mentioned before: we need a V4L2_BUF_CAP capability.

I can add V4L2_BUF_CAP for memory consistency. Isn't it just q->memory
property though? User space may request MMAP consistent memory or MMAP
inconsistent memory.

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 04/15] videobuf2: add queue memory consistency parameter
  2020-01-22  2:05     ` Sergey Senozhatsky
@ 2020-01-23 11:02       ` Hans Verkuil
  2020-01-24  2:04         ` Sergey Senozhatsky
  0 siblings, 1 reply; 56+ messages in thread
From: Hans Verkuil @ 2020-01-23 11:02 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski, Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	linux-media, linux-kernel

On 1/22/20 3:05 AM, Sergey Senozhatsky wrote:
> On (20/01/10 10:47), Hans Verkuil wrote:
>> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
>>> Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support.
>>>
>>> Extend vb2_core_reqbufs() with queue memory consistency flag.
>>> API permits queue's consistency attribute adjustment only if
>>> the queue has no allocated buffers, not busy, and does not have
>>> buffers waiting to be de-queued.
>>
>> Actually, you can call vb2_core_reqbufs() when buffers are allocated:
>> it will free the old buffers, then allocate the new ones.
>> So drop the 'has no allocated buffers' bit.
> 
> Well, the wording, basically, follows the existing vb2_core_reqbufs()
> behavior "queue memory type"-wise. What I'm trying to say:

How about this commit log replacement of the first paragraph:

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

The bits about 'only if the queue has no allocated buffers, not busy, and does
not have buffers waiting to be de-queued.' is really irrelevant and confusing
(at least to me!).

Regards,

	Hans

> 
> [..]
> int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> 		bool consistent_mem, unsigned int *count)
> {
> 	unsigned int num_buffers, allocated_buffers, num_planes = 0;
> 	unsigned plane_sizes[VB2_MAX_PLANES] = { };
> 	unsigned int i;
> 	int ret;
> 
> 	if (q->streaming) {
> 		dprintk(1, "streaming active\n");
> 		return -EBUSY;
> 	}
> 
> 	if (q->waiting_in_dqbuf && *count) {
> 		dprintk(1, "another dup()ped fd is waiting for a buffer\n");
> 		return -EBUSY;
> 	}
> 
> 	if (*count == 0 || q->num_buffers != 0 ||
> 	    (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory)) {
> 		/*
> 		 * We already have buffers allocated, so first check if they
> 		 * are not in use and can be freed.
> 		 */
> 		mutex_lock(&q->mmap_lock);
> 		if (debug && q->memory == VB2_MEMORY_MMAP &&
> 		    __buffers_in_use(q))
> 			dprintk(1, "memory in use, orphaning buffers\n");
> 
> 		/*
> 		 * Call queue_cancel to clean up any buffers in the
> 		 * QUEUED state which is possible if buffers were prepared or
> 		 * queued without ever calling STREAMON.
> 		 */
> 		__vb2_queue_cancel(q);
> 		ret = __vb2_queue_free(q, q->num_buffers);
> 		mutex_unlock(&q->mmap_lock);
> 		if (ret)
> 			return ret;
> 
> 		/*
> 		 * In case of REQBUFS(0) return immediately without calling
> 		 * driver's queue_setup() callback and allocating resources.
> 		 */
> 		if (*count == 0)
> 			return 0;
> 	}
> 
> 	/*
> 	 * Make sure the requested values and current defaults are sane.
> 	 */
> 	WARN_ON(q->min_buffers_needed > VB2_MAX_FRAME);
> 	num_buffers = max_t(unsigned int, *count, q->min_buffers_needed);
> 	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);
> 
> [..]
> 
> So we set/change queue consistency attribute when we set/change
> queue memory type. Is there a use case for more flexibility when
> it comes to queue consistency?
> 
>>> If user-space attempts to allocate a buffer with consistency
>>> requirements which don't match queue's consistency model such
>>> allocation requests will be failed.
>>
>> Is this last paragraph right? I don't see any code for that.
> 
> Yeah, this was more about the general direction. The actual code
> was added later in the series.
> 
>> BTW, a general comment about patches 4-6: I prefer if you changes
>> this to two patches: one that adds videobuf2-core.c support for
>> this for reqbufs and create_bufs, then another that wires up the
>> new V4L2 flag in videobuf2-v4l2.c.
> 
> I'll take a look.
> 
> 	-ss
> 


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS
  2020-01-22  3:48       ` Sergey Senozhatsky
@ 2020-01-23 11:08         ` Hans Verkuil
  2020-01-28  4:45           ` Tomasz Figa
  0 siblings, 1 reply; 56+ messages in thread
From: Hans Verkuil @ 2020-01-23 11:08 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski, Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	linux-media, linux-kernel

On 1/22/20 4:48 AM, Sergey Senozhatsky wrote:
> On (20/01/22 11:18), Sergey Senozhatsky wrote:
> [..]
>>>> +    * -
>>>> +      - __u32
>>>>        - ``reserved``\ [1]
>>>>        - A place holder for future extensions. Drivers and applications
>>>> -	must set the array to zero.
>>>> +	must set the array to zero, unless application wants to specify
>>>> +        buffer management ``flags``.
>>>
>>> I think support for this flag should be signaled as a V4L2_BUF_CAP capability.
>>> If the capability is not set, then vb2 should set 'flags' to 0 to preserve the
>>> old 'Drivers and applications must set the array to zero' behavior.
>>
>> The patch set adds V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS towards the end of the
>> series, I guess I can shuffle the patches and change the wording here.
> 
> Or I can add separate queue flag and V4L2_BUF_CAP:
> 
> struct vb2_queue {
> ...
> 	allow_cache_hints:1
> +	allow_consistency_hints:1
> ...
> }
> 
> and then have CAP_SUPPORTS_CACHE_HINTS/CAP_SUPPORTS_CONSISTENCY_HINTS.

Don't these two go hand-in-hand? I.e. either neither are supported, or
both are supported? If so, then one queue flag is sufficient.

Regards,

	Hans

> 
> 	-ss
> 


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues
  2020-01-22  5:05     ` Sergey Senozhatsky
@ 2020-01-23 11:35       ` Hans Verkuil
  2020-01-24  2:25         ` Sergey Senozhatsky
  2020-01-28  7:19         ` Tomasz Figa
  0 siblings, 2 replies; 56+ messages in thread
From: Hans Verkuil @ 2020-01-23 11:35 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski, Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	linux-media, linux-kernel

On 1/22/20 6:05 AM, Sergey Senozhatsky wrote:
> On (20/01/10 11:30), Hans Verkuil wrote:
> [..]
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> index 1762849288ae..2b9d3318e6fb 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> @@ -341,8 +341,22 @@ static void set_buffer_cache_hints(struct vb2_queue *q,
>>>  				   struct vb2_buffer *vb,
>>>  				   struct v4l2_buffer *b)
>>>  {
>>> -	vb->need_cache_sync_on_prepare = 1;
>>> +	/*
>>> +	 * DMA exporter should take care of cache syncs, so we can avoid
>>> +	 * explicit ->prepare()/->finish() syncs.
>>> +	 */
>>> +	if (q->memory == VB2_MEMORY_DMABUF) {
>>> +		vb->need_cache_sync_on_finish = 0;
>>> +		vb->need_cache_sync_on_prepare = 0;
>>> +		return;
>>> +	}
>>>  
>>> +	/*
>>> +	 * For other ->memory types we always need ->prepare() cache
>>> +	 * sync. ->finish() cache sync, however, can be avoided when queue
>>> +	 * direction is TO_DEVICE.
>>> +	 */
>>> +	vb->need_cache_sync_on_prepare = 1;
>>
>> I'm trying to remember: what needs to be done in prepare()
>> for a capture buffer? I thought that for capture you only
>> needed to invalidate the cache in finish(), but nothing needs
>> to be done in the prepare().
> 
> Hmm. Not sure. A precaution in case if user-space wrote to that buffer?

But whatever was written in the buffer is going to be overwritten anyway.

Unless I am mistaken the current situation is that the cache syncs are done
in both prepare and finish, regardless of the DMA direction.

I would keep that behavior to avoid introducing any unexpected regressions.

Then, if q->allow_cache_hint is set, then default to a cache sync (cache clean)
in the prepare for OUTPUT buffers and a cache sync (cache invalidate) in the
finish for CAPTURE buffers.

This also means that any drivers that want to access a buffer in between the
prepare...finish calls will need to do a begin/end_cpu_access. But that's a
separate matter.

Regards,

	Hans

> 
> +	if (q->dma_dir == DMA_FROM_DEVICE)
> +		q->need_cache_sync_on_prepare = 0;
> 
> ?
> 
> 	-ss
> 


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 06/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE_BUFS
  2020-01-23  3:41     ` Sergey Senozhatsky
@ 2020-01-23 11:41       ` Hans Verkuil
  2020-01-24  1:28         ` Sergey Senozhatsky
  0 siblings, 1 reply; 56+ messages in thread
From: Hans Verkuil @ 2020-01-23 11:41 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski, Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	linux-media, linux-kernel

On 1/23/20 4:41 AM, Sergey Senozhatsky wrote:
> On (20/01/10 10:59), Hans Verkuil wrote:
> [..]
>>>  
>>>  	num_buffers = min(*count, VB2_MAX_FRAME - q->num_buffers);
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> index 0eabb589684f..48d123a1ac2a 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>> @@ -730,6 +730,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);
>>> @@ -775,7 +776,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>>>  	for (i = 0; i < requested_planes; i++)
>>>  		if (requested_sizes[i] == 0)
>>>  			return -EINVAL;
>>> -	return ret ? ret : vb2_core_create_bufs(q, create->memory,
>>> +
>>> +	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);
>>
>> As mentioned before: we need a V4L2_BUF_CAP capability.
> 
> I can add V4L2_BUF_CAP for memory consistency. Isn't it just q->memory
> property though? User space may request MMAP consistent memory or MMAP
> inconsistent memory.

So instead of adding a flag we add a V4L2_MEMORY_MMAP_NON_CONSISTENT memory
type and add a V4L2_BUF_CAP_SUPPORTS_MMAP_NON_CONSISTENT to signal support
for this?

I like that better than a flag. It also automatically enforces that all
buffers must be of that type.

Regards,

	Hans

> 
> 	-ss
> 


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 06/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE_BUFS
  2020-01-23 11:41       ` Hans Verkuil
@ 2020-01-24  1:28         ` Sergey Senozhatsky
  0 siblings, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-24  1:28 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski,
	Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel

On (20/01/23 12:41), Hans Verkuil wrote:
[..]
> >>>  
> >>>  	fill_buf_caps(q, &create->capabilities);
> >>> @@ -775,7 +776,11 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
> >>>  	for (i = 0; i < requested_planes; i++)
> >>>  		if (requested_sizes[i] == 0)
> >>>  			return -EINVAL;
> >>> -	return ret ? ret : vb2_core_create_bufs(q, create->memory,
> >>> +
> >>> +	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);
> >>
> >> As mentioned before: we need a V4L2_BUF_CAP capability.
> > 
> > I can add V4L2_BUF_CAP for memory consistency. Isn't it just q->memory
> > property though? User space may request MMAP consistent memory or MMAP
> > inconsistent memory.
> 
> So instead of adding a flag we add a V4L2_MEMORY_MMAP_NON_CONSISTENT memory
> type and add a V4L2_BUF_CAP_SUPPORTS_MMAP_NON_CONSISTENT to signal support
> for this?
> 
> I like that better than a flag. It also automatically enforces that all
> buffers must be of that type.

Yes, we had this idea as well. The conclusion was it makes the patch
set bigger and harder to verify and review. Passing memory consistency
attribute via ->flags was the shortest path at the end. Namely due to all
those numerous places that test q->memory:

 455                 if (q->memory == VB2_MEMORY_MMAP)
 456                         __vb2_buf_mem_free(vb);
 457                 else if (q->memory == VB2_MEMORY_DMABUF)
 458                         __vb2_buf_dmabuf_put(vb);
 459                 else
 460                         __vb2_buf_userptr_put(vb);

[..]

 737                 mutex_lock(&q->mmap_lock);
 738                 if (debug && q->memory == VB2_MEMORY_MMAP &&
 739                     __buffers_in_use(q))
 740                         dprintk(1, "memory in use, orphaning buffers\n");

[..]

etc.

As a workaround we looked at the idea that V4L2_MEMORY_MMAP_NON_CONSISTENT
flag might make sense only on the very high level - when user space requests
V4L2_MEMORY_MMAP_NON_CONSISTENT then we simply set DMA attribute and then
"downgrade" requested V4L2_MEMORY_MMAP_NON_CONSISTENT memory type to
V4L2_MEMORY_MMAP.


Something like this.

---

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 4489744fbbd9..60afbfcca995 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -803,6 +803,14 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
 }
 EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
 
+static void __set_queue_consistency(struct vb2_queue *q, bool consistent_mem)
+{
+	if (consistent_mem)
+		q->dma_attrs &= ~DMA_ATTR_NON_CONSISTENT;
+	else
+		q->dma_attrs |= DMA_ATTR_NON_CONSISTENT;
+}
+
 int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 		unsigned int *count, unsigned requested_planes,
 		const unsigned requested_sizes[])
@@ -810,6 +818,10 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 	unsigned int num_planes = 0, num_buffers, allocated_buffers;
 	unsigned plane_sizes[VB2_MAX_PLANES] = { };
 	int ret;
+	bool consistent_mem = (memory == V4L2_MEMORY_MMAP_NON_CONSISTENT);
+
+	if (consistent_mem)
+		memory = V4L2_MEMORY_MMAP;
 
 	if (q->num_buffers == VB2_MAX_FRAME) {
 		dprintk(1, "maximum number of buffers already allocated\n");
@@ -822,6 +834,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 			return -EBUSY;
 		}
 		memset(q->alloc_devs, 0, sizeof(q->alloc_devs));
+		__set_queue_consistency(q, consistent_mem);
 		q->memory = memory;
 		q->waiting_for_buffers = !q->is_output;
 	} else if (q->memory != memory) {

^ permalink raw reply related	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 04/15] videobuf2: add queue memory consistency parameter
  2020-01-23 11:02       ` Hans Verkuil
@ 2020-01-24  2:04         ` Sergey Senozhatsky
  0 siblings, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-24  2:04 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski,
	Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel

On (20/01/23 12:02), Hans Verkuil wrote:
> On 1/22/20 3:05 AM, Sergey Senozhatsky wrote:
> > On (20/01/10 10:47), Hans Verkuil wrote:
> >> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> >>> Preparations for future V4L2_FLAG_MEMORY_NON_CONSISTENT support.
> >>>
> >>> Extend vb2_core_reqbufs() with queue memory consistency flag.
> >>> API permits queue's consistency attribute adjustment only if
> >>> the queue has no allocated buffers, not busy, and does not have
> >>> buffers waiting to be de-queued.
> >>
> >> Actually, you can call vb2_core_reqbufs() when buffers are allocated:
> >> it will free the old buffers, then allocate the new ones.
> >> So drop the 'has no allocated buffers' bit.
> > 
> > Well, the wording, basically, follows the existing vb2_core_reqbufs()
> > behavior "queue memory type"-wise. What I'm trying to say:
> 
> How about this commit log replacement of the first paragraph:
> 
> "Extend vb2_core_reqbufs() with queue memory consistency flag that is
> applied to the newly allocated buffers."

Looks good.

> The bits about 'only if the queue has no allocated buffers, not busy, and does
> not have buffers waiting to be de-queued.' is really irrelevant and confusing
> (at least to me!).

Agreed, those bits describe implementation details which can change.
Better get rid of them.

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues
  2020-01-23 11:35       ` Hans Verkuil
@ 2020-01-24  2:25         ` Sergey Senozhatsky
  2020-01-24  7:32           ` Sergey Senozhatsky
  2020-01-28  7:19         ` Tomasz Figa
  1 sibling, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-24  2:25 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Tomasz Figa,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski,
	Sakari Ailus, Laurent Pinchart, Pawel Osciak, linux-media,
	linux-kernel

On (20/01/23 12:35), Hans Verkuil wrote:
> On 1/22/20 6:05 AM, Sergey Senozhatsky wrote:
> > On (20/01/10 11:30), Hans Verkuil wrote:

[..]

> But whatever was written in the buffer is going to be overwritten anyway.
> 
> Unless I am mistaken the current situation is that the cache syncs are done
> in both prepare and finish, regardless of the DMA direction.
> 
> I would keep that behavior to avoid introducing any unexpected regressions.

OK.

> Then, if q->allow_cache_hint is set, then default to a cache sync (cache clean)
> in the prepare for OUTPUT buffers and a cache sync (cache invalidate) in the
> finish for CAPTURE buffers.

We alter default cache sync behaviour based both on queue ->memory
type and queue ->dma_dir. Shall both of those cases depend on
->allow_cache_hints, or q->memory can be independent?

static void set_buffer_cache_hints(struct vb2_queue *q,
				   struct vb2_buffer *vb,
				   struct v4l2_buffer *b)
{
	if (!q->allow_cache_hints)
		return;

	/*
	 * 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;
	}

	/*
	 * ->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;
	else
		vb->need_cache_sync_on_finish = 1;

	/*
	 * ->prepare() cache sync can be avoided when queue direction is
	 * FROM_DEVICE.
	 */
	if (q->dma_dir == DMA_FROM_DEVICE)
		vb->need_cache_sync_on_prepare = 0;
	else
		vb->need_cache_sync_on_prepare = 1;

	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;
}

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues
  2020-01-24  2:25         ` Sergey Senozhatsky
@ 2020-01-24  7:32           ` Sergey Senozhatsky
  2020-01-28  7:22             ` Tomasz Figa
  0 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-24  7:32 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Hans Verkuil, Tomasz Figa, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski, Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	linux-media, linux-kernel, Sergey Senozhatsky

On (20/01/24 11:25), Sergey Senozhatsky wrote:
[..]
>
> > Then, if q->allow_cache_hint is set, then default to a cache sync (cache clean)
> > in the prepare for OUTPUT buffers and a cache sync (cache invalidate) in the
> > finish for CAPTURE buffers.
>
> We alter default cache sync behaviour based both on queue ->memory
> type and queue ->dma_dir. Shall both of those cases depend on
> ->allow_cache_hints, or q->memory can be independent?
>
> static void set_buffer_cache_hints(struct vb2_queue *q,
> 				   struct vb2_buffer *vb,
> 				   struct v4l2_buffer *b)
> {
> 	if (!q->allow_cache_hints)
> 		return;
>
> 	/*
> 	 * 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;
> 	}

I personally would prefer this to be above ->allow_cache_hints check,
IOW to be independent. Because we need to skip flush/invalidation for
DMABUF anyway, that's what allocators do in ->prepare() and ->finish()
currently.

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 02/15] videobuf2: handle V4L2 buffer cache flags
  2020-01-22  2:53       ` Sergey Senozhatsky
@ 2020-01-28  4:35         ` Tomasz Figa
  0 siblings, 0 replies; 56+ messages in thread
From: Tomasz Figa @ 2020-01-28  4:35 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Hans Verkuil, Hans Verkuil, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski, Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	Linux Media Mailing List, Linux Kernel Mailing List

On Wed, Jan 22, 2020 at 11:53 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (20/01/22 10:39), Sergey Senozhatsky wrote:
> > [..]
> > > >  }
> > > >
> > > > +static void set_buffer_cache_hints(struct vb2_queue *q,
> > > > +                            struct vb2_buffer *vb,
> > > > +                            struct v4l2_buffer *b)
> > > > +{
> > > > + vb->need_cache_sync_on_prepare = 1;
> > > > +
> > > > + if (q->dma_dir != DMA_TO_DEVICE)
> > >
> > > What should be done when dma_dir == DMA_BIDIRECTIONAL?
> >
>
> [..]
>
> > We probably cannot enforce any other behavior here. Am I missing
> > something?
>
> Never mind. I got your point.

DMA_BIDIRECTIONAL by default needs sync on both prepare and finish.
need_cache_sync_on_prepare is initialized to 1. Since
DMA_BIDIRECTIONAL != DMA_TO_DEVICE, need_cache_sync_on_finish would
also be set to 1. Is anything still missing?

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg
  2020-01-10 10:13   ` Hans Verkuil
  2020-01-22  6:37     ` Sergey Senozhatsky
@ 2020-01-28  4:38     ` Tomasz Figa
  2020-01-28  8:36       ` Hans Verkuil
  1 sibling, 1 reply; 56+ messages in thread
From: Tomasz Figa @ 2020-01-28  4:38 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Mauro Carvalho Chehab,
	Kyungmin Park, Marek Szyprowski, Sakari Ailus, Laurent Pinchart,
	Pawel Osciak, Linux Media Mailing List,
	Linux Kernel Mailing List

On Fri, Jan 10, 2020 at 7:13 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> > Provide begin_cpu_access() and end_cpu_access() dma_buf_ops
> > callbacks for cache synchronisation on exported buffers.
> >
> > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> > ---
> >  .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > index 6db60e9d5183..bfc99a0cb7b9 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf)
> >       vb2_dma_sg_put(dbuf->priv);
> >  }
> >
>
> There is no corresponding vb2_sg_buffer_consistent function here.
>
> Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs
> argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no
> effect on dma-sg buffers.

videobuf2-dma-sg allocates the memory using the page allocator
directly, which means that there is no memory consistency guarantee.

>
> Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()?
>

V4L2_FLAG_MEMORY_NON_CONSISTENT is a flag for dma_alloc_attrs(). It
isn't supposed to do anything for dma_map_sg_attrs(), which is only
supposed to create the device (e.g. IOMMU) mapping for already
allocated memory.

> I suspect it was just laziness in the past, and that it should be wired
> up, just as for dma-contig.
>
> Regards,
>
>         Hans
>
> > +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 +508,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,
> >
>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS
  2020-01-23 11:08         ` Hans Verkuil
@ 2020-01-28  4:45           ` Tomasz Figa
  2020-01-28  8:38             ` Hans Verkuil
  0 siblings, 1 reply; 56+ messages in thread
From: Tomasz Figa @ 2020-01-28  4:45 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Mauro Carvalho Chehab,
	Kyungmin Park, Marek Szyprowski, Sakari Ailus, Laurent Pinchart,
	Pawel Osciak, Linux Media Mailing List,
	Linux Kernel Mailing List

On Thu, Jan 23, 2020 at 8:08 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 1/22/20 4:48 AM, Sergey Senozhatsky wrote:
> > On (20/01/22 11:18), Sergey Senozhatsky wrote:
> > [..]
> >>>> +    * -
> >>>> +      - __u32
> >>>>        - ``reserved``\ [1]
> >>>>        - A place holder for future extensions. Drivers and applications
> >>>> -  must set the array to zero.
> >>>> +  must set the array to zero, unless application wants to specify
> >>>> +        buffer management ``flags``.
> >>>
> >>> I think support for this flag should be signaled as a V4L2_BUF_CAP capability.
> >>> If the capability is not set, then vb2 should set 'flags' to 0 to preserve the
> >>> old 'Drivers and applications must set the array to zero' behavior.
> >>
> >> The patch set adds V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS towards the end of the
> >> series, I guess I can shuffle the patches and change the wording here.
> >
> > Or I can add separate queue flag and V4L2_BUF_CAP:
> >
> > struct vb2_queue {
> > ...
> >       allow_cache_hints:1
> > +     allow_consistency_hints:1
> > ...
> > }
> >
> > and then have CAP_SUPPORTS_CACHE_HINTS/CAP_SUPPORTS_CONSISTENCY_HINTS.
>
> Don't these two go hand-in-hand? I.e. either neither are supported, or
> both are supported? If so, then one queue flag is sufficient.

Cache sync hints are already part of the standard UAPI, so I think
there isn't any capability bit needed for them. That said, they aren't
really tied to non-consistent MMAP buffers. Userspace using USERPTR
can also use them.

MMAP buffer consistency hint deserves a capability bit indeed.

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues
  2020-01-23 11:35       ` Hans Verkuil
  2020-01-24  2:25         ` Sergey Senozhatsky
@ 2020-01-28  7:19         ` Tomasz Figa
  2020-01-28  8:47           ` Hans Verkuil
  1 sibling, 1 reply; 56+ messages in thread
From: Tomasz Figa @ 2020-01-28  7:19 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Mauro Carvalho Chehab,
	Kyungmin Park, Marek Szyprowski, Sakari Ailus, Laurent Pinchart,
	Pawel Osciak, Linux Media Mailing List,
	Linux Kernel Mailing List

On Thu, Jan 23, 2020 at 8:35 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 1/22/20 6:05 AM, Sergey Senozhatsky wrote:
> > On (20/01/10 11:30), Hans Verkuil wrote:
> > [..]
> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> >>> index 1762849288ae..2b9d3318e6fb 100644
> >>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> >>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> >>> @@ -341,8 +341,22 @@ static void set_buffer_cache_hints(struct vb2_queue *q,
> >>>                                struct vb2_buffer *vb,
> >>>                                struct v4l2_buffer *b)
> >>>  {
> >>> -   vb->need_cache_sync_on_prepare = 1;
> >>> +   /*
> >>> +    * DMA exporter should take care of cache syncs, so we can avoid
> >>> +    * explicit ->prepare()/->finish() syncs.
> >>> +    */
> >>> +   if (q->memory == VB2_MEMORY_DMABUF) {
> >>> +           vb->need_cache_sync_on_finish = 0;
> >>> +           vb->need_cache_sync_on_prepare = 0;
> >>> +           return;
> >>> +   }
> >>>
> >>> +   /*
> >>> +    * For other ->memory types we always need ->prepare() cache
> >>> +    * sync. ->finish() cache sync, however, can be avoided when queue
> >>> +    * direction is TO_DEVICE.
> >>> +    */
> >>> +   vb->need_cache_sync_on_prepare = 1;
> >>
> >> I'm trying to remember: what needs to be done in prepare()
> >> for a capture buffer? I thought that for capture you only
> >> needed to invalidate the cache in finish(), but nothing needs
> >> to be done in the prepare().
> >
> > Hmm. Not sure. A precaution in case if user-space wrote to that buffer?
>
> But whatever was written in the buffer is going to be overwritten anyway.
>
> Unless I am mistaken the current situation is that the cache syncs are done
> in both prepare and finish, regardless of the DMA direction.
>
> I would keep that behavior to avoid introducing any unexpected regressions.
>

It wouldn't be surprising if the buffer was first filled with default
values (e.g. all zeroes) on the CPU. That would make the cache lines
dirty and they could overwrite what the device writes. So we need to
flush (aka clean) the write-back caches on prepare for CAPTURE
buffers.

> Then, if q->allow_cache_hint is set, then default to a cache sync (cache clean)
> in the prepare for OUTPUT buffers and a cache sync (cache invalidate) in the
> finish for CAPTURE buffers.

I'd still default to the existing behavior even if allow_cache_hint is
set, because of what I wrote above. Then if the userspace doesn't ever
write to the buffers, it can request no flush/clean by setting the
V4L2_BUF_FLAG_NO_CACHE_CLEAN flag when queuing the buffer.

>
> This also means that any drivers that want to access a buffer in between the
> prepare...finish calls will need to do a begin/end_cpu_access. But that's a
> separate matter.

AFAIR with current design of the series, the drivers can opt-in for
userspace cache sync hints, so by default even if the userspace
requests sync to be skipped, it wouldn't have any effect unless the
driver allows so. Then I'd imagine migrating all the drivers to
request clean/invalidate explicitly. Note that the DMA-buf
begin/end_cpu_access isn't enough here. We'd need something like
vb2_begin/end_cpu_access() which also takes care of syncing
inconsistent MMAP and USERPTR buffers.

>
> Regards,
>
>         Hans
>
> >
> > +     if (q->dma_dir == DMA_FROM_DEVICE)
> > +             q->need_cache_sync_on_prepare = 0;
> >
> > ?
> >
> >       -ss
> >
>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues
  2020-01-24  7:32           ` Sergey Senozhatsky
@ 2020-01-28  7:22             ` Tomasz Figa
  2020-01-28  7:34               ` Sergey Senozhatsky
  0 siblings, 1 reply; 56+ messages in thread
From: Tomasz Figa @ 2020-01-28  7:22 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Hans Verkuil, Hans Verkuil, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski, Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	Linux Media Mailing List, Linux Kernel Mailing List

On Fri, Jan 24, 2020 at 4:32 PM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (20/01/24 11:25), Sergey Senozhatsky wrote:
> [..]
> >
> > > Then, if q->allow_cache_hint is set, then default to a cache sync (cache clean)
> > > in the prepare for OUTPUT buffers and a cache sync (cache invalidate) in the
> > > finish for CAPTURE buffers.
> >
> > We alter default cache sync behaviour based both on queue ->memory
> > type and queue ->dma_dir. Shall both of those cases depend on
> > ->allow_cache_hints, or q->memory can be independent?
> >
> > static void set_buffer_cache_hints(struct vb2_queue *q,
> >                                  struct vb2_buffer *vb,
> >                                  struct v4l2_buffer *b)
> > {
> >       if (!q->allow_cache_hints)
> >               return;
> >
> >       /*
> >        * 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;
> >       }
>
> I personally would prefer this to be above ->allow_cache_hints check,
> IOW to be independent. Because we need to skip flush/invalidation for
> DMABUF anyway, that's what allocators do in ->prepare() and ->finish()
> currently.

I think it's even more than personal preference. We shouldn't even
attempt to sync imported DMA-bufs, so the code above may result in
incorrect behavior.

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues
  2020-01-28  7:22             ` Tomasz Figa
@ 2020-01-28  7:34               ` Sergey Senozhatsky
  0 siblings, 0 replies; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-01-28  7:34 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Sergey Senozhatsky, Hans Verkuil, Hans Verkuil,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski,
	Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	Linux Media Mailing List, Linux Kernel Mailing List

On (20/01/28 16:22), Tomasz Figa wrote:
[..]
> > > > Then, if q->allow_cache_hint is set, then default to a cache sync (cache clean)
> > > > in the prepare for OUTPUT buffers and a cache sync (cache invalidate) in the
> > > > finish for CAPTURE buffers.
> > >
> > > We alter default cache sync behaviour based both on queue ->memory
> > > type and queue ->dma_dir. Shall both of those cases depend on
> > > ->allow_cache_hints, or q->memory can be independent?
> > >
> > > static void set_buffer_cache_hints(struct vb2_queue *q,
> > >                                  struct vb2_buffer *vb,
> > >                                  struct v4l2_buffer *b)
> > > {
> > >       if (!q->allow_cache_hints)
> > >               return;
> > >
> > >       /*
> > >        * 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;
> > >       }
> >
> > I personally would prefer this to be above ->allow_cache_hints check,
> > IOW to be independent. Because we need to skip flush/invalidation for
> > DMABUF anyway, that's what allocators do in ->prepare() and ->finish()
> > currently.
>
> I think it's even more than personal preference. We shouldn't even
> attempt to sync imported DMA-bufs, so the code above may result in
> incorrect behavior.

Sure. Allocators test buf->db_attach in prepare()/finish(). This patchset
removes that logic, because we move it to the core code. But if the
conclusion will be to clear ->need_cache_sync_on_finish/prepare only
when ->allow_cache_hints was set, then buf->db_attach bail out must stay
in allocators. And this is where I have preferences :)

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg
  2020-01-28  4:38     ` Tomasz Figa
@ 2020-01-28  8:36       ` Hans Verkuil
  2020-01-30 11:02         ` Tomasz Figa
  0 siblings, 1 reply; 56+ messages in thread
From: Hans Verkuil @ 2020-01-28  8:36 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Sergey Senozhatsky, Hans Verkuil, Mauro Carvalho Chehab,
	Kyungmin Park, Marek Szyprowski, Sakari Ailus, Laurent Pinchart,
	Pawel Osciak, Linux Media Mailing List,
	Linux Kernel Mailing List

On 1/28/20 5:38 AM, Tomasz Figa wrote:
> On Fri, Jan 10, 2020 at 7:13 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
>>> Provide begin_cpu_access() and end_cpu_access() dma_buf_ops
>>> callbacks for cache synchronisation on exported buffers.
>>>
>>> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
>>> ---
>>>  .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++
>>>  1 file changed, 22 insertions(+)
>>>
>>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>>> index 6db60e9d5183..bfc99a0cb7b9 100644
>>> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>>> @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf)
>>>       vb2_dma_sg_put(dbuf->priv);
>>>  }
>>>
>>
>> There is no corresponding vb2_sg_buffer_consistent function here.
>>
>> Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs
>> argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no
>> effect on dma-sg buffers.
> 
> videobuf2-dma-sg allocates the memory using the page allocator
> directly, which means that there is no memory consistency guarantee.
> 
>>
>> Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()?
>>
> 
> V4L2_FLAG_MEMORY_NON_CONSISTENT is a flag for dma_alloc_attrs(). It
> isn't supposed to do anything for dma_map_sg_attrs(), which is only
> supposed to create the device (e.g. IOMMU) mapping for already
> allocated memory.

Ah, right.

But could vb2_dma_sg_alloc_compacted() be modified so that is uses
dma_alloc_attrs() instead of alloc_pages()? Sorry, that might be a stupid
question, I'm not an expert in this area. All I know is that I hate inconsistent
APIs where something works for one thing, but not another.

Regards,

	Hans

> 
>> I suspect it was just laziness in the past, and that it should be wired
>> up, just as for dma-contig.
>>
>> Regards,
>>
>>         Hans
>>
>>> +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 +508,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,
>>>
>>


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS
  2020-01-28  4:45           ` Tomasz Figa
@ 2020-01-28  8:38             ` Hans Verkuil
  0 siblings, 0 replies; 56+ messages in thread
From: Hans Verkuil @ 2020-01-28  8:38 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Sergey Senozhatsky, Hans Verkuil, Mauro Carvalho Chehab,
	Kyungmin Park, Marek Szyprowski, Sakari Ailus, Laurent Pinchart,
	Pawel Osciak, Linux Media Mailing List,
	Linux Kernel Mailing List

On 1/28/20 5:45 AM, Tomasz Figa wrote:
> On Thu, Jan 23, 2020 at 8:08 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 1/22/20 4:48 AM, Sergey Senozhatsky wrote:
>>> On (20/01/22 11:18), Sergey Senozhatsky wrote:
>>> [..]
>>>>>> +    * -
>>>>>> +      - __u32
>>>>>>        - ``reserved``\ [1]
>>>>>>        - A place holder for future extensions. Drivers and applications
>>>>>> -  must set the array to zero.
>>>>>> +  must set the array to zero, unless application wants to specify
>>>>>> +        buffer management ``flags``.
>>>>>
>>>>> I think support for this flag should be signaled as a V4L2_BUF_CAP capability.
>>>>> If the capability is not set, then vb2 should set 'flags' to 0 to preserve the
>>>>> old 'Drivers and applications must set the array to zero' behavior.
>>>>
>>>> The patch set adds V4L2_BUF_CAP_SUPPORTS_CACHE_HINTS towards the end of the
>>>> series, I guess I can shuffle the patches and change the wording here.
>>>
>>> Or I can add separate queue flag and V4L2_BUF_CAP:
>>>
>>> struct vb2_queue {
>>> ...
>>>       allow_cache_hints:1
>>> +     allow_consistency_hints:1
>>> ...
>>> }
>>>
>>> and then have CAP_SUPPORTS_CACHE_HINTS/CAP_SUPPORTS_CONSISTENCY_HINTS.
>>
>> Don't these two go hand-in-hand? I.e. either neither are supported, or
>> both are supported? If so, then one queue flag is sufficient.
> 
> Cache sync hints are already part of the standard UAPI, so I think
> there isn't any capability bit needed for them.

These hints may exist, but they never worked. So I think a capability would
be very useful.

 That said, they aren't
> really tied to non-consistent MMAP buffers. Userspace using USERPTR
> can also use them.

OK, two separate capability bits is fine.

Regards,

	Hans

> 
> MMAP buffer consistency hint deserves a capability bit indeed.
> 
> Best regards,
> Tomasz
> 


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues
  2020-01-28  7:19         ` Tomasz Figa
@ 2020-01-28  8:47           ` Hans Verkuil
  0 siblings, 0 replies; 56+ messages in thread
From: Hans Verkuil @ 2020-01-28  8:47 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Sergey Senozhatsky, Hans Verkuil, Mauro Carvalho Chehab,
	Kyungmin Park, Marek Szyprowski, Sakari Ailus, Laurent Pinchart,
	Pawel Osciak, Linux Media Mailing List,
	Linux Kernel Mailing List

On 1/28/20 8:19 AM, Tomasz Figa wrote:
> On Thu, Jan 23, 2020 at 8:35 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 1/22/20 6:05 AM, Sergey Senozhatsky wrote:
>>> On (20/01/10 11:30), Hans Verkuil wrote:
>>> [..]
>>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>>> index 1762849288ae..2b9d3318e6fb 100644
>>>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>>> @@ -341,8 +341,22 @@ static void set_buffer_cache_hints(struct vb2_queue *q,
>>>>>                                struct vb2_buffer *vb,
>>>>>                                struct v4l2_buffer *b)
>>>>>  {
>>>>> -   vb->need_cache_sync_on_prepare = 1;
>>>>> +   /*
>>>>> +    * DMA exporter should take care of cache syncs, so we can avoid
>>>>> +    * explicit ->prepare()/->finish() syncs.
>>>>> +    */
>>>>> +   if (q->memory == VB2_MEMORY_DMABUF) {
>>>>> +           vb->need_cache_sync_on_finish = 0;
>>>>> +           vb->need_cache_sync_on_prepare = 0;
>>>>> +           return;
>>>>> +   }
>>>>>
>>>>> +   /*
>>>>> +    * For other ->memory types we always need ->prepare() cache
>>>>> +    * sync. ->finish() cache sync, however, can be avoided when queue
>>>>> +    * direction is TO_DEVICE.
>>>>> +    */
>>>>> +   vb->need_cache_sync_on_prepare = 1;
>>>>
>>>> I'm trying to remember: what needs to be done in prepare()
>>>> for a capture buffer? I thought that for capture you only
>>>> needed to invalidate the cache in finish(), but nothing needs
>>>> to be done in the prepare().
>>>
>>> Hmm. Not sure. A precaution in case if user-space wrote to that buffer?
>>
>> But whatever was written in the buffer is going to be overwritten anyway.
>>
>> Unless I am mistaken the current situation is that the cache syncs are done
>> in both prepare and finish, regardless of the DMA direction.
>>
>> I would keep that behavior to avoid introducing any unexpected regressions.
>>
> 
> It wouldn't be surprising if the buffer was first filled with default
> values (e.g. all zeroes) on the CPU. That would make the cache lines
> dirty and they could overwrite what the device writes. So we need to
> flush (aka clean) the write-back caches on prepare for CAPTURE
> buffers.

Very true, I'd forgotten about that. This should be documented in the userspace
documentation. And possible in this function as well.

I think these issues are complex enough that there is no such things as too
much documentation :-)

> 
>> Then, if q->allow_cache_hint is set, then default to a cache sync (cache clean)
>> in the prepare for OUTPUT buffers and a cache sync (cache invalidate) in the
>> finish for CAPTURE buffers.
> 
> I'd still default to the existing behavior even if allow_cache_hint is
> set, because of what I wrote above. Then if the userspace doesn't ever
> write to the buffers, it can request no flush/clean by setting the
> V4L2_BUF_FLAG_NO_CACHE_CLEAN flag when queuing the buffer.
> 
>>
>> This also means that any drivers that want to access a buffer in between the
>> prepare...finish calls will need to do a begin/end_cpu_access. But that's a
>> separate matter.
> 
> AFAIR with current design of the series, the drivers can opt-in for
> userspace cache sync hints, so by default even if the userspace
> requests sync to be skipped, it wouldn't have any effect unless the
> driver allows so. Then I'd imagine migrating all the drivers to
> request clean/invalidate explicitly. Note that the DMA-buf
> begin/end_cpu_access isn't enough here. We'd need something like
> vb2_begin/end_cpu_access() which also takes care of syncing
> inconsistent MMAP and USERPTR buffers.

I did just that in my old git branch for this:

https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=vb2-cpu-access

Regards,

	Hans

> 
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> +     if (q->dma_dir == DMA_FROM_DEVICE)
>>> +             q->need_cache_sync_on_prepare = 0;
>>>
>>> ?
>>>
>>>       -ss
>>>
>>


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg
  2020-01-28  8:36       ` Hans Verkuil
@ 2020-01-30 11:02         ` Tomasz Figa
  2020-01-30 12:18           ` Hans Verkuil
  0 siblings, 1 reply; 56+ messages in thread
From: Tomasz Figa @ 2020-01-30 11:02 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Mauro Carvalho Chehab,
	Kyungmin Park, Marek Szyprowski, Sakari Ailus, Laurent Pinchart,
	Pawel Osciak, Linux Media Mailing List,
	Linux Kernel Mailing List

On Tue, Jan 28, 2020 at 5:36 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 1/28/20 5:38 AM, Tomasz Figa wrote:
> > On Fri, Jan 10, 2020 at 7:13 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> >>> Provide begin_cpu_access() and end_cpu_access() dma_buf_ops
> >>> callbacks for cache synchronisation on exported buffers.
> >>>
> >>> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> >>> ---
> >>>  .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++
> >>>  1 file changed, 22 insertions(+)
> >>>
> >>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >>> index 6db60e9d5183..bfc99a0cb7b9 100644
> >>> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >>> @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf)
> >>>       vb2_dma_sg_put(dbuf->priv);
> >>>  }
> >>>
> >>
> >> There is no corresponding vb2_sg_buffer_consistent function here.
> >>
> >> Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs
> >> argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no
> >> effect on dma-sg buffers.
> >
> > videobuf2-dma-sg allocates the memory using the page allocator
> > directly, which means that there is no memory consistency guarantee.
> >
> >>
> >> Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()?
> >>
> >
> > V4L2_FLAG_MEMORY_NON_CONSISTENT is a flag for dma_alloc_attrs(). It
> > isn't supposed to do anything for dma_map_sg_attrs(), which is only
> > supposed to create the device (e.g. IOMMU) mapping for already
> > allocated memory.
>
> Ah, right.
>
> But could vb2_dma_sg_alloc_compacted() be modified so that is uses
> dma_alloc_attrs() instead of alloc_pages()? Sorry, that might be a stupid
> question, I'm not an expert in this area. All I know is that I hate inconsistent
> APIs where something works for one thing, but not another.
>

dma_alloc_attrs() would allocate contiguous memory, which kind of goes
against the vb2_dma_sg allocator idea. Technically we could call it N
times with size = 1 page to achieve what we want, but is this really
what we want?

Given that vb2_dma_sg has always been returning non-consistent memory,
do we have any good reason to add consistent memory to it? If so,
perhaps we could still do that in an incremental patch, to avoid
complicating this already complex series? :)

Best regards,
Tomasz

> Regards,
>
>         Hans
>
> >
> >> I suspect it was just laziness in the past, and that it should be wired
> >> up, just as for dma-contig.
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>> +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 +508,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,
> >>>
> >>
>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg
  2020-01-30 11:02         ` Tomasz Figa
@ 2020-01-30 12:18           ` Hans Verkuil
  2020-02-03 10:04             ` Tomasz Figa
  0 siblings, 1 reply; 56+ messages in thread
From: Hans Verkuil @ 2020-01-30 12:18 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Sergey Senozhatsky, Hans Verkuil, Mauro Carvalho Chehab,
	Kyungmin Park, Marek Szyprowski, Sakari Ailus, Laurent Pinchart,
	Pawel Osciak, Linux Media Mailing List,
	Linux Kernel Mailing List

On 1/30/20 12:02 PM, Tomasz Figa wrote:
> On Tue, Jan 28, 2020 at 5:36 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 1/28/20 5:38 AM, Tomasz Figa wrote:
>>> On Fri, Jan 10, 2020 at 7:13 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
>>>>> Provide begin_cpu_access() and end_cpu_access() dma_buf_ops
>>>>> callbacks for cache synchronisation on exported buffers.
>>>>>
>>>>> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
>>>>> ---
>>>>>  .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++
>>>>>  1 file changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>>>>> index 6db60e9d5183..bfc99a0cb7b9 100644
>>>>> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>>>>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
>>>>> @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf)
>>>>>       vb2_dma_sg_put(dbuf->priv);
>>>>>  }
>>>>>
>>>>
>>>> There is no corresponding vb2_sg_buffer_consistent function here.
>>>>
>>>> Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs
>>>> argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no
>>>> effect on dma-sg buffers.
>>>
>>> videobuf2-dma-sg allocates the memory using the page allocator
>>> directly, which means that there is no memory consistency guarantee.
>>>
>>>>
>>>> Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()?
>>>>
>>>
>>> V4L2_FLAG_MEMORY_NON_CONSISTENT is a flag for dma_alloc_attrs(). It
>>> isn't supposed to do anything for dma_map_sg_attrs(), which is only
>>> supposed to create the device (e.g. IOMMU) mapping for already
>>> allocated memory.
>>
>> Ah, right.
>>
>> But could vb2_dma_sg_alloc_compacted() be modified so that is uses
>> dma_alloc_attrs() instead of alloc_pages()? Sorry, that might be a stupid
>> question, I'm not an expert in this area. All I know is that I hate inconsistent
>> APIs where something works for one thing, but not another.
>>
> 
> dma_alloc_attrs() would allocate contiguous memory, which kind of goes
> against the vb2_dma_sg allocator idea. Technically we could call it N
> times with size = 1 page to achieve what we want, but is this really
> what we want?
> 
> Given that vb2_dma_sg has always been returning non-consistent memory,
> do we have any good reason to add consistent memory to it? If so,
> perhaps we could still do that in an incremental patch, to avoid
> complicating this already complex series? :)

I very much agree with that. But this should be very clearly documented.
Should V4L2_CAP_MEMORY_NON_CONSISTENT always be set in this case?

Regards,

	Hans

> 
> Best regards,
> Tomasz
> 
>> Regards,
>>
>>         Hans
>>
>>>
>>>> I suspect it was just laziness in the past, and that it should be wired
>>>> up, just as for dma-contig.
>>>>
>>>> Regards,
>>>>
>>>>         Hans
>>>>
>>>>> +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 +508,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,
>>>>>
>>>>
>>


^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg
  2020-01-30 12:18           ` Hans Verkuil
@ 2020-02-03 10:04             ` Tomasz Figa
  2020-02-04  2:50               ` Sergey Senozhatsky
  0 siblings, 1 reply; 56+ messages in thread
From: Tomasz Figa @ 2020-02-03 10:04 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Sergey Senozhatsky, Hans Verkuil, Mauro Carvalho Chehab,
	Kyungmin Park, Marek Szyprowski, Sakari Ailus, Laurent Pinchart,
	Pawel Osciak, Linux Media Mailing List,
	Linux Kernel Mailing List

On Thu, Jan 30, 2020 at 9:18 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 1/30/20 12:02 PM, Tomasz Figa wrote:
> > On Tue, Jan 28, 2020 at 5:36 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 1/28/20 5:38 AM, Tomasz Figa wrote:
> >>> On Fri, Jan 10, 2020 at 7:13 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>
> >>>> On 12/17/19 4:20 AM, Sergey Senozhatsky wrote:
> >>>>> Provide begin_cpu_access() and end_cpu_access() dma_buf_ops
> >>>>> callbacks for cache synchronisation on exported buffers.
> >>>>>
> >>>>> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> >>>>> ---
> >>>>>  .../media/common/videobuf2/videobuf2-dma-sg.c | 22 +++++++++++++++++++
> >>>>>  1 file changed, 22 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >>>>> index 6db60e9d5183..bfc99a0cb7b9 100644
> >>>>> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >>>>> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >>>>> @@ -470,6 +470,26 @@ static void vb2_dma_sg_dmabuf_ops_release(struct dma_buf *dbuf)
> >>>>>       vb2_dma_sg_put(dbuf->priv);
> >>>>>  }
> >>>>>
> >>>>
> >>>> There is no corresponding vb2_sg_buffer_consistent function here.
> >>>>
> >>>> Looking more closely I see that vb2_dma_sg_alloc doesn't pass the dma_attrs
> >>>> argument to dma_map_sg_attrs, thus V4L2_FLAG_MEMORY_NON_CONSISTENT has no
> >>>> effect on dma-sg buffers.
> >>>
> >>> videobuf2-dma-sg allocates the memory using the page allocator
> >>> directly, which means that there is no memory consistency guarantee.
> >>>
> >>>>
> >>>> Is there a reason why dma_attrs isn't passed on to dma_map_sg_attrs()?
> >>>>
> >>>
> >>> V4L2_FLAG_MEMORY_NON_CONSISTENT is a flag for dma_alloc_attrs(). It
> >>> isn't supposed to do anything for dma_map_sg_attrs(), which is only
> >>> supposed to create the device (e.g. IOMMU) mapping for already
> >>> allocated memory.
> >>
> >> Ah, right.
> >>
> >> But could vb2_dma_sg_alloc_compacted() be modified so that is uses
> >> dma_alloc_attrs() instead of alloc_pages()? Sorry, that might be a stupid
> >> question, I'm not an expert in this area. All I know is that I hate inconsistent
> >> APIs where something works for one thing, but not another.
> >>
> >
> > dma_alloc_attrs() would allocate contiguous memory, which kind of goes
> > against the vb2_dma_sg allocator idea. Technically we could call it N
> > times with size = 1 page to achieve what we want, but is this really
> > what we want?
> >
> > Given that vb2_dma_sg has always been returning non-consistent memory,
> > do we have any good reason to add consistent memory to it? If so,
> > perhaps we could still do that in an incremental patch, to avoid
> > complicating this already complex series? :)
>
> I very much agree with that. But this should be very clearly documented.
> Should V4L2_CAP_MEMORY_NON_CONSISTENT always be set in this case?
>

Yes, IMHO that would make sense. My understanding is that currently
the consistency of allocated memory is unspecified, so it can be
either. With V4L2_FLAG_MEMORY_NON_CONSISTENT, the userspace can
explicitly ask for inconsistent memory.

Moreover, I'd vote for setting V4L2_CAP_MEMORY_NON_CONSISTENT when
V4L2_FLAG_MEMORY_NON_CONSISTENT is guaranteed to return inconsistent
memory to avoid "optional" features or "hints" without guaranteed
behavior.

> Regards,
>
>         Hans
>
> >
> > Best regards,
> > Tomasz
> >
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>>> I suspect it was just laziness in the past, and that it should be wired
> >>>> up, just as for dma-contig.
> >>>>
> >>>> Regards,
> >>>>
> >>>>         Hans
> >>>>
> >>>>> +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 +508,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,
> >>>>>
> >>>>
> >>
>

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg
  2020-02-03 10:04             ` Tomasz Figa
@ 2020-02-04  2:50               ` Sergey Senozhatsky
  2020-02-06  8:51                 ` Tomasz Figa
  0 siblings, 1 reply; 56+ messages in thread
From: Sergey Senozhatsky @ 2020-02-04  2:50 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Hans Verkuil, Sergey Senozhatsky, Hans Verkuil,
	Mauro Carvalho Chehab, Kyungmin Park, Marek Szyprowski,
	Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	Linux Media Mailing List, Linux Kernel Mailing List

On (20/02/03 19:04), Tomasz Figa wrote:
[..]
> > I very much agree with that. But this should be very clearly documented.
> > Should V4L2_CAP_MEMORY_NON_CONSISTENT always be set in this case?
> >
> 
> Yes, IMHO that would make sense. My understanding is that currently
> the consistency of allocated memory is unspecified, so it can be
> either. With V4L2_FLAG_MEMORY_NON_CONSISTENT, the userspace can
> explicitly ask for inconsistent memory.
> 
> Moreover, I'd vote for setting V4L2_CAP_MEMORY_NON_CONSISTENT when
> V4L2_FLAG_MEMORY_NON_CONSISTENT is guaranteed to return inconsistent
> memory to avoid "optional" features or "hints" without guaranteed
> behavior.

Documentation/DMA-attributes.txt says the following

  DMA_ATTR_NON_CONSISTENT
  -----------------------

  DMA_ATTR_NON_CONSISTENT lets the platform to choose to return either
  consistent or non-consistent memory as it sees fit.  By using this API,
  you are guaranteeing to the platform that you have all the correct and
  necessary sync points for this memory in the driver.

	-ss

^ permalink raw reply	[flat|nested] 56+ messages in thread

* Re: [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg
  2020-02-04  2:50               ` Sergey Senozhatsky
@ 2020-02-06  8:51                 ` Tomasz Figa
  0 siblings, 0 replies; 56+ messages in thread
From: Tomasz Figa @ 2020-02-06  8:51 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Hans Verkuil, Hans Verkuil, Mauro Carvalho Chehab, Kyungmin Park,
	Marek Szyprowski, Sakari Ailus, Laurent Pinchart, Pawel Osciak,
	Linux Media Mailing List, Linux Kernel Mailing List

On Tue, Feb 4, 2020 at 11:50 AM Sergey Senozhatsky
<senozhatsky@chromium.org> wrote:
>
> On (20/02/03 19:04), Tomasz Figa wrote:
> [..]
> > > I very much agree with that. But this should be very clearly documented.
> > > Should V4L2_CAP_MEMORY_NON_CONSISTENT always be set in this case?
> > >
> >
> > Yes, IMHO that would make sense. My understanding is that currently
> > the consistency of allocated memory is unspecified, so it can be
> > either. With V4L2_FLAG_MEMORY_NON_CONSISTENT, the userspace can
> > explicitly ask for inconsistent memory.
> >
> > Moreover, I'd vote for setting V4L2_CAP_MEMORY_NON_CONSISTENT when
> > V4L2_FLAG_MEMORY_NON_CONSISTENT is guaranteed to return inconsistent
> > memory to avoid "optional" features or "hints" without guaranteed
> > behavior.
>
> Documentation/DMA-attributes.txt says the following
>
>   DMA_ATTR_NON_CONSISTENT
>   -----------------------
>
>   DMA_ATTR_NON_CONSISTENT lets the platform to choose to return either
>   consistent or non-consistent memory as it sees fit.  By using this API,
>   you are guaranteeing to the platform that you have all the correct and
>   necessary sync points for this memory in the driver.

Good point. And I also realized that some platforms just have no way
to make the memory inconsistent, because they may have hardware
coherency.

Then we need to keep it a hint only.

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 56+ messages in thread

end of thread, other threads:[~2020-02-06  8:52 UTC | newest]

Thread overview: 56+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-17  3:20 [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky
2019-12-17  3:20 ` [RFC][PATCH 01/15] videobuf2: add cache management members Sergey Senozhatsky
2019-12-17  3:20 ` [RFC][PATCH 02/15] videobuf2: handle V4L2 buffer cache flags Sergey Senozhatsky
2020-01-10 10:24   ` Hans Verkuil
2020-01-22  1:39     ` Sergey Senozhatsky
2020-01-22  2:53       ` Sergey Senozhatsky
2020-01-28  4:35         ` Tomasz Figa
2019-12-17  3:20 ` [RFC][PATCH 03/15] videobuf2: add V4L2_FLAG_MEMORY_NON_CONSISTENT flag Sergey Senozhatsky
2020-01-10  9:36   ` Hans Verkuil
2020-01-10  9:46     ` Sergey Senozhatsky
2019-12-17  3:20 ` [RFC][PATCH 04/15] videobuf2: add queue memory consistency parameter Sergey Senozhatsky
2020-01-10  9:47   ` Hans Verkuil
2020-01-22  2:05     ` Sergey Senozhatsky
2020-01-23 11:02       ` Hans Verkuil
2020-01-24  2:04         ` Sergey Senozhatsky
2019-12-17  3:20 ` [RFC][PATCH 05/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in REQBUFS Sergey Senozhatsky
2020-01-10  9:55   ` Hans Verkuil
2020-01-22  2:18     ` Sergey Senozhatsky
2020-01-22  3:48       ` Sergey Senozhatsky
2020-01-23 11:08         ` Hans Verkuil
2020-01-28  4:45           ` Tomasz Figa
2020-01-28  8:38             ` Hans Verkuil
2019-12-17  3:20 ` [RFC][PATCH 06/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE_BUFS Sergey Senozhatsky
2020-01-10  9:59   ` Hans Verkuil
2020-01-23  3:41     ` Sergey Senozhatsky
2020-01-23 11:41       ` Hans Verkuil
2020-01-24  1:28         ` Sergey Senozhatsky
2019-12-17  3:20 ` [RFC][PATCH 07/15] videobuf2: factor out planes prepare/finish functions Sergey Senozhatsky
2019-12-17  3:20 ` [RFC][PATCH 08/15] videobuf2: do not sync caches when we are allowed not to Sergey Senozhatsky
2019-12-17  3:20 ` [RFC][PATCH 09/15] videobuf2: check ->synced flag in prepare() and finish() Sergey Senozhatsky
2019-12-17  3:20 ` [RFC][PATCH 10/15] videobuf2: let user-space know when driver supports cache hints Sergey Senozhatsky
2019-12-17  3:20 ` [RFC][PATCH 11/15] videobuf2: add begin/end cpu_access callbacks to dma-contig Sergey Senozhatsky
2019-12-17  3:20 ` [RFC][PATCH 12/15] videobuf2: add begin/end cpu_access callbacks to dma-sg Sergey Senozhatsky
2020-01-10 10:13   ` Hans Verkuil
2020-01-22  6:37     ` Sergey Senozhatsky
2020-01-28  4:38     ` Tomasz Figa
2020-01-28  8:36       ` Hans Verkuil
2020-01-30 11:02         ` Tomasz Figa
2020-01-30 12:18           ` Hans Verkuil
2020-02-03 10:04             ` Tomasz Figa
2020-02-04  2:50               ` Sergey Senozhatsky
2020-02-06  8:51                 ` Tomasz Figa
2019-12-17  3:20 ` [RFC][PATCH 13/15] videobuf2: do not sync buffers for DMABUF queues Sergey Senozhatsky
2020-01-10 10:30   ` Hans Verkuil
2020-01-22  5:05     ` Sergey Senozhatsky
2020-01-23 11:35       ` Hans Verkuil
2020-01-24  2:25         ` Sergey Senozhatsky
2020-01-24  7:32           ` Sergey Senozhatsky
2020-01-28  7:22             ` Tomasz Figa
2020-01-28  7:34               ` Sergey Senozhatsky
2020-01-28  7:19         ` Tomasz Figa
2020-01-28  8:47           ` Hans Verkuil
2019-12-17  3:20 ` [RFC][PATCH 14/15] videobuf2: don't test db_attach in dma-contig prepare and finish Sergey Senozhatsky
2020-01-10 10:32   ` Hans Verkuil
2019-12-17  3:20 ` [RFC][PATCH 15/15] videobuf2: don't test db_attach in dma-sg " Sergey Senozhatsky
2020-01-08  2:27 ` [RFC][PATCH 00/15] Implement V4L2_BUF_FLAG_NO_CACHE_* flags Sergey Senozhatsky

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).