Re: [PATCH 27/37] drm/i915: Exercising filling the top/bottom portions of the ppgtt

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

On ke, 2017-01-11 at 21:09 +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>

>  struct drm_i915_gem_object *
>  i915_gem_object_create_internal(struct drm_i915_private *dev_priv,
> -				unsigned int size);
> +				unsigned long size);

As discussed in IRC, phys_size_t would be more documenting why it
deviates from u64.
 
> +static struct i915_vma *vma_lookup(struct drm_i915_gem_object *obj,
> +				   struct i915_address_space *vm)
> +{
> +	return i915_gem_obj_lookup_or_create_vma(obj, vm, NULL);
> +}

How about finally renaming the original function itself? 'vma_lookup'
is tad confusing, maybe 'vma_instance'? Lookup has strong implication
that it'll just look for existance. The name could be even better.

> +
> +static int igt_ppgtt_fill(void *arg)
> +{
> +	struct drm_i915_private *dev_priv = arg;
> +	unsigned long npages, max_pages = 1 << 20, prime;

Naww, assignment to it's own line (or rather, embed it to the min_t
expression). If you plan on making it a parameter, do so :P

> +	struct drm_i915_gem_object *obj, *on;
> +	struct i915_hw_ppgtt *ppgtt;
> +	struct i915_vma *vma;
> +	LIST_HEAD(objects);
> +	int err = 0;
> +
> +	if (!USES_FULL_PPGTT(dev_priv))
> +		return 0;

This calls for return -ENXIO (or something else we're not using under
DRM and i915) and handling in the callchain.

> +
> +	mutex_lock(&dev_priv->drm.struct_mutex);
> +	ppgtt = i915_ppgtt_create(dev_priv, NULL, "mock");
> +	if (IS_ERR(ppgtt)) {
> +		err = PTR_ERR(ppgtt);
> +		goto err_unlock;
> +	}
> +	GEM_BUG_ON(ppgtt->base.total & ~PAGE_MASK);

IS_ALIGNED or offset_in_page

> +	for_each_prime_number_from(prime, 2, 13) {
> +		for (npages = 1; npages <= max_pages; npages *= prime) {
> +			u64 flags;
> +
> +			GEM_BUG_ON(!npages);
> +			obj = huge_gem_object(dev_priv,
> +					      PAGE_SIZE,
> +					      npages << PAGE_SHIFT);
> +			if (IS_ERR(obj))
> +				break;
> +
> +			list_add(&obj->batch_pool_link, &objects);

Urgh... anonymous union?

> +
> +			/* Fill the GTT top down - hope we don't overstep the end */
> +			flags = ppgtt->base.total | PIN_OFFSET_FIXED | PIN_USER;
> +			list_for_each_entry(obj, &objects, batch_pool_link) {
> +				vma = vma_lookup(obj, &ppgtt->base);
> +				if (IS_ERR(vma))
> +					continue;
> +

GEM_BUG_ON(flags & I915_GTT_PAGE_SIZE < obj->base.size);

> +				flags -= obj->base.size;
> +				err = i915_vma_pin(vma, 0, 0, flags);
> +				if (err) {
> +					pr_err("Fill top-down failed with err=%d on size=%lu pages (prime=%lu)\n", err, npages, prime);

Alternatively dump flags in here so it's obvious from log.

> +					goto err;
> +				}
> +
> +				i915_vma_unpin(vma);
> +			}
> +
> +			flags = ppgtt->base.total | PIN_OFFSET_FIXED | PIN_USER;
> +			list_for_each_entry(obj, &objects, batch_pool_link) {
> +				vma = vma_lookup(obj, &ppgtt->base);
> +				if (IS_ERR(vma))
> +					continue;
> +
> +				flags -= obj->base.size;
> +				if (!drm_mm_node_allocated(&vma->node) ||
> +				    i915_vma_misplaced(vma, 0, 0, flags)) {
> +					pr_err("Fill top-down moved vma.node=%llx + %llx, expected offset %llx\n",
> +					       vma->node.start, vma->node.size,
> +					       flags & PAGE_MASK);
> +					err = -EINVAL;
> +					goto err;
> +				}
> +
> +				err = i915_vma_unbind(vma);
> +				if (err) {
> +					pr_err("Fill top-down unbind of vma.node=%llx + %llx failed with err=%d\n",
> +					       vma->node.start, vma->node.size,
> +					       err);
> +					goto err;
> +				}
> +			}
> +

Maybe convert above into traditional phase[] array to dedup code from
this point on?

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