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.
