* [PATCH v2 0/3] media: add vb2_queue_change_type() helper
@ 2021-04-12 11:02 Tomi Valkeinen
2021-04-12 11:02 ` [PATCH v2 1/3] media: videobuf2-v4l2.c: " Tomi Valkeinen
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Tomi Valkeinen @ 2021-04-12 11:02 UTC (permalink / raw)
To: Tomasz Figa, Marek Szyprowski, Mauro Carvalho Chehab,
Hans Verkuil, linux-media, Laurent Pinchart
Cc: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] media: videobuf2-v4l2.c: add vb2_queue_change_type() helper
2021-04-12 11:02 [PATCH v2 0/3] media: add vb2_queue_change_type() helper Tomi Valkeinen
@ 2021-04-12 11:02 ` Tomi Valkeinen
2021-04-23 8:40 ` Tomi Valkeinen
` (2 more replies)
2021-04-12 11:02 ` [PATCH v2 2/3] media: vivid: remove stream_sliced_vbi_cap field Tomi Valkeinen
2021-04-12 11:02 ` [PATCH v2 3/3] media: vivid: use vb2_queue_change_type Tomi Valkeinen
2 siblings, 3 replies; 11+ messages in thread
From: Tomi Valkeinen @ 2021-04-12 11:02 UTC (permalink / raw)
To: Tomasz Figa, Marek Szyprowski, Mauro Carvalho Chehab,
Hans Verkuil, linux-media, Laurent Pinchart
Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] media: vivid: remove stream_sliced_vbi_cap field
2021-04-12 11:02 [PATCH v2 0/3] media: add vb2_queue_change_type() helper Tomi Valkeinen
2021-04-12 11:02 ` [PATCH v2 1/3] media: videobuf2-v4l2.c: " Tomi Valkeinen
@ 2021-04-12 11:02 ` Tomi Valkeinen
2021-06-04 14:41 ` Laurent Pinchart
2021-04-12 11:02 ` [PATCH v2 3/3] media: vivid: use vb2_queue_change_type Tomi Valkeinen
2 siblings, 1 reply; 11+ messages in thread
From: Tomi Valkeinen @ 2021-04-12 11:02 UTC (permalink / raw)
To: Tomasz Figa, Marek Szyprowski, Mauro Carvalho Chehab,
Hans Verkuil, linux-media, Laurent Pinchart
Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] media: vivid: use vb2_queue_change_type
2021-04-12 11:02 [PATCH v2 0/3] media: add vb2_queue_change_type() helper Tomi Valkeinen
2021-04-12 11:02 ` [PATCH v2 1/3] media: videobuf2-v4l2.c: " Tomi Valkeinen
2021-04-12 11:02 ` [PATCH v2 2/3] media: vivid: remove stream_sliced_vbi_cap field Tomi Valkeinen
@ 2021-04-12 11:02 ` Tomi Valkeinen
2021-06-04 14:44 ` Laurent Pinchart
2 siblings, 1 reply; 11+ messages in thread
From: Tomi Valkeinen @ 2021-04-12 11:02 UTC (permalink / raw)
To: Tomasz Figa, Marek Szyprowski, Mauro Carvalho Chehab,
Hans Verkuil, linux-media, Laurent Pinchart
Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] media: videobuf2-v4l2.c: add vb2_queue_change_type() helper
2021-04-12 11:02 ` [PATCH v2 1/3] media: videobuf2-v4l2.c: " Tomi Valkeinen
@ 2021-04-23 8:40 ` Tomi Valkeinen
2021-06-04 11:13 ` Tomasz Figa
2021-06-04 14:38 ` Laurent Pinchart
2 siblings, 0 replies; 11+ messages in thread
From: Tomi Valkeinen @ 2021-04-23 8:40 UTC (permalink / raw)
To: Tomasz Figa, Marek Szyprowski, Mauro Carvalho Chehab,
Hans Verkuil, linux-media, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] media: videobuf2-v4l2.c: add vb2_queue_change_type() helper
2021-04-12 11:02 ` [PATCH v2 1/3] media: videobuf2-v4l2.c: " Tomi Valkeinen
2021-04-23 8:40 ` Tomi Valkeinen
@ 2021-06-04 11:13 ` Tomasz Figa
2021-06-04 14:09 ` Tomi Valkeinen
2021-06-04 14:38 ` Laurent Pinchart
2 siblings, 1 reply; 11+ messages in thread
From: Tomasz Figa @ 2021-06-04 11:13 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Marek Szyprowski, Mauro Carvalho Chehab, Hans Verkuil,
linux-media, Laurent Pinchart
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] media: videobuf2-v4l2.c: add vb2_queue_change_type() helper
2021-06-04 11:13 ` Tomasz Figa
@ 2021-06-04 14:09 ` Tomi Valkeinen
2021-06-08 5:11 ` Tomasz Figa
0 siblings, 1 reply; 11+ messages in thread
From: Tomi Valkeinen @ 2021-06-04 14:09 UTC (permalink / raw)
To: Tomasz Figa
Cc: Marek Szyprowski, Mauro Carvalho Chehab, Hans Verkuil,
linux-media, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] media: videobuf2-v4l2.c: add vb2_queue_change_type() helper
2021-04-12 11:02 ` [PATCH v2 1/3] media: videobuf2-v4l2.c: " Tomi Valkeinen
2021-04-23 8:40 ` Tomi Valkeinen
2021-06-04 11:13 ` Tomasz Figa
@ 2021-06-04 14:38 ` Laurent Pinchart
2 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2021-06-04 14:38 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Tomasz Figa, Marek Szyprowski, Mauro Carvalho Chehab,
Hans Verkuil, linux-media
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] media: vivid: remove stream_sliced_vbi_cap field
2021-04-12 11:02 ` [PATCH v2 2/3] media: vivid: remove stream_sliced_vbi_cap field Tomi Valkeinen
@ 2021-06-04 14:41 ` Laurent Pinchart
0 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2021-06-04 14:41 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Tomasz Figa, Marek Szyprowski, Mauro Carvalho Chehab,
Hans Verkuil, linux-media
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] media: vivid: use vb2_queue_change_type
2021-04-12 11:02 ` [PATCH v2 3/3] media: vivid: use vb2_queue_change_type Tomi Valkeinen
@ 2021-06-04 14:44 ` Laurent Pinchart
0 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2021-06-04 14:44 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Tomasz Figa, Marek Szyprowski, Mauro Carvalho Chehab,
Hans Verkuil, linux-media
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/3] media: videobuf2-v4l2.c: add vb2_queue_change_type() helper
2021-06-04 14:09 ` Tomi Valkeinen
@ 2021-06-08 5:11 ` Tomasz Figa
0 siblings, 0 replies; 11+ messages in thread
From: Tomasz Figa @ 2021-06-08 5:11 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Marek Szyprowski, Mauro Carvalho Chehab, Hans Verkuil,
Linux Media Mailing List, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-06-08 5:13 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-12 11:02 [PATCH v2 0/3] media: add vb2_queue_change_type() helper Tomi Valkeinen
2021-04-12 11:02 ` [PATCH v2 1/3] media: videobuf2-v4l2.c: " Tomi Valkeinen
2021-04-23 8:40 ` Tomi Valkeinen
2021-06-04 11:13 ` Tomasz Figa
2021-06-04 14:09 ` Tomi Valkeinen
2021-06-08 5:11 ` Tomasz Figa
2021-06-04 14:38 ` Laurent Pinchart
2021-04-12 11:02 ` [PATCH v2 2/3] media: vivid: remove stream_sliced_vbi_cap field Tomi Valkeinen
2021-06-04 14:41 ` Laurent Pinchart
2021-04-12 11:02 ` [PATCH v2 3/3] media: vivid: use vb2_queue_change_type Tomi Valkeinen
2021-06-04 14:44 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).