[v2,3/3] 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 dbuf_duplicated boolean 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 | 27 +++++++++++++++----
include/media/videobuf2-core.h | 3 +++
2 files changed, 25 insertions(+), 5 deletions(-)
Comments
On Wed, Apr 03, 2024 at 06:13:06PM +0900, Yunke Cao 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 dbuf_duplicated boolean 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 | 27 +++++++++++++++----
> include/media/videobuf2-core.h | 3 +++
> 2 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index a5368cef73bb..64fe3801b802 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->dbuf_duplicated) {
> + if (p->dbuf_mapped)
Side note: Now when I'm reading this code I'm starting to wonder if
dbuf_mapped really add any value here. Can we even have a situation when we
have p->dbuf != NULL, but p->dbuf_mapped == false?
> + 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;
>
> @@ -1380,6 +1383,19 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> __vb2_buf_dmabuf_put(vb);
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
Can we add a short comment here explaining that this is an optimization for
using the same DMA-buf for many planes?
Best regards,
Tomasz
> + for (i = 0; i < plane; ++i) {
> + if (planes[plane].dbuf == vb->planes[i].dbuf) {
> + vb->planes[plane].dbuf_duplicated = true;
> + vb->planes[plane].dbuf = vb->planes[i].dbuf;
> + vb->planes[plane].mem_priv = vb->planes[i].mem_priv;
> + break;
> + }
> + }
> +
> + /* There's no need to attach a duplicated dbuf. */
> + if (vb->planes[plane].dbuf_duplicated)
> + continue;
> +
> /* Acquire each plane's memory */
> mem_priv = call_ptr_memop(attach_dmabuf,
> vb,
> @@ -1392,6 +1408,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> goto err_put_dbuf;
> }
>
> + vb->planes[plane].dbuf_duplicated = false;
> vb->planes[plane].dbuf = planes[plane].dbuf;
> vb->planes[plane].mem_priv = mem_priv;
> }
> @@ -1406,7 +1423,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].dbuf_duplicated)
> 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..2484e7d2881d 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 dbuf_duplicated;
> unsigned int bytesused;
> unsigned int length;
> unsigned int min_length;
> --
> 2.44.0.478.gd926399ef9-goog
>
Hi Tomasz,
Thanks for the review.
On Fri, May 17, 2024 at 8:23 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Wed, Apr 03, 2024 at 06:13:06PM +0900, Yunke Cao 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 dbuf_duplicated boolean 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 | 27 +++++++++++++++----
> > include/media/videobuf2-core.h | 3 +++
> > 2 files changed, 25 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index a5368cef73bb..64fe3801b802 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->dbuf_duplicated) {
> > + if (p->dbuf_mapped)
>
> Side note: Now when I'm reading this code I'm starting to wonder if
> dbuf_mapped really add any value here. Can we even have a situation when we
> have p->dbuf != NULL, but p->dbuf_mapped == false?
>
Hmm, I think you are right. It seems we always do map_dmabuf after
attach_dma_buf.
> > + 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;
> >
> > @@ -1380,6 +1383,19 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> > __vb2_buf_dmabuf_put(vb);
> >
> > for (plane = 0; plane < vb->num_planes; ++plane) {
>
> Can we add a short comment here explaining that this is an optimization for
> using the same DMA-buf for many planes?
>
Sure, I will add it in v3.
Best,
Yunke
> Best regards,
> Tomasz
>
> > + for (i = 0; i < plane; ++i) {
> > + if (planes[plane].dbuf == vb->planes[i].dbuf) {
> > + vb->planes[plane].dbuf_duplicated = true;
> > + vb->planes[plane].dbuf = vb->planes[i].dbuf;
> > + vb->planes[plane].mem_priv = vb->planes[i].mem_priv;
> > + break;
> > + }
> > + }
> > +
> > + /* There's no need to attach a duplicated dbuf. */
> > + if (vb->planes[plane].dbuf_duplicated)
> > + continue;
> > +
> > /* Acquire each plane's memory */
> > mem_priv = call_ptr_memop(attach_dmabuf,
> > vb,
> > @@ -1392,6 +1408,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> > goto err_put_dbuf;
> > }
> >
> > + vb->planes[plane].dbuf_duplicated = false;
> > vb->planes[plane].dbuf = planes[plane].dbuf;
> > vb->planes[plane].mem_priv = mem_priv;
> > }
> > @@ -1406,7 +1423,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].dbuf_duplicated)
> > 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..2484e7d2881d 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 dbuf_duplicated;
> > unsigned int bytesused;
> > unsigned int length;
> > unsigned int min_length;
> > --
> > 2.44.0.478.gd926399ef9-goog
> >
@@ -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->dbuf_duplicated) {
+ 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;
@@ -1380,6 +1383,19 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
__vb2_buf_dmabuf_put(vb);
for (plane = 0; plane < vb->num_planes; ++plane) {
+ for (i = 0; i < plane; ++i) {
+ if (planes[plane].dbuf == vb->planes[i].dbuf) {
+ vb->planes[plane].dbuf_duplicated = true;
+ vb->planes[plane].dbuf = vb->planes[i].dbuf;
+ vb->planes[plane].mem_priv = vb->planes[i].mem_priv;
+ break;
+ }
+ }
+
+ /* There's no need to attach a duplicated dbuf. */
+ if (vb->planes[plane].dbuf_duplicated)
+ continue;
+
/* Acquire each plane's memory */
mem_priv = call_ptr_memop(attach_dmabuf,
vb,
@@ -1392,6 +1408,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
goto err_put_dbuf;
}
+ vb->planes[plane].dbuf_duplicated = false;
vb->planes[plane].dbuf = planes[plane].dbuf;
vb->planes[plane].mem_priv = mem_priv;
}
@@ -1406,7 +1423,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].dbuf_duplicated)
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 dbuf_duplicated;
unsigned int bytesused;
unsigned int length;
unsigned int min_length;