On Fri, Apr 24, 2020 at 3:27 PM Andrew F. Davis afd@ti.com wrote:

This new export type exposes to userspace the SRAM area as a DMA-Heap, this allows for allocations as DMA-BUFs that can be consumed by various DMA-BUF supporting devices.

Signed-off-by: Andrew F. Davis afd@ti.com

Nice! Very excited to have the first new heap (that didn't come with the initial patchset)!

Overall looks good! I don't have any comment on the SRAM side of things, but a few minor questions/nits below.

diff --git a/drivers/misc/sram-dma-heap.c b/drivers/misc/sram-dma-heap.c new file mode 100644 index 000000000000..38df0397f294 --- /dev/null +++ b/drivers/misc/sram-dma-heap.c @@ -0,0 +1,243 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • SRAM DMA-Heap userspace exporter
• • Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/

• • Andrew F. Davis <afd@ti.com>
    

• */

+#include <linux/dma-mapping.h> +#include <linux/err.h> +#include <linux/genalloc.h> +#include <linux/io.h> +#include <linux/mm.h> +#include <linux/scatterlist.h> +#include <linux/slab.h> +#include <linux/dma-buf.h> +#include <linux/dma-heap.h>

+#include "sram.h"

+struct sram_dma_heap {

•   struct dma_heap *heap;
  

•   struct gen_pool *pool;
  

+};

+struct sram_dma_heap_buffer {

•   struct gen_pool *pool;
  

•   struct list_head attachments;
  

•   struct mutex attachments_lock;
  

•   unsigned long len;
  

•   void *vaddr;
  

•   phys_addr_t paddr;
  

+};

+struct dma_heap_attachment {

•   struct device *dev;
  

•   struct sg_table *table;
  

•   struct list_head list;
  

+};

+static int dma_heap_attach(struct dma_buf *dmabuf,

•                      struct dma_buf_attachment *attachment)
  

+{

•   struct sram_dma_heap_buffer *buffer = dmabuf->priv;
  

•   struct dma_heap_attachment *a;
  

•   struct sg_table *table;
  

•   a = kzalloc(sizeof(*a), GFP_KERNEL);
  

•   if (!a)
  

•           return -ENOMEM;
  

•   table = kmalloc(sizeof(*table), GFP_KERNEL);
  

•   if (!table) {
  

•           kfree(a);
  

•           return -ENOMEM;
  

•   }
  

•   if (sg_alloc_table(table, 1, GFP_KERNEL)) {
  

•           kfree(table);
  

•           kfree(a);
  

•           return -ENOMEM;
  

•   }
  

•   sg_set_page(table->sgl, pfn_to_page(PFN_DOWN(buffer->paddr)), buffer->len, 0);
  

•   a->table = table;
  

•   a->dev = attachment->dev;
  

•   INIT_LIST_HEAD(&a->list);
  

•   attachment->priv = a;
  

•   mutex_lock(&buffer->attachments_lock);
  

•   list_add(&a->list, &buffer->attachments);
  

•   mutex_unlock(&buffer->attachments_lock);
  

•   return 0;
  

+}

+static void dma_heap_detatch(struct dma_buf *dmabuf,

•                        struct dma_buf_attachment *attachment)
  

+{

•   struct sram_dma_heap_buffer *buffer = dmabuf->priv;
  

•   struct dma_heap_attachment *a = attachment->priv;
  

•   mutex_lock(&buffer->attachments_lock);
  

•   list_del(&a->list);
  

•   mutex_unlock(&buffer->attachments_lock);
  

•   sg_free_table(a->table);
  

•   kfree(a->table);
  

•   kfree(a);
  

+}

+static struct sg_table *dma_heap_map_dma_buf(struct dma_buf_attachment *attachment,

•                                        enum dma_data_direction direction)
  

+{

•   struct dma_heap_attachment *a = attachment->priv;
  

•   struct sg_table *table = a->table;
  

•   if (!dma_map_sg_attrs(attachment->dev, table->sgl, table->nents,
  

•                         direction, DMA_ATTR_SKIP_CPU_SYNC))
  

Might be nice to have a comment as to why you're using SKIP_CPU_SYNC and why it's safe.

•           return ERR_PTR(-ENOMEM);
  

•   return table;
  

+}

+static void dma_heap_unmap_dma_buf(struct dma_buf_attachment *attachment,

•                              struct sg_table *table,
  

•                              enum dma_data_direction direction)
  

+{

•   dma_unmap_sg_attrs(attachment->dev, table->sgl, table->nents,
  

•                      direction, DMA_ATTR_SKIP_CPU_SYNC);
  

+}

+static void dma_heap_dma_buf_release(struct dma_buf *dmabuf) +{

•   struct sram_dma_heap_buffer *buffer = dmabuf->priv;
  

•   gen_pool_free(buffer->pool, (unsigned long)buffer->vaddr, buffer->len);
  

•   kfree(buffer);
  

+}

+static int dma_heap_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma) +{

•   struct sram_dma_heap_buffer *buffer = dmabuf->priv;
  

•   int ret;
  

•   /* SRAM mappings are not cached */
  

•   vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
  

•   ret = vm_iomap_memory(vma, buffer->paddr, buffer->len);
  

•   if (ret)
  

•           pr_err("Could not map buffer to userspace\n");
  

•   return ret;
  

+}

+static void *dma_heap_vmap(struct dma_buf *dmabuf) +{

•   struct sram_dma_heap_buffer *buffer = dmabuf->priv;
  

•   return buffer->vaddr;
  

+}

+const struct dma_buf_ops sram_dma_heap_buf_ops = {

•   .attach = dma_heap_attach,
  

•   .detach = dma_heap_detatch,
  

•   .map_dma_buf = dma_heap_map_dma_buf,
  

•   .unmap_dma_buf = dma_heap_unmap_dma_buf,
  

•   .release = dma_heap_dma_buf_release,
  

•   .mmap = dma_heap_mmap,
  

•   .vmap = dma_heap_vmap,
  

+};

No begin/end_cpu_access functions here? I'm guessing it's because you're always using SKIP_CPU_SYNC so it wouldn't do anything? A small comment in the code might help.

+static int sram_dma_heap_allocate(struct dma_heap *heap,

•                             unsigned long len,
  

•                             unsigned long fd_flags,
  

•                             unsigned long heap_flags)
  

+{

•   struct sram_dma_heap *sram_dma_heap = dma_heap_get_drvdata(heap);
  

•   struct sram_dma_heap_buffer *buffer;
  

•   DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
  

•   struct dma_buf *dmabuf;
  

•   int ret;
  

•   buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
  

•   if (!buffer)
  

•           return -ENOMEM;
  

•   buffer->pool = sram_dma_heap->pool;
  

•   INIT_LIST_HEAD(&buffer->attachments);
  

•   mutex_init(&buffer->attachments_lock);
  

•   buffer->len = len;
  

•   buffer->vaddr = (void *)gen_pool_alloc(buffer->pool, buffer->len);
  

•   if (!buffer->vaddr) {
  

•           ret = -ENOMEM;
  

•           goto free_buffer;
  

•   }
  

•   buffer->paddr = gen_pool_virt_to_phys(buffer->pool, (unsigned long)buffer->vaddr);
  

•   if (buffer->paddr == -1) {
  

•           ret = -ENOMEM;
  

•           goto free_pool;
  

•   }
  

•   /* create the dmabuf */
  

•   exp_info.ops = &sram_dma_heap_buf_ops;
  

•   exp_info.size = buffer->len;
  

•   exp_info.flags = fd_flags;
  

•   exp_info.priv = buffer;
  

•   dmabuf = dma_buf_export(&exp_info);
  

•   if (IS_ERR(dmabuf)) {
  

•           ret = PTR_ERR(dmabuf);
  

•           goto free_pool;
  

•   }
  

•   ret = dma_buf_fd(dmabuf, fd_flags);
  

•   if (ret < 0) {
  

•           dma_buf_put(dmabuf);
  

•           /* just return, as put will call release and that will free */
  

•           return ret;
  

•   }
  

•   return ret;
  

+free_pool:

•   gen_pool_free(buffer->pool, (unsigned long)buffer->vaddr, buffer->len);
  

+free_buffer:

•   kfree(buffer);
  

•   return ret;
  

+}

+static struct dma_heap_ops sram_dma_heap_ops = {

•   .allocate = sram_dma_heap_allocate,
  

+};

+int sram_dma_heap_export(struct sram_dev *sram,

This is totally a bikeshed thing (feel free to ignore), but maybe sram_dma_heap_create() or _add() would be a better name to avoid folks mixing it up with the dmabuf exporter?

•                    struct sram_reserve *block,
  

•                    phys_addr_t start,
  

•                    struct sram_partition *part)
  

+{

•   struct sram_dma_heap *sram_dma_heap;
  

•   struct dma_heap_export_info exp_info;
  

•   dev_info(sram->dev, "Exporting SRAM pool '%s'\n", block->label);
  

Again, shed issue: but for terminology consistency (at least in the dmabuf heaps space), maybe heap instead of pool?

Thanks so much again for submitting this! -john