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@xxxxxxxxx> 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@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 >>>>
