| Message ID | 20190204101134.56283-4-hverkuil-cisco@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve vb2_find_buffer | expand |
On Mon, Feb 4, 2019 at 7:11 PM <hverkuil-cisco@xs4all.nl> wrote: > > From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > 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@xs4all.nl> > --- > 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 >
On 2/13/19 9:01 AM, Alexandre Courbot wrote: > On Mon, Feb 4, 2019 at 7:11 PM <hverkuil-cisco@xs4all.nl> wrote: >> >> From: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> >> 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@xs4all.nl> >> --- >> 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 >>
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@xs4all.nl> 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@xs4all.nl> > > > --- > > > 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 > > >
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@xs4all.nl> 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@xs4all.nl> >>>> --- >>>> 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 >>>>
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@xs4all.nl> > > > > > --- > > > > > 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 > > > > >
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);