| Message ID | 20221026184212.366897-1-m.grzeschik@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] media: videobuf2-dma-sg: fix vmap and vunmap callbacks | expand |
Marek, Can you review this? It looks good to me, but I wonder if videobuf2-dma-config.c has a similar problem. That looks to be mapping as well, but there is no vunmap. Michael, I have a comment below: On 26/10/2022 20:42, Michael Grzeschik wrote: > For dmabuf import users to be able to use the vaddr from another > videobuf2-dma-sg source, the exporter needs to set a proper vaddr on > vb2_dma_sg_dmabuf_ops_vmap callback. > > This patch adds vm_map_ram on map if buf->vaddr was not set, and also > adds the vb2_dma_sg_dmabuf_ops_vunmap function to remove the mapping > afterwards. > > Cc: stable <stable@kernel.org> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > --- > drivers/media/common/videobuf2/videobuf2-dma-sg.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > index fa69158a65b1fd..8d6e154bbbc8b0 100644 > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > @@ -496,11 +496,25 @@ static int vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf, > { > struct vb2_dma_sg_buf *buf = dbuf->priv; > > + if (!buf->vaddr) > + buf->vaddr = vm_map_ram(buf->pages, buf->num_pages, -1); 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. Any reason for not switching to vmap()? Regards, Hans > + > iosys_map_set_vaddr(map, buf->vaddr); > > return 0; > } > > +static void vb2_dma_sg_dmabuf_ops_vunmap(struct dma_buf *dbuf, > + struct iosys_map *map) > +{ > + struct vb2_dma_sg_buf *buf = dbuf->priv; > + > + if (buf->vaddr) > + vm_unmap_ram(buf->vaddr, buf->num_pages); > + > + buf->vaddr = NULL; > +} > + > static int vb2_dma_sg_dmabuf_ops_mmap(struct dma_buf *dbuf, > struct vm_area_struct *vma) > { > @@ -515,6 +529,7 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = { > .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access, > .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access, > .vmap = vb2_dma_sg_dmabuf_ops_vmap, > + .vunmap = vb2_dma_sg_dmabuf_ops_vunmap, > .mmap = vb2_dma_sg_dmabuf_ops_mmap, > .release = vb2_dma_sg_dmabuf_ops_release, > };
On Fri, Nov 04, 2022 at 04:55:25PM +0100, Hans Verkuil wrote: > Marek, > > Can you review this? It looks good to me, but I wonder if videobuf2-dma-config.c > has a similar problem. That looks to be mapping as well, but there is no vunmap. > > Michael, I have a comment below: > > On 26/10/2022 20:42, Michael Grzeschik wrote: > > For dmabuf import users to be able to use the vaddr from another > > videobuf2-dma-sg source, the exporter needs to set a proper vaddr on > > vb2_dma_sg_dmabuf_ops_vmap callback. > > > > This patch adds vm_map_ram on map if buf->vaddr was not set, and also > > adds the vb2_dma_sg_dmabuf_ops_vunmap function to remove the mapping > > afterwards. > > > > Cc: stable <stable@kernel.org> > > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> > > --- > > drivers/media/common/videobuf2/videobuf2-dma-sg.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > index fa69158a65b1fd..8d6e154bbbc8b0 100644 > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c > > @@ -496,11 +496,25 @@ static int vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf, > > { > > struct vb2_dma_sg_buf *buf = dbuf->priv; > > > > + if (!buf->vaddr) > > + buf->vaddr = vm_map_ram(buf->pages, buf->num_pages, -1); > > 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. Any reason for not switching to vmap()? vb2_dma_sg_vaddr() already uses vm_map_ram(), so that would need to be fixed too. I assume the code is copied from there. > > + > > iosys_map_set_vaddr(map, buf->vaddr); > > > > return 0; > > } > > > > +static void vb2_dma_sg_dmabuf_ops_vunmap(struct dma_buf *dbuf, > > + struct iosys_map *map) > > +{ > > + struct vb2_dma_sg_buf *buf = dbuf->priv; > > + > > + if (buf->vaddr) > > + vm_unmap_ram(buf->vaddr, buf->num_pages); > > + > > + buf->vaddr = NULL; > > +} > > + > > static int vb2_dma_sg_dmabuf_ops_mmap(struct dma_buf *dbuf, > > struct vm_area_struct *vma) > > { > > @@ -515,6 +529,7 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = { > > .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access, > > .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access, > > .vmap = vb2_dma_sg_dmabuf_ops_vmap, > > + .vunmap = vb2_dma_sg_dmabuf_ops_vunmap, > > .mmap = vb2_dma_sg_dmabuf_ops_mmap, > > .release = vb2_dma_sg_dmabuf_ops_release, > > };
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index fa69158a65b1fd..8d6e154bbbc8b0 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c @@ -496,11 +496,25 @@ static int vb2_dma_sg_dmabuf_ops_vmap(struct dma_buf *dbuf, { struct vb2_dma_sg_buf *buf = dbuf->priv; + if (!buf->vaddr) + buf->vaddr = vm_map_ram(buf->pages, buf->num_pages, -1); + iosys_map_set_vaddr(map, buf->vaddr); return 0; } +static void vb2_dma_sg_dmabuf_ops_vunmap(struct dma_buf *dbuf, + struct iosys_map *map) +{ + struct vb2_dma_sg_buf *buf = dbuf->priv; + + if (buf->vaddr) + vm_unmap_ram(buf->vaddr, buf->num_pages); + + buf->vaddr = NULL; +} + static int vb2_dma_sg_dmabuf_ops_mmap(struct dma_buf *dbuf, struct vm_area_struct *vma) { @@ -515,6 +529,7 @@ static const struct dma_buf_ops vb2_dma_sg_dmabuf_ops = { .begin_cpu_access = vb2_dma_sg_dmabuf_ops_begin_cpu_access, .end_cpu_access = vb2_dma_sg_dmabuf_ops_end_cpu_access, .vmap = vb2_dma_sg_dmabuf_ops_vmap, + .vunmap = vb2_dma_sg_dmabuf_ops_vunmap, .mmap = vb2_dma_sg_dmabuf_ops_mmap, .release = vb2_dma_sg_dmabuf_ops_release, };
For dmabuf import users to be able to use the vaddr from another videobuf2-dma-sg source, the exporter needs to set a proper vaddr on vb2_dma_sg_dmabuf_ops_vmap callback. This patch adds vm_map_ram on map if buf->vaddr was not set, and also adds the vb2_dma_sg_dmabuf_ops_vunmap function to remove the mapping afterwards. Cc: stable <stable@kernel.org> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de> --- drivers/media/common/videobuf2/videobuf2-dma-sg.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)