diff mbox series

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

Message ID 20221120234441.550908-1-m.grzeschik@pengutronix.de (mailing list archive)
State Changes Requested
Headers
Series media: videobuf2-dma-sg: use v{un,}map instead of vm_{un,}map_ram |

Commit Message

Michael Grzeschik Nov. 20, 2022, 11:44 p.m. UTC 
  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 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Andrzej Pietrasiewicz Nov. 21, 2022, 11:38 a.m. UTC | #1 
Hi Michael,

W dniu 21.11.2022 o 00:44, Michael Grzeschik pisze:
> 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.
> 

The comment is:

/**
  * vm_map_ram - map pages linearly into kernel virtual address (vmalloc space)
  * @pages: an array of pointers to the pages to be mapped
  * @count: number of pages
  * @node: prefer to allocate data structures on this node
  *
  * If you use this function for less than VMAP_MAX_ALLOC pages, it could be
  * faster than vmap so it's good.  But if you mix long-life and short-life
  * objects with vm_map_ram(), it could consume lots of address space through
  * fragmentation (especially on a 32bit machine).  You could see failures in
  * the end.  Please use this function for short-lived objects.
  *
  * Returns: a pointer to the address that has been mapped, or %NULL on failure
  */

As far as I understand the comment means:

- for allocations smaller than VMAP_MAX_ALLOC vm_map_ram() can be faster than
vmap()
- for larger allocations we don't know, maybe vmap() is faster, but the
comment doesn't say that vm_map_ram() cannot be used
- mixing long-life and short-life objects can have side effect of creating
fragmentation (which ultimately can lead to failures)
- the comment requests that the function is used for short-lived objects only
(which maybe is not the same thing as "large objects")

Can you expand your commit message?

Regards,

Andrzej


> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
>   drivers/media/common/videobuf2/videobuf2-dma-sg.c | 7 ++++---
>   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);
>   		}
>   	}
>
Hans Verkuil Nov. 24, 2022, 1:35 p.m. UTC | #2 
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.

Regards,

	Hans

>  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);
>  		}
>  	}
>
Tomasz Figa Dec. 2, 2022, 9:01 a.m. UTC | #3 
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.

Best regards,
Tomasz

>
> Regards,
>
>         Hans
>
> >  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);
> >               }
> >       }
> >
>
Michael Grzeschik May 10, 2023, 2:25 p.m. UTC | #4 
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.

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);
>> >               }
>> >       }
>> >
>>
>
Tomasz Figa May 16, 2023, 10:50 a.m. UTC | #5 
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 |
Tomasz Figa Nov. 1, 2023, 3:43 a.m. UTC | #6 
On Tue, May 16, 2023 at 7:50 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> 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.

Sorry, I forgot last time, so maybe it wasn't clear I'm good with this patch:

Acked-by: Tomasz Figa <tfiga@chromium.org>

Hans, will you pick it? Thanks!

>
> 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 |
Michael Grzeschik Nov. 15, 2023, 9:46 p.m. UTC | #7 
On Wed, Nov 01, 2023 at 12:43:25PM +0900, Tomasz Figa wrote:
>On Tue, May 16, 2023 at 7:50 PM Tomasz Figa <tfiga@chromium.org> wrote:
>>
>> 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.
>
>Sorry, I forgot last time, so maybe it wasn't clear I'm good with this patch:
>
>Acked-by: Tomasz Figa <tfiga@chromium.org>
>
>Hans, will you pick it? Thanks!

Gentle Ping!
Hans Verkuil Nov. 20, 2023, 10:50 a.m. UTC | #8 
Hi Michael,

On 15/11/2023 22:46, Michael Grzeschik wrote:
> On Wed, Nov 01, 2023 at 12:43:25PM +0900, Tomasz Figa wrote:
>> On Tue, May 16, 2023 at 7:50 PM Tomasz Figa <tfiga@chromium.org> wrote:
>>>
>>> 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.
>>
>> Sorry, I forgot last time, so maybe it wasn't clear I'm good with this patch:
>>
>> Acked-by: Tomasz Figa <tfiga@chromium.org>
>>
>> Hans, will you pick it? Thanks!
> 
> Gentle Ping!
> 

This patch is marked with "Changes Requested" in patchwork:

https://patchwork.linuxtv.org/project/linux-media/patch/20221120234441.550908-1-m.grzeschik@pengutronix.de/

Looking at the comments, there is a request to improve a comment and a request
from me to make the same change to videobuf2-vmalloc.c.

I have no problem with the change itself, it makes sense to use vmap.

In any case, a v2 is needed.

Regards,

	Hans
diff mbox series

Patch

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);
 		}
 	}