From: Yunke Cao <yunkec@chromium.org>
To: Tomasz Figa <tfiga@chromium.org>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
linux-media@vger.kernel.org
Subject: Re: [PATCH v2 3/3] media: videobuf2-core: attach once if multiple planes share the same dbuf
Date: Mon, 20 May 2024 11:19:27 +0900 [thread overview]
Message-ID: <CAEDqmY4tfzoAvGmqypaHjb8mdbe9zfgTZLcGBZ4A20MN=gYLaA@mail.gmail.com> (raw)
In-Reply-To: <pz7etaaqk2sxuchbnrcj3efc57pmprbi4amjkx3ltrsqaz4jpo@rws6xfkyoq7j>
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
> >
next prev parent reply other threads:[~2024-05-20 2:19 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-03 9:13 [PATCH v2 0/3] media: videobuf2-core: attach once if multiple planes share the same dbuf Yunke Cao
2024-04-03 9:13 ` [PATCH v2 1/3] media: videobuf2-core: release all planes first in __prepare_dmabuf() Yunke Cao
2024-05-17 11:11 ` Tomasz Figa
2024-05-30 4:13 ` Yunke Cao
2024-04-03 9:13 ` [PATCH v2 2/3] media: videobuf2-core: reverse the iteration order in __vb2_buf_dmabuf_put Yunke Cao
2024-04-24 10:24 ` Hans Verkuil
2024-05-17 11:14 ` Tomasz Figa
2024-04-03 9:13 ` [PATCH v2 3/3] media: videobuf2-core: attach once if multiple planes share the same dbuf Yunke Cao
2024-05-17 11:23 ` Tomasz Figa
2024-05-20 2:19 ` Yunke Cao [this message]
2024-04-24 10:25 ` [PATCH v2 0/3] " Hans Verkuil
2024-05-08 16:02 ` Tomasz Figa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEDqmY4tfzoAvGmqypaHjb8mdbe9zfgTZLcGBZ4A20MN=gYLaA@mail.gmail.com' \
--to=yunkec@chromium.org \
--cc=linux-media@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=mchehab@kernel.org \
--cc=tfiga@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).