Re: [PATCH 31/37] drm/i915: Test creation of VMA

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On ke, 2017-01-11 at 21:09 +0000, Chris Wilson wrote:
> Simple test to exercise creation and lookup of VMA within an object.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> +static int vma_create(struct drm_i915_private *i915,
> +		      struct list_head *objects,
> +		      struct list_head *contexts)

create_vmas?

<SNIP>

> +static int igt_vma_create(void *arg)
> +{
> +	I915_SELFTEST_TIMEOUT(end_time);
> +	LIST_HEAD(objects);
> +	LIST_HEAD(contexts);

Looks aesthetically dispelasing ~ messy, LIST_HEADs could go just
before "int err" as they're not that special?

> +	struct drm_i915_private *i915 = arg;
> +	struct drm_i915_gem_object *obj, *on;
> +	struct i915_gem_context *ctx, *cn;
> +	unsigned long num_obj, num_ctx;
> +	unsigned long no, nc;
> +	int err;
> +
> +	no = 0;
> +	for_each_prime_number(num_obj, 8192) {

max_prime

> +		for (; no < num_obj; no++) {
> +			obj = i915_gem_object_create_internal(i915, PAGE_SIZE);
> +			if (IS_ERR(obj))
> +				goto err;
> +
> +			list_add(&obj->batch_pool_link, &objects);

grumble...

> +		}
> +
> +		list_for_each_entry_safe(ctx, cn, &contexts, link)
> +			mock_context_close(ctx);

I'm unsure why exactly here? On the first round it's empty.

> +
> +		nc = 0;
> +		for_each_prime_number(num_ctx, 8192) {
> +			cond_resched();
> +			if (signal_pending(current)) {
> +				err = -EINTR;
> +				goto err;
> +			}

Again something that could be made into a helper maybe, and then used
in many points? if (igt_exit_point_or_so) return/goto...

> +		if (time_after(jiffies, end_time)) {
> +			pr_warn("%s timed out: after %lu objects\n", __func__, no);
> +			break;
> +		}

Helper too, because it's not important for the testing itself.

<SNIP>

> +int i915_vma_mock_selftests(void)
> +{
> +	static const struct i915_subtest tests[] = {
> +		SUBTEST(igt_vma_create),
> +	};
> +	struct drm_i915_private *i915;
> +	int err;
> +
> +	i915 = mock_gem_device();
> +	if (!i915)
> +		return -ENOMEM;
> +
> +	mutex_lock(&i915->drm.struct_mutex);
> +	err = i915_subtests(tests, i915);
> +	mutex_unlock(&i915->drm.struct_mutex);
> +

I'm unclear if i915 should be released, I feel like it should. If not,
consider renaming mock_gem_device into getterish (but not gibberish).

> +	return err;
> +}
> +
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx