Re: [PATCH 10/18] drm/i915/gtt: Lazily allocate page directories for gen7

From: Matthew Auld <matthew.william.auld@gmail.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 10/18] drm/i915/gtt: Lazily allocate page directories for gen7
Date: Fri, 8 Jun 2018 15:37:43 +0100	[thread overview]
Message-ID: <CAM0jSHOw83AgeVKs2RN5Hj+VQJL-bBNncghvq-hCY_4mtyWT7w@mail.gmail.com> (raw)
In-Reply-To: <20180608125602.17693-11-chris@chris-wilson.co.uk>

On 8 June 2018 at 13:55, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> As we were only supporting aliasing_ppgtt on gen7 for some time, we
> saved a few checks by preallocating the page directories on creation.
> However, since we need 2MiB of page directories for each ppgtt, to
> support arbitrary numbers of user contexts, we need to be more prudent
> in our allocations, and defer the page allocation until it is used. We
> don't recover unused pages yet as we found that doing so on the fly
> (i.e. altering TLB entries) would confuse the GPU.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> Reviewed-by: Matthew Auld <matthew.william.auld@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 67 +++++++++++------------------
>  1 file changed, 26 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index d5af099939f6..e611884596a6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -190,11 +190,19 @@ int intel_sanitize_enable_ppgtt(struct drm_i915_private *dev_priv,
>         return 1;
>  }
>
> -static int gen6_ppgtt_bind_vma(struct i915_vma *vma,
> -                              enum i915_cache_level cache_level,
> -                              u32 unused)
> +static int ppgtt_bind_vma(struct i915_vma *vma,
> +                         enum i915_cache_level cache_level,
> +                         u32 unused)
>  {
>         u32 pte_flags;
> +       int err;
> +
> +       if (!(vma->flags & I915_VMA_LOCAL_BIND)) {
> +               err = vma->vm->allocate_va_range(vma->vm,
> +                                                vma->node.start, vma->size);
> +               if (err)
> +                       return err;
> +       }
>
>         /* Currently applicable only to VLV */
>         pte_flags = 0;
> @@ -206,22 +214,6 @@ static int gen6_ppgtt_bind_vma(struct i915_vma *vma,
>         return 0;
>  }
>
> -static int gen8_ppgtt_bind_vma(struct i915_vma *vma,
> -                              enum i915_cache_level cache_level,
> -                              u32 unused)
> -{
> -       int ret;
> -
> -       if (!(vma->flags & I915_VMA_LOCAL_BIND)) {
> -               ret = vma->vm->allocate_va_range(vma->vm,
> -                                                vma->node.start, vma->size);
> -               if (ret)
> -                       return ret;
> -       }
> -
> -       return gen6_ppgtt_bind_vma(vma, cache_level, unused);
> -}
> -
>  static void ppgtt_unbind_vma(struct i915_vma *vma)
>  {
>         vma->vm->clear_range(vma->vm, vma->node.start, vma->size);
> @@ -1622,7 +1614,7 @@ static struct i915_hw_ppgtt *gen8_ppgtt_create(struct drm_i915_private *i915)
>         ppgtt->vm.cleanup = gen8_ppgtt_cleanup;
>         ppgtt->debug_dump = gen8_dump_ppgtt;
>
> -       ppgtt->vm.vma_ops.bind_vma    = gen8_ppgtt_bind_vma;
> +       ppgtt->vm.vma_ops.bind_vma    = ppgtt_bind_vma;
>         ppgtt->vm.vma_ops.unbind_vma  = ppgtt_unbind_vma;
>         ppgtt->vm.vma_ops.set_pages   = ppgtt_set_pages;
>         ppgtt->vm.vma_ops.clear_pages = clear_pages;
> @@ -1837,7 +1829,8 @@ static void gen6_ppgtt_clear_range(struct i915_address_space *vm,
>
>                 num_entries -= end - pte;
>
> -               /* Note that the hw doesn't support removing PDE on the fly
> +               /*
> +                * Note that the hw doesn't support removing PDE on the fly
>                  * (they are cached inside the context with no means to
>                  * invalidate the cache), so we can only reset the PTE
>                  * entries back to scratch.
> @@ -2106,12 +2099,13 @@ static struct i915_hw_ppgtt *gen6_ppgtt_create(struct drm_i915_private *i915)
>
>         ppgtt->base.vm.total = I915_PDES * GEN6_PTES * PAGE_SIZE;
>
> +       ppgtt->base.vm.allocate_va_range = gen6_alloc_va_range;

Ah, in gen6_alloc_va_range() I think we now need:

unwind_out:
-       gen6_ppgtt_clear_range(vm, from, start);
+       gen6_ppgtt_clear_range(vm, from, start - from);
        return -ENOMEM;
 }

?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx