[PATCH] misc: sram: Add dma-heap-export reserved SRAM area type
Andrew F. Davis
afd at ti.com
Wed Apr 29 18:40:46 UTC 2020
On 4/24/20 8:44 PM, John Stultz wrote:
> On Fri, Apr 24, 2020 at 3:27 PM Andrew F. Davis <afd at 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 at 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 at 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.
>
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
>
More information about the dri-devel
mailing list