[PATCHv5 0/8] vb2/cedrus: use timestamps to identify buffers

[PATCHv5 0/8] vb2/cedrus: use timestamps to identify buffers

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

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. 

Instead, after a long irc discussion:

https://linuxtv.org/irc/irclogger_log/v4l?date=2018-12-12,Wed

it was decided to use the timestamp in v4l2_buffer for this.

However, struct timeval cannot be used in a compound control since
the size of struct timeval differs between 32 and 64 bit architectures,
and there are also changes upcoming for y2038 support.

But internally the kernel converts the timeval to a u64 (nsecs since
boot). So we provide a helper function in videodev2.h that converts
the timeval to a u64, and that u64 can be used inside compound controls.

In the not too distant future we want to create a new struct v4l2_buffer,
and then we'll use u64 from the start, so in that case the helper function
would no longer be needed.

The first three patches add a new m2m helper function to correctly copy
the relevant data from an output buffer to a capture buffer. This will
simplify m2m drivers (in fact, many m2m drivers do not do this quite
right, so a helper function was really needed).

The fourth patch clears up messy timecode documentation that I came
across while working on this.

Patch 5 adds the new v4l2_timeval_to_ns helper function to videodev2.h.
The next patch adds the vb2_find_timestamp() function to find buffers
with a specific u64 timestamp.

Finally the cedrus driver and documentation are updated to use a
timestamp as buffer identifier.

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.

Please note that this patch series will have to be updated one more
time when pending 4.20 fixes are merged back into our master since
those patches will move the cedrus mpeg controls to a different header.

Regards,

        Hans

Hans Verkuil (8):
  v4l2-mem2mem: add v4l2_m2m_buf_copy_data helper function
  vim2m: use v4l2_m2m_buf_copy_data
  vicodec: use v4l2_m2m_buf_copy_data
  buffer.rst: clean up timecode documentation
  videodev2.h: add v4l2_timeval_to_ns inline function
  vb2: add vb2_find_timestamp()
  cedrus: identify buffers by timestamp
  extended-controls.rst: update the mpeg2 compound controls

 Documentation/media/uapi/v4l/buffer.rst       | 11 ++++----
 .../media/uapi/v4l/extended-controls.rst      | 28 +++++++++++--------
 .../media/common/videobuf2/videobuf2-v4l2.c   | 22 +++++++++++++--
 drivers/media/platform/vicodec/vicodec-core.c | 12 +-------
 drivers/media/platform/vim2m.c                | 12 +-------
 drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ------
 drivers/media/v4l2-core/v4l2-mem2mem.c        | 20 +++++++++++++
 drivers/staging/media/sunxi/cedrus/cedrus.h   |  9 ++++--
 .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 ++
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++++--------
 include/media/v4l2-mem2mem.h                  | 20 +++++++++++++
 include/media/videobuf2-v4l2.h                | 19 ++++++++++++-
 include/uapi/linux/v4l2-controls.h            | 14 ++++------
 include/uapi/linux/videodev2.h                | 12 ++++++++
 14 files changed, 136 insertions(+), 75 deletions(-)

-- 
2.19.2

[PATCHv5 1/8] v4l2-mem2mem: add v4l2_m2m_buf_copy_data helper function

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

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-cisco@xs4all.nl>
Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 20 ++++++++++++++++++++
 include/media/v4l2-mem2mem.h           | 20 ++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 5bbdec55b7d7..631f4e2aa942 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -975,6 +975,26 @@ 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_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_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..43e447dcf69d 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -622,6 +622,26 @@ 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), field and the TIMECODE, 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.2

[PATCHv5 2/8] vim2m: use v4l2_m2m_buf_copy_data

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Use the new v4l2_m2m_buf_copy_data() function in vim2m.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/vim2m.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index d01821a6906a..33397d4a1402 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:
-- 
2.19.2

[PATCHv5 3/8] vicodec: use v4l2_m2m_buf_copy_data

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Use the new v4l2_m2m_buf_copy_data() function in vicodec.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/platform/vicodec/vicodec-core.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/media/platform/vicodec/vicodec-core.c b/drivers/media/platform/vicodec/vicodec-core.c
index 2b7daff63425..bed15580f3ec 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,
 	}
 
 	dst_vb->sequence = q_dst->sequence++;
-	dst_vb->vb2_buf.timestamp = src_vb->vb2_buf.timestamp;
-
-	if (src_vb->flags & V4L2_BUF_FLAG_TIMECODE)
-		dst_vb->timecode = src_vb->timecode;
-	dst_vb->field = src_vb->field;
 	dst_vb->flags &= ~V4L2_BUF_FLAG_LAST;
-	dst_vb->flags |= src_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(src_vb, dst_vb, !ctx->is_enc);
 
 	return 0;
 }
-- 
2.19.2

[PATCHv5 4/8] buffer.rst: clean up timecode documentation

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

V4L2_BUF_FLAG_TIMECODE is not video capture specific, so drop that
part.

The 'Timecodes' section was a bit messy, so that's cleaned up.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Reviewed-by: Alexandre Courbot <acourbot@chromium.org>
---
 Documentation/media/uapi/v4l/buffer.rst | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index c5013adaa44d..3a31f308f136 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -230,8 +230,7 @@ struct v4l2_buffer
     * - 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
@@ -711,10 +710,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
-- 
2.19.2

[PATCHv5 5/8] videodev2.h: add v4l2_timeval_to_ns inline function

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

We want to be able to uniquely identify buffers for stateless
codecs. The internal timestamp (a u64) as stored internally in the
kernel is a suitable candidate for that, but in struct v4l2_buffer
it is represented as a struct timeval.

Add a v4l2_timeval_to_ns() function that converts the struct timeval
into a u64 in the same way that the kernel does. This makes it possible
to use this u64 elsewhere as a unique identifier of the buffer.

Since timestamps are also copied from the output buffer to the
corresponding capture buffer(s) by M2M devices, the u64 can be
used to refer to both output and capture buffers.

The plan is that in the future we redesign struct v4l2_buffer and use
u64 for the timestamp instead of a struct timeval (which has lots of
problems with 32 vs 64 bit and y2038 layout changes), and then there
is no more need to use this function.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 include/uapi/linux/videodev2.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 2db1635de956..3580c1ea4fba 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -971,6 +971,18 @@ struct v4l2_buffer {
 	};
 };
 
+/**
+ * v4l2_timeval_to_ns - Convert timeval to nanoseconds
+ * @ts:		pointer to the timeval variable to be converted
+ *
+ * Returns the scalar nanosecond representation of the timeval
+ * parameter.
+ */
+static inline u64 v4l2_timeval_to_ns(const struct timeval *tv)
+{
+	return (__u64)tv->tv_sec * 1000000000ULL + tv->tv_usec * 1000;
+}
+
 /*  Flags for 'flags' field */
 /* Buffer is mapped (flag) */
 #define V4L2_BUF_FLAG_MAPPED			0x00000001
-- 
2.19.2

[PATCHv5 6/8] vb2: add vb2_find_timestamp()

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Use v4l2_timeval_to_ns instead of timeval_to_ns to ensure that
both kernelspace and userspace will use the same conversion
function.

Next add a new vb2_find_timestamp() function to find buffers
with a specific timestamp.

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

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 .../media/common/videobuf2/videobuf2-v4l2.c   | 22 +++++++++++++++++--
 include/media/videobuf2-v4l2.h                | 19 +++++++++++++++-
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 1244c246d0c4..8d1231c2da65 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -143,7 +143,7 @@ static void __copy_timestamp(struct vb2_buffer *vb, const void *pb)
 		 * and the timecode field and flag if needed.
 		 */
 		if (q->copy_timestamp)
-			vb->timestamp = timeval_to_ns(&b->timestamp);
+			vb->timestamp = v4l2_timeval_to_ns(&b->timestamp);
 		vbuf->flags |= b->flags & V4L2_BUF_FLAG_TIMECODE;
 		if (b->flags & V4L2_BUF_FLAG_TIMECODE)
 			vbuf->timecode = b->timecode;
@@ -460,7 +460,8 @@ 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_TIMECODE)
+		b->timecode = vbuf->timecode;
 	b->sequence = vbuf->sequence;
 	b->reserved2 = 0;
 	b->request_fd = 0;
@@ -586,6 +587,23 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
 	.copy_timestamp		= __copy_timestamp,
 };
 
+int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
+		       unsigned int start_idx)
+{
+	unsigned int i;
+
+	for (i = start_idx; i < q->num_buffers; i++) {
+		struct vb2_buffer *vb = q->bufs[i];
+
+		if ((vb->state == VB2_BUF_STATE_DEQUEUED ||
+		     vb->state == VB2_BUF_STATE_DONE) &&
+		    vb->timestamp == timestamp)
+			return i;
+	}
+	return -1;
+}
+EXPORT_SYMBOL_GPL(vb2_find_timestamp);
+
 /*
  * 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..80f1afa0edad 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -32,7 +32,7 @@
  *		&enum v4l2_field.
  * @timecode:	frame timecode.
  * @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
@@ -55,6 +55,23 @@ struct vb2_v4l2_buffer {
 #define to_vb2_v4l2_buffer(vb) \
 	container_of(vb, struct vb2_v4l2_buffer, vb2_buf)
 
+/**
+ * vb2_find_timestamp() - Find buffer with given timestamp in the queue
+ *
+ * @q:		pointer to &struct vb2_queue with videobuf2 queue.
+ * @timestamp:	the timestamp 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 timestamp 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 @timestamp, or
+ * -1 if no buffer with @timestamp was found.
+ */
+int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
+		       unsigned int start_idx);
+
 int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
 
 /**
-- 
2.19.2

[PATCHv5 7/8] cedrus: identify buffers by timestamp

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Use the new v4l2_m2m_buf_copy_data helper function and use
timestamps to refer to reference frames instead of using
buffer indices.

Signed-off-by: Hans Verkuil <hverkuil-cisco@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 ++++++++-----------
 include/uapi/linux/v4l2-controls.h            | 14 +++++--------
 5 files changed, 22 insertions(+), 33 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 129a986fa7e1..e859496e4e95 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1661,15 +1661,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 3acfdcf83691..4aedd24a9848 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus.h
+++ b/drivers/staging/media/sunxi/cedrus/cedrus.h
@@ -140,11 +140,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 591d191d4286..443fb037e1cf 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_dec.c
@@ -50,6 +50,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);
 
 	/* Complete request(s) controls if needed. */
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
index 9abd39cae38c..f05d859d525e 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_timestamp(cap_q, slice_params->forward_ref_ts, 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_timestamp(cap_q, slice_params->backward_ref_ts, 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..6e5067f1aca4 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_ts;
+	__u64	forward_ref_ts;
 
 	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.2

[PATCHv5 8/8] extended-controls.rst: update the mpeg2 compound controls

[hverkuil-cisco]

From: Hans Verkuil <hverkuil-cisco@xs4all.nl>

The layout of the compound controls has changed to fix
32/64 bit alignment issues and the use of timestamps instead of
buffer indices to refer to buffers.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 .../media/uapi/v4l/extended-controls.rst      | 28 +++++++++++--------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
index 0b302413ab3a..cc986002bfc9 100644
--- a/Documentation/media/uapi/v4l/extended-controls.rst
+++ b/Documentation/media/uapi/v4l/extended-controls.rst
@@ -1541,17 +1541,23 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - ``picture``
       - Structure with MPEG-2 picture metadata, merging relevant fields from
 	the picture header and picture coding extension parts of the bitstream.
-    * - __u8
+    * - __u64
+      - ``backward_ref_ts``
+      - Timestamp of the V4L2 capture buffer to use as backward reference, used
+        with B-coded and P-coded frames. The timestamp refers to the
+	``timestamp`` field in struct :c:type:`v4l2_buffer`. Use the
+	:c:func:`v4l2_timeval_to_ns()` function to convert the struct
+	:c:type:`timeval` in struct :c:type:`v4l2_buffer` to a __u64.
+    * - __u64
+      - ``forward_ref_ts``
+      - Timestamp for the V4L2 capture buffer to use as forward reference, used
+        with B-coded frames. The timestamp refers to the ``timestamp`` field in
+	struct :c:type:`v4l2_buffer`. Use the :c:func:`v4l2_timeval_to_ns()`
+	function to convert the struct :c:type:`timeval` in struct
+	:c:type:`v4l2_buffer` to a __u64.
+    * - __u32
       - ``quantiser_scale_code``
       - Code used to determine the quantization scale to use for the IDCT.
-    * - __u8
-      - ``backward_ref_index``
-      - Index for the V4L2 buffer to use as backward reference, used with
-	B-coded and P-coded frames.
-    * - __u8
-      - ``forward_ref_index``
-      - Index for the V4L2 buffer to use as forward reference, used with
-	B-coded frames.
 
 .. c:type:: v4l2_mpeg2_sequence
 
@@ -1572,7 +1578,7 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
       - ``vbv_buffer_size``
       - Used to calculate the required size of the video buffering verifier,
 	defined (in bits) as: 16 * 1024 * vbv_buffer_size.
-    * - __u8
+    * - __u16
       - ``profile_and_level_indication``
       - The current profile and level indication as extracted from the
 	bitstream.
@@ -1630,7 +1636,7 @@ enum v4l2_mpeg_video_h264_hierarchical_coding_type -
     * - __u8
       - ``repeat_first_field``
       - This flag affects the decoding process of progressive frames.
-    * - __u8
+    * - __u16
       - ``progressive_frame``
       - Indicates whether the current frame is progressive.
 
-- 
2.19.2

Re: [PATCHv5 6/8] vb2: add vb2_find_timestamp()

[Jonas Karlman]

Hi Hans,

Since this function only return DEQUEUED and DONE buffers,
it cannot be used to find a capture buffer that is both used for
frame output and is part of the frame reference list.
E.g. a bottom field referencing a top field that is already
part of the capture buffer being used for frame output.
(top and bottom field is output in same buffer)

Jernej Škrabec and me have worked around this issue in cedrus driver by
first checking
the tag/timestamp of the current buffer being used for output frame.


// field pictures may reference current capture buffer and is not
returned by vb2_find_tag
if (v4l2_buf->tag == dpb->tag)
    buf_idx = v4l2_buf->vb2_buf.index;
else
    buf_idx = vb2_find_tag(cap_q, dpb->tag, 0);


What is the recommended way to handle such case?
Could vb2_find_timestamp be extended to allow QUEUED buffers to be returned?


In our sample code we only keep at most one output, one capture buffer
in queue
and use buffer indices as tag/timestamp to simplify buffer handling.
FFmpeg keeps track of buffers/frames referenced and a buffer will not be
reused
until the codec and display pipeline have released all references to it.

Sample code having interlaced and multi-slice support using previous tag
version of this patchset can be found at:
https://github.com/jernejsk/LibreELEC.tv/blob/hw_dec_ffmpeg/projects/Allwinner/patches/linux/0025-H264-fixes.patch#L120-L124
https://github.com/Kwiboo/FFmpeg/compare/4.0.3-Leia-Beta5...v4l2-request-hwaccel

Regards,
Jonas

On 2018-12-12 13:38, hverkuil-cisco@xs4all.nl wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>
> Use v4l2_timeval_to_ns instead of timeval_to_ns to ensure that
> both kernelspace and userspace will use the same conversion
> function.
>
> Next add a new vb2_find_timestamp() function to find buffers
> with a specific timestamp.
>
> This function will only look at DEQUEUED and DONE buffers, i.e.
> buffers that are already processed.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 22 +++++++++++++++++--
>  include/media/videobuf2-v4l2.h                | 19 +++++++++++++++-
>  2 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 1244c246d0c4..8d1231c2da65 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -143,7 +143,7 @@ static void __copy_timestamp(struct vb2_buffer *vb, const void *pb)
>  		 * and the timecode field and flag if needed.
>  		 */
>  		if (q->copy_timestamp)
> -			vb->timestamp = timeval_to_ns(&b->timestamp);
> +			vb->timestamp = v4l2_timeval_to_ns(&b->timestamp);
>  		vbuf->flags |= b->flags & V4L2_BUF_FLAG_TIMECODE;
>  		if (b->flags & V4L2_BUF_FLAG_TIMECODE)
>  			vbuf->timecode = b->timecode;
> @@ -460,7 +460,8 @@ 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_TIMECODE)
> +		b->timecode = vbuf->timecode;
>  	b->sequence = vbuf->sequence;
>  	b->reserved2 = 0;
>  	b->request_fd = 0;
> @@ -586,6 +587,23 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
>  	.copy_timestamp		= __copy_timestamp,
>  };
>  
> +int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
> +		       unsigned int start_idx)
> +{
> +	unsigned int i;
> +
> +	for (i = start_idx; i < q->num_buffers; i++) {
> +		struct vb2_buffer *vb = q->bufs[i];
> +
> +		if ((vb->state == VB2_BUF_STATE_DEQUEUED ||
> +		     vb->state == VB2_BUF_STATE_DONE) &&
> +		    vb->timestamp == timestamp)
> +			return i;
> +	}
> +	return -1;
> +}
> +EXPORT_SYMBOL_GPL(vb2_find_timestamp);
> +
>  /*
>   * 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..80f1afa0edad 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -32,7 +32,7 @@
>   *		&enum v4l2_field.
>   * @timecode:	frame timecode.
>   * @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
> @@ -55,6 +55,23 @@ struct vb2_v4l2_buffer {
>  #define to_vb2_v4l2_buffer(vb) \
>  	container_of(vb, struct vb2_v4l2_buffer, vb2_buf)
>  
> +/**
> + * vb2_find_timestamp() - Find buffer with given timestamp in the queue
> + *
> + * @q:		pointer to &struct vb2_queue with videobuf2 queue.
> + * @timestamp:	the timestamp 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 timestamp 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 @timestamp, or
> + * -1 if no buffer with @timestamp was found.
> + */
> +int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
> +		       unsigned int start_idx);
> +
>  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
>  
>  /**

Re: [PATCHv5 6/8] vb2: add vb2_find_timestamp()

[Hans Verkuil]

On 12/12/18 7:28 PM, Jonas Karlman wrote:
> Hi Hans,
> 
> Since this function only return DEQUEUED and DONE buffers,
> it cannot be used to find a capture buffer that is both used for
> frame output and is part of the frame reference list.
> E.g. a bottom field referencing a top field that is already
> part of the capture buffer being used for frame output.
> (top and bottom field is output in same buffer)
> 
> Jernej Škrabec and me have worked around this issue in cedrus driver by
> first checking
> the tag/timestamp of the current buffer being used for output frame.
> 
> 
> // field pictures may reference current capture buffer and is not
> returned by vb2_find_tag
> if (v4l2_buf->tag == dpb->tag)
>     buf_idx = v4l2_buf->vb2_buf.index;
> else
>     buf_idx = vb2_find_tag(cap_q, dpb->tag, 0);
> 
> 
> What is the recommended way to handle such case?

That is the right approach for this. Interesting corner case, I hadn't
considered that.

> Could vb2_find_timestamp be extended to allow QUEUED buffers to be returned?

No, because only the driver knows what the current buffer is.

Buffers that are queued to the driver are in state ACTIVE. But there may be
multiple ACTIVE buffers and vb2 doesn't know which buffer is currently
being processed by the driver.

So this will have to be checked by the driver itself.

Regards,

	Hans

> 
> 
> In our sample code we only keep at most one output, one capture buffer
> in queue
> and use buffer indices as tag/timestamp to simplify buffer handling.
> FFmpeg keeps track of buffers/frames referenced and a buffer will not be
> reused
> until the codec and display pipeline have released all references to it.
> 
> Sample code having interlaced and multi-slice support using previous tag
> version of this patchset can be found at:
> https://github.com/jernejsk/LibreELEC.tv/blob/hw_dec_ffmpeg/projects/Allwinner/patches/linux/0025-H264-fixes.patch#L120-L124
> https://github.com/Kwiboo/FFmpeg/compare/4.0.3-Leia-Beta5...v4l2-request-hwaccel
> 
> Regards,
> Jonas

Re: [PATCHv5 5/8] videodev2.h: add v4l2_timeval_to_ns inline function

[Jonas Karlman]

On 2018-12-12 13:38, hverkuil-cisco@xs4all.nl wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>
> We want to be able to uniquely identify buffers for stateless
> codecs. The internal timestamp (a u64) as stored internally in the
> kernel is a suitable candidate for that, but in struct v4l2_buffer
> it is represented as a struct timeval.
>
> Add a v4l2_timeval_to_ns() function that converts the struct timeval
> into a u64 in the same way that the kernel does. This makes it possible
> to use this u64 elsewhere as a unique identifier of the buffer.
>
> Since timestamps are also copied from the output buffer to the
> corresponding capture buffer(s) by M2M devices, the u64 can be
> used to refer to both output and capture buffers.
>
> The plan is that in the future we redesign struct v4l2_buffer and use
> u64 for the timestamp instead of a struct timeval (which has lots of
> problems with 32 vs 64 bit and y2038 layout changes), and then there
> is no more need to use this function.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  include/uapi/linux/videodev2.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 2db1635de956..3580c1ea4fba 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -971,6 +971,18 @@ struct v4l2_buffer {
>  	};
>  };
>  
> +/**
> + * v4l2_timeval_to_ns - Convert timeval to nanoseconds
> + * @ts:		pointer to the timeval variable to be converted
> + *
> + * Returns the scalar nanosecond representation of the timeval
> + * parameter.
> + */
> +static inline u64 v4l2_timeval_to_ns(const struct timeval *tv)
> +{
> +	return (__u64)tv->tv_sec * 1000000000ULL + tv->tv_usec * 1000;
> +}
This is causing a compile issue in userspace application, replacing u64
with __u64 solves the compile issue below.

In file included from libavcodec/v4l2_request.h:22,
                 from libavcodec/v4l2_request.c:28:
/home/docker/LibreELEC/build.LibreELEC-H3.arm-9.0-devel/toolchain/armv7ve-libreelec-linux-gnueabi/sysroot/usr/include/linux/videodev2.h:975:19:
error: unknown type name 'u64'
 static __inline__ u64 v4l2_timeval_to_ns(const struct timeval *tv)
                   ^~~

Regards,
Jonas
> +
>  /*  Flags for 'flags' field */
>  /* Buffer is mapped (flag) */
>  #define V4L2_BUF_FLAG_MAPPED			0x00000001

Re: [PATCHv5 5/8] videodev2.h: add v4l2_timeval_to_ns inline function

[Hans Verkuil]

On 12/16/18 10:47 PM, Jonas Karlman wrote:
> 
> On 2018-12-12 13:38, hverkuil-cisco@xs4all.nl wrote:
>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>
>> We want to be able to uniquely identify buffers for stateless
>> codecs. The internal timestamp (a u64) as stored internally in the
>> kernel is a suitable candidate for that, but in struct v4l2_buffer
>> it is represented as a struct timeval.
>>
>> Add a v4l2_timeval_to_ns() function that converts the struct timeval
>> into a u64 in the same way that the kernel does. This makes it possible
>> to use this u64 elsewhere as a unique identifier of the buffer.
>>
>> Since timestamps are also copied from the output buffer to the
>> corresponding capture buffer(s) by M2M devices, the u64 can be
>> used to refer to both output and capture buffers.
>>
>> The plan is that in the future we redesign struct v4l2_buffer and use
>> u64 for the timestamp instead of a struct timeval (which has lots of
>> problems with 32 vs 64 bit and y2038 layout changes), and then there
>> is no more need to use this function.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  include/uapi/linux/videodev2.h | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 2db1635de956..3580c1ea4fba 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -971,6 +971,18 @@ struct v4l2_buffer {
>>  	};
>>  };
>>  
>> +/**
>> + * v4l2_timeval_to_ns - Convert timeval to nanoseconds
>> + * @ts:		pointer to the timeval variable to be converted
>> + *
>> + * Returns the scalar nanosecond representation of the timeval
>> + * parameter.
>> + */
>> +static inline u64 v4l2_timeval_to_ns(const struct timeval *tv)
>> +{
>> +	return (__u64)tv->tv_sec * 1000000000ULL + tv->tv_usec * 1000;
>> +}
> This is causing a compile issue in userspace application, replacing u64
> with __u64 solves the compile issue below.
> 
> In file included from libavcodec/v4l2_request.h:22,
>                  from libavcodec/v4l2_request.c:28:
> /home/docker/LibreELEC/build.LibreELEC-H3.arm-9.0-devel/toolchain/armv7ve-libreelec-linux-gnueabi/sysroot/usr/include/linux/videodev2.h:975:19:
> error: unknown type name 'u64'
>  static __inline__ u64 v4l2_timeval_to_ns(const struct timeval *tv)
>                    ^~~

Oops, I missed that one. Fixed in my git branch.

Thanks for reporting this!

	Hans

> 
> Regards,
> Jonas
>> +
>>  /*  Flags for 'flags' field */
>>  /* Buffer is mapped (flag) */
>>  #define V4L2_BUF_FLAG_MAPPED			0x00000001

Re: [PATCHv5 6/8] vb2: add vb2_find_timestamp()

[Paul Kocialkowski]

Hi,

On Thu, 2018-12-13 at 13:28 +0100, Hans Verkuil wrote:
> On 12/12/18 7:28 PM, Jonas Karlman wrote:
> > Hi Hans,
> > 
> > Since this function only return DEQUEUED and DONE buffers,
> > it cannot be used to find a capture buffer that is both used for
> > frame output and is part of the frame reference list.
> > E.g. a bottom field referencing a top field that is already
> > part of the capture buffer being used for frame output.
> > (top and bottom field is output in same buffer)
> > 
> > Jernej Škrabec and me have worked around this issue in cedrus driver by
> > first checking
> > the tag/timestamp of the current buffer being used for output frame.
> > 
> > 
> > // field pictures may reference current capture buffer and is not
> > returned by vb2_find_tag
> > if (v4l2_buf->tag == dpb->tag)
> >     buf_idx = v4l2_buf->vb2_buf.index;
> > else
> >     buf_idx = vb2_find_tag(cap_q, dpb->tag, 0);
> > 
> > 
> > What is the recommended way to handle such case?
> 
> That is the right approach for this. Interesting corner case, I hadn't
> considered that.
> 
> > Could vb2_find_timestamp be extended to allow QUEUED buffers to be returned?
> 
> No, because only the driver knows what the current buffer is.
> 
> Buffers that are queued to the driver are in state ACTIVE. But there may be
> multiple ACTIVE buffers and vb2 doesn't know which buffer is currently
> being processed by the driver.
> 
> So this will have to be checked by the driver itself.

Interesting corner case indeed, we hadn't considered the possibility of
interlaced pictures refeering to the current capture buffer.

Hans, do you want to include that change in a future revision of this
series or should that be a separate follow-up patch?

I'm fine with both options (and could definitely craft the change in
the latter case).

Cheers,

Paul

> > In our sample code we only keep at most one output, one capture buffer
> > in queue
> > and use buffer indices as tag/timestamp to simplify buffer handling.
> > FFmpeg keeps track of buffers/frames referenced and a buffer will not be
> > reused
> > until the codec and display pipeline have released all references to it.
> > 
> > Sample code having interlaced and multi-slice support using previous tag
> > version of this patchset can be found at:
> > https://github.com/jernejsk/LibreELEC.tv/blob/hw_dec_ffmpeg/projects/Allwinner/patches/linux/0025-H264-fixes.patch#L120-L124
> > https://github.com/Kwiboo/FFmpeg/compare/4.0.3-Leia-Beta5...v4l2-request-hwaccel
> > 
> > Regards,
> > Jonas
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCHv5 0/8] vb2/cedrus: use timestamps to identify buffers

[Paul Kocialkowski]

Hi,

On Wed, 2018-12-12 at 13:38 +0100, hverkuil-cisco@xs4all.nl wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> 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. 
> 
> Instead, after a long irc discussion:
> 
> https://linuxtv.org/irc/irclogger_log/v4l?date=2018-12-12,Wed
> 
> it was decided to use the timestamp in v4l2_buffer for this.
> 
> However, struct timeval cannot be used in a compound control since
> the size of struct timeval differs between 32 and 64 bit architectures,
> and there are also changes upcoming for y2038 support.
> 
> But internally the kernel converts the timeval to a u64 (nsecs since
> boot). So we provide a helper function in videodev2.h that converts
> the timeval to a u64, and that u64 can be used inside compound controls.
> 
> In the not too distant future we want to create a new struct v4l2_buffer,
> and then we'll use u64 from the start, so in that case the helper function
> would no longer be needed.
> 
> The first three patches add a new m2m helper function to correctly copy
> the relevant data from an output buffer to a capture buffer. This will
> simplify m2m drivers (in fact, many m2m drivers do not do this quite
> right, so a helper function was really needed).
> 
> The fourth patch clears up messy timecode documentation that I came
> across while working on this.
> 
> Patch 5 adds the new v4l2_timeval_to_ns helper function to videodev2.h.
> The next patch adds the vb2_find_timestamp() function to find buffers
> with a specific u64 timestamp.
> 
> Finally the cedrus driver and documentation are updated to use a
> timestamp as buffer identifier.
> 
> 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.
> 
> Please note that this patch series will have to be updated one more
> time when pending 4.20 fixes are merged back into our master since
> those patches will move the cedrus mpeg controls to a different header.

I hit the same build issue that Jonas reported already. With the 
appropriate fix, this works fine with the cedrus driver.

So with the related fix included, this is:
Tested-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> Regards,
> 
>         Hans
> 
> Hans Verkuil (8):
>   v4l2-mem2mem: add v4l2_m2m_buf_copy_data helper function
>   vim2m: use v4l2_m2m_buf_copy_data
>   vicodec: use v4l2_m2m_buf_copy_data
>   buffer.rst: clean up timecode documentation
>   videodev2.h: add v4l2_timeval_to_ns inline function
>   vb2: add vb2_find_timestamp()
>   cedrus: identify buffers by timestamp
>   extended-controls.rst: update the mpeg2 compound controls
> 
>  Documentation/media/uapi/v4l/buffer.rst       | 11 ++++----
>  .../media/uapi/v4l/extended-controls.rst      | 28 +++++++++++--------
>  .../media/common/videobuf2/videobuf2-v4l2.c   | 22 +++++++++++++--
>  drivers/media/platform/vicodec/vicodec-core.c | 12 +-------
>  drivers/media/platform/vim2m.c                | 12 +-------
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  9 ------
>  drivers/media/v4l2-core/v4l2-mem2mem.c        | 20 +++++++++++++
>  drivers/staging/media/sunxi/cedrus/cedrus.h   |  9 ++++--
>  .../staging/media/sunxi/cedrus/cedrus_dec.c   |  2 ++
>  .../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 21 ++++++--------
>  include/media/v4l2-mem2mem.h                  | 20 +++++++++++++
>  include/media/videobuf2-v4l2.h                | 19 ++++++++++++-
>  include/uapi/linux/v4l2-controls.h            | 14 ++++------
>  include/uapi/linux/videodev2.h                | 12 ++++++++
>  14 files changed, 136 insertions(+), 75 deletions(-)
> 
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCHv5 6/8] vb2: add vb2_find_timestamp()

[Hans Verkuil]

On 12/18/18 3:21 PM, Paul Kocialkowski wrote:
> Hi,
> 
> On Thu, 2018-12-13 at 13:28 +0100, Hans Verkuil wrote:
>> On 12/12/18 7:28 PM, Jonas Karlman wrote:
>>> Hi Hans,
>>>
>>> Since this function only return DEQUEUED and DONE buffers,
>>> it cannot be used to find a capture buffer that is both used for
>>> frame output and is part of the frame reference list.
>>> E.g. a bottom field referencing a top field that is already
>>> part of the capture buffer being used for frame output.
>>> (top and bottom field is output in same buffer)
>>>
>>> Jernej Škrabec and me have worked around this issue in cedrus driver by
>>> first checking
>>> the tag/timestamp of the current buffer being used for output frame.
>>>
>>>
>>> // field pictures may reference current capture buffer and is not
>>> returned by vb2_find_tag
>>> if (v4l2_buf->tag == dpb->tag)
>>>     buf_idx = v4l2_buf->vb2_buf.index;
>>> else
>>>     buf_idx = vb2_find_tag(cap_q, dpb->tag, 0);
>>>
>>>
>>> What is the recommended way to handle such case?
>>
>> That is the right approach for this. Interesting corner case, I hadn't
>> considered that.
>>
>>> Could vb2_find_timestamp be extended to allow QUEUED buffers to be returned?
>>
>> No, because only the driver knows what the current buffer is.
>>
>> Buffers that are queued to the driver are in state ACTIVE. But there may be
>> multiple ACTIVE buffers and vb2 doesn't know which buffer is currently
>> being processed by the driver.
>>
>> So this will have to be checked by the driver itself.
> 
> Interesting corner case indeed, we hadn't considered the possibility of
> interlaced pictures refeering to the current capture buffer.
> 
> Hans, do you want to include that change in a future revision of this
> series or should that be a separate follow-up patch?
> 
> I'm fine with both options (and could definitely craft the change in
> the latter case).

If you can make a separate patch for this, then that would be great!

Regards,

	Hans

> 
> Cheers,
> 
> Paul
> 
>>> In our sample code we only keep at most one output, one capture buffer
>>> in queue
>>> and use buffer indices as tag/timestamp to simplify buffer handling.
>>> FFmpeg keeps track of buffers/frames referenced and a buffer will not be
>>> reused
>>> until the codec and display pipeline have released all references to it.
>>>
>>> Sample code having interlaced and multi-slice support using previous tag
>>> version of this patchset can be found at:
>>> https://github.com/jernejsk/LibreELEC.tv/blob/hw_dec_ffmpeg/projects/Allwinner/patches/linux/0025-H264-fixes.patch#L120-L124
>>> https://github.com/Kwiboo/FFmpeg/compare/4.0.3-Leia-Beta5...v4l2-request-hwaccel
>>>
>>> Regards,
>>> Jonas

Re: [PATCHv5 6/8] vb2: add vb2_find_timestamp()

[Tomasz Figa]

On Thu, Dec 13, 2018 at 9:28 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 12/12/18 7:28 PM, Jonas Karlman wrote:
> > Hi Hans,
> >
> > Since this function only return DEQUEUED and DONE buffers,
> > it cannot be used to find a capture buffer that is both used for
> > frame output and is part of the frame reference list.
> > E.g. a bottom field referencing a top field that is already
> > part of the capture buffer being used for frame output.
> > (top and bottom field is output in same buffer)
> >
> > Jernej Škrabec and me have worked around this issue in cedrus driver by
> > first checking
> > the tag/timestamp of the current buffer being used for output frame.
> >
> >
> > // field pictures may reference current capture buffer and is not
> > returned by vb2_find_tag
> > if (v4l2_buf->tag == dpb->tag)
> >     buf_idx = v4l2_buf->vb2_buf.index;
> > else
> >     buf_idx = vb2_find_tag(cap_q, dpb->tag, 0);
> >
> >
> > What is the recommended way to handle such case?
>
> That is the right approach for this. Interesting corner case, I hadn't
> considered that.
>
> > Could vb2_find_timestamp be extended to allow QUEUED buffers to be returned?
>
> No, because only the driver knows what the current buffer is.
>
> Buffers that are queued to the driver are in state ACTIVE. But there may be
> multiple ACTIVE buffers and vb2 doesn't know which buffer is currently
> being processed by the driver.
>
> So this will have to be checked by the driver itself.

Hold on, it's a perfectly valid use case to have the buffer queued but
still used as a reference for previously queued buffers, e.g.

QBUF(O, 0)
QBUF(C, 0)
REF(ref0, out_timestamp(0))
QBUF(O, 1)
QBUF(C, 1)
REF(ref0, out_timestamp(0))
QBUF(O, 2)
QBUF(C, 2)
<- driver returns O(0) and C(0) here
<- userspace also knows that any next frame will not reference C(0) anymore
REF(ref0, out_timestamp(2))
QBUF(O, 0)
QBUF(C, 0)
<- driver may pick O(1)+C(1) or O(2)+C(2) to decode here, but C(0)
which is the reference for it is already QUEUED.

It's a perfectly fine scenario and optimal from pipelining point of
view, but if I'm not missing something, the current patch wouldn't
allow it.

Best regards,
Tomasz

Re: [PATCHv5 6/8] vb2: add vb2_find_timestamp()

[Jonas Karlman]

On 2018-12-19 06:10, Tomasz Figa wrote:
> On Thu, Dec 13, 2018 at 9:28 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>> On 12/12/18 7:28 PM, Jonas Karlman wrote:
>>> Hi Hans,
>>>
>>> Since this function only return DEQUEUED and DONE buffers,
>>> it cannot be used to find a capture buffer that is both used for
>>> frame output and is part of the frame reference list.
>>> E.g. a bottom field referencing a top field that is already
>>> part of the capture buffer being used for frame output.
>>> (top and bottom field is output in same buffer)
>>>
>>> Jernej Škrabec and me have worked around this issue in cedrus driver by
>>> first checking
>>> the tag/timestamp of the current buffer being used for output frame.
>>>
>>>
>>> // field pictures may reference current capture buffer and is not
>>> returned by vb2_find_tag
>>> if (v4l2_buf->tag == dpb->tag)
>>>     buf_idx = v4l2_buf->vb2_buf.index;
>>> else
>>>     buf_idx = vb2_find_tag(cap_q, dpb->tag, 0);
>>>
>>>
>>> What is the recommended way to handle such case?
>> That is the right approach for this. Interesting corner case, I hadn't
>> considered that.
>>
>>> Could vb2_find_timestamp be extended to allow QUEUED buffers to be returned?
>> No, because only the driver knows what the current buffer is.
>>
>> Buffers that are queued to the driver are in state ACTIVE. But there may be
>> multiple ACTIVE buffers and vb2 doesn't know which buffer is currently
>> being processed by the driver.
>>
>> So this will have to be checked by the driver itself.
> Hold on, it's a perfectly valid use case to have the buffer queued but
> still used as a reference for previously queued buffers, e.g.
>
> QBUF(O, 0)
> QBUF(C, 0)
> REF(ref0, out_timestamp(0))
> QBUF(O, 1)
> QBUF(C, 1)
> REF(ref0, out_timestamp(0))
> QBUF(O, 2)
> QBUF(C, 2)
> <- driver returns O(0) and C(0) here
> <- userspace also knows that any next frame will not reference C(0) anymore
> REF(ref0, out_timestamp(2))
> QBUF(O, 0)
> QBUF(C, 0)
> <- driver may pick O(1)+C(1) or O(2)+C(2) to decode here, but C(0)
> which is the reference for it is already QUEUED.
>
> It's a perfectly fine scenario and optimal from pipelining point of
> view, but if I'm not missing something, the current patch wouldn't
> allow it.

This scenario should never happen with FFmpeg + v4l2request hwaccel +
Kodi userspace.
FFmpeg would only QBUF O(0)+C(0) again after it has been presented on
screen and Kodi have released the last reference to the AVFrame.

The v4l2request hwaccel will keep a AVFrame pool with preallocated
frames, AVFrame(x) is keeping userspace ref to O(x)+C(x).
An AVFrame will not be released back to the pool until FFmpeg have
removed it from DPB and Kodi have released it after it no longer is
being presented on screen.

E.g. an IPBIPB sequense with display order 0 2 1 3 5 4

FFmpeg: AVFrame(0)
QBUF: O(0)+C(0)
DQBUF: O(0)+C(0)
Kodi: AVFrame(0) returned from FFmpeg and presented on screen
FFmpeg: AVFrame(1) with ref to AVFrame(0)
QBUF: O(1)+C(1) with ref to timestamp(0)
DQBUF: O(1)+C(1)
FFmpeg: AVFrame(2) with ref to AVFrame(0)+AVFrame(1)
QBUF: O(2)+C(2) with ref to timestamp(0)+timestamp(1)
DQBUF: O(2)+C(2)
Kodi: AVFrame(2) returned from FFmpeg and presented on screen
Kodi: AVFrame(0) released (no longer presented)
FFmpeg: AVFrame(3)
QBUF: O(3)+C(3)
DQBUF: O(3)+C(3)
Kodi: AVFrame(1) returned from FFmpeg and presented on screen
Kodi: AVFrame(2) released (no longer presented)
FFmpeg: AVFrame(2) returned to pool
FFmpeg: AVFrame(2) with ref to AVFrame(3)
QBUF: O(2)+C(2) with ref to timestamp(3)
DQBUF: O(2)+C(2)
Kodi: AVFrame(3) returned from FFmpeg and presented on screen
Kodi: AVFrame(1) released (no longer presented)
FFmpeg: AVFrame(0)+AVFrame(1) returned to pool (no longer referenced)
FFmpeg: AVFrame(0) with ref to AVFrame(3)+AVFrame(2)
QBUF: O(0)+C(0) with ref to timestamp(3)+timestamp(2)
DQBUF: O(0)+C(0)
Kodi: AVFrame(0) returned from FFmpeg and presented on screen
Kodi: AVFrame(3) released (no longer presented)
and so on

Here we can see that O(0)+C(0) will not be QBUF until after FFmpeg +
Kodi have released all userspace refs to AVFrame(0).
Above example was simplified, Kodi will normally keep a few decoded
frames in buffer before being presented and FFmpeg will CREATE_BUF
anytime the pool is empty and new O/C buffers is needed.

Regards,
Jonas

> Best regards,
> Tomasz

Re: [PATCHv5 6/8] vb2: add vb2_find_timestamp()

[Tomasz Figa]

On Wed, Dec 19, 2018 at 4:04 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2018-12-19 06:10, Tomasz Figa wrote:
> > On Thu, Dec 13, 2018 at 9:28 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >> On 12/12/18 7:28 PM, Jonas Karlman wrote:
> >>> Hi Hans,
> >>>
> >>> Since this function only return DEQUEUED and DONE buffers,
> >>> it cannot be used to find a capture buffer that is both used for
> >>> frame output and is part of the frame reference list.
> >>> E.g. a bottom field referencing a top field that is already
> >>> part of the capture buffer being used for frame output.
> >>> (top and bottom field is output in same buffer)
> >>>
> >>> Jernej Škrabec and me have worked around this issue in cedrus driver by
> >>> first checking
> >>> the tag/timestamp of the current buffer being used for output frame.
> >>>
> >>>
> >>> // field pictures may reference current capture buffer and is not
> >>> returned by vb2_find_tag
> >>> if (v4l2_buf->tag == dpb->tag)
> >>>     buf_idx = v4l2_buf->vb2_buf.index;
> >>> else
> >>>     buf_idx = vb2_find_tag(cap_q, dpb->tag, 0);
> >>>
> >>>
> >>> What is the recommended way to handle such case?
> >> That is the right approach for this. Interesting corner case, I hadn't
> >> considered that.
> >>
> >>> Could vb2_find_timestamp be extended to allow QUEUED buffers to be returned?
> >> No, because only the driver knows what the current buffer is.
> >>
> >> Buffers that are queued to the driver are in state ACTIVE. But there may be
> >> multiple ACTIVE buffers and vb2 doesn't know which buffer is currently
> >> being processed by the driver.
> >>
> >> So this will have to be checked by the driver itself.
> > Hold on, it's a perfectly valid use case to have the buffer queued but
> > still used as a reference for previously queued buffers, e.g.
> >
> > QBUF(O, 0)
> > QBUF(C, 0)
> > REF(ref0, out_timestamp(0))
> > QBUF(O, 1)
> > QBUF(C, 1)
> > REF(ref0, out_timestamp(0))
> > QBUF(O, 2)
> > QBUF(C, 2)
> > <- driver returns O(0) and C(0) here
> > <- userspace also knows that any next frame will not reference C(0) anymore
> > REF(ref0, out_timestamp(2))
> > QBUF(O, 0)
> > QBUF(C, 0)
> > <- driver may pick O(1)+C(1) or O(2)+C(2) to decode here, but C(0)
> > which is the reference for it is already QUEUED.
> >
> > It's a perfectly fine scenario and optimal from pipelining point of
> > view, but if I'm not missing something, the current patch wouldn't
> > allow it.
>
> This scenario should never happen with FFmpeg + v4l2request hwaccel +
> Kodi userspace.
> FFmpeg would only QBUF O(0)+C(0) again after it has been presented on
> screen and Kodi have released the last reference to the AVFrame.
>

I skipped the display in the example indeed, but we can easily add it:

QBUF(O, 0)
QBUF(C, 0)
REF(ref0, out_timestamp(0))
QBUF(O, 1)
QBUF(C, 1)
<- driver returns O(0) and C(0) here
<- userspace displays C(0)
REF(ref0, out_timestamp(0))
QBUF(O, 2)
QBUF(C, 2)
REF(ref0, out_timestamp(0))
QBUF(O, 3)
QBUF(C, 3)
<- driver returns O(1) and C(1) here
<- userspace displays C(1) and reclaims C(0)
<- userspace also knows that any next frame will not reference C(0) anymore
REF(ref0, out_timestamp(3))
QBUF(O, 0)
QBUF(C, 0)
<- driver may pick O(3)+C(3) to decode here, but C(0)
which is the reference for it is already QUEUED.

Also the fact that FFmpeg wouldn't trigger such case doesn't mean that
it's an invalid one. If I remember correctly, Chromium would actually
trigger such, since we attempt to pipeline things as much as possible.

> The v4l2request hwaccel will keep a AVFrame pool with preallocated
> frames, AVFrame(x) is keeping userspace ref to O(x)+C(x).
> An AVFrame will not be released back to the pool until FFmpeg have
> removed it from DPB and Kodi have released it after it no longer is
> being presented on screen.
>
> E.g. an IPBIPB sequense with display order 0 2 1 3 5 4
>
> FFmpeg: AVFrame(0)
> QBUF: O(0)+C(0)
> DQBUF: O(0)+C(0)
> Kodi: AVFrame(0) returned from FFmpeg and presented on screen
> FFmpeg: AVFrame(1) with ref to AVFrame(0)
> QBUF: O(1)+C(1) with ref to timestamp(0)
> DQBUF: O(1)+C(1)
> FFmpeg: AVFrame(2) with ref to AVFrame(0)+AVFrame(1)
> QBUF: O(2)+C(2) with ref to timestamp(0)+timestamp(1)
> DQBUF: O(2)+C(2)
> Kodi: AVFrame(2) returned from FFmpeg and presented on screen
> Kodi: AVFrame(0) released (no longer presented)
> FFmpeg: AVFrame(3)
> QBUF: O(3)+C(3)
> DQBUF: O(3)+C(3)
> Kodi: AVFrame(1) returned from FFmpeg and presented on screen
> Kodi: AVFrame(2) released (no longer presented)
> FFmpeg: AVFrame(2) returned to pool
> FFmpeg: AVFrame(2) with ref to AVFrame(3)
> QBUF: O(2)+C(2) with ref to timestamp(3)
> DQBUF: O(2)+C(2)
> Kodi: AVFrame(3) returned from FFmpeg and presented on screen
> Kodi: AVFrame(1) released (no longer presented)
> FFmpeg: AVFrame(0)+AVFrame(1) returned to pool (no longer referenced)
> FFmpeg: AVFrame(0) with ref to AVFrame(3)+AVFrame(2)
> QBUF: O(0)+C(0) with ref to timestamp(3)+timestamp(2)
> DQBUF: O(0)+C(0)
> Kodi: AVFrame(0) returned from FFmpeg and presented on screen
> Kodi: AVFrame(3) released (no longer presented)
> and so on
>
> Here we can see that O(0)+C(0) will not be QBUF until after FFmpeg +
> Kodi have released all userspace refs to AVFrame(0).
> Above example was simplified, Kodi will normally keep a few decoded
> frames in buffer before being presented and FFmpeg will CREATE_BUF
> anytime the pool is empty and new O/C buffers is needed.
>
> Regards,
> Jonas
>
> > Best regards,
> > Tomasz

Re: [PATCHv5 6/8] vb2: add vb2_find_timestamp()

[Jonas Karlman]

On 2018-12-19 08:16, Tomasz Figa wrote:
> On Wed, Dec 19, 2018 at 4:04 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>> On 2018-12-19 06:10, Tomasz Figa wrote:
>>> On Thu, Dec 13, 2018 at 9:28 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>> On 12/12/18 7:28 PM, Jonas Karlman wrote:
>>>>> Hi Hans,
>>>>>
>>>>> Since this function only return DEQUEUED and DONE buffers,
>>>>> it cannot be used to find a capture buffer that is both used for
>>>>> frame output and is part of the frame reference list.
>>>>> E.g. a bottom field referencing a top field that is already
>>>>> part of the capture buffer being used for frame output.
>>>>> (top and bottom field is output in same buffer)
>>>>>
>>>>> Jernej Škrabec and me have worked around this issue in cedrus driver by
>>>>> first checking
>>>>> the tag/timestamp of the current buffer being used for output frame.
>>>>>
>>>>>
>>>>> // field pictures may reference current capture buffer and is not
>>>>> returned by vb2_find_tag
>>>>> if (v4l2_buf->tag == dpb->tag)
>>>>>     buf_idx = v4l2_buf->vb2_buf.index;
>>>>> else
>>>>>     buf_idx = vb2_find_tag(cap_q, dpb->tag, 0);
>>>>>
>>>>>
>>>>> What is the recommended way to handle such case?
>>>> That is the right approach for this. Interesting corner case, I hadn't
>>>> considered that.
>>>>
>>>>> Could vb2_find_timestamp be extended to allow QUEUED buffers to be returned?
>>>> No, because only the driver knows what the current buffer is.
>>>>
>>>> Buffers that are queued to the driver are in state ACTIVE. But there may be
>>>> multiple ACTIVE buffers and vb2 doesn't know which buffer is currently
>>>> being processed by the driver.
>>>>
>>>> So this will have to be checked by the driver itself.
>>> Hold on, it's a perfectly valid use case to have the buffer queued but
>>> still used as a reference for previously queued buffers, e.g.
>>>
>>> QBUF(O, 0)
>>> QBUF(C, 0)
>>> REF(ref0, out_timestamp(0))
>>> QBUF(O, 1)
>>> QBUF(C, 1)
>>> REF(ref0, out_timestamp(0))
>>> QBUF(O, 2)
>>> QBUF(C, 2)
>>> <- driver returns O(0) and C(0) here
>>> <- userspace also knows that any next frame will not reference C(0) anymore
>>> REF(ref0, out_timestamp(2))
>>> QBUF(O, 0)
>>> QBUF(C, 0)
>>> <- driver may pick O(1)+C(1) or O(2)+C(2) to decode here, but C(0)
>>> which is the reference for it is already QUEUED.
>>>
>>> It's a perfectly fine scenario and optimal from pipelining point of
>>> view, but if I'm not missing something, the current patch wouldn't
>>> allow it.
>> This scenario should never happen with FFmpeg + v4l2request hwaccel +
>> Kodi userspace.
>> FFmpeg would only QBUF O(0)+C(0) again after it has been presented on
>> screen and Kodi have released the last reference to the AVFrame.
>>
> I skipped the display in the example indeed, but we can easily add it:
>
> QBUF(O, 0)
> QBUF(C, 0)
> REF(ref0, out_timestamp(0))
> QBUF(O, 1)
> QBUF(C, 1)
> <- driver returns O(0) and C(0) here
> <- userspace displays C(0)
> REF(ref0, out_timestamp(0))
> QBUF(O, 2)
> QBUF(C, 2)
> REF(ref0, out_timestamp(0))
> QBUF(O, 3)
> QBUF(C, 3)
> <- driver returns O(1) and C(1) here
> <- userspace displays C(1) and reclaims C(0)
> <- userspace also knows that any next frame will not reference C(0) anymore
> REF(ref0, out_timestamp(3))
> QBUF(O, 0)
> QBUF(C, 0)
> <- driver may pick O(3)+C(3) to decode here, but C(0)
> which is the reference for it is already QUEUED.
>
> Also the fact that FFmpeg wouldn't trigger such case doesn't mean that
> it's an invalid one. If I remember correctly, Chromium would actually
> trigger such, since we attempt to pipeline things as much as possible.
I still think this may be an invalid use-case or otherwise bad handling
from userspace. Since userspace knows that C(0) won't be referenced in
next frame it should also know that it still has two frames queued for
decoding that references C(0) frame and still have not been returned
from decoder.
And if the driver have not started decoding the requests/frames that
reference C(0) how would it know that it cannot pick the now queued C(0)
as output for next frame it decodes?
In FFmpeg case we only re-queue the buffer once all frames that
references that buffer/frame have been output from decoder, I guess the
main difference is that FFmpeg currently only keep one request in flight
at the time (wait on request to finish before queue next), this may
change in future as we continue to improve the v4l2request hwaccel.

Regards,
Jonas
>
>> The v4l2request hwaccel will keep a AVFrame pool with preallocated
>> frames, AVFrame(x) is keeping userspace ref to O(x)+C(x).
>> An AVFrame will not be released back to the pool until FFmpeg have
>> removed it from DPB and Kodi have released it after it no longer is
>> being presented on screen.
>>
>> E.g. an IPBIPB sequense with display order 0 2 1 3 5 4
>>
>> FFmpeg: AVFrame(0)
>> QBUF: O(0)+C(0)
>> DQBUF: O(0)+C(0)
>> Kodi: AVFrame(0) returned from FFmpeg and presented on screen
>> FFmpeg: AVFrame(1) with ref to AVFrame(0)
>> QBUF: O(1)+C(1) with ref to timestamp(0)
>> DQBUF: O(1)+C(1)
>> FFmpeg: AVFrame(2) with ref to AVFrame(0)+AVFrame(1)
>> QBUF: O(2)+C(2) with ref to timestamp(0)+timestamp(1)
>> DQBUF: O(2)+C(2)
>> Kodi: AVFrame(2) returned from FFmpeg and presented on screen
>> Kodi: AVFrame(0) released (no longer presented)
>> FFmpeg: AVFrame(3)
>> QBUF: O(3)+C(3)
>> DQBUF: O(3)+C(3)
>> Kodi: AVFrame(1) returned from FFmpeg and presented on screen
>> Kodi: AVFrame(2) released (no longer presented)
>> FFmpeg: AVFrame(2) returned to pool
>> FFmpeg: AVFrame(2) with ref to AVFrame(3)
>> QBUF: O(2)+C(2) with ref to timestamp(3)
>> DQBUF: O(2)+C(2)
>> Kodi: AVFrame(3) returned from FFmpeg and presented on screen
>> Kodi: AVFrame(1) released (no longer presented)
>> FFmpeg: AVFrame(0)+AVFrame(1) returned to pool (no longer referenced)
>> FFmpeg: AVFrame(0) with ref to AVFrame(3)+AVFrame(2)
>> QBUF: O(0)+C(0) with ref to timestamp(3)+timestamp(2)
>> DQBUF: O(0)+C(0)
>> Kodi: AVFrame(0) returned from FFmpeg and presented on screen
>> Kodi: AVFrame(3) released (no longer presented)
>> and so on
>>
>> Here we can see that O(0)+C(0) will not be QBUF until after FFmpeg +
>> Kodi have released all userspace refs to AVFrame(0).
>> Above example was simplified, Kodi will normally keep a few decoded
>> frames in buffer before being presented and FFmpeg will CREATE_BUF
>> anytime the pool is empty and new O/C buffers is needed.
>>
>> Regards,
>> Jonas
>>
>>> Best regards,
>>> Tomasz

Re: [PATCHv5 6/8] vb2: add vb2_find_timestamp()

[Tomasz Figa]

On Wed, Dec 19, 2018 at 6:18 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> On 2018-12-19 08:16, Tomasz Figa wrote:
> > On Wed, Dec 19, 2018 at 4:04 PM Jonas Karlman <jonas@kwiboo.se> wrote:
> >> On 2018-12-19 06:10, Tomasz Figa wrote:
> >>> On Thu, Dec 13, 2018 at 9:28 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >>>> On 12/12/18 7:28 PM, Jonas Karlman wrote:
> >>>>> Hi Hans,
> >>>>>
> >>>>> Since this function only return DEQUEUED and DONE buffers,
> >>>>> it cannot be used to find a capture buffer that is both used for
> >>>>> frame output and is part of the frame reference list.
> >>>>> E.g. a bottom field referencing a top field that is already
> >>>>> part of the capture buffer being used for frame output.
> >>>>> (top and bottom field is output in same buffer)
> >>>>>
> >>>>> Jernej Škrabec and me have worked around this issue in cedrus driver by
> >>>>> first checking
> >>>>> the tag/timestamp of the current buffer being used for output frame.
> >>>>>
> >>>>>
> >>>>> // field pictures may reference current capture buffer and is not
> >>>>> returned by vb2_find_tag
> >>>>> if (v4l2_buf->tag == dpb->tag)
> >>>>>     buf_idx = v4l2_buf->vb2_buf.index;
> >>>>> else
> >>>>>     buf_idx = vb2_find_tag(cap_q, dpb->tag, 0);
> >>>>>
> >>>>>
> >>>>> What is the recommended way to handle such case?
> >>>> That is the right approach for this. Interesting corner case, I hadn't
> >>>> considered that.
> >>>>
> >>>>> Could vb2_find_timestamp be extended to allow QUEUED buffers to be returned?
> >>>> No, because only the driver knows what the current buffer is.
> >>>>
> >>>> Buffers that are queued to the driver are in state ACTIVE. But there may be
> >>>> multiple ACTIVE buffers and vb2 doesn't know which buffer is currently
> >>>> being processed by the driver.
> >>>>
> >>>> So this will have to be checked by the driver itself.
> >>> Hold on, it's a perfectly valid use case to have the buffer queued but
> >>> still used as a reference for previously queued buffers, e.g.
> >>>
> >>> QBUF(O, 0)
> >>> QBUF(C, 0)
> >>> REF(ref0, out_timestamp(0))
> >>> QBUF(O, 1)
> >>> QBUF(C, 1)
> >>> REF(ref0, out_timestamp(0))
> >>> QBUF(O, 2)
> >>> QBUF(C, 2)
> >>> <- driver returns O(0) and C(0) here
> >>> <- userspace also knows that any next frame will not reference C(0) anymore
> >>> REF(ref0, out_timestamp(2))
> >>> QBUF(O, 0)
> >>> QBUF(C, 0)
> >>> <- driver may pick O(1)+C(1) or O(2)+C(2) to decode here, but C(0)
> >>> which is the reference for it is already QUEUED.
> >>>
> >>> It's a perfectly fine scenario and optimal from pipelining point of
> >>> view, but if I'm not missing something, the current patch wouldn't
> >>> allow it.
> >> This scenario should never happen with FFmpeg + v4l2request hwaccel +
> >> Kodi userspace.
> >> FFmpeg would only QBUF O(0)+C(0) again after it has been presented on
> >> screen and Kodi have released the last reference to the AVFrame.
> >>
> > I skipped the display in the example indeed, but we can easily add it:
> >
> > QBUF(O, 0)
> > QBUF(C, 0)
> > REF(ref0, out_timestamp(0))
> > QBUF(O, 1)
> > QBUF(C, 1)
> > <- driver returns O(0) and C(0) here
> > <- userspace displays C(0)
> > REF(ref0, out_timestamp(0))
> > QBUF(O, 2)
> > QBUF(C, 2)
> > REF(ref0, out_timestamp(0))
> > QBUF(O, 3)
> > QBUF(C, 3)
> > <- driver returns O(1) and C(1) here
> > <- userspace displays C(1) and reclaims C(0)
> > <- userspace also knows that any next frame will not reference C(0) anymore
> > REF(ref0, out_timestamp(3))
> > QBUF(O, 0)
> > QBUF(C, 0)
> > <- driver may pick O(3)+C(3) to decode here, but C(0)
> > which is the reference for it is already QUEUED.
> >
> > Also the fact that FFmpeg wouldn't trigger such case doesn't mean that
> > it's an invalid one. If I remember correctly, Chromium would actually
> > trigger such, since we attempt to pipeline things as much as possible.
> I still think this may be an invalid use-case or otherwise bad handling
> from userspace. Since userspace knows that C(0) won't be referenced in
> next frame it should also know that it still has two frames queued for
> decoding that references C(0) frame and still have not been returned
> from decoder.
> And if the driver have not started decoding the requests/frames that
> reference C(0) how would it know that it cannot pick the now queued C(0)
> as output for next frame it decodes?

Because the application has queued 2 other CAPTURE buffers before
C(0), but indeed, if we assume that the driver can skip buffers (due
to some errors that I don't see how could happen in any real life
scenario), then that could fail.

> In FFmpeg case we only re-queue the buffer once all frames that
> references that buffer/frame have been output from decoder, I guess the
> main difference is that FFmpeg currently only keep one request in flight
> at the time (wait on request to finish before queue next), this may
> change in future as we continue to improve the v4l2request hwaccel.
>
> Regards,
> Jonas
> >
> >> The v4l2request hwaccel will keep a AVFrame pool with preallocated
> >> frames, AVFrame(x) is keeping userspace ref to O(x)+C(x).
> >> An AVFrame will not be released back to the pool until FFmpeg have
> >> removed it from DPB and Kodi have released it after it no longer is
> >> being presented on screen.
> >>
> >> E.g. an IPBIPB sequense with display order 0 2 1 3 5 4
> >>
> >> FFmpeg: AVFrame(0)
> >> QBUF: O(0)+C(0)
> >> DQBUF: O(0)+C(0)
> >> Kodi: AVFrame(0) returned from FFmpeg and presented on screen
> >> FFmpeg: AVFrame(1) with ref to AVFrame(0)
> >> QBUF: O(1)+C(1) with ref to timestamp(0)
> >> DQBUF: O(1)+C(1)
> >> FFmpeg: AVFrame(2) with ref to AVFrame(0)+AVFrame(1)
> >> QBUF: O(2)+C(2) with ref to timestamp(0)+timestamp(1)
> >> DQBUF: O(2)+C(2)
> >> Kodi: AVFrame(2) returned from FFmpeg and presented on screen
> >> Kodi: AVFrame(0) released (no longer presented)
> >> FFmpeg: AVFrame(3)
> >> QBUF: O(3)+C(3)
> >> DQBUF: O(3)+C(3)
> >> Kodi: AVFrame(1) returned from FFmpeg and presented on screen
> >> Kodi: AVFrame(2) released (no longer presented)
> >> FFmpeg: AVFrame(2) returned to pool
> >> FFmpeg: AVFrame(2) with ref to AVFrame(3)
> >> QBUF: O(2)+C(2) with ref to timestamp(3)
> >> DQBUF: O(2)+C(2)
> >> Kodi: AVFrame(3) returned from FFmpeg and presented on screen
> >> Kodi: AVFrame(1) released (no longer presented)
> >> FFmpeg: AVFrame(0)+AVFrame(1) returned to pool (no longer referenced)
> >> FFmpeg: AVFrame(0) with ref to AVFrame(3)+AVFrame(2)
> >> QBUF: O(0)+C(0) with ref to timestamp(3)+timestamp(2)
> >> DQBUF: O(0)+C(0)
> >> Kodi: AVFrame(0) returned from FFmpeg and presented on screen
> >> Kodi: AVFrame(3) released (no longer presented)
> >> and so on
> >>
> >> Here we can see that O(0)+C(0) will not be QBUF until after FFmpeg +
> >> Kodi have released all userspace refs to AVFrame(0).
> >> Above example was simplified, Kodi will normally keep a few decoded
> >> frames in buffer before being presented and FFmpeg will CREATE_BUF
> >> anytime the pool is empty and new O/C buffers is needed.
> >>
> >> Regards,
> >> Jonas
> >>
> >>> Best regards,
> >>> Tomasz

Re: [PATCHv5 6/8] vb2: add vb2_find_timestamp()

[Hans Verkuil]

On 12/19/2018 08:16 AM, Tomasz Figa wrote:
> On Wed, Dec 19, 2018 at 4:04 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>>
>> On 2018-12-19 06:10, Tomasz Figa wrote:
>>> On Thu, Dec 13, 2018 at 9:28 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>>> On 12/12/18 7:28 PM, Jonas Karlman wrote:
>>>>> Hi Hans,
>>>>>
>>>>> Since this function only return DEQUEUED and DONE buffers,
>>>>> it cannot be used to find a capture buffer that is both used for
>>>>> frame output and is part of the frame reference list.
>>>>> E.g. a bottom field referencing a top field that is already
>>>>> part of the capture buffer being used for frame output.
>>>>> (top and bottom field is output in same buffer)
>>>>>
>>>>> Jernej Škrabec and me have worked around this issue in cedrus driver by
>>>>> first checking
>>>>> the tag/timestamp of the current buffer being used for output frame.
>>>>>
>>>>>
>>>>> // field pictures may reference current capture buffer and is not
>>>>> returned by vb2_find_tag
>>>>> if (v4l2_buf->tag == dpb->tag)
>>>>>     buf_idx = v4l2_buf->vb2_buf.index;
>>>>> else
>>>>>     buf_idx = vb2_find_tag(cap_q, dpb->tag, 0);
>>>>>
>>>>>
>>>>> What is the recommended way to handle such case?
>>>> That is the right approach for this. Interesting corner case, I hadn't
>>>> considered that.
>>>>
>>>>> Could vb2_find_timestamp be extended to allow QUEUED buffers to be returned?
>>>> No, because only the driver knows what the current buffer is.
>>>>
>>>> Buffers that are queued to the driver are in state ACTIVE. But there may be
>>>> multiple ACTIVE buffers and vb2 doesn't know which buffer is currently
>>>> being processed by the driver.
>>>>
>>>> So this will have to be checked by the driver itself.
>>> Hold on, it's a perfectly valid use case to have the buffer queued but
>>> still used as a reference for previously queued buffers, e.g.
>>>
>>> QBUF(O, 0)
>>> QBUF(C, 0)
>>> REF(ref0, out_timestamp(0))
>>> QBUF(O, 1)
>>> QBUF(C, 1)
>>> REF(ref0, out_timestamp(0))
>>> QBUF(O, 2)
>>> QBUF(C, 2)
>>> <- driver returns O(0) and C(0) here
>>> <- userspace also knows that any next frame will not reference C(0) anymore
>>> REF(ref0, out_timestamp(2))
>>> QBUF(O, 0)
>>> QBUF(C, 0)
>>> <- driver may pick O(1)+C(1) or O(2)+C(2) to decode here, but C(0)
>>> which is the reference for it is already QUEUED.
>>>
>>> It's a perfectly fine scenario and optimal from pipelining point of
>>> view, but if I'm not missing something, the current patch wouldn't
>>> allow it.
>>
>> This scenario should never happen with FFmpeg + v4l2request hwaccel +
>> Kodi userspace.
>> FFmpeg would only QBUF O(0)+C(0) again after it has been presented on
>> screen and Kodi have released the last reference to the AVFrame.
>>
> 
> I skipped the display in the example indeed, but we can easily add it:
> 
> QBUF(O, 0)
> QBUF(C, 0)
> REF(ref0, out_timestamp(0))
> QBUF(O, 1)
> QBUF(C, 1)
> <- driver returns O(0) and C(0) here
> <- userspace displays C(0)
> REF(ref0, out_timestamp(0))
> QBUF(O, 2)
> QBUF(C, 2)
> REF(ref0, out_timestamp(0))
> QBUF(O, 3)
> QBUF(C, 3)
> <- driver returns O(1) and C(1) here
> <- userspace displays C(1) and reclaims C(0)
> <- userspace also knows that any next frame will not reference C(0) anymore
> REF(ref0, out_timestamp(3))
> QBUF(O, 0)
> QBUF(C, 0)

When C(0) is queued its timestamp field is zeroed by vb2. So it can no
longer be used as a reference.

For now I want to keep the behavior like that (i.e. once you requeue a
capture buffer it can no longer be used as a reference frame for the decoder).

This limitation is something we might want to lift in the future, but
this would require more work internally.

Regards,

	Hans

> <- driver may pick O(3)+C(3) to decode here, but C(0)
> which is the reference for it is already QUEUED.
> 
> Also the fact that FFmpeg wouldn't trigger such case doesn't mean that
> it's an invalid one. If I remember correctly, Chromium would actually
> trigger such, since we attempt to pipeline things as much as possible.
> 
>> The v4l2request hwaccel will keep a AVFrame pool with preallocated
>> frames, AVFrame(x) is keeping userspace ref to O(x)+C(x).
>> An AVFrame will not be released back to the pool until FFmpeg have
>> removed it from DPB and Kodi have released it after it no longer is
>> being presented on screen.
>>
>> E.g. an IPBIPB sequense with display order 0 2 1 3 5 4
>>
>> FFmpeg: AVFrame(0)
>> QBUF: O(0)+C(0)
>> DQBUF: O(0)+C(0)
>> Kodi: AVFrame(0) returned from FFmpeg and presented on screen
>> FFmpeg: AVFrame(1) with ref to AVFrame(0)
>> QBUF: O(1)+C(1) with ref to timestamp(0)
>> DQBUF: O(1)+C(1)
>> FFmpeg: AVFrame(2) with ref to AVFrame(0)+AVFrame(1)
>> QBUF: O(2)+C(2) with ref to timestamp(0)+timestamp(1)
>> DQBUF: O(2)+C(2)
>> Kodi: AVFrame(2) returned from FFmpeg and presented on screen
>> Kodi: AVFrame(0) released (no longer presented)
>> FFmpeg: AVFrame(3)
>> QBUF: O(3)+C(3)
>> DQBUF: O(3)+C(3)
>> Kodi: AVFrame(1) returned from FFmpeg and presented on screen
>> Kodi: AVFrame(2) released (no longer presented)
>> FFmpeg: AVFrame(2) returned to pool
>> FFmpeg: AVFrame(2) with ref to AVFrame(3)
>> QBUF: O(2)+C(2) with ref to timestamp(3)
>> DQBUF: O(2)+C(2)
>> Kodi: AVFrame(3) returned from FFmpeg and presented on screen
>> Kodi: AVFrame(1) released (no longer presented)
>> FFmpeg: AVFrame(0)+AVFrame(1) returned to pool (no longer referenced)
>> FFmpeg: AVFrame(0) with ref to AVFrame(3)+AVFrame(2)
>> QBUF: O(0)+C(0) with ref to timestamp(3)+timestamp(2)
>> DQBUF: O(0)+C(0)
>> Kodi: AVFrame(0) returned from FFmpeg and presented on screen
>> Kodi: AVFrame(3) released (no longer presented)
>> and so on
>>
>> Here we can see that O(0)+C(0) will not be QBUF until after FFmpeg +
>> Kodi have released all userspace refs to AVFrame(0).
>> Above example was simplified, Kodi will normally keep a few decoded
>> frames in buffer before being presented and FFmpeg will CREATE_BUF
>> anytime the pool is empty and new O/C buffers is needed.
>>
>> Regards,
>> Jonas
>>
>>> Best regards,
>>> Tomasz