[v4,4/4] media: videobuf2-core: attach once if multiple planes share the same dbuf

Message ID 20240614073702.316810-5-yunkec@chromium.org (mailing list archive)
State Superseded
Delegated to: Hans Verkuil
Headers
Series media: videobuf2-core: attach once if multiple planes share the same dbuf |

Commit Message

Yunke Cao June 14, 2024, 7:37 a.m. UTC
  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>
---
v4
- Only set dbuf_duplicated when alloc_devs are equal.

v3
- Adjust the patch according to the previous patches to resolve conflicts.
- Add comment to explain the purpose of the change.

v2
- Separate out the refactor changes out to previous patches.
- Fix mem_priv check.
---
 .../media/common/videobuf2/videobuf2-core.c   | 30 ++++++++++++++++---
 include/media/videobuf2-core.h                |  3 ++
 2 files changed, 29 insertions(+), 4 deletions(-)
  

Comments

Hans Verkuil Aug. 12, 2024, 8:06 a.m. UTC | #1
On 14/06/2024 09:37, 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.

I'm getting two documentation warnings:

include/media/videobuf2-core.h:194: warning: Function parameter or struct member 'dbuf_duplicated' not described in 'vb2_plane'
include/media/videobuf2-core.h:194: warning: Excess struct member 'duplicated_dbuf' description in 'vb2_plane'
2 warnings as Errors

Clearly a typo.

So please post a v5 to fix this issue as well.

Regards,

	Hans

> 
> Signed-off-by: Yunke Cao <yunkec@chromium.org>
> ---
> v4
> - Only set dbuf_duplicated when alloc_devs are equal.
> 
> v3
> - Adjust the patch according to the previous patches to resolve conflicts.
> - Add comment to explain the purpose of the change.
> 
> v2
> - Separate out the refactor changes out to previous patches.
> - Fix mem_priv check.
> ---
>  .../media/common/videobuf2/videobuf2-core.c   | 30 ++++++++++++++++---
>  include/media/videobuf2-core.h                |  3 ++
>  2 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index cbc8928f0418..c19f1df3a4d2 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)
> +			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;
> @@ -316,6 +319,7 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p)
>  	p->length = 0;
>  	p->m.fd = 0;
>  	p->data_offset = 0;
> +	p->dbuf_duplicated = false;
>  }
>  
>  /*
> @@ -1374,7 +1378,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;
>  
> @@ -1427,6 +1431,24 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>  		}
>  
>  		for (plane = 0; plane < vb->num_planes; ++plane) {
> +			/*
> +			 * This is an optimization to reduce dma_buf attachment/mapping.
> +			 * When the same dma_buf is used for multiple planes, there is no need
> +			 * to create duplicated attachments.
> +			 */
> +			for (i = 0; i < plane; ++i) {
> +				if (planes[plane].dbuf == vb->planes[i].dbuf &&
> +				    q->alloc_devs[plane] == q->alloc_devs[i]) {
> +					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;
> +				}
> +			}
> +
> +			if (vb->planes[plane].dbuf_duplicated)
> +				continue;
> +
>  			/* Acquire each plane's memory */
>  			mem_priv = call_ptr_memop(attach_dmabuf,
>  						  vb,
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 955237ac503d..053ced60595f 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;
  
Yunke Cao Aug. 13, 2024, 5:19 a.m. UTC | #2
Hi Hans,

On Mon, Aug 12, 2024 at 5:06 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 14/06/2024 09:37, 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.
>
> I'm getting two documentation warnings:
>
> include/media/videobuf2-core.h:194: warning: Function parameter or struct member 'dbuf_duplicated' not described in 'vb2_plane'
> include/media/videobuf2-core.h:194: warning: Excess struct member 'duplicated_dbuf' description in 'vb2_plane'
> 2 warnings as Errors
>
> Clearly a typo.
>
> So please post a v5 to fix this issue as well.

Oops sorry! I will send a v5 soon.

Thank you,
Yunke

>
> Regards,
>
>         Hans
>
> >
> > Signed-off-by: Yunke Cao <yunkec@chromium.org>
> > ---
> > v4
> > - Only set dbuf_duplicated when alloc_devs are equal.
> >
> > v3
> > - Adjust the patch according to the previous patches to resolve conflicts.
> > - Add comment to explain the purpose of the change.
> >
> > v2
> > - Separate out the refactor changes out to previous patches.
> > - Fix mem_priv check.
> > ---
> >  .../media/common/videobuf2/videobuf2-core.c   | 30 ++++++++++++++++---
> >  include/media/videobuf2-core.h                |  3 ++
> >  2 files changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index cbc8928f0418..c19f1df3a4d2 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)
> > +                     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;
> > @@ -316,6 +319,7 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p)
> >       p->length = 0;
> >       p->m.fd = 0;
> >       p->data_offset = 0;
> > +     p->dbuf_duplicated = false;
> >  }
> >
> >  /*
> > @@ -1374,7 +1378,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;
> >
> > @@ -1427,6 +1431,24 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> >               }
> >
> >               for (plane = 0; plane < vb->num_planes; ++plane) {
> > +                     /*
> > +                      * This is an optimization to reduce dma_buf attachment/mapping.
> > +                      * When the same dma_buf is used for multiple planes, there is no need
> > +                      * to create duplicated attachments.
> > +                      */
> > +                     for (i = 0; i < plane; ++i) {
> > +                             if (planes[plane].dbuf == vb->planes[i].dbuf &&
> > +                                 q->alloc_devs[plane] == q->alloc_devs[i]) {
> > +                                     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;
> > +                             }
> > +                     }
> > +
> > +                     if (vb->planes[plane].dbuf_duplicated)
> > +                             continue;
> > +
> >                       /* Acquire each plane's memory */
> >                       mem_priv = call_ptr_memop(attach_dmabuf,
> >                                                 vb,
> > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> > index 955237ac503d..053ced60595f 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;
>
  

Patch

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index cbc8928f0418..c19f1df3a4d2 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)
+			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;
@@ -316,6 +319,7 @@  static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p)
 	p->length = 0;
 	p->m.fd = 0;
 	p->data_offset = 0;
+	p->dbuf_duplicated = false;
 }
 
 /*
@@ -1374,7 +1378,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;
 
@@ -1427,6 +1431,24 @@  static int __prepare_dmabuf(struct vb2_buffer *vb)
 		}
 
 		for (plane = 0; plane < vb->num_planes; ++plane) {
+			/*
+			 * This is an optimization to reduce dma_buf attachment/mapping.
+			 * When the same dma_buf is used for multiple planes, there is no need
+			 * to create duplicated attachments.
+			 */
+			for (i = 0; i < plane; ++i) {
+				if (planes[plane].dbuf == vb->planes[i].dbuf &&
+				    q->alloc_devs[plane] == q->alloc_devs[i]) {
+					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;
+				}
+			}
+
+			if (vb->planes[plane].dbuf_duplicated)
+				continue;
+
 			/* Acquire each plane's memory */
 			mem_priv = call_ptr_memop(attach_dmabuf,
 						  vb,
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 955237ac503d..053ced60595f 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;