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@xxxxxxxxxx>
> > Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
> > ---
> > 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,
> > };
--
Regards,
Laurent Pinchart
