Re: [PATCHv4 8/8] videobuf2: handle non-contiguous DMA allocations

[Sergey Senozhatsky <senozhatsky@chromium.org>]

Hi,

On (21/08/03 12:15), Hans Verkuil wrote:
> >  static void *vb2_dc_vaddr(struct vb2_buffer *vb, void *buf_priv)
> >  {
> >  	struct vb2_dc_buf *buf = buf_priv;
> > -	struct dma_buf_map map;
> > -	int ret;
> >  
> > -	if (!buf->vaddr && buf->db_attach) {
> > -		ret = dma_buf_vmap(buf->db_attach->dmabuf, &map);
> > -		buf->vaddr = ret ? NULL : map.vaddr;
> > +	if (buf->vaddr)
> > +		return buf->vaddr;
> > +
> > +	if (buf->db_attach) {
> > +		struct dma_buf_map map;
> > +
> > +		if (!dma_buf_vmap(buf->db_attach->dmabuf, &map))
> > +			buf->vaddr = map.vaddr;
> > +
> > +		return buf->vaddr;
> >  	}
> >  
> > +	if (!buf->coherent_mem)
> > +		buf->vaddr = dma_vmap_noncontiguous(buf->dev, buf->size,
> > +						    buf->dma_sgt);
> >  	return buf->vaddr;
> >  }
> 
> This function really needs a bunch of comments.
> 
> What I want to see here specifically is under which circumstances this function
> can return NULL.
> 
> - dma_buf_vmap returns an error
> - for non-coherent memory dma_vmap_noncontiguous returns an error
> - coherent memory with DMA_ATTR_NO_KERNEL_MAPPING set.

OK, I added some comments.

> In the latter case, if a buffer with coherent memory and DMA_ATTR_NO_KERNEL_MAPPING
> is exported as a dma_buf, and dma_buf_vmap is called by the importer of this dma-buf,
> what happens then? I think that in that case dma_buf_vmap should return an error?

Should we error out in vb2_dc_vaddr() in this case?

---

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index d4089d0b5ec5..e1d8ae1548fa 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -102,6 +102,9 @@ static void *vb2_dc_vaddr(struct vb2_buffer *vb, void *buf_priv)
        if (buf->db_attach) {
                struct dma_buf_map map;
 
+               if (WARN_ON(buf->attrs & DMA_ATTR_NO_KERNEL_MAPPING))
+                       return NULL;
+
                if (!dma_buf_vmap(buf->db_attach->dmabuf, &map))
                        buf->vaddr = map.vaddr;
 

---


[..]
> > @@ -362,7 +451,7 @@ static int vb2_dc_dmabuf_ops_vmap(struct dma_buf *dbuf, struct dma_buf_map *map)
> >  {
> >  	struct vb2_dc_buf *buf = dbuf->priv;
> >  
> > -	dma_buf_map_set_vaddr(map, buf->vaddr);
> > +	dma_buf_map_set_vaddr(map, vb2_dc_vaddr(buf->vb, buf));
> 
> vb2_dc_vaddr() can return NULL, shouldn't this function return an error in that case?

Done, thanks.

> BTW, looking at where vb2_plane_vaddr() is called in drivers I notice that most (all?)
> drivers do not check for NULL. Somewhat scary, to be honest. That's a separate issue, though.

I may have some time in the future and can add missing if-s to the
drivers.