Re: [RFC][PATCH 06/15] videobuf2: handle V4L2_FLAG_MEMORY_NON_CONSISTENT in CREATE_BUFS

[Sergey Senozhatsky <senozhatsky@chromium.org>]

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) {