Re: [PATCH v2 26/38] drm/i915: Exercise filling the top/bottom portions of the ppgtt

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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@chris-wilson.co.uk>

<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@linux.intel.com>

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