[PATCHv2 0/9] vb2/cedrus: add tag support

[PATCHv2 0/9] vb2/cedrus: add tag support

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

As was discussed here (among other places):

https://lkml.org/lkml/2018/10/19/440

using capture queue buffer indices to refer to reference frames is
not a good idea. A better idea is to use a 'tag' where the
application can assign a u64 tag to an output buffer, which is then 
copied to the capture buffer(s) derived from the output buffer.

A u64 is chosen since this allows userspace to also use pointers to
internal structures as 'tag'.

The first three patches add core tag support, the next patch document
the tag support, then a new helper function is added to v4l2-mem2mem.c
to easily copy data from a source to a destination buffer that drivers
can use.

Next a new supports_tags vb2_queue flag is added to indicate that
the driver supports tags. Ideally this should not be necessary, but
that would require that all m2m drivers are converted to using the
new helper function introduced in the previous patch. That takes more
time then I have now since we want to get this in for 4.20.

Finally the vim2m, vicodec and cedrus drivers are converted to support
tags.

I also removed the 'pad' fields from the mpeg2 control structs (it
should never been added in the first place) and aligned the structs
to a u32 boundary (u64 for the tag values).

Note that this might change further (Paul suggested using bitfields).

Also note that the cedrus code doesn't set the sequence counter, that's
something that should still be added before this driver can be moved
out of staging.

Note: if no buffer is found for a certain tag, then the dma address
is just set to 0. That happened before as well with invalid buffer
indices. This should be checked in the driver!

The previous RFC series was tested successfully with the cedrus driver.

Regards,

        Hans

Changes since v1:

- changed to a u32 tag. Using a 64 bit tag was overly complicated due
  to the bad layout of the v4l2_buffer struct, and there is no real
  need for it by applications.

Main changes since the RFC:

- Added new buffer capability flag
- Added m2m helper to copy data between buffers
- Added documentation
- Added tag logging in v4l2-ioctl.c

Hans Verkuil (9):
  videodev2.h: add tag support
  vb2: add tag support
  v4l2-ioctl.c: log v4l2_buffer tag
  buffer.rst: document the new buffer tag feature.
  v4l2-mem2mem: add v4l2_m2m_buf_copy_data helper function
  vb2: add new supports_tags queue flag
  vim2m: add tag support
  vicodec: add tag support
  cedrus: add tag support

 Documentation/media/uapi/v4l/buffer.rst       | 32 +++++++++----
 .../media/uapi/v4l/vidioc-reqbufs.rst         |  4 ++
 .../media/common/videobuf2/videobuf2-v4l2.c   | 45 ++++++++++++++++---
 drivers/media/platform/vicodec/vicodec-core.c | 14 ++----
 drivers/media/platform/vim2m.c                | 14 ++----
 drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ----
 drivers/media/v4l2-core/v4l2-ioctl.c          |  9 ++--
 drivers/media/v4l2-core/v4l2-mem2mem.c        | 23 ++++++++++
 drivers/staging/media/sunxi/cedrus/cedrus.h   |  9 ++--
 .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++-----
 .../staging/media/sunxi/cedrus/cedrus_video.c |  2 +
 include/media/v4l2-mem2mem.h                  | 21 +++++++++
 include/media/videobuf2-core.h                |  2 +
 include/media/videobuf2-v4l2.h                | 21 ++++++++-
 include/uapi/linux/v4l2-controls.h            | 14 +++---
 include/uapi/linux/videodev2.h                |  9 +++-
 17 files changed, 178 insertions(+), 73 deletions(-)

-- 
2.19.1

[PATCHv2 1/9] videodev2.h: add tag support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add support for 'tags' to struct v4l2_buffer. These can be used
by m2m devices so userspace can set a tag for an output buffer and
this value will then be copied to the capture buffer(s).

This tag can be used to refer to capture buffers, something that
is needed by stateless HW codecs.

The new V4L2_BUF_CAP_SUPPORTS_TAGS capability indicates whether
or not tags are supported.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/uapi/linux/videodev2.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index c8e8ff810190..173a94d2cbef 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -879,6 +879,7 @@ struct v4l2_requestbuffers {
 #define V4L2_BUF_CAP_SUPPORTS_USERPTR	(1 << 1)
 #define V4L2_BUF_CAP_SUPPORTS_DMABUF	(1 << 2)
 #define V4L2_BUF_CAP_SUPPORTS_REQUESTS	(1 << 3)
+#define V4L2_BUF_CAP_SUPPORTS_TAGS	(1 << 4)
 
 /**
  * struct v4l2_plane - plane info for multi-planar buffers
@@ -923,6 +924,7 @@ struct v4l2_plane {
  * @field:	enum v4l2_field; field order of the image in the buffer
  * @timestamp:	frame timestamp
  * @timecode:	frame timecode
+ * @tag:	buffer tag
  * @sequence:	sequence count of this frame
  * @memory:	enum v4l2_memory; the method, in which the actual video data is
  *		passed
@@ -950,7 +952,10 @@ struct v4l2_buffer {
 	__u32			flags;
 	__u32			field;
 	struct timeval		timestamp;
-	struct v4l2_timecode	timecode;
+	union {
+		struct v4l2_timecode	timecode;
+		__u32			tag;
+	};
 	__u32			sequence;
 
 	/* memory location */
@@ -988,6 +993,8 @@ struct v4l2_buffer {
 #define V4L2_BUF_FLAG_IN_REQUEST		0x00000080
 /* timecode field is valid */
 #define V4L2_BUF_FLAG_TIMECODE			0x00000100
+/* tag field is valid */
+#define V4L2_BUF_FLAG_TAG			0x00000200
 /* Buffer is prepared for queuing */
 #define V4L2_BUF_FLAG_PREPARED			0x00000400
 /* Cache handling flags */
-- 
2.19.1

[PATCHv2 2/9] vb2: add tag support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Add support for tags to vb2. Besides just storing and setting
the tag this patch also adds the vb2_find_tag() function that
can be used to find a buffer with the given tag.

This function will only look at DEQUEUED and DONE buffers, i.e.
buffers that are already processed.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 .../media/common/videobuf2/videobuf2-v4l2.c   | 43 ++++++++++++++++---
 include/media/videobuf2-v4l2.h                | 21 ++++++++-
 2 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index a17033ab2c22..115f2868223a 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -50,7 +50,8 @@ module_param(debug, int, 0644);
 				 V4L2_BUF_FLAG_TIMESTAMP_MASK)
 /* Output buffer flags that should be passed on to the driver */
 #define V4L2_BUFFER_OUT_FLAGS	(V4L2_BUF_FLAG_PFRAME | V4L2_BUF_FLAG_BFRAME | \
-				 V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_TIMECODE)
+				 V4L2_BUF_FLAG_KEYFRAME | \
+				 V4L2_BUF_FLAG_TIMECODE | V4L2_BUF_FLAG_TAG)
 
 /*
  * __verify_planes_array() - verify that the planes array passed in struct
@@ -144,8 +145,11 @@ static void __copy_timestamp(struct vb2_buffer *vb, const void *pb)
 		 */
 		if (q->copy_timestamp)
 			vb->timestamp = timeval_to_ns(&b->timestamp);
-		vbuf->flags |= b->flags & V4L2_BUF_FLAG_TIMECODE;
-		if (b->flags & V4L2_BUF_FLAG_TIMECODE)
+		vbuf->flags |= b->flags &
+			(V4L2_BUF_FLAG_TIMECODE | V4L2_BUF_FLAG_TAG);
+		if (b->flags & V4L2_BUF_FLAG_TAG)
+			vbuf->tag = b->tag;
+		else if (b->flags & V4L2_BUF_FLAG_TIMECODE)
 			vbuf->timecode = b->timecode;
 	}
 };
@@ -195,6 +199,7 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
 	}
 	vbuf->sequence = 0;
 	vbuf->request_fd = -1;
+	vbuf->tag = 0;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
 		switch (b->memory) {
@@ -314,13 +319,19 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b
 	}
 
 	if (V4L2_TYPE_IS_OUTPUT(b->type)) {
+		if ((b->flags & V4L2_BUF_FLAG_TIMECODE) &&
+		    (b->flags & V4L2_BUF_FLAG_TAG)) {
+			dprintk(1, "buffer flag TIMECODE cannot be combined with flag TAG\n");
+			return -EINVAL;
+		}
+
 		/*
 		 * For output buffers mask out the timecode flag:
 		 * this will be handled later in vb2_qbuf().
 		 * The 'field' is valid metadata for this output buffer
 		 * and so that needs to be copied here.
 		 */
-		vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE;
+		vbuf->flags &= ~(V4L2_BUF_FLAG_TIMECODE | V4L2_BUF_FLAG_TAG);
 		vbuf->field = b->field;
 	} else {
 		/* Zero any output buffer flags as this is a capture buffer */
@@ -461,7 +472,10 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
 	b->flags = vbuf->flags;
 	b->field = vbuf->field;
 	b->timestamp = ns_to_timeval(vb->timestamp);
-	b->timecode = vbuf->timecode;
+	if (b->flags & V4L2_BUF_FLAG_TAG)
+		b->tag = vbuf->tag;
+	else if (b->flags & V4L2_BUF_FLAG_TIMECODE)
+		b->timecode = vbuf->timecode;
 	b->sequence = vbuf->sequence;
 	b->reserved2 = 0;
 	b->request_fd = 0;
@@ -587,6 +601,25 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
 	.copy_timestamp		= __copy_timestamp,
 };
 
+int vb2_find_tag(const struct vb2_queue *q, u32 tag,
+		 unsigned int start_idx)
+{
+	unsigned int i;
+
+	for (i = start_idx; i < q->num_buffers; i++) {
+		struct vb2_buffer *vb = q->bufs[i];
+		struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+
+		if ((vb->state == VB2_BUF_STATE_DEQUEUED ||
+		     vb->state == VB2_BUF_STATE_DONE) &&
+		    (vbuf->flags & V4L2_BUF_FLAG_TAG) &&
+		    vbuf->tag == tag)
+			return i;
+	}
+	return -1;
+}
+EXPORT_SYMBOL_GPL(vb2_find_tag);
+
 /*
  * vb2_querybuf() - query video buffer information
  * @q:		videobuf queue
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index 727855463838..c2a541af6b2c 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -31,8 +31,9 @@
  * @field:	field order of the image in the buffer, as defined by
  *		&enum v4l2_field.
  * @timecode:	frame timecode.
+ * @tag:	user specified buffer tag value.
  * @sequence:	sequence count of this frame.
- * @request_fd:	the request_fd associated with this buffer
+ * @request_fd:	the request_fd associated with this buffer.
  * @planes:	plane information (userptr/fd, length, bytesused, data_offset).
  *
  * Should contain enough information to be able to cover all the fields
@@ -44,6 +45,7 @@ struct vb2_v4l2_buffer {
 	__u32			flags;
 	__u32			field;
 	struct v4l2_timecode	timecode;
+	__u32			tag;
 	__u32			sequence;
 	__s32			request_fd;
 	struct vb2_plane	planes[VB2_MAX_PLANES];
@@ -55,6 +57,23 @@ struct vb2_v4l2_buffer {
 #define to_vb2_v4l2_buffer(vb) \
 	container_of(vb, struct vb2_v4l2_buffer, vb2_buf)
 
+/**
+ * vb2_find_tag() - Find buffer with given tag in the queue
+ *
+ * @q:		pointer to &struct vb2_queue with videobuf2 queue.
+ * @tag:	the tag to find. Only buffers in state DEQUEUED or DONE
+ *		are considered.
+ * @start_idx:	the start index (usually 0) in the buffer array to start
+ *		searching from. Note that there may be multiple buffers
+ *		with the same tag value, so you can restart the search
+ *		by setting @start_idx to the previously found index + 1.
+ *
+ * Returns the buffer index of the buffer with the given @tag, or
+ * -1 if no buffer with @tag was found.
+ */
+int vb2_find_tag(const struct vb2_queue *q, u32 tag,
+		 unsigned int start_idx);
+
 int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
 
 /**
-- 
2.19.1

[PATCHv2 3/9] v4l2-ioctl.c: log v4l2_buffer tag

[Hans Verkuil]

When debugging is on, log the new tag field of struct v4l2_buffer
as well.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index c63746968fa3..49103787d19a 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -498,9 +498,12 @@ static void v4l_print_buffer(const void *arg, bool write_only)
 			p->bytesused, p->m.userptr, p->length);
 	}
 
-	printk(KERN_DEBUG "timecode=%02d:%02d:%02d type=%d, flags=0x%08x, frames=%d, userbits=0x%08x\n",
-			tc->hours, tc->minutes, tc->seconds,
-			tc->type, tc->flags, tc->frames, *(__u32 *)tc->userbits);
+	if (p->flags & V4L2_BUF_FLAG_TAG)
+		printk(KERN_DEBUG "tag=%x\n", p->tag);
+	else if (p->flags & V4L2_BUF_FLAG_TIMECODE)
+		printk(KERN_DEBUG "timecode=%02d:%02d:%02d type=%d, flags=0x%08x, frames=%d, userbits=0x%08x\n",
+		       tc->hours, tc->minutes, tc->seconds,
+		       tc->type, tc->flags, tc->frames, *(__u32 *)tc->userbits);
 }
 
 static void v4l_print_exportbuffer(const void *arg, bool write_only)
-- 
2.19.1

[PATCHv2 4/9] buffer.rst: document the new buffer tag feature.

[Hans Verkuil]

Document V4L2_BUF_FLAG_TAG and struct v4l2_buffer_tag.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 Documentation/media/uapi/v4l/buffer.rst       | 32 ++++++++++++++-----
 .../media/uapi/v4l/vidioc-reqbufs.rst         |  4 +++
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index 2e266d32470a..0d4aa83d2bce 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -220,17 +220,25 @@ struct v4l2_buffer
 	use ``V4L2_BUF_FLAG_TIMESTAMP_COPY`` the application has to fill
 	in the timestamp which will be copied by the driver to the capture
 	stream.
-    * - struct :c:type:`v4l2_timecode`
+    * - union
+    * -
+      - struct :c:type:`v4l2_timecode`
       - ``timecode``
-      -
-      - When ``type`` is ``V4L2_BUF_TYPE_VIDEO_CAPTURE`` and the
-	``V4L2_BUF_FLAG_TIMECODE`` flag is set in ``flags``, this
+      - When the ``V4L2_BUF_FLAG_TIMECODE`` flag is set in ``flags``, this
 	structure contains a frame timecode. In
 	:c:type:`V4L2_FIELD_ALTERNATE <v4l2_field>` mode the top and
 	bottom field contain the same timecode. Timecodes are intended to
 	help video editing and are typically recorded on video tapes, but
 	also embedded in compressed formats like MPEG. This field is
 	independent of the ``timestamp`` and ``sequence`` fields.
+    * -
+      - __u32
+      - ``tag``
+      - When the ``V4L2_BUF_FLAG_TAG`` flag is set in ``flags``, this
+	structure contains a user-specified tag value.
+
+	It is used by stateless codecs where this tag can be used to
+	refer to buffers that contain reference frames.
     * - __u32
       - ``sequence``
       -
@@ -567,6 +575,14 @@ Buffer Flags
 	when the ``VIDIOC_DQBUF`` ioctl is called. Applications can set
 	this bit and the corresponding ``timecode`` structure when
 	``type`` refers to an output stream.
+    * .. _`V4L2-BUF-FLAG-TAG`:
+
+      - ``V4L2_BUF_FLAG_TAG``
+      - 0x00000200
+      - The ``tag`` field is valid. Applications can set
+	this bit and the corresponding ``tag`` structure. If tags are
+	supported then the ``V4L2_BUF_CAP_SUPPORTS_TAGS`` capability
+	is also set.
     * .. _`V4L2-BUF-FLAG-PREPARED`:
 
       - ``V4L2_BUF_FLAG_PREPARED``
@@ -704,10 +720,10 @@ enum v4l2_memory
 Timecodes
 =========
 
-The struct :c:type:`v4l2_timecode` structure is designed to hold a
-:ref:`smpte12m` or similar timecode. (struct
-struct :c:type:`timeval` timestamps are stored in struct
-:c:type:`v4l2_buffer` field ``timestamp``.)
+The :c:type:`v4l2_buffer_timecode` structure is designed to hold a
+:ref:`smpte12m` or similar timecode.
+(struct :c:type:`timeval` timestamps are stored in the struct
+:c:type:`v4l2_buffer` ``timestamp`` field.)
 
 
 .. c:type:: v4l2_timecode
diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
index d4bbbb0c60e8..5090a62f324c 100644
--- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
+++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
@@ -112,6 +112,7 @@ any DMA in progress, an implicit
 .. _V4L2-BUF-CAP-SUPPORTS-USERPTR:
 .. _V4L2-BUF-CAP-SUPPORTS-DMABUF:
 .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS:
+.. _V4L2-BUF-CAP-SUPPORTS-TAGS:
 
 .. cssclass:: longtable
 
@@ -132,6 +133,9 @@ any DMA in progress, an implicit
     * - ``V4L2_BUF_CAP_SUPPORTS_REQUESTS``
       - 0x00000008
       - This buffer type supports :ref:`requests <media-request-api>`.
+    * - ``V4L2_BUF_CAP_SUPPORTS_TAGS``
+      - 0x00000010
+      - This buffer type supports ``V4L2_BUF_FLAG_TAG``.
 
 Return Value
 ============
-- 
2.19.1

[PATCHv2 5/9] v4l2-mem2mem: add v4l2_m2m_buf_copy_data helper function

[Hans Verkuil]

Memory-to-memory devices should copy various parts of
struct v4l2_buffer from the output buffer to the capture buffer.

Add a helper function that does that to simplify the driver code.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 23 +++++++++++++++++++++++
 include/media/v4l2-mem2mem.h           | 21 +++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 1ed2465972ac..bfd5321fda1c 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -953,6 +953,29 @@ void v4l2_m2m_buf_queue(struct v4l2_m2m_ctx *m2m_ctx,
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_buf_queue);
 
+void v4l2_m2m_buf_copy_data(const struct vb2_v4l2_buffer *out_vb,
+			    struct vb2_v4l2_buffer *cap_vb,
+			    bool copy_frame_flags)
+{
+	u32 mask = V4L2_BUF_FLAG_TIMECODE | V4L2_BUF_FLAG_TAG |
+		   V4L2_BUF_FLAG_TSTAMP_SRC_MASK;
+
+	if (copy_frame_flags)
+		mask |= V4L2_BUF_FLAG_KEYFRAME | V4L2_BUF_FLAG_PFRAME |
+			V4L2_BUF_FLAG_BFRAME;
+
+	cap_vb->vb2_buf.timestamp = out_vb->vb2_buf.timestamp;
+
+	if (out_vb->flags & V4L2_BUF_FLAG_TAG)
+		cap_vb->tag = out_vb->tag;
+	if (out_vb->flags & V4L2_BUF_FLAG_TIMECODE)
+		cap_vb->timecode = out_vb->timecode;
+	cap_vb->field = out_vb->field;
+	cap_vb->flags &= ~mask;
+	cap_vb->flags |= out_vb->flags & mask;
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_buf_copy_data);
+
 void v4l2_m2m_request_queue(struct media_request *req)
 {
 	struct media_request_object *obj, *obj_safe;
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 5467264771ec..bb4feb6969d2 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -622,6 +622,27 @@ v4l2_m2m_dst_buf_remove_by_idx(struct v4l2_m2m_ctx *m2m_ctx, unsigned int idx)
 	return v4l2_m2m_buf_remove_by_idx(&m2m_ctx->cap_q_ctx, idx);
 }
 
+/**
+ * v4l2_m2m_buf_copy_data() - copy buffer data from the output buffer to the
+ * capture buffer
+ *
+ * @out_vb: the output buffer that is the source of the data.
+ * @cap_vb: the capture buffer that will receive the data.
+ * @copy_frame_flags: copy the KEY/B/PFRAME flags as well.
+ *
+ * This helper function copies the timestamp, timecode (if the TIMECODE
+ * buffer flag was set), tag (if the TAG buffer flag was set), field
+ * and the TIMECODE, TAG, KEYFRAME, BFRAME, PFRAME and TSTAMP_SRC_MASK
+ * flags from @out_vb to @cap_vb.
+ *
+ * If @copy_frame_flags is false, then the KEYFRAME, BFRAME and PFRAME
+ * flags are not copied. This is typically needed for encoders that
+ * set this bits explicitly.
+ */
+void v4l2_m2m_buf_copy_data(const struct vb2_v4l2_buffer *out_vb,
+			    struct vb2_v4l2_buffer *cap_vb,
+			    bool copy_frame_flags);
+
 /* v4l2 request helper */
 
 void v4l2_m2m_request_queue(struct media_request *req);
-- 
2.19.1

[PATCHv2 6/9] vb2: add new supports_tags queue flag

[Hans Verkuil]

Add new flag to indicate that buffer tags are supported.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 2 ++
 include/media/videobuf2-core.h                  | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 115f2868223a..d2d4985fb352 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -666,6 +666,8 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
 		*caps |= V4L2_BUF_CAP_SUPPORTS_DMABUF;
 	if (q->supports_requests)
 		*caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
+	if (q->supports_tags)
+		*caps |= V4L2_BUF_CAP_SUPPORTS_TAGS;
 }
 
 int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index e86981d615ae..81f2dbfd0094 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -473,6 +473,7 @@ struct vb2_buf_ops {
  *              has not been called. This is a vb1 idiom that has been adopted
  *              also by vb2.
  * @supports_requests: this queue supports the Request API.
+ * @supports_tags: this queue supports tags in struct v4l2_buffer.
  * @uses_qbuf:	qbuf was used directly for this queue. Set to 1 the first
  *		time this is called. Set to 0 when the queue is canceled.
  *		If this is 1, then you cannot queue buffers from a request.
@@ -547,6 +548,7 @@ struct vb2_queue {
 	unsigned			allow_zero_bytesused:1;
 	unsigned		   quirk_poll_must_check_waiting_for_buffers:1;
 	unsigned			supports_requests:1;
+	unsigned			supports_tags:1;
 	unsigned			uses_qbuf:1;
 	unsigned			uses_requests:1;
 
-- 
2.19.1

[PATCHv2 7/9] vim2m: add tag support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Copy tags in vim2m.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/vim2m.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index d82db738f174..9d1222f489b8 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -241,17 +241,7 @@ static int device_process(struct vim2m_ctx *ctx,
 	out_vb->sequence =
 		get_q_data(ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE)->sequence++;
 	in_vb->sequence = q_data->sequence++;
-	out_vb->vb2_buf.timestamp = in_vb->vb2_buf.timestamp;
-
-	if (in_vb->flags & V4L2_BUF_FLAG_TIMECODE)
-		out_vb->timecode = in_vb->timecode;
-	out_vb->field = in_vb->field;
-	out_vb->flags = in_vb->flags &
-		(V4L2_BUF_FLAG_TIMECODE |
-		 V4L2_BUF_FLAG_KEYFRAME |
-		 V4L2_BUF_FLAG_PFRAME |
-		 V4L2_BUF_FLAG_BFRAME |
-		 V4L2_BUF_FLAG_TSTAMP_SRC_MASK);
+	v4l2_m2m_buf_copy_data(out_vb, in_vb, true);
 
 	switch (ctx->mode) {
 	case MEM2MEM_HFLIP | MEM2MEM_VFLIP:
@@ -856,6 +846,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *ds
 	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	src_vq->lock = &ctx->dev->dev_mutex;
 	src_vq->supports_requests = true;
+	src_vq->supports_tags = true;
 
 	ret = vb2_queue_init(src_vq);
 	if (ret)
@@ -869,6 +860,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *ds
 	dst_vq->mem_ops = &vb2_vmalloc_memops;
 	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	dst_vq->lock = &ctx->dev->dev_mutex;
+	dst_vq->supports_tags = true;
 
 	return vb2_queue_init(dst_vq);
 }
-- 
2.19.1

[PATCHv2 8/9] vicodec: add tag support

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Copy tags in vicodec.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/vicodec/vicodec-core.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index b292cff26c86..72245183b077 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -190,18 +190,8 @@ static int device_process(struct vicodec_ctx *ctx,
 	}
 
 	out_vb->sequence = q_cap->sequence++;
-	out_vb->vb2_buf.timestamp = in_vb->vb2_buf.timestamp;
-
-	if (in_vb->flags & V4L2_BUF_FLAG_TIMECODE)
-		out_vb->timecode = in_vb->timecode;
-	out_vb->field = in_vb->field;
 	out_vb->flags &= ~V4L2_BUF_FLAG_LAST;
-	out_vb->flags |= in_vb->flags &
-		(V4L2_BUF_FLAG_TIMECODE |
-		 V4L2_BUF_FLAG_KEYFRAME |
-		 V4L2_BUF_FLAG_PFRAME |
-		 V4L2_BUF_FLAG_BFRAME |
-		 V4L2_BUF_FLAG_TSTAMP_SRC_MASK);
+	v4l2_m2m_buf_copy_data(in_vb, out_vb, !ctx->is_enc);
 
 	return 0;
 }
@@ -1066,6 +1056,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	src_vq->lock = ctx->is_enc ? &ctx->dev->enc_mutex :
 		&ctx->dev->dec_mutex;
+	src_vq->supports_tags = true;
 
 	ret = vb2_queue_init(src_vq);
 	if (ret)
@@ -1081,6 +1072,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_tags = true;
 
 	return vb2_queue_init(dst_vq);
 }
-- 
2.19.1

[PATCHv2 9/9] cedrus: add tag support

[Hans Verkuil]

Replace old reference frame indices by new tag method.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/v4l2-core/v4l2-ctrls.c          |  9 --------
 drivers/staging/media/sunxi/cedrus/cedrus.h   |  9 +++++---
 .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 ++
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++++++-----------
 .../staging/media/sunxi/cedrus/cedrus_video.c |  2 ++
 include/uapi/linux/v4l2-controls.h            | 14 +++++--------
 6 files changed, 24 insertions(+), 33 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 5f2b033a7a42..b854cceb19dc 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1660,15 +1660,6 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
 			return -EINVAL;
 		}
 
-		if (p_mpeg2_slice_params->backward_ref_index >= VIDEO_MAX_FRAME ||
-		    p_mpeg2_slice_params->forward_ref_index >= VIDEO_MAX_FRAME)
-			return -EINVAL;
-
-		if (p_mpeg2_slice_params->pad ||
-		    p_mpeg2_slice_params->picture.pad ||
-		    p_mpeg2_slice_params->sequence.pad)
-			return -EINVAL;
-
 		return 0;
 
 	case V4L2_CTRL_TYPE_MPEG2_QUANTIZATION:
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.h b/drivers/staging/media/sunxi/cedrus/cedrus.h
index 3f61248c57ac..781676b55a1b 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -142,11 +142,14 @@ static inline dma_addr_t cedrus_buf_addr(struct vb2_buffer *buf,
 }
 
 static inline dma_addr_t cedrus_dst_buf_addr(struct cedrus_ctx *ctx,
-					     unsigned int index,
-					     unsigned int plane)
+					     int index, unsigned int plane)
 {
-	struct vb2_buffer *buf = ctx->dst_bufs[index];
+	struct vb2_buffer *buf;
 
+	if (index < 0)
+		return 0;
+
+	buf = ctx->dst_bufs[index];
 	return buf ? cedrus_buf_addr(buf, &ctx->dst_fmt, plane) : 0;
 }
 
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
index e40180a33951..0cfd6036d0cd 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -53,6 +53,8 @@ void cedrus_device_run(void *priv)
 		break;
 	}
 
+	v4l2_m2m_buf_copy_data(run.src, run.dst, true);
+
 	dev->dec_ops[ctx->current_codec]->setup(ctx, &run);
 
 	spin_unlock_irqrestore(&ctx->dev->irq_lock, flags);
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
index 9abd39cae38c..fdde9a099153 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
@@ -82,7 +82,10 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 	dma_addr_t fwd_luma_addr, fwd_chroma_addr;
 	dma_addr_t bwd_luma_addr, bwd_chroma_addr;
 	struct cedrus_dev *dev = ctx->dev;
+	struct vb2_queue *cap_q = &ctx->fh.m2m_ctx->cap_q_ctx.q;
 	const u8 *matrix;
+	int forward_idx;
+	int backward_idx;
 	unsigned int i;
 	u32 reg;
 
@@ -156,23 +159,17 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 	cedrus_write(dev, VE_DEC_MPEG_PICBOUNDSIZE, reg);
 
 	/* Forward and backward prediction reference buffers. */
+	forward_idx = vb2_find_tag(cap_q, slice_params->forward_ref_tag, 0);
 
-	fwd_luma_addr = cedrus_dst_buf_addr(ctx,
-					    slice_params->forward_ref_index,
-					    0);
-	fwd_chroma_addr = cedrus_dst_buf_addr(ctx,
-					      slice_params->forward_ref_index,
-					      1);
+	fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
+	fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
 
 	cedrus_write(dev, VE_DEC_MPEG_FWD_REF_LUMA_ADDR, fwd_luma_addr);
 	cedrus_write(dev, VE_DEC_MPEG_FWD_REF_CHROMA_ADDR, fwd_chroma_addr);
 
-	bwd_luma_addr = cedrus_dst_buf_addr(ctx,
-					    slice_params->backward_ref_index,
-					    0);
-	bwd_chroma_addr = cedrus_dst_buf_addr(ctx,
-					      slice_params->backward_ref_index,
-					      1);
+	backward_idx = vb2_find_tag(cap_q, slice_params->backward_ref_tag, 0);
+	bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
+	bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
 
 	cedrus_write(dev, VE_DEC_MPEG_BWD_REF_LUMA_ADDR, bwd_luma_addr);
 	cedrus_write(dev, VE_DEC_MPEG_BWD_REF_CHROMA_ADDR, bwd_chroma_addr);
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_video.c b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
index 5c5fce678b93..293df48326cc 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_video.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_video.c
@@ -522,6 +522,7 @@ int cedrus_queue_init(void *priv, struct vb2_queue *src_vq,
 	src_vq->lock = &ctx->dev->dev_mutex;
 	src_vq->dev = ctx->dev->dev;
 	src_vq->supports_requests = true;
+	src_vq->supports_tags = true;
 
 	ret = vb2_queue_init(src_vq);
 	if (ret)
@@ -537,6 +538,7 @@ int cedrus_queue_init(void *priv, struct vb2_queue *src_vq,
 	dst_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	dst_vq->lock = &ctx->dev->dev_mutex;
 	dst_vq->dev = ctx->dev->dev;
+	dst_vq->supports_tags = true;
 
 	return vb2_queue_init(dst_vq);
 }
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 998983a6e6b7..45a55bb27e5a 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1109,10 +1109,9 @@ struct v4l2_mpeg2_sequence {
 	__u32	vbv_buffer_size;
 
 	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Sequence extension */
-	__u8	profile_and_level_indication;
+	__u16	profile_and_level_indication;
 	__u8	progressive_sequence;
 	__u8	chroma_format;
-	__u8	pad;
 };
 
 struct v4l2_mpeg2_picture {
@@ -1130,23 +1129,20 @@ struct v4l2_mpeg2_picture {
 	__u8	intra_vlc_format;
 	__u8	alternate_scan;
 	__u8	repeat_first_field;
-	__u8	progressive_frame;
-	__u8	pad;
+	__u16	progressive_frame;
 };
 
 struct v4l2_ctrl_mpeg2_slice_params {
 	__u32	bit_size;
 	__u32	data_bit_offset;
+	__u32	backward_ref_tag;
+	__u32	forward_ref_tag;
 
 	struct v4l2_mpeg2_sequence sequence;
 	struct v4l2_mpeg2_picture picture;
 
 	/* ISO/IEC 13818-2, ITU-T Rec. H.262: Slice */
-	__u8	quantiser_scale_code;
-
-	__u8	backward_ref_index;
-	__u8	forward_ref_index;
-	__u8	pad;
+	__u32	quantiser_scale_code;
 };
 
 struct v4l2_ctrl_mpeg2_quantization {
-- 
2.19.1

Re: [PATCHv2 0/9] vb2/cedrus: add tag support

[Tomasz Figa]

Hi Hans,

On Wed, Nov 14, 2018 at 10:47 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> From: Hans Verkuil <hansverk@cisco.com>
>
> As was discussed here (among other places):
>
> https://lkml.org/lkml/2018/10/19/440
>
> using capture queue buffer indices to refer to reference frames is
> not a good idea. A better idea is to use a 'tag' where the
> application can assign a u64 tag to an output buffer, which is then
> copied to the capture buffer(s) derived from the output buffer.
>

Thanks for the patches. Please see my comments below.

> A u64 is chosen since this allows userspace to also use pointers to
> internal structures as 'tag'.
>

I think this is not true anymore in this version.

> The first three patches add core tag support, the next patch document
> the tag support, then a new helper function is added to v4l2-mem2mem.c
> to easily copy data from a source to a destination buffer that drivers
> can use.
>
> Next a new supports_tags vb2_queue flag is added to indicate that
> the driver supports tags. Ideally this should not be necessary, but
> that would require that all m2m drivers are converted to using the
> new helper function introduced in the previous patch. That takes more
> time then I have now since we want to get this in for 4.20.
>
> Finally the vim2m, vicodec and cedrus drivers are converted to support
> tags.
>
> I also removed the 'pad' fields from the mpeg2 control structs (it
> should never been added in the first place) and aligned the structs
> to a u32 boundary (u64 for the tag values).

u32 in this version

>
> Note that this might change further (Paul suggested using bitfields).
>
> Also note that the cedrus code doesn't set the sequence counter, that's
> something that should still be added before this driver can be moved
> out of staging.
>
> Note: if no buffer is found for a certain tag, then the dma address
> is just set to 0. That happened before as well with invalid buffer
> indices. This should be checked in the driver!

Note that DMA address 0 may as well be a valid address. Should we have
another way of signaling that?

>
> The previous RFC series was tested successfully with the cedrus driver.
>
> Regards,
>
>         Hans
>
> Changes since v1:
>
> - changed to a u32 tag. Using a 64 bit tag was overly complicated due
>   to the bad layout of the v4l2_buffer struct, and there is no real
>   need for it by applications.

I hope these won't become famous last words. :) For Chromium it should
be okay indeed.

Since we've been thinking about a redesign of the buffer struct,
perhaps we can live with u32 for now and we can design the new struct
to handle u64 nicely?

Best regards,
Tomasz

Re: [PATCHv2 0/9] vb2/cedrus: add tag support

[Hans Verkuil]

On 11/16/2018 10:00 AM, Tomasz Figa wrote:
> Hi Hans,
> 
> On Wed, Nov 14, 2018 at 10:47 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> From: Hans Verkuil <hansverk@cisco.com>
>>
>> As was discussed here (among other places):
>>
>> https://lkml.org/lkml/2018/10/19/440
>>
>> using capture queue buffer indices to refer to reference frames is
>> not a good idea. A better idea is to use a 'tag' where the
>> application can assign a u64 tag to an output buffer, which is then
>> copied to the capture buffer(s) derived from the output buffer.
>>
> 
> Thanks for the patches. Please see my comments below.
> 
>> A u64 is chosen since this allows userspace to also use pointers to
>> internal structures as 'tag'.
>>
> 
> I think this is not true anymore in this version.

Ah, forgot to update the commit message.

> 
>> The first three patches add core tag support, the next patch document
>> the tag support, then a new helper function is added to v4l2-mem2mem.c
>> to easily copy data from a source to a destination buffer that drivers
>> can use.
>>
>> Next a new supports_tags vb2_queue flag is added to indicate that
>> the driver supports tags. Ideally this should not be necessary, but
>> that would require that all m2m drivers are converted to using the
>> new helper function introduced in the previous patch. That takes more
>> time then I have now since we want to get this in for 4.20.
>>
>> Finally the vim2m, vicodec and cedrus drivers are converted to support
>> tags.
>>
>> I also removed the 'pad' fields from the mpeg2 control structs (it
>> should never been added in the first place) and aligned the structs
>> to a u32 boundary (u64 for the tag values).
> 
> u32 in this version
> 
>>
>> Note that this might change further (Paul suggested using bitfields).
>>
>> Also note that the cedrus code doesn't set the sequence counter, that's
>> something that should still be added before this driver can be moved
>> out of staging.
>>
>> Note: if no buffer is found for a certain tag, then the dma address
>> is just set to 0. That happened before as well with invalid buffer
>> indices. This should be checked in the driver!
> 
> Note that DMA address 0 may as well be a valid address. Should we have
> another way of signaling that?

See this patch:

https://patchwork.linuxtv.org/patch/52975/

The memory of the reference buffer should be checked and the refcount
incremented in the request validate function.

Once that's done the dma address will be guaranteed to be OK.

This is missing functionality that is important to add.

> 
>>
>> The previous RFC series was tested successfully with the cedrus driver.
>>
>> Regards,
>>
>>         Hans
>>
>> Changes since v1:
>>
>> - changed to a u32 tag. Using a 64 bit tag was overly complicated due
>>   to the bad layout of the v4l2_buffer struct, and there is no real
>>   need for it by applications.
> 
> I hope these won't become famous last words. :) For Chromium it should
> be okay indeed.

The only 'feature' that is now missing is the ability to store pointers
as the tag. So worst case you need to do an extra lookup in the application.

> 
> Since we've been thinking about a redesign of the buffer struct,
> perhaps we can live with u32 for now and we can design the new struct
> to handle u64 nicely?

It could be done, but I'm not sure if it is a good idea to have
different behavior between v4l2_buffer and v4l2_ext_buffer.

Something to discuss at that time.

Regards,

	Hans

> 
> Best regards,
> Tomasz
> 

Re: [PATCHv2 0/9] vb2/cedrus: add tag support

[Hans Verkuil]

On 11/14/2018 02:47 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hansverk@cisco.com>
> 
> As was discussed here (among other places):
> 
> https://lkml.org/lkml/2018/10/19/440
> 
> using capture queue buffer indices to refer to reference frames is
> not a good idea. A better idea is to use a 'tag' where the
> application can assign a u64 tag to an output buffer, which is then 
> copied to the capture buffer(s) derived from the output buffer.
> 
> A u64 is chosen since this allows userspace to also use pointers to
> internal structures as 'tag'.
> 
> The first three patches add core tag support, the next patch document
> the tag support, then a new helper function is added to v4l2-mem2mem.c
> to easily copy data from a source to a destination buffer that drivers
> can use.
> 
> Next a new supports_tags vb2_queue flag is added to indicate that
> the driver supports tags. Ideally this should not be necessary, but
> that would require that all m2m drivers are converted to using the
> new helper function introduced in the previous patch. That takes more
> time then I have now since we want to get this in for 4.20.
> 
> Finally the vim2m, vicodec and cedrus drivers are converted to support
> tags.
> 
> I also removed the 'pad' fields from the mpeg2 control structs (it
> should never been added in the first place) and aligned the structs
> to a u32 boundary (u64 for the tag values).
> 
> Note that this might change further (Paul suggested using bitfields).
> 
> Also note that the cedrus code doesn't set the sequence counter, that's
> something that should still be added before this driver can be moved
> out of staging.
> 
> Note: if no buffer is found for a certain tag, then the dma address
> is just set to 0. That happened before as well with invalid buffer
> indices. This should be checked in the driver!
> 
> The previous RFC series was tested successfully with the cedrus driver.
> 
> Regards,
> 
>         Hans

I'd like to get some Acked-by or Reviewed-by replies for this series.
Or comments if you don't like something.

I would really like to get this in for 4.20 so the cedrus API is stable
(hopefully), since this is a last outstanding API item.

Tomasz: you commented that the text still referred to the tag as a u64,
but that was only in the cover letter, not the patches themselves. So
I don't plan to post a v3 just for that.

Regards,

	Hans

> 
> Changes since v1:
> 
> - changed to a u32 tag. Using a 64 bit tag was overly complicated due
>   to the bad layout of the v4l2_buffer struct, and there is no real
>   need for it by applications.
> 
> Main changes since the RFC:
> 
> - Added new buffer capability flag
> - Added m2m helper to copy data between buffers
> - Added documentation
> - Added tag logging in v4l2-ioctl.c
> 
> Hans Verkuil (9):
>   videodev2.h: add tag support
>   vb2: add tag support
>   v4l2-ioctl.c: log v4l2_buffer tag
>   buffer.rst: document the new buffer tag feature.
>   v4l2-mem2mem: add v4l2_m2m_buf_copy_data helper function
>   vb2: add new supports_tags queue flag
>   vim2m: add tag support
>   vicodec: add tag support
>   cedrus: add tag support
> 
>  Documentation/media/uapi/v4l/buffer.rst       | 32 +++++++++----
>  .../media/uapi/v4l/vidioc-reqbufs.rst         |  4 ++
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 45 ++++++++++++++++---
>  drivers/media/platform/vicodec/vicodec-core.c | 14 ++----
>  drivers/media/platform/vim2m.c                | 14 ++----
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ----
>  drivers/media/v4l2-core/v4l2-ioctl.c          |  9 ++--
>  drivers/media/v4l2-core/v4l2-mem2mem.c        | 23 ++++++++++
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  9 ++--
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++-----
>  .../staging/media/sunxi/cedrus/cedrus_video.c |  2 +
>  include/media/v4l2-mem2mem.h                  | 21 +++++++++
>  include/media/videobuf2-core.h                |  2 +
>  include/media/videobuf2-v4l2.h                | 21 ++++++++-
>  include/uapi/linux/v4l2-controls.h            | 14 +++---
>  include/uapi/linux/videodev2.h                |  9 +++-
>  17 files changed, 178 insertions(+), 73 deletions(-)
> 

Re: [PATCHv2 0/9] vb2/cedrus: add tag support

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 6149 bytes --]

Hi,

On Mon, 2018-11-19 at 12:18 +0100, Hans Verkuil wrote:
> On 11/14/2018 02:47 PM, Hans Verkuil wrote:
> > From: Hans Verkuil <hansverk@cisco.com>
> > 
> > As was discussed here (among other places):
> > 
> > https://lkml.org/lkml/2018/10/19/440
> > 
> > using capture queue buffer indices to refer to reference frames is
> > not a good idea. A better idea is to use a 'tag' where the
> > application can assign a u64 tag to an output buffer, which is then 
> > copied to the capture buffer(s) derived from the output buffer.
> > 
> > A u64 is chosen since this allows userspace to also use pointers to
> > internal structures as 'tag'.
> > 
> > The first three patches add core tag support, the next patch document
> > the tag support, then a new helper function is added to v4l2-mem2mem.c
> > to easily copy data from a source to a destination buffer that drivers
> > can use.
> > 
> > Next a new supports_tags vb2_queue flag is added to indicate that
> > the driver supports tags. Ideally this should not be necessary, but
> > that would require that all m2m drivers are converted to using the
> > new helper function introduced in the previous patch. That takes more
> > time then I have now since we want to get this in for 4.20.
> > 
> > Finally the vim2m, vicodec and cedrus drivers are converted to support
> > tags.
> > 
> > I also removed the 'pad' fields from the mpeg2 control structs (it
> > should never been added in the first place) and aligned the structs
> > to a u32 boundary (u64 for the tag values).
> > 
> > Note that this might change further (Paul suggested using bitfields).
> > 
> > Also note that the cedrus code doesn't set the sequence counter, that's
> > something that should still be added before this driver can be moved
> > out of staging.
> > 
> > Note: if no buffer is found for a certain tag, then the dma address
> > is just set to 0. That happened before as well with invalid buffer
> > indices. This should be checked in the driver!
> > 
> > The previous RFC series was tested successfully with the cedrus driver.
> > 
> > Regards,
> > 
> >         Hans
> 
> I'd like to get some Acked-by or Reviewed-by replies for this series.
> Or comments if you don't like something.

The series looks good to me, so consider it:
Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Also, I'm glad you made the v4l2_m2m_buf_copy_data helper function,
since all drivers will need to do these same operations anyway.

> I would really like to get this in for 4.20 so the cedrus API is stable
> (hopefully), since this is a last outstanding API item.

I see a few more items that we need to tackle before we can consider the
MPEG-2 stateless API as stable.

* I mentionned using flags in the mpeg2 structures to solve the
alignment issues, but I failed to provide a patch implementing it at
this point. This would make the MPEG-2 controls more similar to the
proposed H.264 ones and generally makes more sense than using a u8 for a
binary value.

* Some time ago, we also discussed adding extra fields to the structures
for later use. Do you think that is still relevant? Adding a flags
elements (with unused bits) would certainly help in this direction
anyway.

* During the discussions on the spec, the concensus was also to use one
slice_params per-slice (instead of per-picture) which requires splitting
the per-picture and the per-slice parameters. On this as well, I fell
behind and didn't send a proposal yet.

So my question is: does it need to be done for 4.20 or can we affoard
going through another version cycle for these changes?

It was my impression that we wanted to wait for another driver to use
the API to declare it stable (and move the driver out of staging).

Cheers,

Paul

> Tomasz: you commented that the text still referred to the tag as a u64,
> but that was only in the cover letter, not the patches themselves. So
> I don't plan to post a v3 just for that.
> 
> Regards,
> 
> 	Hans
> 
> > Changes since v1:
> > 
> > - changed to a u32 tag. Using a 64 bit tag was overly complicated due
> >   to the bad layout of the v4l2_buffer struct, and there is no real
> >   need for it by applications.
> > 
> > Main changes since the RFC:
> > 
> > - Added new buffer capability flag
> > - Added m2m helper to copy data between buffers
> > - Added documentation
> > - Added tag logging in v4l2-ioctl.c
> > 
> > Hans Verkuil (9):
> >   videodev2.h: add tag support
> >   vb2: add tag support
> >   v4l2-ioctl.c: log v4l2_buffer tag
> >   buffer.rst: document the new buffer tag feature.
> >   v4l2-mem2mem: add v4l2_m2m_buf_copy_data helper function
> >   vb2: add new supports_tags queue flag
> >   vim2m: add tag support
> >   vicodec: add tag support
> >   cedrus: add tag support
> > 
> >  Documentation/media/uapi/v4l/buffer.rst       | 32 +++++++++----
> >  .../media/uapi/v4l/vidioc-reqbufs.rst         |  4 ++
> >  .../media/common/videobuf2/videobuf2-v4l2.c   | 45 ++++++++++++++++---
> >  drivers/media/platform/vicodec/vicodec-core.c | 14 ++----
> >  drivers/media/platform/vim2m.c                | 14 ++----
> >  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ----
> >  drivers/media/v4l2-core/v4l2-ioctl.c          |  9 ++--
> >  drivers/media/v4l2-core/v4l2-mem2mem.c        | 23 ++++++++++
> >  drivers/staging/media/sunxi/cedrus/cedrus.h   |  9 ++--
> >  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
> >  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++-----
> >  .../staging/media/sunxi/cedrus/cedrus_video.c |  2 +
> >  include/media/v4l2-mem2mem.h                  | 21 +++++++++
> >  include/media/videobuf2-core.h                |  2 +
> >  include/media/videobuf2-v4l2.h                | 21 ++++++++-
> >  include/uapi/linux/v4l2-controls.h            | 14 +++---
> >  include/uapi/linux/videodev2.h                |  9 +++-
> >  17 files changed, 178 insertions(+), 73 deletions(-)
> > 
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCHv2 0/9] vb2/cedrus: add tag support

[Hans Verkuil]

On 11/19/2018 02:53 PM, Paul Kocialkowski wrote:
> Hi,
> 
> On Mon, 2018-11-19 at 12:18 +0100, Hans Verkuil wrote:
>> On 11/14/2018 02:47 PM, Hans Verkuil wrote:
>>> From: Hans Verkuil <hansverk@cisco.com>
>>>
>>> As was discussed here (among other places):
>>>
>>> https://lkml.org/lkml/2018/10/19/440
>>>
>>> using capture queue buffer indices to refer to reference frames is
>>> not a good idea. A better idea is to use a 'tag' where the
>>> application can assign a u64 tag to an output buffer, which is then 
>>> copied to the capture buffer(s) derived from the output buffer.
>>>
>>> A u64 is chosen since this allows userspace to also use pointers to
>>> internal structures as 'tag'.
>>>
>>> The first three patches add core tag support, the next patch document
>>> the tag support, then a new helper function is added to v4l2-mem2mem.c
>>> to easily copy data from a source to a destination buffer that drivers
>>> can use.
>>>
>>> Next a new supports_tags vb2_queue flag is added to indicate that
>>> the driver supports tags. Ideally this should not be necessary, but
>>> that would require that all m2m drivers are converted to using the
>>> new helper function introduced in the previous patch. That takes more
>>> time then I have now since we want to get this in for 4.20.
>>>
>>> Finally the vim2m, vicodec and cedrus drivers are converted to support
>>> tags.
>>>
>>> I also removed the 'pad' fields from the mpeg2 control structs (it
>>> should never been added in the first place) and aligned the structs
>>> to a u32 boundary (u64 for the tag values).
>>>
>>> Note that this might change further (Paul suggested using bitfields).
>>>
>>> Also note that the cedrus code doesn't set the sequence counter, that's
>>> something that should still be added before this driver can be moved
>>> out of staging.
>>>
>>> Note: if no buffer is found for a certain tag, then the dma address
>>> is just set to 0. That happened before as well with invalid buffer
>>> indices. This should be checked in the driver!
>>>
>>> The previous RFC series was tested successfully with the cedrus driver.
>>>
>>> Regards,
>>>
>>>         Hans
>>
>> I'd like to get some Acked-by or Reviewed-by replies for this series.
>> Or comments if you don't like something.
> 
> The series looks good to me, so consider it:
> Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> 
> Also, I'm glad you made the v4l2_m2m_buf_copy_data helper function,
> since all drivers will need to do these same operations anyway.
> 
>> I would really like to get this in for 4.20 so the cedrus API is stable
>> (hopefully), since this is a last outstanding API item.
> 
> I see a few more items that we need to tackle before we can consider the
> MPEG-2 stateless API as stable.
> 
> * I mentionned using flags in the mpeg2 structures to solve the
> alignment issues, but I failed to provide a patch implementing it at
> this point. This would make the MPEG-2 controls more similar to the
> proposed H.264 ones and generally makes more sense than using a u8 for a
> binary value.

I think it will be good to have this done. If you can make a patch that I can
rebase my tag series on, then that would be best.

> 
> * Some time ago, we also discussed adding extra fields to the structures
> for later use. Do you think that is still relevant? Adding a flags
> elements (with unused bits) would certainly help in this direction
> anyway.

If you switch to bitfields, then there might be enough room anyway for
addition flags in the future.

> 
> * During the discussions on the spec, the concensus was also to use one
> slice_params per-slice (instead of per-picture) which requires splitting
> the per-picture and the per-slice parameters. On this as well, I fell
> behind and didn't send a proposal yet.
> 
> So my question is: does it need to be done for 4.20 or can we affoard
> going through another version cycle for these changes?
> 
> It was my impression that we wanted to wait for another driver to use
> the API to declare it stable (and move the driver out of staging).

Good question, I would like to hear what Mauro says.

I would prefer to postpone this to 4.21, since I don't feel comfortable
with such large changes for an rcX (esp. splitting the per-picture/slice
parameters).

Regards,

	Hans

> 
> Cheers,
> 
> Paul
> 
>> Tomasz: you commented that the text still referred to the tag as a u64,
>> but that was only in the cover letter, not the patches themselves. So
>> I don't plan to post a v3 just for that.
>>
>> Regards,
>>
>> 	Hans
>>
>>> Changes since v1:
>>>
>>> - changed to a u32 tag. Using a 64 bit tag was overly complicated due
>>>   to the bad layout of the v4l2_buffer struct, and there is no real
>>>   need for it by applications.
>>>
>>> Main changes since the RFC:
>>>
>>> - Added new buffer capability flag
>>> - Added m2m helper to copy data between buffers
>>> - Added documentation
>>> - Added tag logging in v4l2-ioctl.c
>>>
>>> Hans Verkuil (9):
>>>   videodev2.h: add tag support
>>>   vb2: add tag support
>>>   v4l2-ioctl.c: log v4l2_buffer tag
>>>   buffer.rst: document the new buffer tag feature.
>>>   v4l2-mem2mem: add v4l2_m2m_buf_copy_data helper function
>>>   vb2: add new supports_tags queue flag
>>>   vim2m: add tag support
>>>   vicodec: add tag support
>>>   cedrus: add tag support
>>>
>>>  Documentation/media/uapi/v4l/buffer.rst       | 32 +++++++++----
>>>  .../media/uapi/v4l/vidioc-reqbufs.rst         |  4 ++
>>>  .../media/common/videobuf2/videobuf2-v4l2.c   | 45 ++++++++++++++++---
>>>  drivers/media/platform/vicodec/vicodec-core.c | 14 ++----
>>>  drivers/media/platform/vim2m.c                | 14 ++----
>>>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ----
>>>  drivers/media/v4l2-core/v4l2-ioctl.c          |  9 ++--
>>>  drivers/media/v4l2-core/v4l2-mem2mem.c        | 23 ++++++++++
>>>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  9 ++--
>>>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
>>>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++-----
>>>  .../staging/media/sunxi/cedrus/cedrus_video.c |  2 +
>>>  include/media/v4l2-mem2mem.h                  | 21 +++++++++
>>>  include/media/videobuf2-core.h                |  2 +
>>>  include/media/videobuf2-v4l2.h                | 21 ++++++++-
>>>  include/uapi/linux/v4l2-controls.h            | 14 +++---
>>>  include/uapi/linux/videodev2.h                |  9 +++-
>>>  17 files changed, 178 insertions(+), 73 deletions(-)
>>>

Re: [PATCHv2 0/9] vb2/cedrus: add tag support

[Alexandre Courbot]

On Wed, Nov 14, 2018 at 10:47 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> From: Hans Verkuil <hansverk@cisco.com>
>
> As was discussed here (among other places):
>
> https://lkml.org/lkml/2018/10/19/440
>
> using capture queue buffer indices to refer to reference frames is
> not a good idea. A better idea is to use a 'tag' where the
> application can assign a u64 tag to an output buffer, which is then
> copied to the capture buffer(s) derived from the output buffer.
>
> A u64 is chosen since this allows userspace to also use pointers to
> internal structures as 'tag'.
>
> The first three patches add core tag support, the next patch document
> the tag support, then a new helper function is added to v4l2-mem2mem.c
> to easily copy data from a source to a destination buffer that drivers
> can use.
>
> Next a new supports_tags vb2_queue flag is added to indicate that
> the driver supports tags. Ideally this should not be necessary, but
> that would require that all m2m drivers are converted to using the
> new helper function introduced in the previous patch. That takes more
> time then I have now since we want to get this in for 4.20.
>
> Finally the vim2m, vicodec and cedrus drivers are converted to support
> tags.
>
> I also removed the 'pad' fields from the mpeg2 control structs (it
> should never been added in the first place) and aligned the structs
> to a u32 boundary (u64 for the tag values).
>
> Note that this might change further (Paul suggested using bitfields).
>
> Also note that the cedrus code doesn't set the sequence counter, that's
> something that should still be added before this driver can be moved
> out of staging.
>
> Note: if no buffer is found for a certain tag, then the dma address
> is just set to 0. That happened before as well with invalid buffer
> indices. This should be checked in the driver!
>
> The previous RFC series was tested successfully with the cedrus driver.
>
> Regards,
>
>         Hans
>
> Changes since v1:
>
> - changed to a u32 tag. Using a 64 bit tag was overly complicated due
>   to the bad layout of the v4l2_buffer struct, and there is no real
>   need for it by applications.
>
> Main changes since the RFC:
>
> - Added new buffer capability flag
> - Added m2m helper to copy data between buffers
> - Added documentation
> - Added tag logging in v4l2-ioctl.c
>
> Hans Verkuil (9):
>   videodev2.h: add tag support
>   vb2: add tag support
>   v4l2-ioctl.c: log v4l2_buffer tag
>   buffer.rst: document the new buffer tag feature.
>   v4l2-mem2mem: add v4l2_m2m_buf_copy_data helper function
>   vb2: add new supports_tags queue flag
>   vim2m: add tag support
>   vicodec: add tag support
>   cedrus: add tag support

Good call on the v4l2_m2m_buf_copy_data() function!

Reviewed-by: Alexandre Courbot <acourbot@chromium.org>

>
>  Documentation/media/uapi/v4l/buffer.rst       | 32 +++++++++----
>  .../media/uapi/v4l/vidioc-reqbufs.rst         |  4 ++
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 45 ++++++++++++++++---
>  drivers/media/platform/vicodec/vicodec-core.c | 14 ++----
>  drivers/media/platform/vim2m.c                | 14 ++----
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ----
>  drivers/media/v4l2-core/v4l2-ioctl.c          |  9 ++--
>  drivers/media/v4l2-core/v4l2-mem2mem.c        | 23 ++++++++++
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  9 ++--
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 +
>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++-----
>  .../staging/media/sunxi/cedrus/cedrus_video.c |  2 +
>  include/media/v4l2-mem2mem.h                  | 21 +++++++++
>  include/media/videobuf2-core.h                |  2 +
>  include/media/videobuf2-v4l2.h                | 21 ++++++++-
>  include/uapi/linux/v4l2-controls.h            | 14 +++---
>  include/uapi/linux/videodev2.h                |  9 +++-
>  17 files changed, 178 insertions(+), 73 deletions(-)
>
> --
> 2.19.1
>