[PATCH v8 8/8] drm/ttm/tests: Add test cases dependent on fence signaling

[Andi Shyti]

Hi Karolina,

...

> +static void ttm_bo_validate_no_placement_signaled(struct kunit *test)
> +{
> +	const struct ttm_bo_validate_test_case *params = test->param_value;
> +	struct ttm_test_devices *priv = test->priv;
> +	struct ttm_buffer_object *bo;
> +	struct ttm_place *place;
> +	struct ttm_tt *old_tt;
> +	struct ttm_placement *placement;
> +	struct ttm_resource_manager *man;
> +	uint32_t mem_type = TTM_PL_SYSTEM;
> +	enum ttm_bo_type bo_type = ttm_bo_type_device;
> +	struct ttm_operation_ctx ctx = { };
> +	uint32_t size = ALIGN(BO_SIZE, PAGE_SIZE);
> +	uint32_t flags;
> +	int err;
> +
> +	place = ttm_place_kunit_init(test, mem_type, 0);
> +	man = ttm_manager_type(priv->ttm_dev, mem_type);

For next time, I find it difficult to follow all these variables,
it's easier to read

	man = ttm_manager_type(priv->ttm_dev, TTM_PL_SYSTEM);

than

	mem_type = TTM_PL_SYSTEM;
	...
	...
	...
	man = ttm_manager_type(priv->ttm_dev, mem_type);


> +	bo = ttm_bo_kunit_init(test, test->priv, size);
> +	bo->type = bo_type;

same here... the bo_type variable is not giving any value.

	bo->type = ttm_bo_type_device;

is way more readable. You keep doing this all the way and I need
to check everytime what's the value in the declaration :-)

I'm not going to comment on this anymore.

> +	if (params->with_ttm) {
> +		old_tt = priv->ttm_dev->funcs->ttm_tt_create(bo, 0);
> +		ttm_pool_alloc(&priv->ttm_dev->pool, old_tt, &ctx);
> +		bo->ttm = old_tt;
> +	}
> +
> +	err = ttm_resource_alloc(bo, place, &bo->resource);
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +	KUNIT_ASSERT_EQ(test, man->usage, size);
> +
> +	placement = kunit_kzalloc(test, sizeof(*placement), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, placement);
> +
> +	ttm_bo_reserve(bo, false, false, NULL);
> +	err = ttm_bo_validate(bo, placement, &ctx);
> +	dma_resv_unlock(bo->base.resv);

why don't you use here ttm_bo_unreserve()?

> +	KUNIT_EXPECT_EQ(test, err, 0);
> +	KUNIT_ASSERT_EQ(test, man->usage, 0);
> +	KUNIT_ASSERT_NOT_NULL(test, bo->ttm);
> +	KUNIT_EXPECT_EQ(test, ctx.bytes_moved, 0);
> +
> +	if (params->with_ttm) {
> +		flags = bo->ttm->page_flags;
> +
> +		KUNIT_ASSERT_PTR_EQ(test, bo->ttm, old_tt);
> +		KUNIT_ASSERT_FALSE(test, flags & TTM_TT_FLAG_PRIV_POPULATED);
> +		KUNIT_ASSERT_TRUE(test, flags & TTM_TT_FLAG_ZERO_ALLOC);
> +	}
> +
> +	ttm_bo_put(bo);
> +}

...

> +static void ttm_bo_validate_move_fence_signaled(struct kunit *test)
> +{
> +	struct ttm_test_devices *priv = test->priv;
> +	struct ttm_buffer_object *bo;
> +	struct ttm_place *place;
> +	struct ttm_placement *placement;
> +	struct ttm_resource_manager *man;
> +	enum ttm_bo_type bo_type = ttm_bo_type_device;
> +	uint32_t mem_type = TTM_PL_SYSTEM;
> +	struct ttm_operation_ctx ctx = { };
> +	uint32_t size = ALIGN(BO_SIZE, PAGE_SIZE);
> +	int err;
> +
> +	man = ttm_manager_type(priv->ttm_dev, mem_type);
> +	spin_lock_init(&fence_lock);

where are we using the fence_lock here?

> +	man->move = dma_fence_get_stub();
> +
> +	bo = ttm_bo_kunit_init(test, test->priv, size);
> +	bo->type = bo_type;
> +	place = ttm_place_kunit_init(test, mem_type, 0);
> +	placement = ttm_placement_kunit_init(test, place, 1, NULL, 0);
> +
> +	ttm_bo_reserve(bo, false, false, NULL);
> +	err = ttm_bo_validate(bo, placement, &ctx);
> +	dma_resv_unlock(bo->base.resv);
> +
> +	KUNIT_EXPECT_EQ(test, err, 0);
> +	KUNIT_EXPECT_EQ(test, bo->resource->mem_type, mem_type);
> +	KUNIT_EXPECT_EQ(test, ctx.bytes_moved, size);

Do we want to check also bo->ttm here?

> +	ttm_bo_put(bo);
> +	dma_fence_put(man->move);
> +}
> +
> +static const struct ttm_bo_validate_test_case ttm_bo_validate_wait_cases[] = {
> +	{
> +		.description = "Waits for GPU",
> +		.no_gpu_wait = false,
> +	},
> +	{
> +		.description = "Tries to lock straight away",
> +		.no_gpu_wait = true,
> +	},
> +};
> +
> +KUNIT_ARRAY_PARAM(ttm_bo_validate_wait, ttm_bo_validate_wait_cases,
> +		  ttm_bo_validate_case_desc);
> +
> +static int threaded_fence_signal(void *arg)
> +{
> +	struct dma_fence *fence = arg;
> +	int err;
> +
> +	msleep(20);
> +	err = dma_fence_signal(fence);
> +
> +	return err;

if you do here "return dma_fence_signal(fence);" you don't need
for err.

Not a binding review, though, your choice.

Andi

...