| Message ID | 20210727070517.443167-9-senozhatsky@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | videobuf2: support new noncontiguous DMA API | expand |
On 27/07/2021 09:05, Sergey Senozhatsky wrote: > This adds support for new noncontiguous DMA API, which for -> for the > requires allocators to have two execution branches: one > for the current API, and one for the new one. > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > Acked-by: Christoph Hellwig <hch@lst.de> > --- > .../common/videobuf2/videobuf2-dma-contig.c | 142 +++++++++++++++--- > 1 file changed, 117 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > index 1e218bc440c6..10f73e27d694 100644 > --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c > +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > @@ -17,6 +17,7 @@ > #include <linux/sched.h> > #include <linux/slab.h> > #include <linux/dma-mapping.h> > +#include <linux/highmem.h> > > #include <media/videobuf2-v4l2.h> > #include <media/videobuf2-dma-contig.h> > @@ -42,6 +43,7 @@ struct vb2_dc_buf { > struct dma_buf_attachment *db_attach; > > struct vb2_buffer *vb; > + bool coherent_mem; I think that this as well should be renamed to non_coherent_mem. > }; > > /*********************************************/ > @@ -78,14 +80,22 @@ static void *vb2_dc_cookie(struct vb2_buffer *vb, void *buf_priv) > 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; > } > > @@ -101,13 +111,26 @@ static void vb2_dc_prepare(void *buf_priv) > struct vb2_dc_buf *buf = buf_priv; > struct sg_table *sgt = buf->dma_sgt; > > + /* This takes care of DMABUF and user-enforced cache sync hint */ > if (buf->vb->skip_cache_sync_on_prepare) > return; > > + /* > + * Coherent MMAP buffers do not need to be synced, unlike USERPTR > + * and non-coherent MMAP buffers. > + */ > + if (buf->vb->memory == V4L2_MEMORY_MMAP && buf->coherent_mem) > + return; > + > if (!sgt) > return; > > + /* For both USERPTR and non-coherent MMAP */ > dma_sync_sgtable_for_device(buf->dev, sgt, buf->dma_dir); > + > + /* Non-coherent MMAP only */ > + if (!buf->coherent_mem && buf->vaddr) > + flush_kernel_vmap_range(buf->vaddr, buf->size); > } > > static void vb2_dc_finish(void *buf_priv) > @@ -115,13 +138,26 @@ static void vb2_dc_finish(void *buf_priv) > struct vb2_dc_buf *buf = buf_priv; > struct sg_table *sgt = buf->dma_sgt; > > + /* This takes care of DMABUF and user-enforced cache sync hint */ > if (buf->vb->skip_cache_sync_on_finish) > return; > > + /* > + * Coherent MMAP buffers do not need to be synced, unlike USERPTR > + * and non-coherent MMAP buffers. > + */ > + if (buf->vb->memory == V4L2_MEMORY_MMAP && buf->coherent_mem) > + return; > + > if (!sgt) > return; > > + /* For both USERPTR and non-coherent MMAP */ > dma_sync_sgtable_for_cpu(buf->dev, sgt, buf->dma_dir); > + > + /* Non-coherent MMAP only */ > + if (!buf->coherent_mem && buf->vaddr) > + invalidate_kernel_vmap_range(buf->vaddr, buf->size); > } > > /*********************************************/ > @@ -139,17 +175,66 @@ static void vb2_dc_put(void *buf_priv) > sg_free_table(buf->sgt_base); > kfree(buf->sgt_base); > } > - dma_free_attrs(buf->dev, buf->size, buf->cookie, buf->dma_addr, > - buf->attrs); > + > + if (buf->coherent_mem) { > + dma_free_attrs(buf->dev, buf->size, buf->cookie, > + buf->dma_addr, buf->attrs); > + } else { > + if (buf->vaddr) > + dma_vunmap_noncontiguous(buf->dev, buf->vaddr); > + dma_free_noncontiguous(buf->dev, buf->size, > + buf->dma_sgt, buf->dma_dir); > + } > put_device(buf->dev); > kfree(buf); > } > > +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; > + > + buf->dma_addr = sg_dma_address(buf->dma_sgt->sgl); > + > + /* > + * For requests that need kernel mapping (DMA_ATTR_NO_KERNEL_MAPPING > + * bit is cleared) we perform dma_vmap_noncontiguous() in vb2_dc_vadd(). > + */ > + return 0; > +} > + > static void *vb2_dc_alloc(struct vb2_buffer *vb, > struct device *dev, > unsigned long size) > { > struct vb2_dc_buf *buf; > + int ret; > > if (WARN_ON(!dev)) > return ERR_PTR(-EINVAL); > @@ -159,27 +244,28 @@ static void *vb2_dc_alloc(struct vb2_buffer *vb, > return ERR_PTR(-ENOMEM); > > buf->attrs = vb->vb2_queue->dma_attrs; > - buf->cookie = dma_alloc_attrs(dev, size, &buf->dma_addr, > - GFP_KERNEL | vb->vb2_queue->gfp_flags, > - buf->attrs); > - if (!buf->cookie) { > - dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size); > - kfree(buf); > - return ERR_PTR(-ENOMEM); > - } > - > - if ((buf->attrs & DMA_ATTR_NO_KERNEL_MAPPING) == 0) > - buf->vaddr = buf->cookie; > + buf->dma_dir = vb->vb2_queue->dma_dir; > + buf->vb = vb; > + buf->coherent_mem = vb->vb2_queue->coherent_mem; > > + buf->size = size; > /* Prevent the device from being released while the buffer is used */ > buf->dev = get_device(dev); > - buf->size = size; > - buf->dma_dir = vb->vb2_queue->dma_dir; > + > + if (buf->coherent_mem) > + ret = vb2_dc_alloc_coherent(buf); > + else > + ret = vb2_dc_alloc_non_coherent(buf); > + > + if (ret) { > + dev_err(dev, "dma alloc of size %ld failed\n", size); > + kfree(buf); > + return ERR_PTR(-ENOMEM); > + } > > buf->handler.refcount = &buf->refcount; > buf->handler.put = vb2_dc_put; > buf->handler.arg = buf; > - buf->vb = vb; > > refcount_set(&buf->refcount, 1); > > @@ -196,9 +282,12 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma) > return -EINVAL; > } > > - ret = dma_mmap_attrs(buf->dev, vma, buf->cookie, > - buf->dma_addr, buf->size, buf->attrs); > - > + if (buf->coherent_mem) > + ret = dma_mmap_attrs(buf->dev, vma, buf->cookie, buf->dma_addr, > + buf->size, buf->attrs); > + else > + ret = dma_mmap_noncontiguous(buf->dev, vma, buf->size, > + buf->dma_sgt); > if (ret) { > pr_err("Remapping memory failed, error: %d\n", ret); > return ret; > @@ -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)); > > return 0; > } > @@ -390,6 +479,9 @@ static struct sg_table *vb2_dc_get_base_sgt(struct vb2_dc_buf *buf) > int ret; > struct sg_table *sgt; > > + if (!buf->coherent_mem) > + return buf->dma_sgt; > + > sgt = kmalloc(sizeof(*sgt), GFP_KERNEL); > if (!sgt) { > dev_err(buf->dev, "failed to alloc sg table\n"); > Regards, Hans
On 03/08/2021 10:33, Hans Verkuil wrote: > On 27/07/2021 09:05, Sergey Senozhatsky wrote: >> This adds support for new noncontiguous DMA API, which > > for -> for the > >> requires allocators to have two execution branches: one >> for the current API, and one for the new one. >> >> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> >> Acked-by: Christoph Hellwig <hch@lst.de> >> --- >> .../common/videobuf2/videobuf2-dma-contig.c | 142 +++++++++++++++--- >> 1 file changed, 117 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c >> index 1e218bc440c6..10f73e27d694 100644 >> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c >> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c >> @@ -17,6 +17,7 @@ >> #include <linux/sched.h> >> #include <linux/slab.h> >> #include <linux/dma-mapping.h> >> +#include <linux/highmem.h> >> >> #include <media/videobuf2-v4l2.h> >> #include <media/videobuf2-dma-contig.h> >> @@ -42,6 +43,7 @@ struct vb2_dc_buf { >> struct dma_buf_attachment *db_attach; >> >> struct vb2_buffer *vb; >> + bool coherent_mem; > > I think that this as well should be renamed to non_coherent_mem. > >> }; >> >> /*********************************************/ >> @@ -78,14 +80,22 @@ static void *vb2_dc_cookie(struct vb2_buffer *vb, void *buf_priv) >> 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; >> } >> >> @@ -101,13 +111,26 @@ static void vb2_dc_prepare(void *buf_priv) >> struct vb2_dc_buf *buf = buf_priv; >> struct sg_table *sgt = buf->dma_sgt; >> >> + /* This takes care of DMABUF and user-enforced cache sync hint */ >> if (buf->vb->skip_cache_sync_on_prepare) >> return; >> >> + /* >> + * Coherent MMAP buffers do not need to be synced, unlike USERPTR >> + * and non-coherent MMAP buffers. >> + */ >> + if (buf->vb->memory == V4L2_MEMORY_MMAP && buf->coherent_mem) >> + return; >> + >> if (!sgt) >> return; >> >> + /* For both USERPTR and non-coherent MMAP */ >> dma_sync_sgtable_for_device(buf->dev, sgt, buf->dma_dir); >> + >> + /* Non-coherent MMAP only */ >> + if (!buf->coherent_mem && buf->vaddr) >> + flush_kernel_vmap_range(buf->vaddr, buf->size); >> } >> >> static void vb2_dc_finish(void *buf_priv) >> @@ -115,13 +138,26 @@ static void vb2_dc_finish(void *buf_priv) >> struct vb2_dc_buf *buf = buf_priv; >> struct sg_table *sgt = buf->dma_sgt; >> >> + /* This takes care of DMABUF and user-enforced cache sync hint */ >> if (buf->vb->skip_cache_sync_on_finish) >> return; >> >> + /* >> + * Coherent MMAP buffers do not need to be synced, unlike USERPTR >> + * and non-coherent MMAP buffers. >> + */ >> + if (buf->vb->memory == V4L2_MEMORY_MMAP && buf->coherent_mem) >> + return; >> + >> if (!sgt) >> return; >> >> + /* For both USERPTR and non-coherent MMAP */ >> dma_sync_sgtable_for_cpu(buf->dev, sgt, buf->dma_dir); >> + >> + /* Non-coherent MMAP only */ >> + if (!buf->coherent_mem && buf->vaddr) >> + invalidate_kernel_vmap_range(buf->vaddr, buf->size); >> } >> >> /*********************************************/ >> @@ -139,17 +175,66 @@ static void vb2_dc_put(void *buf_priv) >> sg_free_table(buf->sgt_base); >> kfree(buf->sgt_base); >> } >> - dma_free_attrs(buf->dev, buf->size, buf->cookie, buf->dma_addr, >> - buf->attrs); >> + >> + if (buf->coherent_mem) { >> + dma_free_attrs(buf->dev, buf->size, buf->cookie, >> + buf->dma_addr, buf->attrs); >> + } else { >> + if (buf->vaddr) >> + dma_vunmap_noncontiguous(buf->dev, buf->vaddr); >> + dma_free_noncontiguous(buf->dev, buf->size, >> + buf->dma_sgt, buf->dma_dir); >> + } >> put_device(buf->dev); >> kfree(buf); >> } >> >> +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; >> + >> + buf->dma_addr = sg_dma_address(buf->dma_sgt->sgl); >> + >> + /* >> + * For requests that need kernel mapping (DMA_ATTR_NO_KERNEL_MAPPING >> + * bit is cleared) we perform dma_vmap_noncontiguous() in vb2_dc_vadd(). Typo: vb2_dc_vadd -> vb2_dc_vaddr Regards, Hans
Hi Sergey, On 27/07/2021 09:05, Sergey Senozhatsky wrote: > This adds support for new noncontiguous DMA API, which > requires allocators to have two execution branches: one > for the current API, and one for the new one. > > Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org> > Acked-by: Christoph Hellwig <hch@lst.de> > --- > .../common/videobuf2/videobuf2-dma-contig.c | 142 +++++++++++++++--- > 1 file changed, 117 insertions(+), 25 deletions(-) > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > index 1e218bc440c6..10f73e27d694 100644 > --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c > +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c > @@ -17,6 +17,7 @@ > #include <linux/sched.h> > #include <linux/slab.h> > #include <linux/dma-mapping.h> > +#include <linux/highmem.h> > > #include <media/videobuf2-v4l2.h> > #include <media/videobuf2-dma-contig.h> > @@ -42,6 +43,7 @@ struct vb2_dc_buf { > struct dma_buf_attachment *db_attach; > > struct vb2_buffer *vb; > + bool coherent_mem; > }; > > /*********************************************/ > @@ -78,14 +80,22 @@ static void *vb2_dc_cookie(struct vb2_buffer *vb, void *buf_priv) > 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. 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? See also my comment below for the vb2_dc_dmabuf_ops_vmap function. > > @@ -101,13 +111,26 @@ static void vb2_dc_prepare(void *buf_priv) > struct vb2_dc_buf *buf = buf_priv; > struct sg_table *sgt = buf->dma_sgt; > > + /* This takes care of DMABUF and user-enforced cache sync hint */ > if (buf->vb->skip_cache_sync_on_prepare) > return; > > + /* > + * Coherent MMAP buffers do not need to be synced, unlike USERPTR > + * and non-coherent MMAP buffers. > + */ > + if (buf->vb->memory == V4L2_MEMORY_MMAP && buf->coherent_mem) > + return; > + > if (!sgt) > return; > > + /* For both USERPTR and non-coherent MMAP */ > dma_sync_sgtable_for_device(buf->dev, sgt, buf->dma_dir); > + > + /* Non-coherent MMAP only */ > + if (!buf->coherent_mem && buf->vaddr) > + flush_kernel_vmap_range(buf->vaddr, buf->size); > } > > static void vb2_dc_finish(void *buf_priv) > @@ -115,13 +138,26 @@ static void vb2_dc_finish(void *buf_priv) > struct vb2_dc_buf *buf = buf_priv; > struct sg_table *sgt = buf->dma_sgt; > > + /* This takes care of DMABUF and user-enforced cache sync hint */ > if (buf->vb->skip_cache_sync_on_finish) > return; > > + /* > + * Coherent MMAP buffers do not need to be synced, unlike USERPTR > + * and non-coherent MMAP buffers. > + */ > + if (buf->vb->memory == V4L2_MEMORY_MMAP && buf->coherent_mem) > + return; > + > if (!sgt) > return; > > + /* For both USERPTR and non-coherent MMAP */ > dma_sync_sgtable_for_cpu(buf->dev, sgt, buf->dma_dir); > + > + /* Non-coherent MMAP only */ > + if (!buf->coherent_mem && buf->vaddr) > + invalidate_kernel_vmap_range(buf->vaddr, buf->size); > } > > /*********************************************/ > @@ -139,17 +175,66 @@ static void vb2_dc_put(void *buf_priv) > sg_free_table(buf->sgt_base); > kfree(buf->sgt_base); > } > - dma_free_attrs(buf->dev, buf->size, buf->cookie, buf->dma_addr, > - buf->attrs); > + > + if (buf->coherent_mem) { > + dma_free_attrs(buf->dev, buf->size, buf->cookie, > + buf->dma_addr, buf->attrs); > + } else { > + if (buf->vaddr) > + dma_vunmap_noncontiguous(buf->dev, buf->vaddr); > + dma_free_noncontiguous(buf->dev, buf->size, > + buf->dma_sgt, buf->dma_dir); > + } > put_device(buf->dev); > kfree(buf); > } > > +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; > + > + buf->dma_addr = sg_dma_address(buf->dma_sgt->sgl); > + > + /* > + * For requests that need kernel mapping (DMA_ATTR_NO_KERNEL_MAPPING > + * bit is cleared) we perform dma_vmap_noncontiguous() in vb2_dc_vadd(). > + */ > + return 0; > +} > + > static void *vb2_dc_alloc(struct vb2_buffer *vb, > struct device *dev, > unsigned long size) > { > struct vb2_dc_buf *buf; > + int ret; > > if (WARN_ON(!dev)) > return ERR_PTR(-EINVAL); > @@ -159,27 +244,28 @@ static void *vb2_dc_alloc(struct vb2_buffer *vb, > return ERR_PTR(-ENOMEM); > > buf->attrs = vb->vb2_queue->dma_attrs; > - buf->cookie = dma_alloc_attrs(dev, size, &buf->dma_addr, > - GFP_KERNEL | vb->vb2_queue->gfp_flags, > - buf->attrs); > - if (!buf->cookie) { > - dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size); > - kfree(buf); > - return ERR_PTR(-ENOMEM); > - } > - > - if ((buf->attrs & DMA_ATTR_NO_KERNEL_MAPPING) == 0) > - buf->vaddr = buf->cookie; > + buf->dma_dir = vb->vb2_queue->dma_dir; > + buf->vb = vb; > + buf->coherent_mem = vb->vb2_queue->coherent_mem; > > + buf->size = size; > /* Prevent the device from being released while the buffer is used */ > buf->dev = get_device(dev); > - buf->size = size; > - buf->dma_dir = vb->vb2_queue->dma_dir; > + > + if (buf->coherent_mem) > + ret = vb2_dc_alloc_coherent(buf); > + else > + ret = vb2_dc_alloc_non_coherent(buf); > + > + if (ret) { > + dev_err(dev, "dma alloc of size %ld failed\n", size); > + kfree(buf); > + return ERR_PTR(-ENOMEM); > + } > > buf->handler.refcount = &buf->refcount; > buf->handler.put = vb2_dc_put; > buf->handler.arg = buf; > - buf->vb = vb; > > refcount_set(&buf->refcount, 1); > > @@ -196,9 +282,12 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma) > return -EINVAL; > } > > - ret = dma_mmap_attrs(buf->dev, vma, buf->cookie, > - buf->dma_addr, buf->size, buf->attrs); > - > + if (buf->coherent_mem) > + ret = dma_mmap_attrs(buf->dev, vma, buf->cookie, buf->dma_addr, > + buf->size, buf->attrs); > + else > + ret = dma_mmap_noncontiguous(buf->dev, vma, buf->size, > + buf->dma_sgt); > if (ret) { > pr_err("Remapping memory failed, error: %d\n", ret); > return ret; > @@ -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? 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. Regards, Hans > > return 0; > } > @@ -390,6 +479,9 @@ static struct sg_table *vb2_dc_get_base_sgt(struct vb2_dc_buf *buf) > int ret; > struct sg_table *sgt; > > + if (!buf->coherent_mem) > + return buf->dma_sgt; > + > sgt = kmalloc(sizeof(*sgt), GFP_KERNEL); > if (!sgt) { > dev_err(buf->dev, "failed to alloc sg table\n"); >
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.
On Tue, Aug 17, 2021 at 8:57 PM Sergey Senozhatsky <senozhatsky@chromium.org> wrote: > > 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? 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. > > --- > > 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; > + 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; > > > --- > > > [..] > > > @@ -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.
On (21/08/03 10:33), Hans Verkuil wrote: > > struct dma_buf_attachment *db_attach; > > > > struct vb2_buffer *vb; > > + bool coherent_mem; > > I think that this as well should be renamed to non_coherent_mem. > Done.
On (21/08/03 10:39), Hans Verkuil wrote: > >> + > >> + /* > >> + * For requests that need kernel mapping (DMA_ATTR_NO_KERNEL_MAPPING > >> + * bit is cleared) we perform dma_vmap_noncontiguous() in vb2_dc_vadd(). > > Typo: vb2_dc_vadd -> vb2_dc_vaddr Done.
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. Done.
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c index 1e218bc440c6..10f73e27d694 100644 --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c @@ -17,6 +17,7 @@ #include <linux/sched.h> #include <linux/slab.h> #include <linux/dma-mapping.h> +#include <linux/highmem.h> #include <media/videobuf2-v4l2.h> #include <media/videobuf2-dma-contig.h> @@ -42,6 +43,7 @@ struct vb2_dc_buf { struct dma_buf_attachment *db_attach; struct vb2_buffer *vb; + bool coherent_mem; }; /*********************************************/ @@ -78,14 +80,22 @@ static void *vb2_dc_cookie(struct vb2_buffer *vb, void *buf_priv) 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; } @@ -101,13 +111,26 @@ static void vb2_dc_prepare(void *buf_priv) struct vb2_dc_buf *buf = buf_priv; struct sg_table *sgt = buf->dma_sgt; + /* This takes care of DMABUF and user-enforced cache sync hint */ if (buf->vb->skip_cache_sync_on_prepare) return; + /* + * Coherent MMAP buffers do not need to be synced, unlike USERPTR + * and non-coherent MMAP buffers. + */ + if (buf->vb->memory == V4L2_MEMORY_MMAP && buf->coherent_mem) + return; + if (!sgt) return; + /* For both USERPTR and non-coherent MMAP */ dma_sync_sgtable_for_device(buf->dev, sgt, buf->dma_dir); + + /* Non-coherent MMAP only */ + if (!buf->coherent_mem && buf->vaddr) + flush_kernel_vmap_range(buf->vaddr, buf->size); } static void vb2_dc_finish(void *buf_priv) @@ -115,13 +138,26 @@ static void vb2_dc_finish(void *buf_priv) struct vb2_dc_buf *buf = buf_priv; struct sg_table *sgt = buf->dma_sgt; + /* This takes care of DMABUF and user-enforced cache sync hint */ if (buf->vb->skip_cache_sync_on_finish) return; + /* + * Coherent MMAP buffers do not need to be synced, unlike USERPTR + * and non-coherent MMAP buffers. + */ + if (buf->vb->memory == V4L2_MEMORY_MMAP && buf->coherent_mem) + return; + if (!sgt) return; + /* For both USERPTR and non-coherent MMAP */ dma_sync_sgtable_for_cpu(buf->dev, sgt, buf->dma_dir); + + /* Non-coherent MMAP only */ + if (!buf->coherent_mem && buf->vaddr) + invalidate_kernel_vmap_range(buf->vaddr, buf->size); } /*********************************************/ @@ -139,17 +175,66 @@ static void vb2_dc_put(void *buf_priv) sg_free_table(buf->sgt_base); kfree(buf->sgt_base); } - dma_free_attrs(buf->dev, buf->size, buf->cookie, buf->dma_addr, - buf->attrs); + + if (buf->coherent_mem) { + dma_free_attrs(buf->dev, buf->size, buf->cookie, + buf->dma_addr, buf->attrs); + } else { + if (buf->vaddr) + dma_vunmap_noncontiguous(buf->dev, buf->vaddr); + dma_free_noncontiguous(buf->dev, buf->size, + buf->dma_sgt, buf->dma_dir); + } put_device(buf->dev); kfree(buf); } +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; + + buf->dma_addr = sg_dma_address(buf->dma_sgt->sgl); + + /* + * For requests that need kernel mapping (DMA_ATTR_NO_KERNEL_MAPPING + * bit is cleared) we perform dma_vmap_noncontiguous() in vb2_dc_vadd(). + */ + return 0; +} + static void *vb2_dc_alloc(struct vb2_buffer *vb, struct device *dev, unsigned long size) { struct vb2_dc_buf *buf; + int ret; if (WARN_ON(!dev)) return ERR_PTR(-EINVAL); @@ -159,27 +244,28 @@ static void *vb2_dc_alloc(struct vb2_buffer *vb, return ERR_PTR(-ENOMEM); buf->attrs = vb->vb2_queue->dma_attrs; - buf->cookie = dma_alloc_attrs(dev, size, &buf->dma_addr, - GFP_KERNEL | vb->vb2_queue->gfp_flags, - buf->attrs); - if (!buf->cookie) { - dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size); - kfree(buf); - return ERR_PTR(-ENOMEM); - } - - if ((buf->attrs & DMA_ATTR_NO_KERNEL_MAPPING) == 0) - buf->vaddr = buf->cookie; + buf->dma_dir = vb->vb2_queue->dma_dir; + buf->vb = vb; + buf->coherent_mem = vb->vb2_queue->coherent_mem; + buf->size = size; /* Prevent the device from being released while the buffer is used */ buf->dev = get_device(dev); - buf->size = size; - buf->dma_dir = vb->vb2_queue->dma_dir; + + if (buf->coherent_mem) + ret = vb2_dc_alloc_coherent(buf); + else + ret = vb2_dc_alloc_non_coherent(buf); + + if (ret) { + dev_err(dev, "dma alloc of size %ld failed\n", size); + kfree(buf); + return ERR_PTR(-ENOMEM); + } buf->handler.refcount = &buf->refcount; buf->handler.put = vb2_dc_put; buf->handler.arg = buf; - buf->vb = vb; refcount_set(&buf->refcount, 1); @@ -196,9 +282,12 @@ static int vb2_dc_mmap(void *buf_priv, struct vm_area_struct *vma) return -EINVAL; } - ret = dma_mmap_attrs(buf->dev, vma, buf->cookie, - buf->dma_addr, buf->size, buf->attrs); - + if (buf->coherent_mem) + ret = dma_mmap_attrs(buf->dev, vma, buf->cookie, buf->dma_addr, + buf->size, buf->attrs); + else + ret = dma_mmap_noncontiguous(buf->dev, vma, buf->size, + buf->dma_sgt); if (ret) { pr_err("Remapping memory failed, error: %d\n", ret); return ret; @@ -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)); return 0; } @@ -390,6 +479,9 @@ static struct sg_table *vb2_dc_get_base_sgt(struct vb2_dc_buf *buf) int ret; struct sg_table *sgt; + if (!buf->coherent_mem) + return buf->dma_sgt; + sgt = kmalloc(sizeof(*sgt), GFP_KERNEL); if (!sgt) { dev_err(buf->dev, "failed to alloc sg table\n");