On Tue, May 16, 2023 at 7:50 PM Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote: > > Hi Michael, > > On Wed, May 10, 2023 at 11:25 PM Michael Grzeschik <mgr@xxxxxxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxx> > > >> > --- > > >> > 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@xxxxxxxxxxxxx > > > > 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@xxxxxxxxxxxx> 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 |
