[Intel-gfx] [PATCH 05/19] drm/i915: align the vma start to the largest gtt page size

[Chris Wilson]

Quoting Matthew Auld (2017-06-21 21:33:31)
> For the 48b PPGTT try to align the vma start address to the required
> page size boundary to guarantee we use said page size in the gtt. If we
> are dealing with multiple page sizes, we can't guarantee anything and
> just align to the largest. For soft pinning and objects which need to be
> tightly packed into the lower 32bits we don't force any alignment.
> 
> v2: various improvements suggested by Chris
> 
> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_vma.c | 15 +++++++++++++++
>  drivers/gpu/drm/i915/i915_vma.h |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index 958be0a95960..cee1d00dc085 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -471,6 +471,9 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
>         if (ret)
>                 return ret;
>  
> +       vma->page_sizes.phys = obj->mm.page_sizes.phys;
> +       vma->page_sizes.sg = obj->mm.page_sizes.sg;

I expected this to be in the same place as where we assigned vma->pages.
That'll take a bit of rejigging to make it look neat. First thought is a
if (!vma->pages) vma->vm->set_pages(vma);

> +
>         if (flags & PIN_OFFSET_FIXED) {
>                 u64 offset = flags & PIN_OFFSET_MASK;
>                 if (!IS_ALIGNED(offset, alignment) ||
> @@ -485,6 +488,18 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
>                 if (ret)
>                         goto err_unpin;
>         } else {
> +               /* We only support huge gtt pages through the 48b PPGTT,
> +                * however we also don't want to force any alignment for
> +                * objects which need to be tightly packed into the low 32bits.
> +                */
> +               if (end > (1ULL << 32) &&
> +                   vma->page_sizes.sg > I915_GTT_PAGE_SIZE) {
> +                       u64 page_alignment =
> +                               rounddown_pow_of_two(vma->page_sizes.sg);
> +
> +                       alignment = max(alignment, page_alignment);
> +               }
> +
>                 ret = i915_gem_gtt_insert(vma->vm, &vma->node,
>                                           size, alignment, obj->cache_level,
>                                           start, end, flags);
> diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h
> index 4a673fc1a432..834f7ca2ada2 100644
> --- a/drivers/gpu/drm/i915/i915_vma.h
> +++ b/drivers/gpu/drm/i915/i915_vma.h
> @@ -52,6 +52,7 @@ struct i915_vma {
>         struct drm_i915_fence_reg *fence;
>         struct reservation_object *resv; /** Alias of obj->resv */
>         struct sg_table *pages;
> +       struct i915_page_sizes page_sizes;

In the middle of a bunch of pointers! Have some decency please!

>         void __iomem *iomap;
>         u64 size;
>         u64 display_alignment;

Especially when there are some related variables right here :)
-Chris