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@xxxxxxxxxxxxxxxxxx>
>
> <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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
