| Message ID | 1550262252-15558-3-git-send-email-john.stultz@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | ION per heap devices | expand |
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 */ >
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
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 */
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(+) -- 2.7.4