[PATCH v2 0/3] media: add vb2_queue_change_type() helper

[PATCH v2 0/3] media: add vb2_queue_change_type() helper

[Tomi Valkeinen]

Hi,

Here's v2, with a small change in the vb2_queue_change_type help text,
and with vivid changed to use vb2_queue_change_type.

I'm not familiar with vivid, or even the v4l2 features it's using, so
please review with care.

v4l2-compliance passes, but I'm not sure if I tested all the
functionality vivid offers.

 Tomi

Tomi Valkeinen (3):
  media: videobuf2-v4l2.c: add vb2_queue_change_type() helper
  media: vivid: remove stream_sliced_vbi_cap field
  media: vivid: use vb2_queue_change_type

 .../media/common/videobuf2/videobuf2-v4l2.c   | 14 ++++++
 drivers/media/test-drivers/vivid/vivid-core.c | 44 ++++++++++++++++++-
 drivers/media/test-drivers/vivid/vivid-core.h |  1 -
 .../test-drivers/vivid/vivid-kthread-cap.c    |  2 +-
 .../media/test-drivers/vivid/vivid-vbi-cap.c  |  8 +---
 include/media/videobuf2-v4l2.h                | 15 +++++++
 6 files changed, 74 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] media: videobuf2-v4l2.c: add vb2_queue_change_type() helper

[Tomi Valkeinen]

On some platforms a video device can capture either video data or
metadata. The driver can implement vidioc functions for both video and
metadata, and use a single vb2_queue for the buffers. However, vb2_queue
requires choosing a single buffer type, which conflicts with the idea of
capturing either video or metadata.

The buffer type of vb2_queue can be changed, but it's not obvious how
this should be done in the drivers. To help this, add a new helper
function vb2_queue_change_type() which ensures the correct checks and
documents how it can be used.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 14 ++++++++++++++
 include/media/videobuf2-v4l2.h                  | 15 +++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 7e96f67c60ba..2988bb38ceb1 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -939,6 +939,20 @@ void vb2_queue_release(struct vb2_queue *q)
 }
 EXPORT_SYMBOL_GPL(vb2_queue_release);
 
+int vb2_queue_change_type(struct vb2_queue *q, unsigned int type)
+{
+	if (type == q->type)
+		return 0;
+
+	if (vb2_is_busy(q))
+		return -EBUSY;
+
+	q->type = type;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vb2_queue_change_type);
+
 __poll_t vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 {
 	struct video_device *vfd = video_devdata(file);
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index c203047eb834..12fa72a664cf 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -261,6 +261,21 @@ int __must_check vb2_queue_init_name(struct vb2_queue *q, const char *name);
  */
 void vb2_queue_release(struct vb2_queue *q);
 
+/**
+ * vb2_queue_change_type() - change the type of an inactive vb2_queue
+ * @q:		pointer to &struct vb2_queue with videobuf2 queue.
+ *
+ * This function changes the type of the vb2_queue. This is only possible
+ * if the queue is not busy (i.e. no buffers have been allocated).
+ *
+ * vb2_queue_change_type() can be used to support multiple buffer types using
+ * the same queue. The driver can implement v4l2_ioctl_ops.vidioc_reqbufs and
+ * v4l2_ioctl_ops.vidioc_create_bufs functions and call vb2_queue_change_type()
+ * before calling vb2_ioctl_reqbufs() or vb2_ioctl_create_bufs(), and thus
+ * "lock" the buffer type until the buffers have been released.
+ */
+int vb2_queue_change_type(struct vb2_queue *q, unsigned int type);
+
 /**
  * vb2_poll() - implements poll userspace operation
  * @q:		pointer to &struct vb2_queue with videobuf2 queue.
-- 
2.25.1

[PATCH v2 2/3] media: vivid: remove stream_sliced_vbi_cap field

[Tomi Valkeinen]

Vivid tracks the VBI capture mode in vivid_dev->stream_sliced_vbi_cap
field.  We can just look at the buffer type instead, and drop the field.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/test-drivers/vivid/vivid-core.h        | 1 -
 drivers/media/test-drivers/vivid/vivid-kthread-cap.c | 2 +-
 drivers/media/test-drivers/vivid/vivid-vbi-cap.c     | 6 ++----
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/media/test-drivers/vivid/vivid-core.h b/drivers/media/test-drivers/vivid/vivid-core.h
index 9c2d1470b597..9af7e843c2cf 100644
--- a/drivers/media/test-drivers/vivid/vivid-core.h
+++ b/drivers/media/test-drivers/vivid/vivid-core.h
@@ -428,7 +428,6 @@ struct vivid_dev {
 	u32				vbi_cap_seq_start;
 	u32				vbi_cap_seq_count;
 	bool				vbi_cap_streaming;
-	bool				stream_sliced_vbi_cap;
 	u32				meta_cap_seq_start;
 	u32				meta_cap_seq_count;
 	bool				meta_cap_streaming;
diff --git a/drivers/media/test-drivers/vivid/vivid-kthread-cap.c b/drivers/media/test-drivers/vivid/vivid-kthread-cap.c
index 67fb3c00f9ad..781763c193eb 100644
--- a/drivers/media/test-drivers/vivid/vivid-kthread-cap.c
+++ b/drivers/media/test-drivers/vivid/vivid-kthread-cap.c
@@ -750,7 +750,7 @@ static noinline_for_stack void vivid_thread_vid_cap_tick(struct vivid_dev *dev,
 
 		v4l2_ctrl_request_setup(vbi_cap_buf->vb.vb2_buf.req_obj.req,
 					&dev->ctrl_hdl_vbi_cap);
-		if (dev->stream_sliced_vbi_cap)
+		if (vbi_cap_buf->vb.vb2_buf.type == V4L2_BUF_TYPE_SLICED_VBI_CAPTURE)
 			vivid_sliced_vbi_cap_process(dev, vbi_cap_buf);
 		else
 			vivid_raw_vbi_cap_process(dev, vbi_cap_buf);
diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-cap.c b/drivers/media/test-drivers/vivid/vivid-vbi-cap.c
index 1a9348eea781..387df4ff01b0 100644
--- a/drivers/media/test-drivers/vivid/vivid-vbi-cap.c
+++ b/drivers/media/test-drivers/vivid/vivid-vbi-cap.c
@@ -255,9 +255,8 @@ int vidioc_s_fmt_vbi_cap(struct file *file, void *priv,
 
 	if (ret)
 		return ret;
-	if (dev->stream_sliced_vbi_cap && vb2_is_busy(&dev->vb_vbi_cap_q))
+	if (f->type != V4L2_BUF_TYPE_VBI_CAPTURE && vb2_is_busy(&dev->vb_vbi_cap_q))
 		return -EBUSY;
-	dev->stream_sliced_vbi_cap = false;
 	dev->vbi_cap_dev.queue->type = V4L2_BUF_TYPE_VBI_CAPTURE;
 	return 0;
 }
@@ -322,10 +321,9 @@ int vidioc_s_fmt_sliced_vbi_cap(struct file *file, void *fh, struct v4l2_format
 
 	if (ret)
 		return ret;
-	if (!dev->stream_sliced_vbi_cap && vb2_is_busy(&dev->vb_vbi_cap_q))
+	if (fmt->type != V4L2_BUF_TYPE_SLICED_VBI_CAPTURE && vb2_is_busy(&dev->vb_vbi_cap_q))
 		return -EBUSY;
 	dev->service_set_cap = vbi->service_set;
-	dev->stream_sliced_vbi_cap = true;
 	dev->vbi_cap_dev.queue->type = V4L2_BUF_TYPE_SLICED_VBI_CAPTURE;
 	return 0;
 }
-- 
2.25.1

[PATCH v2 3/3] media: vivid: use vb2_queue_change_type

[Tomi Valkeinen]

Use the new vb2_queue_change_type() function in .vidioc_reqbufs and
.vidioc_create_bufs instead of changing the queue type manually in
vidioc_s_fmt_vbi_cap() and vidioc_s_fmt_sliced_vbi_cap().

This allows for a more consistent behavior, as .vidioc_reqbufs and
.vidioc_create_bufs are when the queue will become "busy".

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
 drivers/media/test-drivers/vivid/vivid-core.c | 44 ++++++++++++++++++-
 .../media/test-drivers/vivid/vivid-vbi-cap.c  |  2 -
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
index 0dc65ef3aa14..70676f1ac268 100644
--- a/drivers/media/test-drivers/vivid/vivid-core.c
+++ b/drivers/media/test-drivers/vivid/vivid-core.c
@@ -656,6 +656,46 @@ static const struct v4l2_file_operations vivid_radio_fops = {
 	.unlocked_ioctl = video_ioctl2,
 };
 
+static int vidioc_reqbufs(struct file *file, void *priv,
+				 struct v4l2_requestbuffers *p)
+{
+	struct video_device *vdev = video_devdata(file);
+	int r;
+
+	/*
+	 * Sliced and raw VBI capture share the same queue so we must
+	 * change the type.
+	 */
+	if (p->type == V4L2_BUF_TYPE_SLICED_VBI_CAPTURE ||
+	    p->type == V4L2_BUF_TYPE_VBI_CAPTURE) {
+		r = vb2_queue_change_type(vdev->queue, p->type);
+		if (r)
+			return r;
+	}
+
+	return vb2_ioctl_reqbufs(file, priv, p);
+}
+
+static int vidioc_create_bufs(struct file *file, void *priv,
+				     struct v4l2_create_buffers *p)
+{
+	struct video_device *vdev = video_devdata(file);
+	int r;
+
+	/*
+	 * Sliced and raw VBI capture share the same queue so we must
+	 * change the type.
+	 */
+	if (p->format.type == V4L2_BUF_TYPE_SLICED_VBI_CAPTURE ||
+	    p->format.type == V4L2_BUF_TYPE_VBI_CAPTURE) {
+		r = vb2_queue_change_type(vdev->queue, p->format.type);
+		if (r)
+			return r;
+	}
+
+	return vb2_ioctl_create_bufs(file, priv, p);
+}
+
 static const struct v4l2_ioctl_ops vivid_ioctl_ops = {
 	.vidioc_querycap		= vidioc_querycap,
 
@@ -717,8 +757,8 @@ static const struct v4l2_ioctl_ops vivid_ioctl_ops = {
 	.vidioc_g_fbuf			= vidioc_g_fbuf,
 	.vidioc_s_fbuf			= vidioc_s_fbuf,
 
-	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
-	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
+	.vidioc_reqbufs			= vidioc_reqbufs,
+	.vidioc_create_bufs		= vidioc_create_bufs,
 	.vidioc_prepare_buf		= vb2_ioctl_prepare_buf,
 	.vidioc_querybuf		= vb2_ioctl_querybuf,
 	.vidioc_qbuf			= vb2_ioctl_qbuf,
diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-cap.c b/drivers/media/test-drivers/vivid/vivid-vbi-cap.c
index 387df4ff01b0..b65b02eeeb97 100644
--- a/drivers/media/test-drivers/vivid/vivid-vbi-cap.c
+++ b/drivers/media/test-drivers/vivid/vivid-vbi-cap.c
@@ -257,7 +257,6 @@ int vidioc_s_fmt_vbi_cap(struct file *file, void *priv,
 		return ret;
 	if (f->type != V4L2_BUF_TYPE_VBI_CAPTURE && vb2_is_busy(&dev->vb_vbi_cap_q))
 		return -EBUSY;
-	dev->vbi_cap_dev.queue->type = V4L2_BUF_TYPE_VBI_CAPTURE;
 	return 0;
 }
 
@@ -324,7 +323,6 @@ int vidioc_s_fmt_sliced_vbi_cap(struct file *file, void *fh, struct v4l2_format
 	if (fmt->type != V4L2_BUF_TYPE_SLICED_VBI_CAPTURE && vb2_is_busy(&dev->vb_vbi_cap_q))
 		return -EBUSY;
 	dev->service_set_cap = vbi->service_set;
-	dev->vbi_cap_dev.queue->type = V4L2_BUF_TYPE_SLICED_VBI_CAPTURE;
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH v2 1/3] media: videobuf2-v4l2.c: add vb2_queue_change_type() helper

[Tomi Valkeinen]

On 12/04/2021 14:02, Tomi Valkeinen wrote:
> On some platforms a video device can capture either video data or
> metadata. The driver can implement vidioc functions for both video and
> metadata, and use a single vb2_queue for the buffers. However, vb2_queue
> requires choosing a single buffer type, which conflicts with the idea of
> capturing either video or metadata.
> 
> The buffer type of vb2_queue can be changed, but it's not obvious how
> this should be done in the drivers. To help this, add a new helper
> function vb2_queue_change_type() which ensures the correct checks and
> documents how it can be used.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>   drivers/media/common/videobuf2/videobuf2-v4l2.c | 14 ++++++++++++++
>   include/media/videobuf2-v4l2.h                  | 15 +++++++++++++++
>   2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 7e96f67c60ba..2988bb38ceb1 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -939,6 +939,20 @@ void vb2_queue_release(struct vb2_queue *q)
>   }
>   EXPORT_SYMBOL_GPL(vb2_queue_release);
>   
> +int vb2_queue_change_type(struct vb2_queue *q, unsigned int type)
> +{
> +	if (type == q->type)
> +		return 0;
> +
> +	if (vb2_is_busy(q))
> +		return -EBUSY;
> +
> +	q->type = type;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vb2_queue_change_type);
> +
>   __poll_t vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>   {
>   	struct video_device *vfd = video_devdata(file);
> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> index c203047eb834..12fa72a664cf 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -261,6 +261,21 @@ int __must_check vb2_queue_init_name(struct vb2_queue *q, const char *name);
>    */
>   void vb2_queue_release(struct vb2_queue *q);
>   
> +/**
> + * vb2_queue_change_type() - change the type of an inactive vb2_queue
> + * @q:		pointer to &struct vb2_queue with videobuf2 queue.
> + *
> + * This function changes the type of the vb2_queue. This is only possible
> + * if the queue is not busy (i.e. no buffers have been allocated).
> + *
> + * vb2_queue_change_type() can be used to support multiple buffer types using
> + * the same queue. The driver can implement v4l2_ioctl_ops.vidioc_reqbufs and
> + * v4l2_ioctl_ops.vidioc_create_bufs functions and call vb2_queue_change_type()
> + * before calling vb2_ioctl_reqbufs() or vb2_ioctl_create_bufs(), and thus
> + * "lock" the buffer type until the buffers have been released.
> + */
> +int vb2_queue_change_type(struct vb2_queue *q, unsigned int type);
> +
>   /**
>    * vb2_poll() - implements poll userspace operation
>    * @q:		pointer to &struct vb2_queue with videobuf2 queue.
> 

This is missing doc for the type parameter. I have added this to my branch:

@type:      The type to change to (V4L2_BUF_TYPE_VIDEO_*)

I'll send a v3 after I get feedback on the vivid patches.

  Tomi

Re: [PATCH v2 1/3] media: videobuf2-v4l2.c: add vb2_queue_change_type() helper

[Tomasz Figa]

Hi Tomi,

On Mon, Apr 12, 2021 at 02:02:09PM +0300, Tomi Valkeinen wrote:
> On some platforms a video device can capture either video data or
> metadata. The driver can implement vidioc functions for both video and
> metadata, and use a single vb2_queue for the buffers. However, vb2_queue
> requires choosing a single buffer type, which conflicts with the idea of
> capturing either video or metadata.
> 
> The buffer type of vb2_queue can be changed, but it's not obvious how
> this should be done in the drivers. To help this, add a new helper
> function vb2_queue_change_type() which ensures the correct checks and
> documents how it can be used.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 14 ++++++++++++++
>  include/media/videobuf2-v4l2.h                  | 15 +++++++++++++++
>  2 files changed, 29 insertions(+)
> 

Good to see you contributing to the media subsystem. Not sure if you
still remember me from the Common Display Framework discussions. ;)

Anyway, thanks for the patch. I think the code itself is okay, but I'm
wondering why the driver couldn't just have two queues, one for each
type?

Best regards,
Tomasz

> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 7e96f67c60ba..2988bb38ceb1 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -939,6 +939,20 @@ void vb2_queue_release(struct vb2_queue *q)
>  }
>  EXPORT_SYMBOL_GPL(vb2_queue_release);
>  
> +int vb2_queue_change_type(struct vb2_queue *q, unsigned int type)
> +{
> +	if (type == q->type)
> +		return 0;
> +
> +	if (vb2_is_busy(q))
> +		return -EBUSY;
> +
> +	q->type = type;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vb2_queue_change_type);
> +
>  __poll_t vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>  {
>  	struct video_device *vfd = video_devdata(file);
> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> index c203047eb834..12fa72a664cf 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -261,6 +261,21 @@ int __must_check vb2_queue_init_name(struct vb2_queue *q, const char *name);
>   */
>  void vb2_queue_release(struct vb2_queue *q);
>  
> +/**
> + * vb2_queue_change_type() - change the type of an inactive vb2_queue
> + * @q:		pointer to &struct vb2_queue with videobuf2 queue.
> + *
> + * This function changes the type of the vb2_queue. This is only possible
> + * if the queue is not busy (i.e. no buffers have been allocated).
> + *
> + * vb2_queue_change_type() can be used to support multiple buffer types using
> + * the same queue. The driver can implement v4l2_ioctl_ops.vidioc_reqbufs and
> + * v4l2_ioctl_ops.vidioc_create_bufs functions and call vb2_queue_change_type()
> + * before calling vb2_ioctl_reqbufs() or vb2_ioctl_create_bufs(), and thus
> + * "lock" the buffer type until the buffers have been released.
> + */
> +int vb2_queue_change_type(struct vb2_queue *q, unsigned int type);
> +
>  /**
>   * vb2_poll() - implements poll userspace operation
>   * @q:		pointer to &struct vb2_queue with videobuf2 queue.
> -- 
> 2.25.1
> 

Re: [PATCH v2 1/3] media: videobuf2-v4l2.c: add vb2_queue_change_type() helper

[Tomi Valkeinen]

Hi Tomasz,

On 04/06/2021 14:13, Tomasz Figa wrote:
> Hi Tomi,
> 
> On Mon, Apr 12, 2021 at 02:02:09PM +0300, Tomi Valkeinen wrote:
>> On some platforms a video device can capture either video data or
>> metadata. The driver can implement vidioc functions for both video and
>> metadata, and use a single vb2_queue for the buffers. However, vb2_queue
>> requires choosing a single buffer type, which conflicts with the idea of
>> capturing either video or metadata.
>>
>> The buffer type of vb2_queue can be changed, but it's not obvious how
>> this should be done in the drivers. To help this, add a new helper
>> function vb2_queue_change_type() which ensures the correct checks and
>> documents how it can be used.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>   drivers/media/common/videobuf2/videobuf2-v4l2.c | 14 ++++++++++++++
>>   include/media/videobuf2-v4l2.h                  | 15 +++++++++++++++
>>   2 files changed, 29 insertions(+)
>>
> 
> Good to see you contributing to the media subsystem. Not sure if you
> still remember me from the Common Display Framework discussions. ;)

I barely remember CDF... ;)

> Anyway, thanks for the patch. I think the code itself is okay, but I'm
> wondering why the driver couldn't just have two queues, one for each
> type?

There was an email thread about this:

https://www.spinics.net/lists/linux-media/msg189144.html

struct video_device has 'queue' field, so if you have two queues, you'd 
need to change the vd->queue based on the format. Possibly that could be 
a solution too (and, if I recall right, that's what I initially tried as 
a quick hack). Changing the whole queue sounds riskier than changing 
just the type.

  Tomi

Re: [PATCH v2 1/3] media: videobuf2-v4l2.c: add vb2_queue_change_type() helper

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Apr 12, 2021 at 02:02:09PM +0300, Tomi Valkeinen wrote:
> On some platforms a video device can capture either video data or
> metadata. The driver can implement vidioc functions for both video and
> metadata, and use a single vb2_queue for the buffers. However, vb2_queue
> requires choosing a single buffer type, which conflicts with the idea of
> capturing either video or metadata.
> 
> The buffer type of vb2_queue can be changed, but it's not obvious how
> this should be done in the drivers. To help this, add a new helper
> function vb2_queue_change_type() which ensures the correct checks and
> documents how it can be used.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 14 ++++++++++++++
>  include/media/videobuf2-v4l2.h                  | 15 +++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 7e96f67c60ba..2988bb38ceb1 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -939,6 +939,20 @@ void vb2_queue_release(struct vb2_queue *q)
>  }
>  EXPORT_SYMBOL_GPL(vb2_queue_release);
>  
> +int vb2_queue_change_type(struct vb2_queue *q, unsigned int type)
> +{
> +	if (type == q->type)
> +		return 0;
> +
> +	if (vb2_is_busy(q))
> +		return -EBUSY;
> +
> +	q->type = type;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vb2_queue_change_type);
> +
>  __poll_t vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
>  {
>  	struct video_device *vfd = video_devdata(file);
> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> index c203047eb834..12fa72a664cf 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -261,6 +261,21 @@ int __must_check vb2_queue_init_name(struct vb2_queue *q, const char *name);
>   */
>  void vb2_queue_release(struct vb2_queue *q);
>  
> +/**
> + * vb2_queue_change_type() - change the type of an inactive vb2_queue
> + * @q:		pointer to &struct vb2_queue with videobuf2 queue.
> + *
> + * This function changes the type of the vb2_queue. This is only possible
> + * if the queue is not busy (i.e. no buffers have been allocated).
> + *
> + * vb2_queue_change_type() can be used to support multiple buffer types using
> + * the same queue. The driver can implement v4l2_ioctl_ops.vidioc_reqbufs and

I think this should be &v4l2_ioctl_ops.vidioc_reqbufs() (same below) to
generate links properly.

> + * v4l2_ioctl_ops.vidioc_create_bufs functions and call vb2_queue_change_type()
> + * before calling vb2_ioctl_reqbufs() or vb2_ioctl_create_bufs(), and thus
> + * "lock" the buffer type until the buffers have been released.

It would be nice if selection of the type could be moved to the
.queue_setup() operation, but that's more difficult and would require
rework of the reqbufs and create_bufs implementations that may be
tricky.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> + */
> +int vb2_queue_change_type(struct vb2_queue *q, unsigned int type);
> +
>  /**
>   * vb2_poll() - implements poll userspace operation
>   * @q:		pointer to &struct vb2_queue with videobuf2 queue.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] media: vivid: remove stream_sliced_vbi_cap field

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Apr 12, 2021 at 02:02:10PM +0300, Tomi Valkeinen wrote:
> Vivid tracks the VBI capture mode in vivid_dev->stream_sliced_vbi_cap
> field.  We can just look at the buffer type instead, and drop the field.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/test-drivers/vivid/vivid-core.h        | 1 -
>  drivers/media/test-drivers/vivid/vivid-kthread-cap.c | 2 +-
>  drivers/media/test-drivers/vivid/vivid-vbi-cap.c     | 6 ++----
>  3 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vivid/vivid-core.h b/drivers/media/test-drivers/vivid/vivid-core.h
> index 9c2d1470b597..9af7e843c2cf 100644
> --- a/drivers/media/test-drivers/vivid/vivid-core.h
> +++ b/drivers/media/test-drivers/vivid/vivid-core.h
> @@ -428,7 +428,6 @@ struct vivid_dev {
>  	u32				vbi_cap_seq_start;
>  	u32				vbi_cap_seq_count;
>  	bool				vbi_cap_streaming;
> -	bool				stream_sliced_vbi_cap;
>  	u32				meta_cap_seq_start;
>  	u32				meta_cap_seq_count;
>  	bool				meta_cap_streaming;
> diff --git a/drivers/media/test-drivers/vivid/vivid-kthread-cap.c b/drivers/media/test-drivers/vivid/vivid-kthread-cap.c
> index 67fb3c00f9ad..781763c193eb 100644
> --- a/drivers/media/test-drivers/vivid/vivid-kthread-cap.c
> +++ b/drivers/media/test-drivers/vivid/vivid-kthread-cap.c
> @@ -750,7 +750,7 @@ static noinline_for_stack void vivid_thread_vid_cap_tick(struct vivid_dev *dev,
>  
>  		v4l2_ctrl_request_setup(vbi_cap_buf->vb.vb2_buf.req_obj.req,
>  					&dev->ctrl_hdl_vbi_cap);
> -		if (dev->stream_sliced_vbi_cap)
> +		if (vbi_cap_buf->vb.vb2_buf.type == V4L2_BUF_TYPE_SLICED_VBI_CAPTURE)
>  			vivid_sliced_vbi_cap_process(dev, vbi_cap_buf);
>  		else
>  			vivid_raw_vbi_cap_process(dev, vbi_cap_buf);
> diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-cap.c b/drivers/media/test-drivers/vivid/vivid-vbi-cap.c
> index 1a9348eea781..387df4ff01b0 100644
> --- a/drivers/media/test-drivers/vivid/vivid-vbi-cap.c
> +++ b/drivers/media/test-drivers/vivid/vivid-vbi-cap.c
> @@ -255,9 +255,8 @@ int vidioc_s_fmt_vbi_cap(struct file *file, void *priv,
>  
>  	if (ret)
>  		return ret;
> -	if (dev->stream_sliced_vbi_cap && vb2_is_busy(&dev->vb_vbi_cap_q))
> +	if (f->type != V4L2_BUF_TYPE_VBI_CAPTURE && vb2_is_busy(&dev->vb_vbi_cap_q))

I think think this is correct. The code checks if the currently
configured format is the sliced variant, and you replace this with a
check for the new format.

>  		return -EBUSY;
> -	dev->stream_sliced_vbi_cap = false;
>  	dev->vbi_cap_dev.queue->type = V4L2_BUF_TYPE_VBI_CAPTURE;
>  	return 0;
>  }
> @@ -322,10 +321,9 @@ int vidioc_s_fmt_sliced_vbi_cap(struct file *file, void *fh, struct v4l2_format
>  
>  	if (ret)
>  		return ret;
> -	if (!dev->stream_sliced_vbi_cap && vb2_is_busy(&dev->vb_vbi_cap_q))
> +	if (fmt->type != V4L2_BUF_TYPE_SLICED_VBI_CAPTURE && vb2_is_busy(&dev->vb_vbi_cap_q))
>  		return -EBUSY;
>  	dev->service_set_cap = vbi->service_set;
> -	dev->stream_sliced_vbi_cap = true;
>  	dev->vbi_cap_dev.queue->type = V4L2_BUF_TYPE_SLICED_VBI_CAPTURE;
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] media: vivid: use vb2_queue_change_type

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Mon, Apr 12, 2021 at 02:02:11PM +0300, Tomi Valkeinen wrote:
> Use the new vb2_queue_change_type() function in .vidioc_reqbufs and
> .vidioc_create_bufs instead of changing the queue type manually in
> vidioc_s_fmt_vbi_cap() and vidioc_s_fmt_sliced_vbi_cap().
> 
> This allows for a more consistent behavior, as .vidioc_reqbufs and
> .vidioc_create_bufs are when the queue will become "busy".
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
>  drivers/media/test-drivers/vivid/vivid-core.c | 44 ++++++++++++++++++-
>  .../media/test-drivers/vivid/vivid-vbi-cap.c  |  2 -
>  2 files changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
> index 0dc65ef3aa14..70676f1ac268 100644
> --- a/drivers/media/test-drivers/vivid/vivid-core.c
> +++ b/drivers/media/test-drivers/vivid/vivid-core.c
> @@ -656,6 +656,46 @@ static const struct v4l2_file_operations vivid_radio_fops = {
>  	.unlocked_ioctl = video_ioctl2,
>  };
>  
> +static int vidioc_reqbufs(struct file *file, void *priv,
> +				 struct v4l2_requestbuffers *p)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	int r;

s/r/ret/ to match the driver's coding style.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +
> +	/*
> +	 * Sliced and raw VBI capture share the same queue so we must
> +	 * change the type.
> +	 */
> +	if (p->type == V4L2_BUF_TYPE_SLICED_VBI_CAPTURE ||
> +	    p->type == V4L2_BUF_TYPE_VBI_CAPTURE) {
> +		r = vb2_queue_change_type(vdev->queue, p->type);
> +		if (r)
> +			return r;
> +	}
> +
> +	return vb2_ioctl_reqbufs(file, priv, p);
> +}
> +
> +static int vidioc_create_bufs(struct file *file, void *priv,
> +				     struct v4l2_create_buffers *p)
> +{
> +	struct video_device *vdev = video_devdata(file);
> +	int r;
> +
> +	/*
> +	 * Sliced and raw VBI capture share the same queue so we must
> +	 * change the type.
> +	 */
> +	if (p->format.type == V4L2_BUF_TYPE_SLICED_VBI_CAPTURE ||
> +	    p->format.type == V4L2_BUF_TYPE_VBI_CAPTURE) {
> +		r = vb2_queue_change_type(vdev->queue, p->format.type);
> +		if (r)
> +			return r;
> +	}
> +
> +	return vb2_ioctl_create_bufs(file, priv, p);
> +}
> +
>  static const struct v4l2_ioctl_ops vivid_ioctl_ops = {
>  	.vidioc_querycap		= vidioc_querycap,
>  
> @@ -717,8 +757,8 @@ static const struct v4l2_ioctl_ops vivid_ioctl_ops = {
>  	.vidioc_g_fbuf			= vidioc_g_fbuf,
>  	.vidioc_s_fbuf			= vidioc_s_fbuf,
>  
> -	.vidioc_reqbufs			= vb2_ioctl_reqbufs,
> -	.vidioc_create_bufs		= vb2_ioctl_create_bufs,
> +	.vidioc_reqbufs			= vidioc_reqbufs,
> +	.vidioc_create_bufs		= vidioc_create_bufs,
>  	.vidioc_prepare_buf		= vb2_ioctl_prepare_buf,
>  	.vidioc_querybuf		= vb2_ioctl_querybuf,
>  	.vidioc_qbuf			= vb2_ioctl_qbuf,
> diff --git a/drivers/media/test-drivers/vivid/vivid-vbi-cap.c b/drivers/media/test-drivers/vivid/vivid-vbi-cap.c
> index 387df4ff01b0..b65b02eeeb97 100644
> --- a/drivers/media/test-drivers/vivid/vivid-vbi-cap.c
> +++ b/drivers/media/test-drivers/vivid/vivid-vbi-cap.c
> @@ -257,7 +257,6 @@ int vidioc_s_fmt_vbi_cap(struct file *file, void *priv,
>  		return ret;
>  	if (f->type != V4L2_BUF_TYPE_VBI_CAPTURE && vb2_is_busy(&dev->vb_vbi_cap_q))
>  		return -EBUSY;
> -	dev->vbi_cap_dev.queue->type = V4L2_BUF_TYPE_VBI_CAPTURE;
>  	return 0;
>  }
>  
> @@ -324,7 +323,6 @@ int vidioc_s_fmt_sliced_vbi_cap(struct file *file, void *fh, struct v4l2_format
>  	if (fmt->type != V4L2_BUF_TYPE_SLICED_VBI_CAPTURE && vb2_is_busy(&dev->vb_vbi_cap_q))
>  		return -EBUSY;
>  	dev->service_set_cap = vbi->service_set;
> -	dev->vbi_cap_dev.queue->type = V4L2_BUF_TYPE_SLICED_VBI_CAPTURE;
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/3] media: videobuf2-v4l2.c: add vb2_queue_change_type() helper

[Tomasz Figa]

On Fri, Jun 4, 2021 at 11:09 PM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> Hi Tomasz,
>
> On 04/06/2021 14:13, Tomasz Figa wrote:
> > Hi Tomi,
> >
> > On Mon, Apr 12, 2021 at 02:02:09PM +0300, Tomi Valkeinen wrote:
> >> On some platforms a video device can capture either video data or
> >> metadata. The driver can implement vidioc functions for both video and
> >> metadata, and use a single vb2_queue for the buffers. However, vb2_queue
> >> requires choosing a single buffer type, which conflicts with the idea of
> >> capturing either video or metadata.
> >>
> >> The buffer type of vb2_queue can be changed, but it's not obvious how
> >> this should be done in the drivers. To help this, add a new helper
> >> function vb2_queue_change_type() which ensures the correct checks and
> >> documents how it can be used.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >>   drivers/media/common/videobuf2/videobuf2-v4l2.c | 14 ++++++++++++++
> >>   include/media/videobuf2-v4l2.h                  | 15 +++++++++++++++
> >>   2 files changed, 29 insertions(+)
> >>
> >
> > Good to see you contributing to the media subsystem. Not sure if you
> > still remember me from the Common Display Framework discussions. ;)
>
> I barely remember CDF... ;)
>
> > Anyway, thanks for the patch. I think the code itself is okay, but I'm
> > wondering why the driver couldn't just have two queues, one for each
> > type?
>
> There was an email thread about this:
>
> https://www.spinics.net/lists/linux-media/msg189144.html
>
> struct video_device has 'queue' field, so if you have two queues, you'd
> need to change the vd->queue based on the format. Possibly that could be
> a solution too (and, if I recall right, that's what I initially tried as
> a quick hack). Changing the whole queue sounds riskier than changing
> just the type.

Okay, I see. Thanks for the pointer.

Generally I'm fine with this, although it's worth noting that it's not
true that one video_device can only have 1 queue. See the numerous
mem-to-mem devices, which use two queues simultaneously.

Anyway,

Acked-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz