misc: sram: Add dma-heap-export reserved SRAM area type

Commit Message

Andrew Davis April 24, 2020, 10:27 p.m. UTC

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 <[e-mail]>
---
 .../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

Comments

John Stultz April 25, 2020, 12:44 a.m. UTC | #1

On Fri, Apr 24, 2020 at 3:27 PM Andrew F. Davis <[e-mail]> 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 <[e-mail]>

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 <[e-mail]>
> + */
> +
> +#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

Rob Herring April 27, 2020, 3:17 p.m. UTC | #2

On Fri, Apr 24, 2020 at 5:27 PM Andrew F. Davis <[e-mail]> 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 <[e-mail]>
> ---
>  .../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

Andrew Davis April 29, 2020, 6:40 p.m. UTC | #3

On 4/24/20 8:44 PM, John Stultz wrote:
> On Fri, Apr 24, 2020 at 3:27 PM Andrew F. Davis <[e-mail]> 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 <[e-mail]>
> 
> 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 <[e-mail]>
>> + */
>> +
>> +#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
>

Andrew Davis April 29, 2020, 7:05 p.m. UTC | #4

On 4/27/20 11:17 AM, Rob Herring wrote:
> On Fri, Apr 24, 2020 at 5:27 PM Andrew F. Davis <[e-mail]> 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 <[e-mail]>
>> ---
>>  .../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
>

Christian Gmeiner April 1, 2023, 8:35 a.m. UTC | #5

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.

Andrew Davis April 3, 2023, 7:26 p.m. UTC | #6

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

Christian Gmeiner April 4, 2023, 3:02 p.m. UTC | #7

> > 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!

Christian Gmeiner April 14, 2023, 5:44 a.m. UTC | #8

Hi Andrew

Am Di., 4. Apr. 2023 um 17:02 Uhr schrieb Christian Gmeiner
<[e-mail]>:
>
> > > 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?

Andrew Davis April 14, 2023, 2:17 p.m. UTC | #9

On 4/14/23 12:44 AM, Christian Gmeiner wrote:
> Hi Andrew
> 
> Am Di., 4. Apr. 2023 um 17:02 Uhr schrieb Christian Gmeiner
> <[e-mail]>:
>>
>>>> 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/[e-mail]/

Our evil vendor trees are here either way:

https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/

Andrew

Patch

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 <[e-mail]>
+ */
+
+#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 */