linux-kernel - Re: [PATCH v16 8/8] media: test-drivers: Use helper for DELETE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e3b4c627-c495-402d-a40c-5664d19e2523@xs4all.nl>
Date: Mon, 15 Jan 2024 17:53:52 +0100
From: Hans Verkuil <hverkuil@...all.nl>
To: Benjamin Gaignard <benjamin.gaignard@...labora.com>, mchehab@...nel.org
Cc: linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
 kernel@...labora.com
Subject: Re: [PATCH v16 8/8] media: test-drivers: Use helper for DELETE_BUFS
 ioctl

On 15/12/2023 10:08, Benjamin Gaignard wrote:
> Allow test drivers to use DELETE_BUFS by adding vb2_ioctl_delete_bufs() helper.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@...labora.com>
> ---
>  drivers/media/test-drivers/vicodec/vicodec-core.c |  2 ++
>  drivers/media/test-drivers/vimc/vimc-capture.c    |  2 ++
>  drivers/media/test-drivers/visl/visl-video.c      |  2 ++
>  drivers/media/test-drivers/vivid/vivid-core.c     | 13 ++++++++++---

You patched vim2m.c in the previous patch. I'd say that belong in this one
instead.

>  4 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vicodec/vicodec-core.c b/drivers/media/test-drivers/vicodec/vicodec-core.c
> index e13f5452b927..12956d807e05 100644
> --- a/drivers/media/test-drivers/vicodec/vicodec-core.c
> +++ b/drivers/media/test-drivers/vicodec/vicodec-core.c
> @@ -1345,6 +1345,7 @@ static const struct v4l2_ioctl_ops vicodec_ioctl_ops = {
>  	.vidioc_prepare_buf	= v4l2_m2m_ioctl_prepare_buf,
>  	.vidioc_create_bufs	= v4l2_m2m_ioctl_create_bufs,
>  	.vidioc_expbuf		= v4l2_m2m_ioctl_expbuf,
> +	.vidioc_delete_bufs	= v4l2_m2m_ioctl_delete_bufs,
>  
>  	.vidioc_streamon	= v4l2_m2m_ioctl_streamon,
>  	.vidioc_streamoff	= v4l2_m2m_ioctl_streamoff,
> @@ -1731,6 +1732,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
>  	dst_vq->mem_ops = &vb2_vmalloc_memops;
>  	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>  	dst_vq->lock = src_vq->lock;
> +	dst_vq->supports_delete_bufs = true;

Same question as in the previous patch: why just for dst_vq?

It's also why I am skeptical of the cap flag, I think that if you support this
ioctl in an m2m device, then support this for both queues. In the rare case that
that can't be done, then you need to make your own ioctl function that does the
queue check first before calling v4l2_m2m_ioctl_delete_bufs().

>  
>  	return vb2_queue_init(dst_vq);
>  }
> diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
> index 97693561f1e4..a2078d9c2721 100644
> --- a/drivers/media/test-drivers/vimc/vimc-capture.c
> +++ b/drivers/media/test-drivers/vimc/vimc-capture.c
> @@ -221,6 +221,7 @@ static const struct v4l2_ioctl_ops vimc_capture_ioctl_ops = {
>  	.vidioc_expbuf = vb2_ioctl_expbuf,
>  	.vidioc_streamon = vb2_ioctl_streamon,
>  	.vidioc_streamoff = vb2_ioctl_streamoff,
> +	.vidioc_delete_bufs = vb2_ioctl_delete_bufs,
>  };
>  
>  static void vimc_capture_return_all_buffers(struct vimc_capture_device *vcapture,
> @@ -435,6 +436,7 @@ static struct vimc_ent_device *vimc_capture_add(struct vimc_device *vimc,
>  	q->min_reqbufs_allocation = 2;
>  	q->lock = &vcapture->lock;
>  	q->dev = v4l2_dev->dev;
> +	q->supports_delete_bufs = true;
>  
>  	ret = vb2_queue_init(q);
>  	if (ret) {
> diff --git a/drivers/media/test-drivers/visl/visl-video.c b/drivers/media/test-drivers/visl/visl-video.c
> index b9a4b44bd0ed..939c14107700 100644
> --- a/drivers/media/test-drivers/visl/visl-video.c
> +++ b/drivers/media/test-drivers/visl/visl-video.c
> @@ -539,6 +539,7 @@ const struct v4l2_ioctl_ops visl_ioctl_ops = {
>  	.vidioc_prepare_buf		= v4l2_m2m_ioctl_prepare_buf,
>  	.vidioc_create_bufs		= v4l2_m2m_ioctl_create_bufs,
>  	.vidioc_expbuf			= v4l2_m2m_ioctl_expbuf,
> +	.vidioc_delete_bufs		= v4l2_m2m_ioctl_delete_bufs,
>  
>  	.vidioc_streamon		= v4l2_m2m_ioctl_streamon,
>  	.vidioc_streamoff		= v4l2_m2m_ioctl_streamoff,
> @@ -749,6 +750,7 @@ int visl_queue_init(void *priv, struct vb2_queue *src_vq,
>  	dst_vq->mem_ops = &vb2_vmalloc_memops;
>  	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
>  	dst_vq->lock = &ctx->vb_mutex;
> +	dst_vq->supports_delete_bufs = true;
>  
>  	return vb2_queue_init(dst_vq);
>  }
> diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
> index 11b8520d9f57..ad37babb54a2 100644
> --- a/drivers/media/test-drivers/vivid/vivid-core.c
> +++ b/drivers/media/test-drivers/vivid/vivid-core.c
> @@ -769,6 +769,7 @@ static const struct v4l2_ioctl_ops vivid_ioctl_ops = {
>  	.vidioc_expbuf			= vb2_ioctl_expbuf,
>  	.vidioc_streamon		= vb2_ioctl_streamon,
>  	.vidioc_streamoff		= vb2_ioctl_streamoff,
> +	.vidioc_delete_bufs		= vb2_ioctl_delete_bufs,
>  
>  	.vidioc_enum_input		= vivid_enum_input,
>  	.vidioc_g_input			= vivid_g_input,
> @@ -883,12 +884,18 @@ static int vivid_create_queue(struct vivid_dev *dev,
>  	 * PAGE_SHIFT > 12, but then max_num_buffers will be clamped by
>  	 * videobuf2-core.c to MAX_BUFFER_INDEX.
>  	 */
> -	if (buf_type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
> +	if (buf_type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
>  		q->max_num_buffers = 64;
> -	if (buf_type == V4L2_BUF_TYPE_SDR_CAPTURE)
> +		q->supports_delete_bufs = true;
> +	}
> +	if (buf_type == V4L2_BUF_TYPE_SDR_CAPTURE) {
>  		q->max_num_buffers = 1024;
> -	if (buf_type == V4L2_BUF_TYPE_VBI_CAPTURE)
> +		q->supports_delete_bufs = true;
> +	}
> +	if (buf_type == V4L2_BUF_TYPE_VBI_CAPTURE) {
>  		q->max_num_buffers = 32768;
> +		q->supports_delete_bufs = true;
> +	}
>  
>  	if (allocators[dev->inst] != 1)
>  		q->io_modes |= VB2_USERPTR;

Regards,

	Hans