Re: [PATCH 05/28] media/v4l2: remove V4L2-FLAG-MEMORY-NON-CONSISTENT

[Hans Verkuil <hverkuil@xs4all.nl>]

On 10/09/2020 11:49, Sergey Senozhatsky wrote:
> Hi,
> 
> On (20/09/08 23:58), Tomasz Figa wrote:
>>
>> Given the above, we would like to make changes that affect the UAPI.
>> Would you still be able to revert this series?
>>
> 
> If we want to apply only "media/v4l2: remove V4L2-FLAG-MEMORY-NON-CONSISTENT"
> patch and keep the rest of the buffer cache hints series in the kernel, then
> I'd add one or two more patches. We don't need ->flags argument in
> create_bufs() and reqbufs() functions, that argument was introduced in order
> to pass in the requested coherency flag.
> 
> 
> Now.
> 
> Then we have a question - do we need flags member in struct
> v4l2_requestbuffers and v4l2_create_buffers or shall we just
> return back those 4 bytes to reserved[]? We pass BUF_FLAG_NO_CACHE_INVALIDATE
> and V4L2_BUF_FLAG_NO_CACHE_SYNC in struct v4l2_buffer.flags.

Drop the now unused flags member in v4l2_requestbuffers and v4l2_create_buffers.

> 
> If we decide to remove v4l2_requestbuffers and v4l2_create_buffers ->flags,
> then we also need to rollback documentation changes.

Correct.

> 
> Then we need to change CLEAR_AFTER_FIELD(foo, capabilities) in
> v4l2-ioctl to zero out reserved[] areas in v4l2_requestbuffers
> and v4l2_create_buffers. I think v4l2_create_buffers is fine,
> but requstbuffers has flags and reversed[1] in the union so for
> requestbuffers we simply removed the CLEAR_AFTER_FIELD() and
> hence dropped the corresponding check from v4l-compliance.
> 
> So, do we plan on using .flags in v4l2_requestbuffers and
> v4l2_create_buffers?

Perhaps, but this patch is meant to revert *all* changes relating to
V4L2_FLAG_MEMORY_NON_CONSISTENT. We don't want to have unused fields
in the public API.

Regards,

	Hans

> 
> 
> - create_bufs()/reqbufs() flags argument removal patch
> (no struct-s/documentation cleanup yet).
> 
> ====8<====
> Subject: [PATCH] remove redundant flags argument
> 
> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 10 +++++-----
>  drivers/media/common/videobuf2/videobuf2-v4l2.c |  6 ++----
>  include/media/videobuf2-core.h                  |  6 ++----
>  3 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 66a41cef33c1..4eab6d81cce1 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -722,7 +722,7 @@ int vb2_verify_memory_type(struct vb2_queue *q,
>  EXPORT_SYMBOL(vb2_verify_memory_type);
>  
>  int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> -		     unsigned int flags, unsigned int *count)
> +		     unsigned int *count)
>  {
>  	unsigned int num_buffers, allocated_buffers, num_planes = 0;
>  	unsigned plane_sizes[VB2_MAX_PLANES] = { };
> @@ -861,7 +861,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  EXPORT_SYMBOL_GPL(vb2_core_reqbufs);
>  
>  int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> -			 unsigned int flags, unsigned int *count,
> +			 unsigned int *count,
>  			 unsigned int requested_planes,
>  			 const unsigned int requested_sizes[])
>  {
> @@ -2547,7 +2547,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, 0, &fileio->count);
> +	ret = vb2_core_reqbufs(q, fileio->memory, &fileio->count);
>  	if (ret)
>  		goto err_kfree;
>  
> @@ -2604,7 +2604,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
>  
>  err_reqbufs:
>  	fileio->count = 0;
> -	vb2_core_reqbufs(q, fileio->memory, 0, &fileio->count);
> +	vb2_core_reqbufs(q, fileio->memory, &fileio->count);
>  
>  err_kfree:
>  	q->fileio = NULL;
> @@ -2624,7 +2624,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, 0, &fileio->count);
> +		vb2_core_reqbufs(q, fileio->memory, &fileio->count);
>  		kfree(fileio);
>  		dprintk(q, 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 d2879f53455b..96d3b2b2aa31 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -728,8 +728,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->flags, &req->count);
> +	return ret ? ret : vb2_core_reqbufs(q, req->memory, &req->count);
>  }
>  EXPORT_SYMBOL_GPL(vb2_reqbufs);
>  
> @@ -804,7 +803,6 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
>  		if (requested_sizes[i] == 0)
>  			return -EINVAL;
>  	return ret ? ret : vb2_core_create_bufs(q, create->memory,
> -						create->flags,
>  						&create->count,
>  						requested_planes,
>  						requested_sizes);
> @@ -993,7 +991,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->flags, &p->count);
> +	res = vb2_core_reqbufs(vdev->queue, p->memory, &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/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 4c7f25b07e93..bbb3f26fbde9 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -744,7 +744,6 @@ 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.
> - * @flags:	auxiliary queue/buffer management flags.
>   * @count:	requested buffer count.
>   *
>   * Videobuf2 core helper to implement VIDIOC_REQBUF() operation. It is called
> @@ -769,13 +768,12 @@ 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 flags, unsigned int *count);
> +		    unsigned int *count);
>  
>  /**
>   * vb2_core_create_bufs() - Allocate buffers and any required auxiliary structs
>   * @q: pointer to &struct vb2_queue with videobuf2 queue.
>   * @memory: memory type, as defined by &enum vb2_memory.
> - * @flags: auxiliary queue/buffer management flags.
>   * @count: requested buffer count.
>   * @requested_planes: number of planes requested.
>   * @requested_sizes: array with the size of the planes.
> @@ -793,7 +791,7 @@ 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 flags, unsigned int *count,
> +			 unsigned int *count,
>  			 unsigned int requested_planes,
>  			 const unsigned int requested_sizes[]);
>  
>