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

[Chris Wilson <chris@chris-wilson.co.uk>]

On Fri, Jan 13, 2017 at 02:28:54PM +0200, Joonas Lahtinen wrote:
> 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

s/8192/ULONG_MAX/ are you serious! ;)

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

Just put on the rose tinted glasses.
 
> > +		}
> > +
> > +		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.

Just the order in which I wrote the test and there was no reason to move
it...

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

Already igt_timeout(timeout, fmt, ...)

I've been contemplating putting the cond_resched/interruptible check
here. The complaint above is good enough justification.

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

It was a mistake, i.e. I forgot drm_dev_unref(&i915->drm) here.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx