Awsome, thanks for adding the tests!
Got to say I'm not that familiar with the self-test framework idioms,
but from my perspective patches 2 and 3 are
Reviewed-by: Bas Nieuwenhuizen <bas@xxxxxxxxxxxxxxxxxxx>
as well.
On Tue, Jul 14, 2020 at 10:06 PM Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
>
> While sw_sync is purely a debug facility for userspace to create fences
> and timelines it can control, nevertheless it has some tricky locking
> semantics of its own. In particular, Bas Nieuwenhuize reported that we
> had reintroduced a deadlock if a signal callback attempted to destroy
> the fence. So let's add a few trivial selftests to make sure that once
> fixed again, it stays fixed.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Bas Nieuwenhuizen <bas@xxxxxxxxxxxxxxxxxxx>
> ---
> drivers/dma-buf/Makefile | 3 +-
> drivers/dma-buf/selftests.h | 1 +
> drivers/dma-buf/st-sw_sync.c | 279 +++++++++++++++++++++++++++++++++++
> drivers/dma-buf/sw_sync.c | 39 +++++
> drivers/dma-buf/sync_debug.h | 8 +
> 5 files changed, 329 insertions(+), 1 deletion(-)
> create mode 100644 drivers/dma-buf/st-sw_sync.c
>
> diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
> index 995e05f609ff..9be4d4611609 100644
> --- a/drivers/dma-buf/Makefile
> +++ b/drivers/dma-buf/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_UDMABUF) += udmabuf.o
> dmabuf_selftests-y := \
> selftest.o \
> st-dma-fence.o \
> - st-dma-fence-chain.o
> + st-dma-fence-chain.o \
> + st-sw_sync.o
>
> obj-$(CONFIG_DMABUF_SELFTESTS) += dmabuf_selftests.o
> diff --git a/drivers/dma-buf/selftests.h b/drivers/dma-buf/selftests.h
> index bc8cea67bf1e..232499a24872 100644
> --- a/drivers/dma-buf/selftests.h
> +++ b/drivers/dma-buf/selftests.h
> @@ -12,3 +12,4 @@
> selftest(sanitycheck, __sanitycheck__) /* keep first (igt selfcheck) */
> selftest(dma_fence, dma_fence)
> selftest(dma_fence_chain, dma_fence_chain)
> +selftest(sw_sync, sw_sync)
> diff --git a/drivers/dma-buf/st-sw_sync.c b/drivers/dma-buf/st-sw_sync.c
> new file mode 100644
> index 000000000000..3a21e7717df7
> --- /dev/null
> +++ b/drivers/dma-buf/st-sw_sync.c
> @@ -0,0 +1,279 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2020 Intel Corporation
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/dma-fence.h>
> +#include <linux/kernel.h>
> +#include <linux/kthread.h>
> +#include <linux/sched/signal.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "sync_debug.h"
> +#include "selftest.h"
> +
> +static int sanitycheck(void *arg)
> +{
> + struct sync_timeline *tl;
> + struct dma_fence *f;
> + int err = -ENOMEM;
> +
> + /* Quick check we can create the timeline and syncpt */
> +
> + tl = st_sync_timeline_create("mock");
> + if (!tl)
> + return -ENOMEM;
> +
> + f = st_sync_pt_create(tl, 1);
> + if (!f)
> + goto out;
> +
> + dma_fence_signal(f);
> + dma_fence_put(f);
> +
> + err = 0;
> +out:
> + st_sync_timeline_put(tl);
> + return 0;
> +}
> +
> +static int signal(void *arg)
> +{
> + struct sync_timeline *tl;
> + struct dma_fence *f;
> + int err = -EINVAL;
> +
> + /* Check that the syncpt fence is signaled when the timeline advances */
> +
> + tl = st_sync_timeline_create("mock");
> + if (!tl)
> + return -ENOMEM;
> +
> + f = st_sync_pt_create(tl, 1);
> + if (!f) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + if (dma_fence_is_signaled(f)) {
> + pr_err("syncpt:%lld signaled too early\n", f->seqno);
> + goto out_fence;
> + }
> +
> + st_sync_timeline_signal(tl, 1);
> +
> + if (!dma_fence_is_signaled(f)) {
> + pr_err("syncpt:%lld not signaled after increment\n", f->seqno);
> + goto out_fence;
> + }
> +
> + err = 0;
> +out_fence:
> + dma_fence_signal(f);
> + dma_fence_put(f);
> +out:
> + st_sync_timeline_put(tl);
> + return 0;
> +}
> +
> +struct cb_destroy {
> + struct dma_fence_cb cb;
> + struct dma_fence *f;
> +};
> +
> +static void cb_destroy(struct dma_fence *fence, struct dma_fence_cb *_cb)
> +{
> + struct cb_destroy *cb = container_of(_cb, typeof(*cb), cb);
> +
> + pr_info("syncpt:%llx destroying syncpt:%llx\n",
> + fence->seqno, cb->f->seqno);
> + dma_fence_put(cb->f);
> + cb->f = NULL;
> +}
> +
> +static int cb_autodestroy(void *arg)
> +{
> + struct sync_timeline *tl;
> + struct cb_destroy cb;
> + int err = -EINVAL;
> +
> + /* Check that we can drop the final syncpt reference from a callback */
> +
> + tl = st_sync_timeline_create("mock");
> + if (!tl)
> + return -ENOMEM;
> +
> + cb.f = st_sync_pt_create(tl, 1);
> + if (!cb.f) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + if (dma_fence_add_callback(cb.f, &cb.cb, cb_destroy)) {
> + pr_err("syncpt:%lld signaled before increment\n", cb.f->seqno);
> + goto out;
> + }
> +
> + st_sync_timeline_signal(tl, 1);
> + if (cb.f) {
> + pr_err("syncpt:%lld callback not run\n", cb.f->seqno);
> + dma_fence_put(cb.f);
> + goto out;
> + }
> +
> + err = 0;
> +out:
> + st_sync_timeline_put(tl);
> + return 0;
> +}
> +
> +static int cb_destroy_12(void *arg)
> +{
> + struct sync_timeline *tl;
> + struct cb_destroy cb;
> + struct dma_fence *f;
> + int err = -EINVAL;
> +
> + /* Check that we can drop some other syncpt reference from a callback */
> +
> + tl = st_sync_timeline_create("mock");
> + if (!tl)
> + return -ENOMEM;
> +
> + f = st_sync_pt_create(tl, 1);
> + if (!f) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + cb.f = st_sync_pt_create(tl, 2);
> + if (!f) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) {
> + pr_err("syncpt:%lld signaled before increment\n", f->seqno);
> + goto out;
> + }
> +
> + st_sync_timeline_signal(tl, 1);
> + if (cb.f) {
> + pr_err("syncpt:%lld callback not run\n", f->seqno);
> + dma_fence_put(cb.f);
> + goto out_fence;
> + }
> +
> + err = 0;
> +out_fence:
> + dma_fence_put(f);
> +out:
> + st_sync_timeline_put(tl);
> + return 0;
> +}
> +
> +static int cb_destroy_21(void *arg)
> +{
> + struct sync_timeline *tl;
> + struct cb_destroy cb;
> + struct dma_fence *f;
> + int err = -EINVAL;
> +
> + /* Check that we can drop an earlier syncpt reference from a callback */
> +
> + tl = st_sync_timeline_create("mock");
> + if (!tl)
> + return -ENOMEM;
> +
> + cb.f = st_sync_pt_create(tl, 1);
> + if (!f) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + f = st_sync_pt_create(tl, 2);
> + if (!f) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) {
> + pr_err("syncpt:%lld signaled before increment\n", f->seqno);
> + goto out;
> + }
> +
> + st_sync_timeline_signal(tl, 2);
> + if (cb.f) {
> + pr_err("syncpt:%lld callback not run\n", f->seqno);
> + dma_fence_put(cb.f);
> + goto out_fence;
> + }
> +
> + err = 0;
> +out_fence:
> + dma_fence_put(f);
> +out:
> + st_sync_timeline_put(tl);
> + return 0;
> +}
> +
> +static int cb_destroy_22(void *arg)
> +{
> + struct sync_timeline *tl;
> + struct cb_destroy cb;
> + struct dma_fence *f;
> + int err = -EINVAL;
> +
> + /* Check that we can drop the later syncpt reference from a callback */
> +
> + tl = st_sync_timeline_create("mock");
> + if (!tl)
> + return -ENOMEM;
> +
> + f = st_sync_pt_create(tl, 1);
> + if (!f) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + cb.f = st_sync_pt_create(tl, 2);
> + if (!f) {
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + if (dma_fence_add_callback(f, &cb.cb, cb_destroy)) {
> + pr_err("syncpt:%lld signaled before increment\n", f->seqno);
> + goto out;
> + }
> +
> + st_sync_timeline_signal(tl, 2);
> + if (cb.f) {
> + pr_err("syncpt:%lld callback not run\n", f->seqno);
> + dma_fence_put(cb.f);
> + goto out_fence;
> + }
> +
> + err = 0;
> +out_fence:
> + dma_fence_put(f);
> +out:
> + st_sync_timeline_put(tl);
> + return 0;
> +}
> +
> +int sw_sync(void)
> +{
> + static const struct subtest tests[] = {
> + SUBTEST(sanitycheck),
> + SUBTEST(signal),
> + SUBTEST(cb_autodestroy),
> + SUBTEST(cb_destroy_12),
> + SUBTEST(cb_destroy_21),
> + SUBTEST(cb_destroy_22),
> + };
> +
> + return subtests(tests, NULL);
> +}
> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c
> index 116dad6c7905..7c229f4a6b56 100644
> --- a/drivers/dma-buf/sw_sync.c
> +++ b/drivers/dma-buf/sw_sync.c
> @@ -426,3 +426,42 @@ const struct file_operations sw_sync_debugfs_fops = {
> .unlocked_ioctl = sw_sync_ioctl,
> .compat_ioctl = compat_ptr_ioctl,
> };
> +
> +#if IS_ENABLED(CONFIG_DMABUF_SELFTESTS)
> +struct sync_timeline *st_sync_timeline_create(const char *name)
> +{
> + return sync_timeline_create(name);
> +}
> +EXPORT_SYMBOL_GPL(st_sync_timeline_create);
> +
> +void st_sync_timeline_get(struct sync_timeline *tl)
> +{
> + sync_timeline_get(tl);
> +}
> +EXPORT_SYMBOL_GPL(st_sync_timeline_get);
> +
> +void st_sync_timeline_put(struct sync_timeline *tl)
> +{
> + sync_timeline_put(tl);
> +}
> +EXPORT_SYMBOL_GPL(st_sync_timeline_put);
> +
> +void st_sync_timeline_signal(struct sync_timeline *tl, unsigned int inc)
> +{
> + sync_timeline_signal(tl, inc);
> +}
> +EXPORT_SYMBOL_GPL(st_sync_timeline_signal);
> +
> +struct dma_fence *
> +st_sync_pt_create(struct sync_timeline *tl, unsigned int seqno)
> +{
> + struct sync_pt *pt;
> +
> + pt = sync_pt_create(tl, seqno);
> + if (!pt)
> + return NULL;
> +
> + return &pt->base;
> +}
> +EXPORT_SYMBOL_GPL(st_sync_pt_create);
> +#endif
> diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
> index fd073fc32329..7c96d9a2c02d 100644
> --- a/drivers/dma-buf/sync_debug.h
> +++ b/drivers/dma-buf/sync_debug.h
> @@ -71,4 +71,12 @@ void sync_timeline_debug_remove(struct sync_timeline *obj);
> void sync_file_debug_add(struct sync_file *fence);
> void sync_file_debug_remove(struct sync_file *fence);
>
> +struct sync_timeline *st_sync_timeline_create(const char *name);
> +void st_sync_timeline_get(struct sync_timeline *tl);
> +void st_sync_timeline_put(struct sync_timeline *tl);
> +void st_sync_timeline_signal(struct sync_timeline *tl, unsigned int inc);
> +
> +struct dma_fence *
> +st_sync_pt_create(struct sync_timeline *tl, unsigned int seqno);
> +
> #endif /* _LINUX_SYNC_H */
> --
> 2.20.1
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
