Hi,
This series is the follow-up of the discussion that John and I had a few
months ago here:
https://lore.kernel.org/all/CANDhNCquJn6bH3KxKf65BWiTYLVqSd9892-xtFDHHqqyrroCMQ@mail.gmail.com/
The initial problem we were discussing was that I'm currently working on
a platform which has a memory layout with ECC enabled. However, enabling
the ECC has a number of drawbacks on that platform: lower performance,
increased memory usage, etc. So for things like framebuffers, the
trade-off isn't great and thus there's a memory region with ECC disabled
to allocate from for such use cases.
After a suggestion from John, I chose to start using heap allocations
flags to allow for userspace to ask for a particular ECC setup. This is
then backed by a new heap type that runs from reserved memory chunks
flagged as such, and the existing DT properties to specify the ECC
properties.
We could also easily extend this mechanism to support more flags, or
through a new ioctl to discover which flags a given heap supports.
I submitted a draft PR to the DT schema for the bindings used in this
PR:
https://github.com/devicetree-org/dt-schema/pull/138
Let me know what you think,
Maxime
Signed-off-by: Maxime Ripard <[email protected]>
---
Maxime Ripard (8):
dma-buf: heaps: Introduce a new heap for reserved memory
of: Add helper to retrieve ECC memory bits
dma-buf: heaps: Import uAPI header
dma-buf: heaps: Add ECC protection flags
dma-buf: heaps: system: Remove global variable
dma-buf: heaps: system: Handle ECC flags
dma-buf: heaps: cma: Handle ECC flags
dma-buf: heaps: carveout: Handle ECC flags
drivers/dma-buf/dma-heap.c | 4 +
drivers/dma-buf/heaps/Kconfig | 8 +
drivers/dma-buf/heaps/Makefile | 1 +
drivers/dma-buf/heaps/carveout_heap.c | 330 ++++++++++++++++++++++++++++++++++
drivers/dma-buf/heaps/cma_heap.c | 10 ++
drivers/dma-buf/heaps/system_heap.c | 29 ++-
include/linux/dma-heap.h | 2 +
include/linux/of.h | 25 +++
include/uapi/linux/dma-heap.h | 5 +-
9 files changed, 407 insertions(+), 7 deletions(-)
---
base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
change-id: 20240515-dma-buf-ecc-heap-28a311d2c94e
Best regards,
--
Maxime Ripard <[email protected]>
Some reserved memory regions might have particular memory setup or
attributes that make them good candidates for heaps.
Let's provide a heap type that will create a new heap for each reserved
memory region flagged as such.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/dma-buf/heaps/Kconfig | 8 +
drivers/dma-buf/heaps/Makefile | 1 +
drivers/dma-buf/heaps/carveout_heap.c | 314 ++++++++++++++++++++++++++++++++++
3 files changed, 323 insertions(+)
diff --git a/drivers/dma-buf/heaps/Kconfig b/drivers/dma-buf/heaps/Kconfig
index a5eef06c4226..c6981d696733 100644
--- a/drivers/dma-buf/heaps/Kconfig
+++ b/drivers/dma-buf/heaps/Kconfig
@@ -1,5 +1,13 @@
+config DMABUF_HEAPS_CARVEOUT
+ bool "Carveout Heaps"
+ depends on DMABUF_HEAPS
+ help
+ Choose this option to enable the carveout dmabuf heap. The carveout
+ heap is backed by pages from reserved memory regions flagged as
+ exportable. If in doubt, say Y.
+
config DMABUF_HEAPS_SYSTEM
bool "DMA-BUF System Heap"
depends on DMABUF_HEAPS
help
Choose this option to enable the system dmabuf heap. The system heap
diff --git a/drivers/dma-buf/heaps/Makefile b/drivers/dma-buf/heaps/Makefile
index 974467791032..b734647ad5c8 100644
--- a/drivers/dma-buf/heaps/Makefile
+++ b/drivers/dma-buf/heaps/Makefile
@@ -1,3 +1,4 @@
# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_DMABUF_HEAPS_CARVEOUT) += carveout_heap.o
obj-$(CONFIG_DMABUF_HEAPS_SYSTEM) += system_heap.o
obj-$(CONFIG_DMABUF_HEAPS_CMA) += cma_heap.o
diff --git a/drivers/dma-buf/heaps/carveout_heap.c b/drivers/dma-buf/heaps/carveout_heap.c
new file mode 100644
index 000000000000..896ca67e6bd9
--- /dev/null
+++ b/drivers/dma-buf/heaps/carveout_heap.c
@@ -0,0 +1,314 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/dma-buf.h>
+#include <linux/dma-heap.h>
+#include <linux/genalloc.h>
+#include <linux/of_reserved_mem.h>
+
+struct carveout_heap_priv {
+ struct dma_heap *heap;
+ struct gen_pool *pool;
+};
+
+struct carveout_heap_buffer_priv {
+ struct mutex lock;
+ struct list_head attachments;
+
+ unsigned long len;
+ unsigned long num_pages;
+ struct carveout_heap_priv *heap;
+ void *buffer;
+};
+
+struct carveout_heap_attachment {
+ struct list_head head;
+ struct sg_table table;
+
+ struct device *dev;
+ bool mapped;
+};
+
+static int carveout_heap_attach(struct dma_buf *buf,
+ struct dma_buf_attachment *attachment)
+{
+ struct carveout_heap_buffer_priv *priv = buf->priv;
+ struct carveout_heap_attachment *a;
+ int ret;
+
+ a = kzalloc(sizeof(*a), GFP_KERNEL);
+ if (!a)
+ return -ENOMEM;
+ INIT_LIST_HEAD(&a->head);
+ a->dev = attachment->dev;
+ attachment->priv = a;
+
+ ret = sg_alloc_table(&a->table, priv->num_pages, GFP_KERNEL);
+ if (ret)
+ goto err_cleanup_attach;
+
+ mutex_lock(&priv->lock);
+ list_add(&a->head, &priv->attachments);
+ mutex_unlock(&priv->lock);
+
+ return 0;
+
+err_cleanup_attach:
+ kfree(a);
+ return ret;
+}
+
+static void carveout_heap_detach(struct dma_buf *dmabuf,
+ struct dma_buf_attachment *attachment)
+{
+ struct carveout_heap_buffer_priv *priv = dmabuf->priv;
+ struct carveout_heap_attachment *a = attachment->priv;
+
+ mutex_lock(&priv->lock);
+ list_del(&a->head);
+ mutex_unlock(&priv->lock);
+
+ sg_free_table(&a->table);
+ kfree(a);
+}
+
+static struct sg_table *
+carveout_heap_map_dma_buf(struct dma_buf_attachment *attachment,
+ enum dma_data_direction direction)
+{
+ struct carveout_heap_attachment *a = attachment->priv;
+ struct sg_table *table = &a->table;
+ int ret;
+
+ ret = dma_map_sgtable(a->dev, table, direction, 0);
+ if (ret)
+ return ERR_PTR(-ENOMEM);
+
+ a->mapped = true;
+
+ return table;
+}
+
+static void carveout_heap_unmap_dma_buf(struct dma_buf_attachment *attachment,
+ struct sg_table *table,
+ enum dma_data_direction direction)
+{
+ struct carveout_heap_attachment *a = attachment->priv;
+
+ a->mapped = false;
+ dma_unmap_sgtable(a->dev, table, direction, 0);
+}
+
+static int
+carveout_heap_dma_buf_begin_cpu_access(struct dma_buf *dmabuf,
+ enum dma_data_direction direction)
+{
+ struct carveout_heap_buffer_priv *priv = dmabuf->priv;
+ struct carveout_heap_attachment *a;
+
+ mutex_lock(&priv->lock);
+
+ list_for_each_entry(a, &priv->attachments, head) {
+ if (!a->mapped)
+ continue;
+
+ dma_sync_sgtable_for_cpu(a->dev, &a->table, direction);
+ }
+
+ mutex_unlock(&priv->lock);
+
+ return 0;
+}
+
+static int
+carveout_heap_dma_buf_end_cpu_access(struct dma_buf *dmabuf,
+ enum dma_data_direction direction)
+{
+ struct carveout_heap_buffer_priv *priv = dmabuf->priv;
+ struct carveout_heap_attachment *a;
+
+ mutex_lock(&priv->lock);
+
+ list_for_each_entry(a, &priv->attachments, head) {
+ if (!a->mapped)
+ continue;
+
+ dma_sync_sgtable_for_device(a->dev, &a->table, direction);
+ }
+
+ mutex_unlock(&priv->lock);
+
+ return 0;
+}
+
+static int carveout_heap_mmap(struct dma_buf *dmabuf,
+ struct vm_area_struct *vma)
+{
+ struct carveout_heap_buffer_priv *priv = dmabuf->priv;
+ struct page *page = virt_to_page(priv->buffer);
+
+ return remap_pfn_range(vma, vma->vm_start, page_to_pfn(page),
+ priv->num_pages * PAGE_SIZE, vma->vm_page_prot);
+}
+
+static void carveout_heap_dma_buf_release(struct dma_buf *buf)
+{
+ struct carveout_heap_buffer_priv *buffer_priv = buf->priv;
+ struct carveout_heap_priv *heap_priv = buffer_priv->heap;
+
+ gen_pool_free(heap_priv->pool, (unsigned long)buffer_priv->buffer,
+ buffer_priv->len);
+ kfree(buffer_priv);
+}
+
+static const struct dma_buf_ops carveout_heap_buf_ops = {
+ .attach = carveout_heap_attach,
+ .detach = carveout_heap_detach,
+ .map_dma_buf = carveout_heap_map_dma_buf,
+ .unmap_dma_buf = carveout_heap_unmap_dma_buf,
+ .begin_cpu_access = carveout_heap_dma_buf_begin_cpu_access,
+ .end_cpu_access = carveout_heap_dma_buf_end_cpu_access,
+ .mmap = carveout_heap_mmap,
+ .release = carveout_heap_dma_buf_release,
+};
+
+static struct dma_buf *carveout_heap_allocate(struct dma_heap *heap,
+ unsigned long len,
+ unsigned long fd_flags,
+ unsigned long heap_flags)
+{
+ struct carveout_heap_priv *heap_priv = dma_heap_get_drvdata(heap);
+ struct carveout_heap_buffer_priv *buffer_priv;
+ DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
+ struct dma_buf *buf;
+ dma_addr_t daddr;
+ void *buffer;
+ int ret;
+
+ buffer_priv = kzalloc(sizeof(*buffer_priv), GFP_KERNEL);
+ if (!buffer_priv)
+ return ERR_PTR(-ENOMEM);
+
+ INIT_LIST_HEAD(&buffer_priv->attachments);
+ mutex_init(&buffer_priv->lock);
+
+ buffer = gen_pool_dma_zalloc(heap_priv->pool, len, &daddr);
+ if (!buffer) {
+ ret = -ENOMEM;
+ goto err_free_buffer_priv;
+ }
+
+ buffer_priv->buffer = buffer;
+ buffer_priv->heap = heap_priv;
+ buffer_priv->len = len;
+ buffer_priv->num_pages = len / PAGE_SIZE;
+
+ /* create the dmabuf */
+ exp_info.exp_name = dma_heap_get_name(heap);
+ exp_info.ops = &carveout_heap_buf_ops;
+ exp_info.size = buffer_priv->len;
+ exp_info.flags = fd_flags;
+ exp_info.priv = buffer_priv;
+
+ buf = dma_buf_export(&exp_info);
+ if (IS_ERR(buf)) {
+ ret = PTR_ERR(buf);
+ goto err_free_buffer;
+ }
+
+ return buf;
+
+err_free_buffer:
+ gen_pool_free(heap_priv->pool, (unsigned long)buffer, len);
+err_free_buffer_priv:
+ kfree(buffer_priv);
+
+ return ERR_PTR(ret);
+}
+
+static const struct dma_heap_ops carveout_heap_ops = {
+ .allocate = carveout_heap_allocate,
+};
+
+static int __init carveout_heap_setup(struct device_node *node)
+{
+ struct dma_heap_export_info exp_info = {};
+ const struct reserved_mem *rmem;
+ struct carveout_heap_priv *priv;
+ struct dma_heap *heap;
+ struct gen_pool *pool;
+ void *base;
+ int ret;
+
+ rmem = of_reserved_mem_lookup(node);
+ if (!rmem)
+ return -EINVAL;
+
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ pool = gen_pool_create(PAGE_SHIFT, NUMA_NO_NODE);
+ if (!pool) {
+ ret = -ENOMEM;
+ goto err_cleanup_heap;
+ }
+ priv->pool = pool;
+
+ base = memremap(rmem->base, rmem->size, MEMREMAP_WB);
+ if (!base) {
+ ret = -ENOMEM;
+ goto err_release_mem_region;
+ }
+
+ ret = gen_pool_add_virt(pool, (unsigned long)base, rmem->base,
+ rmem->size, NUMA_NO_NODE);
+ if (ret)
+ goto err_unmap;
+
+ exp_info.name = node->full_name;
+ exp_info.ops = &carveout_heap_ops;
+ exp_info.priv = priv;
+
+ heap = dma_heap_add(&exp_info);
+ if (IS_ERR(heap)) {
+ ret = PTR_ERR(heap);
+ goto err_cleanup_pool_region;
+ }
+ priv->heap = heap;
+
+ return 0;
+
+err_cleanup_pool_region:
+ gen_pool_free(pool, (unsigned long)base, rmem->size);
+err_unmap:
+ memunmap(base);
+err_release_mem_region:
+ gen_pool_destroy(pool);
+err_cleanup_heap:
+ kfree(priv);
+ return ret;
+}
+
+static int __init carveout_heap_init(void)
+{
+ struct device_node *rmem_node;
+ struct device_node *node;
+ int ret;
+
+ rmem_node = of_find_node_by_path("/reserved-memory");
+ if (!rmem_node)
+ return 0;
+
+ for_each_child_of_node(rmem_node, node) {
+ if (!of_property_read_bool(node, "export"))
+ continue;
+
+ ret = carveout_heap_setup(node);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+module_init(carveout_heap_init);
--
2.44.0
Some systems run with ECC enabled on the memory by default, but with
some memory regions with ECC disabled to mitigate the downsides of
enabling ECC (reduced performances, increased memory usage) for buffers
that don't require it.
Let's create some allocation flags to ask for a particular ECC setup
when allocate a dma-buf through a heap.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/dma-buf/dma-heap.c | 4 ++++
include/uapi/linux/dma-heap.h | 5 +++--
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
index 84ae708fafe7..a96c1865b627 100644
--- a/drivers/dma-buf/dma-heap.c
+++ b/drivers/dma-buf/dma-heap.c
@@ -106,10 +106,14 @@ static long dma_heap_ioctl_allocate(struct file *file, void *data)
return -EINVAL;
if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
return -EINVAL;
+ if ((heap_allocation->heap_flags & DMA_HEAP_FLAG_ECC_PROTECTED) &&
+ (heap_allocation->heap_flags & DMA_HEAP_FLAG_ECC_UNPROTECTED))
+ return -EINVAL;
+
fd = dma_heap_buffer_alloc(heap, heap_allocation->len,
heap_allocation->fd_flags,
heap_allocation->heap_flags);
if (fd < 0)
return fd;
diff --git a/include/uapi/linux/dma-heap.h b/include/uapi/linux/dma-heap.h
index 6f84fa08e074..677b6355552d 100644
--- a/include/uapi/linux/dma-heap.h
+++ b/include/uapi/linux/dma-heap.h
@@ -16,12 +16,13 @@
*/
/* Valid FD_FLAGS are O_CLOEXEC, O_RDONLY, O_WRONLY, O_RDWR */
#define DMA_HEAP_VALID_FD_FLAGS (O_CLOEXEC | O_ACCMODE)
-/* Currently no heap flags */
-#define DMA_HEAP_VALID_HEAP_FLAGS (0)
+#define DMA_HEAP_FLAG_ECC_PROTECTED BIT(0)
+#define DMA_HEAP_FLAG_ECC_UNPROTECTED BIT(1)
+#define DMA_HEAP_VALID_HEAP_FLAGS (DMA_HEAP_FLAG_ECC_PROTECTED | DMA_HEAP_FLAG_ECC_UNPROTECTED)
/**
* struct dma_heap_allocation_data - metadata passed from userspace for
* allocations
* @len: size of the allocation
--
2.44.0
The system heap has been using its struct dma_heap pointer but wasn't
using it anywhere.
Since we'll need additional parameters to attach to that heap type,
let's create a private structure and set it as the dma_heap drvdata,
removing the global variable in the process.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/dma-buf/heaps/system_heap.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 9076d47ed2ef..8b5e6344eea4 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -19,11 +19,13 @@
#include <linux/module.h>
#include <linux/scatterlist.h>
#include <linux/slab.h>
#include <linux/vmalloc.h>
-static struct dma_heap *sys_heap;
+struct system_heap {
+ struct dma_heap *heap;
+};
struct system_heap_buffer {
struct dma_heap *heap;
struct list_head attachments;
struct mutex lock;
@@ -422,17 +424,22 @@ static const struct dma_heap_ops system_heap_ops = {
};
static int system_heap_create(void)
{
struct dma_heap_export_info exp_info;
+ struct system_heap *sys_heap;
+
+ sys_heap = kzalloc(sizeof(*sys_heap), GFP_KERNEL);
+ if (!sys_heap)
+ return -ENOMEM;
exp_info.name = "system";
exp_info.ops = &system_heap_ops;
- exp_info.priv = NULL;
+ exp_info.priv = sys_heap;
- sys_heap = dma_heap_add(&exp_info);
- if (IS_ERR(sys_heap))
- return PTR_ERR(sys_heap);
+ sys_heap->heap = dma_heap_add(&exp_info);
+ if (IS_ERR(sys_heap->heap))
+ return PTR_ERR(sys_heap->heap);
return 0;
}
module_init(system_heap_create);
--
2.44.0
Now that we have introduced ECC-related flags for the dma-heaps buffer
allocations, let's honour these flags depending on the memory setup.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/dma-buf/heaps/system_heap.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 8b5e6344eea4..dd7c8b6f9cf6 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -18,13 +18,15 @@
#include <linux/mm.h>
#include <linux/module.h>
#include <linux/scatterlist.h>
#include <linux/slab.h>
#include <linux/vmalloc.h>
+#include <linux/of.h>
struct system_heap {
struct dma_heap *heap;
+ bool ecc_enabled;
};
struct system_heap_buffer {
struct dma_heap *heap;
struct list_head attachments;
@@ -336,10 +338,11 @@ static struct page *alloc_largest_available(unsigned long size,
static struct dma_buf *system_heap_allocate(struct dma_heap *heap,
unsigned long len,
unsigned long fd_flags,
unsigned long heap_flags)
{
+ struct system_heap *sys_heap = dma_heap_get_drvdata(heap);
struct system_heap_buffer *buffer;
DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
unsigned long size_remaining = len;
unsigned int max_order = orders[0];
struct dma_buf *dmabuf;
@@ -347,10 +350,16 @@ static struct dma_buf *system_heap_allocate(struct dma_heap *heap,
struct scatterlist *sg;
struct list_head pages;
struct page *page, *tmp_page;
int i, ret = -ENOMEM;
+ if (!sys_heap->ecc_enabled && (heap_flags & DMA_HEAP_FLAG_ECC_PROTECTED))
+ return ERR_PTR(-EINVAL);
+
+ if (sys_heap->ecc_enabled && (heap_flags & DMA_HEAP_FLAG_ECC_UNPROTECTED))
+ return ERR_PTR(-EINVAL);
+
buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
if (!buffer)
return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(&buffer->attachments);
@@ -430,10 +439,13 @@ static int system_heap_create(void)
sys_heap = kzalloc(sizeof(*sys_heap), GFP_KERNEL);
if (!sys_heap)
return -ENOMEM;
+ if (of_memory_get_ecc_correction_bits() > 0)
+ sys_heap->ecc_enabled = true;
+
exp_info.name = "system";
exp_info.ops = &system_heap_ops;
exp_info.priv = sys_heap;
sys_heap->heap = dma_heap_add(&exp_info);
--
2.44.0
Now that we have introduced ECC-related flags for the dma-heaps buffer
allocations, let's honour these flags depending on the memory setup.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/dma-buf/heaps/cma_heap.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
index 4a63567e93ba..1e6babbd8eb5 100644
--- a/drivers/dma-buf/heaps/cma_heap.c
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -24,10 +24,11 @@
struct cma_heap {
struct dma_heap *heap;
struct cma *cma;
+ bool ecc_enabled;
};
struct cma_heap_buffer {
struct cma_heap *heap;
struct list_head attachments;
@@ -286,10 +287,16 @@ static struct dma_buf *cma_heap_allocate(struct dma_heap *heap,
struct page *cma_pages;
struct dma_buf *dmabuf;
int ret = -ENOMEM;
pgoff_t pg;
+ if (!cma_heap->ecc_enabled && (heap_flags & DMA_HEAP_FLAG_ECC_PROTECTED))
+ return -EINVAL;
+
+ if (cma_heap->ecc_enabled && (heap_flags & DMA_HEAP_FLAG_ECC_UNPROTECTED))
+ return -EINVAL;
+
buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
if (!buffer)
return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(&buffer->attachments);
@@ -374,10 +381,13 @@ static int __add_cma_heap(struct cma *cma, void *data)
cma_heap = kzalloc(sizeof(*cma_heap), GFP_KERNEL);
if (!cma_heap)
return -ENOMEM;
cma_heap->cma = cma;
+ if (of_memory_get_ecc_correction_bits() > 0)
+ cma_heap->ecc_enabled = true;
+
exp_info.name = cma_get_name(cma);
exp_info.ops = &cma_heap_ops;
exp_info.priv = cma_heap;
cma_heap->heap = dma_heap_add(&exp_info);
--
2.44.0
Now that we have introduced ECC-related flags for the dma-heaps buffer
allocations, let's honour these flags depending on the memory setup.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/dma-buf/heaps/carveout_heap.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/dma-buf/heaps/carveout_heap.c b/drivers/dma-buf/heaps/carveout_heap.c
index 896ca67e6bd9..81b167785999 100644
--- a/drivers/dma-buf/heaps/carveout_heap.c
+++ b/drivers/dma-buf/heaps/carveout_heap.c
@@ -6,10 +6,11 @@
#include <linux/of_reserved_mem.h>
struct carveout_heap_priv {
struct dma_heap *heap;
struct gen_pool *pool;
+ bool ecc_enabled;
};
struct carveout_heap_buffer_priv {
struct mutex lock;
struct list_head attachments;
@@ -182,10 +183,16 @@ static struct dma_buf *carveout_heap_allocate(struct dma_heap *heap,
struct dma_buf *buf;
dma_addr_t daddr;
void *buffer;
int ret;
+ if (!heap_priv->ecc_enabled && (heap_flags & DMA_HEAP_FLAG_ECC_PROTECTED))
+ return ERR_PTR(-EINVAL);
+
+ if (heap_priv->ecc_enabled && (heap_flags & DMA_HEAP_FLAG_ECC_UNPROTECTED))
+ return ERR_PTR(-EINVAL);
+
buffer_priv = kzalloc(sizeof(*buffer_priv), GFP_KERNEL);
if (!buffer_priv)
return ERR_PTR(-ENOMEM);
INIT_LIST_HEAD(&buffer_priv->attachments);
@@ -235,20 +242,29 @@ static int __init carveout_heap_setup(struct device_node *node)
const struct reserved_mem *rmem;
struct carveout_heap_priv *priv;
struct dma_heap *heap;
struct gen_pool *pool;
void *base;
+ u32 val = 0;
int ret;
rmem = of_reserved_mem_lookup(node);
if (!rmem)
return -EINVAL;
priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
+ of_property_read_u32(node, "ecc-correction-bits", &val);
+ if (val <= 0) {
+ if (of_memory_get_ecc_correction_bits() > 0)
+ priv->ecc_enabled = true;
+ } else {
+ priv->ecc_enabled = true;
+ }
+
pool = gen_pool_create(PAGE_SHIFT, NUMA_NO_NODE);
if (!pool) {
ret = -ENOMEM;
goto err_cleanup_heap;
}
--
2.44.0
The /memory device tree bindings allow to store the ECC detection and
correction bits through the ecc-detection-bits and ecc-correction-bits
properties.
Our next patches rely on whether ECC is enabled, so let's add a helper
to retrieve the ECC correction bits from the /memory node.
Signed-off-by: Maxime Ripard <[email protected]>
---
include/linux/of.h | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/include/linux/of.h b/include/linux/of.h
index a0bedd038a05..2fbee65a7aa9 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -1510,10 +1510,35 @@ static inline int of_get_available_child_count(const struct device_node *np)
num++;
return num;
}
+/**
+ * of_memory_get_ecc_correction_bits() - Returns the number of ECC correction bits
+ *
+ * Search for the number of bits in memory that can be corrected by the
+ * ECC algorithm.
+ *
+ * Returns:
+ * The number of ECC bits, 0 if there's no ECC support, a negative error
+ * code on failure.
+ */
+static inline int of_memory_get_ecc_correction_bits(void)
+{
+ struct device_node *mem;
+ u32 val = 0;
+
+ mem = of_find_node_by_path("/memory");
+ if (!mem)
+ return -ENODEV;
+
+ of_property_read_u32(mem, "ecc-correction-bits", &val);
+ of_node_put(mem);
+
+ return val;
+}
+
#define _OF_DECLARE_STUB(table, name, compat, fn, fn_type) \
static const struct of_device_id __of_table_##name \
__attribute__((unused)) \
= { .compatible = compat, \
.data = (fn == (fn_type)NULL) ? fn : fn }
--
2.44.0
The uAPI header has a bunch of constants and structures that might be
handy in drivers.
Let's include the header in the driver-side dma-heap header.
Signed-off-by: Maxime Ripard <[email protected]>
---
include/linux/dma-heap.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
index 0c05561cad6e..e7cf110c5fdc 100644
--- a/include/linux/dma-heap.h
+++ b/include/linux/dma-heap.h
@@ -10,10 +10,12 @@
#define _DMA_HEAPS_H
#include <linux/cdev.h>
#include <linux/types.h>
+#include <uapi/linux/dma-heap.h>
+
struct dma_heap;
/**
* struct dma_heap_ops - ops to operate on a given heap
* @allocate: allocate dmabuf and return struct dma_buf ptr
--
2.44.0
On Wed, May 15, 2024 at 6:57 AM Maxime Ripard <[email protected]> wrote:
> This series is the follow-up of the discussion that John and I had a few
> months ago here:
>
> https://lore.kernel.org/all/CANDhNCquJn6bH3KxKf65BWiTYLVqSd9892-xtFDHHqqyrroCMQ@mail.gmail.com/
>
> The initial problem we were discussing was that I'm currently working on
> a platform which has a memory layout with ECC enabled. However, enabling
> the ECC has a number of drawbacks on that platform: lower performance,
> increased memory usage, etc. So for things like framebuffers, the
> trade-off isn't great and thus there's a memory region with ECC disabled
> to allocate from for such use cases.
>
> After a suggestion from John, I chose to start using heap allocations
> flags to allow for userspace to ask for a particular ECC setup. This is
> then backed by a new heap type that runs from reserved memory chunks
> flagged as such, and the existing DT properties to specify the ECC
> properties.
>
> We could also easily extend this mechanism to support more flags, or
> through a new ioctl to discover which flags a given heap supports.
Hey! Thanks for sending this along! I'm eager to see more heap related
work being done upstream.
The only thing that makes me a bit hesitant, is the introduction of
allocation flags (as opposed to a uniquely specified/named "ecc"
heap).
We did talk about this earlier, and my earlier press that only if the
ECC flag was general enough to apply to the majority of heaps then it
makes sense as a flag, and your patch here does apply it to all the
heaps. So I don't have an objection.
But it makes me a little nervous to add a new generic allocation flag
for a feature most hardware doesn't support (yet, at least). So it's
hard to weigh how common the actual usage will be across all the
heaps.
I apologize as my worry is mostly born out of seeing vendors really
push opaque feature flags in their old ion heaps, so in providing a
flags argument, it was mostly intended as an escape hatch for
obviously common attributes. So having the first be something that
seems reasonable, but isn't actually that common makes me fret some.
So again, not an objection, just something for folks to stew on to
make sure this is really the right approach.
Another thing to discuss, that I didn't see in your mail: Do we have
an open-source user of this new flag?
thanks
-john
Hi Maxime,
kernel test robot noticed the following build warnings:
[auto build test WARNING on a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6]
url: https://github.com/intel-lab-lkp/linux/commits/Maxime-Ripard/dma-buf-heaps-Introduce-a-new-heap-for-reserved-memory/20240515-215850
base: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
patch link: https://lore.kernel.org/r/20240515-dma-buf-ecc-heap-v1-7-54cbbd049511%40kernel.org
patch subject: [PATCH 7/8] dma-buf: heaps: cma: Handle ECC flags
config: mips-allmodconfig (https://download.01.org/0day-ci/archive/20240516/[email protected]/config)
compiler: mips-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240516/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/dma-buf/heaps/cma_heap.c: In function 'cma_heap_allocate':
>> drivers/dma-buf/heaps/cma_heap.c:293:24: warning: returning 'int' from a function with return type 'struct dma_buf *' makes pointer from integer without a cast [-Wint-conversion]
293 | return -EINVAL;
| ^
drivers/dma-buf/heaps/cma_heap.c:296:24: warning: returning 'int' from a function with return type 'struct dma_buf *' makes pointer from integer without a cast [-Wint-conversion]
296 | return -EINVAL;
| ^
drivers/dma-buf/heaps/cma_heap.c: In function '__add_cma_heap':
drivers/dma-buf/heaps/cma_heap.c:386:13: error: implicit declaration of function 'of_memory_get_ecc_correction_bits' [-Werror=implicit-function-declaration]
386 | if (of_memory_get_ecc_correction_bits() > 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +293 drivers/dma-buf/heaps/cma_heap.c
275
276 static struct dma_buf *cma_heap_allocate(struct dma_heap *heap,
277 unsigned long len,
278 unsigned long fd_flags,
279 unsigned long heap_flags)
280 {
281 struct cma_heap *cma_heap = dma_heap_get_drvdata(heap);
282 struct cma_heap_buffer *buffer;
283 DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
284 size_t size = PAGE_ALIGN(len);
285 pgoff_t pagecount = size >> PAGE_SHIFT;
286 unsigned long align = get_order(size);
287 struct page *cma_pages;
288 struct dma_buf *dmabuf;
289 int ret = -ENOMEM;
290 pgoff_t pg;
291
292 if (!cma_heap->ecc_enabled && (heap_flags & DMA_HEAP_FLAG_ECC_PROTECTED))
> 293 return -EINVAL;
294
295 if (cma_heap->ecc_enabled && (heap_flags & DMA_HEAP_FLAG_ECC_UNPROTECTED))
296 return -EINVAL;
297
298 buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
299 if (!buffer)
300 return ERR_PTR(-ENOMEM);
301
302 INIT_LIST_HEAD(&buffer->attachments);
303 mutex_init(&buffer->lock);
304 buffer->len = size;
305
306 if (align > CONFIG_CMA_ALIGNMENT)
307 align = CONFIG_CMA_ALIGNMENT;
308
309 cma_pages = cma_alloc(cma_heap->cma, pagecount, align, false);
310 if (!cma_pages)
311 goto free_buffer;
312
313 /* Clear the cma pages */
314 if (PageHighMem(cma_pages)) {
315 unsigned long nr_clear_pages = pagecount;
316 struct page *page = cma_pages;
317
318 while (nr_clear_pages > 0) {
319 void *vaddr = kmap_atomic(page);
320
321 memset(vaddr, 0, PAGE_SIZE);
322 kunmap_atomic(vaddr);
323 /*
324 * Avoid wasting time zeroing memory if the process
325 * has been killed by by SIGKILL
326 */
327 if (fatal_signal_pending(current))
328 goto free_cma;
329 page++;
330 nr_clear_pages--;
331 }
332 } else {
333 memset(page_address(cma_pages), 0, size);
334 }
335
336 buffer->pages = kmalloc_array(pagecount, sizeof(*buffer->pages), GFP_KERNEL);
337 if (!buffer->pages) {
338 ret = -ENOMEM;
339 goto free_cma;
340 }
341
342 for (pg = 0; pg < pagecount; pg++)
343 buffer->pages[pg] = &cma_pages[pg];
344
345 buffer->cma_pages = cma_pages;
346 buffer->heap = cma_heap;
347 buffer->pagecount = pagecount;
348
349 /* create the dmabuf */
350 exp_info.exp_name = dma_heap_get_name(heap);
351 exp_info.ops = &cma_heap_buf_ops;
352 exp_info.size = buffer->len;
353 exp_info.flags = fd_flags;
354 exp_info.priv = buffer;
355 dmabuf = dma_buf_export(&exp_info);
356 if (IS_ERR(dmabuf)) {
357 ret = PTR_ERR(dmabuf);
358 goto free_pages;
359 }
360 return dmabuf;
361
362 free_pages:
363 kfree(buffer->pages);
364 free_cma:
365 cma_release(cma_heap->cma, cma_pages, pagecount);
366 free_buffer:
367 kfree(buffer);
368
369 return ERR_PTR(ret);
370 }
371
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Am 15.05.24 um 15:56 schrieb Maxime Ripard:
> The uAPI header has a bunch of constants and structures that might be
> handy in drivers.
>
> Let's include the header in the driver-side dma-heap header.
Well as long as this header doesn't need any symbols from the uAPI
itself I think that is a no-go.
Includes should only be applied for things which are really necessary
and not because some driver might need it.
Regards,
Christian.
>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> include/linux/dma-heap.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> index 0c05561cad6e..e7cf110c5fdc 100644
> --- a/include/linux/dma-heap.h
> +++ b/include/linux/dma-heap.h
> @@ -10,10 +10,12 @@
> #define _DMA_HEAPS_H
>
> #include <linux/cdev.h>
> #include <linux/types.h>
>
> +#include <uapi/linux/dma-heap.h>
> +
> struct dma_heap;
>
> /**
> * struct dma_heap_ops - ops to operate on a given heap
> * @allocate: allocate dmabuf and return struct dma_buf ptr
>
On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote:
> On Wed, May 15, 2024 at 6:57 AM Maxime Ripard <[email protected]> wrote:
> > This series is the follow-up of the discussion that John and I had a few
> > months ago here:
> >
> > https://lore.kernel.org/all/CANDhNCquJn6bH3KxKf65BWiTYLVqSd9892-xtFDHHqqyrroCMQ@mail.gmail.com/
> >
> > The initial problem we were discussing was that I'm currently working on
> > a platform which has a memory layout with ECC enabled. However, enabling
> > the ECC has a number of drawbacks on that platform: lower performance,
> > increased memory usage, etc. So for things like framebuffers, the
> > trade-off isn't great and thus there's a memory region with ECC disabled
> > to allocate from for such use cases.
> >
> > After a suggestion from John, I chose to start using heap allocations
> > flags to allow for userspace to ask for a particular ECC setup. This is
> > then backed by a new heap type that runs from reserved memory chunks
> > flagged as such, and the existing DT properties to specify the ECC
> > properties.
> >
> > We could also easily extend this mechanism to support more flags, or
> > through a new ioctl to discover which flags a given heap supports.
>
> Hey! Thanks for sending this along! I'm eager to see more heap related
> work being done upstream.
>
> The only thing that makes me a bit hesitant, is the introduction of
> allocation flags (as opposed to a uniquely specified/named "ecc"
> heap).
>
> We did talk about this earlier, and my earlier press that only if the
> ECC flag was general enough to apply to the majority of heaps then it
> makes sense as a flag, and your patch here does apply it to all the
> heaps. So I don't have an objection.
>
> But it makes me a little nervous to add a new generic allocation flag
> for a feature most hardware doesn't support (yet, at least). So it's
> hard to weigh how common the actual usage will be across all the
> heaps.
>
> I apologize as my worry is mostly born out of seeing vendors really
> push opaque feature flags in their old ion heaps, so in providing a
> flags argument, it was mostly intended as an escape hatch for
> obviously common attributes. So having the first be something that
> seems reasonable, but isn't actually that common makes me fret some.
>
> So again, not an objection, just something for folks to stew on to
> make sure this is really the right approach.
Another good reason to go with full heap names instead of opaque flags on
existing heaps is that with the former we can use symlinks in sysfs to
specify heaps, with the latter we need a new idea. We haven't yet gotten
around to implement this anywhere, but it's been in the dma-buf/heap todo
since forever, and I like it as a design approach. So would be a good idea
to not toss it. With that display would have symlinks to cma-ecc and cma,
and rendering maybe cma-ecc, shmem, cma heaps (in priority order) for a
SoC where the display needs contig memory for scanout.
> Another thing to discuss, that I didn't see in your mail: Do we have
> an open-source user of this new flag?
I think one option might be to just start using these internally, but not
sure the dma-api would understand a fallback cadence of allocators (afaik
you can specify specific cma regions already, but that doesn't really
covere the case where you can fall back to pages and iommu to remap to
contig dma space) ... And I don't think abandonding the dma-api for
allocating cma buffers is going to be a popular proposal.
-Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi John,
Thanks for your feedback
On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote:
> On Wed, May 15, 2024 at 6:57 AM Maxime Ripard <[email protected]> wrote:
> > This series is the follow-up of the discussion that John and I had a few
> > months ago here:
> >
> > https://lore.kernel.org/all/CANDhNCquJn6bH3KxKf65BWiTYLVqSd9892-xtFDHHqqyrroCMQ@mail.gmail.com/
> >
> > The initial problem we were discussing was that I'm currently working on
> > a platform which has a memory layout with ECC enabled. However, enabling
> > the ECC has a number of drawbacks on that platform: lower performance,
> > increased memory usage, etc. So for things like framebuffers, the
> > trade-off isn't great and thus there's a memory region with ECC disabled
> > to allocate from for such use cases.
> >
> > After a suggestion from John, I chose to start using heap allocations
> > flags to allow for userspace to ask for a particular ECC setup. This is
> > then backed by a new heap type that runs from reserved memory chunks
> > flagged as such, and the existing DT properties to specify the ECC
> > properties.
> >
> > We could also easily extend this mechanism to support more flags, or
> > through a new ioctl to discover which flags a given heap supports.
>
> Hey! Thanks for sending this along! I'm eager to see more heap related
> work being done upstream.
>
> The only thing that makes me a bit hesitant, is the introduction of
> allocation flags (as opposed to a uniquely specified/named "ecc"
> heap).
>
> We did talk about this earlier, and my earlier press that only if the
> ECC flag was general enough to apply to the majority of heaps then it
> makes sense as a flag, and your patch here does apply it to all the
> heaps. So I don't have an objection.
>
> But it makes me a little nervous to add a new generic allocation flag
> for a feature most hardware doesn't support (yet, at least). So it's
> hard to weigh how common the actual usage will be across all the
> heaps.
>
> I apologize as my worry is mostly born out of seeing vendors really
> push opaque feature flags in their old ion heaps, so in providing a
> flags argument, it was mostly intended as an escape hatch for
> obviously common attributes. So having the first be something that
> seems reasonable, but isn't actually that common makes me fret some.
>
> So again, not an objection, just something for folks to stew on to
> make sure this is really the right approach.
I understand your hesitation and concern :) Is there anything we could
provide that would help moving the discussion forward?
> Another thing to discuss, that I didn't see in your mail: Do we have
> an open-source user of this new flag?
Not at the moment. I guess it would be one of the things that would
reduce your concerns, but is it a requirement?
Thanks!
Maxime
On Thu, May 16, 2024 at 12:56:27PM +0200, Daniel Vetter wrote:
> On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote:
> > On Wed, May 15, 2024 at 6:57 AM Maxime Ripard <[email protected]> wrote:
> > > This series is the follow-up of the discussion that John and I had a few
> > > months ago here:
> > >
> > > https://lore.kernel.org/all/CANDhNCquJn6bH3KxKf65BWiTYLVqSd9892-xtFDHHqqyrroCMQ@mail.gmail.com/
> > >
> > > The initial problem we were discussing was that I'm currently working on
> > > a platform which has a memory layout with ECC enabled. However, enabling
> > > the ECC has a number of drawbacks on that platform: lower performance,
> > > increased memory usage, etc. So for things like framebuffers, the
> > > trade-off isn't great and thus there's a memory region with ECC disabled
> > > to allocate from for such use cases.
> > >
> > > After a suggestion from John, I chose to start using heap allocations
> > > flags to allow for userspace to ask for a particular ECC setup. This is
> > > then backed by a new heap type that runs from reserved memory chunks
> > > flagged as such, and the existing DT properties to specify the ECC
> > > properties.
> > >
> > > We could also easily extend this mechanism to support more flags, or
> > > through a new ioctl to discover which flags a given heap supports.
> >
> > Hey! Thanks for sending this along! I'm eager to see more heap related
> > work being done upstream.
> >
> > The only thing that makes me a bit hesitant, is the introduction of
> > allocation flags (as opposed to a uniquely specified/named "ecc"
> > heap).
> >
> > We did talk about this earlier, and my earlier press that only if the
> > ECC flag was general enough to apply to the majority of heaps then it
> > makes sense as a flag, and your patch here does apply it to all the
> > heaps. So I don't have an objection.
> >
> > But it makes me a little nervous to add a new generic allocation flag
> > for a feature most hardware doesn't support (yet, at least). So it's
> > hard to weigh how common the actual usage will be across all the
> > heaps.
> >
> > I apologize as my worry is mostly born out of seeing vendors really
> > push opaque feature flags in their old ion heaps, so in providing a
> > flags argument, it was mostly intended as an escape hatch for
> > obviously common attributes. So having the first be something that
> > seems reasonable, but isn't actually that common makes me fret some.
> >
> > So again, not an objection, just something for folks to stew on to
> > make sure this is really the right approach.
>
> Another good reason to go with full heap names instead of opaque flags on
> existing heaps is that with the former we can use symlinks in sysfs to
> specify heaps, with the latter we need a new idea. We haven't yet gotten
> around to implement this anywhere, but it's been in the dma-buf/heap todo
> since forever, and I like it as a design approach. So would be a good idea
> to not toss it. With that display would have symlinks to cma-ecc and cma,
> and rendering maybe cma-ecc, shmem, cma heaps (in priority order) for a
> SoC where the display needs contig memory for scanout.
I guess it depends what we want to use the heaps for exactly. If we
create a heap by type, then the number of heaps is going to explode and
their name is going to be super weird and inconsistent.
Using the ECC setup here as an example, it means that we would need to
create system (with the default ECC setup for the system), system-ecc,
system-no-ecc, cma, cma-ecc, cma-no-ecc.
Let's say we introduce caching next. do we want to triple the number of
heaps again?
So I guess it all boils down to whether we want to consider heaps as
allocators, and then we need the flags to fine-tune the attributes/exact
semantics, or the combination of an allocator and the semantics which
will make the number of heaps explode (and reduce their general
usefulness, I guess).
Maxime
Hi Maxime,
kernel test robot noticed the following build errors:
[auto build test ERROR on a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6]
url: https://github.com/intel-lab-lkp/linux/commits/Maxime-Ripard/dma-buf-heaps-Introduce-a-new-heap-for-reserved-memory/20240515-215850
base: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
patch link: https://lore.kernel.org/r/20240515-dma-buf-ecc-heap-v1-7-54cbbd049511%40kernel.org
patch subject: [PATCH 7/8] dma-buf: heaps: cma: Handle ECC flags
config: mips-allmodconfig (https://download.01.org/0day-ci/archive/20240516/[email protected]/config)
compiler: mips-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240516/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
drivers/dma-buf/heaps/cma_heap.c: In function 'cma_heap_allocate':
drivers/dma-buf/heaps/cma_heap.c:293:24: warning: returning 'int' from a function with return type 'struct dma_buf *' makes pointer from integer without a cast [-Wint-conversion]
293 | return -EINVAL;
| ^
drivers/dma-buf/heaps/cma_heap.c:296:24: warning: returning 'int' from a function with return type 'struct dma_buf *' makes pointer from integer without a cast [-Wint-conversion]
296 | return -EINVAL;
| ^
drivers/dma-buf/heaps/cma_heap.c: In function '__add_cma_heap':
>> drivers/dma-buf/heaps/cma_heap.c:386:13: error: implicit declaration of function 'of_memory_get_ecc_correction_bits' [-Werror=implicit-function-declaration]
386 | if (of_memory_get_ecc_correction_bits() > 0)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/of_memory_get_ecc_correction_bits +386 drivers/dma-buf/heaps/cma_heap.c
275
276 static struct dma_buf *cma_heap_allocate(struct dma_heap *heap,
277 unsigned long len,
278 unsigned long fd_flags,
279 unsigned long heap_flags)
280 {
281 struct cma_heap *cma_heap = dma_heap_get_drvdata(heap);
282 struct cma_heap_buffer *buffer;
283 DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
284 size_t size = PAGE_ALIGN(len);
285 pgoff_t pagecount = size >> PAGE_SHIFT;
286 unsigned long align = get_order(size);
287 struct page *cma_pages;
288 struct dma_buf *dmabuf;
289 int ret = -ENOMEM;
290 pgoff_t pg;
291
292 if (!cma_heap->ecc_enabled && (heap_flags & DMA_HEAP_FLAG_ECC_PROTECTED))
> 293 return -EINVAL;
294
295 if (cma_heap->ecc_enabled && (heap_flags & DMA_HEAP_FLAG_ECC_UNPROTECTED))
296 return -EINVAL;
297
298 buffer = kzalloc(sizeof(*buffer), GFP_KERNEL);
299 if (!buffer)
300 return ERR_PTR(-ENOMEM);
301
302 INIT_LIST_HEAD(&buffer->attachments);
303 mutex_init(&buffer->lock);
304 buffer->len = size;
305
306 if (align > CONFIG_CMA_ALIGNMENT)
307 align = CONFIG_CMA_ALIGNMENT;
308
309 cma_pages = cma_alloc(cma_heap->cma, pagecount, align, false);
310 if (!cma_pages)
311 goto free_buffer;
312
313 /* Clear the cma pages */
314 if (PageHighMem(cma_pages)) {
315 unsigned long nr_clear_pages = pagecount;
316 struct page *page = cma_pages;
317
318 while (nr_clear_pages > 0) {
319 void *vaddr = kmap_atomic(page);
320
321 memset(vaddr, 0, PAGE_SIZE);
322 kunmap_atomic(vaddr);
323 /*
324 * Avoid wasting time zeroing memory if the process
325 * has been killed by by SIGKILL
326 */
327 if (fatal_signal_pending(current))
328 goto free_cma;
329 page++;
330 nr_clear_pages--;
331 }
332 } else {
333 memset(page_address(cma_pages), 0, size);
334 }
335
336 buffer->pages = kmalloc_array(pagecount, sizeof(*buffer->pages), GFP_KERNEL);
337 if (!buffer->pages) {
338 ret = -ENOMEM;
339 goto free_cma;
340 }
341
342 for (pg = 0; pg < pagecount; pg++)
343 buffer->pages[pg] = &cma_pages[pg];
344
345 buffer->cma_pages = cma_pages;
346 buffer->heap = cma_heap;
347 buffer->pagecount = pagecount;
348
349 /* create the dmabuf */
350 exp_info.exp_name = dma_heap_get_name(heap);
351 exp_info.ops = &cma_heap_buf_ops;
352 exp_info.size = buffer->len;
353 exp_info.flags = fd_flags;
354 exp_info.priv = buffer;
355 dmabuf = dma_buf_export(&exp_info);
356 if (IS_ERR(dmabuf)) {
357 ret = PTR_ERR(dmabuf);
358 goto free_pages;
359 }
360 return dmabuf;
361
362 free_pages:
363 kfree(buffer->pages);
364 free_cma:
365 cma_release(cma_heap->cma, cma_pages, pagecount);
366 free_buffer:
367 kfree(buffer);
368
369 return ERR_PTR(ret);
370 }
371
372 static const struct dma_heap_ops cma_heap_ops = {
373 .allocate = cma_heap_allocate,
374 };
375
376 static int __add_cma_heap(struct cma *cma, void *data)
377 {
378 struct cma_heap *cma_heap;
379 struct dma_heap_export_info exp_info;
380
381 cma_heap = kzalloc(sizeof(*cma_heap), GFP_KERNEL);
382 if (!cma_heap)
383 return -ENOMEM;
384 cma_heap->cma = cma;
385
> 386 if (of_memory_get_ecc_correction_bits() > 0)
387 cma_heap->ecc_enabled = true;
388
389 exp_info.name = cma_get_name(cma);
390 exp_info.ops = &cma_heap_ops;
391 exp_info.priv = cma_heap;
392
393 cma_heap->heap = dma_heap_add(&exp_info);
394 if (IS_ERR(cma_heap->heap)) {
395 int ret = PTR_ERR(cma_heap->heap);
396
397 kfree(cma_heap);
398 return ret;
399 }
400
401 return 0;
402 }
403
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Thu, May 16, 2024 at 3:56 AM Daniel Vetter <[email protected]> wrote:
> On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote:
> > But it makes me a little nervous to add a new generic allocation flag
> > for a feature most hardware doesn't support (yet, at least). So it's
> > hard to weigh how common the actual usage will be across all the
> > heaps.
> >
> > I apologize as my worry is mostly born out of seeing vendors really
> > push opaque feature flags in their old ion heaps, so in providing a
> > flags argument, it was mostly intended as an escape hatch for
> > obviously common attributes. So having the first be something that
> > seems reasonable, but isn't actually that common makes me fret some.
> >
> > So again, not an objection, just something for folks to stew on to
> > make sure this is really the right approach.
>
> Another good reason to go with full heap names instead of opaque flags on
> existing heaps is that with the former we can use symlinks in sysfs to
> specify heaps, with the latter we need a new idea. We haven't yet gotten
> around to implement this anywhere, but it's been in the dma-buf/heap todo
> since forever, and I like it as a design approach. So would be a good idea
> to not toss it. With that display would have symlinks to cma-ecc and cma,
> and rendering maybe cma-ecc, shmem, cma heaps (in priority order) for a
> SoC where the display needs contig memory for scanout.
So indeed that is a good point to keep in mind, but I also think it
might re-inforce the choice of having ECC as a flag here.
Since my understanding of the sysfs symlinks to heaps idea is about
being able to figure out a common heap from a collection of devices,
it's really about the ability for the driver to access the type of
memory. If ECC is just an attribute of the type of memory (as in this
patch series), it being on or off won't necessarily affect
compatibility of the buffer with the device. Similarly "uncached"
seems more of an attribute of memory type and not a type itself.
Hardware that can access non-contiguous "system" buffers can access
uncached system buffers.
thanks
-john
On Thu, May 16, 2024 at 5:22 AM Maxime Ripard <[email protected]> wrote:
> On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote:
> > I apologize as my worry is mostly born out of seeing vendors really
> > push opaque feature flags in their old ion heaps, so in providing a
> > flags argument, it was mostly intended as an escape hatch for
> > obviously common attributes. So having the first be something that
> > seems reasonable, but isn't actually that common makes me fret some.
> >
> > So again, not an objection, just something for folks to stew on to
> > make sure this is really the right approach.
>
> I understand your hesitation and concern :) Is there anything we could
> provide that would help moving the discussion forward?
>
Mostly I just want to make sure it's discussed, which is why I raise
it as a concern.
Getting APIs right is hard, and I know I've made my share of mistakes
(for instance, a situation that very much echoes this current
question: the *_ALARM clockids probably should have used a flag). So
I've found highlighting the trade-offs and getting other folk's
perspectives useful to avoid such issues. But I also don't intend to
needlessly delay things.
> > Another thing to discuss, that I didn't see in your mail: Do we have
> > an open-source user of this new flag?
>
> Not at the moment. I guess it would be one of the things that would
> reduce your concerns, but is it a requirement?
So I'd defer to Sima on this. There have been a number of heap
releated changes that have had to be held out of tree on this
requirement, but I'm hoping recent efforts on upstream device support
will eventually unblock those.
thanks
-john
On Thu, May 16, 2024 at 09:51:35AM -0700, John Stultz wrote:
> On Thu, May 16, 2024 at 3:56 AM Daniel Vetter <[email protected]> wrote:
> > On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote:
> > > But it makes me a little nervous to add a new generic allocation flag
> > > for a feature most hardware doesn't support (yet, at least). So it's
> > > hard to weigh how common the actual usage will be across all the
> > > heaps.
> > >
> > > I apologize as my worry is mostly born out of seeing vendors really
> > > push opaque feature flags in their old ion heaps, so in providing a
> > > flags argument, it was mostly intended as an escape hatch for
> > > obviously common attributes. So having the first be something that
> > > seems reasonable, but isn't actually that common makes me fret some.
> > >
> > > So again, not an objection, just something for folks to stew on to
> > > make sure this is really the right approach.
> >
> > Another good reason to go with full heap names instead of opaque flags on
> > existing heaps is that with the former we can use symlinks in sysfs to
> > specify heaps, with the latter we need a new idea. We haven't yet gotten
> > around to implement this anywhere, but it's been in the dma-buf/heap todo
> > since forever, and I like it as a design approach. So would be a good idea
> > to not toss it. With that display would have symlinks to cma-ecc and cma,
> > and rendering maybe cma-ecc, shmem, cma heaps (in priority order) for a
> > SoC where the display needs contig memory for scanout.
>
> So indeed that is a good point to keep in mind, but I also think it
> might re-inforce the choice of having ECC as a flag here.
>
> Since my understanding of the sysfs symlinks to heaps idea is about
> being able to figure out a common heap from a collection of devices,
> it's really about the ability for the driver to access the type of
> memory. If ECC is just an attribute of the type of memory (as in this
> patch series), it being on or off won't necessarily affect
> compatibility of the buffer with the device. Similarly "uncached"
> seems more of an attribute of memory type and not a type itself.
> Hardware that can access non-contiguous "system" buffers can access
> uncached system buffers.
Yeah, but in graphics there's a wide band where "shit performance" is
defacto "not useable (as intended at least)".
So if we limit the symlink idea to just making sure zero-copy access is
possible, then we might not actually solve the real world problem we need
to solve. And so the symlinks become somewhat useless, and we need to
somewhere encode which flags you need to use with each symlink.
But I also see the argument that there's a bit a combinatorial explosion
possible. So I guess the question is where we want to handle it ...
Also wondering whether we should get the symlink/allocator idea off the
ground first, but given that that hasn't moved in a decade it might be too
much. But then the question is, what userspace are we going to use for all
these new heaps (or heaps with new flags)?
Cheers, Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Tue, May 21, 2024 at 02:06:19PM GMT, Daniel Vetter wrote:
> On Thu, May 16, 2024 at 09:51:35AM -0700, John Stultz wrote:
> > On Thu, May 16, 2024 at 3:56 AM Daniel Vetter <[email protected]> wrote:
> > > On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote:
> > > > But it makes me a little nervous to add a new generic allocation flag
> > > > for a feature most hardware doesn't support (yet, at least). So it's
> > > > hard to weigh how common the actual usage will be across all the
> > > > heaps.
> > > >
> > > > I apologize as my worry is mostly born out of seeing vendors really
> > > > push opaque feature flags in their old ion heaps, so in providing a
> > > > flags argument, it was mostly intended as an escape hatch for
> > > > obviously common attributes. So having the first be something that
> > > > seems reasonable, but isn't actually that common makes me fret some.
> > > >
> > > > So again, not an objection, just something for folks to stew on to
> > > > make sure this is really the right approach.
> > >
> > > Another good reason to go with full heap names instead of opaque flags on
> > > existing heaps is that with the former we can use symlinks in sysfs to
> > > specify heaps, with the latter we need a new idea. We haven't yet gotten
> > > around to implement this anywhere, but it's been in the dma-buf/heap todo
> > > since forever, and I like it as a design approach. So would be a good idea
> > > to not toss it. With that display would have symlinks to cma-ecc and cma,
> > > and rendering maybe cma-ecc, shmem, cma heaps (in priority order) for a
> > > SoC where the display needs contig memory for scanout.
> >
> > So indeed that is a good point to keep in mind, but I also think it
> > might re-inforce the choice of having ECC as a flag here.
> >
> > Since my understanding of the sysfs symlinks to heaps idea is about
> > being able to figure out a common heap from a collection of devices,
> > it's really about the ability for the driver to access the type of
> > memory. If ECC is just an attribute of the type of memory (as in this
> > patch series), it being on or off won't necessarily affect
> > compatibility of the buffer with the device. Similarly "uncached"
> > seems more of an attribute of memory type and not a type itself.
> > Hardware that can access non-contiguous "system" buffers can access
> > uncached system buffers.
>
> Yeah, but in graphics there's a wide band where "shit performance" is
> defacto "not useable (as intended at least)".
Right, but "not useable" is still kind of usage dependent, which
reinforces the need for flags (and possibly some way to discover what
the heap supports).
Like, if I just want to allocate a buffer for a single writeback frame,
then I probably don't have the same requirements than a compositor that
needs to output a frame at 120Hz.
The former probably doesn't care about the buffer attributes aside that
it's accessible by the device. The latter probably can't make any kind
of compromise over what kind of memory characteristics it uses.
If we look into the current discussions we have, a compositor would
probably need a buffer without ECC, non-secure, and probably wouldn't
care about caching and being physically contiguous.
Libcamera's SoftISP would probably require that the buffer is cacheable,
non-secure, without ECC and might ask for physically contiguous buffers.
As we add more memory types / attributes, I think being able to discover
and enforce a particular set of flags will be more and more important,
even more so if we tie heaps to devices, because it just gives a hint
about the memory being reachable from the device, but as you said, you
can still get a buffer with shit performance that won't be what you
want.
> So if we limit the symlink idea to just making sure zero-copy access is
> possible, then we might not actually solve the real world problem we need
> to solve. And so the symlinks become somewhat useless, and we need to
> somewhere encode which flags you need to use with each symlink.
>
> But I also see the argument that there's a bit a combinatorial explosion
> possible. So I guess the question is where we want to handle it ...
>
> Also wondering whether we should get the symlink/allocator idea off the
> ground first, but given that that hasn't moved in a decade it might be too
> much. But then the question is, what userspace are we going to use for all
> these new heaps (or heaps with new flags)?
For ECC here, the compositors are the obvious target. Which loops backs
into the discussion with John. Do you consider dma-buf code have the
same uapi requirements as DRM?
Maxime
On Wed, May 22, 2024 at 03:18:02PM +0200, Maxime Ripard wrote:
> On Tue, May 21, 2024 at 02:06:19PM GMT, Daniel Vetter wrote:
> > On Thu, May 16, 2024 at 09:51:35AM -0700, John Stultz wrote:
> > > On Thu, May 16, 2024 at 3:56 AM Daniel Vetter <[email protected]> wrote:
> > > > On Wed, May 15, 2024 at 11:42:58AM -0700, John Stultz wrote:
> > > > > But it makes me a little nervous to add a new generic allocation flag
> > > > > for a feature most hardware doesn't support (yet, at least). So it's
> > > > > hard to weigh how common the actual usage will be across all the
> > > > > heaps.
> > > > >
> > > > > I apologize as my worry is mostly born out of seeing vendors really
> > > > > push opaque feature flags in their old ion heaps, so in providing a
> > > > > flags argument, it was mostly intended as an escape hatch for
> > > > > obviously common attributes. So having the first be something that
> > > > > seems reasonable, but isn't actually that common makes me fret some.
> > > > >
> > > > > So again, not an objection, just something for folks to stew on to
> > > > > make sure this is really the right approach.
> > > >
> > > > Another good reason to go with full heap names instead of opaque flags on
> > > > existing heaps is that with the former we can use symlinks in sysfs to
> > > > specify heaps, with the latter we need a new idea. We haven't yet gotten
> > > > around to implement this anywhere, but it's been in the dma-buf/heap todo
> > > > since forever, and I like it as a design approach. So would be a good idea
> > > > to not toss it. With that display would have symlinks to cma-ecc and cma,
> > > > and rendering maybe cma-ecc, shmem, cma heaps (in priority order) for a
> > > > SoC where the display needs contig memory for scanout.
> > >
> > > So indeed that is a good point to keep in mind, but I also think it
> > > might re-inforce the choice of having ECC as a flag here.
> > >
> > > Since my understanding of the sysfs symlinks to heaps idea is about
> > > being able to figure out a common heap from a collection of devices,
> > > it's really about the ability for the driver to access the type of
> > > memory. If ECC is just an attribute of the type of memory (as in this
> > > patch series), it being on or off won't necessarily affect
> > > compatibility of the buffer with the device. Similarly "uncached"
> > > seems more of an attribute of memory type and not a type itself.
> > > Hardware that can access non-contiguous "system" buffers can access
> > > uncached system buffers.
> >
> > Yeah, but in graphics there's a wide band where "shit performance" is
> > defacto "not useable (as intended at least)".
>
> Right, but "not useable" is still kind of usage dependent, which
> reinforces the need for flags (and possibly some way to discover what
> the heap supports).
>
> Like, if I just want to allocate a buffer for a single writeback frame,
> then I probably don't have the same requirements than a compositor that
> needs to output a frame at 120Hz.
>
> The former probably doesn't care about the buffer attributes aside that
> it's accessible by the device. The latter probably can't make any kind
> of compromise over what kind of memory characteristics it uses.
>
> If we look into the current discussions we have, a compositor would
> probably need a buffer without ECC, non-secure, and probably wouldn't
> care about caching and being physically contiguous.
>
> Libcamera's SoftISP would probably require that the buffer is cacheable,
> non-secure, without ECC and might ask for physically contiguous buffers.
>
> As we add more memory types / attributes, I think being able to discover
> and enforce a particular set of flags will be more and more important,
> even more so if we tie heaps to devices, because it just gives a hint
> about the memory being reachable from the device, but as you said, you
> can still get a buffer with shit performance that won't be what you
> want.
>
> > So if we limit the symlink idea to just making sure zero-copy access is
> > possible, then we might not actually solve the real world problem we need
> > to solve. And so the symlinks become somewhat useless, and we need to
> > somewhere encode which flags you need to use with each symlink.
> >
> > But I also see the argument that there's a bit a combinatorial explosion
> > possible. So I guess the question is where we want to handle it ...
> >
> > Also wondering whether we should get the symlink/allocator idea off the
> > ground first, but given that that hasn't moved in a decade it might be too
> > much. But then the question is, what userspace are we going to use for all
> > these new heaps (or heaps with new flags)?
>
> For ECC here, the compositors are the obvious target. Which loops backs
> into the discussion with John. Do you consider dma-buf code have the
> same uapi requirements as DRM?
Imo yes, otherwise we'll get really funny stuff like people bypass drm's
userspace requirement for e.g. content protected buffers by just shipping
the feature in a dma-buf heap ... Been there, done that.
Also I think especially with interop across components there's a huge
difference between a quick test program toy and the real thing. And
dma-buf heaps are kinda all about cross component interop.
-Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch