On Fri, Jun 25, 2021 at 12:10 PM Sergey Senozhatsky
[off-list ref] wrote:

Hi Hans,

On (21/06/17 16:56), Sergey Senozhatsky wrote:
[..]

quoted

static void *vb2_dc_vaddr(struct vb2_buffer *vb, void *buf_priv)
{
        struct vb2_dc_buf *buf = buf_priv;

        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;
}

And in vb2_dc_alloc functions set vaddr for !DMA_ATTR_NO_KERNEL_MAPPING
in both coherent and non-coherent. So that we probably can have less
branches when ->vaddr is NULL for one type of allocations, and is not
NULL for another.

I'd prefer if it stayed as is. This opportunistic mapping as in the
current revision is quite nice, because most of the drivers don't
bother to set DMA_ATTR_NO_KERNEL_MAPPING even if they don't need the
kernel mapping. Also, even if the driver itself doesn't need the
kernel mapping, we can still create one on demand if the DMA-buf
importer demands it from us.

quoted

static int vb2_dc_alloc_coherent(struct vb2_dc_buf *buf)
{
        struct vb2_queue *q = buf->vb->vb2_queue;

        buf->cookie = dma_alloc_attrs(buf->dev,
                                      buf->size,
                                      &buf->dma_addr,
                                      GFP_KERNEL | q->gfp_flags,
                                      buf->attrs);
        if (!buf->cookie)
                return -ENOMEM;

        if (q->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING)
                return 0;

        buf->vaddr = buf->cookie;
        return 0;
}

static int vb2_dc_alloc_non_coherent(struct vb2_dc_buf *buf)
{
        struct vb2_queue *q = buf->vb->vb2_queue;

        buf->dma_sgt = dma_alloc_noncontiguous(buf->dev,
                                               buf->size,
                                               buf->dma_dir,
                                               GFP_KERNEL | q->gfp_flags,
                                               buf->attrs);
        if (!buf->dma_sgt)
                return -ENOMEM;

        if (q->dma_attrs & DMA_ATTR_NO_KERNEL_MAPPING)
                return 0;

        buf->vaddr = dma_vmap_noncontiguous(buf->dev, buf->size, buf->dma_sgt);
        if (!buf->vaddr) {
                dma_free_noncontiguous(buf->dev, buf->size,
                                       buf->dma_sgt, buf->dma_addr);
                return -ENOMEM;
        }
        return 0;
}

I guess this should address the case when

"after allocating the buffer, the buffer is exported as a dma_buf and
another device calls dma_buf_ops vb2_dc_dmabuf_ops_vmap, which in turn
calls dma_buf_map_set_vaddr(map, buf->vaddr); with a NULL buf->vaddr"

Sorry, I fail to get what this is about. Where does this quote come from?

Because ->vaddr will not be NULL now after allocation for both coherent
and non-coherent buffers (modulo DMA_ATTR_NO_KERNEL_MAPPING requests).

What do you think?

Hans, any feedback on this? Thanks.

Best regards,
Tomasz