Hi Hans,
On Mon, Aug 12, 2024 at 5:06 PM Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxxxxxxx>
> > ---
> > 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;
>
