Hi Yunke,
On Mon, Apr 1, 2024 at 3:35 PM Yunke Cao <yunkec@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
> .../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.
> + }
>
> - 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.
> + 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...
Best regards,
Tomasz
