[v4,4/4] 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>
---
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
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;
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;
>
@@ -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,
@@ -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;