I left some comments below, but I wonder about the values of these tests
compared to Chris' [1].
Much like the tests from Chris it mostly exercises the dma_fence_* API.
On the drm_syncobj API it's just replace_fence(), get_fence(),
add_point() (that last one cannot fail).
-Lionel
[1] : https://patchwork.freedesktop.org/patch/360865/?series=75743&rev=1
On 10/04/2020 19:51, Venkata Sandeep Dhanalakota wrote:
simple tests using drm api for timeline semaphore.
Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalak...@intel.com>
---
drivers/gpu/drm/selftests/Makefile | 2 +-
.../drm/selftests/drm_timeline_selftests.h | 16 +
.../selftests/test-drm_timeline_semaphore.c | 545 ++++++++++++++++++
3 files changed, 562 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/selftests/drm_timeline_selftests.h
create mode 100644 drivers/gpu/drm/selftests/test-drm_timeline_semaphore.c
diff --git a/drivers/gpu/drm/selftests/Makefile
b/drivers/gpu/drm/selftests/Makefile
index 0856e4b12f70..5bceef7c9d02 100644
--- a/drivers/gpu/drm/selftests/Makefile
+++ b/drivers/gpu/drm/selftests/Makefile
@@ -4,4 +4,4 @@ test-drm_modeset-y := test-drm_modeset_common.o
test-drm_plane_helper.o \
test-drm_damage_helper.o test-drm_dp_mst_helper.o \
test-drm_rect.o
-obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o test-drm_cmdline_parser.o
+obj-$(CONFIG_DRM_DEBUG_SELFTEST) += test-drm_mm.o test-drm_modeset.o
test-drm_cmdline_parser.o test-drm_timeline_semaphore.o
diff --git a/drivers/gpu/drm/selftests/drm_timeline_selftests.h
b/drivers/gpu/drm/selftests/drm_timeline_selftests.h
new file mode 100644
index 000000000000..8922a1eed525
--- /dev/null
+++ b/drivers/gpu/drm/selftests/drm_timeline_selftests.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* List each unit test as selftest(name, function)
+ *
+ * The name is used as both an enum and expanded as igt__name to create
+ * a module parameter. It must be unique and legal for a C identifier.
+ *
+ * Tests are executed in order by igt/drm_timeline_selftests
+ */
+selftest(sanitycheck, igt_sanitycheck) /* keep first (selfcheck for igt) */
+selftest(chainbasic, igt_chainbasic)
+selftest(waitchain, igt_waitchain)
+selftest(signalseqno, igt_signalseqno)
+selftest(waitseqno, igt_waitseqno)
+selftest(addunorder, igt_addunorder)
+selftest(findseqno, igt_findseqno)
+selftest(igt_forward, igt_forward)
diff --git a/drivers/gpu/drm/selftests/test-drm_timeline_semaphore.c
b/drivers/gpu/drm/selftests/test-drm_timeline_semaphore.c
new file mode 100644
index 000000000000..8a964d302e42
--- /dev/null
+++ b/drivers/gpu/drm/selftests/test-drm_timeline_semaphore.c
@@ -0,0 +1,545 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Test cases for the timeline semaphore
+ */
+
+#define pr_fmt(fmt) "drm_tl: " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/random.h>
+#include <linux/vmalloc.h>
+#include <linux/slab.h>
+#include <linux/mm.h>
+#include <linux/kthread.h>
+#include <linux/sched/signal.h>
+
+#include <drm/drm_syncobj.h>
+#include <linux/dma-fence.h>
+#include <linux/dma-fence-chain.h>
+
+#include "../lib/drm_random.h"
+
+#define TESTS "drm_timeline_selftests.h"
+#include "drm_selftest.h"
+
+#define MAX_TIMELINES 64
+#define MAX_THREADS (2 * MAX_TIMELINES)
+
+static struct kmem_cache *slab_timeline;
+static struct kmem_cache *slab_fence_chain;
+
+struct mock_timeline {
+ struct drm_syncobj *syncobj;
+
+ /* cb when base is signalled */
+ struct dma_fence_cb cb;
+ struct dma_fence base;
+ /* lock for dma_fence */
+ spinlock_t lock;
+ u64 point;
+ u32 flags;
+};
+
+struct fence_chain {
+ struct dma_fence_chain chain;
+ struct dma_fence fence;
+ /* cb when fence is signalled */
+ struct dma_fence_cb cb;
+ atomic_t signalers;
+ spinlock_t lock;
+};
+
+struct chain_info {
+ struct fence_chain **chains;
+ int nchains;
+};
+
+static const char *mock_name(struct dma_fence *s)
+{
+ return "timeline";
+}
+
+static void mock_release(struct dma_fence *fence)
+{
+ pr_debug("release %lld\n",fence->seqno);
+}
+
+static const struct dma_fence_ops mock_ops = {
+ .get_driver_name = mock_name,
+ .get_timeline_name = mock_name,
+ .release = mock_release,
+};
+
+static void fence_callback(struct dma_fence *f, struct dma_fence_cb *cb)
+{
+ struct fence_chain *t =
+ container_of(cb, struct fence_chain, cb);
+
+ if (atomic_dec_and_test(&t->signalers))
+ dma_fence_signal(&t->fence);
+}
+
+static void timeline_callback(struct dma_fence *f, struct dma_fence_cb *cb)
+{
+ struct mock_timeline *t =
+ container_of(cb, struct mock_timeline, cb);
+ dma_fence_signal(&t->base);
+}
+
+static struct mock_timeline *timeline(u64 point, u32 flags)
+{
+ struct mock_timeline *t;
+
+ t = kmem_cache_alloc(slab_timeline, GFP_KERNEL | __GFP_ZERO);
+ if (!t)
+ return NULL;
+
+ spin_lock_init(&t->lock);
+ dma_fence_init(&t->base, &mock_ops, &t->lock, 0, point);
+ drm_syncobj_create(&t->syncobj, flags, dma_fence_get(&t->base));
+ t->point = point;
+
+ return t;
+}
+
+static struct fence_chain* fence_chain(struct dma_fence *prev,
+ u64 seqno)
+{
+ struct fence_chain *f;
+
+ f = kmem_cache_alloc(slab_fence_chain, GFP_KERNEL | __GFP_ZERO);
+
+ if (!f)
+ return NULL;
+
+ spin_lock_init(&f->lock);
+ dma_fence_init(&f->fence, &mock_ops,
+ &f->lock, 0, seqno);
+ dma_fence_chain_init(&f->chain,
+ prev,
+ dma_fence_get(&f->fence),
+ seqno);
+
+ return f;
+}
+
+static void allocate_chains(struct chain_info *ci, int count, int start)
+{
+ struct dma_fence *prev_chain = NULL;
+ struct fence_chain **chains;
+ int i;
+
+ ci->chains = kvmalloc_array(count, sizeof(struct fence_chain *),
+ GFP_KERNEL | __GFP_ZERO);
+ if (!ci->chains)
+ return;
+
+ chains = ci->chains;
+ ci->nchains = count;
+ for (i = 0;i < ci->nchains; i++) {
+ chains[i] = fence_chain(prev_chain, start + i);
+ prev_chain = &chains[i]->chain.base;
+ dma_fence_get(prev_chain);
+ }
+}
+
+static void delete_chains(struct chain_info *ci)
+{
+ int i;
+
+ for (i = 0; i < ci->nchains; i++) {
+ dma_fence_release(&ci->chains[i]->fence.refcount);
+ kmem_cache_free(slab_fence_chain, ci->chains[i]);
+ }
+ kvfree(ci->chains);
+}
+
+static int igt_sanitycheck(void *ignored)
+{
+ struct mock_timeline *t;
+ struct dma_fence *f;
+ int err = 0;
+
+ t = timeline(1, 0);
+
+ if (!t)
+ return -ENOMEM;
+
+ dma_fence_signal(&t->base);
+
+ f = drm_syncobj_fence_get(t->syncobj);
+ if (!dma_fence_is_signaled(f))
+ err = -1;
+
+ dma_fence_put(&t->base);
+ drm_syncobj_put(t->syncobj);
+ kmem_cache_free(slab_timeline, t);
+ return err;
+}
+
+static int igt_chainbasic(void *ignored)
+{
+ struct fence_chain *last, *chain;
+ struct dma_fence *first = NULL;
+ struct chain_info ci;
+ int i, count = 10;
+ int err = 0;
+
+ allocate_chains(&ci, count, 0);
+ if (IS_ERR_OR_NULL(ci.chains))
+ return -ENOMEM;
+
+ chain = ci.chains[0];
+ first = &chain->fence;
+
+ for (i = 1; i < count; i++) {
+ chain = ci.chains[i];
+ dma_fence_signal(&chain->fence);
+ last = chain;
+ }
I guess you could add another check here :
if (dma_fence_is_signaled(&last->chain.base))
err = -1;
+ dma_fence_signal(first);
+
+ if (!dma_fence_is_signaled(&last->chain.base))
+ err = -1;
+
+ delete_chains(&ci);
+ return err;
+}
+
+static int igt_findseqno(void *ignored)
+{
+ struct dma_fence *f, *first;
+ struct chain_info ci;
+ int count = 15, start = 3;
+ int err = 0;
+
+ allocate_chains(&ci, count, start);
+ if (IS_ERR_OR_NULL(ci.chains))
+ return -ENOMEM;
+
+ f = &ci.chains[count - 1]->chain.base;
Don't you need to increment the refcount of f?
Otherwise delete_chains() will unref once too many time.
+ first = &ci.chains[0]->chain.base;
+
+ dma_fence_chain_find_seqno(&f, 1);
+ if (f && f != first) {
+ pr_err("Incorrect chain-fence.seqno:%lld reported for completed
seqno:1\n",
+ f->seqno);
+ dma_fence_get(f);
+ err = dma_fence_chain_find_seqno(&f, start);
+ dma_fence_put(f);
+ if (err) {
+ pr_err("Reported %d for finding self!\n", err);
+ err = -EINVAL;
+ }
+ }
+
+ delete_chains(&ci);
+ return err;
+}
+
+static int igt_waitchain(void *ignored)
+{
+ struct fence_chain *last;
+ struct chain_info ci;
+ struct mock_timeline *t;
+ struct dma_fence *f;
+ int count = 10, i;
+ int start = 7;
+ int err = 0;
+
+ t = timeline(start, 0x0);
+
+ if (!t)
+ return -ENOMEM;
+
+ allocate_chains(&ci, count, start + 1);
+ if (IS_ERR_OR_NULL(ci.chains))
+ return -ENOMEM;
+
+ last = ci.chains[count - 1];
+ drm_syncobj_replace_fence(t->syncobj, &last->chain.base);
+
+ f = drm_syncobj_fence_get(t->syncobj);
+ if(dma_fence_is_signaled(f)) {
Coding style doesn't look right. There should be a space after 'if'.
+ err = -1;
+ goto err;
+ }
+
+ for (i = 0; i < count; i++)
+ dma_fence_signal(&ci.chains[i]->fence);
+
+ if(!dma_fence_is_signaled(f))
+ err = -1;
+err:
+ delete_chains(&ci);
+ drm_syncobj_put(t->syncobj);
+ kmem_cache_free(slab_timeline, t);
+ return err;
+}
+
+static int igt_signalseqno(void *ignored)
+{
+ struct fence_chain *wait;
+ struct chain_info ci;
+ struct mock_timeline *t[6];
+ struct dma_fence *f;
+ int i, count = 5;
+ int err = 0;
+
+ allocate_chains(&ci, 1, 0);
+ if (IS_ERR_OR_NULL(ci.chains))
+ return -ENOMEM;
+
+ wait = ci.chains[0];
+
+ for (i = 0;i < count; i++) {
+ t[i] = timeline(i, 0x0);
+ dma_fence_add_callback(&wait->fence,
+ &t[i]->cb,
+ timeline_callback);
+ }
+
+ /* wait for available */
+ for (i = 0; i < count; i++) {
+ f = drm_syncobj_fence_get(t[i]->syncobj);
+ if(dma_fence_is_signaled(f)) {
+ err = -1;
+ goto err;
+ }
+ }
+
+ dma_fence_signal(&wait->fence);
+ for (i = 0; i < count; i++) {
+ f = drm_syncobj_fence_get(t[i]->syncobj);
+ if(!dma_fence_is_signaled(f))
+ err = -1;
+ }
+
+err:
+ for (i = 0;i < count; i++) {
+ drm_syncobj_free(&t[i]->syncobj->refcount);
+ kmem_cache_free(slab_timeline, t[i]);
+ }
+
+ delete_chains(&ci);
+ return err;
+}
+
+static int igt_waitseqno(void *ignored)
+{
+ struct fence_chain *signal;
+ struct mock_timeline *t[6];
+ struct chain_info ci;
+ struct dma_fence *f;
+ int i, count = 5;
+ int err = 0;
+
+ allocate_chains(&ci, 1, 0);
+ if (IS_ERR_OR_NULL(ci.chains))
+ return -ENOMEM;
+
+ signal = ci.chains[0];
+ atomic_set(&signal->signalers, count);
+
+ /* wait for submit */
+ for (i = 0;i < count; i++) {
+ t[i] = timeline(i, 0x0);
+ dma_fence_add_callback(t[i]->syncobj->fence,
+ &signal->cb,
+ fence_callback);
+ }
+
+ for (i = 0;i < count; i++) {
+ if(dma_fence_is_signaled(&signal->chain.base))
+ err = -1;
+
+ f = drm_syncobj_fence_get(t[i]->syncobj);
+ dma_fence_signal(f);
dma_fence_put(f) ?
+ }
+
+ if(!dma_fence_is_signaled(&signal->chain.base))
+ err = -1;
+
+ for (i = 0;i < count; i++) {
+ dma_fence_put(t[i]->syncobj->fence);
+ kmem_cache_free(slab_timeline, t[i]);
+ }
+ delete_chains(&ci);
+ return err;
+}
+
+static int igt_addunorder(void *ignored)
+{
+ struct fence_chain *wait;
+ struct chain_info ci;
+ struct mock_timeline *t;
+ struct dma_fence *f;
+ int err = 0;
+
+ t = timeline(6, 0x0);
+ allocate_chains(&ci, 1, 2);
+ wait = ci.chains[0];
+ if (IS_ERR_OR_NULL(ci.chains))
+ return -ENOMEM;
+
+ drm_syncobj_add_point(t->syncobj, &wait->chain, &wait->fence, 2);
This doesn't look right, because drm_syncobj_add_point() will initialize
the fence-chain using dma_fence_chain_init.
Yet allocate_chains already calls dma_fence_chain_init on the same object.
+
+ dma_fence_signal(&wait->fence);
+ f = drm_syncobj_fence_get(t->syncobj);
+ if (dma_fence_is_signaled(&wait->chain.base)) {
+ err = -1;
+ goto err;
+ }
+
+ dma_fence_signal(f);
+ if (!dma_fence_is_signaled(&wait->chain.base)) {
+ err = -1;
+ goto err;
+ }
+err:
+ delete_chains(&ci);
+ drm_syncobj_put(t->syncobj);
+ kmem_cache_free(slab_timeline, t);
+ return err;
+}
+
+static int __signal_timeline(void *arg)
+{
+ struct mock_timeline *timeline = arg;
+ struct dma_fence *f;
+
+ f = drm_syncobj_fence_get(timeline->syncobj);
+
+ if (f && dma_fence_wait(f, true)){
+ drm_syncobj_put(timeline->syncobj);
+ return -EIO;
+ }
Leaking a ref on the fence?
+
+ return 0;
+}
+
+static int __wait_timeline(void *arg)
+{
+ struct mock_timeline *timeline = arg;
+ struct dma_fence *f;
+
+ f = drm_syncobj_fence_get(timeline->syncobj);
+ dma_fence_signal(f);
Leaking a ref on the fence?
+
+ return 0;
+}
+
+static int igt_forward(void *ignored)
+{
+ struct mock_timeline *s[MAX_TIMELINES], *t[MAX_TIMELINES];
+ struct task_struct *tasks[MAX_THREADS], *tsk;
+ struct chain_info ci, c[MAX_TIMELINES];
+ struct fence_chain *chain, *signaler;
+ int i, err = 0, count = 0;
+ struct dma_fence *last;
+
+ //signaler will signal the points in timelines;
Kernel coding style doesn't all // comments.
+ allocate_chains(&ci, 1, 21);
+ signaler = ci.chains[0];
+
+ dma_fence_get(&signaler->fence);
+ for (i = 0;i < MAX_TIMELINES; i++) {
+ s[i] = timeline(i, 0x0);
+ drm_syncobj_replace_fence(s[i]->syncobj, &signaler->fence);
+ tsk = kthread_run(__signal_timeline, s[i], "signal");
+ if (IS_ERR(tsk)) {
+ err = PTR_ERR(tsk);
+ goto err;
+ }
+ get_task_struct(tsk);
+ yield_to(tsk, true);
+ tasks[count++] = tsk;
+ }
+
+ atomic_set(&signaler->signalers, MAX_TIMELINES);
+ for (i = 0;i < MAX_TIMELINES; i++) {
+
+ allocate_chains(&c[i], 1, i);
+ t[i] = timeline(i, 0x0);
+ chain = c[i].chains[0];
+
+ drm_syncobj_replace_fence(t[i]->syncobj,
+ dma_fence_get(&chain->fence));
drm_syncobj_replace_fence() already calls dma_fence_get() internally.
+ last = &chain->chain.base;
+ dma_fence_add_callback(t[i]->syncobj->fence,
+ &signaler->cb,
+ fence_callback);
+ tsk = kthread_run(__wait_timeline, t[i], "wait");
+ if (IS_ERR(tsk)) {
+ err = PTR_ERR(tsk);
+ goto err;
+ }
+ get_task_struct(tsk);
+ yield_to(tsk, true);
+ tasks[count++] = tsk;
+ }
+
+ dma_fence_wait(last, true);
+ dma_fence_wait(&signaler->fence, true);
+ dma_fence_put(&signaler->fence);
+err:
+ for (i = 0;i < count; i++) {
+ int ret;
+
+ ret = kthread_stop(tasks[i]);
+ if (ret && !err)
+ err = ret;
+ put_task_struct(tasks[i]);
+ }
+
+ for (i = 0; i < MAX_TIMELINES; i++) {
+ chain = c[i].chains[0];
+
+ if (!dma_fence_get_status(&chain->chain.base) ||
+ !dma_fence_get_status(&chain->fence)) {
+ pr_err("Freeing an unsignaled fence\n");
+ err = -1;
+ }
+ delete_chains(&c[i]);
+ kmem_cache_free(slab_timeline, t[i]);
+ kmem_cache_free(slab_timeline, s[i]);
+ }
+ delete_chains(&ci);
+ return err;
+}
+
+#include "drm_selftest.c"
+
+static int __init test_drm_timline_init(void)
+{
+ int err = 0;
+
+ slab_timeline = KMEM_CACHE(mock_timeline,
+ SLAB_TYPESAFE_BY_RCU |
+ SLAB_HWCACHE_ALIGN);
+
+ slab_fence_chain = KMEM_CACHE(fence_chain,
+ SLAB_TYPESAFE_BY_RCU |
+ SLAB_HWCACHE_ALIGN);
+ if (!slab_timeline)
+ return -ENOMEM;
+
+ pr_info("Testing timeline semaphore\n");
+ err = run_selftests(selftests, ARRAY_SIZE(selftests), NULL);
+
+ return err > 0 ? 0 : err;
+}
+
+static void __exit test_drm_timeline_exit(void)
+{
+ kmem_cache_destroy(slab_timeline);
+ kmem_cache_destroy(slab_fence_chain);
+}
+
+module_init(test_drm_timline_init);
+module_exit(test_drm_timeline_exit);
+
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
