| Message ID | 20221120234441.550908-1-m.grzeschik@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: videobuf2-dma-sg: use v{un,}map instead of vm_{un,}map_ram | expand |
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); > } > } >
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); > } > } >
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); > > } > > } > > >
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); >> > } >> > } >> > >> >
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 |
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 |
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!
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 --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); } }
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(-)