[RFC PATCHv2 0/5] vb2/cedrus: add tag support

[RFC PATCHv2 0/5] vb2/cedrus: add tag support

[Hans Verkuil]

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' (thanks to Alexandre
for the excellent name; it's much better than 'cookie') 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 two patches add core tag support, the next two patches
add tag support to vim2m and vicodec, and the final patch (compile
tested only!) adds support to the cedrus driver.

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).

The cedrus code now also copies the timestamps (didn't happen before)
but the sequence counter is still not set, that's something that should
still be added.

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!

Also missing in this series are documentation updates, which is why
it is marked RFC.

I would very much appreciate it if someone can test the cedrus driver
with these changes. If it works, then I can prepare a real patch series
for 4.20. It would be really good if the API is as stable as we can make
it before 4.20 is released.

Regards,

        Hans

Changes since v1:

- cookie -> tag
- renamed v4l2_tag to v4l2_buffer_tag
- dropped spurious 'to' in the commit log of patch 1

Hans Verkuil (5):
  videodev2.h: add tag support
  vb2: add tag support
  vim2m: add tag support
  vicodec: add tag support
  cedrus: add tag support

 .../media/common/videobuf2/videobuf2-v4l2.c   | 43 ++++++++++++++++---
 drivers/media/platform/vicodec/vicodec-core.c |  3 ++
 drivers/media/platform/vim2m.c                |  3 ++
 drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ----
 drivers/staging/media/sunxi/cedrus/cedrus.h   |  8 ++--
 .../staging/media/sunxi/cedrus/cedrus_dec.c   | 10 +++++
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++-----
 include/media/videobuf2-v4l2.h                | 18 ++++++++
 include/uapi/linux/v4l2-controls.h            | 14 +++---
 include/uapi/linux/videodev2.h                | 37 +++++++++++++++-
 10 files changed, 127 insertions(+), 39 deletions(-)

-- 
2.19.1

[RFC PATCHv2 1/5] 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.

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

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index c8e8ff810190..a6f81f368e01 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -912,6 +912,11 @@ struct v4l2_plane {
 	__u32			reserved[11];
 };
 
+struct v4l2_buffer_tag {
+	__u32 low;
+	__u32 high;
+};
+
 /**
  * struct v4l2_buffer - video buffer info
  * @index:	id number of the buffer
@@ -950,7 +955,10 @@ struct v4l2_buffer {
 	__u32			flags;
 	__u32			field;
 	struct timeval		timestamp;
-	struct v4l2_timecode	timecode;
+	union {
+		struct v4l2_timecode	timecode;
+		struct v4l2_buffer_tag	tag;
+	};
 	__u32			sequence;
 
 	/* memory location */
@@ -988,6 +996,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 */
@@ -1007,6 +1017,31 @@ struct v4l2_buffer {
 /* request_fd is valid */
 #define V4L2_BUF_FLAG_REQUEST_FD		0x00800000
 
+static inline void v4l2_buffer_set_tag(struct v4l2_buffer *buf, __u64 tag)
+{
+	buf->tag.high = tag >> 32;
+	buf->tag.low = tag & 0xffffffffULL;
+	buf->flags |= V4L2_BUF_FLAG_TAG;
+}
+
+static inline void v4l2_buffer_set_tag_ptr(struct v4l2_buffer *buf,
+					   const void *tag)
+{
+	v4l2_buffer_set_tag(buf, (__u64)tag);
+}
+
+static inline __u64 v4l2_buffer_get_tag(const struct v4l2_buffer *buf)
+{
+	if (!(buf->flags & V4L2_BUF_FLAG_TAG))
+		return 0;
+	return (((__u64)buf->tag.high) << 32) | (__u64)buf->tag.low;
+}
+
+static inline void *v4l2_buffer_get_tag_ptr(const struct v4l2_buffer *buf)
+{
+	return (void *)v4l2_buffer_get_tag(buf);
+}
+
 /**
  * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
  *
-- 
2.19.1

Re: [RFC PATCHv2 1/5] videodev2.h: add tag support

[Paul Kocialkowski]

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

Hi,

On Mon, 2018-11-12 at 09:33 +0100, Hans Verkuil wrote:
> 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.

I am getting the following warning when building with this patch:

videodev2.h:1044:9: warning: cast to pointer from integer of different
size [-Wint-to-pointer-cast]
  return (void *)v4l2_buffer_get_tag(buf);

That is because we are on a 32-bit architecture here, so pointers are
not 64-bit long, thus we can't have an equivalency between tag and
pointer.

It looks like this isn't used in the following patches, so perhaps it
should be dropped?

Cheers,

Paul

> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  include/uapi/linux/videodev2.h | 37 +++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index c8e8ff810190..a6f81f368e01 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -912,6 +912,11 @@ struct v4l2_plane {
>  	__u32			reserved[11];
>  };
>  
> +struct v4l2_buffer_tag {
> +	__u32 low;
> +	__u32 high;
> +};
> +
>  /**
>   * struct v4l2_buffer - video buffer info
>   * @index:	id number of the buffer
> @@ -950,7 +955,10 @@ struct v4l2_buffer {
>  	__u32			flags;
>  	__u32			field;
>  	struct timeval		timestamp;
> -	struct v4l2_timecode	timecode;
> +	union {
> +		struct v4l2_timecode	timecode;
> +		struct v4l2_buffer_tag	tag;
> +	};
>  	__u32			sequence;
>  
>  	/* memory location */
> @@ -988,6 +996,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 */
> @@ -1007,6 +1017,31 @@ struct v4l2_buffer {
>  /* request_fd is valid */
>  #define V4L2_BUF_FLAG_REQUEST_FD		0x00800000
>  
> +static inline void v4l2_buffer_set_tag(struct v4l2_buffer *buf, __u64 tag)
> +{
> +	buf->tag.high = tag >> 32;
> +	buf->tag.low = tag & 0xffffffffULL;
> +	buf->flags |= V4L2_BUF_FLAG_TAG;
> +}
> +
> +static inline void v4l2_buffer_set_tag_ptr(struct v4l2_buffer *buf,
> +					   const void *tag)
> +{
> +	v4l2_buffer_set_tag(buf, (__u64)tag);
> +}
> +
> +static inline __u64 v4l2_buffer_get_tag(const struct v4l2_buffer *buf)
> +{
> +	if (!(buf->flags & V4L2_BUF_FLAG_TAG))
> +		return 0;
> +	return (((__u64)buf->tag.high) << 32) | (__u64)buf->tag.low;
> +}
> +
> +static inline void *v4l2_buffer_get_tag_ptr(const struct v4l2_buffer *buf)
> +{
> +	return (void *)v4l2_buffer_get_tag(buf);
> +}
> +
>  /**
>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
>   *
-- 
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 --]

[RFC PATCHv2 2/5] 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                | 18 ++++++++
 2 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index a17033ab2c22..0f909573517f 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 = v4l2_buffer_get_tag(b);
+		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)
+		v4l2_buffer_set_tag(b, vbuf->tag);
+	else
+		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, u64 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..28fd1dbbf8ba 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -44,6 +44,7 @@ struct vb2_v4l2_buffer {
 	__u32			flags;
 	__u32			field;
 	struct v4l2_timecode	timecode;
+	__u64			tag;
 	__u32			sequence;
 	__s32			request_fd;
 	struct vb2_plane	planes[VB2_MAX_PLANES];
@@ -55,6 +56,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, u64 tag,
+		    unsigned int start_idx);
+
 int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
 
 /**
-- 
2.19.1

[RFC PATCHv2 3/5] 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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index d82db738f174..e6ae5a9ac77e 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -245,9 +245,12 @@ static int device_process(struct vim2m_ctx *ctx,
 
 	if (in_vb->flags & V4L2_BUF_FLAG_TIMECODE)
 		out_vb->timecode = in_vb->timecode;
+	else if (in_vb->flags & V4L2_BUF_FLAG_TAG)
+		out_vb->tag = in_vb->tag;
 	out_vb->field = in_vb->field;
 	out_vb->flags = in_vb->flags &
 		(V4L2_BUF_FLAG_TIMECODE |
+		 V4L2_BUF_FLAG_TAG |
 		 V4L2_BUF_FLAG_KEYFRAME |
 		 V4L2_BUF_FLAG_PFRAME |
 		 V4L2_BUF_FLAG_BFRAME |
-- 
2.19.1

[RFC PATCHv2 4/5] 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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index b292cff26c86..4a6b9e841508 100644
--- a/drivers/media/platform/vicodec/vicodec-core.c
+++ b/drivers/media/platform/vicodec/vicodec-core.c
@@ -194,10 +194,13 @@ static int device_process(struct vicodec_ctx *ctx,
 
 	if (in_vb->flags & V4L2_BUF_FLAG_TIMECODE)
 		out_vb->timecode = in_vb->timecode;
+	else if (in_vb->flags & V4L2_BUF_FLAG_TAG)
+		out_vb->tag = in_vb->tag;
 	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_TAG |
 		 V4L2_BUF_FLAG_KEYFRAME |
 		 V4L2_BUF_FLAG_PFRAME |
 		 V4L2_BUF_FLAG_BFRAME |
-- 
2.19.1

[RFC PATCHv2 5/5] 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   |  8 ++++---
 .../staging/media/sunxi/cedrus/cedrus_dec.c   | 10 +++++++++
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++++++-----------
 include/uapi/linux/v4l2-controls.h            | 14 +++++--------
 5 files changed, 29 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..a4bc19ae6bcc 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -142,11 +142,13 @@ 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..76fed2f1f5e2 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -53,6 +53,16 @@ void cedrus_device_run(void *priv)
 		break;
 	}
 
+	run.dst->vb2_buf.timestamp = run.src->vb2_buf.timestamp;
+	if (run.src->flags & V4L2_BUF_FLAG_TIMECODE)
+		run.dst->timecode = run.src->timecode;
+	else if (run.src->flags & V4L2_BUF_FLAG_TAG)
+		run.dst->tag = run.src->tag;
+	run.dst->flags = run.src->flags &
+		(V4L2_BUF_FLAG_TIMECODE |
+		 V4L2_BUF_FLAG_TAG |
+		 V4L2_BUF_FLAG_TSTAMP_SRC_MASK);
+
 	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/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 998983a6e6b7..43f2f9148b3c 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;
+	__u64	backward_ref_tag;
+	__u64	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: [RFC PATCHv2 5/5] cedrus: add tag support

[Paul Kocialkowski]

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

Hi,

On Mon, 2018-11-12 at 09:33 +0100, Hans Verkuil wrote:
> Replace old reference frame indices by new tag method.

I tested this for the cedrus driver and it works properly!
Thanks a lot for implementating this for our driver.
I have one minor cosmetic comment below.

Regarding the padding concerns, I am wondering if we should convert some
of the fields to flags (as it's done in the proposed H264 spec) when
they are binary. We could then use this flags element as padding,
instead of picking the last item and increasing its size.

What do you think?

> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 --------
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  8 ++++---
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   | 10 +++++++++
>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++++++-----------
>  include/uapi/linux/v4l2-controls.h            | 14 +++++--------
>  5 files changed, 29 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..a4bc19ae6bcc 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
> @@ -142,11 +142,13 @@ 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;

Maybe adding a new line here would increase readability?

Cheers,

Paul

> +	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..76fed2f1f5e2 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
> @@ -53,6 +53,16 @@ void cedrus_device_run(void *priv)
>  		break;
>  	}
>  
> +	run.dst->vb2_buf.timestamp = run.src->vb2_buf.timestamp;
> +	if (run.src->flags & V4L2_BUF_FLAG_TIMECODE)
> +		run.dst->timecode = run.src->timecode;
> +	else if (run.src->flags & V4L2_BUF_FLAG_TAG)
> +		run.dst->tag = run.src->tag;
> +	run.dst->flags = run.src->flags &
> +		(V4L2_BUF_FLAG_TIMECODE |
> +		 V4L2_BUF_FLAG_TAG |
> +		 V4L2_BUF_FLAG_TSTAMP_SRC_MASK);
> +
>  	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/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 998983a6e6b7..43f2f9148b3c 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;
> +	__u64	backward_ref_tag;
> +	__u64	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 {
-- 
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: [RFC PATCHv2 5/5] cedrus: add tag support

[Hans Verkuil]

On 11/12/2018 05:47 PM, Paul Kocialkowski wrote:
> Hi,
> 
> On Mon, 2018-11-12 at 09:33 +0100, Hans Verkuil wrote:
>> Replace old reference frame indices by new tag method.
> 
> I tested this for the cedrus driver and it works properly!
> Thanks a lot for implementating this for our driver.
> I have one minor cosmetic comment below.
> 
> Regarding the padding concerns, I am wondering if we should convert some
> of the fields to flags (as it's done in the proposed H264 spec) when
> they are binary. We could then use this flags element as padding,
> instead of picking the last item and increasing its size.
> 
> What do you think?

Can you take a look at how that would work out? Make a proposal or something
like that?

It's not a bad idea.

Regards,

	Hans

> 
>> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
>> ---
>>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 --------
>>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  8 ++++---
>>  .../staging/media/sunxi/cedrus/cedrus_dec.c   | 10 +++++++++
>>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++++++-----------
>>  include/uapi/linux/v4l2-controls.h            | 14 +++++--------
>>  5 files changed, 29 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..a4bc19ae6bcc 100644
>> --- a/drivers/staging/media/sunxi/cedrus/cedrus.h
>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
>> @@ -142,11 +142,13 @@ 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;
> 
> Maybe adding a new line here would increase readability?
> 
> Cheers,
> 
> Paul
> 
>> +	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..76fed2f1f5e2 100644
>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
>> @@ -53,6 +53,16 @@ void cedrus_device_run(void *priv)
>>  		break;
>>  	}
>>  
>> +	run.dst->vb2_buf.timestamp = run.src->vb2_buf.timestamp;
>> +	if (run.src->flags & V4L2_BUF_FLAG_TIMECODE)
>> +		run.dst->timecode = run.src->timecode;
>> +	else if (run.src->flags & V4L2_BUF_FLAG_TAG)
>> +		run.dst->tag = run.src->tag;
>> +	run.dst->flags = run.src->flags &
>> +		(V4L2_BUF_FLAG_TIMECODE |
>> +		 V4L2_BUF_FLAG_TAG |
>> +		 V4L2_BUF_FLAG_TSTAMP_SRC_MASK);
>> +
>>  	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/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 998983a6e6b7..43f2f9148b3c 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;
>> +	__u64	backward_ref_tag;
>> +	__u64	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 {

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

[Paul Kocialkowski]

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

Hi,

On Mon, 2018-11-12 at 09:33 +0100, Hans Verkuil wrote:
> 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' (thanks to Alexandre
> for the excellent name; it's much better than 'cookie') 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'.

As I mentionned in the dedicated patch, this approach is troublesome on
32-bit platforms. Do we really need this equivalency?

> The first two patches add core tag support, the next two patches
> add tag support to vim2m and vicodec, and the final patch (compile
> tested only!) adds support to the cedrus driver.
> 
> 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).
> 
> The cedrus code now also copies the timestamps (didn't happen before)
> but the sequence counter is still not set, that's something that should
> still be added.
> 
> 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!

Thanks for making these changes!

> Also missing in this series are documentation updates, which is why
> it is marked RFC.
> 
> I would very much appreciate it if someone can test the cedrus driver
> with these changes. If it works, then I can prepare a real patch series
> for 4.20. It would be really good if the API is as stable as we can make
> it before 4.20 is released.

I just had a go at testing the patches on cedrus with minimal userspace
adaptation to deal with the tags and everything looks good!

I only set the tag when queing each OUTPUT buffer and the driver
properly matched the CAPTURE reference buffer.

I think we should make it clear in the stateless spec that multiple
OUTPUT buffers can be allowed for the same tag, but that a single
CAPTURE buffer should be used. Otherwise, the hardware can't use
different partly-decoded buffers as references (and the tag API doesn't
allow that either, since a single buffer index is returned for a tag).

What do you think?

Cheers,

Paul

> Regards,
> 
>         Hans
> 
> Changes since v1:
> 
> - cookie -> tag
> - renamed v4l2_tag to v4l2_buffer_tag
> - dropped spurious 'to' in the commit log of patch 1
> 
> Hans Verkuil (5):
>   videodev2.h: add tag support
>   vb2: add tag support
>   vim2m: add tag support
>   vicodec: add tag support
>   cedrus: add tag support
> 
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 43 ++++++++++++++++---
>  drivers/media/platform/vicodec/vicodec-core.c |  3 ++
>  drivers/media/platform/vim2m.c                |  3 ++
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ----
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  8 ++--
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   | 10 +++++
>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++-----
>  include/media/videobuf2-v4l2.h                | 18 ++++++++
>  include/uapi/linux/v4l2-controls.h            | 14 +++---
>  include/uapi/linux/videodev2.h                | 37 +++++++++++++++-
>  10 files changed, 127 insertions(+), 39 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: [RFC PATCHv2 0/5] vb2/cedrus: add tag support

[Hans Verkuil]

On 11/12/2018 05:32 PM, Paul Kocialkowski wrote:
> Hi,
> 
> On Mon, 2018-11-12 at 09:33 +0100, Hans Verkuil wrote:
>> 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' (thanks to Alexandre
>> for the excellent name; it's much better than 'cookie') 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'.
> 
> As I mentionned in the dedicated patch, this approach is troublesome on
> 32-bit platforms. Do we really need this equivalency?

It's just a bug in the header that I need to fix. I just need to use
the right cast. I think it is a desirable feature.

>> The first two patches add core tag support, the next two patches
>> add tag support to vim2m and vicodec, and the final patch (compile
>> tested only!) adds support to the cedrus driver.
>>
>> 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).
>>
>> The cedrus code now also copies the timestamps (didn't happen before)
>> but the sequence counter is still not set, that's something that should
>> still be added.
>>
>> 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!
> 
> Thanks for making these changes!
> 
>> Also missing in this series are documentation updates, which is why
>> it is marked RFC.
>>
>> I would very much appreciate it if someone can test the cedrus driver
>> with these changes. If it works, then I can prepare a real patch series
>> for 4.20. It would be really good if the API is as stable as we can make
>> it before 4.20 is released.
> 
> I just had a go at testing the patches on cedrus with minimal userspace
> adaptation to deal with the tags and everything looks good!

Great!

> I only set the tag when queing each OUTPUT buffer and the driver
> properly matched the CAPTURE reference buffer.
> 
> I think we should make it clear in the stateless spec that multiple
> OUTPUT buffers can be allowed for the same tag, but that a single
> CAPTURE buffer should be used. Otherwise, the hardware can't use
> different partly-decoded buffers as references (and the tag API doesn't
> allow that either, since a single buffer index is returned for a tag).

Actually, the tag API allows for multiple buffers for the same tag: that's
what the last argument of vb2_find_tag() is for: you can continue looking
for buffers with the given tag from where you left off:

	second_idx = -1;

	first_idx = vb2_find_tag(q, tag, 0);
	if (first_idx >= 0)
		second_idx = vb2_find_tag(q, tag, first_idx + 1);

I think how OUTPUT buffers relate to CAPTURE buffers is really a property
of the codec that's used (I mean H.264 vs MPEG vs VP8 etc). The tag API
supports any combination.

For the stateless MPEG codec it is simple: one OUTPUT frame produces one
CAPTURE frame. So this can be documented for the control that has the
buffer references.

Thank you very much for testing this. I'll prepare a new patch series this
week which will hopefully be the final version.

Regards,

	Hans

> 
> What do you think?
> 
> Cheers,
> 
> Paul
> 
>> Regards,
>>
>>         Hans
>>
>> Changes since v1:
>>
>> - cookie -> tag
>> - renamed v4l2_tag to v4l2_buffer_tag
>> - dropped spurious 'to' in the commit log of patch 1
>>
>> Hans Verkuil (5):
>>   videodev2.h: add tag support
>>   vb2: add tag support
>>   vim2m: add tag support
>>   vicodec: add tag support
>>   cedrus: add tag support
>>
>>  .../media/common/videobuf2/videobuf2-v4l2.c   | 43 ++++++++++++++++---
>>  drivers/media/platform/vicodec/vicodec-core.c |  3 ++
>>  drivers/media/platform/vim2m.c                |  3 ++
>>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ----
>>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  8 ++--
>>  .../staging/media/sunxi/cedrus/cedrus_dec.c   | 10 +++++
>>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++-----
>>  include/media/videobuf2-v4l2.h                | 18 ++++++++
>>  include/uapi/linux/v4l2-controls.h            | 14 +++---
>>  include/uapi/linux/videodev2.h                | 37 +++++++++++++++-
>>  10 files changed, 127 insertions(+), 39 deletions(-)
>>