Re: [PATCH] media: videobuf2-dma-sg: use v{un,}map instead of vm_{un,}map_ram

From: Tomasz Figa <tfiga@chromium.org>
To: Michael Grzeschik <mgr@pengutronix.de>
Cc: Hans Verkuil <hverkuil@xs4all.nl>,
	linux-media@vger.kernel.org, m.szyprowski@samsung.com,
	kernel@pengutronix.de,
	Andrzej Pietrasiewicz <andrzej.p@collabora.com>,
	Sergey Senozhatsky <senozhatsky@chromium.org>
Subject: Re: [PATCH] media: videobuf2-dma-sg: use v{un,}map instead of vm_{un,}map_ram
Date: Tue, 16 May 2023 19:50:19 +0900	[thread overview]
Message-ID: <CAAFQd5AmVBc6LQ1eyZ=WrvtLR4oSD4K0mMeszcuY12CK7djiUA@mail.gmail.com> (raw)
In-Reply-To: <20230510142509.GA14356@pengutronix.de>

Hi Michael,

On Wed, May 10, 2023 at 11:25 PM Michael Grzeschik <mgr@pengutronix.de> wrote:
>
> Sorry for the late comeback, however here are some thoughts.
>
> On Fri, Dec 02, 2022 at 06:01:02PM +0900, Tomasz Figa wrote:
> >On Thu, Nov 24, 2022 at 10:35 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 21/11/2022 00:44, Michael Grzeschik wrote:
> >> > The comments before the vm_map_ram function state that it should be used
> >> > for up to 256 KB only, and video buffers are definitely much larger. It
> >> > recommends using vmap in that case.
> >> >
> >> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> >> > ---
> >> >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 7 ++++---
> >>
> >> drivers/media/common/videobuf2/videobuf2-vmalloc.c uses it as well,
> >> probably also incorrectly. It makes sense to change that one as well.
> >
> >Comparing vm_map_ram() and vmap(..., VM_MAP, PAGE_KERNEL), for blocks
> >bigger than VMAP_MAX_ALLOC they're equivalent and for smaller blocks
> >the former should be faster, so I don't see what's wrong with the
> >current code.
>
> I got another comment on this from Andrzej Pietrasiewicz
> where he expands the comment on the use of vmap over vm_map_ram.
>
> https://lore.kernel.org/linux-media/64375ff4-dbbb-3d5b-eaf6-32d6780fd496@collabora.com
>
> As I understand this, we should probably update the vm_map_ram to vmap,
> due to the expectation that video buffers are long-living objects.
>
> Since there are some more places that would probably need to be updated
> if we should decide to use vmap over vm_map_ram in the whole
> videbuf2-* users, I would like to clarify on this before making
> a series.

Ah, I see. Thanks for the pointer.

VB2 buffers would usually require long-lived mappings, so based on
that, we should switch to vmap() indeed.

As a side note, not directly related to this patch, I wonder if we
should also call invalidate/flush_kernel_vmap_range() in
vb2_dma_sg_prepare/finish(). (In principle we shouldn't, but so far
our drivers don't explicitly call begin/end_cpu_access() and rely on
prepare/finish to handle the cache maintenance of the kernel
mapping...) Let me also add Sergey on CC for visibility.

Best regards,
Tomasz

>
> Regards,
> Michael
>
> >> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >> > index dcb8de5ab3e84a..e86621fba350f3 100644
> >> > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >> > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> >> > @@ -188,7 +188,7 @@ static void vb2_dma_sg_put(void *buf_priv)
> >> >               dma_unmap_sgtable(buf->dev, sgt, buf->dma_dir,
> >> >                                 DMA_ATTR_SKIP_CPU_SYNC);
> >> >               if (buf->vaddr)
> >> > -                     vm_unmap_ram(buf->vaddr, buf->num_pages);
> >> > +                     vunmap(buf->vaddr);
> >> >               sg_free_table(buf->dma_sgt);
> >> >               while (--i >= 0)
> >> >                       __free_page(buf->pages[i]);
> >> > @@ -289,7 +289,7 @@ static void vb2_dma_sg_put_userptr(void *buf_priv)
> >> >              __func__, buf->num_pages);
> >> >       dma_unmap_sgtable(buf->dev, sgt, buf->dma_dir, DMA_ATTR_SKIP_CPU_SYNC);
> >> >       if (buf->vaddr)
> >> > -             vm_unmap_ram(buf->vaddr, buf->num_pages);
> >> > +             vunmap(buf->vaddr);
> >> >       sg_free_table(buf->dma_sgt);
> >> >       if (buf->dma_dir == DMA_FROM_DEVICE ||
> >> >           buf->dma_dir == DMA_BIDIRECTIONAL)
> >> > @@ -312,7 +312,8 @@ static void *vb2_dma_sg_vaddr(struct vb2_buffer *vb, void *buf_priv)
> >> >                       ret = dma_buf_vmap(buf->db_attach->dmabuf, &map);
> >> >                       buf->vaddr = ret ? NULL : map.vaddr;
> >> >               } else {
> >> > -                     buf->vaddr = vm_map_ram(buf->pages, buf->num_pages, -1);
> >> > +                     buf->vaddr = vmap(buf->pages, buf->num_pages, VM_MAP,
> >> > +                                       PAGE_KERNEL);
> >> >               }
> >> >       }
> >> >
> >>
> >
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |