Re: [RFC][PATCH 04/15] videobuf2: add queue memory consistency parameter

[Sergey Senozhatsky <senozhatsky@chromium.org>]

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