[EARLY RFC][PATCH 0/4] ION per heap devices

[EARLY RFC][PATCH 0/4] ION per heap devices

[John Stultz via dri-devel]

This is a *very early RFC* (it builds, that's all I'll say :)
but I wanted to share it to get some initial feedback before I
go down the rabit hole of trying to adapt the Android userland
code to get this fully validated.

This patchset tries to implement the per-heap devices for ION.
The main benefit is that it avoids multiplexing heap operations
through the /dev/ion interface, and allows for each heap to have
its own permissions/sepolicy rules.

Feedback would be greatly appreciated!
thanks
-john

Cc: Laura Abbott <labbott@redhat.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Alistair Strachan <astrachan@google.com>
Cc: dri-devel@lists.freedesktop.org

John Stultz (4):
  ion: Add ION_VERSION ioctl
  ion: Initial hack to create per heap devices
  ion: Add HEAP_INFO ioctl to be able to fetch heap type
  ion: Make "legacy" /dev/ion support optional

 drivers/staging/android/ion/Kconfig     |  7 +++
 drivers/staging/android/ion/ion-ioctl.c | 80 +++++++++++++++++++++++++++++++++
 drivers/staging/android/ion/ion.c       | 51 ++++++++++++++++-----
 drivers/staging/android/ion/ion.h       |  6 +++
 drivers/staging/android/uapi/ion.h      | 57 +++++++++++++++++++++++
 5 files changed, 191 insertions(+), 10 deletions(-)

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[EARLY RFC][PATCH 1/4] ion: Add ION_VERSION ioctl

[John Stultz via dri-devel]

With all the slight interface changes ion has had
through its time in staging, keeping userland working
properly has been a pain. Assuming more churn going
forward, provide a proper version interface.

Cc: Laura Abbott <labbott@redhat.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Alistair Strachan <astrachan@google.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/ion/ion-ioctl.c | 4 ++++
 drivers/staging/android/ion/ion.h       | 2 ++
 drivers/staging/android/uapi/ion.h      | 7 +++++++
 3 files changed, 13 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index a8d3cc4..458a9f2 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -13,6 +13,7 @@
 union ion_ioctl_arg {
 	struct ion_allocation_data allocation;
 	struct ion_heap_query query;
+	u32 version;
 };
 
 static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
@@ -86,6 +87,9 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	case ION_IOC_HEAP_QUERY:
 		ret = ion_query_heaps(&data.query);
 		break;
+	case ION_IOC_VERSION:
+		data.version = ION_VERSION;
+		break;
 	default:
 		return -ENOTTY;
 	}
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index 47b594c..439e682 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -21,6 +21,8 @@
 
 #include "../uapi/ion.h"
 
+#define ION_VERSION 3
+
 /**
  * struct ion_platform_heap - defines a heap in the given platform
  * @type:	type of the heap from ion_heap_type enum
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
index 5d70098..c480448 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -124,4 +124,11 @@ struct ion_heap_query {
 #define ION_IOC_HEAP_QUERY     _IOWR(ION_IOC_MAGIC, 8, \
 					struct ion_heap_query)
 
+/**
+ * DOC: ION_IOC_VERSION - Get ION interface version
+ *
+ * Takes a u32 and returns the ION interface version
+ */
+#define ION_IOC_VERSION		_IOR(ION_IOC_MAGIC, 9, u32)
+
 #endif /* _UAPI_LINUX_ION_H */
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[EARLY RFC][PATCH 2/4] ion: Initial hack to create per heap devices

[John Stultz via dri-devel]

One of the issues w/ the /dev/ion interface is that we have to
provide the complexity of a heap query interface and we end up
multiplexing all the heap access through that one interface via
a bit mask (which currently limits the heaps to 32).

There has been a long running todo to provide per-heap devices
which would make the heap discovery/query interface "ls", and
would allow for different heaps to have different permisisons
and sepolicy rules.

TODOs:
* Android doesn't use udev so "ion_heaps/%s" names don't
  automatically create a /dev/ subdir. I need to rework
  from miscdev to creating a proper device class and add
  a "subsystem" entry for the DeviceHandler to match with

* Each CMA region is exposed via a separate heap, not sure
  if this is desired or not, and we may need to improve the
  naming.

Cc: Laura Abbott <labbott@redhat.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Alistair Strachan <astrachan@google.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/ion/ion-ioctl.c | 62 +++++++++++++++++++++++++++++++++
 drivers/staging/android/ion/ion.c       | 18 ++++++++++
 drivers/staging/android/ion/ion.h       |  2 ++
 drivers/staging/android/uapi/ion.h      | 28 +++++++++++++++
 4 files changed, 110 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index 458a9f2..ea8d263 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -12,6 +12,7 @@
 
 union ion_ioctl_arg {
 	struct ion_allocation_data allocation;
+	struct ion_heap_allocation_data heap_allocation;
 	struct ion_heap_query query;
 	u32 version;
 };
@@ -100,3 +101,64 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	}
 	return ret;
 }
+
+long ion_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
+{
+	int ret = 0;
+	unsigned int dir;
+	union ion_ioctl_arg data;
+
+	dir = ion_ioctl_dir(cmd);
+
+	if (_IOC_SIZE(cmd) > sizeof(data))
+		return -EINVAL;
+
+	/*
+	 * The copy_from_user is unconditional here for both read and write
+	 * to do the validate. If there is no write for the ioctl, the
+	 * buffer is cleared
+	 */
+	if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
+		return -EFAULT;
+
+	ret = validate_ioctl_arg(cmd, &data);
+	if (ret) {
+		pr_warn_once("%s: ioctl validate failed\n", __func__);
+		return ret;
+	}
+
+	if (!(dir & _IOC_WRITE))
+		memset(&data, 0, sizeof(data));
+
+	switch (cmd) {
+	case ION_IOC_HEAP_ALLOC:
+	{
+		struct miscdevice *miscdev = filp->private_data;
+		struct ion_heap *heap;
+		int fd;
+
+		heap = container_of(miscdev, struct ion_heap, heap_dev);
+
+		fd = ion_alloc(data.heap_allocation.len,
+			       (1 << heap->id),
+			       data.heap_allocation.flags);
+		if (fd < 0)
+			return fd;
+
+		data.heap_allocation.fd = fd;
+
+		break;
+	}
+	case ION_IOC_VERSION:
+		data.version = ION_VERSION;
+		break;
+	default:
+		return -ENOTTY;
+	}
+
+	if (dir & _IOC_READ) {
+		if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd)))
+			return -EFAULT;
+	}
+	return ret;
+}
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 6f5afab..1f7c893 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -492,6 +492,14 @@ int ion_query_heaps(struct ion_heap_query *query)
 	return ret;
 }
 
+static const struct file_operations ion_heap_fops = {
+	.owner          = THIS_MODULE,
+	.unlocked_ioctl = ion_heap_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= ion_heap_ioctl,
+#endif
+};
+
 static const struct file_operations ion_fops = {
 	.owner          = THIS_MODULE,
 	.unlocked_ioctl = ion_ioctl,
@@ -540,12 +548,22 @@ void ion_device_add_heap(struct ion_heap *heap)
 	struct ion_device *dev = internal_dev;
 	int ret;
 	struct dentry *heap_root;
+	char *heap_name;
 	char debug_name[64];
 
 	if (!heap->ops->allocate || !heap->ops->free)
 		pr_err("%s: can not add heap with invalid ops struct.\n",
 		       __func__);
 
+	heap_name = kasprintf(GFP_KERNEL, "ion_heaps/%s", heap->name);
+	heap->heap_dev.name = heap_name;
+	heap->heap_dev.minor = MISC_DYNAMIC_MINOR;
+	heap->heap_dev.fops = &ion_heap_fops;
+	heap->heap_dev.parent = NULL;
+	ret = misc_register(&heap->heap_dev);
+	if (ret)
+		pr_err("ion: failed to register misc device.\n");
+
 	spin_lock_init(&heap->free_lock);
 	spin_lock_init(&heap->stat_lock);
 	heap->free_list_size = 0;
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index 439e682..7ed4a6a 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -170,6 +170,7 @@ struct ion_heap_ops {
  */
 struct ion_heap {
 	struct plist_node node;
+	struct miscdevice heap_dev;
 	struct ion_device *dev;
 	enum ion_heap_type type;
 	struct ion_heap_ops *ops;
@@ -333,6 +334,7 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask,
 			 int nr_to_scan);
 
 long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
+long ion_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 
 int ion_query_heaps(struct ion_heap_query *query);
 
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
index c480448..20db09f 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -57,6 +57,25 @@ enum ion_heap_type {
  */
 
 /**
+ * struct ion_heap_allocation_data - metadata passed from userspace for
+ *                                   allocations
+ * @len:		size of the allocation
+ * @heap_id_mask:	mask of heap ids to allocate from
+ * @flags:		flags passed to heap
+ * @handle:		pointer that will be populated with a cookie to use to
+ *			refer to this allocation
+ *
+ * Provided by userspace as an argument to the ioctl
+ */
+struct ion_heap_allocation_data {
+	__u64 len;
+	__u32 flags;
+	__u32 fd;
+	__u32 reserved0;
+	__u32 reserved1;
+};
+
+/**
  * struct ion_allocation_data - metadata passed from userspace for allocations
  * @len:		size of the allocation
  * @heap_id_mask:	mask of heap ids to allocate from
@@ -131,4 +150,13 @@ struct ion_heap_query {
  */
 #define ION_IOC_VERSION		_IOR(ION_IOC_MAGIC, 9, u32)
 
+/**
+ * DOC: ION_IOC_HEAP_ALLOC - allocate memory from heap
+ *
+ * Takes an ion_heap_allocation_data struct and returns it with the handle field
+ * populated with the opaque handle for the allocation.
+ */
+#define ION_IOC_HEAP_ALLOC	_IOWR(ION_IOC_MAGIC, 10, \
+				      struct ion_heap_allocation_data)
+
 #endif /* _UAPI_LINUX_ION_H */
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[EARLY RFC][PATCH 3/4] ion: Add HEAP_INFO ioctl to be able to fetch heap type

[John Stultz via dri-devel]

The per-device heaps don't support HEAP_QUERY ioctl, since
the name is provided in the devnode path and the heapid isn't
useful with the new interface (one uses the fd of heapdevice).

But, one missing bit of functionality is a way to find the
heap type. So provide a HEAP_INFO ioctl which exposes the
heap type out so there is the potential for some sort of
dynamic heap matching/discovery.

Most likely this IOCTL will be useful when extended to allow
some sort of opaque constraint bitfield to be shared so userland
can match heaps with devices in a fully dynamic way.

Cc: Laura Abbott <labbott@redhat.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Alistair Strachan <astrachan@google.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/ion/ion-ioctl.c | 12 ++++++++++++
 drivers/staging/android/uapi/ion.h      | 22 ++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index ea8d263..6db5969 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -14,6 +14,7 @@ union ion_ioctl_arg {
 	struct ion_allocation_data allocation;
 	struct ion_heap_allocation_data heap_allocation;
 	struct ion_heap_query query;
+	struct ion_heap_info heap_info;
 	u32 version;
 };
 
@@ -149,6 +150,17 @@ long ion_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 
 		break;
 	}
+	case ION_IOC_HEAP_INFO:
+	{
+		struct miscdevice *miscdev = filp->private_data;
+		struct ion_heap *heap;
+
+		heap = container_of(miscdev, struct ion_heap, heap_dev);
+
+		data.heap_info.type = heap->type;
+
+		break;
+	}
 	case ION_IOC_VERSION:
 		data.version = ION_VERSION;
 		break;
diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
index 20db09f..1b3ca1e 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -111,6 +111,19 @@ struct ion_heap_data {
 };
 
 /**
+ * struct ion_heap_info - Info about the heap
+ *
+ */
+struct ion_heap_info {
+	__u32 type;
+	__u32 reserved0;
+	__u32 reserved1;
+	__u32 reserved2;
+	__u32 reserved3;
+	__u32 reserved4;
+};
+
+/**
  * struct ion_heap_query - collection of data about all heaps
  * @cnt - total number of heaps to be copied
  * @heaps - buffer to copy heap data
@@ -159,4 +172,13 @@ struct ion_heap_query {
 #define ION_IOC_HEAP_ALLOC	_IOWR(ION_IOC_MAGIC, 10, \
 				      struct ion_heap_allocation_data)
 
+/**
+ * DOC: ION_IOC_HEAP_INFO - allocate memory from heap
+ *
+ * Takes an ion_heap_query structure and populates information about
+ * available Ion heaps.
+ */
+#define ION_IOC_HEAP_INFO	_IOWR(ION_IOC_MAGIC, 11, \
+				      struct ion_heap_allocation_data)
+
 #endif /* _UAPI_LINUX_ION_H */
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[EARLY RFC][PATCH 4/4] ion: Make "legacy" /dev/ion support optional

[John Stultz via dri-devel]

Add a config option to make the "legacy" /dev/ion
interface optional.

Not yet clear if this will come out of staging or
not, but to help with the transition.

Cc: Laura Abbott <labbott@redhat.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Andrew F. Davis <afd@ti.com>
Cc: Alistair Strachan <astrachan@google.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/staging/android/ion/Kconfig     |  7 ++++++
 drivers/staging/android/ion/ion-ioctl.c |  2 ++
 drivers/staging/android/ion/ion.c       | 43 +++++++++++++++++++++------------
 drivers/staging/android/ion/ion.h       |  2 ++
 4 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/android/ion/Kconfig b/drivers/staging/android/ion/Kconfig
index 0fdda6f..7f5a0b2 100644
--- a/drivers/staging/android/ion/Kconfig
+++ b/drivers/staging/android/ion/Kconfig
@@ -42,3 +42,10 @@ config ION_CMA_HEAP
 	  Choose this option to enable CMA heaps with Ion. This heap is backed
 	  by the Contiguous Memory Allocator (CMA). If your system has these
 	  regions, you should say Y here.
+
+config ION_COMPAT_INTERFACE
+	bool "Legacy /dev/ion interface support"
+	depends on ION
+	help
+	  Choose this option to enable the legacy /dev/ion interface which
+	  provides compatibility with userspace designed for 4.12+ kernels
diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
index 6db5969..427980c 100644
--- a/drivers/staging/android/ion/ion-ioctl.c
+++ b/drivers/staging/android/ion/ion-ioctl.c
@@ -43,6 +43,7 @@ static unsigned int ion_ioctl_dir(unsigned int cmd)
 	}
 }
 
+#ifdef CONFIG_ION_COMPAT_INTERFACE
 long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	int ret = 0;
@@ -102,6 +103,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	}
 	return ret;
 }
+#endif
 
 long ion_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 1f7c893..9a73526 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -500,14 +500,6 @@ static const struct file_operations ion_heap_fops = {
 #endif
 };
 
-static const struct file_operations ion_fops = {
-	.owner          = THIS_MODULE,
-	.unlocked_ioctl = ion_ioctl,
-#ifdef CONFIG_COMPAT
-	.compat_ioctl	= ion_ioctl,
-#endif
-};
-
 static int debug_shrink_set(void *data, u64 val)
 {
 	struct ion_heap *heap = data;
@@ -619,15 +611,19 @@ void ion_device_add_heap(struct ion_heap *heap)
 }
 EXPORT_SYMBOL(ion_device_add_heap);
 
-static int ion_device_create(void)
+#ifdef CONFIG_ION_COMPAT_INTERFACE
+static const struct file_operations ion_fops = {
+	.owner          = THIS_MODULE,
+	.unlocked_ioctl = ion_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl	= ion_ioctl,
+#endif
+};
+
+static int ion_create_legacy_interface(struct ion_device *idev)
 {
-	struct ion_device *idev;
 	int ret;
 
-	idev = kzalloc(sizeof(*idev), GFP_KERNEL);
-	if (!idev)
-		return -ENOMEM;
-
 	idev->dev.minor = MISC_DYNAMIC_MINOR;
 	idev->dev.name = "ion";
 	idev->dev.fops = &ion_fops;
@@ -636,8 +632,25 @@ static int ion_device_create(void)
 	if (ret) {
 		pr_err("ion: failed to register misc device.\n");
 		kfree(idev);
-		return ret;
 	}
+	return ret;
+}
+#else
+#define ion_create_legacy_interface(x) 0
+#endif
+
+static int ion_device_create(void)
+{
+	struct ion_device *idev;
+	int ret;
+
+	idev = kzalloc(sizeof(*idev), GFP_KERNEL);
+	if (!idev)
+		return -ENOMEM;
+
+	ret = ion_create_legacy_interface(idev);
+	if (ret)
+		return ret;
 
 	idev->debug_root = debugfs_create_dir("ion", NULL);
 	idev->buffers = RB_ROOT;
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index 7ed4a6a..a82178b 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -333,7 +333,9 @@ void ion_page_pool_free(struct ion_page_pool *pool, struct page *page);
 int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask,
 			 int nr_to_scan);
 
+#ifdef CONFIG_ION_COMPAT_INTERFACE
 long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
+#endif
 long ion_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
 
 int ion_query_heaps(struct ion_heap_query *query);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 0/4] ION per heap devices

[Brian Starkey]

Hi John,

On Fri, Feb 15, 2019 at 12:24:08PM -0800, John Stultz wrote:
> This is a *very early RFC* (it builds, that's all I'll say :)
> but I wanted to share it to get some initial feedback before I
> go down the rabit hole of trying to adapt the Android userland
> code to get this fully validated.
> 
> This patchset tries to implement the per-heap devices for ION.
> The main benefit is that it avoids multiplexing heap operations
> through the /dev/ion interface, and allows for each heap to have
> its own permissions/sepolicy rules.

In general, I've always thought that having a device node per-heap is
a bit messy for userspace. Multiplexing through the single node
doesn't seem like an insurmountable problem, but the point about
permissions/sepolicy is reasonably compelling if it's a real
requirement. What would be the reasons for that?

Thanks,
-Brian

> 
> Feedback would be greatly appreciated!
> thanks
> -john
> 
> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Alistair Strachan <astrachan@google.com>
> Cc: dri-devel@lists.freedesktop.org
> 
> John Stultz (4):
>   ion: Add ION_VERSION ioctl
>   ion: Initial hack to create per heap devices
>   ion: Add HEAP_INFO ioctl to be able to fetch heap type
>   ion: Make "legacy" /dev/ion support optional
> 
>  drivers/staging/android/ion/Kconfig     |  7 +++
>  drivers/staging/android/ion/ion-ioctl.c | 80 +++++++++++++++++++++++++++++++++
>  drivers/staging/android/ion/ion.c       | 51 ++++++++++++++++-----
>  drivers/staging/android/ion/ion.h       |  6 +++
>  drivers/staging/android/uapi/ion.h      | 57 +++++++++++++++++++++++
>  5 files changed, 191 insertions(+), 10 deletions(-)
> 
> -- 
> 2.7.4
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 0/4] ION per heap devices

[John Stultz]

On Mon, Feb 18, 2019 at 3:51 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
> On Fri, Feb 15, 2019 at 12:24:08PM -0800, John Stultz wrote:
> > This is a *very early RFC* (it builds, that's all I'll say :)
> > but I wanted to share it to get some initial feedback before I
> > go down the rabit hole of trying to adapt the Android userland
> > code to get this fully validated.
> >
> > This patchset tries to implement the per-heap devices for ION.
> > The main benefit is that it avoids multiplexing heap operations
> > through the /dev/ion interface, and allows for each heap to have
> > its own permissions/sepolicy rules.
>
> In general, I've always thought that having a device node per-heap is
> a bit messy for userspace. Multiplexing through the single node
> doesn't seem like an insurmountable problem, but the point about

Hrm. I guess for me having a custom enumeration interface over ioctl
seems less ideal compared to a directory list.

> permissions/sepolicy is reasonably compelling if it's a real
> requirement. What would be the reasons for that?

Its a bit second hand for me, so hopefully I don't have it wrong. I've
cc'ed some additional google folks and Benjamin for extra context, but
my sense of it is that you may have some less-trusted code that we're
fine with allocating system heap dma-bufs, but don't want to to be
giving access to more limited heaps like carveout or cma, or more
potentially security troubling heaps like system-contig.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 1/4] ion: Add ION_VERSION ioctl

[Laura Abbott]

On 2/15/19 12:24 PM, John Stultz wrote:
> With all the slight interface changes ion has had
> through its time in staging, keeping userland working
> properly has been a pain. Assuming more churn going
> forward, provide a proper version interface.
> 
> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Alistair Strachan <astrachan@google.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>   drivers/staging/android/ion/ion-ioctl.c | 4 ++++
>   drivers/staging/android/ion/ion.h       | 2 ++
>   drivers/staging/android/uapi/ion.h      | 7 +++++++
>   3 files changed, 13 insertions(+)
> 
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index a8d3cc4..458a9f2 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -13,6 +13,7 @@
>   union ion_ioctl_arg {
>   	struct ion_allocation_data allocation;
>   	struct ion_heap_query query;
> +	u32 version;
>   };
>   
>   static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> @@ -86,6 +87,9 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>   	case ION_IOC_HEAP_QUERY:
>   		ret = ion_query_heaps(&data.query);
>   		break;
> +	case ION_IOC_VERSION:
> +		data.version = ION_VERSION;
> +		break;
>   	default:
>   		return -ENOTTY;
>   	}
> diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
> index 47b594c..439e682 100644
> --- a/drivers/staging/android/ion/ion.h
> +++ b/drivers/staging/android/ion/ion.h
> @@ -21,6 +21,8 @@
>   
>   #include "../uapi/ion.h"
>   
> +#define ION_VERSION 3
> +
>   /**
>    * struct ion_platform_heap - defines a heap in the given platform
>    * @type:	type of the heap from ion_heap_type enum
> diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
> index 5d70098..c480448 100644
> --- a/drivers/staging/android/uapi/ion.h
> +++ b/drivers/staging/android/uapi/ion.h
> @@ -124,4 +124,11 @@ struct ion_heap_query {
>   #define ION_IOC_HEAP_QUERY     _IOWR(ION_IOC_MAGIC, 8, \
>   					struct ion_heap_query)
>   
> +/**
> + * DOC: ION_IOC_VERSION - Get ION interface version
> + *
> + * Takes a u32 and returns the ION interface version
> + */
> +#define ION_IOC_VERSION		_IOR(ION_IOC_MAGIC, 9, u32)
> +
>   #endif /* _UAPI_LINUX_ION_H */
> 

Like I said on the other thread, I was told no before
https://lore.kernel.org/lkml/1472769644-11039-4-git-send-email-labbott@redhat.com/T/#u
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 0/4] ION per heap devices

[Laura Abbott]

On 2/19/19 9:21 AM, John Stultz wrote:
> On Mon, Feb 18, 2019 at 3:51 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
>> On Fri, Feb 15, 2019 at 12:24:08PM -0800, John Stultz wrote:
>>> This is a *very early RFC* (it builds, that's all I'll say :)
>>> but I wanted to share it to get some initial feedback before I
>>> go down the rabit hole of trying to adapt the Android userland
>>> code to get this fully validated.
>>>
>>> This patchset tries to implement the per-heap devices for ION.
>>> The main benefit is that it avoids multiplexing heap operations
>>> through the /dev/ion interface, and allows for each heap to have
>>> its own permissions/sepolicy rules.
>>
>> In general, I've always thought that having a device node per-heap is
>> a bit messy for userspace. Multiplexing through the single node
>> doesn't seem like an insurmountable problem, but the point about
> 
> Hrm. I guess for me having a custom enumeration interface over ioctl
> seems less ideal compared to a directory list.
> 
>> permissions/sepolicy is reasonably compelling if it's a real
>> requirement. What would be the reasons for that?
> 
> Its a bit second hand for me, so hopefully I don't have it wrong. I've
> cc'ed some additional google folks and Benjamin for extra context, but
> my sense of it is that you may have some less-trusted code that we're
> fine with allocating system heap dma-bufs, but don't want to to be
> giving access to more limited heaps like carveout or cma, or more
> potentially security troubling heaps like system-contig.
> 
> thanks
> -john
> 

Yes, the discussion was always based on being able to set separate
security policy for each heap. It's not clear to me how strong a
requirement is these days or if there's other options to enforce
the same thing.

Thanks,
Laura
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 3/4] ion: Add HEAP_INFO ioctl to be able to fetch heap type

[Laura Abbott]

On 2/15/19 12:24 PM, John Stultz wrote:
> The per-device heaps don't support HEAP_QUERY ioctl, since
> the name is provided in the devnode path and the heapid isn't
> useful with the new interface (one uses the fd of heapdevice).
> 
> But, one missing bit of functionality is a way to find the
> heap type. So provide a HEAP_INFO ioctl which exposes the
> heap type out so there is the potential for some sort of
> dynamic heap matching/discovery.
> 
> Most likely this IOCTL will be useful when extended to allow
> some sort of opaque constraint bitfield to be shared so userland
> can match heaps with devices in a fully dynamic way.
> 

We've been waiting on the constraint solving for a while and
it's never really happened :(

It certainly works but I'm concerned about adding this and
then finding (yet again) that it doesn't work. We're
getting the heap name now but do we lose anything if we
don't expose it as part of the ABI?

> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Alistair Strachan <astrachan@google.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>   drivers/staging/android/ion/ion-ioctl.c | 12 ++++++++++++
>   drivers/staging/android/uapi/ion.h      | 22 ++++++++++++++++++++++
>   2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index ea8d263..6db5969 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -14,6 +14,7 @@ union ion_ioctl_arg {
>   	struct ion_allocation_data allocation;
>   	struct ion_heap_allocation_data heap_allocation;
>   	struct ion_heap_query query;
> +	struct ion_heap_info heap_info;
>   	u32 version;
>   };
>   
> @@ -149,6 +150,17 @@ long ion_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>   
>   		break;
>   	}
> +	case ION_IOC_HEAP_INFO:
> +	{
> +		struct miscdevice *miscdev = filp->private_data;
> +		struct ion_heap *heap;
> +
> +		heap = container_of(miscdev, struct ion_heap, heap_dev);
> +
> +		data.heap_info.type = heap->type;
> +
> +		break;
> +	}
>   	case ION_IOC_VERSION:
>   		data.version = ION_VERSION;
>   		break;
> diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
> index 20db09f..1b3ca1e 100644
> --- a/drivers/staging/android/uapi/ion.h
> +++ b/drivers/staging/android/uapi/ion.h
> @@ -111,6 +111,19 @@ struct ion_heap_data {
>   };
>   
>   /**
> + * struct ion_heap_info - Info about the heap
> + *
> + */
> +struct ion_heap_info {
> +	__u32 type;
> +	__u32 reserved0;
> +	__u32 reserved1;
> +	__u32 reserved2;
> +	__u32 reserved3;
> +	__u32 reserved4;
> +};
> +
> +/**
>    * struct ion_heap_query - collection of data about all heaps
>    * @cnt - total number of heaps to be copied
>    * @heaps - buffer to copy heap data
> @@ -159,4 +172,13 @@ struct ion_heap_query {
>   #define ION_IOC_HEAP_ALLOC	_IOWR(ION_IOC_MAGIC, 10, \
>   				      struct ion_heap_allocation_data)
>   
> +/**
> + * DOC: ION_IOC_HEAP_INFO - allocate memory from heap
> + *
> + * Takes an ion_heap_query structure and populates information about
> + * available Ion heaps.
> + */
> +#define ION_IOC_HEAP_INFO	_IOWR(ION_IOC_MAGIC, 11, \
> +				      struct ion_heap_allocation_data)
> +
>   #endif /* _UAPI_LINUX_ION_H */
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 2/4] ion: Initial hack to create per heap devices

[Laura Abbott]

On 2/15/19 12:24 PM, John Stultz wrote:
> One of the issues w/ the /dev/ion interface is that we have to
> provide the complexity of a heap query interface and we end up
> multiplexing all the heap access through that one interface via
> a bit mask (which currently limits the heaps to 32).
> 
> There has been a long running todo to provide per-heap devices
> which would make the heap discovery/query interface "ls", and
> would allow for different heaps to have different permisisons
> and sepolicy rules.
> 
> TODOs:
> * Android doesn't use udev so "ion_heaps/%s" names don't
>    automatically create a /dev/ subdir. I need to rework
>    from miscdev to creating a proper device class and add
>    a "subsystem" entry for the DeviceHandler to match with
> 
> * Each CMA region is exposed via a separate heap, not sure
>    if this is desired or not, and we may need to improve the
>    naming.
> 

Every CMA region getting exposed was a side effect of doing
the eneumeration without tying it to devicetree or other firmware.
I'm not opposed to limiting the heaps exposed if we can find
a good way to do so that's still compliant with devicetree/whatever.

Thanks,
Laura

> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Alistair Strachan <astrachan@google.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>   drivers/staging/android/ion/ion-ioctl.c | 62 +++++++++++++++++++++++++++++++++
>   drivers/staging/android/ion/ion.c       | 18 ++++++++++
>   drivers/staging/android/ion/ion.h       |  2 ++
>   drivers/staging/android/uapi/ion.h      | 28 +++++++++++++++
>   4 files changed, 110 insertions(+)
> 
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index 458a9f2..ea8d263 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -12,6 +12,7 @@
>   
>   union ion_ioctl_arg {
>   	struct ion_allocation_data allocation;
> +	struct ion_heap_allocation_data heap_allocation;
>   	struct ion_heap_query query;
>   	u32 version;
>   };
> @@ -100,3 +101,64 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>   	}
>   	return ret;
>   }
> +
> +long ion_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> +	int ret = 0;
> +	unsigned int dir;
> +	union ion_ioctl_arg data;
> +
> +	dir = ion_ioctl_dir(cmd);
> +
> +	if (_IOC_SIZE(cmd) > sizeof(data))
> +		return -EINVAL;
> +
> +	/*
> +	 * The copy_from_user is unconditional here for both read and write
> +	 * to do the validate. If there is no write for the ioctl, the
> +	 * buffer is cleared
> +	 */
> +	if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd)))
> +		return -EFAULT;
> +
> +	ret = validate_ioctl_arg(cmd, &data);
> +	if (ret) {
> +		pr_warn_once("%s: ioctl validate failed\n", __func__);
> +		return ret;
> +	}
> +
> +	if (!(dir & _IOC_WRITE))
> +		memset(&data, 0, sizeof(data));
> +
> +	switch (cmd) {
> +	case ION_IOC_HEAP_ALLOC:
> +	{
> +		struct miscdevice *miscdev = filp->private_data;
> +		struct ion_heap *heap;
> +		int fd;
> +
> +		heap = container_of(miscdev, struct ion_heap, heap_dev);
> +
> +		fd = ion_alloc(data.heap_allocation.len,
> +			       (1 << heap->id),
> +			       data.heap_allocation.flags);
> +		if (fd < 0)
> +			return fd;
> +
> +		data.heap_allocation.fd = fd;
> +
> +		break;
> +	}
> +	case ION_IOC_VERSION:
> +		data.version = ION_VERSION;
> +		break;
> +	default:
> +		return -ENOTTY;
> +	}
> +
> +	if (dir & _IOC_READ) {
> +		if (copy_to_user((void __user *)arg, &data, _IOC_SIZE(cmd)))
> +			return -EFAULT;
> +	}
> +	return ret;
> +}
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 6f5afab..1f7c893 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -492,6 +492,14 @@ int ion_query_heaps(struct ion_heap_query *query)
>   	return ret;
>   }
>   
> +static const struct file_operations ion_heap_fops = {
> +	.owner          = THIS_MODULE,
> +	.unlocked_ioctl = ion_heap_ioctl,
> +#ifdef CONFIG_COMPAT
> +	.compat_ioctl	= ion_heap_ioctl,
> +#endif
> +};
> +
>   static const struct file_operations ion_fops = {
>   	.owner          = THIS_MODULE,
>   	.unlocked_ioctl = ion_ioctl,
> @@ -540,12 +548,22 @@ void ion_device_add_heap(struct ion_heap *heap)
>   	struct ion_device *dev = internal_dev;
>   	int ret;
>   	struct dentry *heap_root;
> +	char *heap_name;
>   	char debug_name[64];
>   
>   	if (!heap->ops->allocate || !heap->ops->free)
>   		pr_err("%s: can not add heap with invalid ops struct.\n",
>   		       __func__);
>   
> +	heap_name = kasprintf(GFP_KERNEL, "ion_heaps/%s", heap->name);
> +	heap->heap_dev.name = heap_name;
> +	heap->heap_dev.minor = MISC_DYNAMIC_MINOR;
> +	heap->heap_dev.fops = &ion_heap_fops;
> +	heap->heap_dev.parent = NULL;
> +	ret = misc_register(&heap->heap_dev);
> +	if (ret)
> +		pr_err("ion: failed to register misc device.\n");
> +
>   	spin_lock_init(&heap->free_lock);
>   	spin_lock_init(&heap->stat_lock);
>   	heap->free_list_size = 0;
> diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
> index 439e682..7ed4a6a 100644
> --- a/drivers/staging/android/ion/ion.h
> +++ b/drivers/staging/android/ion/ion.h
> @@ -170,6 +170,7 @@ struct ion_heap_ops {
>    */
>   struct ion_heap {
>   	struct plist_node node;
> +	struct miscdevice heap_dev;
>   	struct ion_device *dev;
>   	enum ion_heap_type type;
>   	struct ion_heap_ops *ops;
> @@ -333,6 +334,7 @@ int ion_page_pool_shrink(struct ion_page_pool *pool, gfp_t gfp_mask,
>   			 int nr_to_scan);
>   
>   long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
> +long ion_heap_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
>   
>   int ion_query_heaps(struct ion_heap_query *query);
>   
> diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
> index c480448..20db09f 100644
> --- a/drivers/staging/android/uapi/ion.h
> +++ b/drivers/staging/android/uapi/ion.h
> @@ -57,6 +57,25 @@ enum ion_heap_type {
>    */
>   
>   /**
> + * struct ion_heap_allocation_data - metadata passed from userspace for
> + *                                   allocations
> + * @len:		size of the allocation
> + * @heap_id_mask:	mask of heap ids to allocate from
> + * @flags:		flags passed to heap
> + * @handle:		pointer that will be populated with a cookie to use to
> + *			refer to this allocation
> + *
> + * Provided by userspace as an argument to the ioctl
> + */
> +struct ion_heap_allocation_data {
> +	__u64 len;
> +	__u32 flags;
> +	__u32 fd;
> +	__u32 reserved0;
> +	__u32 reserved1;
> +};
> +
> +/**
>    * struct ion_allocation_data - metadata passed from userspace for allocations
>    * @len:		size of the allocation
>    * @heap_id_mask:	mask of heap ids to allocate from
> @@ -131,4 +150,13 @@ struct ion_heap_query {
>    */
>   #define ION_IOC_VERSION		_IOR(ION_IOC_MAGIC, 9, u32)
>   
> +/**
> + * DOC: ION_IOC_HEAP_ALLOC - allocate memory from heap
> + *
> + * Takes an ion_heap_allocation_data struct and returns it with the handle field
> + * populated with the opaque handle for the allocation.
> + */
> +#define ION_IOC_HEAP_ALLOC	_IOWR(ION_IOC_MAGIC, 10, \
> +				      struct ion_heap_allocation_data)
> +
>   #endif /* _UAPI_LINUX_ION_H */
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 0/4] ION per heap devices

[Laura Abbott]

On 2/15/19 12:24 PM, John Stultz wrote:
> This is a *very early RFC* (it builds, that's all I'll say :)
> but I wanted to share it to get some initial feedback before I
> go down the rabit hole of trying to adapt the Android userland
> code to get this fully validated.
> 
> This patchset tries to implement the per-heap devices for ION.
> The main benefit is that it avoids multiplexing heap operations
> through the /dev/ion interface, and allows for each heap to have
> its own permissions/sepolicy rules.
> 
> Feedback would be greatly appreciated!
> thanks
> -john
> 
> Cc: Laura Abbott <labbott@redhat.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Andrew F. Davis <afd@ti.com>
> Cc: Alistair Strachan <astrachan@google.com>
> Cc: dri-devel@lists.freedesktop.org
> 
> John Stultz (4):
>    ion: Add ION_VERSION ioctl
>    ion: Initial hack to create per heap devices
>    ion: Add HEAP_INFO ioctl to be able to fetch heap type
>    ion: Make "legacy" /dev/ion support optional
> 
>   drivers/staging/android/ion/Kconfig     |  7 +++
>   drivers/staging/android/ion/ion-ioctl.c | 80 +++++++++++++++++++++++++++++++++
>   drivers/staging/android/ion/ion.c       | 51 ++++++++++++++++-----
>   drivers/staging/android/ion/ion.h       |  6 +++
>   drivers/staging/android/uapi/ion.h      | 57 +++++++++++++++++++++++
>   5 files changed, 191 insertions(+), 10 deletions(-)
> 

So it occurs to me if this is going to be a new ABI
all together maybe we should just declare a new allocation ioctl
to be used with it. We can keep the old ioctls around
for legacy use cases and maybe eventually delete them
and just use the new allocation ioctl with the new
split heaps.

Thanks,
Laura
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 1/4] ion: Add ION_VERSION ioctl

[John Stultz]

On Tue, Feb 19, 2019 at 12:46 PM Laura Abbott <labbott@redhat.com> wrote:
>
> On 2/15/19 12:24 PM, John Stultz wrote:
> > With all the slight interface changes ion has had
> > through its time in staging, keeping userland working
> > properly has been a pain. Assuming more churn going
> > forward, provide a proper version interface.
> >
> > Cc: Laura Abbott <labbott@redhat.com>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: Liam Mark <lmark@codeaurora.org>
> > Cc: Brian Starkey <Brian.Starkey@arm.com>
> > Cc: Andrew F. Davis <afd@ti.com>
> > Cc: Alistair Strachan <astrachan@google.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > ---
> >   drivers/staging/android/ion/ion-ioctl.c | 4 ++++
> >   drivers/staging/android/ion/ion.h       | 2 ++
> >   drivers/staging/android/uapi/ion.h      | 7 +++++++
> >   3 files changed, 13 insertions(+)
> >
> > diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> > index a8d3cc4..458a9f2 100644
> > --- a/drivers/staging/android/ion/ion-ioctl.c
> > +++ b/drivers/staging/android/ion/ion-ioctl.c
> > @@ -13,6 +13,7 @@
> >   union ion_ioctl_arg {
> >       struct ion_allocation_data allocation;
> >       struct ion_heap_query query;
> > +     u32 version;
> >   };
> >
> >   static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> > @@ -86,6 +87,9 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> >       case ION_IOC_HEAP_QUERY:
> >               ret = ion_query_heaps(&data.query);
> >               break;
> > +     case ION_IOC_VERSION:
> > +             data.version = ION_VERSION;
> > +             break;
> >       default:
> >               return -ENOTTY;
> >       }
> > diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
> > index 47b594c..439e682 100644
> > --- a/drivers/staging/android/ion/ion.h
> > +++ b/drivers/staging/android/ion/ion.h
> > @@ -21,6 +21,8 @@
> >
> >   #include "../uapi/ion.h"
> >
> > +#define ION_VERSION 3
> > +
> >   /**
> >    * struct ion_platform_heap - defines a heap in the given platform
> >    * @type:   type of the heap from ion_heap_type enum
> > diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
> > index 5d70098..c480448 100644
> > --- a/drivers/staging/android/uapi/ion.h
> > +++ b/drivers/staging/android/uapi/ion.h
> > @@ -124,4 +124,11 @@ struct ion_heap_query {
> >   #define ION_IOC_HEAP_QUERY     _IOWR(ION_IOC_MAGIC, 8, \
> >                                       struct ion_heap_query)
> >
> > +/**
> > + * DOC: ION_IOC_VERSION - Get ION interface version
> > + *
> > + * Takes a u32 and returns the ION interface version
> > + */
> > +#define ION_IOC_VERSION              _IOR(ION_IOC_MAGIC, 9, u32)
> > +
> >   #endif /* _UAPI_LINUX_ION_H */
> >
>
> Like I said on the other thread, I was told no before
> https://lore.kernel.org/lkml/1472769644-11039-4-git-send-email-labbott@redhat.com/T/#u

Alright, then. I'm ok with dropping it. Hopefully we can just avoid
any more really subtle abi breaks.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 0/4] ION per heap devices

[Andrew F. Davis]

On 2/19/19 3:25 PM, Laura Abbott wrote:
> On 2/15/19 12:24 PM, John Stultz wrote:
>> This is a *very early RFC* (it builds, that's all I'll say :)
>> but I wanted to share it to get some initial feedback before I
>> go down the rabit hole of trying to adapt the Android userland
>> code to get this fully validated.
>>
>> This patchset tries to implement the per-heap devices for ION.
>> The main benefit is that it avoids multiplexing heap operations
>> through the /dev/ion interface, and allows for each heap to have
>> its own permissions/sepolicy rules.
>>
>> Feedback would be greatly appreciated!
>> thanks
>> -john
>>
>> Cc: Laura Abbott <labbott@redhat.com>
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: Liam Mark <lmark@codeaurora.org>
>> Cc: Brian Starkey <Brian.Starkey@arm.com>
>> Cc: Andrew F. Davis <afd@ti.com>
>> Cc: Alistair Strachan <astrachan@google.com>
>> Cc: dri-devel@lists.freedesktop.org
>>
>> John Stultz (4):
>>    ion: Add ION_VERSION ioctl
>>    ion: Initial hack to create per heap devices
>>    ion: Add HEAP_INFO ioctl to be able to fetch heap type
>>    ion: Make "legacy" /dev/ion support optional
>>
>>   drivers/staging/android/ion/Kconfig     |  7 +++
>>   drivers/staging/android/ion/ion-ioctl.c | 80
>> +++++++++++++++++++++++++++++++++
>>   drivers/staging/android/ion/ion.c       | 51 ++++++++++++++++-----
>>   drivers/staging/android/ion/ion.h       |  6 +++
>>   drivers/staging/android/uapi/ion.h      | 57 +++++++++++++++++++++++
>>   5 files changed, 191 insertions(+), 10 deletions(-)
>>
> 
> So it occurs to me if this is going to be a new ABI
> all together maybe we should just declare a new allocation ioctl
> to be used with it. We can keep the old ioctls around
> for legacy use cases and maybe eventually delete them
> and just use the new allocation ioctl with the new
> split heaps.
> 

Why keep the old ones, this is staging, there are no legacy users (that
matter to kernel).. Slowing progress for the sake of backwards compat
with staging just slows the de-staging down.

Andrew

> Thanks,
> Laura
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 2/4] ion: Initial hack to create per heap devices

[John Stultz]

On Tue, Feb 19, 2019 at 1:17 PM Laura Abbott <labbott@redhat.com> wrote:
>
> On 2/15/19 12:24 PM, John Stultz wrote:
> > One of the issues w/ the /dev/ion interface is that we have to
> > provide the complexity of a heap query interface and we end up
> > multiplexing all the heap access through that one interface via
> > a bit mask (which currently limits the heaps to 32).
> >
> > There has been a long running todo to provide per-heap devices
> > which would make the heap discovery/query interface "ls", and
> > would allow for different heaps to have different permisisons
> > and sepolicy rules.
> >
> > TODOs:
> > * Android doesn't use udev so "ion_heaps/%s" names don't
> >    automatically create a /dev/ subdir. I need to rework
> >    from miscdev to creating a proper device class and add
> >    a "subsystem" entry for the DeviceHandler to match with
> >
> > * Each CMA region is exposed via a separate heap, not sure
> >    if this is desired or not, and we may need to improve the
> >    naming.
> >
>
> Every CMA region getting exposed was a side effect of doing
> the eneumeration without tying it to devicetree or other firmware.
> I'm not opposed to limiting the heaps exposed if we can find
> a good way to do so that's still compliant with devicetree/whatever.
>

I suspect this will actually be preferred in the end.  Its just that
having the CMA heaps (and whatever ill thought naming was used in the
dts) more visible made my nose crinkle a bit. But now its a good
motivator for more clear names in the dts' of future devices. But
allowing for separate cma heaps that can be logically partitioned
between uses doesn't seem like a negative to me.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 3/4] ion: Add HEAP_INFO ioctl to be able to fetch heap type

[Andrew F. Davis]

On 2/19/19 3:13 PM, Laura Abbott wrote:
> On 2/15/19 12:24 PM, John Stultz wrote:
>> The per-device heaps don't support HEAP_QUERY ioctl, since
>> the name is provided in the devnode path and the heapid isn't
>> useful with the new interface (one uses the fd of heapdevice).
>>
>> But, one missing bit of functionality is a way to find the
>> heap type. So provide a HEAP_INFO ioctl which exposes the
>> heap type out so there is the potential for some sort of
>> dynamic heap matching/discovery.
>>
>> Most likely this IOCTL will be useful when extended to allow
>> some sort of opaque constraint bitfield to be shared so userland
>> can match heaps with devices in a fully dynamic way.
>>
> 
> We've been waiting on the constraint solving for a while and
> it's never really happened :(
> 

Most likely there will never be a one-size-fits-all solution here. So
allowing for an extensible ABI that permits new information to be
exported as needed will be important.

> It certainly works but I'm concerned about adding this and
> then finding (yet again) that it doesn't work. We're
> getting the heap name now but do we lose anything if we
> don't expose it as part of the ABI?
> 

We can always add more ioctls, we cant go back and remove the old ones
if we make them too clunky and have to remove something they expose. A
simple starting ABI seems to make the most sense here. Even heap type
doesn't look like a good thing to expose, it is just as static and
one-off as heap name, I don't see it having all that much use :/

Andrew

>> Cc: Laura Abbott <labbott@redhat.com>
>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>> Cc: Liam Mark <lmark@codeaurora.org>
>> Cc: Brian Starkey <Brian.Starkey@arm.com>
>> Cc: Andrew F. Davis <afd@ti.com>
>> Cc: Alistair Strachan <astrachan@google.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>>   drivers/staging/android/ion/ion-ioctl.c | 12 ++++++++++++
>>   drivers/staging/android/uapi/ion.h      | 22 ++++++++++++++++++++++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>> b/drivers/staging/android/ion/ion-ioctl.c
>> index ea8d263..6db5969 100644
>> --- a/drivers/staging/android/ion/ion-ioctl.c
>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>> @@ -14,6 +14,7 @@ union ion_ioctl_arg {
>>       struct ion_allocation_data allocation;
>>       struct ion_heap_allocation_data heap_allocation;
>>       struct ion_heap_query query;
>> +    struct ion_heap_info heap_info;
>>       u32 version;
>>   };
>>   @@ -149,6 +150,17 @@ long ion_heap_ioctl(struct file *filp, unsigned
>> int cmd, unsigned long arg)
>>             break;
>>       }
>> +    case ION_IOC_HEAP_INFO:
>> +    {
>> +        struct miscdevice *miscdev = filp->private_data;
>> +        struct ion_heap *heap;
>> +
>> +        heap = container_of(miscdev, struct ion_heap, heap_dev);
>> +
>> +        data.heap_info.type = heap->type;
>> +
>> +        break;
>> +    }
>>       case ION_IOC_VERSION:
>>           data.version = ION_VERSION;
>>           break;
>> diff --git a/drivers/staging/android/uapi/ion.h
>> b/drivers/staging/android/uapi/ion.h
>> index 20db09f..1b3ca1e 100644
>> --- a/drivers/staging/android/uapi/ion.h
>> +++ b/drivers/staging/android/uapi/ion.h
>> @@ -111,6 +111,19 @@ struct ion_heap_data {
>>   };
>>     /**
>> + * struct ion_heap_info - Info about the heap
>> + *
>> + */
>> +struct ion_heap_info {
>> +    __u32 type;
>> +    __u32 reserved0;
>> +    __u32 reserved1;
>> +    __u32 reserved2;
>> +    __u32 reserved3;
>> +    __u32 reserved4;
>> +};
>> +
>> +/**
>>    * struct ion_heap_query - collection of data about all heaps
>>    * @cnt - total number of heaps to be copied
>>    * @heaps - buffer to copy heap data
>> @@ -159,4 +172,13 @@ struct ion_heap_query {
>>   #define ION_IOC_HEAP_ALLOC    _IOWR(ION_IOC_MAGIC, 10, \
>>                         struct ion_heap_allocation_data)
>>   +/**
>> + * DOC: ION_IOC_HEAP_INFO - allocate memory from heap
>> + *
>> + * Takes an ion_heap_query structure and populates information about
>> + * available Ion heaps.
>> + */
>> +#define ION_IOC_HEAP_INFO    _IOWR(ION_IOC_MAGIC, 11, \
>> +                      struct ion_heap_allocation_data)
>> +
>>   #endif /* _UAPI_LINUX_ION_H */
>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 3/4] ion: Add HEAP_INFO ioctl to be able to fetch heap type

[Laura Abbott]

On 2/19/19 1:39 PM, Andrew F. Davis wrote:
> On 2/19/19 3:13 PM, Laura Abbott wrote:
>> On 2/15/19 12:24 PM, John Stultz wrote:
>>> The per-device heaps don't support HEAP_QUERY ioctl, since
>>> the name is provided in the devnode path and the heapid isn't
>>> useful with the new interface (one uses the fd of heapdevice).
>>>
>>> But, one missing bit of functionality is a way to find the
>>> heap type. So provide a HEAP_INFO ioctl which exposes the
>>> heap type out so there is the potential for some sort of
>>> dynamic heap matching/discovery.
>>>
>>> Most likely this IOCTL will be useful when extended to allow
>>> some sort of opaque constraint bitfield to be shared so userland
>>> can match heaps with devices in a fully dynamic way.
>>>
>>
>> We've been waiting on the constraint solving for a while and
>> it's never really happened :(
>>
> 
> Most likely there will never be a one-size-fits-all solution here. So
> allowing for an extensible ABI that permits new information to be
> exported as needed will be important.
> 
>> It certainly works but I'm concerned about adding this and
>> then finding (yet again) that it doesn't work. We're
>> getting the heap name now but do we lose anything if we
>> don't expose it as part of the ABI?
>>
> 
> We can always add more ioctls, we cant go back and remove the old ones
> if we make them too clunky and have to remove something they expose. A
> simple starting ABI seems to make the most sense here. Even heap type
> doesn't look like a good thing to expose, it is just as static and
> one-off as heap name, I don't see it having all that much use :/
> 

That's my point though, why are we adding this ioctl now if we
don't have a good idea of its use case or why we want the heap
type exposed? If we come up with a good use later we can
add the ioctl then with better requirements.

> Andrew
> 
>>> Cc: Laura Abbott <labbott@redhat.com>
>>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>>> Cc: Liam Mark <lmark@codeaurora.org>
>>> Cc: Brian Starkey <Brian.Starkey@arm.com>
>>> Cc: Andrew F. Davis <afd@ti.com>
>>> Cc: Alistair Strachan <astrachan@google.com>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>> ---
>>>    drivers/staging/android/ion/ion-ioctl.c | 12 ++++++++++++
>>>    drivers/staging/android/uapi/ion.h      | 22 ++++++++++++++++++++++
>>>    2 files changed, 34 insertions(+)
>>>
>>> diff --git a/drivers/staging/android/ion/ion-ioctl.c
>>> b/drivers/staging/android/ion/ion-ioctl.c
>>> index ea8d263..6db5969 100644
>>> --- a/drivers/staging/android/ion/ion-ioctl.c
>>> +++ b/drivers/staging/android/ion/ion-ioctl.c
>>> @@ -14,6 +14,7 @@ union ion_ioctl_arg {
>>>        struct ion_allocation_data allocation;
>>>        struct ion_heap_allocation_data heap_allocation;
>>>        struct ion_heap_query query;
>>> +    struct ion_heap_info heap_info;
>>>        u32 version;
>>>    };
>>>    @@ -149,6 +150,17 @@ long ion_heap_ioctl(struct file *filp, unsigned
>>> int cmd, unsigned long arg)
>>>              break;
>>>        }
>>> +    case ION_IOC_HEAP_INFO:
>>> +    {
>>> +        struct miscdevice *miscdev = filp->private_data;
>>> +        struct ion_heap *heap;
>>> +
>>> +        heap = container_of(miscdev, struct ion_heap, heap_dev);
>>> +
>>> +        data.heap_info.type = heap->type;
>>> +
>>> +        break;
>>> +    }
>>>        case ION_IOC_VERSION:
>>>            data.version = ION_VERSION;
>>>            break;
>>> diff --git a/drivers/staging/android/uapi/ion.h
>>> b/drivers/staging/android/uapi/ion.h
>>> index 20db09f..1b3ca1e 100644
>>> --- a/drivers/staging/android/uapi/ion.h
>>> +++ b/drivers/staging/android/uapi/ion.h
>>> @@ -111,6 +111,19 @@ struct ion_heap_data {
>>>    };
>>>      /**
>>> + * struct ion_heap_info - Info about the heap
>>> + *
>>> + */
>>> +struct ion_heap_info {
>>> +    __u32 type;
>>> +    __u32 reserved0;
>>> +    __u32 reserved1;
>>> +    __u32 reserved2;
>>> +    __u32 reserved3;
>>> +    __u32 reserved4;
>>> +};
>>> +
>>> +/**
>>>     * struct ion_heap_query - collection of data about all heaps
>>>     * @cnt - total number of heaps to be copied
>>>     * @heaps - buffer to copy heap data
>>> @@ -159,4 +172,13 @@ struct ion_heap_query {
>>>    #define ION_IOC_HEAP_ALLOC    _IOWR(ION_IOC_MAGIC, 10, \
>>>                          struct ion_heap_allocation_data)
>>>    +/**
>>> + * DOC: ION_IOC_HEAP_INFO - allocate memory from heap
>>> + *
>>> + * Takes an ion_heap_query structure and populates information about
>>> + * available Ion heaps.
>>> + */
>>> +#define ION_IOC_HEAP_INFO    _IOWR(ION_IOC_MAGIC, 11, \
>>> +                      struct ion_heap_allocation_data)
>>> +
>>>    #endif /* _UAPI_LINUX_ION_H */
>>>
>>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 3/4] ion: Add HEAP_INFO ioctl to be able to fetch heap type

[John Stultz]

On Tue, Feb 19, 2019 at 1:13 PM Laura Abbott <labbott@redhat.com> wrote:
>
> On 2/15/19 12:24 PM, John Stultz wrote:
> > The per-device heaps don't support HEAP_QUERY ioctl, since
> > the name is provided in the devnode path and the heapid isn't
> > useful with the new interface (one uses the fd of heapdevice).
> >
> > But, one missing bit of functionality is a way to find the
> > heap type. So provide a HEAP_INFO ioctl which exposes the
> > heap type out so there is the potential for some sort of
> > dynamic heap matching/discovery.
> >
> > Most likely this IOCTL will be useful when extended to allow
> > some sort of opaque constraint bitfield to be shared so userland
> > can match heaps with devices in a fully dynamic way.
> >
>
> We've been waiting on the constraint solving for a while and
> it's never really happened :(
>

Yea. I'm not trying to open that up again.

> It certainly works but I'm concerned about adding this and
> then finding (yet again) that it doesn't work. We're
> getting the heap name now but do we lose anything if we
> don't expose it as part of the ABI?

Right. So all we're exporting in this patch is the heap_type. This was
somewhat of an afterthought for me, as practically, I suspect the
gralloc users of ion will know which heap they want by name, and won't
do any sort of dynamic heap finding.

That said, ion's current API provides the QUERY interface which gives
you a list of heap ids/names/types, so if you wanted something that on
any random system was able to find a ION_HEAP_TYPE_DMA heap and use
it, you could.

So this HEAP_INFO ioctl provides a way to do the same. That's it.

That said, I could envision the ioctl to be extended (via the reserved
values) to provide some sort of constraint cookie to allow for
constraint solving, but that's still a unsolved issue at large.

Given the handwaving at the constraints part, and that the heap type
is a pretty coarse grained enum (only 6 types, as of now - one being
catchall "custom"), I'm fine holding off on this bit unless folks
really see it as valuable.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 3/4] ion: Add HEAP_INFO ioctl to be able to fetch heap type

[John Stultz]

On Tue, Feb 19, 2019 at 1:46 PM Laura Abbott <labbott@redhat.com> wrote:
>
> On 2/19/19 1:39 PM, Andrew F. Davis wrote:
> > On 2/19/19 3:13 PM, Laura Abbott wrote:
> >> On 2/15/19 12:24 PM, John Stultz wrote:
> >>> The per-device heaps don't support HEAP_QUERY ioctl, since
> >>> the name is provided in the devnode path and the heapid isn't
> >>> useful with the new interface (one uses the fd of heapdevice).
> >>>
> >>> But, one missing bit of functionality is a way to find the
> >>> heap type. So provide a HEAP_INFO ioctl which exposes the
> >>> heap type out so there is the potential for some sort of
> >>> dynamic heap matching/discovery.
> >>>
> >>> Most likely this IOCTL will be useful when extended to allow
> >>> some sort of opaque constraint bitfield to be shared so userland
> >>> can match heaps with devices in a fully dynamic way.
> >>>
> >>
> >> We've been waiting on the constraint solving for a while and
> >> it's never really happened :(
> >>
> >
> > Most likely there will never be a one-size-fits-all solution here. So
> > allowing for an extensible ABI that permits new information to be
> > exported as needed will be important.
> >
> >> It certainly works but I'm concerned about adding this and
> >> then finding (yet again) that it doesn't work. We're
> >> getting the heap name now but do we lose anything if we
> >> don't expose it as part of the ABI?
> >>
> >
> > We can always add more ioctls, we cant go back and remove the old ones
> > if we make them too clunky and have to remove something they expose. A
> > simple starting ABI seems to make the most sense here. Even heap type
> > doesn't look like a good thing to expose, it is just as static and
> > one-off as heap name, I don't see it having all that much use :/
> >
>
> That's my point though, why are we adding this ioctl now if we
> don't have a good idea of its use case or why we want the heap
> type exposed? If we come up with a good use later we can
> add the ioctl then with better requirements.

Ok. I think we three are in agreement here.  Best to drop this bit and
leave it till someone has a clear need/use.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 0/4] ION per heap devices

[Laura Abbott]

On 2/19/19 1:30 PM, Andrew F. Davis wrote:
> On 2/19/19 3:25 PM, Laura Abbott wrote:
>> On 2/15/19 12:24 PM, John Stultz wrote:
>>> This is a *very early RFC* (it builds, that's all I'll say :)
>>> but I wanted to share it to get some initial feedback before I
>>> go down the rabit hole of trying to adapt the Android userland
>>> code to get this fully validated.
>>>
>>> This patchset tries to implement the per-heap devices for ION.
>>> The main benefit is that it avoids multiplexing heap operations
>>> through the /dev/ion interface, and allows for each heap to have
>>> its own permissions/sepolicy rules.
>>>
>>> Feedback would be greatly appreciated!
>>> thanks
>>> -john
>>>
>>> Cc: Laura Abbott <labbott@redhat.com>
>>> Cc: Sumit Semwal <sumit.semwal@linaro.org>
>>> Cc: Liam Mark <lmark@codeaurora.org>
>>> Cc: Brian Starkey <Brian.Starkey@arm.com>
>>> Cc: Andrew F. Davis <afd@ti.com>
>>> Cc: Alistair Strachan <astrachan@google.com>
>>> Cc: dri-devel@lists.freedesktop.org
>>>
>>> John Stultz (4):
>>>     ion: Add ION_VERSION ioctl
>>>     ion: Initial hack to create per heap devices
>>>     ion: Add HEAP_INFO ioctl to be able to fetch heap type
>>>     ion: Make "legacy" /dev/ion support optional
>>>
>>>    drivers/staging/android/ion/Kconfig     |  7 +++
>>>    drivers/staging/android/ion/ion-ioctl.c | 80
>>> +++++++++++++++++++++++++++++++++
>>>    drivers/staging/android/ion/ion.c       | 51 ++++++++++++++++-----
>>>    drivers/staging/android/ion/ion.h       |  6 +++
>>>    drivers/staging/android/uapi/ion.h      | 57 +++++++++++++++++++++++
>>>    5 files changed, 191 insertions(+), 10 deletions(-)
>>>
>>
>> So it occurs to me if this is going to be a new ABI
>> all together maybe we should just declare a new allocation ioctl
>> to be used with it. We can keep the old ioctls around
>> for legacy use cases and maybe eventually delete them
>> and just use the new allocation ioctl with the new
>> split heaps.
>>
> 
> Why keep the old ones, this is staging, there are no legacy users (that
> matter to kernel).. Slowing progress for the sake of backwards compat
> with staging just slows the de-staging down.
> 

I think we just fundamentally disagree here. I don't see keeping
legacy users as slowing anything down. We're still getting
the new ABI that we actually like and we get the chance to easily
go back and test. Having a non broken ABI makes it much
easier to do testing and validation and comparison. We can remove
the last ABI before we move it out of staging.

Thanks,
Laura

> Andrew
> 
>> Thanks,
>> Laura

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 0/4] ION per heap devices

[John Stultz]

On Tue, Feb 19, 2019 at 1:25 PM Laura Abbott <labbott@redhat.com> wrote:
>
> On 2/15/19 12:24 PM, John Stultz wrote:
> > This is a *very early RFC* (it builds, that's all I'll say :)
> > but I wanted to share it to get some initial feedback before I
> > go down the rabit hole of trying to adapt the Android userland
> > code to get this fully validated.
> >
> > This patchset tries to implement the per-heap devices for ION.
> > The main benefit is that it avoids multiplexing heap operations
> > through the /dev/ion interface, and allows for each heap to have
> > its own permissions/sepolicy rules.
> >
> > Feedback would be greatly appreciated!
> > thanks
> > -john
> >
> > Cc: Laura Abbott <labbott@redhat.com>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: Liam Mark <lmark@codeaurora.org>
> > Cc: Brian Starkey <Brian.Starkey@arm.com>
> > Cc: Andrew F. Davis <afd@ti.com>
> > Cc: Alistair Strachan <astrachan@google.com>
> > Cc: dri-devel@lists.freedesktop.org
> >
> > John Stultz (4):
> >    ion: Add ION_VERSION ioctl
> >    ion: Initial hack to create per heap devices
> >    ion: Add HEAP_INFO ioctl to be able to fetch heap type
> >    ion: Make "legacy" /dev/ion support optional
> >
> >   drivers/staging/android/ion/Kconfig     |  7 +++
> >   drivers/staging/android/ion/ion-ioctl.c | 80 +++++++++++++++++++++++++++++++++
> >   drivers/staging/android/ion/ion.c       | 51 ++++++++++++++++-----
> >   drivers/staging/android/ion/ion.h       |  6 +++
> >   drivers/staging/android/uapi/ion.h      | 57 +++++++++++++++++++++++
> >   5 files changed, 191 insertions(+), 10 deletions(-)
> >
>
> So it occurs to me if this is going to be a new ABI
> all together maybe we should just declare a new allocation ioctl
> to be used with it. We can keep the old ioctls around
> for legacy use cases and maybe eventually delete them
> and just use the new allocation ioctl with the new
> split heaps.

So... I did add ION_IOC_HEAP_ALLOC in my patchset.  Or are you
suggesting we use a new ION_IOC_MAGIC value for the new ABI? Or some
larger rework?

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 3/4] ion: Add HEAP_INFO ioctl to be able to fetch heap type

[Brian Starkey]

On Tue, Feb 19, 2019 at 01:47:36PM -0800, John Stultz wrote:
> On Tue, Feb 19, 2019 at 1:13 PM Laura Abbott <labbott@redhat.com> wrote:
> >
> > On 2/15/19 12:24 PM, John Stultz wrote:
> > > The per-device heaps don't support HEAP_QUERY ioctl, since
> > > the name is provided in the devnode path and the heapid isn't
> > > useful with the new interface (one uses the fd of heapdevice).
> > >
> > > But, one missing bit of functionality is a way to find the
> > > heap type. So provide a HEAP_INFO ioctl which exposes the
> > > heap type out so there is the potential for some sort of
> > > dynamic heap matching/discovery.
> > >
> > > Most likely this IOCTL will be useful when extended to allow
> > > some sort of opaque constraint bitfield to be shared so userland
> > > can match heaps with devices in a fully dynamic way.
> > >
> >
> > We've been waiting on the constraint solving for a while and
> > it's never really happened :(
> >
> 
> Yea. I'm not trying to open that up again.
> 
> > It certainly works but I'm concerned about adding this and
> > then finding (yet again) that it doesn't work. We're
> > getting the heap name now but do we lose anything if we
> > don't expose it as part of the ABI?
> 
> Right. So all we're exporting in this patch is the heap_type. This was
> somewhat of an afterthought for me, as practically, I suspect the
> gralloc users of ion will know which heap they want by name, and won't
> do any sort of dynamic heap finding.
> 
> That said, ion's current API provides the QUERY interface which gives
> you a list of heap ids/names/types, so if you wanted something that on
> any random system was able to find a ION_HEAP_TYPE_DMA heap and use
> it, you could.
> 
> So this HEAP_INFO ioctl provides a way to do the same. That's it.
> 
> That said, I could envision the ioctl to be extended (via the reserved
> values) to provide some sort of constraint cookie to allow for
> constraint solving, but that's still a unsolved issue at large.
> 
> Given the handwaving at the constraints part, and that the heap type
> is a pretty coarse grained enum (only 6 types, as of now - one being
> catchall "custom"), I'm fine holding off on this bit unless folks
> really see it as valuable.

What do people think about putting the heap type into the device name?
The whole query API is then replaced with `ls`

-Brian

> 
> thanks
> -john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [EARLY RFC][PATCH 3/4] ion: Add HEAP_INFO ioctl to be able to fetch heap type

[John Stultz]

On Wed, Feb 20, 2019 at 7:07 AM Brian Starkey <Brian.Starkey@arm.com> wrote:
>
> On Tue, Feb 19, 2019 at 01:47:36PM -0800, John Stultz wrote:
> > On Tue, Feb 19, 2019 at 1:13 PM Laura Abbott <labbott@redhat.com> wrote:
> > >
> > > On 2/15/19 12:24 PM, John Stultz wrote:
> > > > The per-device heaps don't support HEAP_QUERY ioctl, since
> > > > the name is provided in the devnode path and the heapid isn't
> > > > useful with the new interface (one uses the fd of heapdevice).
> > > >
> > > > But, one missing bit of functionality is a way to find the
> > > > heap type. So provide a HEAP_INFO ioctl which exposes the
> > > > heap type out so there is the potential for some sort of
> > > > dynamic heap matching/discovery.
> > > >
> > > > Most likely this IOCTL will be useful when extended to allow
> > > > some sort of opaque constraint bitfield to be shared so userland
> > > > can match heaps with devices in a fully dynamic way.
> > > >
> > >
> > > We've been waiting on the constraint solving for a while and
> > > it's never really happened :(
> > >
> >
> > Yea. I'm not trying to open that up again.
> >
> > > It certainly works but I'm concerned about adding this and
> > > then finding (yet again) that it doesn't work. We're
> > > getting the heap name now but do we lose anything if we
> > > don't expose it as part of the ABI?
> >
> > Right. So all we're exporting in this patch is the heap_type. This was
> > somewhat of an afterthought for me, as practically, I suspect the
> > gralloc users of ion will know which heap they want by name, and won't
> > do any sort of dynamic heap finding.
> >
> > That said, ion's current API provides the QUERY interface which gives
> > you a list of heap ids/names/types, so if you wanted something that on
> > any random system was able to find a ION_HEAP_TYPE_DMA heap and use
> > it, you could.
> >
> > So this HEAP_INFO ioctl provides a way to do the same. That's it.
> >
> > That said, I could envision the ioctl to be extended (via the reserved
> > values) to provide some sort of constraint cookie to allow for
> > constraint solving, but that's still a unsolved issue at large.
> >
> > Given the handwaving at the constraints part, and that the heap type
> > is a pretty coarse grained enum (only 6 types, as of now - one being
> > catchall "custom"), I'm fine holding off on this bit unless folks
> > really see it as valuable.
>
> What do people think about putting the heap type into the device name?
> The whole query API is then replaced with `ls`

Hrm. I'm a bit hesitant on that, as that enshrines a naming convention as abi.

Also I personally don't think the heap-types are particularly well
defined. The memory itself is either contiguous or not, 32bit dma
accessible or not, etc. -  the specific implementation "type"
[system-contig, carveout, chunk, cma] isn't as useful from userland
other then conventions of how the heaps may be partitioned between
users.

So I'm probably happier to hide that detail.  If someone wants to do
dynamic heap discovery, we probably need a proper constraint solution.

For now, a potential generic gralloc would just have to have some
device specific policy file that defines which heap device to use for
which type of allocation it wants. Not completely unlike an fstab.

thanks
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel