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
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
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.
>
>
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.
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)) {
>
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
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
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
>