| Message ID | 20200204025641.218376-3-senozhatsky@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement V4L2_BUF_FLAG_NO_CACHE_* flags | expand |
On (20/02/04 11:56), Sergey Senozhatsky wrote: > +static void set_buffer_cache_hints(struct vb2_queue *q, > + struct vb2_buffer *vb, > + struct v4l2_buffer *b) > +{ > + /* > + * DMA exporter should take care of cache syncs, so we can avoid > + * explicit ->prepare()/->finish() syncs. For other ->memory types > + * we always need ->prepare() or/and ->finish() cache sync. > + */ > + if (q->memory == VB2_MEMORY_DMABUF) { > + vb->need_cache_sync_on_finish = 0; > + vb->need_cache_sync_on_prepare = 0; > + return; > + } > + > + if (!q->allow_cache_hints) > + return; > + > + vb->need_cache_sync_on_prepare = 1; > + /* > + * ->finish() cache sync can be avoided when queue direction is > + * TO_DEVICE. > + */ > + if (q->dma_dir != DMA_TO_DEVICE) > + vb->need_cache_sync_on_finish = 1; > + else > + vb->need_cache_sync_on_finish = 0; > + > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE) > + vb->need_cache_sync_on_finish = 0; > + > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN) > + vb->need_cache_sync_on_prepare = 0; > +} Last minute changes (tm), sorry. This is not right. ====8<====8<==== From: Sergey Senozhatsky <senozhatsky@chromium.org> Subject: [PATCH] videobuf2: handle V4L2 buffer cache flags Set video buffer cache management flags corresponding to V4L2 cache flags. Both ->prepare() and ->finish() cache management hints should be passed during this stage (buffer preparation), because there is no other way for user-space to skip ->finish() cache flush. There are two possible alternative approaches: - The first one is to move cache sync from ->finish() to dqbuf(). But this breaks some drivers, that need to fix-up buffers before dequeueing them. - The second one is to move ->finish() call from ->done() to dqbuf. Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- .../media/common/videobuf2/videobuf2-v4l2.c | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index eb5d5db96552..8ef57496b34a 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -337,6 +337,41 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b return 0; } +static void set_buffer_cache_hints(struct vb2_queue *q, + struct vb2_buffer *vb, + struct v4l2_buffer *b) +{ + /* + * DMA exporter should take care of cache syncs, so we can avoid + * explicit ->prepare()/->finish() syncs. For other ->memory types + * we always need ->prepare() or/and ->finish() cache sync. + */ + if (q->memory == VB2_MEMORY_DMABUF) { + vb->need_cache_sync_on_finish = 0; + vb->need_cache_sync_on_prepare = 0; + return; + } + + vb->need_cache_sync_on_prepare = 1; + vb->need_cache_sync_on_finish = 1; + + if (!q->allow_cache_hints) + return; + + /* + * ->finish() cache sync can be avoided when queue direction is + * TO_DEVICE. + */ + if (q->dma_dir == DMA_TO_DEVICE) + vb->need_cache_sync_on_finish = 0; + + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE) + vb->need_cache_sync_on_finish = 0; + + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN) + vb->need_cache_sync_on_prepare = 0; +} + static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *mdev, struct v4l2_buffer *b, bool is_prepare, struct media_request **p_req) @@ -381,6 +416,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md } if (!vb->prepared) { + set_buffer_cache_hints(q, vb, b); /* Copy relevant information provided by the userspace */ memset(vbuf->planes, 0, sizeof(vbuf->planes[0]) * vb->num_planes);
On 2/4/20 3:56 AM, Sergey Senozhatsky wrote: > Set video buffer cache management flags corresponding to V4L2 cache > flags. > > Both ->prepare() and ->finish() cache management hints should be > passed during this stage (buffer preparation), because there is no > other way for user-space to skip ->finish() cache flush. > > There are two possible alternative approaches: > - The first one is to move cache sync from ->finish() to dqbuf(). > But this breaks some drivers, that need to fix-up buffers before > dequeueing them. > > - The second one is to move ->finish() call from ->done() to dqbuf. > > Change-Id: I3bef1d1fb93a5fba290ce760eaeecdc8e7d6885a > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > .../media/common/videobuf2/videobuf2-v4l2.c | 36 +++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index eb5d5db96552..2da06a2ad6c4 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -337,6 +337,41 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b > return 0; > } > > +static void set_buffer_cache_hints(struct vb2_queue *q, > + struct vb2_buffer *vb, > + struct v4l2_buffer *b) > +{ > + /* > + * DMA exporter should take care of cache syncs, so we can avoid > + * explicit ->prepare()/->finish() syncs. For other ->memory types > + * we always need ->prepare() or/and ->finish() cache sync. > + */ > + if (q->memory == VB2_MEMORY_DMABUF) { > + vb->need_cache_sync_on_finish = 0; > + vb->need_cache_sync_on_prepare = 0; > + return; > + } > + > + if (!q->allow_cache_hints) > + return; > + > + vb->need_cache_sync_on_prepare = 1; This needs a comment explaining why prepare is set to 1 by default. I remember we discussed this earlier, and the conclusion of that discussion needs to be documented here in a comment. Regards, Hans > + /* > + * ->finish() cache sync can be avoided when queue direction is > + * TO_DEVICE. > + */ > + if (q->dma_dir != DMA_TO_DEVICE) > + vb->need_cache_sync_on_finish = 1; > + else > + vb->need_cache_sync_on_finish = 0; > + > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE) > + vb->need_cache_sync_on_finish = 0; > + > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN) > + vb->need_cache_sync_on_prepare = 0; > +} > + > static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *mdev, > struct v4l2_buffer *b, bool is_prepare, > struct media_request **p_req) > @@ -381,6 +416,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md > } > > if (!vb->prepared) { > + set_buffer_cache_hints(q, vb, b); > /* Copy relevant information provided by the userspace */ > memset(vbuf->planes, 0, > sizeof(vbuf->planes[0]) * vb->num_planes); >
On (20/02/19 09:07), Hans Verkuil wrote: [..] > > +static void set_buffer_cache_hints(struct vb2_queue *q, > > + struct vb2_buffer *vb, > > + struct v4l2_buffer *b) > > +{ > > + /* > > + * DMA exporter should take care of cache syncs, so we can avoid > > + * explicit ->prepare()/->finish() syncs. For other ->memory types > > + * we always need ->prepare() or/and ->finish() cache sync. > > + */ > > + if (q->memory == VB2_MEMORY_DMABUF) { > > + vb->need_cache_sync_on_finish = 0; > > + vb->need_cache_sync_on_prepare = 0; > > + return; > > + } > > + > > + if (!q->allow_cache_hints) > > + return; > > + > > + vb->need_cache_sync_on_prepare = 1; > > This needs a comment explaining why prepare is set to 1 by default. I remember > we discussed this earlier, and the conclusion of that discussion needs to be > documented here in a comment. Please ignore this patch. There is a follow up which sets _both_ flags by default. The purpose is to preserve the existing behaviour, we can do all sorts of incremental changes (clear flags in more cases, etc.) later on. Do you want me to document this in the code? -ss
On 2/19/20 9:13 AM, Sergey Senozhatsky wrote: > On (20/02/19 09:07), Hans Verkuil wrote: > [..] >>> +static void set_buffer_cache_hints(struct vb2_queue *q, >>> + struct vb2_buffer *vb, >>> + struct v4l2_buffer *b) >>> +{ >>> + /* >>> + * DMA exporter should take care of cache syncs, so we can avoid >>> + * explicit ->prepare()/->finish() syncs. For other ->memory types >>> + * we always need ->prepare() or/and ->finish() cache sync. >>> + */ >>> + if (q->memory == VB2_MEMORY_DMABUF) { >>> + vb->need_cache_sync_on_finish = 0; >>> + vb->need_cache_sync_on_prepare = 0; >>> + return; >>> + } >>> + >>> + if (!q->allow_cache_hints) >>> + return; >>> + >>> + vb->need_cache_sync_on_prepare = 1; >> >> This needs a comment explaining why prepare is set to 1 by default. I remember >> we discussed this earlier, and the conclusion of that discussion needs to be >> documented here in a comment. > > Please ignore this patch. There is a follow up which sets _both_ > flags by default. The purpose is to preserve the existing behaviour, > we can do all sorts of incremental changes (clear flags in more cases, > etc.) later on. Do you want me to document this in the code? Yes please! Regards, Hans > > -ss >
On 2/4/20 3:56 AM, Sergey Senozhatsky wrote: > Set video buffer cache management flags corresponding to V4L2 cache > flags. > > Both ->prepare() and ->finish() cache management hints should be > passed during this stage (buffer preparation), because there is no > other way for user-space to skip ->finish() cache flush. > > There are two possible alternative approaches: > - The first one is to move cache sync from ->finish() to dqbuf(). > But this breaks some drivers, that need to fix-up buffers before > dequeueing them. > > - The second one is to move ->finish() call from ->done() to dqbuf. > > Change-Id: I3bef1d1fb93a5fba290ce760eaeecdc8e7d6885a > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > --- > .../media/common/videobuf2/videobuf2-v4l2.c | 36 +++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c > index eb5d5db96552..2da06a2ad6c4 100644 > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c > @@ -337,6 +337,41 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b > return 0; > } > > +static void set_buffer_cache_hints(struct vb2_queue *q, > + struct vb2_buffer *vb, > + struct v4l2_buffer *b) > +{ > + /* > + * DMA exporter should take care of cache syncs, so we can avoid > + * explicit ->prepare()/->finish() syncs. For other ->memory types > + * we always need ->prepare() or/and ->finish() cache sync. > + */ > + if (q->memory == VB2_MEMORY_DMABUF) { > + vb->need_cache_sync_on_finish = 0; > + vb->need_cache_sync_on_prepare = 0; > + return; > + } > + > + if (!q->allow_cache_hints) If q->allow_cache_hints is 0, then if userspace attempts to set these flags in v4l2_buffer, they should be cleared. That's to indicate to userspace that these flags won't work. That should be done in vb2_fill_vb2_v4l2_buffer(). Regards, Hans > + return; > + > + vb->need_cache_sync_on_prepare = 1; > + /* > + * ->finish() cache sync can be avoided when queue direction is > + * TO_DEVICE. > + */ > + if (q->dma_dir != DMA_TO_DEVICE) > + vb->need_cache_sync_on_finish = 1; > + else > + vb->need_cache_sync_on_finish = 0; > + > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE) > + vb->need_cache_sync_on_finish = 0; > + > + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN) > + vb->need_cache_sync_on_prepare = 0; > +} > + > static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *mdev, > struct v4l2_buffer *b, bool is_prepare, > struct media_request **p_req) > @@ -381,6 +416,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md > } > > if (!vb->prepared) { > + set_buffer_cache_hints(q, vb, b); > /* Copy relevant information provided by the userspace */ > memset(vbuf->planes, 0, > sizeof(vbuf->planes[0]) * vb->num_planes); >
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c index eb5d5db96552..2da06a2ad6c4 100644 --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c @@ -337,6 +337,41 @@ static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b return 0; } +static void set_buffer_cache_hints(struct vb2_queue *q, + struct vb2_buffer *vb, + struct v4l2_buffer *b) +{ + /* + * DMA exporter should take care of cache syncs, so we can avoid + * explicit ->prepare()/->finish() syncs. For other ->memory types + * we always need ->prepare() or/and ->finish() cache sync. + */ + if (q->memory == VB2_MEMORY_DMABUF) { + vb->need_cache_sync_on_finish = 0; + vb->need_cache_sync_on_prepare = 0; + return; + } + + if (!q->allow_cache_hints) + return; + + vb->need_cache_sync_on_prepare = 1; + /* + * ->finish() cache sync can be avoided when queue direction is + * TO_DEVICE. + */ + if (q->dma_dir != DMA_TO_DEVICE) + vb->need_cache_sync_on_finish = 1; + else + vb->need_cache_sync_on_finish = 0; + + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_INVALIDATE) + vb->need_cache_sync_on_finish = 0; + + if (b->flags & V4L2_BUF_FLAG_NO_CACHE_CLEAN) + vb->need_cache_sync_on_prepare = 0; +} + static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *mdev, struct v4l2_buffer *b, bool is_prepare, struct media_request **p_req) @@ -381,6 +416,7 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct media_device *md } if (!vb->prepared) { + set_buffer_cache_hints(q, vb, b); /* Copy relevant information provided by the userspace */ memset(vbuf->planes, 0, sizeof(vbuf->planes[0]) * vb->num_planes);
Set video buffer cache management flags corresponding to V4L2 cache flags. Both ->prepare() and ->finish() cache management hints should be passed during this stage (buffer preparation), because there is no other way for user-space to skip ->finish() cache flush. There are two possible alternative approaches: - The first one is to move cache sync from ->finish() to dqbuf(). But this breaks some drivers, that need to fix-up buffers before dequeueing them. - The second one is to move ->finish() call from ->done() to dqbuf. Change-Id: I3bef1d1fb93a5fba290ce760eaeecdc8e7d6885a Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> --- .../media/common/videobuf2/videobuf2-v4l2.c | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+)