On 2/13/19 9:01 AM, Alexandre Courbot wrote:
> 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).
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@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
>>
