Re: [PATCH 1/2] media: videobuf2-dma-sg: fix vmap and vunmap callbacks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux