Re: [PATCH v7 3/8] drm/ttm/tests: Add tests for ttm_bo functions
Hi Karolina,
> +/*
> + * A test case heavily inspired by ww_test_edeadlk_normal(). Checks
> + * if -EDEADLK is properly propagated by ttm_bo_reserve()
> + */
> +static void ttm_bo_reserve_deadlock(struct kunit *test)
> +{
> + struct ttm_buffer_object *bo1, *bo2;
> + struct ww_acquire_ctx ctx1, ctx2;
> + bool interruptible = false;
> + bool no_wait = false;
> + int err;
> +
> + bo1 = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
> + bo2 = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
> +
> + ww_acquire_init(, _ww_class);
> + mutex_lock(>base.resv->lock.base);
> +
> + /* The deadlock will be caught by WW mutex, don't warn about it */
> + lock_release(>base.resv->lock.base.dep_map, 1);
OK... by motidfying the lock map, you avoid lockdep to complain.
> + bo2->base.resv->lock.ctx =
> + ctx2 = ctx1;
> + ctx2.stamp--; /* Make the context holding the lock younger */
> + err = ttm_bo_reserve(bo1, interruptible, no_wait, );
> + KUNIT_ASSERT_EQ(test, err, 0);
> +
> + err = ttm_bo_reserve(bo2, interruptible, no_wait, );
> + KUNIT_ASSERT_EQ(test, err, -EDEADLK);
> +
> + dma_resv_unlock(bo1->base.resv);
> + ww_acquire_fini();
> +}
so... what you're doing here is swapping the lock timing and
catch and report the deadlock... Could you please add some more
comment to better explain the idea behind this test and its
implementation?
Anyway, looks good to me and you can add my:
Reviewed-by: Andi Shyti
Andi
Re: [PATCH v7 3/8] drm/ttm/tests: Add tests for ttm_bo functions
On 21.11.2023 15:29, Christian König wrote:
Am 17.11.23 um 09:49 schrieb Karolina Stolarek:
Test reservation and release of TTM buffer objects. Add tests to check
pin and unpin operations.
Signed-off-by: Karolina Stolarek
Tested-by: Amaranath Somalapuram
From the TTM side it looks good, but I can't judge if the lockdep hack
in the deadlock test is valid or not.
Feel free to add Acked-by: Christian König .
Thanks a lot for taking a look. I'll get someone else to verify if that
hack makes sense.
All the best,
Karolina
Regards,
Christian.
---
drivers/gpu/drm/ttm/tests/Makefile | 1 +
drivers/gpu/drm/ttm/tests/ttm_bo_test.c | 619 ++
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 6 +
3 files changed, 626 insertions(+)
create mode 100644 drivers/gpu/drm/ttm/tests/ttm_bo_test.c
diff --git a/drivers/gpu/drm/ttm/tests/Makefile
b/drivers/gpu/drm/ttm/tests/Makefile
index f570530bbb60..468535f7eed2 100644
--- a/drivers/gpu/drm/ttm/tests/Makefile
+++ b/drivers/gpu/drm/ttm/tests/Makefile
@@ -5,4 +5,5 @@ obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += \
ttm_pool_test.o \
ttm_resource_test.o \
ttm_tt_test.o \
+ ttm_bo_test.o \
ttm_kunit_helpers.o
diff --git a/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
b/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
new file mode 100644
index ..71bca47205ed
--- /dev/null
+++ b/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
@@ -0,0 +1,619 @@
+// SPDX-License-Identifier: GPL-2.0 AND MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#include "ttm_kunit_helpers.h"
+
+#define BO_SIZE SZ_8K
+
+struct ttm_bo_test_case {
+ const char *description;
+ bool interruptible;
+ bool no_wait;
+};
+
+static const struct ttm_bo_test_case ttm_bo_reserved_cases[] = {
+ {
+ .description = "Cannot be interrupted and sleeps",
+ .interruptible = false,
+ .no_wait = false,
+ },
+ {
+ .description = "Cannot be interrupted, locks straight away",
+ .interruptible = false,
+ .no_wait = true,
+ },
+ {
+ .description = "Can be interrupted, sleeps",
+ .interruptible = true,
+ .no_wait = false,
+ },
+};
+
+static void ttm_bo_init_case_desc(const struct ttm_bo_test_case *t,
+ char *desc)
+{
+ strscpy(desc, t->description, KUNIT_PARAM_DESC_SIZE);
+}
+
+KUNIT_ARRAY_PARAM(ttm_bo_reserve, ttm_bo_reserved_cases,
ttm_bo_init_case_desc);
+
+static void ttm_bo_reserve_optimistic_no_ticket(struct kunit *test)
+{
+ const struct ttm_bo_test_case *params = test->param_value;
+ struct ttm_buffer_object *bo;
+ int err;
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ err = ttm_bo_reserve(bo, params->interruptible, params->no_wait,
NULL);
+ KUNIT_ASSERT_EQ(test, err, 0);
+
+ dma_resv_unlock(bo->base.resv);
+}
+
+static void ttm_bo_reserve_locked_no_sleep(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ bool interruptible = false;
+ bool no_wait = true;
+ int err;
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ /* Let's lock it beforehand */
+ dma_resv_lock(bo->base.resv, NULL);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, NULL);
+ dma_resv_unlock(bo->base.resv);
+
+ KUNIT_ASSERT_EQ(test, err, -EBUSY);
+}
+
+static void ttm_bo_reserve_no_wait_ticket(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ struct ww_acquire_ctx ctx;
+ bool interruptible = false;
+ bool no_wait = true;
+ int err;
+
+ ww_acquire_init(, _ww_class);
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, );
+ KUNIT_ASSERT_EQ(test, err, -EBUSY);
+
+ ww_acquire_fini();
+}
+
+static void ttm_bo_reserve_double_resv(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ struct ww_acquire_ctx ctx;
+ bool interruptible = false;
+ bool no_wait = false;
+ int err;
+
+ ww_acquire_init(, _ww_class);
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, );
+ KUNIT_ASSERT_EQ(test, err, 0);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, );
+
+ dma_resv_unlock(bo->base.resv);
+ ww_acquire_fini();
+
+ KUNIT_ASSERT_EQ(test, err, -EALREADY);
+}
+
+/*
+ * A test case heavily inspired by ww_test_edeadlk_normal(). Checks
+ * if -EDEADLK is properly propagated by ttm_bo_reserve()
+ */
+static void ttm_bo_reserve_deadlock(struct kunit *test)
+{
+ struct ttm_buffer_object *bo1, *bo2;
+ struct ww_acquire_ctx ctx1, ctx2;
+ bool interruptible = false;
+ bool no_wait = false;
+ int err;
+
+ bo1 = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+ bo2 = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ ww_acquire_init(, _ww_class);
+
Re: [PATCH v7 3/8] drm/ttm/tests: Add tests for ttm_bo functions
Am 17.11.23 um 09:49 schrieb Karolina Stolarek:
Test reservation and release of TTM buffer objects. Add tests to check
pin and unpin operations.
Signed-off-by: Karolina Stolarek
Tested-by: Amaranath Somalapuram
From the TTM side it looks good, but I can't judge if the lockdep hack
in the deadlock test is valid or not.
Feel free to add Acked-by: Christian König .
Regards,
Christian.
---
drivers/gpu/drm/ttm/tests/Makefile| 1 +
drivers/gpu/drm/ttm/tests/ttm_bo_test.c | 619 ++
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 6 +
3 files changed, 626 insertions(+)
create mode 100644 drivers/gpu/drm/ttm/tests/ttm_bo_test.c
diff --git a/drivers/gpu/drm/ttm/tests/Makefile
b/drivers/gpu/drm/ttm/tests/Makefile
index f570530bbb60..468535f7eed2 100644
--- a/drivers/gpu/drm/ttm/tests/Makefile
+++ b/drivers/gpu/drm/ttm/tests/Makefile
@@ -5,4 +5,5 @@ obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += \
ttm_pool_test.o \
ttm_resource_test.o \
ttm_tt_test.o \
+ttm_bo_test.o \
ttm_kunit_helpers.o
diff --git a/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
b/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
new file mode 100644
index ..71bca47205ed
--- /dev/null
+++ b/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
@@ -0,0 +1,619 @@
+// SPDX-License-Identifier: GPL-2.0 AND MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#include "ttm_kunit_helpers.h"
+
+#define BO_SIZESZ_8K
+
+struct ttm_bo_test_case {
+ const char *description;
+ bool interruptible;
+ bool no_wait;
+};
+
+static const struct ttm_bo_test_case ttm_bo_reserved_cases[] = {
+ {
+ .description = "Cannot be interrupted and sleeps",
+ .interruptible = false,
+ .no_wait = false,
+ },
+ {
+ .description = "Cannot be interrupted, locks straight away",
+ .interruptible = false,
+ .no_wait = true,
+ },
+ {
+ .description = "Can be interrupted, sleeps",
+ .interruptible = true,
+ .no_wait = false,
+ },
+};
+
+static void ttm_bo_init_case_desc(const struct ttm_bo_test_case *t,
+ char *desc)
+{
+ strscpy(desc, t->description, KUNIT_PARAM_DESC_SIZE);
+}
+
+KUNIT_ARRAY_PARAM(ttm_bo_reserve, ttm_bo_reserved_cases,
ttm_bo_init_case_desc);
+
+static void ttm_bo_reserve_optimistic_no_ticket(struct kunit *test)
+{
+ const struct ttm_bo_test_case *params = test->param_value;
+ struct ttm_buffer_object *bo;
+ int err;
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ err = ttm_bo_reserve(bo, params->interruptible, params->no_wait, NULL);
+ KUNIT_ASSERT_EQ(test, err, 0);
+
+ dma_resv_unlock(bo->base.resv);
+}
+
+static void ttm_bo_reserve_locked_no_sleep(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ bool interruptible = false;
+ bool no_wait = true;
+ int err;
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ /* Let's lock it beforehand */
+ dma_resv_lock(bo->base.resv, NULL);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, NULL);
+ dma_resv_unlock(bo->base.resv);
+
+ KUNIT_ASSERT_EQ(test, err, -EBUSY);
+}
+
+static void ttm_bo_reserve_no_wait_ticket(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ struct ww_acquire_ctx ctx;
+ bool interruptible = false;
+ bool no_wait = true;
+ int err;
+
+ ww_acquire_init(, _ww_class);
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, );
+ KUNIT_ASSERT_EQ(test, err, -EBUSY);
+
+ ww_acquire_fini();
+}
+
+static void ttm_bo_reserve_double_resv(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ struct ww_acquire_ctx ctx;
+ bool interruptible = false;
+ bool no_wait = false;
+ int err;
+
+ ww_acquire_init(, _ww_class);
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, );
+ KUNIT_ASSERT_EQ(test, err, 0);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, );
+
+ dma_resv_unlock(bo->base.resv);
+ ww_acquire_fini();
+
+ KUNIT_ASSERT_EQ(test, err, -EALREADY);
+}
+
+/*
+ * A test case heavily inspired by ww_test_edeadlk_normal(). Checks
+ * if -EDEADLK is properly propagated by ttm_bo_reserve()
+ */
+static void ttm_bo_reserve_deadlock(struct kunit *test)
+{
+ struct ttm_buffer_object *bo1, *bo2;
+ struct ww_acquire_ctx ctx1, ctx2;
+ bool interruptible = false;
+ bool no_wait = false;
+ int err;
+
+ bo1 = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+ bo2 = ttm_bo_kunit_init(test,
[PATCH v7 3/8] drm/ttm/tests: Add tests for ttm_bo functions
Test reservation and release of TTM buffer objects. Add tests to check
pin and unpin operations.
Signed-off-by: Karolina Stolarek
Tested-by: Amaranath Somalapuram
---
drivers/gpu/drm/ttm/tests/Makefile| 1 +
drivers/gpu/drm/ttm/tests/ttm_bo_test.c | 619 ++
drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 6 +
3 files changed, 626 insertions(+)
create mode 100644 drivers/gpu/drm/ttm/tests/ttm_bo_test.c
diff --git a/drivers/gpu/drm/ttm/tests/Makefile
b/drivers/gpu/drm/ttm/tests/Makefile
index f570530bbb60..468535f7eed2 100644
--- a/drivers/gpu/drm/ttm/tests/Makefile
+++ b/drivers/gpu/drm/ttm/tests/Makefile
@@ -5,4 +5,5 @@ obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += \
ttm_pool_test.o \
ttm_resource_test.o \
ttm_tt_test.o \
+ttm_bo_test.o \
ttm_kunit_helpers.o
diff --git a/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
b/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
new file mode 100644
index ..71bca47205ed
--- /dev/null
+++ b/drivers/gpu/drm/ttm/tests/ttm_bo_test.c
@@ -0,0 +1,619 @@
+// SPDX-License-Identifier: GPL-2.0 AND MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#include "ttm_kunit_helpers.h"
+
+#define BO_SIZESZ_8K
+
+struct ttm_bo_test_case {
+ const char *description;
+ bool interruptible;
+ bool no_wait;
+};
+
+static const struct ttm_bo_test_case ttm_bo_reserved_cases[] = {
+ {
+ .description = "Cannot be interrupted and sleeps",
+ .interruptible = false,
+ .no_wait = false,
+ },
+ {
+ .description = "Cannot be interrupted, locks straight away",
+ .interruptible = false,
+ .no_wait = true,
+ },
+ {
+ .description = "Can be interrupted, sleeps",
+ .interruptible = true,
+ .no_wait = false,
+ },
+};
+
+static void ttm_bo_init_case_desc(const struct ttm_bo_test_case *t,
+ char *desc)
+{
+ strscpy(desc, t->description, KUNIT_PARAM_DESC_SIZE);
+}
+
+KUNIT_ARRAY_PARAM(ttm_bo_reserve, ttm_bo_reserved_cases,
ttm_bo_init_case_desc);
+
+static void ttm_bo_reserve_optimistic_no_ticket(struct kunit *test)
+{
+ const struct ttm_bo_test_case *params = test->param_value;
+ struct ttm_buffer_object *bo;
+ int err;
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ err = ttm_bo_reserve(bo, params->interruptible, params->no_wait, NULL);
+ KUNIT_ASSERT_EQ(test, err, 0);
+
+ dma_resv_unlock(bo->base.resv);
+}
+
+static void ttm_bo_reserve_locked_no_sleep(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ bool interruptible = false;
+ bool no_wait = true;
+ int err;
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ /* Let's lock it beforehand */
+ dma_resv_lock(bo->base.resv, NULL);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, NULL);
+ dma_resv_unlock(bo->base.resv);
+
+ KUNIT_ASSERT_EQ(test, err, -EBUSY);
+}
+
+static void ttm_bo_reserve_no_wait_ticket(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ struct ww_acquire_ctx ctx;
+ bool interruptible = false;
+ bool no_wait = true;
+ int err;
+
+ ww_acquire_init(, _ww_class);
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, );
+ KUNIT_ASSERT_EQ(test, err, -EBUSY);
+
+ ww_acquire_fini();
+}
+
+static void ttm_bo_reserve_double_resv(struct kunit *test)
+{
+ struct ttm_buffer_object *bo;
+ struct ww_acquire_ctx ctx;
+ bool interruptible = false;
+ bool no_wait = false;
+ int err;
+
+ ww_acquire_init(, _ww_class);
+
+ bo = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, );
+ KUNIT_ASSERT_EQ(test, err, 0);
+
+ err = ttm_bo_reserve(bo, interruptible, no_wait, );
+
+ dma_resv_unlock(bo->base.resv);
+ ww_acquire_fini();
+
+ KUNIT_ASSERT_EQ(test, err, -EALREADY);
+}
+
+/*
+ * A test case heavily inspired by ww_test_edeadlk_normal(). Checks
+ * if -EDEADLK is properly propagated by ttm_bo_reserve()
+ */
+static void ttm_bo_reserve_deadlock(struct kunit *test)
+{
+ struct ttm_buffer_object *bo1, *bo2;
+ struct ww_acquire_ctx ctx1, ctx2;
+ bool interruptible = false;
+ bool no_wait = false;
+ int err;
+
+ bo1 = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+ bo2 = ttm_bo_kunit_init(test, test->priv, BO_SIZE);
+
+ ww_acquire_init(, _ww_class);
+ mutex_lock(>base.resv->lock.base);
+
+ /* The deadlock will be caught by WW mutex, don't warn about it */
+ lock_release(>base.resv->lock.base.dep_map, 1);
+
+
