[RFC] media: videobuf2-core: attach once if multiple planes share the same dbuf

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(-)

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