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