On Mon, Feb 4, 2019 at 7:11 PM <hverkuil-cisco@xxxxxxxxx> wrote:
>
> From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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
>
