On ke, 2017-01-11 at 21:09 +0000, Chris Wilson wrote:
> Mock testing to ensure we can create and lookup partial VMA.
> 
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>

<SNIP>

> +static int igt_vma_partial(void *arg)
> +{
> +     struct drm_i915_private *i915 = arg;
> +     const unsigned int npages = 1021; /* prime! */

Yes, but why this prime?

<SNIP>

> +     for (loop = 0; loop <= 1; loop++) { /* exercise both create/lookup */

create_not_lookup, no need for comments

> +             unsigned int count, nvma;
> +
> +             nvma = loop;

nvma = create_not_lookup ? 0 : 1; would make this less cryptic to read.
Compiler shall optimize then.

> +             for_each_prime_number_from(sz, 1, npages) {
> +                     for_each_prime_number_from(offset, 0, npages - sz) {
> +                             struct i915_ggtt_view view;
> +
> +                             view.type = I915_GGTT_VIEW_PARTIAL;
> +                             view.partial.offset_size =
> +                                     offset << INTEL_PARTIAL_SIZE_BITS | (sz 
> - 1);

Could initialize in named manner when declaring?

> +
> +                             if (sz == npages)
> +                                     view.type = I915_GGTT_VIEW_NORMAL;
> +
> +                             vma = i915_gem_obj_lookup_or_create_vma(obj, 
> &i915->ggtt.base, &view);
> +                             if (IS_ERR(vma)) {
> +                                     err = PTR_ERR(vma);
> +                                     goto err_object;
> +                             }
> +
> +                             if (!i915_vma_is_ggtt(vma)) {
> +                                     pr_err("VMA is not in the GGTT!\n");
> +                                     err = -EINVAL;
> +                                     goto err_object;
> +                             }
> +
> +                             err = i915_vma_pin(vma, 0, 0, PIN_GLOBAL);
> +                             if (err)
> +                                     goto err_object;
> +
> +                             if (vma->size != sz*PAGE_SIZE) {

Why do you delay this check after pinning?

> +                             if (view.type != I915_GGTT_VIEW_NORMAL) {
> +                                     if (memcmp(&vma->ggtt_view, &view, 
> sizeof(view))) {
> +                                             pr_err("VMA mismatch upon 
> creation!\n");

Maybe here.

> +                                             err = -EINVAL;
> +                                             goto err_object;
> +                                     }
> +
> +                                     if (vma->pages == obj->mm.pages) {
> +                                             pr_err("VMA using unrotated 
> object pages!\n");

At least here speak of "partial VMA", not rotated/regular VMA.

> +                             i915_vma_unpin(vma);
> +                             nvma++;

num_vma would not be that much worse to type?

<SNIP>

From here;

> +             if (vma->size != obj->base.size) {
> +                     pr_err("VMA is wrong size, expected %lu, found %llu\n",
> +                            sz*PAGE_SIZE, vma->size);
> +                     err = -EINVAL;
> +                     goto err_object;
> +             }
> +
> +             if (vma->node.size < vma->size) {
> +                     pr_err("VMA binding too small, expected %llu, found 
> %llu\n",
> +                            vma->size, vma->node.size);
> +                     err = -EINVAL;
> +                     goto err_object;
> +             }
> +
> +             if (vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL) {
> +                     pr_err("Not the normal ggtt view! Found %d\n",
> +                            vma->ggtt_view.type);
> +                     err = -EINVAL;
> +                     goto err_object;
> +             }
> +
> +             if (vma->pages != obj->mm.pages) {
> +                     pr_err("VMA not using object pages!\n");
> +                     err = -EINVAL;
> +                     goto err_object;
> +             }

One big helper function?

Regards, Joonas
-- 
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