| Message ID | 20201127094136.1051071-1-sergey.senozhatsky@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: vb2: always set buffer cache sync hints | expand |
On (20/11/27 18:41), 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. > > Reported-by: Tomasz Figa <tfiga@chromium.org> > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> Fixes: f5f5fa73fbfb ("media: videobuf2: handle V4L2 buffer cache flags") -ss
On 27/11/2020 10:41, 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. > > Reported-by: Tomasz Figa <tfiga@chromium.org> > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > drivers/media/common/videobuf2/videobuf2-core.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 5499013cf82e..14a26888a892 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -414,6 +414,8 @@ 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; > + vb->need_cache_sync_on_prepare = 1; > + vb->need_cache_sync_on_finish = 1; I think this needs a comment, basically explaining what you said in the commit log. It's not obvious from the code that this is a workaround. 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/27 13:59), Hans Verkuil wrote: > > I think this needs a comment, basically explaining what you said in > the commit log. It's not obvious from the code that this is a > workaround. Something like this? --- diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 5499013cf82e..21b2b0ae3629 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -414,6 +414,16 @@ 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 always call ->prepare()/->finish() + * cache sync/flush on vb2 buffers. 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. + */ + 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];
On 27/11/2020 15:30, Sergey Senozhatsky wrote: > On (20/11/27 13:59), Hans Verkuil wrote: >> >> I think this needs a comment, basically explaining what you said in >> the commit log. It's not obvious from the code that this is a >> workaround. > > Something like this? Yes. BTW, wouldn't it be sufficient to change this code to: if (!q->allow_cache_hints && q->memory != VB2_MEMORY_DMABUF) { vb->need_cache_sync_on_prepare = 1; vb->need_cache_sync_on_finish = 1; } Or am I missing something? DVB drivers do not set this flag, and even for DVB cache sync shouldn't be needed for DMABUF, right? Regards, Hans > > --- > > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c > index 5499013cf82e..21b2b0ae3629 100644 > --- a/drivers/media/common/videobuf2/videobuf2-core.c > +++ b/drivers/media/common/videobuf2/videobuf2-core.c > @@ -414,6 +414,16 @@ 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 always call ->prepare()/->finish() > + * cache sync/flush on vb2 buffers. 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. > + */ > + 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]; >
On (20/11/27 15:56), Hans Verkuil wrote: > Yes. > > BTW, wouldn't it be sufficient to change this code to: > > if (!q->allow_cache_hints && q->memory != VB2_MEMORY_DMABUF) { > vb->need_cache_sync_on_prepare = 1; > vb->need_cache_sync_on_finish = 1; > } I think it would be sufficient. -ss
On Sat, Nov 28, 2020 at 1:35 AM Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote: > > On (20/11/27 15:56), Hans Verkuil wrote: > > Yes. > > > > BTW, wouldn't it be sufficient to change this code to: > > > > if (!q->allow_cache_hints && q->memory != VB2_MEMORY_DMABUF) { > > vb->need_cache_sync_on_prepare = 1; > > vb->need_cache_sync_on_finish = 1; > > } > > I think it would be sufficient. Does it matter at this point if allow_cache_hints is set or not? Best regards, Tomasz
On (20/11/28 01:50), Tomasz Figa wrote: > On Sat, Nov 28, 2020 at 1:35 AM Sergey Senozhatsky > <sergey.senozhatsky@gmail.com> wrote: > > > > On (20/11/27 15:56), Hans Verkuil wrote: > > > Yes. > > > > > > BTW, wouldn't it be sufficient to change this code to: > > > > > > if (!q->allow_cache_hints && q->memory != VB2_MEMORY_DMABUF) { > > > vb->need_cache_sync_on_prepare = 1; > > > vb->need_cache_sync_on_finish = 1; > > > } > > > > I think it would be sufficient. > > Does it matter at this point if allow_cache_hints is set or not? That's a good question. I'd say that it'll probably make sense to set need_cache_sync for as many buffers as possible, regardless the queue configuration (except for ->memory type), just to stay on the safe side. I can spin another patch version. -ss
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 5499013cf82e..14a26888a892 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -414,6 +414,8 @@ 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; + 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. Reported-by: Tomasz Figa <tfiga@chromium.org> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- drivers/media/common/videobuf2/videobuf2-core.c | 2 ++ 1 file changed, 2 insertions(+)