* [PATCHv2 0/3] Improve vb2_find_buffer
@ 2019-02-04 10:11 hverkuil-cisco
2019-02-04 10:11 ` [PATCHv2 1/3] vb2: replace bool by bitfield in vb2_buffer hverkuil-cisco
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: hverkuil-cisco @ 2019-02-04 10:11 UTC (permalink / raw)
To: linux-media
Cc: Nicolas Dufresne, Tomasz Figa, Alexandre Courbot, Paul Kocialkowski
From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
This supersedes my previous patch:
https://www.mail-archive.com/linux-media@vger.kernel.org/msg144099.html
Rather than marking a timestamp of 0 as 'special', I add a bitfield
to keep track of the validity of the timestamp.
The last patch also adds extra checks to verify that the found buffer
matches what is expected w.r.t. the number of planes and the size of
the planes.
I still think vb2_find_timestamp should increase some refcount for
the buffer since it is possible that the application requeues the
found buffer with a different dmabuf fd, thus potentially freeing the
buffer memory while it is still being processed.
But to be honest, I'm not sure how that should be done.
Regards,
Hans
Hans Verkuil (3):
vb2: replace bool by bitfield in vb2_buffer
vb2: keep track of timestamp status
vb2: add 'match' arg to vb2_find_buffer()
drivers/media/common/videobuf2/videobuf2-core.c | 15 +++++++++------
drivers/media/common/videobuf2/videobuf2-v4l2.c | 16 +++++++++++++---
drivers/media/v4l2-core/v4l2-mem2mem.c | 1 +
.../staging/media/sunxi/cedrus/cedrus_mpeg2.c | 8 ++++----
include/media/videobuf2-core.h | 7 +++++--
include/media/videobuf2-v4l2.h | 3 ++-
6 files changed, 34 insertions(+), 16 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCHv2 1/3] vb2: replace bool by bitfield in vb2_buffer 2019-02-04 10:11 [PATCHv2 0/3] Improve vb2_find_buffer hverkuil-cisco @ 2019-02-04 10:11 ` hverkuil-cisco 2019-02-13 9:22 ` Paul Kocialkowski 2019-02-04 10:11 ` [PATCHv2 2/3] vb2: keep track of timestamp status hverkuil-cisco 2019-02-04 10:11 ` [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer() hverkuil-cisco 2 siblings, 1 reply; 11+ messages in thread From: hverkuil-cisco @ 2019-02-04 10:11 UTC (permalink / raw) To: linux-media Cc: Nicolas Dufresne, Tomasz Figa, Alexandre Courbot, Paul Kocialkowski, Hans Verkuil From: Hans Verkuil <hverkuil-cisco@xs4all.nl> The bool type is not recommended for use in structs, so replace these by bitfields. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/media/common/videobuf2/videobuf2-core.c | 12 ++++++------ include/media/videobuf2-core.h | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index e07b6bdb6982..35cf36686e20 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -934,7 +934,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) /* sync buffers */ for (plane = 0; plane < vb->num_planes; ++plane) call_void_memop(vb, finish, vb->planes[plane].mem_priv); - vb->synced = false; + vb->synced = 0; } spin_lock_irqsave(&q->done_lock, flags); @@ -1313,8 +1313,8 @@ static int __buf_prepare(struct vb2_buffer *vb) for (plane = 0; plane < vb->num_planes; ++plane) call_void_memop(vb, prepare, vb->planes[plane].mem_priv); - vb->synced = true; - vb->prepared = true; + vb->synced = 1; + vb->prepared = 1; vb->state = orig_state; return 0; @@ -1803,7 +1803,7 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, } call_void_vb_qop(vb, buf_finish, vb); - vb->prepared = false; + vb->prepared = 0; if (pindex) *pindex = vb->index; @@ -1927,12 +1927,12 @@ static void __vb2_queue_cancel(struct vb2_queue *q) for (plane = 0; plane < vb->num_planes; ++plane) call_void_memop(vb, finish, vb->planes[plane].mem_priv); - vb->synced = false; + vb->synced = 0; } if (vb->prepared) { call_void_vb_qop(vb, buf_finish, vb); - vb->prepared = false; + vb->prepared = 0; } __vb2_dqbuf(vb); diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 4849b865b908..2757d1902609 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -269,8 +269,8 @@ struct vb2_buffer { * vb2_plane: per-plane information; do not change */ enum vb2_buffer_state state; - bool synced; - bool prepared; + unsigned int synced:1; + unsigned int prepared:1; struct vb2_plane planes[VB2_MAX_PLANES]; struct list_head queued_entry; -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv2 1/3] vb2: replace bool by bitfield in vb2_buffer 2019-02-04 10:11 ` [PATCHv2 1/3] vb2: replace bool by bitfield in vb2_buffer hverkuil-cisco @ 2019-02-13 9:22 ` Paul Kocialkowski 0 siblings, 0 replies; 11+ messages in thread From: Paul Kocialkowski @ 2019-02-13 9:22 UTC (permalink / raw) To: hverkuil-cisco, linux-media Cc: Nicolas Dufresne, Tomasz Figa, Alexandre Courbot Hi, On Mon, 2019-02-04 at 11:11 +0100, hverkuil-cisco@xs4all.nl wrote: > From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > The bool type is not recommended for use in structs, so replace these > by bitfields. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Cheers, Paul > --- > drivers/media/common/videobuf2/videobuf2-core.c | 12 ++++++------ > include/media/videobuf2-core.h | 4 ++-- > 2 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index e07b6bdb6982..35cf36686e20 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -934,7 +934,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > /* sync buffers */ > for (plane = 0; plane < vb->num_planes; ++plane) > call_void_memop(vb, finish, vb->planes[plane].mem_priv); > - vb->synced = false; > + vb->synced = 0; > } > > spin_lock_irqsave(&q->done_lock, flags); > @@ -1313,8 +1313,8 @@ static int __buf_prepare(struct vb2_buffer *vb) > for (plane = 0; plane < vb->num_planes; ++plane) > call_void_memop(vb, prepare, vb->planes[plane].mem_priv); > > - vb->synced = true; > - vb->prepared = true; > + vb->synced = 1; > + vb->prepared = 1; > vb->state = orig_state; > > return 0; > @@ -1803,7 +1803,7 @@ int vb2_core_dqbuf(struct vb2_queue *q, unsigned int *pindex, void *pb, > } > > call_void_vb_qop(vb, buf_finish, vb); > - vb->prepared = false; > + vb->prepared = 0; > > if (pindex) > *pindex = vb->index; > @@ -1927,12 +1927,12 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > for (plane = 0; plane < vb->num_planes; ++plane) > call_void_memop(vb, finish, > vb->planes[plane].mem_priv); > - vb->synced = false; > + vb->synced = 0; > } > > if (vb->prepared) { > call_void_vb_qop(vb, buf_finish, vb); > - vb->prepared = false; > + vb->prepared = 0; > } > __vb2_dqbuf(vb); > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 4849b865b908..2757d1902609 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -269,8 +269,8 @@ struct vb2_buffer { > * vb2_plane: per-plane information; do not change > */ > enum vb2_buffer_state state; > - bool synced; > - bool prepared; > + unsigned int synced:1; > + unsigned int prepared:1; > > struct vb2_plane planes[VB2_MAX_PLANES]; > struct list_head queued_entry; -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCHv2 2/3] vb2: keep track of timestamp status 2019-02-04 10:11 [PATCHv2 0/3] Improve vb2_find_buffer hverkuil-cisco 2019-02-04 10:11 ` [PATCHv2 1/3] vb2: replace bool by bitfield in vb2_buffer hverkuil-cisco @ 2019-02-04 10:11 ` hverkuil-cisco 2019-02-13 9:22 ` Paul Kocialkowski 2019-02-04 10:11 ` [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer() hverkuil-cisco 2 siblings, 1 reply; 11+ messages in thread From: hverkuil-cisco @ 2019-02-04 10:11 UTC (permalink / raw) To: linux-media Cc: Nicolas Dufresne, Tomasz Figa, Alexandre Courbot, Paul Kocialkowski, Hans Verkuil From: Hans Verkuil <hverkuil-cisco@xs4all.nl> If a stream is stopped, or if a USERPTR/DMABUF buffer is queued backed by a different user address or dmabuf fd, then the timestamp should be skipped by vb2_find_timestamp since the memory it refers to is no longer valid. So keep track of a 'copied_timestamp' state: it is set when the timestamp is copied from an output to a capture buffer, and is cleared when it is no longer valid. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> --- drivers/media/common/videobuf2/videobuf2-core.c | 3 +++ drivers/media/common/videobuf2/videobuf2-v4l2.c | 3 ++- drivers/media/v4l2-core/v4l2-mem2mem.c | 1 + include/media/videobuf2-core.h | 3 +++ 4 files changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 35cf36686e20..dc29bd01d6c5 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -1041,6 +1041,7 @@ static int __prepare_userptr(struct vb2_buffer *vb) if (vb->planes[plane].mem_priv) { if (!reacquired) { reacquired = true; + vb->copied_timestamp = 0; call_void_vb_qop(vb, buf_cleanup, vb); } call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv); @@ -1165,6 +1166,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) if (!reacquired) { reacquired = true; + vb->copied_timestamp = 0; call_void_vb_qop(vb, buf_cleanup, vb); } @@ -1943,6 +1945,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q) if (vb->request) media_request_put(vb->request); vb->request = NULL; + vb->copied_timestamp = 0; } } diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index 3aeaea3af42a..55277370c313 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -604,7 +604,8 @@ int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp, unsigned int i; for (i = start_idx; i < q->num_buffers; i++) - if (q->bufs[i]->timestamp == timestamp) + if (q->bufs[i]->copied_timestamp && + q->bufs[i]->timestamp == timestamp) return i; return -1; } diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c index 631f4e2aa942..64b19ee1c847 100644 --- a/drivers/media/v4l2-core/v4l2-mem2mem.c +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c @@ -992,6 +992,7 @@ void v4l2_m2m_buf_copy_data(const struct vb2_v4l2_buffer *out_vb, cap_vb->field = out_vb->field; cap_vb->flags &= ~mask; cap_vb->flags |= out_vb->flags & mask; + cap_vb->vb2_buf.copied_timestamp = 1; } EXPORT_SYMBOL_GPL(v4l2_m2m_buf_copy_data); diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 2757d1902609..62488d901747 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -262,6 +262,8 @@ struct vb2_buffer { * prepared: this buffer has been prepared, i.e. the * buf_prepare op was called. It is cleared again * after the 'buf_finish' op is called. + * copied_timestamp: the timestamp of this capture buffer was copied + * from an output buffer. * queued_entry: entry on the queued buffers list, which holds * all buffers queued from userspace * done_entry: entry on the list that stores all buffers ready @@ -271,6 +273,7 @@ struct vb2_buffer { enum vb2_buffer_state state; unsigned int synced:1; unsigned int prepared:1; + unsigned int copied_timestamp:1; struct vb2_plane planes[VB2_MAX_PLANES]; struct list_head queued_entry; -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv2 2/3] vb2: keep track of timestamp status 2019-02-04 10:11 ` [PATCHv2 2/3] vb2: keep track of timestamp status hverkuil-cisco @ 2019-02-13 9:22 ` Paul Kocialkowski 0 siblings, 0 replies; 11+ messages in thread From: Paul Kocialkowski @ 2019-02-13 9:22 UTC (permalink / raw) To: hverkuil-cisco, linux-media Cc: Nicolas Dufresne, Tomasz Figa, Alexandre Courbot Hi, On Mon, 2019-02-04 at 11:11 +0100, hverkuil-cisco@xs4all.nl wrote: > From: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > If a stream is stopped, or if a USERPTR/DMABUF buffer is queued > backed by a different user address or dmabuf fd, then the timestamp > should be skipped by vb2_find_timestamp since the memory it refers > to is no longer valid. > > So keep track of a 'copied_timestamp' state: it is set when the > timestamp is copied from an output to a capture buffer, and is > cleared when it is no longer valid. Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Cheers, Paul > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 3 +++ > drivers/media/common/videobuf2/videobuf2-v4l2.c | 3 ++- > drivers/media/v4l2-core/v4l2-mem2mem.c | 1 + > include/media/videobuf2-core.h | 3 +++ > 4 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 35cf36686e20..dc29bd01d6c5 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -1041,6 +1041,7 @@ static int __prepare_userptr(struct vb2_buffer *vb) > if (vb->planes[plane].mem_priv) { > if (!reacquired) { > reacquired = true; > + vb->copied_timestamp = 0; > call_void_vb_qop(vb, buf_cleanup, vb); > } > call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv); > @@ -1165,6 +1166,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) > > if (!reacquired) { > reacquired = true; > + vb->copied_timestamp = 0; > call_void_vb_qop(vb, buf_cleanup, vb); > } > > @@ -1943,6 +1945,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > if (vb->request) > media_request_put(vb->request); > vb->request = NULL; > + vb->copied_timestamp = 0; > } > } > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index 3aeaea3af42a..55277370c313 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -604,7 +604,8 @@ int vb2_find_timestamp(const struct vb2_queue *q, u64 timestamp, > unsigned int i; > > for (i = start_idx; i < q->num_buffers; i++) > - if (q->bufs[i]->timestamp == timestamp) > + if (q->bufs[i]->copied_timestamp && > + q->bufs[i]->timestamp == timestamp) > return i; > return -1; > } > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c > index 631f4e2aa942..64b19ee1c847 100644 > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c > @@ -992,6 +992,7 @@ void v4l2_m2m_buf_copy_data(const struct vb2_v4l2_buffer *out_vb, > cap_vb->field = out_vb->field; > cap_vb->flags &= ~mask; > cap_vb->flags |= out_vb->flags & mask; > + cap_vb->vb2_buf.copied_timestamp = 1; > } > EXPORT_SYMBOL_GPL(v4l2_m2m_buf_copy_data); > > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 2757d1902609..62488d901747 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -262,6 +262,8 @@ struct vb2_buffer { > * prepared: this buffer has been prepared, i.e. the > * buf_prepare op was called. It is cleared again > * after the 'buf_finish' op is called. > + * copied_timestamp: the timestamp of this capture buffer was copied > + * from an output buffer. > * queued_entry: entry on the queued buffers list, which holds > * all buffers queued from userspace > * done_entry: entry on the list that stores all buffers ready > @@ -271,6 +273,7 @@ struct vb2_buffer { > enum vb2_buffer_state state; > unsigned int synced:1; > unsigned int prepared:1; > + unsigned int copied_timestamp:1; > > struct vb2_plane planes[VB2_MAX_PLANES]; > struct list_head queued_entry; -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer() 2019-02-04 10:11 [PATCHv2 0/3] Improve vb2_find_buffer hverkuil-cisco 2019-02-04 10:11 ` [PATCHv2 1/3] vb2: replace bool by bitfield in vb2_buffer hverkuil-cisco 2019-02-04 10:11 ` [PATCHv2 2/3] vb2: keep track of timestamp status hverkuil-cisco @ 2019-02-04 10:11 ` hverkuil-cisco 2019-02-13 8:01 ` Alexandre Courbot 2 siblings, 1 reply; 11+ messages in thread From: hverkuil-cisco @ 2019-02-04 10:11 UTC (permalink / raw) To: linux-media Cc: Nicolas Dufresne, Tomasz Figa, Alexandre Courbot, Paul Kocialkowski, Hans Verkuil From: Hans Verkuil <hverkuil-cisco@xs4all.nl> When finding a buffer vb2_find_buffer() should also check if the properties of the found buffer (i.e. number of planes and plane sizes) match the properties of the 'match' buffer. 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer() 2019-02-04 10:11 ` [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer() hverkuil-cisco @ 2019-02-13 8:01 ` Alexandre Courbot 2019-02-13 8:20 ` Hans Verkuil 0 siblings, 1 reply; 11+ messages in thread From: Alexandre Courbot @ 2019-02-13 8:01 UTC (permalink / raw) To: Hans Verkuil Cc: Linux Media Mailing List, Nicolas Dufresne, Tomasz Figa, Paul Kocialkowski 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 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer() 2019-02-13 8:01 ` Alexandre Courbot @ 2019-02-13 8:20 ` Hans Verkuil 2019-02-13 9:08 ` Paul Kocialkowski 0 siblings, 1 reply; 11+ messages in thread From: Hans Verkuil @ 2019-02-13 8:20 UTC (permalink / raw) To: Alexandre Courbot Cc: Linux Media Mailing List, Nicolas Dufresne, Tomasz Figa, Paul Kocialkowski 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 >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer() 2019-02-13 8:20 ` Hans Verkuil @ 2019-02-13 9:08 ` Paul Kocialkowski 2019-02-13 9:39 ` Hans Verkuil 0 siblings, 1 reply; 11+ messages in thread From: Paul Kocialkowski @ 2019-02-13 9:08 UTC (permalink / raw) To: Hans Verkuil, Alexandre Courbot Cc: Linux Media Mailing List, Nicolas Dufresne, Tomasz Figa 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 > > > -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer() 2019-02-13 9:08 ` Paul Kocialkowski @ 2019-02-13 9:39 ` Hans Verkuil 2019-02-13 12:51 ` Paul Kocialkowski 0 siblings, 1 reply; 11+ messages in thread From: Hans Verkuil @ 2019-02-13 9:39 UTC (permalink / raw) To: Paul Kocialkowski, Alexandre Courbot Cc: Linux Media Mailing List, Nicolas Dufresne, Tomasz Figa 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 >>>> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer() 2019-02-13 9:39 ` Hans Verkuil @ 2019-02-13 12:51 ` Paul Kocialkowski 0 siblings, 0 replies; 11+ messages in thread From: Paul Kocialkowski @ 2019-02-13 12:51 UTC (permalink / raw) To: Hans Verkuil, Alexandre Courbot Cc: Linux Media Mailing List, Nicolas Dufresne, Tomasz Figa 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 > > > > > -- Paul Kocialkowski, Bootlin Embedded Linux and kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-02-13 12:51 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-04 10:11 [PATCHv2 0/3] Improve vb2_find_buffer hverkuil-cisco 2019-02-04 10:11 ` [PATCHv2 1/3] vb2: replace bool by bitfield in vb2_buffer hverkuil-cisco 2019-02-13 9:22 ` Paul Kocialkowski 2019-02-04 10:11 ` [PATCHv2 2/3] vb2: keep track of timestamp status hverkuil-cisco 2019-02-13 9:22 ` Paul Kocialkowski 2019-02-04 10:11 ` [PATCHv2 3/3] vb2: add 'match' arg to vb2_find_buffer() hverkuil-cisco 2019-02-13 8:01 ` Alexandre Courbot 2019-02-13 8:20 ` Hans Verkuil 2019-02-13 9:08 ` Paul Kocialkowski 2019-02-13 9:39 ` Hans Verkuil 2019-02-13 12:51 ` Paul Kocialkowski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).