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@xxxxxxxxxxxxxxxxxx>
<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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
