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

[Hans Verkuil <hverkuil@xs4all.nl>]

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
>