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 <ch...@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

Reply via email to