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 <[email protected]>
---
.../devicetree/bindings/sram/sram.yaml | 8 +
drivers/misc/Kconfig | 7 +
drivers/misc/Makefile | 1 +
drivers/misc/sram-dma-heap.c | 243 ++++++++++++++++++
drivers/misc/sram.c | 20 +-
drivers/misc/sram.h | 17 ++
6 files changed, 292 insertions(+), 4 deletions(-)
create mode 100644 drivers/misc/sram-dma-heap.c
diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
index 7b83cc6c9bfa..b8e33c8d205d 100644
--- a/Documentation/devicetree/bindings/sram/sram.yaml
+++ b/Documentation/devicetree/bindings/sram/sram.yaml
@@ -105,6 +105,14 @@ patternProperties:
manipulation of the page attributes.
type: boolean
+ dma-heap-export:
+ description:
+ Similar to 'pool' and 'export' this region will be exported for use
+ by drivers, devices, and userspace using the DMA-Heaps framework.
+ NOTE: This region must be page aligned on start and end in order to
+ properly allow manipulation of the page attributes.
+ type: boolean
+
label:
description:
The name for the reserved partition, if omitted, the label is taken
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 99e151475d8f..10a9aed531c4 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -423,6 +423,13 @@ config SRAM
config SRAM_EXEC
bool
+config SRAM_DMA_HEAP
+ bool "Export on-chip SRAM pools using DMA-Heaps"
+ depends on DMABUF_HEAPS && SRAM
+ help
+ This driver allows the export of on-chip SRAM marked as exportable
+ to userspace using the DMA-Heaps interface.
+
config VEXPRESS_SYSCFG
bool "Versatile Express System Configuration driver"
depends on VEXPRESS_CONFIG
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 9abf2923d831..c5b5db26ebb2 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_VMWARE_VMCI) += vmw_vmci/
obj-$(CONFIG_LATTICE_ECP3_CONFIG) += lattice-ecp3-config.o
obj-$(CONFIG_SRAM) += sram.o
obj-$(CONFIG_SRAM_EXEC) += sram-exec.o
+obj-$(CONFIG_SRAM_DMA_HEAP) += sram-dma-heap.o
obj-y += mic/
obj-$(CONFIG_GENWQE) += genwqe/
obj-$(CONFIG_ECHO) += echo/
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 <[email protected]>
+ */
+
+#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))
+ 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,
+};
+
+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,
+ 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);
+
+ sram_dma_heap = kzalloc(sizeof(*sram_dma_heap), GFP_KERNEL);
+ if (!sram_dma_heap)
+ return -ENOMEM;
+ sram_dma_heap->pool = part->pool;
+
+ exp_info.name = kasprintf(GFP_KERNEL, "sram_%s", block->label);
+ exp_info.ops = &sram_dma_heap_ops;
+ exp_info.priv = sram_dma_heap;
+ sram_dma_heap->heap = dma_heap_add(&exp_info);
+ if (IS_ERR(sram_dma_heap->heap)) {
+ int ret = PTR_ERR(sram_dma_heap->heap);
+ kfree(sram_dma_heap);
+ return ret;
+ }
+
+ return 0;
+}
diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c
index 6c1a23cb3e8c..0337e271cfe2 100644
--- a/drivers/misc/sram.c
+++ b/drivers/misc/sram.c
@@ -109,6 +109,15 @@ static int sram_add_partition(struct sram_dev *sram, struct sram_reserve *block,
if (ret)
return ret;
}
+ if (block->dma_heap_export) {
+ ret = sram_add_pool(sram, block, start, part);
+ if (ret)
+ return ret;
+
+ ret = sram_dma_heap_export(sram, block, start, part);
+ if (ret)
+ return ret;
+ }
if (block->protect_exec) {
ret = sram_check_protect_exec(sram, block, part);
if (ret)
@@ -209,8 +218,11 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
if (of_find_property(child, "protect-exec", NULL))
block->protect_exec = true;
- if ((block->export || block->pool || block->protect_exec) &&
- block->size) {
+ if (of_find_property(child, "dma-heap-export", NULL))
+ block->dma_heap_export = true;
+
+ if ((block->export || block->pool || block->protect_exec ||
+ block->dma_heap_export) && block->size) {
exports++;
label = NULL;
@@ -272,8 +284,8 @@ static int sram_reserve_regions(struct sram_dev *sram, struct resource *res)
goto err_chunks;
}
- if ((block->export || block->pool || block->protect_exec) &&
- block->size) {
+ if ((block->export || block->pool || block->protect_exec ||
+ block->dma_heap_export) && block->size) {
ret = sram_add_partition(sram, block,
res->start + block->start);
if (ret) {
diff --git a/drivers/misc/sram.h b/drivers/misc/sram.h
index 9c1d21ff7347..e60ab13e8e6b 100644
--- a/drivers/misc/sram.h
+++ b/drivers/misc/sram.h
@@ -32,6 +32,7 @@ struct sram_reserve {
bool export;
bool pool;
bool protect_exec;
+ bool dma_heap_export;
const char *label;
};
@@ -52,4 +53,20 @@ static inline int sram_add_protect_exec(struct sram_partition *part)
return -ENODEV;
}
#endif /* CONFIG_SRAM_EXEC */
+
+#ifdef CONFIG_SRAM_DMA_HEAP
+int sram_dma_heap_export(struct sram_dev *sram,
+ struct sram_reserve *block,
+ phys_addr_t start,
+ struct sram_partition *part);
+#else
+static inline int sram_dma_heap_export(struct sram_dev *sram,
+ struct sram_reserve *block,
+ phys_addr_t start,
+ struct sram_partition *part)
+{
+ return 0;
+}
+#endif /* CONFIG_SRAM_DMA_HEAP */
+
#endif /* __SRAM_H */
--
2.17.1
On Fri, Apr 24, 2020 at 3:27 PM Andrew F. Davis <[email protected]> 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 <[email protected]>
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 <[email protected]>
> + */
> +
> +#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
On Fri, Apr 24, 2020 at 5:27 PM Andrew F. Davis <[email protected]> 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 <[email protected]>
> ---
> .../devicetree/bindings/sram/sram.yaml | 8 +
Separate patch and needs to go to DT list...
> drivers/misc/Kconfig | 7 +
> drivers/misc/Makefile | 1 +
> drivers/misc/sram-dma-heap.c | 243 ++++++++++++++++++
> drivers/misc/sram.c | 20 +-
> drivers/misc/sram.h | 17 ++
> 6 files changed, 292 insertions(+), 4 deletions(-)
> create mode 100644 drivers/misc/sram-dma-heap.c
>
> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
> index 7b83cc6c9bfa..b8e33c8d205d 100644
> --- a/Documentation/devicetree/bindings/sram/sram.yaml
> +++ b/Documentation/devicetree/bindings/sram/sram.yaml
> @@ -105,6 +105,14 @@ patternProperties:
> manipulation of the page attributes.
> type: boolean
>
> + dma-heap-export:
> + description:
> + Similar to 'pool' and 'export' this region will be exported for use
> + by drivers, devices, and userspace using the DMA-Heaps framework.
> + NOTE: This region must be page aligned on start and end in order to
> + properly allow manipulation of the page attributes.
> + type: boolean
Though I'm not sure this should be in DT. You have to change your
firmware to enable a new kernel feature? We also already have 'export'
which sounds like the same function. Or 'pool' though reading the
description, I don't really understand it's use.
What combination of all 3 of these options would be valid?
Rob
On 4/24/20 8:44 PM, John Stultz wrote:
> On Fri, Apr 24, 2020 at 3:27 PM Andrew F. Davis <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
>> + */
>> +
>> +#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.
>
Ack, should be simple enough to explain that SRAM is non-cached and so
this sync is not needed (and may not work either given the SRAM region
does not have valid page structures assdociated).
>> + 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.
>
Yes, same idea, non-cached/coherent means the access does not need to be
bracketed by *_cpu_access functions. Will comment.
>
>> +
>> +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?
>
Maybe sram_add_dma_heap() which will match up better with the other SRAM
functions, will change.
>> + 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?
>
Ack, s/pool/heap.
> Thanks so much again for submitting this!
Thanks for the review,
Andrew
> -john
>
On 4/27/20 11:17 AM, Rob Herring wrote:
> On Fri, Apr 24, 2020 at 5:27 PM Andrew F. Davis <[email protected]> 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 <[email protected]>
>> ---
>> .../devicetree/bindings/sram/sram.yaml | 8 +
>
> Separate patch and needs to go to DT list...
>
Okay, will split for v2.
>> drivers/misc/Kconfig | 7 +
>> drivers/misc/Makefile | 1 +
>> drivers/misc/sram-dma-heap.c | 243 ++++++++++++++++++
>> drivers/misc/sram.c | 20 +-
>> drivers/misc/sram.h | 17 ++
>> 6 files changed, 292 insertions(+), 4 deletions(-)
>> create mode 100644 drivers/misc/sram-dma-heap.c
>>
>> diff --git a/Documentation/devicetree/bindings/sram/sram.yaml b/Documentation/devicetree/bindings/sram/sram.yaml
>> index 7b83cc6c9bfa..b8e33c8d205d 100644
>> --- a/Documentation/devicetree/bindings/sram/sram.yaml
>> +++ b/Documentation/devicetree/bindings/sram/sram.yaml
>> @@ -105,6 +105,14 @@ patternProperties:
>> manipulation of the page attributes.
>> type: boolean
>>
>> + dma-heap-export:
>> + description:
>> + Similar to 'pool' and 'export' this region will be exported for use
>> + by drivers, devices, and userspace using the DMA-Heaps framework.
>> + NOTE: This region must be page aligned on start and end in order to
>> + properly allow manipulation of the page attributes.
>> + type: boolean
>
> Though I'm not sure this should be in DT. You have to change your
> firmware to enable a new kernel feature? We also already have 'export'
> which sounds like the same function. Or 'pool' though reading the
> description, I don't really understand it's use.
>
Maybe I could just re-use 'export', right now that property causes the
SRAM region to be exported to userspace as a file one can read/write.
Exporting via dma-heaps/dma-buf allows more flexibility as one can pass
the exported regions to other devices or mmap them directly in userspace.
It's up to the SRAM driver maintainer if re-purposing that property in
the driver is acceptable.
'Pool' doesn't make much sense to me either, it creates a gen-pool out
of the region, but I cant find any users not in combination with the
other properties.
> What combination of all 3 of these options would be valid?
>
From looking at the SRAM driver, it seems each of the option must be
used exclusively per region, combining will cause issues. I can update
the documentation for the same after we settle on a strategy for this
new property.
Andrew
> Rob
>
Hi Andrew
>
>
> Okay, will split for v2.
>
>
Was there a follow-up v2 of this patchset? AFAICT this series did not
make it into the mainline kernel.
Do you have any plans to work on it? If not I would like to help out
as we have a use case where we want to
use a dma-buf sram exporter.
--
greets
--
Christian Gmeiner, MSc
https://christian-gmeiner.info/privacypolicy
On 4/1/23 3:35 AM, Christian Gmeiner wrote:
> Hi Andrew
>
>>
>>
>> Okay, will split for v2.
>>
>>
>
> Was there a follow-up v2 of this patchset? AFAICT this series did not
> make it into the mainline kernel.
> Do you have any plans to work on it? If not I would like to help out
> as we have a use case where we want to
> use a dma-buf sram exporter.
>
>
Sure, I've been keeping it alive in our evil vendor tree, but if
there is interest upstream now I'll post a v2 and CC you.
Thanks,
Andrew
> > Hi Andrew
> >
> >>
> >>
> >> Okay, will split for v2.
> >>
> >>
> >
> > Was there a follow-up v2 of this patchset? AFAICT this series did not
> > make it into the mainline kernel.
> > Do you have any plans to work on it? If not I would like to help out
> > as we have a use case where we want to
> > use a dma-buf sram exporter.
> >
> >
>
> Sure, I've been keeping it alive in our evil vendor tree, but if
> there is interest upstream now I'll post a v2 and CC you.
That would be great!
--
greets
--
Christian Gmeiner, MSc
https://christian-gmeiner.info/privacypolicy
Hi Andrew
Am Di., 4. Apr. 2023 um 17:02 Uhr schrieb Christian Gmeiner
<[email protected]>:
>
> > > Hi Andrew
> > >
> > >>
> > >>
> > >> Okay, will split for v2.
> > >>
> > >>
> > >
> > > Was there a follow-up v2 of this patchset? AFAICT this series did not
> > > make it into the mainline kernel.
> > > Do you have any plans to work on it? If not I would like to help out
> > > as we have a use case where we want to
> > > use a dma-buf sram exporter.
> > >
> > >
> >
> > Sure, I've been keeping it alive in our evil vendor tree, but if
> > there is interest upstream now I'll post a v2 and CC you.
>
> That would be great!
>
Did you find time to prepare a v2? If not, can you point me to the
evil vendor tree?
--
thanks
--
Christian Gmeiner, MSc
https://christian-gmeiner.info/privacypolicy
On 4/14/23 12:44 AM, Christian Gmeiner wrote:
> Hi Andrew
>
> Am Di., 4. Apr. 2023 um 17:02 Uhr schrieb Christian Gmeiner
> <[email protected]>:
>>
>>>> Hi Andrew
>>>>
>>>>>
>>>>>
>>>>> Okay, will split for v2.
>>>>>
>>>>>
>>>>
>>>> Was there a follow-up v2 of this patchset? AFAICT this series did not
>>>> make it into the mainline kernel.
>>>> Do you have any plans to work on it? If not I would like to help out
>>>> as we have a use case where we want to
>>>> use a dma-buf sram exporter.
>>>>
>>>>
>>>
>>> Sure, I've been keeping it alive in our evil vendor tree, but if
>>> there is interest upstream now I'll post a v2 and CC you.
>>
>> That would be great!
>>
>
> Did you find time to prepare a v2? If not, can you point me to the
> evil vendor tree?
>
>
I did find some time and CC'd you on v2, the patch's subject was slightly
renamed, so maybe your emailer missed it?
https://patchwork.kernel.org/project/linux-media/patch/[email protected]/
Our evil vendor trees are here either way:
https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/
Andrew