[RFC] media: videobuf2-core: attach once if multiple planes share the same dbuf
Commit Message
When multiple planes use the same dma buf, each plane will have its own dma
buf attachment and mapping. It is a waste of IOVA space.
This patch adds a duplicated_dbuf flag in vb2_plane. If a plane's dbuf
is the same as an existing plane, do not create another attachment and
mapping.
Signed-off-by: Yunke Cao <yunkec@chromium.org>
---
.../media/common/videobuf2/videobuf2-core.c | 30 +++++++++++++++----
include/media/videobuf2-core.h | 3 ++
2 files changed, 28 insertions(+), 5 deletions(-)
Comments
Hi Yunke,
On Mon, Apr 1, 2024 at 3:35 PM Yunke Cao <yunkec@chromium.org> wrote:
>
> When multiple planes use the same dma buf, each plane will have its own dma
> buf attachment and mapping. It is a waste of IOVA space.
>
> This patch adds a duplicated_dbuf flag in vb2_plane. If a plane's dbuf
> is the same as an existing plane, do not create another attachment and
> mapping.
>
> Signed-off-by: Yunke Cao <yunkec@chromium.org>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 30 +++++++++++++++----
> include/media/videobuf2-core.h | 3 ++
> 2 files changed, 28 insertions(+), 5 deletions(-)
>
Thanks a lot for the patch! Please take a look at my comments inline.
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index b6bf8f232f48..b03e058ef2b1 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -304,10 +304,13 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p)
> if (!p->mem_priv)
> return;
>
> - if (p->dbuf_mapped)
> - call_void_memop(vb, unmap_dmabuf, p->mem_priv);
> + if (!p->duplicated_dbuf) {
> + if (p->dbuf_mapped)
> + call_void_memop(vb, unmap_dmabuf, p->mem_priv);
> +
> + call_void_memop(vb, detach_dmabuf, p->mem_priv);
I wonder if we may want to reverse the iteration order in
__vb2_buf_dmabuf_put() so that we don't leave dangling pointers in
further planes when previous planes have their mem_priv freed.
> + }
>
> - call_void_memop(vb, detach_dmabuf, p->mem_priv);
> dma_buf_put(p->dbuf);
> p->mem_priv = NULL;
> p->dbuf = NULL;
> @@ -1327,7 +1330,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> struct vb2_plane planes[VB2_MAX_PLANES];
> struct vb2_queue *q = vb->vb2_queue;
> void *mem_priv;
> - unsigned int plane;
> + unsigned int plane, i;
> int ret = 0;
> bool reacquired = vb->planes[0].mem_priv == NULL;
>
> @@ -1383,6 +1386,22 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> vb->planes[plane].m.fd = 0;
> vb->planes[plane].data_offset = 0;
>
> + if (mem_priv && plane > 0) {
Is mem_priv the right thing to check for here? I think it would point
to the private data of the previous plane (i.e. plane - 1), but in the
loop below we may end up finding the match in an earlier plane (e.g.
plane - 2).
> + for (i = 0; i < plane; ++i) {
> + if (dbuf == vb->planes[i].dbuf) {
> + vb->planes[plane].duplicated_dbuf = true;
I guess we can set ...[plane].mem_priv to [i].mem_priv here.
> + break;
> + }
> + }
> + }
> +
> + /* There's no need to attach a duplicated dbuf. */
> + if (vb->planes[plane].duplicated_dbuf) {
> + vb->planes[plane].dbuf = dbuf;
> + vb->planes[plane].mem_priv = mem_priv;
I think this mem_priv would be the one from planes[plane-1] and not
necessarily the one with matching dbuf.
> + continue;
> + }
> +
> /* Acquire each plane's memory */
> mem_priv = call_ptr_memop(attach_dmabuf,
> vb,
> @@ -1396,6 +1415,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> goto err;
> }
>
> + vb->planes[plane].duplicated_dbuf = false;
> vb->planes[plane].dbuf = dbuf;
> vb->planes[plane].mem_priv = mem_priv;
> }
> @@ -1406,7 +1426,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> * userspace knows sooner rather than later if the dma-buf map fails.
> */
> for (plane = 0; plane < vb->num_planes; ++plane) {
> - if (vb->planes[plane].dbuf_mapped)
> + if (vb->planes[plane].dbuf_mapped || vb->planes[plane].duplicated_dbuf)
> continue;
>
> ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 8b86996b2719..5db781da2ebc 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -154,6 +154,8 @@ struct vb2_mem_ops {
> * @mem_priv: private data with this plane.
> * @dbuf: dma_buf - shared buffer object.
> * @dbuf_mapped: flag to show whether dbuf is mapped or not
> + * @duplicated_dbuf: boolean to show whether dbuf is duplicated with a
> + * previous plane of the buffer.
> * @bytesused: number of bytes occupied by data in the plane (payload).
> * @length: size of this plane (NOT the payload) in bytes. The maximum
> * valid size is MAX_UINT - PAGE_SIZE.
> @@ -179,6 +181,7 @@ struct vb2_plane {
> void *mem_priv;
> struct dma_buf *dbuf;
> unsigned int dbuf_mapped;
> + bool duplicated_dbuf;
nit: We kind of seem to use the dbuf_ prefix already, so how about
dbuf_duplicated? Or maybe dbuf_reused? Hmm, naming is always hard...
Best regards,
Tomasz
Hi Tomasz,
Thanks for the review!
On Tue, Apr 2, 2024 at 7:21 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> Hi Yunke,
>
> On Mon, Apr 1, 2024 at 3:35 PM Yunke Cao <yunkec@chromium.org> wrote:
> >
> > When multiple planes use the same dma buf, each plane will have its own dma
> > buf attachment and mapping. It is a waste of IOVA space.
> >
> > This patch adds a duplicated_dbuf flag in vb2_plane. If a plane's dbuf
> > is the same as an existing plane, do not create another attachment and
> > mapping.
> >
> > Signed-off-by: Yunke Cao <yunkec@chromium.org>
> > ---
> > .../media/common/videobuf2/videobuf2-core.c | 30 +++++++++++++++----
> > include/media/videobuf2-core.h | 3 ++
> > 2 files changed, 28 insertions(+), 5 deletions(-)
> >
>
> Thanks a lot for the patch! Please take a look at my comments inline.
>
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index b6bf8f232f48..b03e058ef2b1 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -304,10 +304,13 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p)
> > if (!p->mem_priv)
> > return;
> >
> > - if (p->dbuf_mapped)
> > - call_void_memop(vb, unmap_dmabuf, p->mem_priv);
> > + if (!p->duplicated_dbuf) {
> > + if (p->dbuf_mapped)
> > + call_void_memop(vb, unmap_dmabuf, p->mem_priv);
> > +
> > + call_void_memop(vb, detach_dmabuf, p->mem_priv);
>
> I wonder if we may want to reverse the iteration order in
> __vb2_buf_dmabuf_put() so that we don't leave dangling pointers in
> further planes when previous planes have their mem_priv freed.
>
The loop in __prepare_dmabuf() is of the same iteration order, so the
same will happen.
Is it still necessary to reverse the order in __vb2_buf_dmabuf_put()?
> > + }
> >
> > - call_void_memop(vb, detach_dmabuf, p->mem_priv);
> > dma_buf_put(p->dbuf);
> > p->mem_priv = NULL;
> > p->dbuf = NULL;
> > @@ -1327,7 +1330,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> > struct vb2_plane planes[VB2_MAX_PLANES];
> > struct vb2_queue *q = vb->vb2_queue;
> > void *mem_priv;
> > - unsigned int plane;
> > + unsigned int plane, i;
> > int ret = 0;
> > bool reacquired = vb->planes[0].mem_priv == NULL;
> >
> > @@ -1383,6 +1386,22 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> > vb->planes[plane].m.fd = 0;
> > vb->planes[plane].data_offset = 0;
> >
> > + if (mem_priv && plane > 0) {
>
> Is mem_priv the right thing to check for here? I think it would point
> to the private data of the previous plane (i.e. plane - 1), but in the
> loop below we may end up finding the match in an earlier plane (e.g.
> plane - 2).
>
> > + for (i = 0; i < plane; ++i) {
> > + if (dbuf == vb->planes[i].dbuf) {
> > + vb->planes[plane].duplicated_dbuf = true;
>
> I guess we can set ...[plane].mem_priv to [i].mem_priv here.
>
> > + break;
> > + }
> > + }
> > + }
> > +
> > + /* There's no need to attach a duplicated dbuf. */
> > + if (vb->planes[plane].duplicated_dbuf) {
> > + vb->planes[plane].dbuf = dbuf;
> > + vb->planes[plane].mem_priv = mem_priv;
>
> I think this mem_priv would be the one from planes[plane-1] and not
> necessarily the one with matching dbuf.
Thanks for catching the error. Will fix it in the next version.
>
>
> > + continue;
> > + }
> > +
> > /* Acquire each plane's memory */
> > mem_priv = call_ptr_memop(attach_dmabuf,
> > vb,
> > @@ -1396,6 +1415,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> > goto err;
> > }
> >
> > + vb->planes[plane].duplicated_dbuf = false;
> > vb->planes[plane].dbuf = dbuf;
> > vb->planes[plane].mem_priv = mem_priv;
> > }
> > @@ -1406,7 +1426,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> > * userspace knows sooner rather than later if the dma-buf map fails.
> > */
> > for (plane = 0; plane < vb->num_planes; ++plane) {
> > - if (vb->planes[plane].dbuf_mapped)
> > + if (vb->planes[plane].dbuf_mapped || vb->planes[plane].duplicated_dbuf)
> > continue;
> >
> > ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index 8b86996b2719..5db781da2ebc 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -154,6 +154,8 @@ struct vb2_mem_ops {
> > * @mem_priv: private data with this plane.
> > * @dbuf: dma_buf - shared buffer object.
> > * @dbuf_mapped: flag to show whether dbuf is mapped or not
> > + * @duplicated_dbuf: boolean to show whether dbuf is duplicated with a
> > + * previous plane of the buffer.
> > * @bytesused: number of bytes occupied by data in the plane (payload).
> > * @length: size of this plane (NOT the payload) in bytes. The maximum
> > * valid size is MAX_UINT - PAGE_SIZE.
> > @@ -179,6 +181,7 @@ struct vb2_plane {
> > void *mem_priv;
> > struct dma_buf *dbuf;
> > unsigned int dbuf_mapped;
> > + bool duplicated_dbuf;
>
> nit: We kind of seem to use the dbuf_ prefix already, so how about
> dbuf_duplicated? Or maybe dbuf_reused? Hmm, naming is always hard...
>
dbuf_duplicated sounds good to me.
Best,
Yunke
> Best regards,
> Tomasz
@@ -304,10 +304,13 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p)
if (!p->mem_priv)
return;
- if (p->dbuf_mapped)
- call_void_memop(vb, unmap_dmabuf, p->mem_priv);
+ if (!p->duplicated_dbuf) {
+ if (p->dbuf_mapped)
+ call_void_memop(vb, unmap_dmabuf, p->mem_priv);
+
+ call_void_memop(vb, detach_dmabuf, p->mem_priv);
+ }
- call_void_memop(vb, detach_dmabuf, p->mem_priv);
dma_buf_put(p->dbuf);
p->mem_priv = NULL;
p->dbuf = NULL;
@@ -1327,7 +1330,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
struct vb2_plane planes[VB2_MAX_PLANES];
struct vb2_queue *q = vb->vb2_queue;
void *mem_priv;
- unsigned int plane;
+ unsigned int plane, i;
int ret = 0;
bool reacquired = vb->planes[0].mem_priv == NULL;
@@ -1383,6 +1386,22 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
vb->planes[plane].m.fd = 0;
vb->planes[plane].data_offset = 0;
+ if (mem_priv && plane > 0) {
+ for (i = 0; i < plane; ++i) {
+ if (dbuf == vb->planes[i].dbuf) {
+ vb->planes[plane].duplicated_dbuf = true;
+ break;
+ }
+ }
+ }
+
+ /* There's no need to attach a duplicated dbuf. */
+ if (vb->planes[plane].duplicated_dbuf) {
+ vb->planes[plane].dbuf = dbuf;
+ vb->planes[plane].mem_priv = mem_priv;
+ continue;
+ }
+
/* Acquire each plane's memory */
mem_priv = call_ptr_memop(attach_dmabuf,
vb,
@@ -1396,6 +1415,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
goto err;
}
+ vb->planes[plane].duplicated_dbuf = false;
vb->planes[plane].dbuf = dbuf;
vb->planes[plane].mem_priv = mem_priv;
}
@@ -1406,7 +1426,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
* userspace knows sooner rather than later if the dma-buf map fails.
*/
for (plane = 0; plane < vb->num_planes; ++plane) {
- if (vb->planes[plane].dbuf_mapped)
+ if (vb->planes[plane].dbuf_mapped || vb->planes[plane].duplicated_dbuf)
continue;
ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
@@ -154,6 +154,8 @@ struct vb2_mem_ops {
* @mem_priv: private data with this plane.
* @dbuf: dma_buf - shared buffer object.
* @dbuf_mapped: flag to show whether dbuf is mapped or not
+ * @duplicated_dbuf: boolean to show whether dbuf is duplicated with a
+ * previous plane of the buffer.
* @bytesused: number of bytes occupied by data in the plane (payload).
* @length: size of this plane (NOT the payload) in bytes. The maximum
* valid size is MAX_UINT - PAGE_SIZE.
@@ -179,6 +181,7 @@ struct vb2_plane {
void *mem_priv;
struct dma_buf *dbuf;
unsigned int dbuf_mapped;
+ bool duplicated_dbuf;
unsigned int bytesused;
unsigned int length;
unsigned int min_length;