[PATCHv2 0/3] Improve vb2_find_buffer

[PATCHv2 0/3] Improve vb2_find_buffer

[hverkuil-cisco]

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

This supersedes my previous patch:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg144099.html

Rather than marking a timestamp of 0 as 'special', I add a bitfield
to keep track of the validity of the timestamp.

The last patch also adds extra checks to verify that the found buffer
matches what is expected w.r.t. the number of planes and the size of
the planes.

I still think vb2_find_timestamp should increase some refcount for
the buffer since it is possible that the application requeues the
found buffer with a different dmabuf fd, thus potentially freeing the
buffer memory while it is still being processed.

But to be honest, I'm not sure how that should be done.

Regards,

	Hans

Hans Verkuil (3):
  vb2: replace bool by bitfield in vb2_buffer
  vb2: keep track of timestamp status
  vb2: add 'match' arg to vb2_find_buffer()

 drivers/media/common/videobuf2/videobuf2-core.c  | 15 +++++++++------
 drivers/media/common/videobuf2/videobuf2-v4l2.c  | 16 +++++++++++++---
 drivers/media/v4l2-core/v4l2-mem2mem.c           |  1 +
 .../staging/media/sunxi/cedrus/cedrus_mpeg2.c    |  8 ++++----
 include/media/videobuf2-core.h                   |  7 +++++--
 include/media/videobuf2-v4l2.h                   |  3 ++-
 6 files changed, 34 insertions(+), 16 deletions(-)

-- 
2.20.1

[PATCHv2 1/3] vb2: replace bool by bitfield in vb2_buffer

[hverkuil-cisco]

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

The bool type is not recommended for use in structs, so replace these
by bitfields.

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

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index e07b6bdb6982..35cf36686e20 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -934,7 +934,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 		/* sync buffers */
 		for (plane = 0; plane < vb->num_planes; ++plane)
 			call_void_memop(vb, finish, vb->planes[plane].mem_priv);
-		vb->synced = false;
+		vb->synced = 0;
 	}
 
 	spin_lock_irqsave(&q->done_lock, flags);
@@ -1313,8 +1313,8 @@ static int __buf_prepare(struct vb2_buffer *vb)
 	for (plane = 0; plane < vb->num_planes; ++plane)
 		call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
 
-	vb->synced = true;
-	vb->prepared = true;
+	vb->synced = 1;
+	vb->prepared = 1;
 	vb->state = orig_state;
 
 	return 0;
@@ -1803,7 +1803,7 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb,
 	}
 
 	call_void_vb_qop(vb, buf_finish, vb);
-	vb->prepared = false;
+	vb->prepared = 0;
 
 	if (pindex)
 		*pindex = vb->index;
@@ -1927,12 +1927,12 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 			for (plane = 0; plane < vb->num_planes; ++plane)
 				call_void_memop(vb, finish,
 						vb->planes[plane].mem_priv);
-			vb->synced = false;
+			vb->synced = 0;
 		}
 
 		if (vb->prepared) {
 			call_void_vb_qop(vb, buf_finish, vb);
-			vb->prepared = false;
+			vb->prepared = 0;
 		}
 		__vb2_dqbuf(vb);
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 4849b865b908..2757d1902609 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -269,8 +269,8 @@ struct vb2_buffer {
 	 * vb2_plane:		per-plane information; do not change
 	 */
 	enum vb2_buffer_state	state;
-	bool			synced;
-	bool			prepared;
+	unsigned int		synced:1;
+	unsigned int		prepared:1;
 
 	struct vb2_plane	planes[VB2_MAX_PLANES];
 	struct list_head	queued_entry;
-- 
2.20.1

[PATCHv2 2/3] vb2: keep track of timestamp status

[hverkuil-cisco]

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

If a stream is stopped, or if a USERPTR/DMABUF buffer is queued
backed by a different user address or dmabuf fd, then the timestamp
should be skipped by vb2_find_timestamp since the memory it refers
to is no longer valid.

So keep track of a 'copied_timestamp' state: it is set when the
timestamp is copied from an output to a capture buffer, and is
cleared when it is no longer valid.

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

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 35cf36686e20..dc29bd01d6c5 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1041,6 +1041,7 @@ static int __prepare_userptr(struct vb2_buffer *vb)
 		if (vb->planes[plane].mem_priv) {
 			if (!reacquired) {
 				reacquired = true;
+				vb->copied_timestamp = 0;
 				call_void_vb_qop(vb, buf_cleanup, vb);
 			}
 			call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv);
@@ -1165,6 +1166,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
 
 		if (!reacquired) {
 			reacquired = true;
+			vb->copied_timestamp = 0;
 			call_void_vb_qop(vb, buf_cleanup, vb);
 		}
 
@@ -1943,6 +1945,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 		if (vb->request)
 			media_request_put(vb->request);
 		vb->request = NULL;
+		vb->copied_timestamp = 0;
 	}
 }
 
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 3aeaea3af42a..55277370c313 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -604,7 +604,8 @@ int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
 	unsigned int i;
 
 	for (i = start_idx; i < q->num_buffers; i++)
-		if (q->bufs[i]->timestamp == timestamp)
+		if (q->bufs[i]->copied_timestamp &&
+		    q->bufs[i]->timestamp == timestamp)
 			return i;
 	return -1;
 }
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 631f4e2aa942..64b19ee1c847 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -992,6 +992,7 @@ void v4l2_m2m_buf_copy_data(const struct vb2_v4l2_buffer *out_vb,
 	cap_vb->field = out_vb->field;
 	cap_vb->flags &= ~mask;
 	cap_vb->flags |= out_vb->flags & mask;
+	cap_vb->vb2_buf.copied_timestamp = 1;
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_buf_copy_data);
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 2757d1902609..62488d901747 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -262,6 +262,8 @@ struct vb2_buffer {
 	 * prepared:		this buffer has been prepared, i.e. the
 	 *			buf_prepare op was called. It is cleared again
 	 *			after the 'buf_finish' op is called.
+	 * copied_timestamp:	the timestamp of this capture buffer was copied
+	 *			from an output buffer.
 	 * queued_entry:	entry on the queued buffers list, which holds
 	 *			all buffers queued from userspace
 	 * done_entry:		entry on the list that stores all buffers ready
@@ -271,6 +273,7 @@ struct vb2_buffer {
 	enum vb2_buffer_state	state;
 	unsigned int		synced:1;
 	unsigned int		prepared:1;
+	unsigned int		copied_timestamp:1;
 
 	struct vb2_plane	planes[VB2_MAX_PLANES];
 	struct list_head	queued_entry;
-- 
2.20.1

[PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer()

[hverkuil-cisco]

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

When finding a buffer vb2_find_buffer() should also check if the
properties of the found buffer (i.e. number of planes and plane sizes)
match the properties of the 'match' buffer.

Update the cedrus driver accordingly.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c   | 15 ++++++++++++---
 drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c |  8 ++++----
 include/media/videobuf2-v4l2.h                    |  3 ++-
 3 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 55277370c313..0207493c8877 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -599,14 +599,23 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
 };
 
 int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
-		       unsigned int start_idx)
+		       const struct vb2_buffer *match, unsigned int start_idx)
 {
 	unsigned int i;
 
 	for (i = start_idx; i < q->num_buffers; i++)
 		if (q->bufs[i]->copied_timestamp &&
-		    q->bufs[i]->timestamp == timestamp)
-			return i;
+		    q->bufs[i]->timestamp == timestamp &&
+		    q->bufs[i]->num_planes == match->num_planes) {
+			unsigned int p;
+
+			for (p = 0; p < match->num_planes; p++)
+				if (q->bufs[i]->planes[p].length <
+				    match->planes[p].length)
+					break;
+			if (p == match->num_planes)
+				return i;
+		}
 	return -1;
 }
 EXPORT_SYMBOL_GPL(vb2_find_timestamp);
diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
index cb45fda9aaeb..16bc82f1cb2c 100644
--- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
+++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
@@ -159,8 +159,8 @@ 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);
+	forward_idx = vb2_find_timestamp(cap_q, slice_params->forward_ref_ts,
+					 &run->dst->vb2_buf, 0);
 
 	fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
 	fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
@@ -168,8 +168,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
 	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);
 
-	backward_idx = vb2_find_timestamp(cap_q,
-					  slice_params->backward_ref_ts, 0);
+	backward_idx = vb2_find_timestamp(cap_q, slice_params->backward_ref_ts,
+					  &run->dst->vb2_buf, 0);
 	bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
 	bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
 
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index 8a10889dc2fd..b123d12424ba 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -60,6 +60,7 @@ struct vb2_v4l2_buffer {
  *
  * @q:		pointer to &struct vb2_queue with videobuf2 queue.
  * @timestamp:	the timestamp to find.
+ * @match:	the properties of the buffer to find must match this buffer.
  * @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
@@ -69,7 +70,7 @@ struct vb2_v4l2_buffer {
  * -1 if no buffer with @timestamp was found.
  */
 int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
-		       unsigned int start_idx);
+		       const struct vb2_buffer *match, unsigned int start_idx);
 
 int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
 
-- 
2.20.1

Re: [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer()

[Alexandre Courbot]

On Mon, Feb 4, 2019 at 7:11 PM <hverkuil-cisco@xs4all.nl> wrote:
>
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>
> When finding a buffer vb2_find_buffer() should also check if the

I think this is about vb2_find_timestamp() rather than
vb2_find_buffer()? (also in the mail title and in patch 0/3).

> properties of the found buffer (i.e. number of planes and plane sizes)
> match the properties of the 'match' buffer.

What cases does this extra check protect us against? Is this in case
user-space requeues a buffer with a different number of planes/dmabufs
than what it had when its timestamp has been copied? If so, shouldn't
such cases be covered by the reference count increase on the buffer
resource that you mention in 0/3?



>
> Update the cedrus driver accordingly.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/common/videobuf2/videobuf2-v4l2.c   | 15 ++++++++++++---
>  drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c |  8 ++++----
>  include/media/videobuf2-v4l2.h                    |  3 ++-
>  3 files changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 55277370c313..0207493c8877 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -599,14 +599,23 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
>  };
>
>  int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
> -                      unsigned int start_idx)
> +                      const struct vb2_buffer *match, unsigned int start_idx)
>  {
>         unsigned int i;
>
>         for (i = start_idx; i < q->num_buffers; i++)
>                 if (q->bufs[i]->copied_timestamp &&
> -                   q->bufs[i]->timestamp == timestamp)
> -                       return i;
> +                   q->bufs[i]->timestamp == timestamp &&
> +                   q->bufs[i]->num_planes == match->num_planes) {
> +                       unsigned int p;
> +
> +                       for (p = 0; p < match->num_planes; p++)
> +                               if (q->bufs[i]->planes[p].length <
> +                                   match->planes[p].length)
> +                                       break;
> +                       if (p == match->num_planes)
> +                               return i;
> +               }
>         return -1;
>  }
>  EXPORT_SYMBOL_GPL(vb2_find_timestamp);
> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> index cb45fda9aaeb..16bc82f1cb2c 100644
> --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> @@ -159,8 +159,8 @@ 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);
> +       forward_idx = vb2_find_timestamp(cap_q, slice_params->forward_ref_ts,
> +                                        &run->dst->vb2_buf, 0);
>
>         fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
>         fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
> @@ -168,8 +168,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>         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);
>
> -       backward_idx = vb2_find_timestamp(cap_q,
> -                                         slice_params->backward_ref_ts, 0);
> +       backward_idx = vb2_find_timestamp(cap_q, slice_params->backward_ref_ts,
> +                                         &run->dst->vb2_buf, 0);
>         bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
>         bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
>
> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> index 8a10889dc2fd..b123d12424ba 100644
> --- a/include/media/videobuf2-v4l2.h
> +++ b/include/media/videobuf2-v4l2.h
> @@ -60,6 +60,7 @@ struct vb2_v4l2_buffer {
>   *
>   * @q:         pointer to &struct vb2_queue with videobuf2 queue.
>   * @timestamp: the timestamp to find.
> + * @match:     the properties of the buffer to find must match this buffer.
>   * @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
> @@ -69,7 +70,7 @@ struct vb2_v4l2_buffer {
>   * -1 if no buffer with @timestamp was found.
>   */
>  int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
> -                      unsigned int start_idx);
> +                      const struct vb2_buffer *match, unsigned int start_idx);
>
>  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
>
> --
> 2.20.1
>

Re: [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer()

[Hans Verkuil]

On 2/13/19 9:01 AM, Alexandre Courbot wrote:
> On Mon, Feb 4, 2019 at 7:11 PM <hverkuil-cisco@xs4all.nl> wrote:
>>
>> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>
>> When finding a buffer vb2_find_buffer() should also check if the
> 
> I think this is about vb2_find_timestamp() rather than
> vb2_find_buffer()? (also in the mail title and in patch 0/3).

Oops. Yes, you are right.

> 
>> properties of the found buffer (i.e. number of planes and plane sizes)
>> match the properties of the 'match' buffer.
> 
> What cases does this extra check protect us against? Is this in case
> user-space requeues a buffer with a different number of planes/dmabufs
> than what it had when its timestamp has been copied? If so, shouldn't
> such cases be covered by the reference count increase on the buffer
> resource that you mention in 0/3?

It checks that the buffer containing the reference image is a match for the
to-be-filled new capture buffer.

But I think I'll drop this patch for now because it shouldn't check against
match->planes[p].length but instead against match->planes[p].bytesused.

The basic idea is that in the future you might have a mix of smaller and
larger buffers, and you don't want to e.g. decode a 1080p frame using a
reference frame whose buffer was sized for 720p.

This can't happen today with the cedrus driver AFAIK. Or can it?

The refcount increase is unrelated to this. That would protect against
a potential race condition, not against a size mismatch.

In the meantime, can you review/test patches 1 and 2? I'd like to get
those in first.

Thanks!

	Hans

> 
> 
> 
>>
>> Update the cedrus driver accordingly.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/common/videobuf2/videobuf2-v4l2.c   | 15 ++++++++++++---
>>  drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c |  8 ++++----
>>  include/media/videobuf2-v4l2.h                    |  3 ++-
>>  3 files changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 55277370c313..0207493c8877 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -599,14 +599,23 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
>>  };
>>
>>  int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
>> -                      unsigned int start_idx)
>> +                      const struct vb2_buffer *match, unsigned int start_idx)
>>  {
>>         unsigned int i;
>>
>>         for (i = start_idx; i < q->num_buffers; i++)
>>                 if (q->bufs[i]->copied_timestamp &&
>> -                   q->bufs[i]->timestamp == timestamp)
>> -                       return i;
>> +                   q->bufs[i]->timestamp == timestamp &&
>> +                   q->bufs[i]->num_planes == match->num_planes) {
>> +                       unsigned int p;
>> +
>> +                       for (p = 0; p < match->num_planes; p++)
>> +                               if (q->bufs[i]->planes[p].length <
>> +                                   match->planes[p].length)
>> +                                       break;
>> +                       if (p == match->num_planes)
>> +                               return i;
>> +               }
>>         return -1;
>>  }
>>  EXPORT_SYMBOL_GPL(vb2_find_timestamp);
>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
>> index cb45fda9aaeb..16bc82f1cb2c 100644
>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
>> @@ -159,8 +159,8 @@ 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);
>> +       forward_idx = vb2_find_timestamp(cap_q, slice_params->forward_ref_ts,
>> +                                        &run->dst->vb2_buf, 0);
>>
>>         fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
>>         fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
>> @@ -168,8 +168,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>>         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);
>>
>> -       backward_idx = vb2_find_timestamp(cap_q,
>> -                                         slice_params->backward_ref_ts, 0);
>> +       backward_idx = vb2_find_timestamp(cap_q, slice_params->backward_ref_ts,
>> +                                         &run->dst->vb2_buf, 0);
>>         bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
>>         bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
>>
>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>> index 8a10889dc2fd..b123d12424ba 100644
>> --- a/include/media/videobuf2-v4l2.h
>> +++ b/include/media/videobuf2-v4l2.h
>> @@ -60,6 +60,7 @@ struct vb2_v4l2_buffer {
>>   *
>>   * @q:         pointer to &struct vb2_queue with videobuf2 queue.
>>   * @timestamp: the timestamp to find.
>> + * @match:     the properties of the buffer to find must match this buffer.
>>   * @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
>> @@ -69,7 +70,7 @@ struct vb2_v4l2_buffer {
>>   * -1 if no buffer with @timestamp was found.
>>   */
>>  int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
>> -                      unsigned int start_idx);
>> +                      const struct vb2_buffer *match, unsigned int start_idx);
>>
>>  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
>>
>> --
>> 2.20.1
>>

Re: [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer()

[Paul Kocialkowski]

Hi,

On Wed, 2019-02-13 at 09:20 +0100, Hans Verkuil wrote:
> On 2/13/19 9:01 AM, Alexandre Courbot wrote:
> > On Mon, Feb 4, 2019 at 7:11 PM <hverkuil-cisco@xs4all.nl> wrote:
> > > properties of the found buffer (i.e. number of planes and plane sizes)
> > > match the properties of the 'match' buffer.
> > 
> > What cases does this extra check protect us against? Is this in case
> > user-space requeues a buffer with a different number of planes/dmabufs
> > than what it had when its timestamp has been copied? If so, shouldn't
> > such cases be covered by the reference count increase on the buffer
> > resource that you mention in 0/3?
> 
> It checks that the buffer containing the reference image is a match for the
> to-be-filled new capture buffer.
> 
> But I think I'll drop this patch for now because it shouldn't check against
> match->planes[p].length but instead against match->planes[p].bytesused.
> 
> The basic idea is that in the future you might have a mix of smaller and
> larger buffers, and you don't want to e.g. decode a 1080p frame using a
> reference frame whose buffer was sized for 720p.

If that's the case in the future, I think it's only fair to let
userspace deal with associating the right references with the
associated buffers so that there is no mismatch. Having an extra check
in the kernel doesn't hurt, but it feels optional IMO.

> This can't happen today with the cedrus driver AFAIK. Or can it?

As far as I understand from [0], the capture and output formats can't
be changed once we have allocated buffers so I don't really see how we
could end up with mixed resolutions.

[0]: https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/open.html#multiple-opens

Cheers,

Paul

> The refcount increase is unrelated to this. That would protect against
> a potential race condition, not against a size mismatch.
> 
> In the meantime, can you review/test patches 1 and 2? I'd like to get
> those in first.
> 
> Thanks!
> 
> 	Hans
> 
> > 
> > 
> > > Update the cedrus driver accordingly.
> > > 
> > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > ---
> > >  drivers/media/common/videobuf2/videobuf2-v4l2.c   | 15 ++++++++++++---
> > >  drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c |  8 ++++----
> > >  include/media/videobuf2-v4l2.h                    |  3 ++-
> > >  3 files changed, 18 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > index 55277370c313..0207493c8877 100644
> > > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > @@ -599,14 +599,23 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
> > >  };
> > > 
> > >  int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
> > > -                      unsigned int start_idx)
> > > +                      const struct vb2_buffer *match, unsigned int start_idx)
> > >  {
> > >         unsigned int i;
> > > 
> > >         for (i = start_idx; i < q->num_buffers; i++)
> > >                 if (q->bufs[i]->copied_timestamp &&
> > > -                   q->bufs[i]->timestamp == timestamp)
> > > -                       return i;
> > > +                   q->bufs[i]->timestamp == timestamp &&
> > > +                   q->bufs[i]->num_planes == match->num_planes) {
> > > +                       unsigned int p;
> > > +
> > > +                       for (p = 0; p < match->num_planes; p++)
> > > +                               if (q->bufs[i]->planes[p].length <
> > > +                                   match->planes[p].length)
> > > +                                       break;
> > > +                       if (p == match->num_planes)
> > > +                               return i;
> > > +               }
> > >         return -1;
> > >  }
> > >  EXPORT_SYMBOL_GPL(vb2_find_timestamp);
> > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > index cb45fda9aaeb..16bc82f1cb2c 100644
> > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > @@ -159,8 +159,8 @@ 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);
> > > +       forward_idx = vb2_find_timestamp(cap_q, slice_params->forward_ref_ts,
> > > +                                        &run->dst->vb2_buf, 0);
> > > 
> > >         fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
> > >         fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
> > > @@ -168,8 +168,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
> > >         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);
> > > 
> > > -       backward_idx = vb2_find_timestamp(cap_q,
> > > -                                         slice_params->backward_ref_ts, 0);
> > > +       backward_idx = vb2_find_timestamp(cap_q, slice_params->backward_ref_ts,
> > > +                                         &run->dst->vb2_buf, 0);
> > >         bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
> > >         bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
> > > 
> > > diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> > > index 8a10889dc2fd..b123d12424ba 100644
> > > --- a/include/media/videobuf2-v4l2.h
> > > +++ b/include/media/videobuf2-v4l2.h
> > > @@ -60,6 +60,7 @@ struct vb2_v4l2_buffer {
> > >   *
> > >   * @q:         pointer to &struct vb2_queue with videobuf2 queue.
> > >   * @timestamp: the timestamp to find.
> > > + * @match:     the properties of the buffer to find must match this buffer.
> > >   * @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
> > > @@ -69,7 +70,7 @@ struct vb2_v4l2_buffer {
> > >   * -1 if no buffer with @timestamp was found.
> > >   */
> > >  int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
> > > -                      unsigned int start_idx);
> > > +                      const struct vb2_buffer *match, unsigned int start_idx);
> > > 
> > >  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
> > > 
> > > --
> > > 2.20.1
> > > 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCHv2 1/3] vb2: replace bool by bitfield in vb2_buffer

[Paul Kocialkowski]

Hi,

On Mon, 2019-02-04 at 11:11 +0100, hverkuil-cisco@xs4all.nl wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> The bool type is not recommended for use in structs, so replace these
> by bitfields.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 12 ++++++------
>  include/media/videobuf2-core.h                  |  4 ++--
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index e07b6bdb6982..35cf36686e20 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -934,7 +934,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
>  		/* sync buffers */
>  		for (plane = 0; plane < vb->num_planes; ++plane)
>  			call_void_memop(vb, finish, vb->planes[plane].mem_priv);
> -		vb->synced = false;
> +		vb->synced = 0;
>  	}
>  
>  	spin_lock_irqsave(&q->done_lock, flags);
> @@ -1313,8 +1313,8 @@ static int __buf_prepare(struct vb2_buffer *vb)
>  	for (plane = 0; plane < vb->num_planes; ++plane)
>  		call_void_memop(vb, prepare, vb->planes[plane].mem_priv);
>  
> -	vb->synced = true;
> -	vb->prepared = true;
> +	vb->synced = 1;
> +	vb->prepared = 1;
>  	vb->state = orig_state;
>  
>  	return 0;
> @@ -1803,7 +1803,7 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb,
>  	}
>  
>  	call_void_vb_qop(vb, buf_finish, vb);
> -	vb->prepared = false;
> +	vb->prepared = 0;
>  
>  	if (pindex)
>  		*pindex = vb->index;
> @@ -1927,12 +1927,12 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>  			for (plane = 0; plane < vb->num_planes; ++plane)
>  				call_void_memop(vb, finish,
>  						vb->planes[plane].mem_priv);
> -			vb->synced = false;
> +			vb->synced = 0;
>  		}
>  
>  		if (vb->prepared) {
>  			call_void_vb_qop(vb, buf_finish, vb);
> -			vb->prepared = false;
> +			vb->prepared = 0;
>  		}
>  		__vb2_dqbuf(vb);
>  
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 4849b865b908..2757d1902609 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -269,8 +269,8 @@ struct vb2_buffer {
>  	 * vb2_plane:		per-plane information; do not change
>  	 */
>  	enum vb2_buffer_state	state;
> -	bool			synced;
> -	bool			prepared;
> +	unsigned int		synced:1;
> +	unsigned int		prepared:1;
>  
>  	struct vb2_plane	planes[VB2_MAX_PLANES];
>  	struct list_head	queued_entry;
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCHv2 2/3] vb2: keep track of timestamp status

[Paul Kocialkowski]

Hi,

On Mon, 2019-02-04 at 11:11 +0100, hverkuil-cisco@xs4all.nl wrote:
> From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> If a stream is stopped, or if a USERPTR/DMABUF buffer is queued
> backed by a different user address or dmabuf fd, then the timestamp
> should be skipped by vb2_find_timestamp since the memory it refers
> to is no longer valid.
> 
> So keep track of a 'copied_timestamp' state: it is set when the
> timestamp is copied from an output to a capture buffer, and is
> cleared when it is no longer valid.

Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Cheers,

Paul

> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 3 +++
>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 3 ++-
>  drivers/media/v4l2-core/v4l2-mem2mem.c          | 1 +
>  include/media/videobuf2-core.h                  | 3 +++
>  4 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 35cf36686e20..dc29bd01d6c5 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1041,6 +1041,7 @@ static int __prepare_userptr(struct vb2_buffer *vb)
>  		if (vb->planes[plane].mem_priv) {
>  			if (!reacquired) {
>  				reacquired = true;
> +				vb->copied_timestamp = 0;
>  				call_void_vb_qop(vb, buf_cleanup, vb);
>  			}
>  			call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv);
> @@ -1165,6 +1166,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>  
>  		if (!reacquired) {
>  			reacquired = true;
> +			vb->copied_timestamp = 0;
>  			call_void_vb_qop(vb, buf_cleanup, vb);
>  		}
>  
> @@ -1943,6 +1945,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
>  		if (vb->request)
>  			media_request_put(vb->request);
>  		vb->request = NULL;
> +		vb->copied_timestamp = 0;
>  	}
>  }
>  
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index 3aeaea3af42a..55277370c313 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -604,7 +604,8 @@ int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
>  	unsigned int i;
>  
>  	for (i = start_idx; i < q->num_buffers; i++)
> -		if (q->bufs[i]->timestamp == timestamp)
> +		if (q->bufs[i]->copied_timestamp &&
> +		    q->bufs[i]->timestamp == timestamp)
>  			return i;
>  	return -1;
>  }
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index 631f4e2aa942..64b19ee1c847 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -992,6 +992,7 @@ void v4l2_m2m_buf_copy_data(const struct vb2_v4l2_buffer *out_vb,
>  	cap_vb->field = out_vb->field;
>  	cap_vb->flags &= ~mask;
>  	cap_vb->flags |= out_vb->flags & mask;
> +	cap_vb->vb2_buf.copied_timestamp = 1;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_m2m_buf_copy_data);
>  
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 2757d1902609..62488d901747 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -262,6 +262,8 @@ struct vb2_buffer {
>  	 * prepared:		this buffer has been prepared, i.e. the
>  	 *			buf_prepare op was called. It is cleared again
>  	 *			after the 'buf_finish' op is called.
> +	 * copied_timestamp:	the timestamp of this capture buffer was copied
> +	 *			from an output buffer.
>  	 * queued_entry:	entry on the queued buffers list, which holds
>  	 *			all buffers queued from userspace
>  	 * done_entry:		entry on the list that stores all buffers ready
> @@ -271,6 +273,7 @@ struct vb2_buffer {
>  	enum vb2_buffer_state	state;
>  	unsigned int		synced:1;
>  	unsigned int		prepared:1;
> +	unsigned int		copied_timestamp:1;
>  
>  	struct vb2_plane	planes[VB2_MAX_PLANES];
>  	struct list_head	queued_entry;
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer()

[Hans Verkuil]

On 2/13/19 10:08 AM, Paul Kocialkowski wrote:
> Hi,
> 
> On Wed, 2019-02-13 at 09:20 +0100, Hans Verkuil wrote:
>> On 2/13/19 9:01 AM, Alexandre Courbot wrote:
>>> On Mon, Feb 4, 2019 at 7:11 PM <hverkuil-cisco@xs4all.nl> wrote:
>>>> properties of the found buffer (i.e. number of planes and plane sizes)
>>>> match the properties of the 'match' buffer.
>>>
>>> What cases does this extra check protect us against? Is this in case
>>> user-space requeues a buffer with a different number of planes/dmabufs
>>> than what it had when its timestamp has been copied? If so, shouldn't
>>> such cases be covered by the reference count increase on the buffer
>>> resource that you mention in 0/3?
>>
>> It checks that the buffer containing the reference image is a match for the
>> to-be-filled new capture buffer.
>>
>> But I think I'll drop this patch for now because it shouldn't check against
>> match->planes[p].length but instead against match->planes[p].bytesused.
>>
>> The basic idea is that in the future you might have a mix of smaller and
>> larger buffers, and you don't want to e.g. decode a 1080p frame using a
>> reference frame whose buffer was sized for 720p.
> 
> If that's the case in the future, I think it's only fair to let
> userspace deal with associating the right references with the
> associated buffers so that there is no mismatch. Having an extra check
> in the kernel doesn't hurt, but it feels optional IMO.

Never trust anything userspace gives you! You may get garbage, either by
accident or by design.

> 
>> This can't happen today with the cedrus driver AFAIK. Or can it?
> 
> As far as I understand from [0], the capture and output formats can't
> be changed once we have allocated buffers so I don't really see how we
> could end up with mixed resolutions.

Actually, looking at the cedrus code it is missing the check for this:
both cedrus_s_fmt_vid_cap and cedrus_s_fmt_vid_out should call vb2_is_busy()
and return EBUSY if that returns true.

Right now it is perfectly fine for userspace to call S_FMT while buffers
are allocated.

This is a cedrus bug that should be fixed.

Note that v4l2-compliance doesn't check for this, but it probably should.
Or at least warn about it. It is not necessarily wrong to call S_FMT while
buffers are allocated, but the driver has to be able to handle this and
do the necessary checks. But I don't think there are any drivers today that
can handle this.

Regards,

	Hans

> 
> [0]: https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/open.html#multiple-opens
> 
> Cheers,
> 
> Paul
> 
>> The refcount increase is unrelated to this. That would protect against
>> a potential race condition, not against a size mismatch.
>>
>> In the meantime, can you review/test patches 1 and 2? I'd like to get
>> those in first.
>>
>> Thanks!
>>
>> 	Hans
>>
>>>
>>>
>>>> Update the cedrus driver accordingly.
>>>>
>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>> ---
>>>>  drivers/media/common/videobuf2/videobuf2-v4l2.c   | 15 ++++++++++++---
>>>>  drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c |  8 ++++----
>>>>  include/media/videobuf2-v4l2.h                    |  3 ++-
>>>>  3 files changed, 18 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>> index 55277370c313..0207493c8877 100644
>>>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>>>> @@ -599,14 +599,23 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
>>>>  };
>>>>
>>>>  int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
>>>> -                      unsigned int start_idx)
>>>> +                      const struct vb2_buffer *match, unsigned int start_idx)
>>>>  {
>>>>         unsigned int i;
>>>>
>>>>         for (i = start_idx; i < q->num_buffers; i++)
>>>>                 if (q->bufs[i]->copied_timestamp &&
>>>> -                   q->bufs[i]->timestamp == timestamp)
>>>> -                       return i;
>>>> +                   q->bufs[i]->timestamp == timestamp &&
>>>> +                   q->bufs[i]->num_planes == match->num_planes) {
>>>> +                       unsigned int p;
>>>> +
>>>> +                       for (p = 0; p < match->num_planes; p++)
>>>> +                               if (q->bufs[i]->planes[p].length <
>>>> +                                   match->planes[p].length)
>>>> +                                       break;
>>>> +                       if (p == match->num_planes)
>>>> +                               return i;
>>>> +               }
>>>>         return -1;
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(vb2_find_timestamp);
>>>> diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
>>>> index cb45fda9aaeb..16bc82f1cb2c 100644
>>>> --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
>>>> +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
>>>> @@ -159,8 +159,8 @@ 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);
>>>> +       forward_idx = vb2_find_timestamp(cap_q, slice_params->forward_ref_ts,
>>>> +                                        &run->dst->vb2_buf, 0);
>>>>
>>>>         fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
>>>>         fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
>>>> @@ -168,8 +168,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
>>>>         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);
>>>>
>>>> -       backward_idx = vb2_find_timestamp(cap_q,
>>>> -                                         slice_params->backward_ref_ts, 0);
>>>> +       backward_idx = vb2_find_timestamp(cap_q, slice_params->backward_ref_ts,
>>>> +                                         &run->dst->vb2_buf, 0);
>>>>         bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
>>>>         bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
>>>>
>>>> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
>>>> index 8a10889dc2fd..b123d12424ba 100644
>>>> --- a/include/media/videobuf2-v4l2.h
>>>> +++ b/include/media/videobuf2-v4l2.h
>>>> @@ -60,6 +60,7 @@ struct vb2_v4l2_buffer {
>>>>   *
>>>>   * @q:         pointer to &struct vb2_queue with videobuf2 queue.
>>>>   * @timestamp: the timestamp to find.
>>>> + * @match:     the properties of the buffer to find must match this buffer.
>>>>   * @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
>>>> @@ -69,7 +70,7 @@ struct vb2_v4l2_buffer {
>>>>   * -1 if no buffer with @timestamp was found.
>>>>   */
>>>>  int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
>>>> -                      unsigned int start_idx);
>>>> +                      const struct vb2_buffer *match, unsigned int start_idx);
>>>>
>>>>  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
>>>>
>>>> --
>>>> 2.20.1
>>>>

Re: [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer()

[Paul Kocialkowski]

On Wed, 2019-02-13 at 10:39 +0100, Hans Verkuil wrote:
> On 2/13/19 10:08 AM, Paul Kocialkowski wrote:
> > Hi,
> > 
> > On Wed, 2019-02-13 at 09:20 +0100, Hans Verkuil wrote:
> > As far as I understand from [0], the capture and output formats can't
> > be changed once we have allocated buffers so I don't really see how we
> > could end up with mixed resolutions.
> 
> Actually, looking at the cedrus code it is missing the check for this:
> both cedrus_s_fmt_vid_cap and cedrus_s_fmt_vid_out should call vb2_is_busy()
> and return EBUSY if that returns true.
> 
> Right now it is perfectly fine for userspace to call S_FMT while buffers
> are allocated.
> 
> This is a cedrus bug that should be fixed.

Definitely yes, we totally missed that. I'll send a fix soon.

> Note that v4l2-compliance doesn't check for this, but it probably should.
> Or at least warn about it. It is not necessarily wrong to call S_FMT while
> buffers are allocated, but the driver has to be able to handle this and
> do the necessary checks. But I don't think there are any drivers today that
> can handle this.

Yes I agree that it would make sense to check this.

Cheers,

Paul

> Regards,
> 
> 	Hans
> 
> > [0]: https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/open.html#multiple-opens
> > 
> > Cheers,
> > 
> > Paul
> > 
> > > The refcount increase is unrelated to this. That would protect against
> > > a potential race condition, not against a size mismatch.
> > > 
> > > In the meantime, can you review/test patches 1 and 2? I'd like to get
> > > those in first.
> > > 
> > > Thanks!
> > > 
> > > 	Hans
> > > 
> > > > 
> > > > > Update the cedrus driver accordingly.
> > > > > 
> > > > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > > > ---
> > > > >  drivers/media/common/videobuf2/videobuf2-v4l2.c   | 15 ++++++++++++---
> > > > >  drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c |  8 ++++----
> > > > >  include/media/videobuf2-v4l2.h                    |  3 ++-
> > > > >  3 files changed, 18 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > > > index 55277370c313..0207493c8877 100644
> > > > > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > > > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > > > @@ -599,14 +599,23 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
> > > > >  };
> > > > > 
> > > > >  int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
> > > > > -                      unsigned int start_idx)
> > > > > +                      const struct vb2_buffer *match, unsigned int start_idx)
> > > > >  {
> > > > >         unsigned int i;
> > > > > 
> > > > >         for (i = start_idx; i < q->num_buffers; i++)
> > > > >                 if (q->bufs[i]->copied_timestamp &&
> > > > > -                   q->bufs[i]->timestamp == timestamp)
> > > > > -                       return i;
> > > > > +                   q->bufs[i]->timestamp == timestamp &&
> > > > > +                   q->bufs[i]->num_planes == match->num_planes) {
> > > > > +                       unsigned int p;
> > > > > +
> > > > > +                       for (p = 0; p < match->num_planes; p++)
> > > > > +                               if (q->bufs[i]->planes[p].length <
> > > > > +                                   match->planes[p].length)
> > > > > +                                       break;
> > > > > +                       if (p == match->num_planes)
> > > > > +                               return i;
> > > > > +               }
> > > > >         return -1;
> > > > >  }
> > > > >  EXPORT_SYMBOL_GPL(vb2_find_timestamp);
> > > > > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > > > index cb45fda9aaeb..16bc82f1cb2c 100644
> > > > > --- a/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > > > +++ b/drivers/staging/media/sunxi/cedrus/cedrus_mpeg2.c
> > > > > @@ -159,8 +159,8 @@ 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);
> > > > > +       forward_idx = vb2_find_timestamp(cap_q, slice_params->forward_ref_ts,
> > > > > +                                        &run->dst->vb2_buf, 0);
> > > > > 
> > > > >         fwd_luma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 0);
> > > > >         fwd_chroma_addr = cedrus_dst_buf_addr(ctx, forward_idx, 1);
> > > > > @@ -168,8 +168,8 @@ static void cedrus_mpeg2_setup(struct cedrus_ctx *ctx, struct cedrus_run *run)
> > > > >         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);
> > > > > 
> > > > > -       backward_idx = vb2_find_timestamp(cap_q,
> > > > > -                                         slice_params->backward_ref_ts, 0);
> > > > > +       backward_idx = vb2_find_timestamp(cap_q, slice_params->backward_ref_ts,
> > > > > +                                         &run->dst->vb2_buf, 0);
> > > > >         bwd_luma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 0);
> > > > >         bwd_chroma_addr = cedrus_dst_buf_addr(ctx, backward_idx, 1);
> > > > > 
> > > > > diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
> > > > > index 8a10889dc2fd..b123d12424ba 100644
> > > > > --- a/include/media/videobuf2-v4l2.h
> > > > > +++ b/include/media/videobuf2-v4l2.h
> > > > > @@ -60,6 +60,7 @@ struct vb2_v4l2_buffer {
> > > > >   *
> > > > >   * @q:         pointer to &struct vb2_queue with videobuf2 queue.
> > > > >   * @timestamp: the timestamp to find.
> > > > > + * @match:     the properties of the buffer to find must match this buffer.
> > > > >   * @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
> > > > > @@ -69,7 +70,7 @@ struct vb2_v4l2_buffer {
> > > > >   * -1 if no buffer with @timestamp was found.
> > > > >   */
> > > > >  int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp,
> > > > > -                      unsigned int start_idx);
> > > > > +                      const struct vb2_buffer *match, unsigned int start_idx);
> > > > > 
> > > > >  int vb2_querybuf(struct vb2_queue *q, struct v4l2_buffer *b);
> > > > > 
> > > > > --
> > > > > 2.20.1
> > > > > 
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com