| Message ID | 20201129031545.557586-1-sergey.senozhatsky@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv3] media: vb2: always set buffer cache sync hints | expand |
On 29/11/2020 04:15, Sergey Senozhatsky wrote: > We need to always set ->need_cache_sync_on_prepare and > ->need_cache_sync_on_finish when we initialize vb2 buffer. > > Currently these flags are set/adjusted only in V4L2's > vb2_queue_or_prepare_buf(), which means that for the code > paths that don't use V4L2 vb2 will always tell videobuf2 > core to skip ->prepare() and ->finish() cache syncs/flushes. > > This is a quick solution that should do the trick. The > proper fix, however, is much more complicated and requires > a rather big videobuf2 refactoring - we need to move cache > sync/flush decision making out of core videobuf2 to the > allocators. > > Fixes: f5f5fa73fbfb ("media: videobuf2: handle V4L2 buffer cache flags") > Reported-by: Tomasz Figa <tfiga@chromium.org> > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > v3: Improved code comment and dropped queue allow_cache_hints check (Tomasz) > v2: Added a comment and set cache sync flags only for specific buffers (Hans) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 5499013cf82e..3f11fc5b5d9a 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -414,6 +414,20 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, > vb->index = q->num_buffers + buffer; > vb->type = q->type; > vb->memory = memory; > + /* > + * A workaround fix. We need to set these flags here so that > + * videobuf2 core will call ->prepare()/->finish() cache > + * sync/flush on vb2 buffers when appropriate. Otherwise, for > + * backends that don't rely on V4L2 (perhaps dvb) these flags > + * will always be false and, hence, videobuf2 core will skip > + * cache sync/flush operations. However, we can avoid explicit > + * ->prepare() and ->finish() cache sync for DMABUF buffers, > + * because DMA exporter takes care of it. > + */ > + if (q->memory != VB2_MEMORY_DMABUF) { > + vb->need_cache_sync_on_prepare = 1; > + vb->need_cache_sync_on_finish = 1; > + } Is this a work-around fix? Isn't this just a bug fix? It seems reasonable to always set this when allocating buffers for a queue. And for v4l2 these values can be changed if supported by the driver (allow_cache_hints is set). So the comment would be: /* * We need to set these flags here so that the videobuf2 core * will call ->prepare()/->finish() cache sync/flush on vb2 * buffers when appropriate. However, we can avoid explicit * ->prepare() and ->finish() cache sync for DMABUF buffers, * because DMA exporter takes care of it. */ The commit message would need to be modified as well. Regards, Hans > for (plane = 0; plane < num_planes; ++plane) { > vb->planes[plane].length = plane_sizes[plane]; > vb->planes[plane].min_length = plane_sizes[plane]; >
On (20/11/30 11:02), Hans Verkuil wrote: > > @@ -414,6 +414,20 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, > > vb->index = q->num_buffers + buffer; > > vb->type = q->type; > > vb->memory = memory; > > + /* > > + * A workaround fix. We need to set these flags here so that > > + * videobuf2 core will call ->prepare()/->finish() cache > > + * sync/flush on vb2 buffers when appropriate. Otherwise, for > > + * backends that don't rely on V4L2 (perhaps dvb) these flags > > + * will always be false and, hence, videobuf2 core will skip > > + * cache sync/flush operations. However, we can avoid explicit > > + * ->prepare() and ->finish() cache sync for DMABUF buffers, > > + * because DMA exporter takes care of it. > > + */ > > + if (q->memory != VB2_MEMORY_DMABUF) { > > + vb->need_cache_sync_on_prepare = 1; > > + vb->need_cache_sync_on_finish = 1; > > + } > > Is this a work-around fix? Isn't this just a bug fix? It seems reasonable > to always set this when allocating buffers for a queue. And for v4l2 these > values can be changed if supported by the driver (allow_cache_hints is set). Yes, it's a bug fix. It's a workaround bug fix in a sense that a proper bug fix is to rename these flags and invert them (and patch all the functions that set and test them). A sneak preview of what's in my tree: --- diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index bd78f6c7f342..3b79042d9d5c 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -264,10 +264,10 @@ struct vb2_buffer { * after the 'buf_finish' op is called. * copied_timestamp: the timestamp of this capture buffer was copied * from an output buffer. - * need_cache_sync_on_prepare: when set buffer's ->prepare() function - * performs cache sync/invalidation. - * need_cache_sync_on_finish: when set buffer's ->finish() function - * performs cache sync/invalidation. + * no_cache_sync_on_prepare: when set buffer's ->prepare() function + * skips cache sync/invalidation. + * no_cache_sync_on_finish: when set buffer's ->finish() function + * skips cache sync/invalidation. * 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 @@ -278,8 +278,8 @@ struct vb2_buffer { unsigned int synced:1; unsigned int prepared:1; unsigned int copied_timestamp:1; - unsigned int need_cache_sync_on_prepare:1; - unsigned int need_cache_sync_on_finish:1; + unsigned int no_cache_sync_on_prepare:1; + unsigned int no_cache_sync_on_finish:1; struct vb2_plane planes[VB2_MAX_PLANES]; struct list_head queued_entry; --- and it's much bigger than a 3-liner ("workaround") fix. > So the comment would be: > > /* > * We need to set these flags here so that the videobuf2 core > * will call ->prepare()/->finish() cache sync/flush on vb2 > * buffers when appropriate. However, we can avoid explicit > * ->prepare() and ->finish() cache sync for DMABUF buffers, > * because DMA exporter takes care of it. > */ > > The commit message would need to be modified as well. OK. -ss
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 5499013cf82e..3f11fc5b5d9a 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -414,6 +414,20 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory, vb->index = q->num_buffers + buffer; vb->type = q->type; vb->memory = memory; + /* + * A workaround fix. We need to set these flags here so that + * videobuf2 core will call ->prepare()/->finish() cache + * sync/flush on vb2 buffers when appropriate. Otherwise, for + * backends that don't rely on V4L2 (perhaps dvb) these flags + * will always be false and, hence, videobuf2 core will skip + * cache sync/flush operations. However, we can avoid explicit + * ->prepare() and ->finish() cache sync for DMABUF buffers, + * because DMA exporter takes care of it. + */ + if (q->memory != VB2_MEMORY_DMABUF) { + vb->need_cache_sync_on_prepare = 1; + vb->need_cache_sync_on_finish = 1; + } for (plane = 0; plane < num_planes; ++plane) { vb->planes[plane].length = plane_sizes[plane]; vb->planes[plane].min_length = plane_sizes[plane];
We need to always set ->need_cache_sync_on_prepare and ->need_cache_sync_on_finish when we initialize vb2 buffer. Currently these flags are set/adjusted only in V4L2's vb2_queue_or_prepare_buf(), which means that for the code paths that don't use V4L2 vb2 will always tell videobuf2 core to skip ->prepare() and ->finish() cache syncs/flushes. This is a quick solution that should do the trick. The proper fix, however, is much more complicated and requires a rather big videobuf2 refactoring - we need to move cache sync/flush decision making out of core videobuf2 to the allocators. Fixes: f5f5fa73fbfb ("media: videobuf2: handle V4L2 buffer cache flags") Reported-by: Tomasz Figa <tfiga@chromium.org> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- drivers/media/common/videobuf2/videobuf2-core.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) v3: Improved code comment and dropped queue allow_cache_hints check (Tomasz) v2: Added a comment and set cache sync flags only for specific buffers (Hans)