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. > 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@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 > > > -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com
