The selftests, fix the error handling, remove unused functions and stop leaking memory in failed tests.
v2: fix the memory leak correctly.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/st-dma-fence-unwrap.c | 48 +++++++++++---------------- 1 file changed, 19 insertions(+), 29 deletions(-)
diff --git a/drivers/dma-buf/st-dma-fence-unwrap.c b/drivers/dma-buf/st-dma-fence-unwrap.c index 039f016b57be..e20c5a7dcfe4 100644 --- a/drivers/dma-buf/st-dma-fence-unwrap.c +++ b/drivers/dma-buf/st-dma-fence-unwrap.c @@ -4,27 +4,19 @@ * Copyright (C) 2022 Advanced Micro Devices, Inc. */
+#include <linux/dma-fence.h> +#include <linux/dma-fence-array.h> +#include <linux/dma-fence-chain.h> #include <linux/dma-fence-unwrap.h> -#if 0 -#include <linux/kernel.h> -#include <linux/kthread.h> -#include <linux/mm.h> -#include <linux/sched/signal.h> -#include <linux/slab.h> -#include <linux/spinlock.h> -#include <linux/random.h> -#endif
#include "selftest.h"
#define CHAIN_SZ (4 << 10)
-static inline struct mock_fence { +struct mock_fence { struct dma_fence base; spinlock_t lock; -} *to_mock_fence(struct dma_fence *f) { - return container_of(f, struct mock_fence, base); -} +};
static const char *mock_name(struct dma_fence *f) { @@ -45,7 +37,8 @@ static struct dma_fence *mock_fence(void) return NULL;
spin_lock_init(&f->lock); - dma_fence_init(&f->base, &mock_ops, &f->lock, 0, 0); + dma_fence_init(&f->base, &mock_ops, &f->lock, + dma_fence_context_alloc(1), 1);
return &f->base; } @@ -59,7 +52,7 @@ static struct dma_fence *mock_array(unsigned int num_fences, ...)
fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL); if (!fences) - return NULL; + goto error_put;
va_start(valist, num_fences); for (i = 0; i < num_fences; ++i) @@ -70,13 +63,17 @@ static struct dma_fence *mock_array(unsigned int num_fences, ...) dma_fence_context_alloc(1), 1, false); if (!array) - goto cleanup; + goto error_free; return &array->base;
-cleanup: - for (i = 0; i < num_fences; ++i) - dma_fence_put(fences[i]); +error_free: kfree(fences); + +error_put: + va_start(valist, num_fences); + for (i = 0; i < num_fences; ++i) + dma_fence_put(va_arg(valist, typeof(*fences))); + va_end(valist); return NULL; }
@@ -113,7 +110,6 @@ static int sanitycheck(void *arg) if (!chain) return -ENOMEM;
- dma_fence_signal(f); dma_fence_put(chain); return err; } @@ -154,10 +150,8 @@ static int unwrap_array(void *arg) err = -EINVAL; }
- dma_fence_signal(f1); - dma_fence_signal(f2); dma_fence_put(array); - return 0; + return err; }
static int unwrap_chain(void *arg) @@ -196,10 +190,8 @@ static int unwrap_chain(void *arg) err = -EINVAL; }
- dma_fence_signal(f1); - dma_fence_signal(f2); dma_fence_put(chain); - return 0; + return err; }
static int unwrap_chain_array(void *arg) @@ -242,10 +234,8 @@ static int unwrap_chain_array(void *arg) err = -EINVAL; }
- dma_fence_signal(f1); - dma_fence_signal(f2); dma_fence_put(chain); - return 0; + return err; }
int dma_fence_unwrap(void)
Move the code from the inline functions into exported functions.
Signed-off-by: Christian König christian.koenig@amd.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/dma-buf/Makefile | 2 +- drivers/dma-buf/dma-fence-unwrap.c | 59 ++++++++++++++++++++++++++++++ include/linux/dma-fence-unwrap.h | 52 ++------------------------ 3 files changed, 64 insertions(+), 49 deletions(-) create mode 100644 drivers/dma-buf/dma-fence-unwrap.c
diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index 4c9eb53ba3f8..70ec901edf2c 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0-only obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \ - dma-resv.o + dma-fence-unwrap.o dma-resv.o obj-$(CONFIG_DMABUF_HEAPS) += dma-heap.o obj-$(CONFIG_DMABUF_HEAPS) += heaps/ obj-$(CONFIG_SYNC_FILE) += sync_file.o diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c new file mode 100644 index 000000000000..711be125428c --- /dev/null +++ b/drivers/dma-buf/dma-fence-unwrap.c @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * dma-fence-util: misc functions for dma_fence objects + * + * Copyright (C) 2022 Advanced Micro Devices, Inc. + * Authors: + * Christian König christian.koenig@amd.com + */ + +#include <linux/dma-fence.h> +#include <linux/dma-fence-array.h> +#include <linux/dma-fence-chain.h> +#include <linux/dma-fence-unwrap.h> + +/* Internal helper to start new array iteration, don't use directly */ +static struct dma_fence * +__dma_fence_unwrap_array(struct dma_fence_unwrap *cursor) +{ + cursor->array = dma_fence_chain_contained(cursor->chain); + cursor->index = 0; + return dma_fence_array_first(cursor->array); +} + +/** + * dma_fence_unwrap_first - return the first fence from fence containers + * @head: the entrypoint into the containers + * @cursor: current position inside the containers + * + * Unwraps potential dma_fence_chain/dma_fence_array containers and return the + * first fence. + */ +struct dma_fence *dma_fence_unwrap_first(struct dma_fence *head, + struct dma_fence_unwrap *cursor) +{ + cursor->chain = dma_fence_get(head); + return __dma_fence_unwrap_array(cursor); +} +EXPORT_SYMBOL_GPL(dma_fence_unwrap_first); + +/** + * dma_fence_unwrap_next - return the next fence from a fence containers + * @cursor: current position inside the containers + * + * Continue unwrapping the dma_fence_chain/dma_fence_array containers and return + * the next fence from them. + */ +struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor) +{ + struct dma_fence *tmp; + + ++cursor->index; + tmp = dma_fence_array_next(cursor->array, cursor->index); + if (tmp) + return tmp; + + cursor->chain = dma_fence_chain_walk(cursor->chain); + return __dma_fence_unwrap_array(cursor); +} +EXPORT_SYMBOL_GPL(dma_fence_unwrap_next); diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index 77e335a1bcac..e7c219da4ed7 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -1,7 +1,5 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* - * fence-chain: chain fences together in a timeline - * * Copyright (C) 2022 Advanced Micro Devices, Inc. * Authors: * Christian König christian.koenig@amd.com @@ -10,8 +8,7 @@ #ifndef __LINUX_DMA_FENCE_UNWRAP_H #define __LINUX_DMA_FENCE_UNWRAP_H
-#include <linux/dma-fence-chain.h> -#include <linux/dma-fence-array.h> +struct dma_fence;
/** * struct dma_fence_unwrap - cursor into the container structure @@ -33,50 +30,9 @@ struct dma_fence_unwrap { unsigned int index; };
-/* Internal helper to start new array iteration, don't use directly */ -static inline struct dma_fence * -__dma_fence_unwrap_array(struct dma_fence_unwrap * cursor) -{ - cursor->array = dma_fence_chain_contained(cursor->chain); - cursor->index = 0; - return dma_fence_array_first(cursor->array); -} - -/** - * dma_fence_unwrap_first - return the first fence from fence containers - * @head: the entrypoint into the containers - * @cursor: current position inside the containers - * - * Unwraps potential dma_fence_chain/dma_fence_array containers and return the - * first fence. - */ -static inline struct dma_fence * -dma_fence_unwrap_first(struct dma_fence *head, struct dma_fence_unwrap *cursor) -{ - cursor->chain = dma_fence_get(head); - return __dma_fence_unwrap_array(cursor); -} - -/** - * dma_fence_unwrap_next - return the next fence from a fence containers - * @cursor: current position inside the containers - * - * Continue unwrapping the dma_fence_chain/dma_fence_array containers and return - * the next fence from them. - */ -static inline struct dma_fence * -dma_fence_unwrap_next(struct dma_fence_unwrap *cursor) -{ - struct dma_fence *tmp; - - ++cursor->index; - tmp = dma_fence_array_next(cursor->array, cursor->index); - if (tmp) - return tmp; - - cursor->chain = dma_fence_chain_walk(cursor->chain); - return __dma_fence_unwrap_array(cursor); -} +struct dma_fence *dma_fence_unwrap_first(struct dma_fence *head, + struct dma_fence_unwrap *cursor); +struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor);
/** * dma_fence_unwrap_for_each - iterate over all fences in containers
dma_fence_chain containers cleanup signaled fences automatically, so filter those out from arrays as well.
v2: fix missing walk over the array v3: massively simplify the patch and actually update the description.
Signed-off-by: Christian König christian.koenig@amd.com --- include/linux/dma-fence-unwrap.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index e7c219da4ed7..a4d342fef8e0 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -43,9 +43,13 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor); * Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all * potential fences in them. If @head is just a normal fence only that one is * returned. + * + * Note that signalled fences are opportunistically filtered out, which + * means the iteration is potentially over no fence at all. */ #define dma_fence_unwrap_for_each(fence, cursor, head) \ for (fence = dma_fence_unwrap_first(head, cursor); fence; \ - fence = dma_fence_unwrap_next(cursor)) + fence = dma_fence_unwrap_next(cursor)) \ + if (!dma_fence_is_signaled(fence))
#endif
On Fri, May 06, 2022 at 04:10:07PM +0200, Christian König wrote:
dma_fence_chain containers cleanup signaled fences automatically, so filter those out from arrays as well.
v2: fix missing walk over the array v3: massively simplify the patch and actually update the description.
Signed-off-by: Christian König christian.koenig@amd.com
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
include/linux/dma-fence-unwrap.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index e7c219da4ed7..a4d342fef8e0 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -43,9 +43,13 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor);
- Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all
- potential fences in them. If @head is just a normal fence only that one is
- returned.
- Note that signalled fences are opportunistically filtered out, which
*/
- means the iteration is potentially over no fence at all.
#define dma_fence_unwrap_for_each(fence, cursor, head) \ for (fence = dma_fence_unwrap_first(head, cursor); fence; \
fence = dma_fence_unwrap_next(cursor))
fence = dma_fence_unwrap_next(cursor)) \
if (!dma_fence_is_signaled(fence))
#endif
2.25.1
Hi Christian,
I'm sorry for digging this one out so late.
On 06.05.2022 16:10, Christian König wrote:
dma_fence_chain containers cleanup signaled fences automatically, so filter those out from arrays as well.
v2: fix missing walk over the array v3: massively simplify the patch and actually update the description.
Signed-off-by: Christian König christian.koenig@amd.com
include/linux/dma-fence-unwrap.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index e7c219da4ed7..a4d342fef8e0 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -43,9 +43,13 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor);
- Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all
- potential fences in them. If @head is just a normal fence only that one is
- returned.
- Note that signalled fences are opportunistically filtered out, which
*/ #define dma_fence_unwrap_for_each(fence, cursor, head) \ for (fence = dma_fence_unwrap_first(head, cursor); fence; \
- means the iteration is potentially over no fence at all.
fence = dma_fence_unwrap_next(cursor))
fence = dma_fence_unwrap_next(cursor)) \
if (!dma_fence_is_signaled(fence))
#endif
It looks like this particular patch affects merging Sync Fences, which is reflected by failing IGT test (igt@sw_sync)[1]. The failing subtests are: - sync_merge - merging different fences on the same timeline, neither single nor merged fences are signaled
- sync_merge_same - merging the fence with itself on the same timeline, the fence didn't signal at all
- sync_multi_timeline_wait - merging different fences on different timelines; the subtest checks if counting fences of various states works. Currently, it can only see 2 active fences, 0 signaling (should be 2 active, 1 signaling)
Reverting this commit on the top of drm-tip fixes the issue, but I'm not sure if it wouldn't impact other places in the code. Please let me know if I can be of any help.
All the best, Karolina
--------------------- [1] - reproducible locally, but can be also seen in the CI: https://intel-gfx-ci.01.org/tree/drm-tip/index.html?testfilter=igt@sw_sync
Hi Karolina,
Am 11.07.22 um 11:44 schrieb Karolina Drobnik:
Hi Christian,
I'm sorry for digging this one out so late.
On 06.05.2022 16:10, Christian König wrote:
dma_fence_chain containers cleanup signaled fences automatically, so filter those out from arrays as well.
v2: fix missing walk over the array v3: massively simplify the patch and actually update the description.
Signed-off-by: Christian König christian.koenig@amd.com
include/linux/dma-fence-unwrap.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index e7c219da4ed7..a4d342fef8e0 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -43,9 +43,13 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor); * Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all * potential fences in them. If @head is just a normal fence only that one is * returned.
- Note that signalled fences are opportunistically filtered out, which
- means the iteration is potentially over no fence at all.
*/ #define dma_fence_unwrap_for_each(fence, cursor, head) \ for (fence = dma_fence_unwrap_first(head, cursor); fence; \ - fence = dma_fence_unwrap_next(cursor)) + fence = dma_fence_unwrap_next(cursor)) \ + if (!dma_fence_is_signaled(fence)) #endif
It looks like this particular patch affects merging Sync Fences, which is reflected by failing IGT test (igt@sw_sync)[1]. The failing subtests are: - sync_merge - merging different fences on the same timeline, neither single nor merged fences are signaled
- sync_merge_same - merging the fence with itself on the same timeline, the fence didn't signal at all
- sync_multi_timeline_wait - merging different fences on different timelines; the subtest checks if counting fences of various states works. Currently, it can only see 2 active fences, 0 signaling (should be 2 active, 1 signaling)
Reverting this commit on the top of drm-tip fixes the issue, but I'm not sure if it wouldn't impact other places in the code. Please let me know if I can be of any help.
Thanks for letting me know. Not sure what's going on here, but I can take a look today if time permits.
Do you have a description how to easy reproduce this? E.g. how to run just those specific igts?
Thanks, Christian.
All the best, Karolina
[1] - reproducible locally, but can be also seen in the CI: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fintel-gfx-...
Hi Christian,
On 11.07.2022 11:57, Christian König wrote:
Hi Karolina,
Am 11.07.22 um 11:44 schrieb Karolina Drobnik:
Hi Christian,
I'm sorry for digging this one out so late.
On 06.05.2022 16:10, Christian König wrote:
dma_fence_chain containers cleanup signaled fences automatically, so filter those out from arrays as well.
v2: fix missing walk over the array v3: massively simplify the patch and actually update the description.
Signed-off-by: Christian König christian.koenig@amd.com
include/linux/dma-fence-unwrap.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index e7c219da4ed7..a4d342fef8e0 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -43,9 +43,13 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor); * Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all * potential fences in them. If @head is just a normal fence only that one is * returned.
- Note that signalled fences are opportunistically filtered out, which
- means the iteration is potentially over no fence at all.
*/ #define dma_fence_unwrap_for_each(fence, cursor, head) \ for (fence = dma_fence_unwrap_first(head, cursor); fence; \ - fence = dma_fence_unwrap_next(cursor)) + fence = dma_fence_unwrap_next(cursor)) \ + if (!dma_fence_is_signaled(fence)) #endif
It looks like this particular patch affects merging Sync Fences, which is reflected by failing IGT test (igt@sw_sync)[1]. The failing subtests are: - sync_merge - merging different fences on the same timeline, neither single nor merged fences are signaled
- sync_merge_same - merging the fence with itself on the same timeline, the fence didn't signal at all
- sync_multi_timeline_wait - merging different fences on different timelines; the subtest checks if counting fences of various states works. Currently, it can only see 2 active fences, 0 signaling (should be 2 active, 1 signaling)
Reverting this commit on the top of drm-tip fixes the issue, but I'm not sure if it wouldn't impact other places in the code. Please let me know if I can be of any help.
Thanks for letting me know. Not sure what's going on here, but I can take a look today if time permits.
The reproduction with IGTs should be quite easy. You'll need to clone/download the IGT code and follow instructions for Building[1] the project (make sure you have meson and ninja installed):
https://gitlab.freedesktop.org/drm/igt-gpu-tools
Once you have it up and running, go to <igt path>/build/tests, and run the subtests:
./sw_sync --run sync_merge ./sw_sync --run sync_merge_same ./sw_sync --run sync_multi_timeline_wait
You can run all the subtests with ./sw_sync, but I think these are the most relevant to you.
Many thanks, Karolina
------------------ [1] - https://gitlab.freedesktop.org/drm/igt-gpu-tools#building
Do you have a description how to easy reproduce this? E.g. how to run just those specific igts?
Thanks, Christian.
All the best, Karolina
Hi Karolina,
Am 11.07.22 um 14:17 schrieb Karolina Drobnik:
Hi Christian,
On 11.07.2022 11:57, Christian König wrote:
Hi Karolina,
Am 11.07.22 um 11:44 schrieb Karolina Drobnik:
Hi Christian,
I'm sorry for digging this one out so late.
On 06.05.2022 16:10, Christian König wrote:
dma_fence_chain containers cleanup signaled fences automatically, so filter those out from arrays as well.
v2: fix missing walk over the array v3: massively simplify the patch and actually update the description.
Signed-off-by: Christian König christian.koenig@amd.com
include/linux/dma-fence-unwrap.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index e7c219da4ed7..a4d342fef8e0 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -43,9 +43,13 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor); * Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all * potential fences in them. If @head is just a normal fence only that one is * returned.
- Note that signalled fences are opportunistically filtered out,
which
- means the iteration is potentially over no fence at all.
*/ #define dma_fence_unwrap_for_each(fence, cursor, head) \ for (fence = dma_fence_unwrap_first(head, cursor); fence; \ - fence = dma_fence_unwrap_next(cursor)) + fence = dma_fence_unwrap_next(cursor)) \ + if (!dma_fence_is_signaled(fence)) #endif
It looks like this particular patch affects merging Sync Fences, which is reflected by failing IGT test (igt@sw_sync)[1]. The failing subtests are: - sync_merge - merging different fences on the same timeline, neither single nor merged fences are signaled
- sync_merge_same - merging the fence with itself on the same timeline, the fence didn't signal at all
- sync_multi_timeline_wait - merging different fences on different timelines; the subtest checks if counting fences of various states works. Currently, it can only see 2 active fences, 0 signaling (should be 2 active, 1 signaling)
Reverting this commit on the top of drm-tip fixes the issue, but I'm not sure if it wouldn't impact other places in the code. Please let me know if I can be of any help.
Thanks for letting me know. Not sure what's going on here, but I can take a look today if time permits.
The reproduction with IGTs should be quite easy. You'll need to clone/download the IGT code and follow instructions for Building[1] the project (make sure you have meson and ninja installed):
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.fre...
Once you have it up and running, go to <igt path>/build/tests, and run the subtests:
./sw_sync --run sync_merge ./sw_sync --run sync_merge_same ./sw_sync --run sync_multi_timeline_wait
You can run all the subtests with ./sw_sync, but I think these are the most relevant to you.
Thanks, I've already managed to reproduce it.
Not sure what's going on here, but could be that the test case was never correct in the first place. Need to double check.
Thanks, Christian.
Many thanks, Karolina
[1] - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.fre...
Do you have a description how to easy reproduce this? E.g. how to run just those specific igts?
Thanks, Christian.
All the best, Karolina
Hi Christian,
On 11.07.2022 14:25, Christian König wrote:
Hi Karolina,
Am 11.07.22 um 14:17 schrieb Karolina Drobnik:
Hi Christian,
On 11.07.2022 11:57, Christian König wrote:
Hi Karolina,
Am 11.07.22 um 11:44 schrieb Karolina Drobnik:
Hi Christian,
I'm sorry for digging this one out so late.
On 06.05.2022 16:10, Christian König wrote:
dma_fence_chain containers cleanup signaled fences automatically, so filter those out from arrays as well.
v2: fix missing walk over the array v3: massively simplify the patch and actually update the description.
Signed-off-by: Christian König christian.koenig@amd.com
include/linux/dma-fence-unwrap.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index e7c219da4ed7..a4d342fef8e0 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -43,9 +43,13 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor); * Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all * potential fences in them. If @head is just a normal fence only that one is * returned.
- Note that signalled fences are opportunistically filtered out,
which
- means the iteration is potentially over no fence at all.
*/ #define dma_fence_unwrap_for_each(fence, cursor, head) \ for (fence = dma_fence_unwrap_first(head, cursor); fence; \ - fence = dma_fence_unwrap_next(cursor)) + fence = dma_fence_unwrap_next(cursor)) \ + if (!dma_fence_is_signaled(fence)) #endif
It looks like this particular patch affects merging Sync Fences, which is reflected by failing IGT test (igt@sw_sync)[1]. The failing subtests are: - sync_merge - merging different fences on the same timeline, neither single nor merged fences are signaled
- sync_merge_same - merging the fence with itself on the same timeline, the fence didn't signal at all
- sync_multi_timeline_wait - merging different fences on different timelines; the subtest checks if counting fences of various states works. Currently, it can only see 2 active fences, 0 signaling (should be 2 active, 1 signaling)
Reverting this commit on the top of drm-tip fixes the issue, but I'm not sure if it wouldn't impact other places in the code. Please let me know if I can be of any help.
Thanks for letting me know. Not sure what's going on here, but I can take a look today if time permits.
The reproduction with IGTs should be quite easy. You'll need to clone/download the IGT code and follow instructions for Building[1] the project (make sure you have meson and ninja installed):
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.fre...
Once you have it up and running, go to <igt path>/build/tests, and run the subtests:
./sw_sync --run sync_merge ./sw_sync --run sync_merge_same ./sw_sync --run sync_multi_timeline_wait
You can run all the subtests with ./sw_sync, but I think these are the most relevant to you.
Thanks, I've already managed to reproduce it.
Not sure what's going on here, but could be that the test case was never correct in the first place. Need to double check.
That's also a possibility, but I couldn't verify it before writing to you, as it's not my area of expertise.
Thanks for taking a look at this.
All the best, Karolina
Thanks, Christian.
Many thanks, Karolina
[1] - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.fre...
Do you have a description how to easy reproduce this? E.g. how to run just those specific igts?
Thanks, Christian.
All the best, Karolina
Introduce a dma_fence_unwrap_merge() macro which allows to unwrap fences which potentially can be containers as well and then merge them back together into a flat dma_fence_array.
v2: rename the function, add some more comments about how the wrapper is used, move filtering of signaled fences into the unwrap iterator, add complex selftest which covers more cases.
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/dma-buf/dma-fence-unwrap.c | 99 +++++++++++++++++++++ drivers/dma-buf/st-dma-fence-unwrap.c | 109 +++++++++++++++++++++++ drivers/dma-buf/sync_file.c | 119 ++------------------------ include/linux/dma-fence-unwrap.h | 24 ++++++ 4 files changed, 238 insertions(+), 113 deletions(-)
diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c index 711be125428c..0e51547bbabd 100644 --- a/drivers/dma-buf/dma-fence-unwrap.c +++ b/drivers/dma-buf/dma-fence-unwrap.c @@ -11,6 +11,7 @@ #include <linux/dma-fence-array.h> #include <linux/dma-fence-chain.h> #include <linux/dma-fence-unwrap.h> +#include <linux/slab.h>
/* Internal helper to start new array iteration, don't use directly */ static struct dma_fence * @@ -57,3 +58,101 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor) return __dma_fence_unwrap_array(cursor); } EXPORT_SYMBOL_GPL(dma_fence_unwrap_next); + +/* Implementation for the dma_fence_merge() marco, don't use directly */ +struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences, + struct dma_fence **fences, + struct dma_fence_unwrap *iter) +{ + struct dma_fence_array *result; + struct dma_fence *tmp, **array; + unsigned int i; + size_t count; + + count = 0; + for (i = 0; i < num_fences; ++i) { + dma_fence_unwrap_for_each(tmp, &iter[i], fences[i]) + ++count; + } + + if (count == 0) + return dma_fence_get_stub(); + + array = kmalloc_array(count, sizeof(*array), GFP_KERNEL); + if (!array) + return NULL; + + /* + * This trashes the input fence array and uses it as position for the + * following merge loop. This works because the dma_fence_merge() + * wrapper macro is creating this temporary array on the stack together + * with the iterators. + */ + for (i = 0; i < num_fences; ++i) + fences[i] = dma_fence_unwrap_first(fences[i], &iter[i]); + + count = 0; + do { + unsigned int sel; + +restart: + tmp = NULL; + for (i = 0; i < num_fences; ++i) { + struct dma_fence *next = fences[i]; + + if (!next) + continue; + + /* + * We can't guarantee that inpute fences are ordered by + * context, but it is still quite likely when this + * function is used multiple times. So attempt to order + * the fences by context as we pass over them and merge + * fences with the same context. + */ + if (!tmp || tmp->context > next->context) { + tmp = next; + sel = i; + + } else if (tmp->context < next->context) { + continue; + + } else if (dma_fence_is_later(tmp, next)) { + fences[i] = dma_fence_unwrap_next(&iter[i]); + goto restart; + } else { + fences[sel] = dma_fence_unwrap_next(&iter[sel]); + goto restart; + } + } + + if (tmp) { + array[count++] = dma_fence_get(tmp); + fences[sel] = dma_fence_unwrap_next(&iter[sel]); + } + } while (tmp); + + if (count == 0) { + tmp = dma_fence_get_stub(); + goto return_tmp; + } + + if (count == 1) { + tmp = array[0]; + goto return_tmp; + } + + result = dma_fence_array_create(count, array, + dma_fence_context_alloc(1), + 1, false); + if (!result) { + tmp = NULL; + goto return_tmp; + } + return &result->base; + +return_tmp: + kfree(array); + return tmp; +} +EXPORT_SYMBOL_GPL(__dma_fence_unwrap_merge); diff --git a/drivers/dma-buf/st-dma-fence-unwrap.c b/drivers/dma-buf/st-dma-fence-unwrap.c index e20c5a7dcfe4..4105d5ea8dde 100644 --- a/drivers/dma-buf/st-dma-fence-unwrap.c +++ b/drivers/dma-buf/st-dma-fence-unwrap.c @@ -238,6 +238,113 @@ static int unwrap_chain_array(void *arg) return err; }
+static int unwrap_merge(void *arg) +{ + struct dma_fence *fence, *f1, *f2, *f3; + struct dma_fence_unwrap iter; + int err = 0; + + f1 = mock_fence(); + if (!f1) + return -ENOMEM; + + f2 = mock_fence(); + if (!f2) { + err = -ENOMEM; + goto error_put_f1; + } + + f3 = dma_fence_unwrap_merge(f1, f2); + if (!f3) { + err = -ENOMEM; + goto error_put_f2; + } + + dma_fence_unwrap_for_each(fence, &iter, f3) { + if (fence == f1) { + dma_fence_put(f1); + f1 = NULL; + } else if (fence == f2) { + dma_fence_put(f2); + f2 = NULL; + } else { + pr_err("Unexpected fence!\n"); + err = -EINVAL; + } + } + + if (f1 || f2) { + pr_err("Not all fences seen!\n"); + err = -EINVAL; + } + + dma_fence_put(f3); +error_put_f2: + dma_fence_put(f2); +error_put_f1: + dma_fence_put(f1); + return err; +} + +static int unwrap_merge_complex(void *arg) +{ + struct dma_fence *fence, *f1, *f2, *f3, *f4, *f5; + struct dma_fence_unwrap iter; + int err = -ENOMEM; + + f1 = mock_fence(); + if (!f1) + return -ENOMEM; + + f2 = mock_fence(); + if (!f2) + goto error_put_f1; + + f3 = dma_fence_unwrap_merge(f1, f2); + if (!f3) + goto error_put_f2; + + /* The resulting array has the fences in reverse */ + f4 = dma_fence_unwrap_merge(f2, f1); + if (!f4) + goto error_put_f3; + + /* Signaled fences should be filtered, the two arrays merged. */ + f5 = dma_fence_unwrap_merge(f3, f4, dma_fence_get_stub()); + if (!f5) + goto error_put_f4; + + err = 0; + dma_fence_unwrap_for_each(fence, &iter, f5) { + if (fence == f1) { + dma_fence_put(f1); + f1 = NULL; + } else if (fence == f2) { + dma_fence_put(f2); + f2 = NULL; + } else { + pr_err("Unexpected fence!\n"); + err = -EINVAL; + } + } + + if (f1 || f2) { + pr_err("Not all fences seen!\n"); + err = -EINVAL; + } + + dma_fence_put(f5); +error_put_f4: + dma_fence_put(f4); +error_put_f3: + dma_fence_put(f3); +error_put_f2: + dma_fence_put(f2); +error_put_f1: + dma_fence_put(f1); + return err; +} + int dma_fence_unwrap(void) { static const struct subtest tests[] = { @@ -245,6 +352,8 @@ int dma_fence_unwrap(void) SUBTEST(unwrap_array), SUBTEST(unwrap_chain), SUBTEST(unwrap_chain_array), + SUBTEST(unwrap_merge), + SUBTEST(unwrap_merge_complex), };
return subtests(tests, NULL); diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index 0fe564539166..3ebec19a8e02 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -146,50 +146,6 @@ char *sync_file_get_name(struct sync_file *sync_file, char *buf, int len) return buf; }
-static int sync_file_set_fence(struct sync_file *sync_file, - struct dma_fence **fences, int num_fences) -{ - struct dma_fence_array *array; - - /* - * The reference for the fences in the new sync_file and held - * in add_fence() during the merge procedure, so for num_fences == 1 - * we already own a new reference to the fence. For num_fence > 1 - * we own the reference of the dma_fence_array creation. - */ - - if (num_fences == 0) { - sync_file->fence = dma_fence_get_stub(); - kfree(fences); - - } else if (num_fences == 1) { - sync_file->fence = fences[0]; - kfree(fences); - - } else { - array = dma_fence_array_create(num_fences, fences, - dma_fence_context_alloc(1), - 1, false); - if (!array) - return -ENOMEM; - - sync_file->fence = &array->base; - } - - return 0; -} - -static void add_fence(struct dma_fence **fences, - int *i, struct dma_fence *fence) -{ - fences[*i] = fence; - - if (!dma_fence_is_signaled(fence)) { - dma_fence_get(fence); - (*i)++; - } -} - /** * sync_file_merge() - merge two sync_files * @name: name of new fence @@ -203,84 +159,21 @@ static void add_fence(struct dma_fence **fences, static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, struct sync_file *b) { - struct dma_fence *a_fence, *b_fence, **fences; - struct dma_fence_unwrap a_iter, b_iter; - unsigned int index, num_fences; struct sync_file *sync_file; + struct dma_fence *fence;
sync_file = sync_file_alloc(); if (!sync_file) return NULL;
- num_fences = 0; - dma_fence_unwrap_for_each(a_fence, &a_iter, a->fence) - ++num_fences; - dma_fence_unwrap_for_each(b_fence, &b_iter, b->fence) - ++num_fences; - - if (num_fences > INT_MAX) - goto err_free_sync_file; - - fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL); - if (!fences) - goto err_free_sync_file; - - /* - * We can't guarantee that fences in both a and b are ordered, but it is - * still quite likely. - * - * So attempt to order the fences as we pass over them and merge fences - * with the same context. - */ - - index = 0; - for (a_fence = dma_fence_unwrap_first(a->fence, &a_iter), - b_fence = dma_fence_unwrap_first(b->fence, &b_iter); - a_fence || b_fence; ) { - - if (!b_fence) { - add_fence(fences, &index, a_fence); - a_fence = dma_fence_unwrap_next(&a_iter); - - } else if (!a_fence) { - add_fence(fences, &index, b_fence); - b_fence = dma_fence_unwrap_next(&b_iter); - - } else if (a_fence->context < b_fence->context) { - add_fence(fences, &index, a_fence); - a_fence = dma_fence_unwrap_next(&a_iter); - - } else if (b_fence->context < a_fence->context) { - add_fence(fences, &index, b_fence); - b_fence = dma_fence_unwrap_next(&b_iter); - - } else if (__dma_fence_is_later(a_fence->seqno, b_fence->seqno, - a_fence->ops)) { - add_fence(fences, &index, a_fence); - a_fence = dma_fence_unwrap_next(&a_iter); - b_fence = dma_fence_unwrap_next(&b_iter); - - } else { - add_fence(fences, &index, b_fence); - a_fence = dma_fence_unwrap_next(&a_iter); - b_fence = dma_fence_unwrap_next(&b_iter); - } + fence = dma_fence_unwrap_merge(a->fence, b->fence); + if (!fence) { + fput(sync_file->file); + return NULL; } - - if (sync_file_set_fence(sync_file, fences, index) < 0) - goto err_put_fences; - + sync_file->fence = fence; strlcpy(sync_file->user_name, name, sizeof(sync_file->user_name)); return sync_file; - -err_put_fences: - while (index) - dma_fence_put(fences[--index]); - kfree(fences); - -err_free_sync_file: - fput(sync_file->file); - return NULL; }
static int sync_file_release(struct inode *inode, struct file *file) diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index a4d342fef8e0..390de1ee9d35 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -52,4 +52,28 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor); fence = dma_fence_unwrap_next(cursor)) \ if (!dma_fence_is_signaled(fence))
+struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences, + struct dma_fence **fences, + struct dma_fence_unwrap *cursors); + +/** + * dma_fence_unwrap_merge - unwrap and merge fences + * + * All fences given as parameters are unwrapped and merged back together as flat + * dma_fence_array. Useful if multiple containers need to be merged together. + * + * Implemented as a macro to allocate the necessary arrays on the stack and + * account the stack frame size to the caller. + * + * Returns NULL on memory allocation failure, a dma_fence object representing + * all the given fences otherwise. + */ +#define dma_fence_unwrap_merge(...) \ + ({ \ + struct dma_fence *__f[] = { __VA_ARGS__ }; \ + struct dma_fence_unwrap __c[ARRAY_SIZE(__f)]; \ + \ + __dma_fence_unwrap_merge(ARRAY_SIZE(__f), __f, __c); \ + }) + #endif
Greeting,
FYI, we noticed the following commit (built with gcc-11):
commit: a9290ca07a36882b114c3cd9bbd8f66ed47508bd ("[PATCH 4/5] dma-buf: generalize dma_fence unwrap & merging v2") url: https://github.com/intel-lab-lkp/linux/commits/Christian-K-nig/dma-buf-clean... base: git://anongit.freedesktop.org/drm/drm drm-next patch link: https://lore.kernel.org/dri-devel/20220506141009.18047-4-christian.koenig@am...
in testcase: igt version: igt-x86_64-eddc67c5-1_20220430 with following parameters:
group: group-04 ucode: 0xc2
on test machine: 20 threads 1 sockets Commet Lake with 16G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag Reported-by: kernel test robot oliver.sang@intel.com
kern :err : [ 35.911985] BUG: KASAN: slab-out-of-bounds in __dma_fence_unwrap_merge (drivers/dma-buf/dma-fence-unwrap.c:130) kern :err : [ 35.920255] Write of size 8 at addr ffff888105400508 by task api_intel_bb/1309
kern :err : [ 35.930379] CPU: 4 PID: 1309 Comm: api_intel_bb Not tainted 5.18.0-rc5-01118-ga9290ca07a36 #1 kern :err : [ 35.939601] Hardware name: Intel Corporation CometLake Client Platform/CometLake S UDIMM (ERB/CRB), BIOS CMLSFWR1.R00.2212.D00.2104290922 04/29/2021 kern :err : [ 35.953601] Call Trace: kern :err : [ 35.956758] <TASK> kern :err : [ 35.959564] ? __dma_fence_unwrap_merge (drivers/dma-buf/dma-fence-unwrap.c:130) kern :err : [ 35.965157] dump_stack_lvl (lib/dump_stack.c:107 (discriminator 1)) kern :err : [ 35.969534] print_address_description+0x1f/0x200 kern :err : [ 35.975983] ? __dma_fence_unwrap_merge (drivers/dma-buf/dma-fence-unwrap.c:130) kern :err : [ 35.981562] print_report.cold (mm/kasan/report.c:430) kern :err : [ 35.986277] ? _raw_spin_lock_irqsave (arch/x86/include/asm/atomic.h:202 include/linux/atomic/atomic-instrumented.h:543 include/asm-generic/qspinlock.h:82 include/linux/spinlock.h:185 include/linux/spinlock_api_smp.h:111 kernel/locking/spinlock.c:162) kern :err : [ 35.991606] kasan_report (mm/kasan/report.c:162 mm/kasan/report.c:493) kern :err : [ 35.995892] ? __dma_fence_unwrap_merge (drivers/dma-buf/dma-fence-unwrap.c:130) kern :err : [ 36.001474] __dma_fence_unwrap_merge (drivers/dma-buf/dma-fence-unwrap.c:130) kern :err : [ 36.006878] sync_file_merge+0xf7/0x240 kern :err : [ 36.012465] ? _raw_spin_lock (arch/x86/include/asm/atomic.h:202 include/linux/atomic/atomic-instrumented.h:543 include/asm-generic/qspinlock.h:82 include/linux/spinlock.h:185 include/linux/spinlock_api_smp.h:134 kernel/locking/spinlock.c:154) kern :err : [ 36.017088] ? sync_file_create (drivers/dma-buf/sync_file.c:159) kern :err : [ 36.021798] ? __fget_files (arch/x86/include/asm/atomic64_64.h:22 include/linux/atomic/atomic-arch-fallback.h:2293 include/linux/atomic/atomic-arch-fallback.h:2318 include/linux/atomic/atomic-long.h:491 include/linux/atomic/atomic-instrumented.h:1846 fs/file.c:903 fs/file.c:934) kern :err : [ 36.026342] sync_file_ioctl (drivers/dma-buf/sync_file.c:235 drivers/dma-buf/sync_file.c:360) kern :err : [ 36.030966] ? sync_file_ioctl_fence_info (drivers/dma-buf/sync_file.c:355) kern :err : [ 36.036717] ? task_work_run (kernel/task_work.c:167 (discriminator 1)) kern :err : [ 36.041254] __x64_sys_ioctl (fs/ioctl.c:51 fs/ioctl.c:870 fs/ioctl.c:856 fs/ioctl.c:856) kern :err : [ 36.045884] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80) kern :err : [ 36.050166] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:115) kern :err : [ 36.055922] RIP: 0033:0x7fd878745e57 kern :err : [ 36.060203] Code: 00 00 90 48 8b 05 39 a0 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 09 a0 0c 00 f7 d8 64 89 01 48 All code ======== 0: 00 00 add %al,(%rax) 2: 90 nop 3: 48 8b 05 39 a0 0c 00 mov 0xca039(%rip),%rax # 0xca043 a: 64 c7 00 26 00 00 00 movl $0x26,%fs:(%rax) 11: 48 c7 c0 ff ff ff ff mov $0xffffffffffffffff,%rax 18: c3 retq 19: 66 2e 0f 1f 84 00 00 nopw %cs:0x0(%rax,%rax,1) 20: 00 00 00 23: b8 10 00 00 00 mov $0x10,%eax 28: 0f 05 syscall 2a:* 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax <-- trapping instruction 30: 73 01 jae 0x33 32: c3 retq 33: 48 8b 0d 09 a0 0c 00 mov 0xca009(%rip),%rcx # 0xca043 3a: f7 d8 neg %eax 3c: 64 89 01 mov %eax,%fs:(%rcx) 3f: 48 rex.W
Code starting with the faulting instruction =========================================== 0: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax 6: 73 01 jae 0x9 8: c3 retq 9: 48 8b 0d 09 a0 0c 00 mov 0xca009(%rip),%rcx # 0xca019 10: f7 d8 neg %eax 12: 64 89 01 mov %eax,%fs:(%rcx) 15: 48 rex.W kern :err : [ 36.079659] RSP: 002b:00007ffe4d4d2e88 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 kern :err : [ 36.087937] RAX: ffffffffffffffda RBX: 00005558619a1940 RCX: 00007fd878745e57 kern :err : [ 36.095770] RDX: 00007ffe4d4d2e90 RSI: 00000000c0303e03 RDI: 0000000000000008 kern :err : [ 36.103613] RBP: 0000000000000006 R08: 000000000000000f R09: 00005558619a4c30 kern :err : [ 36.111444] R10: 0000000000000006 R11: 0000000000000246 R12: 00005558619a1a00 kern :err : [ 36.119279] R13: 00005558619a46e0 R14: 00007ffe4d4d2ef0 R15: 0000000000000000 kern :err : [ 36.127113] </TASK>
kern :err : [ 36.132209] Allocated by task 1309: kern :warn : [ 36.136405] kasan_save_stack (mm/kasan/common.c:39) kern :warn : [ 36.140943] __kasan_kmalloc (mm/kasan/common.c:45 mm/kasan/common.c:436 mm/kasan/common.c:515 mm/kasan/common.c:524) kern :warn : [ 36.145395] __dma_fence_unwrap_merge (include/linux/slab.h:621 drivers/dma-buf/dma-fence-unwrap.c:81) kern :warn : [ 36.150800] sync_file_merge+0xf7/0x240 kern :warn : [ 36.156386] sync_file_ioctl (drivers/dma-buf/sync_file.c:235 drivers/dma-buf/sync_file.c:360) kern :warn : [ 36.161010] __x64_sys_ioctl (fs/ioctl.c:51 fs/ioctl.c:870 fs/ioctl.c:856 fs/ioctl.c:856) kern :warn : [ 36.165643] do_syscall_64 (arch/x86/entry/common.c:50 arch/x86/entry/common.c:80) kern :warn : [ 36.169921] entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:115)
kern :err : [ 36.177867] The buggy address belongs to the object at ffff888105400500 which belongs to the cache kmalloc-8 of size 8 kern :err : [ 36.191437] The buggy address is located 0 bytes to the right of 8-byte region [ffff888105400500, ffff888105400508)
kern :err : [ 36.206942] The buggy address belongs to the physical page: kern :warn : [ 36.213220] page:00000000c4ee5dee refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff8881054008c0 pfn:0x105400 kern :warn : [ 36.224636] flags: 0x17ffffc0000200(slab|node=0|zone=2|lastcpupid=0x1fffff) kern :warn : [ 36.232305] raw: 0017ffffc0000200 ffffea0004155e80 dead000000000002 ffff888100042280 kern :warn : [ 36.240745] raw: ffff8881054008c0 0000000080660035 00000001ffffffff 0000000000000000 kern :warn : [ 36.249190] page dumped because: kasan: bad access detected
kern :err : [ 36.257659] Memory state around the buggy address: kern :err : [ 36.263155] ffff888105400400: fc fc fa fc fc fc fc fb fc fc fc fc fb fc fc fc kern :err : [ 36.271079] ffff888105400480: fc fb fc fc fc fc fb fc fc fc fc fb fc fc fc fc kern :err : [ 36.279001] >ffff888105400500: 00 fc fc fc fc fb fc fc fc fc fa fc fc fc fc fb kern :err : [ 36.286921] ^ kern :err : [ 36.291117] ffff888105400580: fc fc fc fc fb fc fc fc fc fb fc fc fc fc fb fc kern :err : [ 36.299043] ffff888105400600: fc fc fc fa fc fc fc fc fb fc fc fc fc fb fc fc kern :err : [ 36.306970] ================================================================== kern :warn : [ 36.314953] Disabling lock debugging due to kernel taint user :info : [ 36.321624] [IGT] api_intel_bb: exiting, ret=0 kern :info : [ 36.381966] Console: switching to colour frame buffer device 160x64 kern :info : [ 36.448188] Console: switching to colour dummy device 80x25 user :info : [ 36.454538] [IGT] api_intel_bb: executing user :info : [ 36.459757] [IGT] api_intel_bb: starting subtest blit-noreloc-keep-cache-random user :info : [ 36.471434] [IGT] api_intel_bb: exiting, ret=0 kern :info : [ 36.531917] Console: switching to colour frame buffer device 160x64 kern :info : [ 36.598425] Console: switching to colour dummy device 80x25 user :info : [ 36.604786] [IGT] api_intel_bb: executing user :info : [ 36.609923] [IGT] api_intel_bb: starting subtest blit-noreloc-purge-cache user :info : [ 36.621155] [IGT] api_intel_bb: exiting, ret=0 kern :info : [ 36.681867] Console: switching to colour frame buffer device 160x64 kern :info : [ 36.748514] Console: switching to colour dummy device 80x25 user :info : [ 36.755092] [IGT] api_intel_bb: executing user :info : [ 36.760433] [IGT] api_intel_bb: starting subtest blit-noreloc-purge-cache-random user :info : [ 36.772151] [IGT] api_intel_bb: exiting, ret=0 kern :info : [ 36.831817] Console: switching to colour frame buffer device 160x64 kern :info : [ 36.897995] Console: switching to colour dummy device 80x25 user :info : [ 36.904350] [IGT] api_intel_bb: executing user :info : [ 36.909457] [IGT] api_intel_bb: starting subtest blit-reloc-keep-cache user :info : [ 36.921693] [IGT] api_intel_bb: exiting, ret=0 kern :info : [ 36.981895] Console: switching to colour frame buffer device 160x64 kern :info : [ 37.047892] Console: switching to colour dummy device 80x25 user :info : [ 37.054232] [IGT] api_intel_bb: executing user :info : [ 37.059343] [IGT] api_intel_bb: starting subtest blit-reloc-purge-cache user :info : [ 37.071548] [IGT] api_intel_bb: exiting, ret=0 kern :info : [ 37.131724] Console: switching to colour frame buffer device 160x64 kern :info : [ 37.197818] Console: switching to colour dummy device 80x25 user :info : [ 37.204190] [IGT] api_intel_bb: executing user :info : [ 37.209296] [IGT] api_intel_bb: starting subtest delta-check user :info : [ 37.216856] [IGT] api_intel_bb: exiting, ret=0 user :notice: [ 37.245164] result_service: raw_upload, RESULT_MNT: /internal-lkp-server/result, RESULT_ROOT: /internal-lkp-server/result/igt/group-04-ucode=0xc2/lkp-cml-d02/debian-10.4-x86_64-20200603.cgz/x86_64-rhel-8.3-func/gcc-11/a9290ca07a36882b114c3cd9bbd8f66ed47508bd/1, TMP_RESULT_ROOT: /tmp/lkp/result
user :notice: [ 37.276355] run-job /lkp/jobs/scheduled/lkp-cml-d02/igt-group-04-ucode=0xc2-debian-10.4-x86_64-20200603.cgz-a9290ca07a36882b114c3cd9bbd8f66ed47508bd-20220511-19224-132epq3-1.yaml
kern :info : [ 37.281678] Console: switching to colour frame buffer device 160x64 kern :info : [ 37.366074] Console: switching to colour dummy device 80x25 user :info : [ 37.372429] [IGT] api_intel_bb: executing user :info : [ 37.377548] [IGT] api_intel_bb: starting subtest destroy-bb user :info : [ 37.388923] [IGT] api_intel_bb: exiting, ret=0 kern :info : [ 37.431625] Console: switching to colour frame buffer device 160x64 kern :info : [ 37.497522] Console: switching to colour dummy device 80x25 user :info : [ 37.503871] [IGT] api_intel_bb: executing user :info : [ 37.508999] [IGT] api_intel_bb: starting subtest full-batch user :info : [ 37.516733] [IGT] api_intel_bb: exiting, ret=0 kern :info : [ 37.564907] Console: switching to colour frame buffer device 160x64 kern :info : [ 37.630954] Console: switching to colour dummy device 80x25 user :info : [ 37.637306] [IGT] api_intel_bb: executing user :info : [ 37.642423] [IGT] api_intel_bb: starting subtest intel-bb-blit-none user :notice: [ 38.035871] /usr/bin/wget -q --timeout=1800 --tries=1 --local-encoding=UTF-8 http://internal-lkp-server:80/~lkp/cgi-bin/lkp-jobfile-append-var?job_file=/... -O /dev/null
user :notice: [ 38.069080] target ucode: 0xc2
user :notice: [ 38.075557] current_version: c2, target_version: c2
To reproduce:
git clone https://github.com/intel/lkp-tests.git cd lkp-tests sudo bin/lkp install job.yaml # job file is attached in this email bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test, # please remove ~/.lkp and /lkp dir to run from a clean state.
The unwrap merge function is now intended for this use case.
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/drm_syncobj.c | 57 +++++------------------------------ 1 file changed, 7 insertions(+), 50 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 7e48dcd1bee4..bbad9e4696e7 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -184,6 +184,7 @@ */
#include <linux/anon_inodes.h> +#include <linux/dma-fence-unwrap.h> #include <linux/file.h> #include <linux/fs.h> #include <linux/sched/signal.h> @@ -853,57 +854,12 @@ drm_syncobj_fd_to_handle_ioctl(struct drm_device *dev, void *data, &args->handle); }
- -/* - * Try to flatten a dma_fence_chain into a dma_fence_array so that it can be - * added as timeline fence to a chain again. - */ -static int drm_syncobj_flatten_chain(struct dma_fence **f) -{ - struct dma_fence_chain *chain = to_dma_fence_chain(*f); - struct dma_fence *tmp, **fences; - struct dma_fence_array *array; - unsigned int count; - - if (!chain) - return 0; - - count = 0; - dma_fence_chain_for_each(tmp, &chain->base) - ++count; - - fences = kmalloc_array(count, sizeof(*fences), GFP_KERNEL); - if (!fences) - return -ENOMEM; - - count = 0; - dma_fence_chain_for_each(tmp, &chain->base) - fences[count++] = dma_fence_get(tmp); - - array = dma_fence_array_create(count, fences, - dma_fence_context_alloc(1), - 1, false); - if (!array) - goto free_fences; - - dma_fence_put(*f); - *f = &array->base; - return 0; - -free_fences: - while (count--) - dma_fence_put(fences[count]); - - kfree(fences); - return -ENOMEM; -} - static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, struct drm_syncobj_transfer *args) { struct drm_syncobj *timeline_syncobj = NULL; + struct dma_fence *fence, *tmp; struct dma_fence_chain *chain; - struct dma_fence *fence; int ret;
timeline_syncobj = drm_syncobj_find(file_private, args->dst_handle); @@ -912,13 +868,14 @@ static int drm_syncobj_transfer_to_timeline(struct drm_file *file_private, } ret = drm_syncobj_find_fence(file_private, args->src_handle, args->src_point, args->flags, - &fence); + &tmp); if (ret) goto err_put_timeline;
- ret = drm_syncobj_flatten_chain(&fence); - if (ret) - goto err_free_fence; + fence = dma_fence_unwrap_merge(tmp); + dma_fence_put(tmp); + if (!fence) + goto err_put_timeline;
chain = dma_fence_chain_alloc(); if (!chain) {
I had to send this out once more.
This time with the right mail addresses and a much simplified patch #3.
Christian.
Am 06.05.22 um 16:10 schrieb Christian König:
The selftests, fix the error handling, remove unused functions and stop leaking memory in failed tests.
v2: fix the memory leak correctly.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/st-dma-fence-unwrap.c | 48 +++++++++++---------------- 1 file changed, 19 insertions(+), 29 deletions(-)
diff --git a/drivers/dma-buf/st-dma-fence-unwrap.c b/drivers/dma-buf/st-dma-fence-unwrap.c index 039f016b57be..e20c5a7dcfe4 100644 --- a/drivers/dma-buf/st-dma-fence-unwrap.c +++ b/drivers/dma-buf/st-dma-fence-unwrap.c @@ -4,27 +4,19 @@
- Copyright (C) 2022 Advanced Micro Devices, Inc.
*/ +#include <linux/dma-fence.h> +#include <linux/dma-fence-array.h> +#include <linux/dma-fence-chain.h> #include <linux/dma-fence-unwrap.h> -#if 0 -#include <linux/kernel.h> -#include <linux/kthread.h> -#include <linux/mm.h> -#include <linux/sched/signal.h> -#include <linux/slab.h> -#include <linux/spinlock.h> -#include <linux/random.h> -#endif #include "selftest.h" #define CHAIN_SZ (4 << 10) -static inline struct mock_fence { +struct mock_fence { struct dma_fence base; spinlock_t lock; -} *to_mock_fence(struct dma_fence *f) {
- return container_of(f, struct mock_fence, base);
-} +}; static const char *mock_name(struct dma_fence *f) { @@ -45,7 +37,8 @@ static struct dma_fence *mock_fence(void) return NULL; spin_lock_init(&f->lock);
- dma_fence_init(&f->base, &mock_ops, &f->lock, 0, 0);
- dma_fence_init(&f->base, &mock_ops, &f->lock,
dma_fence_context_alloc(1), 1);
return &f->base; } @@ -59,7 +52,7 @@ static struct dma_fence *mock_array(unsigned int num_fences, ...) fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL); if (!fences)
return NULL;
goto error_put;
va_start(valist, num_fences); for (i = 0; i < num_fences; ++i) @@ -70,13 +63,17 @@ static struct dma_fence *mock_array(unsigned int num_fences, ...) dma_fence_context_alloc(1), 1, false); if (!array)
goto cleanup;
return &array->base;goto error_free;
-cleanup:
- for (i = 0; i < num_fences; ++i)
dma_fence_put(fences[i]);
+error_free: kfree(fences);
+error_put:
- va_start(valist, num_fences);
- for (i = 0; i < num_fences; ++i)
dma_fence_put(va_arg(valist, typeof(*fences)));
- va_end(valist); return NULL; }
@@ -113,7 +110,6 @@ static int sanitycheck(void *arg) if (!chain) return -ENOMEM;
- dma_fence_signal(f); dma_fence_put(chain); return err; }
@@ -154,10 +150,8 @@ static int unwrap_array(void *arg) err = -EINVAL; }
- dma_fence_signal(f1);
- dma_fence_signal(f2); dma_fence_put(array);
- return 0;
- return err; }
static int unwrap_chain(void *arg) @@ -196,10 +190,8 @@ static int unwrap_chain(void *arg) err = -EINVAL; }
- dma_fence_signal(f1);
- dma_fence_signal(f2); dma_fence_put(chain);
- return 0;
- return err; }
static int unwrap_chain_array(void *arg) @@ -242,10 +234,8 @@ static int unwrap_chain_array(void *arg) err = -EINVAL; }
- dma_fence_signal(f1);
- dma_fence_signal(f2); dma_fence_put(chain);
- return 0;
- return err; }
int dma_fence_unwrap(void)
Hey Daniel,
a gentle ping on this here. Those patches come before my drm-exec work, so would be nice if we could get that reviewed first.
Thanks, Christian.
Am 06.05.22 um 16:11 schrieb Christian König:
I had to send this out once more.
This time with the right mail addresses and a much simplified patch #3.
Christian.
Am 06.05.22 um 16:10 schrieb Christian König:
The selftests, fix the error handling, remove unused functions and stop leaking memory in failed tests.
v2: fix the memory leak correctly.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/st-dma-fence-unwrap.c | 48 +++++++++++---------------- 1 file changed, 19 insertions(+), 29 deletions(-)
diff --git a/drivers/dma-buf/st-dma-fence-unwrap.c b/drivers/dma-buf/st-dma-fence-unwrap.c index 039f016b57be..e20c5a7dcfe4 100644 --- a/drivers/dma-buf/st-dma-fence-unwrap.c +++ b/drivers/dma-buf/st-dma-fence-unwrap.c @@ -4,27 +4,19 @@ * Copyright (C) 2022 Advanced Micro Devices, Inc. */ +#include <linux/dma-fence.h> +#include <linux/dma-fence-array.h> +#include <linux/dma-fence-chain.h> #include <linux/dma-fence-unwrap.h> -#if 0 -#include <linux/kernel.h> -#include <linux/kthread.h> -#include <linux/mm.h> -#include <linux/sched/signal.h> -#include <linux/slab.h> -#include <linux/spinlock.h> -#include <linux/random.h> -#endif #include "selftest.h" #define CHAIN_SZ (4 << 10) -static inline struct mock_fence { +struct mock_fence { struct dma_fence base; spinlock_t lock; -} *to_mock_fence(struct dma_fence *f) { - return container_of(f, struct mock_fence, base); -} +}; static const char *mock_name(struct dma_fence *f) { @@ -45,7 +37,8 @@ static struct dma_fence *mock_fence(void) return NULL; spin_lock_init(&f->lock); - dma_fence_init(&f->base, &mock_ops, &f->lock, 0, 0); + dma_fence_init(&f->base, &mock_ops, &f->lock, + dma_fence_context_alloc(1), 1); return &f->base; } @@ -59,7 +52,7 @@ static struct dma_fence *mock_array(unsigned int num_fences, ...) fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL); if (!fences) - return NULL; + goto error_put; va_start(valist, num_fences); for (i = 0; i < num_fences; ++i) @@ -70,13 +63,17 @@ static struct dma_fence *mock_array(unsigned int num_fences, ...) dma_fence_context_alloc(1), 1, false); if (!array) - goto cleanup; + goto error_free; return &array->base; -cleanup: - for (i = 0; i < num_fences; ++i) - dma_fence_put(fences[i]); +error_free: kfree(fences);
+error_put: + va_start(valist, num_fences); + for (i = 0; i < num_fences; ++i) + dma_fence_put(va_arg(valist, typeof(*fences))); + va_end(valist); return NULL; } @@ -113,7 +110,6 @@ static int sanitycheck(void *arg) if (!chain) return -ENOMEM; - dma_fence_signal(f); dma_fence_put(chain); return err; } @@ -154,10 +150,8 @@ static int unwrap_array(void *arg) err = -EINVAL; } - dma_fence_signal(f1); - dma_fence_signal(f2); dma_fence_put(array); - return 0; + return err; } static int unwrap_chain(void *arg) @@ -196,10 +190,8 @@ static int unwrap_chain(void *arg) err = -EINVAL; } - dma_fence_signal(f1); - dma_fence_signal(f2); dma_fence_put(chain); - return 0; + return err; } static int unwrap_chain_array(void *arg) @@ -242,10 +234,8 @@ static int unwrap_chain_array(void *arg) err = -EINVAL; } - dma_fence_signal(f1); - dma_fence_signal(f2); dma_fence_put(chain); - return 0; + return err; } int dma_fence_unwrap(void)
On Fri, May 06, 2022 at 04:10:05PM +0200, Christian König wrote:
The selftests, fix the error handling, remove unused functions and stop leaking memory in failed tests.
v2: fix the memory leak correctly.
Signed-off-by: Christian König christian.koenig@amd.com
I'm still a bit lost on all this (maybe add an explainer why you want to drop dma_fence_signal() - it's just not necessary for test functionality).
But I think it's at least correct now.
Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch
I've seen you've resent it to get intel-gfx-ci to look at it, so assuming that's all fine I think it's now all reviewed and ready for merging. -Daniel
drivers/dma-buf/st-dma-fence-unwrap.c | 48 +++++++++++---------------- 1 file changed, 19 insertions(+), 29 deletions(-)
diff --git a/drivers/dma-buf/st-dma-fence-unwrap.c b/drivers/dma-buf/st-dma-fence-unwrap.c index 039f016b57be..e20c5a7dcfe4 100644 --- a/drivers/dma-buf/st-dma-fence-unwrap.c +++ b/drivers/dma-buf/st-dma-fence-unwrap.c @@ -4,27 +4,19 @@
- Copyright (C) 2022 Advanced Micro Devices, Inc.
*/ +#include <linux/dma-fence.h> +#include <linux/dma-fence-array.h> +#include <linux/dma-fence-chain.h> #include <linux/dma-fence-unwrap.h> -#if 0 -#include <linux/kernel.h> -#include <linux/kthread.h> -#include <linux/mm.h> -#include <linux/sched/signal.h> -#include <linux/slab.h> -#include <linux/spinlock.h> -#include <linux/random.h> -#endif #include "selftest.h" #define CHAIN_SZ (4 << 10) -static inline struct mock_fence { +struct mock_fence { struct dma_fence base; spinlock_t lock; -} *to_mock_fence(struct dma_fence *f) {
- return container_of(f, struct mock_fence, base);
-} +}; static const char *mock_name(struct dma_fence *f) { @@ -45,7 +37,8 @@ static struct dma_fence *mock_fence(void) return NULL; spin_lock_init(&f->lock);
- dma_fence_init(&f->base, &mock_ops, &f->lock, 0, 0);
- dma_fence_init(&f->base, &mock_ops, &f->lock,
dma_fence_context_alloc(1), 1);
return &f->base; } @@ -59,7 +52,7 @@ static struct dma_fence *mock_array(unsigned int num_fences, ...) fences = kcalloc(num_fences, sizeof(*fences), GFP_KERNEL); if (!fences)
return NULL;
goto error_put;
va_start(valist, num_fences); for (i = 0; i < num_fences; ++i) @@ -70,13 +63,17 @@ static struct dma_fence *mock_array(unsigned int num_fences, ...) dma_fence_context_alloc(1), 1, false); if (!array)
goto cleanup;
return &array->base;goto error_free;
-cleanup:
- for (i = 0; i < num_fences; ++i)
dma_fence_put(fences[i]);
+error_free: kfree(fences);
+error_put:
- va_start(valist, num_fences);
- for (i = 0; i < num_fences; ++i)
dma_fence_put(va_arg(valist, typeof(*fences)));
- va_end(valist); return NULL;
} @@ -113,7 +110,6 @@ static int sanitycheck(void *arg) if (!chain) return -ENOMEM;
- dma_fence_signal(f); dma_fence_put(chain); return err;
} @@ -154,10 +150,8 @@ static int unwrap_array(void *arg) err = -EINVAL; }
- dma_fence_signal(f1);
- dma_fence_signal(f2); dma_fence_put(array);
- return 0;
- return err;
} static int unwrap_chain(void *arg) @@ -196,10 +190,8 @@ static int unwrap_chain(void *arg) err = -EINVAL; }
- dma_fence_signal(f1);
- dma_fence_signal(f2); dma_fence_put(chain);
- return 0;
- return err;
} static int unwrap_chain_array(void *arg) @@ -242,10 +234,8 @@ static int unwrap_chain_array(void *arg) err = -EINVAL; }
- dma_fence_signal(f1);
- dma_fence_signal(f2); dma_fence_put(chain);
- return 0;
- return err;
} int dma_fence_unwrap(void) -- 2.25.1
linaro-mm-sig@lists.linaro.org