[RFC simple allocator v2 0/2] Simple allocator

[RFC simple allocator v2 0/2] Simple allocator

[Benjamin Gaignard]

version 2:
- rebase code on 4.10-rc7
- fix bug in CMA allocator
- do more tests with wayland dmabuf protocol:
  https://git.linaro.org/people/benjamin.gaignard/simple_allocator.git

The goal of this RFC is to understand if a common ioctl for specific memory
regions allocations is needed/welcome.

Obviously it will not replace allocation done in linux kernel frameworks like
v4l2, drm/kms or others, but offer an alternative when you don't want/need to
use them for buffer allocation.
To keep a compatibility with what already exist allocated buffers are exported
in userland as dmabuf file descriptor (like ION is doing).

"Unix Device Memory Allocator" project [1] wants to create a userland library
which may allow to select, depending of the devices constraint, the best
back-end for allocation. With this RFC I would to propose to have common ioctl
for a maximum of allocators to avoid to duplicated back-ends for this library.

One of the issues that lead me to propose this RFC it is that since the beginning
it is a problem to allocate contiguous memory (CMA) without using v4l2 or
drm/kms so the first allocator available in this RFC use CMA memory.

An other question is: do we have others memory regions that could be interested
by this new framework ? I have in mind that some title memory regions could use
it or replace ION heaps (system, carveout, etc...).
Maybe it only solve CMA allocation issue, in this case there is no need to create
a new framework but only a dedicated ioctl.

Maybe the first thing to do is to change the name and the location of this 
module, suggestions are welcome.

I have testing this code with the following program:

#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
#include <sys/stat.h>
#include <sys/types.h>

#include "simple-allocator.h"

#define LENGTH 1024*16

void main (void)
{
	struct simple_allocate_data data;
	int fd = open("/dev/cma0", O_RDWR, 0);
	int ret;
	void *mem;

	if (fd < 0) {
		printf("Can't open /dev/cma0\n");
		return;
	}

	memset(&data, 0, sizeof(data));

	data.length = LENGTH;
	data.flags = O_RDWR | O_CLOEXEC;

	ret = ioctl(fd, SA_IOC_ALLOC, &data);
	if (ret) {
		printf("Buffer allocation failed\n");
		goto end;
	}

	mem = mmap(0, LENGTH, PROT_READ | PROT_WRITE, MAP_SHARED, data.fd, 0);
	if (mem == MAP_FAILED) {
		printf("mmap failed\n");
	}

	memset(mem, 0xFF, LENGTH);
	munmap(mem, LENGTH);

	printf("test simple allocator CMA OK\n");
end:
	close(fd);
}

[1] https://github.com/cubanismo/allocator


Benjamin Gaignard (2):
  Create Simple Allocator module
  add CMA simple allocator module

 Documentation/simple-allocator.txt              |  81 ++++++++++
 drivers/Kconfig                                 |   2 +
 drivers/Makefile                                |   1 +
 drivers/simpleallocator/Kconfig                 |  17 +++
 drivers/simpleallocator/Makefile                |   2 +
 drivers/simpleallocator/simple-allocator-cma.c  | 187 ++++++++++++++++++++++++
 drivers/simpleallocator/simple-allocator-priv.h |  33 +++++
 drivers/simpleallocator/simple-allocator.c      | 180 +++++++++++++++++++++++
 include/uapi/linux/simple-allocator.h           |  35 +++++
 9 files changed, 538 insertions(+)
 create mode 100644 Documentation/simple-allocator.txt
 create mode 100644 drivers/simpleallocator/Kconfig
 create mode 100644 drivers/simpleallocator/Makefile
 create mode 100644 drivers/simpleallocator/simple-allocator-cma.c
 create mode 100644 drivers/simpleallocator/simple-allocator-priv.h
 create mode 100644 drivers/simpleallocator/simple-allocator.c
 create mode 100644 include/uapi/linux/simple-allocator.h

-- 
1.9.1

[RFC simple allocator v2 1/2] Create Simple Allocator module

[Benjamin Gaignard]

This is the core of simple allocator module.
It aim to offert one common ioctl to allocate specific memory.

version 2:
- rebased on 4.10-rc7

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 Documentation/simple-allocator.txt              |  81 +++++++++++
 drivers/Kconfig                                 |   2 +
 drivers/Makefile                                |   1 +
 drivers/simpleallocator/Kconfig                 |  10 ++
 drivers/simpleallocator/Makefile                |   1 +
 drivers/simpleallocator/simple-allocator-priv.h |  33 +++++
 drivers/simpleallocator/simple-allocator.c      | 180 ++++++++++++++++++++++++
 include/uapi/linux/simple-allocator.h           |  35 +++++
 8 files changed, 343 insertions(+)
 create mode 100644 Documentation/simple-allocator.txt
 create mode 100644 drivers/simpleallocator/Kconfig
 create mode 100644 drivers/simpleallocator/Makefile
 create mode 100644 drivers/simpleallocator/simple-allocator-priv.h
 create mode 100644 drivers/simpleallocator/simple-allocator.c
 create mode 100644 include/uapi/linux/simple-allocator.h

diff --git a/Documentation/simple-allocator.txt b/Documentation/simple-allocator.txt
new file mode 100644
index 0000000..89ba883
--- /dev/null
+++ b/Documentation/simple-allocator.txt
@@ -0,0 +1,81 @@
+Simple Allocator Framework
+
+Simple Allocator offer a single ioctl SA_IOC_ALLOC to allocate buffers
+on dedicated memory regions and export them as a dmabuf file descriptor.
+Using dmabuf file descriptor allow to share this memory between processes
+and/or import it into other frameworks like v4l2 or drm/kms (prime).
+When userland wants to free the memory only a call to close() in needed
+so it could done even without knowing that buffer has been allocated by
+simple allocator ioctl.
+
+Each memory regions will be seen as a filein /dev/.
+For example CMA regions will exposed has /dev/cmaX.
+
+Implementing a simple allocator
+-------------------------------
+
+Simple Allocator provide helpers functions to register/unregister an
+allocator:
+- simple_allocator_register(struct sa_device *sadev)
+  Register simple_allocator_device using sa_device structure where name,
+  owner and allocate fields must be set.
+
+- simple_allocator_unregister(struct sa_device *sadev)
+  Unregister a simple allocator device.
+
+Using Simple Allocator /dev interface example
+---------------------------------------------
+
+This example of code allocate a buffer on the first CMA region (/dev/cma0)
+before mmap and close it.
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/ioctl.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+#include "simple-allocator.h"
+
+#define LENGTH 1024*16
+
+void main (void)
+{
+	struct simple_allocate_data data;
+	int fd = open("/dev/cma0", O_RDWR, 0);
+	int ret;
+	void *mem;
+
+	if (fd < 0) {
+		printf("Can't open /dev/cma0\n");
+		return;
+	}
+
+	memset(&data, 0, sizeof(data));
+
+	data.length = LENGTH;
+	data.flags = O_RDWR | O_CLOEXEC;
+
+	ret = ioctl(fd, SA_IOC_ALLOC, &data);
+	if (ret) {
+		printf("Buffer allocation failed\n");
+		goto end;
+	}
+
+	mem = mmap(0, LENGTH, PROT_READ | PROT_WRITE, MAP_SHARED, data.fd, 0);
+	if (mem == MAP_FAILED) {
+		printf("mmap failed\n");
+	}
+
+	memset(mem, 0xFF, LENGTH);
+	munmap(mem, LENGTH);
+
+	printf("test simple allocator CMA OK\n");
+end:
+	close(fd);
+}
diff --git a/drivers/Kconfig b/drivers/Kconfig
index e1e2066..a6d8828 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -202,4 +202,6 @@ source "drivers/hwtracing/intel_th/Kconfig"
 
 source "drivers/fpga/Kconfig"
 
+source "drivers/simpleallocator/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index 060026a..5081eb8 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -173,3 +173,4 @@ obj-$(CONFIG_STM)		+= hwtracing/stm/
 obj-$(CONFIG_ANDROID)		+= android/
 obj-$(CONFIG_NVMEM)		+= nvmem/
 obj-$(CONFIG_FPGA)		+= fpga/
+obj-$(CONFIG_SIMPLE_ALLOCATOR) 	+= simpleallocator/
diff --git a/drivers/simpleallocator/Kconfig b/drivers/simpleallocator/Kconfig
new file mode 100644
index 0000000..c6fc2e3
--- /dev/null
+++ b/drivers/simpleallocator/Kconfig
@@ -0,0 +1,10 @@
+menu "Simple Allocator"
+
+config SIMPLE_ALLOCATOR
+	tristate "Simple Alllocator Framework"
+	select DMA_SHARED_BUFFER
+	---help---
+	   The Simple Allocator Framework adds an API to allocate and share
+	   memory in userland.
+
+endmenu
diff --git a/drivers/simpleallocator/Makefile b/drivers/simpleallocator/Makefile
new file mode 100644
index 0000000..e27c6ad
--- /dev/null
+++ b/drivers/simpleallocator/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SIMPLE_ALLOCATOR) += simple-allocator.o
diff --git a/drivers/simpleallocator/simple-allocator-priv.h b/drivers/simpleallocator/simple-allocator-priv.h
new file mode 100644
index 0000000..33f5a33
--- /dev/null
+++ b/drivers/simpleallocator/simple-allocator-priv.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright (C) Linaro 2016
+ *
+ * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
+ *
+ * License terms:  GNU General Public License (GPL)
+ */
+
+#ifndef _SIMPLE_ALLOCATOR_PRIV_H_
+#define _SIMPLE_ALLOCATOR_PRIV_H_
+
+#include <linux/cdev.h>
+#include <linux/device.h>
+#include <linux/dma-buf.h>
+
+/**
+ * struct sa_device - simple allocator device
+ * @owner: module owner, must be set to THIS_MODULE
+ * @name: name of the allocator
+ * @allocate: callabck for memory allocation
+ */
+struct sa_device {
+	struct device	dev;
+	struct cdev	chrdev;
+	struct module	*owner;
+	const char	*name;
+	struct dma_buf *(*allocate)(struct sa_device *, u64 length, u32 flags);
+};
+
+int simple_allocator_register(struct sa_device *sadev);
+void simple_allocator_unregister(struct sa_device *sadev);
+
+#endif
diff --git a/drivers/simpleallocator/simple-allocator.c b/drivers/simpleallocator/simple-allocator.c
new file mode 100644
index 0000000..d89ccbf
--- /dev/null
+++ b/drivers/simpleallocator/simple-allocator.c
@@ -0,0 +1,180 @@
+/*
+ * Copyright (C) Linaro 2016
+ *
+ * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
+ *
+ * License terms:  GNU General Public License (GPL)
+ */
+
+#include <linux/module.h>
+#include <linux/simple-allocator.h>
+#include <linux/uaccess.h>
+
+#include "simple-allocator-priv.h"
+
+#define SA_MAJOR	222
+#define SA_NUM_DEVICES	256
+#define SA_NAME		"simple_allocator"
+
+static int sa_minor;
+
+static struct class sa_class = {
+	.name = SA_NAME,
+};
+
+static long sa_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	struct sa_device *sadev = filp->private_data;
+	int ret = -ENODEV;
+
+	switch (cmd) {
+	case SA_IOC_ALLOC:
+	{
+		struct simple_allocate_data data;
+		struct dma_buf *dmabuf;
+
+		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
+			return -EFAULT;
+
+		if (data.version != 0)
+			return -EINVAL;
+
+		dmabuf = sadev->allocate(sadev, data.length, data.flags);
+		if (!dmabuf)
+			return -EINVAL;
+
+		data.fd = dma_buf_fd(dmabuf, data.flags);
+		if (data.fd < 0) {
+			dma_buf_put(dmabuf);
+			return -EINVAL;
+		}
+
+		data.length = dmabuf->size;
+
+		if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
+			dma_buf_put(dmabuf);
+			return -EFAULT;
+		}
+
+		return 0;
+	}
+	}
+	return ret;
+}
+
+static int sa_open(struct inode *inode, struct file *filp)
+{
+	struct sa_device *sadev = container_of(inode->i_cdev,
+					       struct sa_device, chrdev);
+
+	if (!sadev)
+		return -ENODEV;
+
+	get_device(&sadev->dev);
+	filp->private_data = sadev;
+	return 0;
+}
+
+static int sa_release(struct inode *inode, struct file *filp)
+{
+	struct sa_device *sadev = container_of(inode->i_cdev,
+					       struct sa_device, chrdev);
+
+	if (!sadev)
+		return -ENODEV;
+
+	put_device(&sadev->dev);
+	return 0;
+}
+
+static const struct file_operations sa_fops = {
+	.owner = THIS_MODULE,
+	.open = sa_open,
+	.release = sa_release,
+	.unlocked_ioctl = sa_ioctl,
+};
+
+/**
+ * simple_allocator_register - register a simple allocator
+ * @sadev: simple allocator structure to be registered
+ *
+ * Return 0 if allocator has been regsitered, either a negative value.
+ */
+int simple_allocator_register(struct sa_device *sadev)
+{
+	int ret;
+
+	if (!sadev->name || !sadev->allocate)
+		return -EINVAL;
+
+	cdev_init(&sadev->chrdev, &sa_fops);
+	sadev->chrdev.owner = sadev->owner;
+
+	ret = cdev_add(&sadev->chrdev, MKDEV(SA_MAJOR, sa_minor), 1);
+	if (ret < 0)
+		return ret;
+
+	sadev->dev.class = &sa_class;
+	sadev->dev.devt = MKDEV(SA_MAJOR, sa_minor);
+	dev_set_name(&sadev->dev, "%s%d", sadev->name, sa_minor);
+	ret = device_register(&sadev->dev);
+	if (ret < 0)
+		goto cleanup;
+
+	sa_minor++;
+	return 0;
+
+cleanup:
+	cdev_del(&sadev->chrdev);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(simple_allocator_register);
+
+/**
+ * simple_allocator_unregister - unregister a simple allocator
+ * @sadev: simple allocator device to be unregistered
+ */
+void simple_allocator_unregister(struct sa_device *sadev)
+{
+	if (!sadev)
+		return;
+
+	cdev_del(&sadev->chrdev);
+	device_del(&sadev->dev);
+	put_device(&sadev->dev);
+}
+EXPORT_SYMBOL_GPL(simple_allocator_unregister);
+
+static int __init sa_init(void)
+{
+	dev_t dev = MKDEV(SA_MAJOR, 0);
+	int ret;
+
+	ret = register_chrdev_region(dev, SA_NUM_DEVICES, SA_NAME);
+	if (ret < 0)
+		return ret;
+
+	ret = class_register(&sa_class);
+	if (ret < 0) {
+		unregister_chrdev_region(dev, SA_NUM_DEVICES);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void __exit sa_exit(void)
+{
+	dev_t dev = MKDEV(SA_MAJOR, 0);
+
+	class_unregister(&sa_class);
+	unregister_chrdev_region(dev, SA_NUM_DEVICES);
+}
+
+subsys_initcall(sa_init);
+module_exit(sa_exit);
+
+MODULE_AUTHOR("Benjamin Gaignard <benjamin.gaignard@linaro.org>");
+MODULE_DESCRIPTION("Simple allocator");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS_CHARDEV_MAJOR(SA_MAJOR);
diff --git a/include/uapi/linux/simple-allocator.h b/include/uapi/linux/simple-allocator.h
new file mode 100644
index 0000000..5520a85
--- /dev/null
+++ b/include/uapi/linux/simple-allocator.h
@@ -0,0 +1,35 @@
+/*
+ * Copyright (C) Linaro 2016
+ *
+ * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
+ *
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+
+#ifndef _SIMPLE_ALLOCATOR_H_
+#define _SIMPLE_ALLOCATOR_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+/**
+ * struct simple_allocate_data - allocation parameters
+ * @version:	structure version (must be set to 0)
+ * @length:	size of the requested buffer
+ * @flags:	mode flags for the file like O_RDWR or O_CLOEXEC
+ * @fd:		returned file descriptor
+ */
+struct simple_allocate_data {
+	__u64 version;
+	__u64 length;
+	__u32 flags;
+	__u32 reserved1;
+	__s32 fd;
+	__u32 reserved2;
+};
+
+#define SA_IOC_MAGIC 'S'
+
+#define SA_IOC_ALLOC _IOWR(SA_IOC_MAGIC, 0, struct simple_allocate_data)
+
+#endif
-- 
1.9.1

Re: [RFC simple allocator v2 1/2] Create Simple Allocator module

[Laurent Pinchart]

Hi Benjamin,

Thank you for the patch. I've CC'ed the linux-api mailing list.

On Monday 13 Feb 2017 15:45:05 Benjamin Gaignard wrote:
> This is the core of simple allocator module.
> It aim to offert one common ioctl to allocate specific memory.
> 
> version 2:
> - rebased on 4.10-rc7
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> ---
>  Documentation/simple-allocator.txt              |  81 +++++++++++
>  drivers/Kconfig                                 |   2 +
>  drivers/Makefile                                |   1 +
>  drivers/simpleallocator/Kconfig                 |  10 ++
>  drivers/simpleallocator/Makefile                |   1 +
>  drivers/simpleallocator/simple-allocator-priv.h |  33 +++++
>  drivers/simpleallocator/simple-allocator.c      | 180 +++++++++++++++++++++
>  include/uapi/linux/simple-allocator.h           |  35 +++++
>  8 files changed, 343 insertions(+)
>  create mode 100644 Documentation/simple-allocator.txt
>  create mode 100644 drivers/simpleallocator/Kconfig
>  create mode 100644 drivers/simpleallocator/Makefile
>  create mode 100644 drivers/simpleallocator/simple-allocator-priv.h
>  create mode 100644 drivers/simpleallocator/simple-allocator.c
>  create mode 100644 include/uapi/linux/simple-allocator.h
> 
> diff --git a/Documentation/simple-allocator.txt
> b/Documentation/simple-allocator.txt new file mode 100644
> index 0000000..89ba883
> --- /dev/null
> +++ b/Documentation/simple-allocator.txt
> @@ -0,0 +1,81 @@
> +Simple Allocator Framework

There's nothing simple in buffer allocation, otherwise we would have solved 
the problem a long time ago. Let's not use a misleading name.

> +Simple Allocator offer a single ioctl SA_IOC_ALLOC to allocate buffers
> +on dedicated memory regions and export them as a dmabuf file descriptor.
> +Using dmabuf file descriptor allow to share this memory between processes
> +and/or import it into other frameworks like v4l2 or drm/kms (prime).
> +When userland wants to free the memory only a call to close() in needed
> +so it could done even without knowing that buffer has been allocated by
> +simple allocator ioctl.
> +
> +Each memory regions will be seen as a filein /dev/.
> +For example CMA regions will exposed has /dev/cmaX.

Why do you need multiple devices ? Furthermore, given how central this API 
will become, I believe it needs very strict review, and would be a candidate 
for a syscall.

Without diving into details yet, there are a few particular points I'm 
concerned about.

- What is the scope of this API ? What problems do you want to solve, plan to 
solve in the future, and consider as out of scope ?

- How should we handle permissions and resource limits ? Is file-based 
permission on a device node a good model ? How do we prevent or at least 
mitigate memory-related DoS ?

- How should such a central allocator API interact with containers and 
virtualization in general ?

- What model do we want to expose to applications to select a memory type ? 
You still haven't convinced me that we should expose memory pools explicitly 
to application (à la ION), and I believe a usage/constraint model would be 
better.

> +Implementing a simple allocator
> +-------------------------------
> +
> +Simple Allocator provide helpers functions to register/unregister an
> +allocator:
> +- simple_allocator_register(struct sa_device *sadev)
> +  Register simple_allocator_device using sa_device structure where name,
> +  owner and allocate fields must be set.
> +
> +- simple_allocator_unregister(struct sa_device *sadev)
> +  Unregister a simple allocator device.
> +
> +Using Simple Allocator /dev interface example
> +---------------------------------------------
> +
> +This example of code allocate a buffer on the first CMA region (/dev/cma0)
> +before mmap and close it.
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/ioctl.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +
> +#include "simple-allocator.h"
> +
> +#define LENGTH 1024*16
> +
> +void main (void)
> +{
> +	struct simple_allocate_data data;
> +	int fd = open("/dev/cma0", O_RDWR, 0);
> +	int ret;
> +	void *mem;
> +
> +	if (fd < 0) {
> +		printf("Can't open /dev/cma0\n");
> +		return;
> +	}
> +
> +	memset(&data, 0, sizeof(data));
> +
> +	data.length = LENGTH;
> +	data.flags = O_RDWR | O_CLOEXEC;
> +
> +	ret = ioctl(fd, SA_IOC_ALLOC, &data);
> +	if (ret) {
> +		printf("Buffer allocation failed\n");
> +		goto end;
> +	}
> +
> +	mem = mmap(0, LENGTH, PROT_READ | PROT_WRITE, MAP_SHARED, data.fd, 0);
> +	if (mem == MAP_FAILED) {
> +		printf("mmap failed\n");
> +	}
> +
> +	memset(mem, 0xFF, LENGTH);
> +	munmap(mem, LENGTH);
> +
> +	printf("test simple allocator CMA OK\n");
> +end:
> +	close(fd);
> +}
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index e1e2066..a6d8828 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -202,4 +202,6 @@ source "drivers/hwtracing/intel_th/Kconfig"
> 
>  source "drivers/fpga/Kconfig"
> 
> +source "drivers/simpleallocator/Kconfig"
> +
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 060026a..5081eb8 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -173,3 +173,4 @@ obj-$(CONFIG_STM)		+= hwtracing/stm/
>  obj-$(CONFIG_ANDROID)		+= android/
>  obj-$(CONFIG_NVMEM)		+= nvmem/
>  obj-$(CONFIG_FPGA)		+= fpga/
> +obj-$(CONFIG_SIMPLE_ALLOCATOR) 	+= simpleallocator/
> diff --git a/drivers/simpleallocator/Kconfig
> b/drivers/simpleallocator/Kconfig new file mode 100644
> index 0000000..c6fc2e3
> --- /dev/null
> +++ b/drivers/simpleallocator/Kconfig
> @@ -0,0 +1,10 @@
> +menu "Simple Allocator"
> +
> +config SIMPLE_ALLOCATOR
> +	tristate "Simple Alllocator Framework"
> +	select DMA_SHARED_BUFFER
> +	---help---
> +	   The Simple Allocator Framework adds an API to allocate and share
> +	   memory in userland.
> +
> +endmenu
> diff --git a/drivers/simpleallocator/Makefile
> b/drivers/simpleallocator/Makefile new file mode 100644
> index 0000000..e27c6ad
> --- /dev/null
> +++ b/drivers/simpleallocator/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_SIMPLE_ALLOCATOR) += simple-allocator.o
> diff --git a/drivers/simpleallocator/simple-allocator-priv.h
> b/drivers/simpleallocator/simple-allocator-priv.h new file mode 100644
> index 0000000..33f5a33
> --- /dev/null
> +++ b/drivers/simpleallocator/simple-allocator-priv.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) Linaro 2016
> + *
> + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> + *
> + * License terms:  GNU General Public License (GPL)
> + */
> +
> +#ifndef _SIMPLE_ALLOCATOR_PRIV_H_
> +#define _SIMPLE_ALLOCATOR_PRIV_H_
> +
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/dma-buf.h>
> +
> +/**
> + * struct sa_device - simple allocator device
> + * @owner: module owner, must be set to THIS_MODULE
> + * @name: name of the allocator
> + * @allocate: callabck for memory allocation
> + */
> +struct sa_device {
> +	struct device	dev;
> +	struct cdev	chrdev;
> +	struct module	*owner;
> +	const char	*name;
> +	struct dma_buf *(*allocate)(struct sa_device *, u64 length, u32 
flags);
> +};
> +
> +int simple_allocator_register(struct sa_device *sadev);
> +void simple_allocator_unregister(struct sa_device *sadev);
> +
> +#endif
> diff --git a/drivers/simpleallocator/simple-allocator.c
> b/drivers/simpleallocator/simple-allocator.c new file mode 100644
> index 0000000..d89ccbf
> --- /dev/null
> +++ b/drivers/simpleallocator/simple-allocator.c
> @@ -0,0 +1,180 @@
> +/*
> + * Copyright (C) Linaro 2016
> + *
> + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> + *
> + * License terms:  GNU General Public License (GPL)
> + */
> +
> +#include <linux/module.h>
> +#include <linux/simple-allocator.h>
> +#include <linux/uaccess.h>
> +
> +#include "simple-allocator-priv.h"
> +
> +#define SA_MAJOR	222
> +#define SA_NUM_DEVICES	256
> +#define SA_NAME		"simple_allocator"
> +
> +static int sa_minor;
> +
> +static struct class sa_class = {
> +	.name = SA_NAME,
> +};
> +
> +static long sa_ioctl(struct file *filp, unsigned int cmd, unsigned long
> arg) +{
> +	struct sa_device *sadev = filp->private_data;
> +	int ret = -ENODEV;
> +
> +	switch (cmd) {
> +	case SA_IOC_ALLOC:
> +	{
> +		struct simple_allocate_data data;
> +		struct dma_buf *dmabuf;
> +
> +		if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> +			return -EFAULT;
> +
> +		if (data.version != 0)
> +			return -EINVAL;
> +
> +		dmabuf = sadev->allocate(sadev, data.length, data.flags);
> +		if (!dmabuf)
> +			return -EINVAL;
> +
> +		data.fd = dma_buf_fd(dmabuf, data.flags);
> +		if (data.fd < 0) {
> +			dma_buf_put(dmabuf);
> +			return -EINVAL;
> +		}
> +
> +		data.length = dmabuf->size;
> +
> +		if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
> +			dma_buf_put(dmabuf);
> +			return -EFAULT;
> +		}
> +
> +		return 0;
> +	}
> +	}
> +	return ret;
> +}
> +
> +static int sa_open(struct inode *inode, struct file *filp)
> +{
> +	struct sa_device *sadev = container_of(inode->i_cdev,
> +					       struct sa_device, chrdev);
> +
> +	if (!sadev)
> +		return -ENODEV;
> +
> +	get_device(&sadev->dev);
> +	filp->private_data = sadev;
> +	return 0;
> +}
> +
> +static int sa_release(struct inode *inode, struct file *filp)
> +{
> +	struct sa_device *sadev = container_of(inode->i_cdev,
> +					       struct sa_device, chrdev);
> +
> +	if (!sadev)
> +		return -ENODEV;
> +
> +	put_device(&sadev->dev);
> +	return 0;
> +}
> +
> +static const struct file_operations sa_fops = {
> +	.owner = THIS_MODULE,
> +	.open = sa_open,
> +	.release = sa_release,
> +	.unlocked_ioctl = sa_ioctl,
> +};
> +
> +/**
> + * simple_allocator_register - register a simple allocator
> + * @sadev: simple allocator structure to be registered
> + *
> + * Return 0 if allocator has been regsitered, either a negative value.
> + */
> +int simple_allocator_register(struct sa_device *sadev)
> +{
> +	int ret;
> +
> +	if (!sadev->name || !sadev->allocate)
> +		return -EINVAL;
> +
> +	cdev_init(&sadev->chrdev, &sa_fops);
> +	sadev->chrdev.owner = sadev->owner;
> +
> +	ret = cdev_add(&sadev->chrdev, MKDEV(SA_MAJOR, sa_minor), 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	sadev->dev.class = &sa_class;
> +	sadev->dev.devt = MKDEV(SA_MAJOR, sa_minor);
> +	dev_set_name(&sadev->dev, "%s%d", sadev->name, sa_minor);
> +	ret = device_register(&sadev->dev);
> +	if (ret < 0)
> +		goto cleanup;
> +
> +	sa_minor++;
> +	return 0;
> +
> +cleanup:
> +	cdev_del(&sadev->chrdev);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(simple_allocator_register);
> +
> +/**
> + * simple_allocator_unregister - unregister a simple allocator
> + * @sadev: simple allocator device to be unregistered
> + */
> +void simple_allocator_unregister(struct sa_device *sadev)
> +{
> +	if (!sadev)
> +		return;
> +
> +	cdev_del(&sadev->chrdev);
> +	device_del(&sadev->dev);
> +	put_device(&sadev->dev);
> +}
> +EXPORT_SYMBOL_GPL(simple_allocator_unregister);
> +
> +static int __init sa_init(void)
> +{
> +	dev_t dev = MKDEV(SA_MAJOR, 0);
> +	int ret;
> +
> +	ret = register_chrdev_region(dev, SA_NUM_DEVICES, SA_NAME);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = class_register(&sa_class);
> +	if (ret < 0) {
> +		unregister_chrdev_region(dev, SA_NUM_DEVICES);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit sa_exit(void)
> +{
> +	dev_t dev = MKDEV(SA_MAJOR, 0);
> +
> +	class_unregister(&sa_class);
> +	unregister_chrdev_region(dev, SA_NUM_DEVICES);
> +}
> +
> +subsys_initcall(sa_init);
> +module_exit(sa_exit);
> +
> +MODULE_AUTHOR("Benjamin Gaignard <benjamin.gaignard@linaro.org>");
> +MODULE_DESCRIPTION("Simple allocator");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_CHARDEV_MAJOR(SA_MAJOR);
> diff --git a/include/uapi/linux/simple-allocator.h
> b/include/uapi/linux/simple-allocator.h new file mode 100644
> index 0000000..5520a85
> --- /dev/null
> +++ b/include/uapi/linux/simple-allocator.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (C) Linaro 2016
> + *
> + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> + *
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#ifndef _SIMPLE_ALLOCATOR_H_
> +#define _SIMPLE_ALLOCATOR_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/**
> + * struct simple_allocate_data - allocation parameters
> + * @version:	structure version (must be set to 0)
> + * @length:	size of the requested buffer
> + * @flags:	mode flags for the file like O_RDWR or O_CLOEXEC
> + * @fd:		returned file descriptor
> + */
> +struct simple_allocate_data {
> +	__u64 version;
> +	__u64 length;
> +	__u32 flags;
> +	__u32 reserved1;
> +	__s32 fd;
> +	__u32 reserved2;
> +};
> +
> +#define SA_IOC_MAGIC 'S'
> +
> +#define SA_IOC_ALLOC _IOWR(SA_IOC_MAGIC, 0, struct simple_allocate_data)
> +
> +#endif

-- 
Regards,

Laurent Pinchart

Re: [RFC simple allocator v2 1/2] Create Simple Allocator module

[Benjamin Gaignard]

2017-02-14 20:30 GMT+01:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Benjamin,
>
> Thank you for the patch. I've CC'ed the linux-api mailing list.
>
> On Monday 13 Feb 2017 15:45:05 Benjamin Gaignard wrote:
>> This is the core of simple allocator module.
>> It aim to offert one common ioctl to allocate specific memory.
>>
>> version 2:
>> - rebased on 4.10-rc7
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> ---
>>  Documentation/simple-allocator.txt              |  81 +++++++++++
>>  drivers/Kconfig                                 |   2 +
>>  drivers/Makefile                                |   1 +
>>  drivers/simpleallocator/Kconfig                 |  10 ++
>>  drivers/simpleallocator/Makefile                |   1 +
>>  drivers/simpleallocator/simple-allocator-priv.h |  33 +++++
>>  drivers/simpleallocator/simple-allocator.c      | 180 +++++++++++++++++++++
>>  include/uapi/linux/simple-allocator.h           |  35 +++++
>>  8 files changed, 343 insertions(+)
>>  create mode 100644 Documentation/simple-allocator.txt
>>  create mode 100644 drivers/simpleallocator/Kconfig
>>  create mode 100644 drivers/simpleallocator/Makefile
>>  create mode 100644 drivers/simpleallocator/simple-allocator-priv.h
>>  create mode 100644 drivers/simpleallocator/simple-allocator.c
>>  create mode 100644 include/uapi/linux/simple-allocator.h
>>
>> diff --git a/Documentation/simple-allocator.txt
>> b/Documentation/simple-allocator.txt new file mode 100644
>> index 0000000..89ba883
>> --- /dev/null
>> +++ b/Documentation/simple-allocator.txt
>> @@ -0,0 +1,81 @@
>> +Simple Allocator Framework
>
> There's nothing simple in buffer allocation, otherwise we would have solved
> the problem a long time ago. Let's not use a misleading name.

Simple was not about the problem, it was about the using only one ioctl.
Anyway a better name is needed: "Limited allocation API", "Basic
allocator framework" ?
suggestions are welcomes

>
>> +Simple Allocator offer a single ioctl SA_IOC_ALLOC to allocate buffers
>> +on dedicated memory regions and export them as a dmabuf file descriptor.
>> +Using dmabuf file descriptor allow to share this memory between processes
>> +and/or import it into other frameworks like v4l2 or drm/kms (prime).
>> +When userland wants to free the memory only a call to close() in needed
>> +so it could done even without knowing that buffer has been allocated by
>> +simple allocator ioctl.
>> +
>> +Each memory regions will be seen as a filein /dev/.
>> +For example CMA regions will exposed has /dev/cmaX.
>
> Why do you need multiple devices ? Furthermore, given how central this API
> will become, I believe it needs very strict review, and would be a candidate
> for a syscall.

I believe that having a device per memory regions is simple than have having,
like ION, heaps which request to add masks (or heap ids) to select them.
My goal here is not to have a a central device /dev/simple_allocator
where allcoations
are dispatched over heaps but only a common ioctl to make userland life easier.
Maybe I should regroup all the device under the same directory: /dev/simple/ ?

>
> Without diving into details yet, there are a few particular points I'm
> concerned about.
>
> - What is the scope of this API ? What problems do you want to solve, plan to
> solve in the future, and consider as out of scope ?

My problem is to be able to allocate a buffer in contiguous memory
(CMA) when using
a software decoder and Weston. This will save the copy between decoder
and display
because display only accepted contiguous memory.
Wayland dmabuf protocol can't allocate buffers so I need an other way
to get access to CMA.
I also other use cases where I need to allocate memory in special
memory region for secure
purpose.

For those both cases I need an API to allocate memory from userland so
I decided to
propose a common ioctl which may be used for even more memory regions.

>
> - How should we handle permissions and resource limits ? Is file-based
> permission on a device node a good model ? How do we prevent or at least
> mitigate memory-related DoS ?

Since each memory region will be represented by a device we could set per
device permissions for example with SELinux policy.

>
> - How should such a central allocator API interact with containers and
> virtualization in general ?

I'm not expert enough in container and virtualization but I know they could have
access to some files/directories to store data so I guess it will
possible for them
to call (for example) /dev/cma0  to allocate memory

> - What model do we want to expose to applications to select a memory type ?
> You still haven't convinced me that we should expose memory pools explicitly
> to application (à la ION), and I believe a usage/constraint model would be
> better.

Memory type selection should be solved by something more smarter than my code.
My goal here is only to provided a common way to perform allocations.
If  "Unix Device Memory Allocator" project manage to define a way to expose
memory constraint I plan to add one ioctl to let it know the
capabilities of each memory region.
I hope that what I'm proposing could become a backend for "Unix Device
Memory Allocator".

>
>> +Implementing a simple allocator
>> +-------------------------------
>> +
>> +Simple Allocator provide helpers functions to register/unregister an
>> +allocator:
>> +- simple_allocator_register(struct sa_device *sadev)
>> +  Register simple_allocator_device using sa_device structure where name,
>> +  owner and allocate fields must be set.
>> +
>> +- simple_allocator_unregister(struct sa_device *sadev)
>> +  Unregister a simple allocator device.
>> +
>> +Using Simple Allocator /dev interface example
>> +---------------------------------------------
>> +
>> +This example of code allocate a buffer on the first CMA region (/dev/cma0)
>> +before mmap and close it.
>> +
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <unistd.h>
>> +#include <sys/ioctl.h>
>> +#include <sys/mman.h>
>> +#include <sys/stat.h>
>> +#include <sys/types.h>
>> +
>> +#include "simple-allocator.h"
>> +
>> +#define LENGTH 1024*16
>> +
>> +void main (void)
>> +{
>> +     struct simple_allocate_data data;
>> +     int fd = open("/dev/cma0", O_RDWR, 0);
>> +     int ret;
>> +     void *mem;
>> +
>> +     if (fd < 0) {
>> +             printf("Can't open /dev/cma0\n");
>> +             return;
>> +     }
>> +
>> +     memset(&data, 0, sizeof(data));
>> +
>> +     data.length = LENGTH;
>> +     data.flags = O_RDWR | O_CLOEXEC;
>> +
>> +     ret = ioctl(fd, SA_IOC_ALLOC, &data);
>> +     if (ret) {
>> +             printf("Buffer allocation failed\n");
>> +             goto end;
>> +     }
>> +
>> +     mem = mmap(0, LENGTH, PROT_READ | PROT_WRITE, MAP_SHARED, data.fd, 0);
>> +     if (mem == MAP_FAILED) {
>> +             printf("mmap failed\n");
>> +     }
>> +
>> +     memset(mem, 0xFF, LENGTH);
>> +     munmap(mem, LENGTH);
>> +
>> +     printf("test simple allocator CMA OK\n");
>> +end:
>> +     close(fd);
>> +}
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index e1e2066..a6d8828 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -202,4 +202,6 @@ source "drivers/hwtracing/intel_th/Kconfig"
>>
>>  source "drivers/fpga/Kconfig"
>>
>> +source "drivers/simpleallocator/Kconfig"
>> +
>>  endmenu
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index 060026a..5081eb8 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -173,3 +173,4 @@ obj-$(CONFIG_STM)         += hwtracing/stm/
>>  obj-$(CONFIG_ANDROID)                += android/
>>  obj-$(CONFIG_NVMEM)          += nvmem/
>>  obj-$(CONFIG_FPGA)           += fpga/
>> +obj-$(CONFIG_SIMPLE_ALLOCATOR)       += simpleallocator/
>> diff --git a/drivers/simpleallocator/Kconfig
>> b/drivers/simpleallocator/Kconfig new file mode 100644
>> index 0000000..c6fc2e3
>> --- /dev/null
>> +++ b/drivers/simpleallocator/Kconfig
>> @@ -0,0 +1,10 @@
>> +menu "Simple Allocator"
>> +
>> +config SIMPLE_ALLOCATOR
>> +     tristate "Simple Alllocator Framework"
>> +     select DMA_SHARED_BUFFER
>> +     ---help---
>> +        The Simple Allocator Framework adds an API to allocate and share
>> +        memory in userland.
>> +
>> +endmenu
>> diff --git a/drivers/simpleallocator/Makefile
>> b/drivers/simpleallocator/Makefile new file mode 100644
>> index 0000000..e27c6ad
>> --- /dev/null
>> +++ b/drivers/simpleallocator/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_SIMPLE_ALLOCATOR) += simple-allocator.o
>> diff --git a/drivers/simpleallocator/simple-allocator-priv.h
>> b/drivers/simpleallocator/simple-allocator-priv.h new file mode 100644
>> index 0000000..33f5a33
>> --- /dev/null
>> +++ b/drivers/simpleallocator/simple-allocator-priv.h
>> @@ -0,0 +1,33 @@
>> +/*
>> + * Copyright (C) Linaro 2016
>> + *
>> + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> + *
>> + * License terms:  GNU General Public License (GPL)
>> + */
>> +
>> +#ifndef _SIMPLE_ALLOCATOR_PRIV_H_
>> +#define _SIMPLE_ALLOCATOR_PRIV_H_
>> +
>> +#include <linux/cdev.h>
>> +#include <linux/device.h>
>> +#include <linux/dma-buf.h>
>> +
>> +/**
>> + * struct sa_device - simple allocator device
>> + * @owner: module owner, must be set to THIS_MODULE
>> + * @name: name of the allocator
>> + * @allocate: callabck for memory allocation
>> + */
>> +struct sa_device {
>> +     struct device   dev;
>> +     struct cdev     chrdev;
>> +     struct module   *owner;
>> +     const char      *name;
>> +     struct dma_buf *(*allocate)(struct sa_device *, u64 length, u32
> flags);
>> +};
>> +
>> +int simple_allocator_register(struct sa_device *sadev);
>> +void simple_allocator_unregister(struct sa_device *sadev);
>> +
>> +#endif
>> diff --git a/drivers/simpleallocator/simple-allocator.c
>> b/drivers/simpleallocator/simple-allocator.c new file mode 100644
>> index 0000000..d89ccbf
>> --- /dev/null
>> +++ b/drivers/simpleallocator/simple-allocator.c
>> @@ -0,0 +1,180 @@
>> +/*
>> + * Copyright (C) Linaro 2016
>> + *
>> + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> + *
>> + * License terms:  GNU General Public License (GPL)
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/simple-allocator.h>
>> +#include <linux/uaccess.h>
>> +
>> +#include "simple-allocator-priv.h"
>> +
>> +#define SA_MAJOR     222
>> +#define SA_NUM_DEVICES       256
>> +#define SA_NAME              "simple_allocator"
>> +
>> +static int sa_minor;
>> +
>> +static struct class sa_class = {
>> +     .name = SA_NAME,
>> +};
>> +
>> +static long sa_ioctl(struct file *filp, unsigned int cmd, unsigned long
>> arg) +{
>> +     struct sa_device *sadev = filp->private_data;
>> +     int ret = -ENODEV;
>> +
>> +     switch (cmd) {
>> +     case SA_IOC_ALLOC:
>> +     {
>> +             struct simple_allocate_data data;
>> +             struct dma_buf *dmabuf;
>> +
>> +             if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
>> +                     return -EFAULT;
>> +
>> +             if (data.version != 0)
>> +                     return -EINVAL;
>> +
>> +             dmabuf = sadev->allocate(sadev, data.length, data.flags);
>> +             if (!dmabuf)
>> +                     return -EINVAL;
>> +
>> +             data.fd = dma_buf_fd(dmabuf, data.flags);
>> +             if (data.fd < 0) {
>> +                     dma_buf_put(dmabuf);
>> +                     return -EINVAL;
>> +             }
>> +
>> +             data.length = dmabuf->size;
>> +
>> +             if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
>> +                     dma_buf_put(dmabuf);
>> +                     return -EFAULT;
>> +             }
>> +
>> +             return 0;
>> +     }
>> +     }
>> +     return ret;
>> +}
>> +
>> +static int sa_open(struct inode *inode, struct file *filp)
>> +{
>> +     struct sa_device *sadev = container_of(inode->i_cdev,
>> +                                            struct sa_device, chrdev);
>> +
>> +     if (!sadev)
>> +             return -ENODEV;
>> +
>> +     get_device(&sadev->dev);
>> +     filp->private_data = sadev;
>> +     return 0;
>> +}
>> +
>> +static int sa_release(struct inode *inode, struct file *filp)
>> +{
>> +     struct sa_device *sadev = container_of(inode->i_cdev,
>> +                                            struct sa_device, chrdev);
>> +
>> +     if (!sadev)
>> +             return -ENODEV;
>> +
>> +     put_device(&sadev->dev);
>> +     return 0;
>> +}
>> +
>> +static const struct file_operations sa_fops = {
>> +     .owner = THIS_MODULE,
>> +     .open = sa_open,
>> +     .release = sa_release,
>> +     .unlocked_ioctl = sa_ioctl,
>> +};
>> +
>> +/**
>> + * simple_allocator_register - register a simple allocator
>> + * @sadev: simple allocator structure to be registered
>> + *
>> + * Return 0 if allocator has been regsitered, either a negative value.
>> + */
>> +int simple_allocator_register(struct sa_device *sadev)
>> +{
>> +     int ret;
>> +
>> +     if (!sadev->name || !sadev->allocate)
>> +             return -EINVAL;
>> +
>> +     cdev_init(&sadev->chrdev, &sa_fops);
>> +     sadev->chrdev.owner = sadev->owner;
>> +
>> +     ret = cdev_add(&sadev->chrdev, MKDEV(SA_MAJOR, sa_minor), 1);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     sadev->dev.class = &sa_class;
>> +     sadev->dev.devt = MKDEV(SA_MAJOR, sa_minor);
>> +     dev_set_name(&sadev->dev, "%s%d", sadev->name, sa_minor);
>> +     ret = device_register(&sadev->dev);
>> +     if (ret < 0)
>> +             goto cleanup;
>> +
>> +     sa_minor++;
>> +     return 0;
>> +
>> +cleanup:
>> +     cdev_del(&sadev->chrdev);
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(simple_allocator_register);
>> +
>> +/**
>> + * simple_allocator_unregister - unregister a simple allocator
>> + * @sadev: simple allocator device to be unregistered
>> + */
>> +void simple_allocator_unregister(struct sa_device *sadev)
>> +{
>> +     if (!sadev)
>> +             return;
>> +
>> +     cdev_del(&sadev->chrdev);
>> +     device_del(&sadev->dev);
>> +     put_device(&sadev->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(simple_allocator_unregister);
>> +
>> +static int __init sa_init(void)
>> +{
>> +     dev_t dev = MKDEV(SA_MAJOR, 0);
>> +     int ret;
>> +
>> +     ret = register_chrdev_region(dev, SA_NUM_DEVICES, SA_NAME);
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     ret = class_register(&sa_class);
>> +     if (ret < 0) {
>> +             unregister_chrdev_region(dev, SA_NUM_DEVICES);
>> +             return -EIO;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static void __exit sa_exit(void)
>> +{
>> +     dev_t dev = MKDEV(SA_MAJOR, 0);
>> +
>> +     class_unregister(&sa_class);
>> +     unregister_chrdev_region(dev, SA_NUM_DEVICES);
>> +}
>> +
>> +subsys_initcall(sa_init);
>> +module_exit(sa_exit);
>> +
>> +MODULE_AUTHOR("Benjamin Gaignard <benjamin.gaignard@linaro.org>");
>> +MODULE_DESCRIPTION("Simple allocator");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS_CHARDEV_MAJOR(SA_MAJOR);
>> diff --git a/include/uapi/linux/simple-allocator.h
>> b/include/uapi/linux/simple-allocator.h new file mode 100644
>> index 0000000..5520a85
>> --- /dev/null
>> +++ b/include/uapi/linux/simple-allocator.h
>> @@ -0,0 +1,35 @@
>> +/*
>> + * Copyright (C) Linaro 2016
>> + *
>> + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> + *
>> + * License terms:  GNU General Public License (GPL), version 2
>> + */
>> +
>> +#ifndef _SIMPLE_ALLOCATOR_H_
>> +#define _SIMPLE_ALLOCATOR_H_
>> +
>> +#include <linux/ioctl.h>
>> +#include <linux/types.h>
>> +
>> +/**
>> + * struct simple_allocate_data - allocation parameters
>> + * @version: structure version (must be set to 0)
>> + * @length:  size of the requested buffer
>> + * @flags:   mode flags for the file like O_RDWR or O_CLOEXEC
>> + * @fd:              returned file descriptor
>> + */
>> +struct simple_allocate_data {
>> +     __u64 version;
>> +     __u64 length;
>> +     __u32 flags;
>> +     __u32 reserved1;
>> +     __s32 fd;
>> +     __u32 reserved2;
>> +};
>> +
>> +#define SA_IOC_MAGIC 'S'
>> +
>> +#define SA_IOC_ALLOC _IOWR(SA_IOC_MAGIC, 0, struct simple_allocate_data)
>> +
>> +#endif
>
> --
> Regards,
>
> Laurent Pinchart
>



-- 
Benjamin Gaignard

Graphic Study Group

Linaro.org │ Open source software for ARM SoCs

Follow Linaro: Facebook | Twitter | Blog

Re: [RFC simple allocator v2 1/2] Create Simple Allocator module

[Daniel Vetter]

On Mon, Feb 13, 2017 at 3:45 PM, Benjamin Gaignard
<benjamin.gaignard@linaro.org> wrote:
> This is the core of simple allocator module.
> It aim to offert one common ioctl to allocate specific memory.
>
> version 2:
> - rebased on 4.10-rc7
>
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>

Why not ION? It's a bit a broken record question, but if there is a
clear answer it should be in the patch&docs ...
-Daniel

> ---
>  Documentation/simple-allocator.txt              |  81 +++++++++++
>  drivers/Kconfig                                 |   2 +
>  drivers/Makefile                                |   1 +
>  drivers/simpleallocator/Kconfig                 |  10 ++
>  drivers/simpleallocator/Makefile                |   1 +
>  drivers/simpleallocator/simple-allocator-priv.h |  33 +++++
>  drivers/simpleallocator/simple-allocator.c      | 180 ++++++++++++++++++++++++
>  include/uapi/linux/simple-allocator.h           |  35 +++++
>  8 files changed, 343 insertions(+)
>  create mode 100644 Documentation/simple-allocator.txt
>  create mode 100644 drivers/simpleallocator/Kconfig
>  create mode 100644 drivers/simpleallocator/Makefile
>  create mode 100644 drivers/simpleallocator/simple-allocator-priv.h
>  create mode 100644 drivers/simpleallocator/simple-allocator.c
>  create mode 100644 include/uapi/linux/simple-allocator.h
>
> diff --git a/Documentation/simple-allocator.txt b/Documentation/simple-allocator.txt
> new file mode 100644
> index 0000000..89ba883
> --- /dev/null
> +++ b/Documentation/simple-allocator.txt
> @@ -0,0 +1,81 @@
> +Simple Allocator Framework
> +
> +Simple Allocator offer a single ioctl SA_IOC_ALLOC to allocate buffers
> +on dedicated memory regions and export them as a dmabuf file descriptor.
> +Using dmabuf file descriptor allow to share this memory between processes
> +and/or import it into other frameworks like v4l2 or drm/kms (prime).
> +When userland wants to free the memory only a call to close() in needed
> +so it could done even without knowing that buffer has been allocated by
> +simple allocator ioctl.
> +
> +Each memory regions will be seen as a filein /dev/.
> +For example CMA regions will exposed has /dev/cmaX.
> +
> +Implementing a simple allocator
> +-------------------------------
> +
> +Simple Allocator provide helpers functions to register/unregister an
> +allocator:
> +- simple_allocator_register(struct sa_device *sadev)
> +  Register simple_allocator_device using sa_device structure where name,
> +  owner and allocate fields must be set.
> +
> +- simple_allocator_unregister(struct sa_device *sadev)
> +  Unregister a simple allocator device.
> +
> +Using Simple Allocator /dev interface example
> +---------------------------------------------
> +
> +This example of code allocate a buffer on the first CMA region (/dev/cma0)
> +before mmap and close it.
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <sys/ioctl.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +
> +#include "simple-allocator.h"
> +
> +#define LENGTH 1024*16
> +
> +void main (void)
> +{
> +       struct simple_allocate_data data;
> +       int fd = open("/dev/cma0", O_RDWR, 0);
> +       int ret;
> +       void *mem;
> +
> +       if (fd < 0) {
> +               printf("Can't open /dev/cma0\n");
> +               return;
> +       }
> +
> +       memset(&data, 0, sizeof(data));
> +
> +       data.length = LENGTH;
> +       data.flags = O_RDWR | O_CLOEXEC;
> +
> +       ret = ioctl(fd, SA_IOC_ALLOC, &data);
> +       if (ret) {
> +               printf("Buffer allocation failed\n");
> +               goto end;
> +       }
> +
> +       mem = mmap(0, LENGTH, PROT_READ | PROT_WRITE, MAP_SHARED, data.fd, 0);
> +       if (mem == MAP_FAILED) {
> +               printf("mmap failed\n");
> +       }
> +
> +       memset(mem, 0xFF, LENGTH);
> +       munmap(mem, LENGTH);
> +
> +       printf("test simple allocator CMA OK\n");
> +end:
> +       close(fd);
> +}
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index e1e2066..a6d8828 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -202,4 +202,6 @@ source "drivers/hwtracing/intel_th/Kconfig"
>
>  source "drivers/fpga/Kconfig"
>
> +source "drivers/simpleallocator/Kconfig"
> +
>  endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index 060026a..5081eb8 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -173,3 +173,4 @@ obj-$(CONFIG_STM)           += hwtracing/stm/
>  obj-$(CONFIG_ANDROID)          += android/
>  obj-$(CONFIG_NVMEM)            += nvmem/
>  obj-$(CONFIG_FPGA)             += fpga/
> +obj-$(CONFIG_SIMPLE_ALLOCATOR)         += simpleallocator/
> diff --git a/drivers/simpleallocator/Kconfig b/drivers/simpleallocator/Kconfig
> new file mode 100644
> index 0000000..c6fc2e3
> --- /dev/null
> +++ b/drivers/simpleallocator/Kconfig
> @@ -0,0 +1,10 @@
> +menu "Simple Allocator"
> +
> +config SIMPLE_ALLOCATOR
> +       tristate "Simple Alllocator Framework"
> +       select DMA_SHARED_BUFFER
> +       ---help---
> +          The Simple Allocator Framework adds an API to allocate and share
> +          memory in userland.
> +
> +endmenu
> diff --git a/drivers/simpleallocator/Makefile b/drivers/simpleallocator/Makefile
> new file mode 100644
> index 0000000..e27c6ad
> --- /dev/null
> +++ b/drivers/simpleallocator/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_SIMPLE_ALLOCATOR) += simple-allocator.o
> diff --git a/drivers/simpleallocator/simple-allocator-priv.h b/drivers/simpleallocator/simple-allocator-priv.h
> new file mode 100644
> index 0000000..33f5a33
> --- /dev/null
> +++ b/drivers/simpleallocator/simple-allocator-priv.h
> @@ -0,0 +1,33 @@
> +/*
> + * Copyright (C) Linaro 2016
> + *
> + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> + *
> + * License terms:  GNU General Public License (GPL)
> + */
> +
> +#ifndef _SIMPLE_ALLOCATOR_PRIV_H_
> +#define _SIMPLE_ALLOCATOR_PRIV_H_
> +
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/dma-buf.h>
> +
> +/**
> + * struct sa_device - simple allocator device
> + * @owner: module owner, must be set to THIS_MODULE
> + * @name: name of the allocator
> + * @allocate: callabck for memory allocation
> + */
> +struct sa_device {
> +       struct device   dev;
> +       struct cdev     chrdev;
> +       struct module   *owner;
> +       const char      *name;
> +       struct dma_buf *(*allocate)(struct sa_device *, u64 length, u32 flags);
> +};
> +
> +int simple_allocator_register(struct sa_device *sadev);
> +void simple_allocator_unregister(struct sa_device *sadev);
> +
> +#endif
> diff --git a/drivers/simpleallocator/simple-allocator.c b/drivers/simpleallocator/simple-allocator.c
> new file mode 100644
> index 0000000..d89ccbf
> --- /dev/null
> +++ b/drivers/simpleallocator/simple-allocator.c
> @@ -0,0 +1,180 @@
> +/*
> + * Copyright (C) Linaro 2016
> + *
> + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> + *
> + * License terms:  GNU General Public License (GPL)
> + */
> +
> +#include <linux/module.h>
> +#include <linux/simple-allocator.h>
> +#include <linux/uaccess.h>
> +
> +#include "simple-allocator-priv.h"
> +
> +#define SA_MAJOR       222
> +#define SA_NUM_DEVICES 256
> +#define SA_NAME                "simple_allocator"
> +
> +static int sa_minor;
> +
> +static struct class sa_class = {
> +       .name = SA_NAME,
> +};
> +
> +static long sa_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +       struct sa_device *sadev = filp->private_data;
> +       int ret = -ENODEV;
> +
> +       switch (cmd) {
> +       case SA_IOC_ALLOC:
> +       {
> +               struct simple_allocate_data data;
> +               struct dma_buf *dmabuf;
> +
> +               if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> +                       return -EFAULT;
> +
> +               if (data.version != 0)
> +                       return -EINVAL;
> +
> +               dmabuf = sadev->allocate(sadev, data.length, data.flags);
> +               if (!dmabuf)
> +                       return -EINVAL;
> +
> +               data.fd = dma_buf_fd(dmabuf, data.flags);
> +               if (data.fd < 0) {
> +                       dma_buf_put(dmabuf);
> +                       return -EINVAL;
> +               }
> +
> +               data.length = dmabuf->size;
> +
> +               if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd))) {
> +                       dma_buf_put(dmabuf);
> +                       return -EFAULT;
> +               }
> +
> +               return 0;
> +       }
> +       }
> +       return ret;
> +}
> +
> +static int sa_open(struct inode *inode, struct file *filp)
> +{
> +       struct sa_device *sadev = container_of(inode->i_cdev,
> +                                              struct sa_device, chrdev);
> +
> +       if (!sadev)
> +               return -ENODEV;
> +
> +       get_device(&sadev->dev);
> +       filp->private_data = sadev;
> +       return 0;
> +}
> +
> +static int sa_release(struct inode *inode, struct file *filp)
> +{
> +       struct sa_device *sadev = container_of(inode->i_cdev,
> +                                              struct sa_device, chrdev);
> +
> +       if (!sadev)
> +               return -ENODEV;
> +
> +       put_device(&sadev->dev);
> +       return 0;
> +}
> +
> +static const struct file_operations sa_fops = {
> +       .owner = THIS_MODULE,
> +       .open = sa_open,
> +       .release = sa_release,
> +       .unlocked_ioctl = sa_ioctl,
> +};
> +
> +/**
> + * simple_allocator_register - register a simple allocator
> + * @sadev: simple allocator structure to be registered
> + *
> + * Return 0 if allocator has been regsitered, either a negative value.
> + */
> +int simple_allocator_register(struct sa_device *sadev)
> +{
> +       int ret;
> +
> +       if (!sadev->name || !sadev->allocate)
> +               return -EINVAL;
> +
> +       cdev_init(&sadev->chrdev, &sa_fops);
> +       sadev->chrdev.owner = sadev->owner;
> +
> +       ret = cdev_add(&sadev->chrdev, MKDEV(SA_MAJOR, sa_minor), 1);
> +       if (ret < 0)
> +               return ret;
> +
> +       sadev->dev.class = &sa_class;
> +       sadev->dev.devt = MKDEV(SA_MAJOR, sa_minor);
> +       dev_set_name(&sadev->dev, "%s%d", sadev->name, sa_minor);
> +       ret = device_register(&sadev->dev);
> +       if (ret < 0)
> +               goto cleanup;
> +
> +       sa_minor++;
> +       return 0;
> +
> +cleanup:
> +       cdev_del(&sadev->chrdev);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(simple_allocator_register);
> +
> +/**
> + * simple_allocator_unregister - unregister a simple allocator
> + * @sadev: simple allocator device to be unregistered
> + */
> +void simple_allocator_unregister(struct sa_device *sadev)
> +{
> +       if (!sadev)
> +               return;
> +
> +       cdev_del(&sadev->chrdev);
> +       device_del(&sadev->dev);
> +       put_device(&sadev->dev);
> +}
> +EXPORT_SYMBOL_GPL(simple_allocator_unregister);
> +
> +static int __init sa_init(void)
> +{
> +       dev_t dev = MKDEV(SA_MAJOR, 0);
> +       int ret;
> +
> +       ret = register_chrdev_region(dev, SA_NUM_DEVICES, SA_NAME);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = class_register(&sa_class);
> +       if (ret < 0) {
> +               unregister_chrdev_region(dev, SA_NUM_DEVICES);
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
> +static void __exit sa_exit(void)
> +{
> +       dev_t dev = MKDEV(SA_MAJOR, 0);
> +
> +       class_unregister(&sa_class);
> +       unregister_chrdev_region(dev, SA_NUM_DEVICES);
> +}
> +
> +subsys_initcall(sa_init);
> +module_exit(sa_exit);
> +
> +MODULE_AUTHOR("Benjamin Gaignard <benjamin.gaignard@linaro.org>");
> +MODULE_DESCRIPTION("Simple allocator");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS_CHARDEV_MAJOR(SA_MAJOR);
> diff --git a/include/uapi/linux/simple-allocator.h b/include/uapi/linux/simple-allocator.h
> new file mode 100644
> index 0000000..5520a85
> --- /dev/null
> +++ b/include/uapi/linux/simple-allocator.h
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (C) Linaro 2016
> + *
> + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> + *
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +
> +#ifndef _SIMPLE_ALLOCATOR_H_
> +#define _SIMPLE_ALLOCATOR_H_
> +
> +#include <linux/ioctl.h>
> +#include <linux/types.h>
> +
> +/**
> + * struct simple_allocate_data - allocation parameters
> + * @version:   structure version (must be set to 0)
> + * @length:    size of the requested buffer
> + * @flags:     mode flags for the file like O_RDWR or O_CLOEXEC
> + * @fd:                returned file descriptor
> + */
> +struct simple_allocate_data {
> +       __u64 version;
> +       __u64 length;
> +       __u32 flags;
> +       __u32 reserved1;
> +       __s32 fd;
> +       __u32 reserved2;
> +};
> +
> +#define SA_IOC_MAGIC 'S'
> +
> +#define SA_IOC_ALLOC _IOWR(SA_IOC_MAGIC, 0, struct simple_allocate_data)
> +
> +#endif
> --
> 1.9.1
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC simple allocator v2 1/2] Create Simple Allocator module

[Laurent Pinchart]

Hi Daniel,

On Tuesday 14 Feb 2017 20:33:58 Daniel Vetter wrote:
> On Mon, Feb 13, 2017 at 3:45 PM, Benjamin Gaignard wrote:
> > This is the core of simple allocator module.
> > It aim to offert one common ioctl to allocate specific memory.
> > 
> > version 2:
> > - rebased on 4.10-rc7
> > 
> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> 
> Why not ION? It's a bit a broken record question, but if there is a
> clear answer it should be in the patch&docs ...

There's a bit of love & hate relationship between Linux developers and ION. 
The API has shortcomings, and attempts to fix the issues went nowhere. As 
Laura explained, starting from a blank slate (obviously keeping in mind the 
lessons learnt so far with ION and other similar APIs) and then adding a 
wrapper to expose ION on Android systems (at least as an interim measure) was 
thought to be a better option. I still believe it is, but we seem to lack 
traction. The problem has been around for so long that I feel everybody has 
lost hope.

I don't think this is unsolvable, but we need to regain motivation. In my 
opinion the first step would be to define the precise extent of the problem we 
want to solve.

> > ---
> > 
> >  Documentation/simple-allocator.txt              |  81 +++++++++++
> >  drivers/Kconfig                                 |   2 +
> >  drivers/Makefile                                |   1 +
> >  drivers/simpleallocator/Kconfig                 |  10 ++
> >  drivers/simpleallocator/Makefile                |   1 +
> >  drivers/simpleallocator/simple-allocator-priv.h |  33 +++++
> >  drivers/simpleallocator/simple-allocator.c      | 180 +++++++++++++++++++
> >  include/uapi/linux/simple-allocator.h           |  35 +++++
> >  8 files changed, 343 insertions(+)
> >  create mode 100644 Documentation/simple-allocator.txt
> >  create mode 100644 drivers/simpleallocator/Kconfig
> >  create mode 100644 drivers/simpleallocator/Makefile
> >  create mode 100644 drivers/simpleallocator/simple-allocator-priv.h
> >  create mode 100644 drivers/simpleallocator/simple-allocator.c
> >  create mode 100644 include/uapi/linux/simple-allocator.h
> > 
> > diff --git a/Documentation/simple-allocator.txt
> > b/Documentation/simple-allocator.txt new file mode 100644
> > index 0000000..89ba883
> > --- /dev/null
> > +++ b/Documentation/simple-allocator.txt
> > @@ -0,0 +1,81 @@
> > +Simple Allocator Framework
> > +
> > +Simple Allocator offer a single ioctl SA_IOC_ALLOC to allocate buffers
> > +on dedicated memory regions and export them as a dmabuf file descriptor.
> > +Using dmabuf file descriptor allow to share this memory between processes
> > +and/or import it into other frameworks like v4l2 or drm/kms (prime).
> > +When userland wants to free the memory only a call to close() in needed
> > +so it could done even without knowing that buffer has been allocated by
> > +simple allocator ioctl.
> > +
> > +Each memory regions will be seen as a filein /dev/.
> > +For example CMA regions will exposed has /dev/cmaX.
> > +
> > +Implementing a simple allocator
> > +-------------------------------
> > +
> > +Simple Allocator provide helpers functions to register/unregister an
> > +allocator:
> > +- simple_allocator_register(struct sa_device *sadev)
> > +  Register simple_allocator_device using sa_device structure where name,
> > +  owner and allocate fields must be set.
> > +
> > +- simple_allocator_unregister(struct sa_device *sadev)
> > +  Unregister a simple allocator device.
> > +
> > +Using Simple Allocator /dev interface example
> > +---------------------------------------------
> > +
> > +This example of code allocate a buffer on the first CMA region
> > (/dev/cma0)
> > +before mmap and close it.
> > +
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <unistd.h>
> > +#include <sys/ioctl.h>
> > +#include <sys/mman.h>
> > +#include <sys/stat.h>
> > +#include <sys/types.h>
> > +
> > +#include "simple-allocator.h"
> > +
> > +#define LENGTH 1024*16
> > +
> > +void main (void)
> > +{
> > +       struct simple_allocate_data data;
> > +       int fd = open("/dev/cma0", O_RDWR, 0);
> > +       int ret;
> > +       void *mem;
> > +
> > +       if (fd < 0) {
> > +               printf("Can't open /dev/cma0\n");
> > +               return;
> > +       }
> > +
> > +       memset(&data, 0, sizeof(data));
> > +
> > +       data.length = LENGTH;
> > +       data.flags = O_RDWR | O_CLOEXEC;
> > +
> > +       ret = ioctl(fd, SA_IOC_ALLOC, &data);
> > +       if (ret) {
> > +               printf("Buffer allocation failed\n");
> > +               goto end;
> > +       }
> > +
> > +       mem = mmap(0, LENGTH, PROT_READ | PROT_WRITE, MAP_SHARED, data.fd,
> > 0); +       if (mem == MAP_FAILED) {
> > +               printf("mmap failed\n");
> > +       }
> > +
> > +       memset(mem, 0xFF, LENGTH);
> > +       munmap(mem, LENGTH);
> > +
> > +       printf("test simple allocator CMA OK\n");
> > +end:
> > +       close(fd);
> > +}
> > diff --git a/drivers/Kconfig b/drivers/Kconfig
> > index e1e2066..a6d8828 100644
> > --- a/drivers/Kconfig
> > +++ b/drivers/Kconfig
> > @@ -202,4 +202,6 @@ source "drivers/hwtracing/intel_th/Kconfig"
> > 
> >  source "drivers/fpga/Kconfig"
> > 
> > +source "drivers/simpleallocator/Kconfig"
> > +
> > 
> >  endmenu
> > 
> > diff --git a/drivers/Makefile b/drivers/Makefile
> > index 060026a..5081eb8 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -173,3 +173,4 @@ obj-$(CONFIG_STM)           += hwtracing/stm/
> > 
> >  obj-$(CONFIG_ANDROID)          += android/
> >  obj-$(CONFIG_NVMEM)            += nvmem/
> >  obj-$(CONFIG_FPGA)             += fpga/
> > 
> > +obj-$(CONFIG_SIMPLE_ALLOCATOR)         += simpleallocator/
> > diff --git a/drivers/simpleallocator/Kconfig
> > b/drivers/simpleallocator/Kconfig new file mode 100644
> > index 0000000..c6fc2e3
> > --- /dev/null
> > +++ b/drivers/simpleallocator/Kconfig
> > @@ -0,0 +1,10 @@
> > +menu "Simple Allocator"
> > +
> > +config SIMPLE_ALLOCATOR
> > +       tristate "Simple Alllocator Framework"
> > +       select DMA_SHARED_BUFFER
> > +       ---help---
> > +          The Simple Allocator Framework adds an API to allocate and
> > share
> > +          memory in userland.
> > +
> > +endmenu
> > diff --git a/drivers/simpleallocator/Makefile
> > b/drivers/simpleallocator/Makefile new file mode 100644
> > index 0000000..e27c6ad
> > --- /dev/null
> > +++ b/drivers/simpleallocator/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_SIMPLE_ALLOCATOR) += simple-allocator.o
> > diff --git a/drivers/simpleallocator/simple-allocator-priv.h
> > b/drivers/simpleallocator/simple-allocator-priv.h new file mode 100644
> > index 0000000..33f5a33
> > --- /dev/null
> > +++ b/drivers/simpleallocator/simple-allocator-priv.h
> > @@ -0,0 +1,33 @@
> > +/*
> > + * Copyright (C) Linaro 2016
> > + *
> > + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> > + *
> > + * License terms:  GNU General Public License (GPL)
> > + */
> > +
> > +#ifndef _SIMPLE_ALLOCATOR_PRIV_H_
> > +#define _SIMPLE_ALLOCATOR_PRIV_H_
> > +
> > +#include <linux/cdev.h>
> > +#include <linux/device.h>
> > +#include <linux/dma-buf.h>
> > +
> > +/**
> > + * struct sa_device - simple allocator device
> > + * @owner: module owner, must be set to THIS_MODULE
> > + * @name: name of the allocator
> > + * @allocate: callabck for memory allocation
> > + */
> > +struct sa_device {
> > +       struct device   dev;
> > +       struct cdev     chrdev;
> > +       struct module   *owner;
> > +       const char      *name;
> > +       struct dma_buf *(*allocate)(struct sa_device *, u64 length, u32
> > flags); +};
> > +
> > +int simple_allocator_register(struct sa_device *sadev);
> > +void simple_allocator_unregister(struct sa_device *sadev);
> > +
> > +#endif
> > diff --git a/drivers/simpleallocator/simple-allocator.c
> > b/drivers/simpleallocator/simple-allocator.c new file mode 100644
> > index 0000000..d89ccbf
> > --- /dev/null
> > +++ b/drivers/simpleallocator/simple-allocator.c
> > @@ -0,0 +1,180 @@
> > +/*
> > + * Copyright (C) Linaro 2016
> > + *
> > + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> > + *
> > + * License terms:  GNU General Public License (GPL)
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/simple-allocator.h>
> > +#include <linux/uaccess.h>
> > +
> > +#include "simple-allocator-priv.h"
> > +
> > +#define SA_MAJOR       222
> > +#define SA_NUM_DEVICES 256
> > +#define SA_NAME                "simple_allocator"
> > +
> > +static int sa_minor;
> > +
> > +static struct class sa_class = {
> > +       .name = SA_NAME,
> > +};
> > +
> > +static long sa_ioctl(struct file *filp, unsigned int cmd, unsigned long
> > arg) +{
> > +       struct sa_device *sadev = filp->private_data;
> > +       int ret = -ENODEV;
> > +
> > +       switch (cmd) {
> > +       case SA_IOC_ALLOC:
> > +       {
> > +               struct simple_allocate_data data;
> > +               struct dma_buf *dmabuf;
> > +
> > +               if (copy_from_user(&data, (void __user *)arg,
> > _IOC_SIZE(cmd))) +                       return -EFAULT;
> > +
> > +               if (data.version != 0)
> > +                       return -EINVAL;
> > +
> > +               dmabuf = sadev->allocate(sadev, data.length, data.flags);
> > +               if (!dmabuf)
> > +                       return -EINVAL;
> > +
> > +               data.fd = dma_buf_fd(dmabuf, data.flags);
> > +               if (data.fd < 0) {
> > +                       dma_buf_put(dmabuf);
> > +                       return -EINVAL;
> > +               }
> > +
> > +               data.length = dmabuf->size;
> > +
> > +               if (copy_to_user((void __user *)arg, &data,
> > _IOC_SIZE(cmd))) { +                       dma_buf_put(dmabuf);
> > +                       return -EFAULT;
> > +               }
> > +
> > +               return 0;
> > +       }
> > +       }
> > +       return ret;
> > +}
> > +
> > +static int sa_open(struct inode *inode, struct file *filp)
> > +{
> > +       struct sa_device *sadev = container_of(inode->i_cdev,
> > +                                              struct sa_device, chrdev);
> > +
> > +       if (!sadev)
> > +               return -ENODEV;
> > +
> > +       get_device(&sadev->dev);
> > +       filp->private_data = sadev;
> > +       return 0;
> > +}
> > +
> > +static int sa_release(struct inode *inode, struct file *filp)
> > +{
> > +       struct sa_device *sadev = container_of(inode->i_cdev,
> > +                                              struct sa_device, chrdev);
> > +
> > +       if (!sadev)
> > +               return -ENODEV;
> > +
> > +       put_device(&sadev->dev);
> > +       return 0;
> > +}
> > +
> > +static const struct file_operations sa_fops = {
> > +       .owner = THIS_MODULE,
> > +       .open = sa_open,
> > +       .release = sa_release,
> > +       .unlocked_ioctl = sa_ioctl,
> > +};
> > +
> > +/**
> > + * simple_allocator_register - register a simple allocator
> > + * @sadev: simple allocator structure to be registered
> > + *
> > + * Return 0 if allocator has been regsitered, either a negative value.
> > + */
> > +int simple_allocator_register(struct sa_device *sadev)
> > +{
> > +       int ret;
> > +
> > +       if (!sadev->name || !sadev->allocate)
> > +               return -EINVAL;
> > +
> > +       cdev_init(&sadev->chrdev, &sa_fops);
> > +       sadev->chrdev.owner = sadev->owner;
> > +
> > +       ret = cdev_add(&sadev->chrdev, MKDEV(SA_MAJOR, sa_minor), 1);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       sadev->dev.class = &sa_class;
> > +       sadev->dev.devt = MKDEV(SA_MAJOR, sa_minor);
> > +       dev_set_name(&sadev->dev, "%s%d", sadev->name, sa_minor);
> > +       ret = device_register(&sadev->dev);
> > +       if (ret < 0)
> > +               goto cleanup;
> > +
> > +       sa_minor++;
> > +       return 0;
> > +
> > +cleanup:
> > +       cdev_del(&sadev->chrdev);
> > +       return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(simple_allocator_register);
> > +
> > +/**
> > + * simple_allocator_unregister - unregister a simple allocator
> > + * @sadev: simple allocator device to be unregistered
> > + */
> > +void simple_allocator_unregister(struct sa_device *sadev)
> > +{
> > +       if (!sadev)
> > +               return;
> > +
> > +       cdev_del(&sadev->chrdev);
> > +       device_del(&sadev->dev);
> > +       put_device(&sadev->dev);
> > +}
> > +EXPORT_SYMBOL_GPL(simple_allocator_unregister);
> > +
> > +static int __init sa_init(void)
> > +{
> > +       dev_t dev = MKDEV(SA_MAJOR, 0);
> > +       int ret;
> > +
> > +       ret = register_chrdev_region(dev, SA_NUM_DEVICES, SA_NAME);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       ret = class_register(&sa_class);
> > +       if (ret < 0) {
> > +               unregister_chrdev_region(dev, SA_NUM_DEVICES);
> > +               return -EIO;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static void __exit sa_exit(void)
> > +{
> > +       dev_t dev = MKDEV(SA_MAJOR, 0);
> > +
> > +       class_unregister(&sa_class);
> > +       unregister_chrdev_region(dev, SA_NUM_DEVICES);
> > +}
> > +
> > +subsys_initcall(sa_init);
> > +module_exit(sa_exit);
> > +
> > +MODULE_AUTHOR("Benjamin Gaignard <benjamin.gaignard@linaro.org>");
> > +MODULE_DESCRIPTION("Simple allocator");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS_CHARDEV_MAJOR(SA_MAJOR);
> > diff --git a/include/uapi/linux/simple-allocator.h
> > b/include/uapi/linux/simple-allocator.h new file mode 100644
> > index 0000000..5520a85
> > --- /dev/null
> > +++ b/include/uapi/linux/simple-allocator.h
> > @@ -0,0 +1,35 @@
> > +/*
> > + * Copyright (C) Linaro 2016
> > + *
> > + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> > + *
> > + * License terms:  GNU General Public License (GPL), version 2
> > + */
> > +
> > +#ifndef _SIMPLE_ALLOCATOR_H_
> > +#define _SIMPLE_ALLOCATOR_H_
> > +
> > +#include <linux/ioctl.h>
> > +#include <linux/types.h>
> > +
> > +/**
> > + * struct simple_allocate_data - allocation parameters
> > + * @version:   structure version (must be set to 0)
> > + * @length:    size of the requested buffer
> > + * @flags:     mode flags for the file like O_RDWR or O_CLOEXEC
> > + * @fd:                returned file descriptor
> > + */
> > +struct simple_allocate_data {
> > +       __u64 version;
> > +       __u64 length;
> > +       __u32 flags;
> > +       __u32 reserved1;
> > +       __s32 fd;
> > +       __u32 reserved2;
> > +};
> > +
> > +#define SA_IOC_MAGIC 'S'
> > +
> > +#define SA_IOC_ALLOC _IOWR(SA_IOC_MAGIC, 0, struct simple_allocate_data)
> > +
> > +#endif
> > --
> > 1.9.1

-- 
Regards,

Laurent Pinchart

Re: [RFC simple allocator v2 1/2] Create Simple Allocator module

[Daniel Vetter]

On Tue, Feb 14, 2017 at 8:39 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Daniel,
>
> On Tuesday 14 Feb 2017 20:33:58 Daniel Vetter wrote:
>> On Mon, Feb 13, 2017 at 3:45 PM, Benjamin Gaignard wrote:
>> > This is the core of simple allocator module.
>> > It aim to offert one common ioctl to allocate specific memory.
>> >
>> > version 2:
>> > - rebased on 4.10-rc7
>> >
>> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>>
>> Why not ION? It's a bit a broken record question, but if there is a
>> clear answer it should be in the patch&docs ...
>
> There's a bit of love & hate relationship between Linux developers and ION.
> The API has shortcomings, and attempts to fix the issues went nowhere. As
> Laura explained, starting from a blank slate (obviously keeping in mind the
> lessons learnt so far with ION and other similar APIs) and then adding a
> wrapper to expose ION on Android systems (at least as an interim measure) was
> thought to be a better option. I still believe it is, but we seem to lack
> traction. The problem has been around for so long that I feel everybody has
> lost hope.
>
> I don't think this is unsolvable, but we need to regain motivation. In my
> opinion the first step would be to define the precise extent of the problem we
> want to solve.

I'm not sure anyone really tried hard enough (in the same way no one
tried hard enough to destage android syncpts, until last year). And
anything new should at least very clearly explain why ION (even with
the various todo items we collected at a few conferences) won't work,
and how exactly the new allocator is different from ION. I don't think
we need a full design doc (like you say, buffer allocation is hard,
we'll get it wrong anyway), but at least a proper comparison with the
existing thing. Plus explanation why we can't reuse the uabi.

Because ime when you rewrite something, you generally get one thing
right (the one thing that pissed you off about the old solution), plus
lots and lots of things that the old solution got right, wrong
(because it's all lost in the history). ADF was probably the best
example in this. KMS also took a while until all the fbdev wheels have
been properly reinvented (some are still the same old squeaky onces as
fbdev had, e.g. fbcon).

And I don't think destaging ION is going to be hard, just a bit of
work (could be a nice gsoc or whatever).
-Daniel

>
>> > ---
>> >
>> >  Documentation/simple-allocator.txt              |  81 +++++++++++
>> >  drivers/Kconfig                                 |   2 +
>> >  drivers/Makefile                                |   1 +
>> >  drivers/simpleallocator/Kconfig                 |  10 ++
>> >  drivers/simpleallocator/Makefile                |   1 +
>> >  drivers/simpleallocator/simple-allocator-priv.h |  33 +++++
>> >  drivers/simpleallocator/simple-allocator.c      | 180 +++++++++++++++++++
>> >  include/uapi/linux/simple-allocator.h           |  35 +++++
>> >  8 files changed, 343 insertions(+)
>> >  create mode 100644 Documentation/simple-allocator.txt
>> >  create mode 100644 drivers/simpleallocator/Kconfig
>> >  create mode 100644 drivers/simpleallocator/Makefile
>> >  create mode 100644 drivers/simpleallocator/simple-allocator-priv.h
>> >  create mode 100644 drivers/simpleallocator/simple-allocator.c
>> >  create mode 100644 include/uapi/linux/simple-allocator.h
>> >
>> > diff --git a/Documentation/simple-allocator.txt
>> > b/Documentation/simple-allocator.txt new file mode 100644
>> > index 0000000..89ba883
>> > --- /dev/null
>> > +++ b/Documentation/simple-allocator.txt
>> > @@ -0,0 +1,81 @@
>> > +Simple Allocator Framework
>> > +
>> > +Simple Allocator offer a single ioctl SA_IOC_ALLOC to allocate buffers
>> > +on dedicated memory regions and export them as a dmabuf file descriptor.
>> > +Using dmabuf file descriptor allow to share this memory between processes
>> > +and/or import it into other frameworks like v4l2 or drm/kms (prime).
>> > +When userland wants to free the memory only a call to close() in needed
>> > +so it could done even without knowing that buffer has been allocated by
>> > +simple allocator ioctl.
>> > +
>> > +Each memory regions will be seen as a filein /dev/.
>> > +For example CMA regions will exposed has /dev/cmaX.
>> > +
>> > +Implementing a simple allocator
>> > +-------------------------------
>> > +
>> > +Simple Allocator provide helpers functions to register/unregister an
>> > +allocator:
>> > +- simple_allocator_register(struct sa_device *sadev)
>> > +  Register simple_allocator_device using sa_device structure where name,
>> > +  owner and allocate fields must be set.
>> > +
>> > +- simple_allocator_unregister(struct sa_device *sadev)
>> > +  Unregister a simple allocator device.
>> > +
>> > +Using Simple Allocator /dev interface example
>> > +---------------------------------------------
>> > +
>> > +This example of code allocate a buffer on the first CMA region
>> > (/dev/cma0)
>> > +before mmap and close it.
>> > +
>> > +#include <errno.h>
>> > +#include <fcntl.h>
>> > +#include <stdio.h>
>> > +#include <stdlib.h>
>> > +#include <string.h>
>> > +#include <unistd.h>
>> > +#include <sys/ioctl.h>
>> > +#include <sys/mman.h>
>> > +#include <sys/stat.h>
>> > +#include <sys/types.h>
>> > +
>> > +#include "simple-allocator.h"
>> > +
>> > +#define LENGTH 1024*16
>> > +
>> > +void main (void)
>> > +{
>> > +       struct simple_allocate_data data;
>> > +       int fd = open("/dev/cma0", O_RDWR, 0);
>> > +       int ret;
>> > +       void *mem;
>> > +
>> > +       if (fd < 0) {
>> > +               printf("Can't open /dev/cma0\n");
>> > +               return;
>> > +       }
>> > +
>> > +       memset(&data, 0, sizeof(data));
>> > +
>> > +       data.length = LENGTH;
>> > +       data.flags = O_RDWR | O_CLOEXEC;
>> > +
>> > +       ret = ioctl(fd, SA_IOC_ALLOC, &data);
>> > +       if (ret) {
>> > +               printf("Buffer allocation failed\n");
>> > +               goto end;
>> > +       }
>> > +
>> > +       mem = mmap(0, LENGTH, PROT_READ | PROT_WRITE, MAP_SHARED, data.fd,
>> > 0); +       if (mem == MAP_FAILED) {
>> > +               printf("mmap failed\n");
>> > +       }
>> > +
>> > +       memset(mem, 0xFF, LENGTH);
>> > +       munmap(mem, LENGTH);
>> > +
>> > +       printf("test simple allocator CMA OK\n");
>> > +end:
>> > +       close(fd);
>> > +}
>> > diff --git a/drivers/Kconfig b/drivers/Kconfig
>> > index e1e2066..a6d8828 100644
>> > --- a/drivers/Kconfig
>> > +++ b/drivers/Kconfig
>> > @@ -202,4 +202,6 @@ source "drivers/hwtracing/intel_th/Kconfig"
>> >
>> >  source "drivers/fpga/Kconfig"
>> >
>> > +source "drivers/simpleallocator/Kconfig"
>> > +
>> >
>> >  endmenu
>> >
>> > diff --git a/drivers/Makefile b/drivers/Makefile
>> > index 060026a..5081eb8 100644
>> > --- a/drivers/Makefile
>> > +++ b/drivers/Makefile
>> > @@ -173,3 +173,4 @@ obj-$(CONFIG_STM)           += hwtracing/stm/
>> >
>> >  obj-$(CONFIG_ANDROID)          += android/
>> >  obj-$(CONFIG_NVMEM)            += nvmem/
>> >  obj-$(CONFIG_FPGA)             += fpga/
>> >
>> > +obj-$(CONFIG_SIMPLE_ALLOCATOR)         += simpleallocator/
>> > diff --git a/drivers/simpleallocator/Kconfig
>> > b/drivers/simpleallocator/Kconfig new file mode 100644
>> > index 0000000..c6fc2e3
>> > --- /dev/null
>> > +++ b/drivers/simpleallocator/Kconfig
>> > @@ -0,0 +1,10 @@
>> > +menu "Simple Allocator"
>> > +
>> > +config SIMPLE_ALLOCATOR
>> > +       tristate "Simple Alllocator Framework"
>> > +       select DMA_SHARED_BUFFER
>> > +       ---help---
>> > +          The Simple Allocator Framework adds an API to allocate and
>> > share
>> > +          memory in userland.
>> > +
>> > +endmenu
>> > diff --git a/drivers/simpleallocator/Makefile
>> > b/drivers/simpleallocator/Makefile new file mode 100644
>> > index 0000000..e27c6ad
>> > --- /dev/null
>> > +++ b/drivers/simpleallocator/Makefile
>> > @@ -0,0 +1 @@
>> > +obj-$(CONFIG_SIMPLE_ALLOCATOR) += simple-allocator.o
>> > diff --git a/drivers/simpleallocator/simple-allocator-priv.h
>> > b/drivers/simpleallocator/simple-allocator-priv.h new file mode 100644
>> > index 0000000..33f5a33
>> > --- /dev/null
>> > +++ b/drivers/simpleallocator/simple-allocator-priv.h
>> > @@ -0,0 +1,33 @@
>> > +/*
>> > + * Copyright (C) Linaro 2016
>> > + *
>> > + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> > + *
>> > + * License terms:  GNU General Public License (GPL)
>> > + */
>> > +
>> > +#ifndef _SIMPLE_ALLOCATOR_PRIV_H_
>> > +#define _SIMPLE_ALLOCATOR_PRIV_H_
>> > +
>> > +#include <linux/cdev.h>
>> > +#include <linux/device.h>
>> > +#include <linux/dma-buf.h>
>> > +
>> > +/**
>> > + * struct sa_device - simple allocator device
>> > + * @owner: module owner, must be set to THIS_MODULE
>> > + * @name: name of the allocator
>> > + * @allocate: callabck for memory allocation
>> > + */
>> > +struct sa_device {
>> > +       struct device   dev;
>> > +       struct cdev     chrdev;
>> > +       struct module   *owner;
>> > +       const char      *name;
>> > +       struct dma_buf *(*allocate)(struct sa_device *, u64 length, u32
>> > flags); +};
>> > +
>> > +int simple_allocator_register(struct sa_device *sadev);
>> > +void simple_allocator_unregister(struct sa_device *sadev);
>> > +
>> > +#endif
>> > diff --git a/drivers/simpleallocator/simple-allocator.c
>> > b/drivers/simpleallocator/simple-allocator.c new file mode 100644
>> > index 0000000..d89ccbf
>> > --- /dev/null
>> > +++ b/drivers/simpleallocator/simple-allocator.c
>> > @@ -0,0 +1,180 @@
>> > +/*
>> > + * Copyright (C) Linaro 2016
>> > + *
>> > + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> > + *
>> > + * License terms:  GNU General Public License (GPL)
>> > + */
>> > +
>> > +#include <linux/module.h>
>> > +#include <linux/simple-allocator.h>
>> > +#include <linux/uaccess.h>
>> > +
>> > +#include "simple-allocator-priv.h"
>> > +
>> > +#define SA_MAJOR       222
>> > +#define SA_NUM_DEVICES 256
>> > +#define SA_NAME                "simple_allocator"
>> > +
>> > +static int sa_minor;
>> > +
>> > +static struct class sa_class = {
>> > +       .name = SA_NAME,
>> > +};
>> > +
>> > +static long sa_ioctl(struct file *filp, unsigned int cmd, unsigned long
>> > arg) +{
>> > +       struct sa_device *sadev = filp->private_data;
>> > +       int ret = -ENODEV;
>> > +
>> > +       switch (cmd) {
>> > +       case SA_IOC_ALLOC:
>> > +       {
>> > +               struct simple_allocate_data data;
>> > +               struct dma_buf *dmabuf;
>> > +
>> > +               if (copy_from_user(&data, (void __user *)arg,
>> > _IOC_SIZE(cmd))) +                       return -EFAULT;
>> > +
>> > +               if (data.version != 0)
>> > +                       return -EINVAL;
>> > +
>> > +               dmabuf = sadev->allocate(sadev, data.length, data.flags);
>> > +               if (!dmabuf)
>> > +                       return -EINVAL;
>> > +
>> > +               data.fd = dma_buf_fd(dmabuf, data.flags);
>> > +               if (data.fd < 0) {
>> > +                       dma_buf_put(dmabuf);
>> > +                       return -EINVAL;
>> > +               }
>> > +
>> > +               data.length = dmabuf->size;
>> > +
>> > +               if (copy_to_user((void __user *)arg, &data,
>> > _IOC_SIZE(cmd))) { +                       dma_buf_put(dmabuf);
>> > +                       return -EFAULT;
>> > +               }
>> > +
>> > +               return 0;
>> > +       }
>> > +       }
>> > +       return ret;
>> > +}
>> > +
>> > +static int sa_open(struct inode *inode, struct file *filp)
>> > +{
>> > +       struct sa_device *sadev = container_of(inode->i_cdev,
>> > +                                              struct sa_device, chrdev);
>> > +
>> > +       if (!sadev)
>> > +               return -ENODEV;
>> > +
>> > +       get_device(&sadev->dev);
>> > +       filp->private_data = sadev;
>> > +       return 0;
>> > +}
>> > +
>> > +static int sa_release(struct inode *inode, struct file *filp)
>> > +{
>> > +       struct sa_device *sadev = container_of(inode->i_cdev,
>> > +                                              struct sa_device, chrdev);
>> > +
>> > +       if (!sadev)
>> > +               return -ENODEV;
>> > +
>> > +       put_device(&sadev->dev);
>> > +       return 0;
>> > +}
>> > +
>> > +static const struct file_operations sa_fops = {
>> > +       .owner = THIS_MODULE,
>> > +       .open = sa_open,
>> > +       .release = sa_release,
>> > +       .unlocked_ioctl = sa_ioctl,
>> > +};
>> > +
>> > +/**
>> > + * simple_allocator_register - register a simple allocator
>> > + * @sadev: simple allocator structure to be registered
>> > + *
>> > + * Return 0 if allocator has been regsitered, either a negative value.
>> > + */
>> > +int simple_allocator_register(struct sa_device *sadev)
>> > +{
>> > +       int ret;
>> > +
>> > +       if (!sadev->name || !sadev->allocate)
>> > +               return -EINVAL;
>> > +
>> > +       cdev_init(&sadev->chrdev, &sa_fops);
>> > +       sadev->chrdev.owner = sadev->owner;
>> > +
>> > +       ret = cdev_add(&sadev->chrdev, MKDEV(SA_MAJOR, sa_minor), 1);
>> > +       if (ret < 0)
>> > +               return ret;
>> > +
>> > +       sadev->dev.class = &sa_class;
>> > +       sadev->dev.devt = MKDEV(SA_MAJOR, sa_minor);
>> > +       dev_set_name(&sadev->dev, "%s%d", sadev->name, sa_minor);
>> > +       ret = device_register(&sadev->dev);
>> > +       if (ret < 0)
>> > +               goto cleanup;
>> > +
>> > +       sa_minor++;
>> > +       return 0;
>> > +
>> > +cleanup:
>> > +       cdev_del(&sadev->chrdev);
>> > +       return ret;
>> > +}
>> > +EXPORT_SYMBOL_GPL(simple_allocator_register);
>> > +
>> > +/**
>> > + * simple_allocator_unregister - unregister a simple allocator
>> > + * @sadev: simple allocator device to be unregistered
>> > + */
>> > +void simple_allocator_unregister(struct sa_device *sadev)
>> > +{
>> > +       if (!sadev)
>> > +               return;
>> > +
>> > +       cdev_del(&sadev->chrdev);
>> > +       device_del(&sadev->dev);
>> > +       put_device(&sadev->dev);
>> > +}
>> > +EXPORT_SYMBOL_GPL(simple_allocator_unregister);
>> > +
>> > +static int __init sa_init(void)
>> > +{
>> > +       dev_t dev = MKDEV(SA_MAJOR, 0);
>> > +       int ret;
>> > +
>> > +       ret = register_chrdev_region(dev, SA_NUM_DEVICES, SA_NAME);
>> > +       if (ret < 0)
>> > +               return ret;
>> > +
>> > +       ret = class_register(&sa_class);
>> > +       if (ret < 0) {
>> > +               unregister_chrdev_region(dev, SA_NUM_DEVICES);
>> > +               return -EIO;
>> > +       }
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static void __exit sa_exit(void)
>> > +{
>> > +       dev_t dev = MKDEV(SA_MAJOR, 0);
>> > +
>> > +       class_unregister(&sa_class);
>> > +       unregister_chrdev_region(dev, SA_NUM_DEVICES);
>> > +}
>> > +
>> > +subsys_initcall(sa_init);
>> > +module_exit(sa_exit);
>> > +
>> > +MODULE_AUTHOR("Benjamin Gaignard <benjamin.gaignard@linaro.org>");
>> > +MODULE_DESCRIPTION("Simple allocator");
>> > +MODULE_LICENSE("GPL");
>> > +MODULE_ALIAS_CHARDEV_MAJOR(SA_MAJOR);
>> > diff --git a/include/uapi/linux/simple-allocator.h
>> > b/include/uapi/linux/simple-allocator.h new file mode 100644
>> > index 0000000..5520a85
>> > --- /dev/null
>> > +++ b/include/uapi/linux/simple-allocator.h
>> > @@ -0,0 +1,35 @@
>> > +/*
>> > + * Copyright (C) Linaro 2016
>> > + *
>> > + * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> > + *
>> > + * License terms:  GNU General Public License (GPL), version 2
>> > + */
>> > +
>> > +#ifndef _SIMPLE_ALLOCATOR_H_
>> > +#define _SIMPLE_ALLOCATOR_H_
>> > +
>> > +#include <linux/ioctl.h>
>> > +#include <linux/types.h>
>> > +
>> > +/**
>> > + * struct simple_allocate_data - allocation parameters
>> > + * @version:   structure version (must be set to 0)
>> > + * @length:    size of the requested buffer
>> > + * @flags:     mode flags for the file like O_RDWR or O_CLOEXEC
>> > + * @fd:                returned file descriptor
>> > + */
>> > +struct simple_allocate_data {
>> > +       __u64 version;
>> > +       __u64 length;
>> > +       __u32 flags;
>> > +       __u32 reserved1;
>> > +       __s32 fd;
>> > +       __u32 reserved2;
>> > +};
>> > +
>> > +#define SA_IOC_MAGIC 'S'
>> > +
>> > +#define SA_IOC_ALLOC _IOWR(SA_IOC_MAGIC, 0, struct simple_allocate_data)
>> > +
>> > +#endif
>> > --
>> > 1.9.1
>
> --
> Regards,
>
> Laurent Pinchart
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC simple allocator v2 1/2] Create Simple Allocator module

[Laurent Pinchart]

Hi Daniel,

On Tuesday 14 Feb 2017 20:44:44 Daniel Vetter wrote:
> On Tue, Feb 14, 2017 at 8:39 PM, Laurent Pinchart wrote:
> > On Tuesday 14 Feb 2017 20:33:58 Daniel Vetter wrote:
> >> On Mon, Feb 13, 2017 at 3:45 PM, Benjamin Gaignard wrote:
> >>> This is the core of simple allocator module.
> >>> It aim to offert one common ioctl to allocate specific memory.
> >>> 
> >>> version 2:
> >>> - rebased on 4.10-rc7
> >>> 
> >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
> >> 
> >> Why not ION? It's a bit a broken record question, but if there is a
> >> clear answer it should be in the patch&docs ...
> > 
> > There's a bit of love & hate relationship between Linux developers and
> > ION. The API has shortcomings, and attempts to fix the issues went
> > nowhere. As Laura explained, starting from a blank slate (obviously
> > keeping in mind the lessons learnt so far with ION and other similar APIs)
> > and then adding a wrapper to expose ION on Android systems (at least as an
> > interim measure) was thought to be a better option. I still believe it is,
> > but we seem to lack traction. The problem has been around for so long that
> > I feel everybody has lost hope.
> > 
> > I don't think this is unsolvable, but we need to regain motivation. In my
> > opinion the first step would be to define the precise extent of the
> > problem we want to solve.
> 
> I'm not sure anyone really tried hard enough (in the same way no one
> tried hard enough to destage android syncpts, until last year). And
> anything new should at least very clearly explain why ION (even with
> the various todo items we collected at a few conferences) won't work,
> and how exactly the new allocator is different from ION. I don't think
> we need a full design doc (like you say, buffer allocation is hard,
> we'll get it wrong anyway), but at least a proper comparison with the
> existing thing. Plus explanation why we can't reuse the uabi.

I've explained several of my concerns (including open questions that need 
answers) in another reply to this patch, let's discuss them there to avoid 
splitting the discussion.

> Because ime when you rewrite something, you generally get one thing
> right (the one thing that pissed you off about the old solution), plus
> lots and lots of things that the old solution got right, wrong
> (because it's all lost in the history).

History, repeating mistakes, all that. History never repeats itself though. We 
might make similar or identical mistakes, but there's no fatality, unless we 
decide not to try before even starting :-)

> ADF was probably the best example in this. KMS also took a while until all
> the fbdev wheels have been properly reinvented (some are still the same old
> squeaky onces as fbdev had, e.g. fbcon).
> 
> And I don't think destaging ION is going to be hard, just a bit of
> work (could be a nice gsoc or whatever).

Oh, technically speaking, it would be pretty simple. The main issue is to 
decide whether we want to commit to the existing ION API. I don't :-)

-- 
Regards,

Laurent Pinchart

Re: [RFC simple allocator v2 1/2] Create Simple Allocator module

[Benjamin Gaignard]

2017-02-14 20:59 GMT+01:00 Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> Hi Daniel,
>
> On Tuesday 14 Feb 2017 20:44:44 Daniel Vetter wrote:
>> On Tue, Feb 14, 2017 at 8:39 PM, Laurent Pinchart wrote:
>> > On Tuesday 14 Feb 2017 20:33:58 Daniel Vetter wrote:
>> >> On Mon, Feb 13, 2017 at 3:45 PM, Benjamin Gaignard wrote:
>> >>> This is the core of simple allocator module.
>> >>> It aim to offert one common ioctl to allocate specific memory.
>> >>>
>> >>> version 2:
>> >>> - rebased on 4.10-rc7
>> >>>
>> >>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
>> >>
>> >> Why not ION? It's a bit a broken record question, but if there is a
>> >> clear answer it should be in the patch&docs ...
>> >
>> > There's a bit of love & hate relationship between Linux developers and
>> > ION. The API has shortcomings, and attempts to fix the issues went
>> > nowhere. As Laura explained, starting from a blank slate (obviously
>> > keeping in mind the lessons learnt so far with ION and other similar APIs)
>> > and then adding a wrapper to expose ION on Android systems (at least as an
>> > interim measure) was thought to be a better option. I still believe it is,
>> > but we seem to lack traction. The problem has been around for so long that
>> > I feel everybody has lost hope.
>> >
>> > I don't think this is unsolvable, but we need to regain motivation. In my
>> > opinion the first step would be to define the precise extent of the
>> > problem we want to solve.
>>
>> I'm not sure anyone really tried hard enough (in the same way no one
>> tried hard enough to destage android syncpts, until last year). And
>> anything new should at least very clearly explain why ION (even with
>> the various todo items we collected at a few conferences) won't work,
>> and how exactly the new allocator is different from ION. I don't think
>> we need a full design doc (like you say, buffer allocation is hard,
>> we'll get it wrong anyway), but at least a proper comparison with the
>> existing thing. Plus explanation why we can't reuse the uabi.
>
> I've explained several of my concerns (including open questions that need
> answers) in another reply to this patch, let's discuss them there to avoid
> splitting the discussion.
>
>> Because ime when you rewrite something, you generally get one thing
>> right (the one thing that pissed you off about the old solution), plus
>> lots and lots of things that the old solution got right, wrong
>> (because it's all lost in the history).
>
> History, repeating mistakes, all that. History never repeats itself though. We
> might make similar or identical mistakes, but there's no fatality, unless we
> decide not to try before even starting :-)
>
>> ADF was probably the best example in this. KMS also took a while until all
>> the fbdev wheels have been properly reinvented (some are still the same old
>> squeaky onces as fbdev had, e.g. fbcon).
>>
>> And I don't think destaging ION is going to be hard, just a bit of
>> work (could be a nice gsoc or whatever).
>
> Oh, technically speaking, it would be pretty simple. The main issue is to
> decide whether we want to commit to the existing ION API. I don't :-)

I think that Laura have give her felling about ION when commenting the previous
version of this patchset:
https://lkml.org/lkml/2017/1/25/76

>
> --
> Regards,
>
> Laurent Pinchart

Re: [RFC simple allocator v2 1/2] Create Simple Allocator module

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

On Tue, Feb 14, 2017 at 09:59:55PM +0200, Laurent Pinchart wrote:
> On Tuesday 14 Feb 2017 20:44:44 Daniel Vetter wrote:

> > ADF was probably the best example in this. KMS also took a while until all
> > the fbdev wheels have been properly reinvented (some are still the same old
> > squeaky onces as fbdev had, e.g. fbcon).

> > And I don't think destaging ION is going to be hard, just a bit of
> > work (could be a nice gsoc or whatever).

> Oh, technically speaking, it would be pretty simple. The main issue is to 
> decide whether we want to commit to the existing ION API. I don't :-)

Right, we need to figure out what people should be doing and let them
work on it.  At the minute anyone who wants to use this stuff in
mainline is kind of stuck as attempts to add ION drivers get pushback

   https://lkml.org/lkml/2016/11/7/806

but so do attempts to do something different (there was a statement in
that thread that new ION drivers could be added if we could ever figure
out bindings but I'm not sure there's any prospect of that).  There's no
clear direction for people to follow if they want to make progress.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC simple allocator v2 1/2] Create Simple Allocator module

[Daniel Vetter]

On Wed, Feb 15, 2017 at 12:15:26PM +0000, Mark Brown wrote:
> On Tue, Feb 14, 2017 at 09:59:55PM +0200, Laurent Pinchart wrote:
> > On Tuesday 14 Feb 2017 20:44:44 Daniel Vetter wrote:
> 
> > > ADF was probably the best example in this. KMS also took a while until all
> > > the fbdev wheels have been properly reinvented (some are still the same old
> > > squeaky onces as fbdev had, e.g. fbcon).
> 
> > > And I don't think destaging ION is going to be hard, just a bit of
> > > work (could be a nice gsoc or whatever).
> 
> > Oh, technically speaking, it would be pretty simple. The main issue is to 
> > decide whether we want to commit to the existing ION API. I don't :-)
> 
> Right, we need to figure out what people should be doing and let them
> work on it.  At the minute anyone who wants to use this stuff in
> mainline is kind of stuck as attempts to add ION drivers get pushback
> 
>    https://lkml.org/lkml/2016/11/7/806
> 
> but so do attempts to do something different (there was a statement in
> that thread that new ION drivers could be added if we could ever figure
> out bindings but I'm not sure there's any prospect of that).  There's no
> clear direction for people to follow if they want to make progress.

Hm, this feels like a misunderstanding ... the unix device memory
allocator discussion is all about how to solve the userspace side on a
generic system (i.e. when you can't just hardcode everything in gralloc).
It's not really about where to actually allocate the kernel memory, for
that I think ION still looks as reasonable as anything else.

We just need to get around to working down the destaging todo items and
push it into something like drivers/gpu/ion or whatever. Feel free to cc
me and Laura and dri-devel on any such effort, this has been stuck way too
long.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[RFC simple allocator v2 2/2] add CMA simple allocator module

[Benjamin Gaignard]

This patch add simple allocator for CMA regions.

version 2:
- fix size and page count computation

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org>
---
 drivers/simpleallocator/Kconfig                |   7 +
 drivers/simpleallocator/Makefile               |   1 +
 drivers/simpleallocator/simple-allocator-cma.c | 187 +++++++++++++++++++++++++
 3 files changed, 195 insertions(+)
 create mode 100644 drivers/simpleallocator/simple-allocator-cma.c

diff --git a/drivers/simpleallocator/Kconfig b/drivers/simpleallocator/Kconfig
index c6fc2e3..788fb0b 100644
--- a/drivers/simpleallocator/Kconfig
+++ b/drivers/simpleallocator/Kconfig
@@ -7,4 +7,11 @@ config SIMPLE_ALLOCATOR
 	   The Simple Allocator Framework adds an API to allocate and share
 	   memory in userland.
 
+config SIMPLE_ALLOCATOR_CMA
+	tristate "Simple Allocator CMA"
+	select SIMPLE_ALLOCATOR
+	depends on DMA_CMA
+	---help---
+	   Select this option to enable Simple Allocator on CMA area.
+
 endmenu
diff --git a/drivers/simpleallocator/Makefile b/drivers/simpleallocator/Makefile
index e27c6ad..4e11611 100644
--- a/drivers/simpleallocator/Makefile
+++ b/drivers/simpleallocator/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_SIMPLE_ALLOCATOR) += simple-allocator.o
+obj-$(CONFIG_SIMPLE_ALLOCATOR_CMA) += simple-allocator-cma.o
diff --git a/drivers/simpleallocator/simple-allocator-cma.c b/drivers/simpleallocator/simple-allocator-cma.c
new file mode 100644
index 0000000..07cbf5b
--- /dev/null
+++ b/drivers/simpleallocator/simple-allocator-cma.c
@@ -0,0 +1,187 @@
+/*
+ * Copyright (C) Linaro 2017
+ *
+ * Author: Benjamin Gaignard <benjamin.gaignard@linaro.org>
+ *
+ * License terms:  GNU General Public License (GPL)
+ */
+
+#include <linux/cma.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include "simple-allocator-priv.h"
+#include "../mm/cma.h"
+
+struct sa_cma_device {
+	struct sa_device parent;
+	struct cma *cma;
+};
+
+struct sa_cma_buffer_info {
+	void *vaddr;
+	size_t count;
+	size_t size;
+	struct page *pages;
+	struct sa_cma_device *sa_cma;
+};
+
+static struct sa_cma_device *sa_cma[MAX_CMA_AREAS];
+
+static inline struct sa_cma_device *to_sa_cma(struct sa_device *sadev)
+{
+	return container_of(sadev, struct sa_cma_device, parent);
+}
+
+static struct sg_table *sa_cma_map_dma_buf(struct dma_buf_attachment *attach,
+					   enum dma_data_direction direction)
+{
+	struct dma_buf *dmabuf = attach->dmabuf;
+	struct sa_cma_buffer_info *info = dmabuf->priv;
+	struct sg_table *sgt;
+	int ret;
+
+	ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
+	if (unlikely(ret))
+		return NULL;
+
+	sg_set_page(sgt->sgl, info->pages, info->size, 0);
+	sg_dma_address(sgt->sgl) = (dma_addr_t) page_address(info->pages);
+	sg_dma_len(sgt->sgl) = info->size;
+
+	return sgt;
+}
+
+static void sa_cma_unmap_dma_buf(struct dma_buf_attachment *attach,
+				 struct sg_table *sgt,
+				 enum dma_data_direction dir)
+{
+	kfree(sgt);
+}
+
+static int sa_cma_mmap_dma_buf(struct dma_buf *dmabuf,
+			       struct vm_area_struct *vma)
+{
+	struct sa_cma_buffer_info *info = dmabuf->priv;
+	unsigned long user_count = vma_pages(vma);
+	unsigned long count = info->count;
+	unsigned long pfn = page_to_pfn(info->pages);
+	unsigned long off = vma->vm_pgoff;
+	int ret = -ENXIO;
+
+	if (off < count && user_count <= (count - off)) {
+		ret = remap_pfn_range(vma, vma->vm_start,
+				      pfn + off,
+				      user_count << PAGE_SHIFT,
+				      vma->vm_page_prot);
+	}
+
+	return ret;
+}
+
+static void sa_cma_release_dma_buf(struct dma_buf *dmabuf)
+{
+	struct sa_cma_buffer_info *info = dmabuf->priv;
+
+	cma_release(info->sa_cma->cma, info->pages, info->count);
+
+	kfree(info);
+}
+
+static void *sa_cma_kmap_dma_buf(struct dma_buf *dmabuf, unsigned long offset)
+{
+	struct sa_cma_buffer_info *info = dmabuf->priv;
+
+	return page_address(info->pages) + offset;
+}
+
+static struct dma_buf_ops sa_dma_buf_ops = {
+	.map_dma_buf = sa_cma_map_dma_buf,
+	.unmap_dma_buf = sa_cma_unmap_dma_buf,
+	.mmap = sa_cma_mmap_dma_buf,
+	.release = sa_cma_release_dma_buf,
+	.kmap_atomic = sa_cma_kmap_dma_buf,
+	.kmap = sa_cma_kmap_dma_buf,
+};
+
+static struct dma_buf *sa_cma_allocate(struct sa_device *sadev,
+				       u64 length, u32 flags)
+{
+	DEFINE_DMA_BUF_EXPORT_INFO(exp_info);
+	struct sa_cma_buffer_info *info;
+	struct dma_buf *dmabuf;
+
+	info = kzalloc(sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return NULL;
+
+	info->size = round_up(length, PAGE_SIZE);
+	info->count = PAGE_ALIGN(info->size) >> PAGE_SHIFT;
+	info->sa_cma = to_sa_cma(sadev);
+
+	info->pages = cma_alloc(info->sa_cma->cma, info->count, 0);
+
+	if (!info->pages)
+		goto cleanup;
+
+	exp_info.ops = &sa_dma_buf_ops;
+	exp_info.size = info->size;
+	exp_info.flags = flags;
+	exp_info.priv = info;
+
+	dmabuf = dma_buf_export(&exp_info);
+	if (IS_ERR(dmabuf))
+		goto export_failed;
+
+	return dmabuf;
+
+export_failed:
+	cma_release(info->sa_cma->cma, info->pages, info->count);
+cleanup:
+	kfree(info);
+	return NULL;
+}
+
+struct sa_cma_device *simple_allocator_register_cma(struct cma *cma)
+{
+	struct sa_cma_device *sa_cma;
+	int ret;
+
+	sa_cma = kzalloc(sizeof(*sa_cma), GFP_KERNEL);
+	if (!sa_cma)
+		return NULL;
+
+	sa_cma->cma = cma;
+	sa_cma->parent.owner = THIS_MODULE;
+	sa_cma->parent.name = "cma";
+	sa_cma->parent.allocate = sa_cma_allocate;
+
+	ret = simple_allocator_register(&sa_cma->parent);
+	if (ret) {
+		kfree(sa_cma);
+		return NULL;
+	}
+
+	return sa_cma;
+}
+
+static int __init sa_cma_init(void)
+{
+	int i;
+
+	for (i = 0; i < cma_area_count; i++)
+		sa_cma[i] = simple_allocator_register_cma(&cma_areas[i]);
+
+	return 0;
+}
+
+static void __exit sa_cma_exit(void)
+{
+	int i;
+
+	for (i = 0; i < cma_area_count; i++)
+		simple_allocator_unregister(&sa_cma[i]->parent);
+}
+
+module_init(sa_cma_init);
+module_exit(sa_cma_exit);
-- 
1.9.1

Re: [RFC simple allocator v2 0/2] Simple allocator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 564 bytes --]

On Mon, Feb 13, 2017 at 03:45:04PM +0100, Benjamin Gaignard wrote:

> An other question is: do we have others memory regions that could be interested
> by this new framework ? I have in mind that some title memory regions could use
> it or replace ION heaps (system, carveout, etc...).
> Maybe it only solve CMA allocation issue, in this case there is no need to create
> a new framework but only a dedicated ioctl.

The software defined networking people seemed to think they had a use
case for this as well.  They're not entirely upstream of course but
still...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC simple allocator v2 0/2] Simple allocator

[Laura Abbott]

On 02/13/2017 10:18 AM, Mark Brown wrote:
> On Mon, Feb 13, 2017 at 03:45:04PM +0100, Benjamin Gaignard wrote:
> 
>> An other question is: do we have others memory regions that could be interested
>> by this new framework ? I have in mind that some title memory regions could use
>> it or replace ION heaps (system, carveout, etc...).
>> Maybe it only solve CMA allocation issue, in this case there is no need to create
>> a new framework but only a dedicated ioctl.
> 
> The software defined networking people seemed to think they had a use
> case for this as well.  They're not entirely upstream of course but
> still...
> 

This is the first I've heard of anything like this. Do you have any more
details/reading?

Thanks,
Laura

Re: [RFC simple allocator v2 0/2] Simple allocator

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

On Mon, Feb 13, 2017 at 11:01:14AM -0800, Laura Abbott wrote:
> On 02/13/2017 10:18 AM, Mark Brown wrote:

> > The software defined networking people seemed to think they had a use
> > case for this as well.  They're not entirely upstream of course but
> > still...

> This is the first I've heard of anything like this. Do you have any more
> details/reading?

No, unfortunately it was in a meeting and I was asking for more details
on what specifically the hardware was doing myself.  My understanding is
that it's very similar to the GPU/video needs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]