On Tue, Aug 17, 2021 at 8:57 PM Sergey Senozhatsky
[off-list ref] wrote:

Hi,

On (21/08/03 12:15), Hans Verkuil wrote:

quoted

quoted

 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.

quoted

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?

Yes, because there is no way to create a kernel mapping for buffer
allocated with dma_alloc_coherent() post-factum. Of course it's not
the case for dma_alloc_noncontiguous() for which we can create the
kernel mapping on demand.

Why here? It's the case for buffers imported _into_ vb2, not exported
from vb2 and imported to other users.

                if (!dma_buf_vmap(buf->db_attach->dmabuf, &map))
                        buf->vaddr = map.vaddr;


---


[..]

quoted

quoted

@@ -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.

quoted

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.