On to, 2017-01-19 at 11:41 +0000, Chris Wilson wrote:
> Allocate objects with varying number of pages (which should hopefully
> consist of a mixture of contiguous page chunks and so coalesced sg
> lists) and check that the sg walkers in insert_pages cope.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
<SNIP>
> +static int fill_hole(struct drm_i915_private *i915,
> + struct i915_address_space *vm,
> + u64 hole_start, u64 hole_end,
> + unsigned long end_time)
> +{
> + const u64 hole_size = hole_end - hole_start;
> + struct drm_i915_gem_object *obj;
> + const unsigned long max_pages =
> + min_t(u64, 1 << 20, hole_size/2 >> PAGE_SHIFT);
At least make a comment, why this specific number. It's good to know if
something is a hard limit vs. pulled out of thin air.
> + for_each_prime_number_from(prime, 2, 13) {
SMALL_PRIME_MAX or something similar? Also, what are we targeting with
the selected number, staying below X bytes, N seconds or what?
I think all the tests could be clarified with such comments.
<SNIP>
> + GEM_BUG_ON(!full_size);
This could be in huge_gem_object too?
> + obj = huge_gem_object(i915, PAGE_SIZE, full_size);
> + if (IS_ERR(obj))
> + break;
> +
> + list_add(&obj->batch_pool_link, &objects);
> +
> + /* Align differing sized objects against the edges, and
> + * check we don't walk off into the void when binding
> + * them into the GTT.
> + */
> + for (p = phases; p->name; p++) {
> + u64 flags;
> +
> + flags = p->base;
"offset" and "flags" could be separate variables, just for readability
as this is a test.
> + list_for_each_entry(obj, &objects, batch_pool_link) {
> + vma = i915_vma_instance(obj, vm, NULL);
> + if (IS_ERR(vma))
> + continue;
> +
> + err = i915_vma_pin(vma, 0, 0, flags);
> + if (err) {
> + pr_err("Fill %s pin failed with err=%d on size=%lu pages (prime=%lu), flags=%llx\n", p->name, err, npages, prime, flags);
> + goto err;
> + }
> +
> + i915_vma_unpin(vma);
> +
> + flags += p->step;
> + if (flags < hole_start ||
> + flags > hole_end)
This is also why I'd prefer the variables to be separate, you could
check <= and >= .
> + break;
Make a comment for this block, each previous object is smaller, and
that we rely on the list for ordering.
Even when the lack of comments tried to deceive me, I think I
understood it right;
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
