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

[Karolina Stolarek <karolina.stolarek@intel.com>]

Hi Andi,

> 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 :-)

The idea was that I'd provide these as parameters, but I limited the
scope of my tests and stick to set values. Also, in some cases, I
defined them because I keep checking for them in assertions, and didn't
want to repeat myself.

> 
> 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()?

That's a good question! I think that unreserve would work here as well.

(...)

>> +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?

Argh, it's a copy-paste mistake, sorry. We don't need it, as we use a
stub fence in man->move. I will delete that.

> 
>> +	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?

Hmm, I think that I covered that case with "normal" testing of
ttm_bo_validate, and kept this one minimal. Here, I'm just making sure
that a signaled move fence doesn't get into a way of BO validation.

> 
>> +	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.

That is true!

> Not a binding review, though, your choice.

You spotted a couple of things I can improve, so I plan to include them 
in the next version, after getting more review comments.

All the best,
Karolina