- Linaro-mm-sig - lists.linaro.org

[PATCH] dma-fence: Don't BUG_ON when not absolutely needed

by Daniel Vetter

It makes debugging a massive pain. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Gustavo Padovan <gustavo(a)padovan.org> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/dma-buf/dma-fence.c | 4 ++-- include/linux/dma-fence.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 56e0a0e1b600..9a302799040e 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -48,7 +48,7 @@ static atomic64_t dma_fence_context_counter = ATOMIC64_INIT(0); */ u64 dma_fence_context_alloc(unsigned num) { - BUG_ON(!num); + WARN_ON(!num); return atomic64_add_return(num, &dma_fence_context_counter) - num; } EXPORT_SYMBOL(dma_fence_context_alloc); @@ -172,7 +172,7 @@ void dma_fence_release(struct kref *kref) trace_dma_fence_destroy(fence); - BUG_ON(!list_empty(&fence->cb_list)); + WARN_ON(!list_empty(&fence->cb_list)); if (fence->ops->release) fence->ops->release(fence); diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 9342cf0dada4..171895072435 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -431,8 +431,8 @@ int dma_fence_get_status(struct dma_fence *fence); static inline void dma_fence_set_error(struct dma_fence *fence, int error) { - BUG_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)); - BUG_ON(error >= 0 || error < -MAX_ERRNO); + WARN_ON(test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)); + WARN_ON(error >= 0 || error < -MAX_ERRNO); fence->error = error; } -- 2.13.2

8 years, 4 months

2
1
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: avoid scheduling on fence status query v2

by Gustavo Padovan

2017-04-26 Christian König <deathsimple(a)vodafone.de>: > Am 26.04.2017 um 16:46 schrieb Andres Rodriguez: > > When a timeout of zero is specified, the caller is only interested in > > the fence status. > > > > In the current implementation, dma_fence_default_wait will always call > > schedule_timeout() at least once for an unsignaled fence. This adds a > > significant overhead to a fence status query. > > > > Avoid this overhead by returning early if a zero timeout is specified. > > > > v2: move early return after enable_signaling > > > > Signed-off-by: Andres Rodriguez <andresx7(a)gmail.com> > > Reviewed-by: Christian König <christian.koenig(a)amd.com> pushed to drm-misc-next. Thanks all. Gustavo

8 years, 6 months

3
2
0 0

[PATCH 0/4] DMA-buf: Fine-tuning for four function implementations

by SF Markus Elfring

From: Markus Elfring <elfring(a)users.sourceforge.net> Date: Mon, 8 May 2017 11:05:05 +0200 A few update suggestions were taken into account from static source code analysis. Markus Elfring (4): Combine two function calls into one in dma_buf_debug_show() Improve a size determination in dma_buf_attach() Adjust a null pointer check in dma_buf_attach() Use seq_putc() in two functions drivers/dma-buf/dma-buf.c | 8 +++----- drivers/dma-buf/sync_debug.c | 6 +++--- 2 files changed, 6 insertions(+), 8 deletions(-) -- 2.12.2

8 years, 7 months

3
10
0 0

[PATCH] dma-buf: avoid scheduling on fence status query v2

by Andres Rodriguez

When a timeout of zero is specified, the caller is only interested in the fence status. In the current implementation, dma_fence_default_wait will always call schedule_timeout() at least once for an unsignaled fence. This adds a significant overhead to a fence status query. Avoid this overhead by returning early if a zero timeout is specified. v2: move early return after enable_signaling Signed-off-by: Andres Rodriguez <andresx7(a)gmail.com> --- If I'm understanding correctly, I don't think we need to register the default wait callback. But if that isn't the case please let me know. This patch has the same perf improvements as v1. drivers/dma-buf/dma-fence.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 0918d3f..57da14c 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -402,6 +402,11 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) } } + if (!timeout) { + ret = 0; + goto out; + } + cb.base.func = dma_fence_default_wait_cb; cb.task = current; list_add(&cb.base.node, &fence->cb_list); -- 2.9.3

8 years, 7 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: avoid scheduling on fence status query

by Andres Rodriguez

On 2017-04-26 06:13 AM, Christian König wrote: > Am 26.04.2017 um 11:59 schrieb Dave Airlie: >> On 26 April 2017 at 17:20, Christian König <deathsimple(a)vodafone.de> >> wrote: >>> NAK, I'm wondering how often I have to reject that change. We should >>> probably add a comment here. >>> >>> Even with a zero timeout we still need to enable signaling, otherwise >>> some >>> fence will never signal if userspace just polls on them. >>> >>> If a caller is only interested in the fence status without enabling the >>> signaling it should call dma_fence_is_signaled() instead. >> Can we not move the return 0 (with spin unlock) down after we enabling >> signalling, but before >> we enter the schedule_timeout(1)? > > Yes, that would be an option. > I was actually arguing with Dave about this on IRC yesterday. Seems like I owe him a beer now. -Andres > Christian. > >> >> Dave. > >

8 years, 7 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: avoid scheduling on fence status query

by Dave Airlie

On 26 April 2017 at 17:20, Christian König <deathsimple(a)vodafone.de> wrote: > NAK, I'm wondering how often I have to reject that change. We should > probably add a comment here. > > Even with a zero timeout we still need to enable signaling, otherwise some > fence will never signal if userspace just polls on them. > > If a caller is only interested in the fence status without enabling the > signaling it should call dma_fence_is_signaled() instead. Can we not move the return 0 (with spin unlock) down after we enabling signalling, but before we enter the schedule_timeout(1)? Dave.

8 years, 7 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: avoid scheduling on fence status query

by Andres Rodriguez

CC a few extra lists I missed. Regards, Andres On 2017-04-25 09:36 PM, Andres Rodriguez wrote: > When a timeout of zero is specified, the caller is only interested in > the fence status. > > In the current implementation, dma_fence_default_wait will always call > schedule_timeout() at least once for an unsignaled fence. This adds a > significant overhead to a fence status query. > > Avoid this overhead by returning early if a zero timeout is specified. > > Signed-off-by: Andres Rodriguez <andresx7(a)gmail.com> > --- > > This heavily affects the performance of the Source2 engine running on > radv. > > This patch improves dota2(radv) perf on a i7-6700k+RX480 system from > 72fps->81fps. > > drivers/dma-buf/dma-fence.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 0918d3f..348e9e2 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -380,6 +380,9 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) > if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > return ret; > > + if (!timeout) > + return 0; > + > spin_lock_irqsave(fence->lock, flags); > > if (intr && signal_pending(current)) { >

8 years, 7 months

1
0
0 0

[PATCHv4 00/12] Ion cleanup in preparation for moving out of staging

by Laura Abbott

Hi, This is v4 of the series to cleanup to Ion. Greg took some of the patches that weren't CMA related already. There was a minor bisectability problem with the CMA APIs so this is a new version to address that. I also addressed some minor comments on the patch to collapse header files. Thanks, Laura Laura Abbott (12): cma: Store a name in the cma structure cma: Introduce cma_for_each_area staging: android: ion: Use CMA APIs directly staging: android: ion: Stop butchering the DMA address staging: android: ion: Break the ABI in the name of forward progress staging: android: ion: Get rid of ion_phys_addr_t staging: android: ion: Collapse internal header files staging: android: ion: Rework heap registration/enumeration staging: android: ion: Drop ion_map_kernel interface staging: android: ion: Remove ion_handle and ion_client staging: android: ion: Set query return value staging/android: Update Ion TODO list arch/powerpc/kvm/book3s_hv_builtin.c | 3 +- drivers/base/dma-contiguous.c | 5 +- drivers/staging/android/TODO | 21 +- drivers/staging/android/ion/Kconfig | 32 + drivers/staging/android/ion/Makefile | 11 +- drivers/staging/android/ion/compat_ion.c | 152 ----- drivers/staging/android/ion/compat_ion.h | 29 - drivers/staging/android/ion/ion-ioctl.c | 55 +- drivers/staging/android/ion/ion.c | 812 ++---------------------- drivers/staging/android/ion/ion.h | 386 ++++++++--- drivers/staging/android/ion/ion_carveout_heap.c | 21 +- drivers/staging/android/ion/ion_chunk_heap.c | 16 +- drivers/staging/android/ion/ion_cma_heap.c | 120 ++-- drivers/staging/android/ion/ion_heap.c | 68 -- drivers/staging/android/ion/ion_page_pool.c | 3 +- drivers/staging/android/ion/ion_priv.h | 453 ------------- drivers/staging/android/ion/ion_system_heap.c | 39 +- drivers/staging/android/uapi/ion.h | 36 +- include/linux/cma.h | 6 +- mm/cma.c | 31 +- mm/cma.h | 1 + mm/cma_debug.c | 2 +- 22 files changed, 524 insertions(+), 1778 deletions(-) delete mode 100644 drivers/staging/android/ion/compat_ion.c delete mode 100644 drivers/staging/android/ion/compat_ion.h delete mode 100644 drivers/staging/android/ion/ion_priv.h -- 2.7.4

8 years, 7 months

4
17
0 0

[PATCHv3 00/22] Ion clean up in preparation in moving out of staging

by Laura Abbott

Hi, This is v3 of the series to do some serious Ion cleanup in preparation for moving out of staging. I didn't hear much on v2 so I'm going to assume people are okay with the series as is. I know there were still some open questions about moving away from /dev/ion but in the interest of small steps I'd like to go ahead and merge this series assuming there are no more major objections. More work can happen on top of this. Changes from v2: - Dropped the RFC tag - Minor bisectability fixes - Sumit's comment about CMA naming - Updated the TODO list Thanks, Laura Laura Abbott (22): cma: Store a name in the cma structure cma: Introduce cma_for_each_area staging: android: ion: Remove dmap_cnt staging: android: ion: Remove alignment from allocation field staging: android: ion: Duplicate sg_table staging: android: ion: Call dma_map_sg for syncing and mapping staging: android: ion: Remove page faulting support staging: android: ion: Remove crufty cache support staging: android: ion: Remove custom ioctl interface staging: android: ion: Remove import interface staging: android: ion: Remove duplicate ION_IOC_MAP staging: android: ion: Remove old platform support staging: android: ion: Use CMA APIs directly staging: android: ion: Stop butchering the DMA address staging: android: ion: Break the ABI in the name of forward progress staging: android: ion: Get rid of ion_phys_addr_t staging: android: ion: Collapse internal header files staging: android: ion: Rework heap registration/enumeration staging: android: ion: Drop ion_map_kernel interface staging: android: ion: Remove ion_handle and ion_client staging: android: ion: Set query return value staging/android: Update Ion TODO list arch/powerpc/kvm/book3s_hv_builtin.c | 3 +- drivers/base/dma-contiguous.c | 5 +- drivers/staging/android/TODO | 21 +- drivers/staging/android/ion/Kconfig | 56 +- drivers/staging/android/ion/Makefile | 18 +- drivers/staging/android/ion/compat_ion.c | 195 ---- drivers/staging/android/ion/compat_ion.h | 29 - drivers/staging/android/ion/hisilicon/Kconfig | 5 - drivers/staging/android/ion/hisilicon/Makefile | 1 - drivers/staging/android/ion/hisilicon/hi6220_ion.c | 113 -- drivers/staging/android/ion/ion-ioctl.c | 85 +- drivers/staging/android/ion/ion.c | 1168 +++----------------- drivers/staging/android/ion/ion.h | 389 +++++-- drivers/staging/android/ion/ion_carveout_heap.c | 37 +- drivers/staging/android/ion/ion_chunk_heap.c | 27 +- drivers/staging/android/ion/ion_cma_heap.c | 125 +-- drivers/staging/android/ion/ion_dummy_driver.c | 155 --- drivers/staging/android/ion/ion_heap.c | 68 -- drivers/staging/android/ion/ion_of.c | 184 --- drivers/staging/android/ion/ion_of.h | 37 - drivers/staging/android/ion/ion_page_pool.c | 6 +- drivers/staging/android/ion/ion_priv.h | 473 -------- drivers/staging/android/ion/ion_system_heap.c | 53 +- drivers/staging/android/ion/ion_test.c | 305 ----- drivers/staging/android/ion/tegra/Makefile | 1 - drivers/staging/android/ion/tegra/tegra_ion.c | 80 -- drivers/staging/android/uapi/ion.h | 86 +- drivers/staging/android/uapi/ion_test.h | 69 -- include/linux/cma.h | 6 +- mm/cma.c | 31 +- mm/cma.h | 1 + mm/cma_debug.c | 2 +- 32 files changed, 620 insertions(+), 3214 deletions(-) delete mode 100644 drivers/staging/android/ion/compat_ion.c delete mode 100644 drivers/staging/android/ion/compat_ion.h delete mode 100644 drivers/staging/android/ion/hisilicon/Kconfig delete mode 100644 drivers/staging/android/ion/hisilicon/Makefile delete mode 100644 drivers/staging/android/ion/hisilicon/hi6220_ion.c delete mode 100644 drivers/staging/android/ion/ion_dummy_driver.c delete mode 100644 drivers/staging/android/ion/ion_of.c delete mode 100644 drivers/staging/android/ion/ion_of.h delete mode 100644 drivers/staging/android/ion/ion_priv.h delete mode 100644 drivers/staging/android/ion/ion_test.c delete mode 100644 drivers/staging/android/ion/tegra/Makefile delete mode 100644 drivers/staging/android/ion/tegra/tegra_ion.c delete mode 100644 drivers/staging/android/uapi/ion_test.h -- 2.7.4

8 years, 8 months

4
31
0 0

Re: [Linaro-mm-sig] [PATCHv3 17/22] staging: android: ion: Collapse internal header files

by Laura Abbott

On 04/08/2017 11:12 AM, Emil Velikov wrote: > Hi Laura, > > Couple of trivial nitpicks below. > > On 3 April 2017 at 19:57, Laura Abbott <labbott(a)redhat.com> wrote: > >> --- a/drivers/staging/android/ion/ion.h >> +++ b/drivers/staging/android/ion/ion.h >> @@ -1,5 +1,5 @@ >> /* >> - * drivers/staging/android/ion/ion.h >> + * drivers/staging/android/ion/ion_priv.h > Does not match the actual filename. > >> * >> * Copyright (C) 2011 Google, Inc. >> * >> @@ -14,24 +14,26 @@ >> * >> */ >> >> -#ifndef _LINUX_ION_H >> -#define _LINUX_ION_H >> +#ifndef _ION_PRIV_H >> +#define _ION_PRIV_H >> > Ditto. > >> +#include <linux/device.h> >> +#include <linux/dma-direction.h> >> +#include <linux/kref.h> >> +#include <linux/mm_types.h> >> +#include <linux/mutex.h> >> +#include <linux/rbtree.h> >> +#include <linux/sched.h> >> +#include <linux/shrinker.h> >> #include <linux/types.h> >> +#include <linux/miscdevice.h> >> >> #include "../uapi/ion.h" >> > You don't want to use "../" in includes. Perhaps address with another > patch, if you haven't already ? > There isn't a better option until this driver moves out of staging. Once it moves out it can be fixed up. Thanks, Laura > Regards, > Emil >

8 years, 8 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: align debugfs output

by Sumit Semwal

Hi Russell, On 31 March 2017 at 15:33, Russell King <rmk+kernel(a)armlinux.org.uk> wrote: > Align the heading with the values output from debugfs. > > Signed-off-by: Russell King <rmk+kernel(a)armlinux.org.uk> Thanks for your patch; applied to drm-misc-next. > --- > drivers/dma-buf/dma-buf.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index ebaf1923ad6b..f72aaacbe023 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1072,7 +1072,8 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > return ret; > > seq_puts(s, "\nDma-buf Objects:\n"); > - seq_puts(s, "size\tflags\tmode\tcount\texp_name\n"); > + seq_printf(s, "%-8s\t%-8s\t%-8s\t%-8s\texp_name\n", > + "size", "flags", "mode", "count"); > > list_for_each_entry(buf_obj, &db_list.head, list_node) { > ret = mutex_lock_interruptible(&buf_obj->lock); > -- > 2.7.4 > Best, Sumit.

8 years, 8 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: fence debugging

by Gustavo Padovan

Hi Russell, 2017-03-31 Russell King <rmk+kernel(a)arm.linux.org.uk>: > Add debugfs output to report shared and exclusive fences on a dma_buf > object. This produces output such as: > > Dma-buf Objects: > size flags mode count exp_name > 08294400 00000000 00000005 00000005 drm > Exclusive fence: etnaviv 134000.gpu signalled > Attached Devices: > gpu-subsystem > Total 1 devices attached > > > Total 1 objects, 8294400 bytes > > > Signed-off-by: Russell King <rmk+kernel(a)arm.linux.org.uk> > --- > drivers/dma-buf/dma-buf.c | 34 +++++++++++++++++++++++++++++++++- > 1 file changed, 33 insertions(+), 1 deletions(-) Applied to to drm-misc-next. > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index 0007b792827b..f72aaacbe023 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1059,7 +1059,11 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) > int ret; > struct dma_buf *buf_obj; > struct dma_buf_attachment *attach_obj; > - int count = 0, attach_count; > + struct reservation_object *robj; > + struct reservation_object_list *fobj; > + struct dma_fence *fence; > + unsigned seq; Our maintainer tools warned about this line, so I made it "unsigned int" Gustavo

8 years, 8 months

1
0
0 0

[RFC PATCHv2 00/21] Ion clean in preparation for moving out of staging

by Laura Abbott

Hi, This is v2 of the series to do some serious Ion clean up in preparation for moving out of staging. I got good feedback last time so this series mostly attempts to address that feedback and do more still more cleanup. Highlights: - All calls to DMA APIs should now be with a real actual proper device structure - Patch to stop setting sg_dma_address manually now included - Fix for a bug in the query interface - Removal of custom ioctl interface - Removal of import interface - Removal of any notion of using Ion as an in kernel interface. - Cleanup of ABI so compat interface is no longer needed - Deletion of a bit more platform code - Combined heap enumeration and heap registration code up so there are fewer layers of abstraction - Some general cleanup and header reduction. - Removal of both the ion_client and ion_handle structures since these mostly become redundant. As a result, Ion only returns a dma_buf fd. The overall result is that the only Ion interfaces are the query ioctl and the alloc ioctl. The following are still TODOs/open problems: - Sumit's comments about the CMA naming. - Bindings/platform for chunk and carveout heap - There was some discussion about making the sg_table duplication generic. I got bogged down in handling some of the edge cases for generic handling so I put this aside. Making it generic is still something that should happen. - More fine-grained support for restricting heap access. There are good arguments to be made for having a way for having good integration with selinux and other policy mechanisms. - While not on the original list, there is still no good good test standalone test framework. I noticed that the existing ion_test was fairly generic so I proposed moving it to dma_buf. Daniel Vetter suggested just using the VGEM module instead. Ideally, the tests can live as part of some other existing test set (drm tests maybe?) Feedback appreciated as always. Thanks, Laura Laura Abbott (21): cma: Store a name in the cma structure cma: Introduce cma_for_each_area staging: android: ion: Remove dmap_cnt staging: android: ion: Remove alignment from allocation field staging: android: ion: Duplicate sg_table staging: android: ion: Call dma_map_sg for syncing and mapping staging: android: ion: Remove page faulting support staging: android: ion: Remove crufty cache support staging: android: ion: Remove custom ioctl interface staging: android: ion: Remove import interface staging: android: ion: Remove duplicate ION_IOC_MAP staging: android: ion: Remove old platform support staging: android: ion: Use CMA APIs directly staging: android: ion: Stop butchering the DMA address staging: android: ion: Break the ABI in the name of forward progress staging: android: ion: Get rid of ion_phys_addr_t staging: android: ion: Collapse internal header files staging: android: ion: Rework heap registration/enumeration staging: android: ion: Drop ion_map_kernel interface staging: android: ion: Remove ion_handle and ion_client staging: android: ion: Set query return value drivers/base/dma-contiguous.c | 5 +- drivers/staging/android/ion/Kconfig | 56 +- drivers/staging/android/ion/Makefile | 18 +- drivers/staging/android/ion/compat_ion.c | 195 ---- drivers/staging/android/ion/compat_ion.h | 29 - drivers/staging/android/ion/hisilicon/Kconfig | 5 - drivers/staging/android/ion/hisilicon/Makefile | 1 - drivers/staging/android/ion/hisilicon/hi6220_ion.c | 113 -- drivers/staging/android/ion/ion-ioctl.c | 85 +- drivers/staging/android/ion/ion.c | 1164 +++----------------- drivers/staging/android/ion/ion.h | 393 +++++-- drivers/staging/android/ion/ion_carveout_heap.c | 37 +- drivers/staging/android/ion/ion_chunk_heap.c | 27 +- drivers/staging/android/ion/ion_cma_heap.c | 125 +-- drivers/staging/android/ion/ion_dummy_driver.c | 156 --- drivers/staging/android/ion/ion_heap.c | 68 -- drivers/staging/android/ion/ion_of.c | 184 ---- drivers/staging/android/ion/ion_of.h | 37 - drivers/staging/android/ion/ion_page_pool.c | 6 +- drivers/staging/android/ion/ion_priv.h | 473 -------- drivers/staging/android/ion/ion_system_heap.c | 53 +- drivers/staging/android/ion/ion_test.c | 305 ----- drivers/staging/android/ion/tegra/Makefile | 1 - drivers/staging/android/ion/tegra/tegra_ion.c | 80 -- drivers/staging/android/uapi/ion.h | 86 +- drivers/staging/android/uapi/ion_test.h | 69 -- include/linux/cma.h | 6 +- mm/cma.c | 25 +- mm/cma.h | 1 + mm/cma_debug.c | 2 +- 30 files changed, 610 insertions(+), 3195 deletions(-) delete mode 100644 drivers/staging/android/ion/compat_ion.c delete mode 100644 drivers/staging/android/ion/compat_ion.h delete mode 100644 drivers/staging/android/ion/hisilicon/Kconfig delete mode 100644 drivers/staging/android/ion/hisilicon/Makefile delete mode 100644 drivers/staging/android/ion/hisilicon/hi6220_ion.c delete mode 100644 drivers/staging/android/ion/ion_dummy_driver.c delete mode 100644 drivers/staging/android/ion/ion_of.c delete mode 100644 drivers/staging/android/ion/ion_of.h delete mode 100644 drivers/staging/android/ion/ion_priv.h delete mode 100644 drivers/staging/android/ion/ion_test.c delete mode 100644 drivers/staging/android/ion/tegra/Makefile delete mode 100644 drivers/staging/android/ion/tegra/tegra_ion.c delete mode 100644 drivers/staging/android/uapi/ion_test.h -- 2.7.4

8 years, 8 months

2
23
0 0

[RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

by Laura Abbott

Hi, There's been some recent discussions[1] about Ion-like frameworks. There's apparently interest in just keeping Ion since it works reasonablly well. This series does what should be the final clean ups for it to possibly be moved out of staging. This includes the following: - Some general clean up and removal of features that never got a lot of use as far as I can tell. - Fixing up the caching. This is the series I proposed back in December[2] but never heard any feedback on. It will certainly break existing applications that rely on the implicit caching. I'd rather make an effort to move to a model that isn't going directly against the establishement though. - Fixing up the platform support. The devicetree approach was never well recieved by DT maintainers. The proposal here is to think of Ion less as specifying requirements and more of a framework for exposing memory to userspace. - CMA allocations now happen without the need of a dummy device structure. This fixes a bunch of the reasons why I attempted to add devicetree support before. I've had problems getting feedback in the past so if I don't hear any major objections I'm going to send out with the RFC dropped to be picked up. The only reason there isn't a patch to come out of staging is to discuss any other changes to the ABI people might want. Once this comes out of staging, I really don't want to mess with the ABI. Feedback appreciated. Thanks, Laura [1] https://marc.info/?l=linux-kernel&m=148699712602105&w=2 [2] https://marc.info/?l=linaro-mm-sig&m=148176050802908&w=2 Laura Abbott (12): staging: android: ion: Remove dmap_cnt staging: android: ion: Remove alignment from allocation field staging: android: ion: Duplicate sg_table staging: android: ion: Call dma_map_sg for syncing and mapping staging: android: ion: Remove page faulting support staging: android: ion: Remove crufty cache support staging: android: ion: Remove old platform support cma: Store a name in the cma structure cma: Introduce cma_for_each_area staging: android: ion: Use CMA APIs directly staging: android: ion: Make Ion heaps selectable staging; android: ion: Enumerate all available heaps drivers/base/dma-contiguous.c | 5 +- drivers/staging/android/ion/Kconfig | 51 ++-- drivers/staging/android/ion/Makefile | 14 +- drivers/staging/android/ion/hisilicon/Kconfig | 5 - drivers/staging/android/ion/hisilicon/Makefile | 1 - drivers/staging/android/ion/hisilicon/hi6220_ion.c | 113 --------- drivers/staging/android/ion/ion-ioctl.c | 6 - drivers/staging/android/ion/ion.c | 282 ++++++--------------- drivers/staging/android/ion/ion.h | 5 +- drivers/staging/android/ion/ion_carveout_heap.c | 16 +- drivers/staging/android/ion/ion_chunk_heap.c | 15 +- drivers/staging/android/ion/ion_cma_heap.c | 102 ++------ drivers/staging/android/ion/ion_dummy_driver.c | 156 ------------ drivers/staging/android/ion/ion_enumerate.c | 89 +++++++ drivers/staging/android/ion/ion_of.c | 184 -------------- drivers/staging/android/ion/ion_of.h | 37 --- drivers/staging/android/ion/ion_page_pool.c | 3 - drivers/staging/android/ion/ion_priv.h | 57 ++++- drivers/staging/android/ion/ion_system_heap.c | 14 +- drivers/staging/android/ion/tegra/Makefile | 1 - drivers/staging/android/ion/tegra/tegra_ion.c | 80 ------ include/linux/cma.h | 6 +- mm/cma.c | 25 +- mm/cma.h | 1 + mm/cma_debug.c | 2 +- 25 files changed, 312 insertions(+), 958 deletions(-) delete mode 100644 drivers/staging/android/ion/hisilicon/Kconfig delete mode 100644 drivers/staging/android/ion/hisilicon/Makefile delete mode 100644 drivers/staging/android/ion/hisilicon/hi6220_ion.c delete mode 100644 drivers/staging/android/ion/ion_dummy_driver.c create mode 100644 drivers/staging/android/ion/ion_enumerate.c delete mode 100644 drivers/staging/android/ion/ion_of.c delete mode 100644 drivers/staging/android/ion/ion_of.h delete mode 100644 drivers/staging/android/ion/tegra/Makefile delete mode 100644 drivers/staging/android/ion/tegra/tegra_ion.c -- 2.7.4

8 years, 9 months

8
44
0 0

[RFC][PATCH] dma-buf: Introduce dma-buf test module

by Laura Abbott

dma-buf is designed to share buffers. Sharing means that there needs to be another subsystem to accept those buffers. Introduce a simple test module to act as a dummy system to accept dma_bufs from elsewhere. The goal is to provide a very simple interface to validate exported buffers do something reasonable. This is based on ion_test.c that existed for the Ion framework. Signed-off-by: Laura Abbott <labbott(a)redhat.com> --- This is basically a drop in of what was available as drivers/staging/android/ion/ion_test.c. Given it has no Ion specific parts it might be useful as a more general test module. RFC mostly to see if this is generally useful or not. --- drivers/dma-buf/Kconfig | 9 ++ drivers/dma-buf/Makefile | 1 + drivers/dma-buf/dma-buf-test.c | 309 ++++++++++++++++++++++++++++++++++++++ include/uapi/linux/dma_buf_test.h | 67 +++++++++ 4 files changed, 386 insertions(+) create mode 100644 drivers/dma-buf/dma-buf-test.c create mode 100644 include/uapi/linux/dma_buf_test.h diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index ed3b785..8b3fdb1 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -30,4 +30,13 @@ config SW_SYNC WARNING: improper use of this can result in deadlocking kernel drivers from userspace. Intended for test and debug only. +config DMA_BUF_TEST + bool "Test module for dma-buf" + default n + ---help--- + A test module to validate dma_buf APIs. This should not be + enabled for general use. + + Say N here unless you know you want this. + endmenu diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index c33bf88..5029608 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -1,3 +1,4 @@ obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o seqno-fence.o obj-$(CONFIG_SYNC_FILE) += sync_file.o obj-$(CONFIG_SW_SYNC) += sw_sync.o sync_debug.o +obj-$(CONFIG_DMA_BUF_TEST) += dma-buf-test.o diff --git a/drivers/dma-buf/dma-buf-test.c b/drivers/dma-buf/dma-buf-test.c new file mode 100644 index 0000000..3af131c --- /dev/null +++ b/drivers/dma-buf/dma-buf-test.c @@ -0,0 +1,309 @@ +/* + * Copyright (C) 2013 Google, Inc. + * + * This software is licensed under the terms of the GNU General Public + * License versdma_buf 2, as published by the Free Software Foundatdma_buf, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#define pr_fmt(fmt) "dma-buf-test: " fmt + +#include <linux/dma-buf.h> +#include <linux/dma-direction.h> +#include <linux/fs.h> +#include <linux/miscdevice.h> +#include <linux/mm.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/sched.h> +#include <linux/slab.h> +#include <linux/uaccess.h> +#include <linux/vmalloc.h> + +#include <uapi/linux/dma_buf_test.h> + +struct dma_buf_test_device { + struct miscdevice misc; +}; + +struct dma_buf_test_data { + struct dma_buf *dma_buf; + struct device *dev; +}; + +static int dma_buf_handle_test_dma(struct device *dev, struct dma_buf *dma_buf, + void __user *ptr, size_t offset, size_t size, + bool write) +{ + int ret = 0; + struct dma_buf_attachment *attach; + struct sg_table *table; + pgprot_t pgprot = pgprot_writecombine(PAGE_KERNEL); + enum dma_data_direction dir = write ? DMA_FROM_DEVICE : DMA_TO_DEVICE; + struct sg_page_iter sg_iter; + unsigned long offset_page; + + attach = dma_buf_attach(dma_buf, dev); + if (IS_ERR(attach)) + return PTR_ERR(attach); + + table = dma_buf_map_attachment(attach, dir); + if (IS_ERR(table)) + return PTR_ERR(table); + + offset_page = offset >> PAGE_SHIFT; + offset %= PAGE_SIZE; + + for_each_sg_page(table->sgl, &sg_iter, table->nents, offset_page) { + struct page *page = sg_page_iter_page(&sg_iter); + void *vaddr = vmap(&page, 1, VM_MAP, pgprot); + size_t to_copy = PAGE_SIZE - offset; + + to_copy = min(to_copy, size); + if (!vaddr) { + ret = -ENOMEM; + goto err; + } + + if (write) + ret = copy_from_user(vaddr + offset, ptr, to_copy); + else + ret = copy_to_user(ptr, vaddr + offset, to_copy); + + vunmap(vaddr); + if (ret) { + ret = -EFAULT; + goto err; + } + size -= to_copy; + if (!size) + break; + ptr += to_copy; + offset = 0; + } + +err: + dma_buf_unmap_attachment(attach, table, dir); + dma_buf_detach(dma_buf, attach); + return ret; +} + +static int dma_buf_handle_test_kernel(struct dma_buf *dma_buf, void __user *ptr, + size_t offset, size_t size, bool write) +{ + int ret; + unsigned long page_offset = offset >> PAGE_SHIFT; + size_t copy_offset = offset % PAGE_SIZE; + size_t copy_size = size; + enum dma_data_direction dir = write ? DMA_FROM_DEVICE : DMA_TO_DEVICE; + + if (offset > dma_buf->size || size > dma_buf->size - offset) + return -EINVAL; + + ret = dma_buf_begin_cpu_access(dma_buf, dir); + if (ret) + return ret; + + while (copy_size > 0) { + size_t to_copy; + void *vaddr = dma_buf_kmap(dma_buf, page_offset); + + if (!vaddr) + goto err; + + to_copy = min_t(size_t, PAGE_SIZE - copy_offset, copy_size); + + if (write) + ret = copy_from_user(vaddr + copy_offset, ptr, to_copy); + else + ret = copy_to_user(ptr, vaddr + copy_offset, to_copy); + + dma_buf_kunmap(dma_buf, page_offset, vaddr); + if (ret) { + ret = -EFAULT; + goto err; + } + + copy_size -= to_copy; + ptr += to_copy; + page_offset++; + copy_offset = 0; + } +err: + dma_buf_end_cpu_access(dma_buf, dir); + return ret; +} + +static long dma_buf_test_ioctl(struct file *filp, unsigned int cmd, + unsigned long arg) +{ + struct dma_buf_test_data *test_data = filp->private_data; + int ret = 0; + + union { + struct dma_buf_test_rw_data test_rw; + } data; + + if (_IOC_SIZE(cmd) > sizeof(data)) + return -EINVAL; + + if (_IOC_DIR(cmd) & _IOC_WRITE) + if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) + return -EFAULT; + + switch (cmd) { + case DMA_BUF_IOC_TEST_SET_FD: + { + struct dma_buf *dma_buf = NULL; + int fd = arg; + + if (fd >= 0) { + dma_buf = dma_buf_get((int)arg); + if (IS_ERR(dma_buf)) + return PTR_ERR(dma_buf); + } + if (test_data->dma_buf) + dma_buf_put(test_data->dma_buf); + test_data->dma_buf = dma_buf; + break; + } + case DMA_BUF_IOC_TEST_DMA_MAPPING: + { + ret = dma_buf_handle_test_dma(test_data->dev, + test_data->dma_buf, + u64_to_user_ptr(data.test_rw.ptr), + data.test_rw.offset, + data.test_rw.size, + data.test_rw.write); + break; + } + case DMA_BUF_IOC_TEST_KERNEL_MAPPING: + { + ret = dma_buf_handle_test_kernel(test_data->dma_buf, + u64_to_user_ptr(data.test_rw.ptr), + data.test_rw.offset, + data.test_rw.size, + data.test_rw.write); + break; + } + default: + return -ENOTTY; + } + + if (_IOC_DIR(cmd) & _IOC_READ) { + if (copy_to_user((void __user *)arg, &data, sizeof(data))) + return -EFAULT; + } + return ret; +} + +static int dma_buf_test_open(struct inode *inode, struct file *file) +{ + struct dma_buf_test_data *data; + struct miscdevice *miscdev = file->private_data; + + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->dev = miscdev->parent; + + file->private_data = data; + + return 0; +} + +static int dma_buf_test_release(struct inode *inode, struct file *file) +{ + struct dma_buf_test_data *data = file->private_data; + + kfree(data); + + return 0; +} + +static const struct file_operations dma_buf_test_fops = { + .owner = THIS_MODULE, + .unlocked_ioctl = dma_buf_test_ioctl, + .compat_ioctl = dma_buf_test_ioctl, + .open = dma_buf_test_open, + .release = dma_buf_test_release, +}; + +static int __init dma_buf_test_probe(struct platform_device *pdev) +{ + int ret; + struct dma_buf_test_device *testdev; + + testdev = devm_kzalloc(&pdev->dev, sizeof(struct dma_buf_test_device), + GFP_KERNEL); + if (!testdev) + return -ENOMEM; + + testdev->misc.minor = MISC_DYNAMIC_MINOR; + testdev->misc.name = "dma_buf-test"; + testdev->misc.fops = &dma_buf_test_fops; + testdev->misc.parent = &pdev->dev; + /* + * We need to force 'something' for a DMA mask. This isn't an actual + * device and won't be doing actual DMA so pick 'something' that + * probably won't blow up. Probably. + */ + dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); + ret = misc_register(&testdev->misc); + if (ret) { + pr_err("failed to register misc device.\n"); + return ret; + } + + platform_set_drvdata(pdev, testdev); + + return 0; +} + +static int dma_buf_test_remove(struct platform_device *pdev) +{ + struct dma_buf_test_device *testdev; + + testdev = platform_get_drvdata(pdev); + if (!testdev) + return -ENODATA; + + misc_deregister(&testdev->misc); + return 0; +} + +static struct platform_device *dma_buf_test_pdev; +static struct platform_driver dma_buf_test_platform_driver = { + .remove = dma_buf_test_remove, + .driver = { + .name = "dma_buf-test", + }, +}; + +static int __init dma_buf_test_init(void) +{ + dma_buf_test_pdev = platform_device_register_simple("dma-buf-test", + -1, NULL, 0); + if (IS_ERR(dma_buf_test_pdev)) + return PTR_ERR(dma_buf_test_pdev); + + return platform_driver_probe(&dma_buf_test_platform_driver, + dma_buf_test_probe); +} + +static void __exit dma_buf_test_exit(void) +{ + platform_driver_unregister(&dma_buf_test_platform_driver); + platform_device_unregister(dma_buf_test_pdev); +} + +module_init(dma_buf_test_init); +module_exit(dma_buf_test_exit); +MODULE_LICENSE("GPL v2"); diff --git a/include/uapi/linux/dma_buf_test.h b/include/uapi/linux/dma_buf_test.h new file mode 100644 index 0000000..af2d521 --- /dev/null +++ b/include/uapi/linux/dma_buf_test.h @@ -0,0 +1,67 @@ +/* + * Copyright (C) 2011 Google, Inc. + * + * This software is licensed under the terms of the GNU General Public + * License versdma_buf 2, as published by the Free Software Foundatdma_buf, and + * may be copied, distributed, and modified under those terms. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + */ + +#ifndef _UAPI_LINUX_DMA_BUF_TEST_H +#define _UAPI_LINUX_DMA_BUF_TEST_H + +#include <linux/ioctl.h> +#include <linux/types.h> + +/** + * struct dma_buf_test_rw_data - metadata passed to the kernel to read handle + * @ptr: a pointer to an area at least as large as size + * @offset: offset into the dma_buf buffer to start reading + * @size: size to read or write + * @write: 1 to write, 0 to read + */ +struct dma_buf_test_rw_data { + __u64 ptr; + __u64 offset; + __u64 size; + int write; + int __padding; +}; + +#define DMA_BUF_IOC_MAGIC 'I' + +/** + * DOC: DMA_BUF_IOC_TEST_SET_DMA_BUF - attach a dma buf to the test driver + * + * Attaches a dma buf fd to the test driver. Passing a second fd or -1 will + * release the first fd. + */ +#define DMA_BUF_IOC_TEST_SET_FD \ + _IO(DMA_BUF_IOC_MAGIC, 0xf0) + +/** + * DOC: DMA_BUF_IOC_TEST_DMA_MAPPING - read or write memory from a handle as DMA + * + * Reads or writes the memory from a handle using an uncached mapping. Can be + * used by unit tests to emulate a DMA engine as close as possible. Only + * expected to be used for debugging and testing, may not always be available. + */ +#define DMA_BUF_IOC_TEST_DMA_MAPPING \ + _IOW(DMA_BUF_IOC_MAGIC, 0xf1, struct dma_buf_test_rw_data) + +/** + * DOC: DMA_BUF_IOC_TEST_KERNEL_MAPPING - read or write memory from a handle + * + * Reads or writes the memory from a handle using a kernel mapping. Can be + * used by unit tests to test heap map_kernel functdma_bufs. Only expected to be + * used for debugging and testing, may not always be available. + */ +#define DMA_BUF_IOC_TEST_KERNEL_MAPPING \ + _IOW(DMA_BUF_IOC_MAGIC, 0xf2, struct dma_buf_test_rw_data) + +#endif /* _UAPI_LINUX_DMA_BUF_TEST_H */ -- 2.7.4

8 years, 9 months

2
3
0 0

Re: [Linaro-mm-sig] [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

by Daniel Vetter

On Mon, Mar 06, 2017 at 11:58:05AM +0100, Mark Brown wrote: > On Mon, Mar 06, 2017 at 11:40:41AM +0100, Daniel Vetter wrote: > > > No one gave a thing about android in upstream, so Greg KH just dumped it > > all into staging/android/. We've discussed ION a bunch of times, recorded > > anything we'd like to fix in staging/android/TODO, and Laura's patch > > series here addresses a big chunk of that. > > > This is pretty much the same approach we (gpu folks) used to de-stage the > > syncpt stuff. > > Well, there's also the fact that quite a few people have issues with the > design (like Laurent). It seems like a lot of them have either got more > comfortable with it over time, or at least not managed to come up with > any better ideas in the meantime. See the TODO, it has everything a really big group (look at the patch for the full Cc: list) figured needs to be improved at LPC 2015. We don't just merge stuff because merging stuff is fun :-) Laurent was even in that group ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

8 years, 9 months

7
17
0 0

Re: [Linaro-mm-sig] [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

by Laura Abbott

On 03/13/2017 06:21 AM, Mark Brown wrote: > On Mon, Mar 13, 2017 at 10:54:33AM +0000, Brian Starkey wrote: >> On Sun, Mar 12, 2017 at 02:34:14PM +0100, Benjamin Gaignard wrote: > >>> Another point is how can we put secure rules (like selinux policy) on >>> heaps since all the allocations >>> go to the same device (/dev/ion) ? For example, until now, in Android >>> we have to give the same >>> access rights to all the process that use ION. >>> It will become problem when we will add secure heaps because we won't >>> be able to distinguish secure >>> processes to standard ones or set specific policy per heaps. >>> Maybe I'm wrong here but I have never see selinux policy checking an >>> ioctl field but if that >>> exist it could be a solution. > >> I might be thinking of a different type of "secure", but... > >> Should the security of secure heaps be enforced by OS-level >> permissions? I don't know about other architectures, but at least on >> arm/arm64 this is enforced in hardware; it doesn't matter who has >> access to the ion heap, because only secure devices (or the CPU >> running a secure process) is physically able to access the memory >> backing the buffer. > 3 >> In fact, in the use-cases I know of, the process asking for the ion >> allocation is not a secure process, and so we wouldn't *want* to >> restrict the secure heap to be allocated from only by secure >> processes. > > I think there's an orthogonal level of OS level security that can be > applied here - it's reasonable for it to want to say things like "only > processes that are supposed to be implementing functionality X should be > able to try to allocate memory set aside for that functionality". This > mitigates against escallation attacks and so on, it's not really > directly related to secure memory as such though. > Ion also makes it pretty trivial to allocate large amounts of kernel memory and possibly DoS the system. I'd like to have as little policy in Ion as possible but more important would be a general security review and people shouting "bad idea ahead". Thanks, Laura

8 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [RFC PATCH 06/12] staging: android: ion: Remove crufty cache support

by Laura Abbott

On 03/06/2017 09:00 AM, Emil Velikov wrote: > On 6 March 2017 at 10:29, Daniel Vetter <daniel(a)ffwll.ch> wrote: >> On Fri, Mar 03, 2017 at 10:46:03AM -0800, Laura Abbott wrote: >>> On 03/03/2017 08:39 AM, Laurent Pinchart wrote: >>>> Hi Daniel, >>>> >>>> On Friday 03 Mar 2017 10:56:54 Daniel Vetter wrote: >>>>> On Thu, Mar 02, 2017 at 01:44:38PM -0800, Laura Abbott wrote: >>>>>> Now that we call dma_map in the dma_buf API callbacks there is no need >>>>>> to use the existing cache APIs. Remove the sync ioctl and the existing >>>>>> bad dma_sync calls. Explicit caching can be handled with the dma_buf >>>>>> sync API. >>>>>> >>>>>> Signed-off-by: Laura Abbott <labbott(a)redhat.com> >>>>>> --- >>>>>> >>>>>> drivers/staging/android/ion/ion-ioctl.c | 5 ---- >>>>>> drivers/staging/android/ion/ion.c | 40 -------------------- >>>>>> drivers/staging/android/ion/ion_carveout_heap.c | 6 ---- >>>>>> drivers/staging/android/ion/ion_chunk_heap.c | 6 ---- >>>>>> drivers/staging/android/ion/ion_page_pool.c | 3 -- >>>>>> drivers/staging/android/ion/ion_system_heap.c | 5 ---- >>>>>> 6 files changed, 65 deletions(-) >>>>>> >>>>>> diff --git a/drivers/staging/android/ion/ion-ioctl.c >>>>>> b/drivers/staging/android/ion/ion-ioctl.c index 5b2e93f..f820d77 100644 >>>>>> --- a/drivers/staging/android/ion/ion-ioctl.c >>>>>> +++ b/drivers/staging/android/ion/ion-ioctl.c >>>>>> @@ -146,11 +146,6 @@ long ion_ioctl(struct file *filp, unsigned int cmd, >>>>>> unsigned long arg)> >>>>>> data.handle.handle = handle->id; >>>>>> >>>>>> break; >>>>>> >>>>>> } >>>>>> >>>>>> - case ION_IOC_SYNC: >>>>>> - { >>>>>> - ret = ion_sync_for_device(client, data.fd.fd); >>>>>> - break; >>>>>> - } >>>>> >>>>> You missed the case ION_IOC_SYNC: in compat_ion.c. >>>>> >>>>> While at it: Should we also remove the entire custom_ioctl infrastructure? >>>>> It's entirely unused afaict, and for a pure buffer allocator I don't see >>>>> any need to have custom ioctl. >>>> >>>> I second that, if you want to make ion a standard API, then we certainly don't >>>> want any custom ioctl. >>>> >>>>> More code to remove potentially: >>>>> - The entire compat ioctl stuff - would be an abi break, but I guess if we >>>>> pick the 32bit abi and clean up the uapi headers we'll be mostly fine. >>>>> would allow us to remove compat_ion.c entirely. >>>>> >>>>> - ION_IOC_IMPORT: With this ion is purely an allocator, so not sure we >>>>> still need to be able to import anything. All the cache flushing/mapping >>>>> is done through dma-buf ops/ioctls. >>>>> >>>>> >>> >>> Good point to all of the above. I was considering keeping the import around >>> for backwards compatibility reasons but given how much other stuff is being >>> potentially broken, everything should just get ripped out. >> >> If you're ok with breaking the world, then I strongly suggest we go >> through the uapi header and replace all types with the standard >> fixed-width ones (__s32, __s64 and __u32, __u64). Allows us to remove all >> the compat ioctl code :-) > > I think the other comments from your "botching-up ioctls" [1] also apply ;-) > Namely - align structs to multiple of 64bit, add "flags" and properly > verity user input returning -EINVAL. > > -Emil > > [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Docume… I'm more torn on this. There's a difference between dropping an old ioctl/implicit caching vs. changing an actual ioctl ABI. Maybe having obvious breakage is better than subtle though, plus nobody has come begging me not to break the ABI yet. I might leave this for right before we do the actual move out of staging. Thanks, Laura

8 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

by Daniel Vetter

On Mon, Mar 06, 2017 at 08:42:59AM +0100, Michal Hocko wrote: > On Fri 03-03-17 09:37:55, Laura Abbott wrote: > > On 03/03/2017 05:29 AM, Michal Hocko wrote: > > > On Thu 02-03-17 13:44:32, Laura Abbott wrote: > > >> Hi, > > >> > > >> There's been some recent discussions[1] about Ion-like frameworks. There's > > >> apparently interest in just keeping Ion since it works reasonablly well. > > >> This series does what should be the final clean ups for it to possibly be > > >> moved out of staging. > > >> > > >> This includes the following: > > >> - Some general clean up and removal of features that never got a lot of use > > >> as far as I can tell. > > >> - Fixing up the caching. This is the series I proposed back in December[2] > > >> but never heard any feedback on. It will certainly break existing > > >> applications that rely on the implicit caching. I'd rather make an effort > > >> to move to a model that isn't going directly against the establishement > > >> though. > > >> - Fixing up the platform support. The devicetree approach was never well > > >> recieved by DT maintainers. The proposal here is to think of Ion less as > > >> specifying requirements and more of a framework for exposing memory to > > >> userspace. > > >> - CMA allocations now happen without the need of a dummy device structure. > > >> This fixes a bunch of the reasons why I attempted to add devicetree > > >> support before. > > >> > > >> I've had problems getting feedback in the past so if I don't hear any major > > >> objections I'm going to send out with the RFC dropped to be picked up. > > >> The only reason there isn't a patch to come out of staging is to discuss any > > >> other changes to the ABI people might want. Once this comes out of staging, > > >> I really don't want to mess with the ABI. > > > > > > Could you recapitulate concerns preventing the code being merged > > > normally rather than through the staging tree and how they were > > > addressed? > > > > > > > Sorry, I'm really not understanding your question here, can you > > clarify? > > There must have been a reason why this code ended up in the staging > tree, right? So my question is what those reasons were and how they were > handled in order to move the code from the staging subtree. No one gave a thing about android in upstream, so Greg KH just dumped it all into staging/android/. We've discussed ION a bunch of times, recorded anything we'd like to fix in staging/android/TODO, and Laura's patch series here addresses a big chunk of that. This is pretty much the same approach we (gpu folks) used to de-stage the syncpt stuff. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

8 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [RFC PATCH 00/12] Ion cleanup in preparation for moving out of staging

by Laura Abbott

On 03/03/2017 05:29 AM, Michal Hocko wrote: > On Thu 02-03-17 13:44:32, Laura Abbott wrote: >> Hi, >> >> There's been some recent discussions[1] about Ion-like frameworks. There's >> apparently interest in just keeping Ion since it works reasonablly well. >> This series does what should be the final clean ups for it to possibly be >> moved out of staging. >> >> This includes the following: >> - Some general clean up and removal of features that never got a lot of use >> as far as I can tell. >> - Fixing up the caching. This is the series I proposed back in December[2] >> but never heard any feedback on. It will certainly break existing >> applications that rely on the implicit caching. I'd rather make an effort >> to move to a model that isn't going directly against the establishement >> though. >> - Fixing up the platform support. The devicetree approach was never well >> recieved by DT maintainers. The proposal here is to think of Ion less as >> specifying requirements and more of a framework for exposing memory to >> userspace. >> - CMA allocations now happen without the need of a dummy device structure. >> This fixes a bunch of the reasons why I attempted to add devicetree >> support before. >> >> I've had problems getting feedback in the past so if I don't hear any major >> objections I'm going to send out with the RFC dropped to be picked up. >> The only reason there isn't a patch to come out of staging is to discuss any >> other changes to the ABI people might want. Once this comes out of staging, >> I really don't want to mess with the ABI. > > Could you recapitulate concerns preventing the code being merged > normally rather than through the staging tree and how they were > addressed? > Sorry, I'm really not understanding your question here, can you clarify? Thanks, Laura

8 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [RFCv3][PATCH 3/5] arm64: Implement ARCH_HAS_FORCE_CACHE

by Laura Abbott

On 02/22/2017 05:01 PM, Chen Feng wrote: > > > On 2017/2/22 3:29, Laura Abbott wrote: >> On 02/20/2017 10:05 PM, Chen Feng wrote: >>> Hi Laura, >>> >>> When we enable kernel v4.4 or newer version on our platform, we meet the issue >>> of flushing cache without reference device. It seems that this patch set is >>> a solution. I'm curious the progress of the discussion. Do you have any plan >>> to fix it in v4.4 and newer kernel verison? >>> >> >> No, I've abandoned this approach based on feedback. The APIs had too much >> potential for incorrect usage. I'm ripping out the implicit caching in Ion >> and switching it to a model where there should always be a device available. >> >> What's your use case where you don't have a device structure? >> > Userspace use ioctl to flush cache for device. > > ion_sync_for_device > dma_sync_sg_for_device(NULL, buffer->sg_table->sgl, > buffer->sg_table->nents, DMA_BIDIRECTIONAL); > > And sys-heap when allocate a zero buffer flush zero data to ddr. > alloc_buffer_page > ion_pages_sync_for_device(NULL, page, PAGE_SIZE << order, > DMA_BIDIRECTIONAL); > > Yes, those calls are being removed. This is what I proposed back in December https://marc.info/?l=linux-kernel&m=148176054902921&w=2 I never heard any feedback on it so I assume everyone was okay with the general direction. I plan on pushing a revised and expanded version of that series once the merge window closes. Thanks, Laura > >> Thanks, >> Laura >> >>> On 2016/9/14 2:41, Laura Abbott wrote: >>>> On 09/13/2016 08:14 AM, Will Deacon wrote: >>>>> On Tue, Sep 13, 2016 at 08:02:20AM -0700, Laura Abbott wrote: >>>>>> On 09/13/2016 02:19 AM, Will Deacon wrote: >>>>>>> On Mon, Sep 12, 2016 at 02:32:56PM -0700, Laura Abbott wrote: >>>>>>>> >>>>>>>> arm64 may need to guarantee the caches are synced. Implement versions of >>>>>>>> the kernel_force_cache API to allow this. >>>>>>>> >>>>>>>> Signed-off-by: Laura Abbott <labbott(a)redhat.com> >>>>>>>> --- >>>>>>>> v3: Switch to calling cache operations directly instead of relying on >>>>>>>> DMA mapping. >>>>>>>> --- >>>>>>>> arch/arm64/include/asm/cacheflush.h | 8 ++++++++ >>>>>>>> arch/arm64/mm/cache.S | 24 ++++++++++++++++++++---- >>>>>>>> arch/arm64/mm/flush.c | 11 +++++++++++ >>>>>>>> 3 files changed, 39 insertions(+), 4 deletions(-) >>>>>>> >>>>>>> I'm really hesitant to expose these cache routines as an API solely to >>>>>>> support a driver sitting in staging/. I appreciate that there's a chicken >>>>>>> and egg problem here, but we *really* don't want people using these routines >>>>>>> in preference to the DMA API, and I fear that we'll simply grow a bunch >>>>>>> more users of these things if we promote it as an API like you're proposing. >>>>>>> >>>>>>> Can the code not be contained under staging/, as part of ion? >>>>>>> >>>>>> >>>>>> I proposed that in V1 and it was suggested I make it a proper API >>>>>> >>>>>> http://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg47654… >>>>>> http://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg47672… >>>>> >>>>> :/ then I guess we're in disagreement. If ion really needs this stuff >>>>> (which I don't fully grok), perhaps we should be exposing something at >>>>> a higher level from the architecture, so it really can't be used for >>>>> anything other than ion. >>>> >>>> I talked/complained about this at a past plumbers. The gist is that Ion >>>> ends up acting as a fake DMA layer for clients. It doesn't match nicely >>>> because clients can allocate both coherent and non-coherent memory. >>>> Trying to use dma_map doesn't work because a) a device for coherency isn't >>>> known at allocation time b) it kills performance. Part of the motivation >>>> for taking this approach is to avoid the need to rework the existing >>>> Android userspace and keep the existing behavior, as terrible as it >>>> is. Having Ion out of staging and not actually usable isn't helpful. >>>> >>>> I'll give this all some more thought and hopefully have one or two more >>>> proposals before Connect/Plumbers. >>>> >>>>> >>>>> Will >>>>> >>>> >>>> Thanks, >>>> Laura >>>> _______________________________________________ >>>> Linaro-mm-sig mailing list >>>> Linaro-mm-sig(a)lists.linaro.org >>>> https://lists.linaro.org/mailman/listinfo/linaro-mm-sig >>> >>> >>> _______________________________________________ >>> linux-arm-kernel mailing list >>> linux-arm-kernel(a)lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>> >> >> >> . >> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel(a)lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >

8 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: add support for compat ioctl

by Daniel Vetter

On Tue, Feb 21, 2017 at 4:08 PM, Christian König <deathsimple(a)vodafone.de> wrote: > Am 21.02.2017 um 15:55 schrieb Marek Szyprowski: >> >> Dear All, >> >> On 2017-02-21 15:37, Marek Szyprowski wrote: >>> >>> Hi Christian, >>> >>> On 2017-02-21 14:59, Christian König wrote: >>>> >>>> Am 21.02.2017 um 14:21 schrieb Marek Szyprowski: >>>>> >>>>> Add compat ioctl support to dma-buf. This lets one to use >>>>> DMA_BUF_IOCTL_SYNC >>>>> ioctl from 32bit application on 64bit kernel. Data structures for both >>>>> 32 >>>>> and 64bit modes are same, so there is no need for additional >>>>> translation >>>>> layer. >>>> >>>> >>>> Well I might be wrong, but IIRC compat_ioctl was just optional and if >>>> not specified unlocked_ioctl was called instead. >>>> >>>> If that is true your patch wouldn't have any effect at all. >>> >>> >>> Well, then why I got -ENOTTY in the 32bit test app for this ioctl on >>> 64bit ARM64 kernel without this patch? >>> >> >> I've checked in fs/compat_ioctl.c, I see no fallback in >> COMPAT_SYSCALL_DEFINE3, >> so one has to provide compat_ioctl callback to have ioctl working with >> 32bit >> apps. > > > Then my memory cheated on me. > > In this case the patch is Reviewed-by: Christian König > <christian.koenig(a)amd.com>. Since you have commit rights for drm-misc, care to push this to drm-misc-next-fixes pls? Also I think this warrants a cc: stable, clearly an obvious screw-up in creating this api on our side :( So feel free to smash my ack on the patch. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch

8 years, 9 months

2
1
0 0

Re: [Linaro-mm-sig] [RFCv3][PATCH 3/5] arm64: Implement ARCH_HAS_FORCE_CACHE

by Laura Abbott

On 02/20/2017 10:05 PM, Chen Feng wrote: > Hi Laura, > > When we enable kernel v4.4 or newer version on our platform, we meet the issue > of flushing cache without reference device. It seems that this patch set is > a solution. I'm curious the progress of the discussion. Do you have any plan > to fix it in v4.4 and newer kernel verison? > No, I've abandoned this approach based on feedback. The APIs had too much potential for incorrect usage. I'm ripping out the implicit caching in Ion and switching it to a model where there should always be a device available. What's your use case where you don't have a device structure? Thanks, Laura > On 2016/9/14 2:41, Laura Abbott wrote: >> On 09/13/2016 08:14 AM, Will Deacon wrote: >>> On Tue, Sep 13, 2016 at 08:02:20AM -0700, Laura Abbott wrote: >>>> On 09/13/2016 02:19 AM, Will Deacon wrote: >>>>> On Mon, Sep 12, 2016 at 02:32:56PM -0700, Laura Abbott wrote: >>>>>> >>>>>> arm64 may need to guarantee the caches are synced. Implement versions of >>>>>> the kernel_force_cache API to allow this. >>>>>> >>>>>> Signed-off-by: Laura Abbott <labbott(a)redhat.com> >>>>>> --- >>>>>> v3: Switch to calling cache operations directly instead of relying on >>>>>> DMA mapping. >>>>>> --- >>>>>> arch/arm64/include/asm/cacheflush.h | 8 ++++++++ >>>>>> arch/arm64/mm/cache.S | 24 ++++++++++++++++++++---- >>>>>> arch/arm64/mm/flush.c | 11 +++++++++++ >>>>>> 3 files changed, 39 insertions(+), 4 deletions(-) >>>>> >>>>> I'm really hesitant to expose these cache routines as an API solely to >>>>> support a driver sitting in staging/. I appreciate that there's a chicken >>>>> and egg problem here, but we *really* don't want people using these routines >>>>> in preference to the DMA API, and I fear that we'll simply grow a bunch >>>>> more users of these things if we promote it as an API like you're proposing. >>>>> >>>>> Can the code not be contained under staging/, as part of ion? >>>>> >>>> >>>> I proposed that in V1 and it was suggested I make it a proper API >>>> >>>> http://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg47654… >>>> http://www.mail-archive.com/driverdev-devel@linuxdriverproject.org/msg47672… >>> >>> :/ then I guess we're in disagreement. If ion really needs this stuff >>> (which I don't fully grok), perhaps we should be exposing something at >>> a higher level from the architecture, so it really can't be used for >>> anything other than ion. >> >> I talked/complained about this at a past plumbers. The gist is that Ion >> ends up acting as a fake DMA layer for clients. It doesn't match nicely >> because clients can allocate both coherent and non-coherent memory. >> Trying to use dma_map doesn't work because a) a device for coherency isn't >> known at allocation time b) it kills performance. Part of the motivation >> for taking this approach is to avoid the need to rework the existing >> Android userspace and keep the existing behavior, as terrible as it >> is. Having Ion out of staging and not actually usable isn't helpful. >> >> I'll give this all some more thought and hopefully have one or two more >> proposals before Connect/Plumbers. >> >>> >>> Will >>> >> >> Thanks, >> Laura >> _______________________________________________ >> Linaro-mm-sig mailing list >> Linaro-mm-sig(a)lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/linaro-mm-sig > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel(a)lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >

8 years, 9 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH] dma-buf: add support for compat ioctl

by Marek Szyprowski

Hi Christian, On 2017-02-21 14:59, Christian König wrote: > Am 21.02.2017 um 14:21 schrieb Marek Szyprowski: >> Add compat ioctl support to dma-buf. This lets one to use >> DMA_BUF_IOCTL_SYNC >> ioctl from 32bit application on 64bit kernel. Data structures for >> both 32 >> and 64bit modes are same, so there is no need for additional translation >> layer. > > Well I might be wrong, but IIRC compat_ioctl was just optional and if > not specified unlocked_ioctl was called instead. > > If that is true your patch wouldn't have any effect at all. Well, then why I got -ENOTTY in the 32bit test app for this ioctl on 64bit ARM64 kernel without this patch? Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland

8 years, 9 months

1
1
0 0

[PATCH] dma-buf: add support for compat ioctl

by Marek Szyprowski

Add compat ioctl support to dma-buf. This lets one to use DMA_BUF_IOCTL_SYNC ioctl from 32bit application on 64bit kernel. Data structures for both 32 and 64bit modes are same, so there is no need for additional translation layer. Signed-off-by: Marek Szyprowski <m.szyprowski(a)samsung.com> --- drivers/dma-buf/dma-buf.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 718f832a5c71..0007b792827b 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -325,6 +325,9 @@ static long dma_buf_ioctl(struct file *file, .llseek = dma_buf_llseek, .poll = dma_buf_poll, .unlocked_ioctl = dma_buf_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = dma_buf_ioctl, +#endif }; /* -- 1.9.1

8 years, 9 months

1
0
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig