Re: [PATCH 01/12] drm/i915/bdw: Make pdp allocation more dynamic

[akash goel <akash.goels@gmail.com>]

On Fri, Feb 20, 2015 at 11:15 PM, Michel Thierry
<michel.thierry@intel.com> wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
>
> This transitional patch doesn't do much for the existing code. However,
> it should make upcoming patches to use the full 48b address space a bit
> easier to swallow. The patch also introduces the PML4, ie. the new top
> level structure of the page tables.
>
> v2: Renamed  pdp_free to be similar to  pd/pt (unmap_and_free_pdp),
> To facilitate testing, 48b mode will be available on Broadwell, when
> i915.enable_ppgtt = 3.
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com> (v2)
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |   7 ++-
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 108 +++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/i915_gem_gtt.h |  41 +++++++++++---
>  3 files changed, 126 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2dedd43..af0d149 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2432,7 +2432,12 @@ struct drm_i915_cmd_table {
>  #define HAS_HW_CONTEXTS(dev)   (INTEL_INFO(dev)->gen >= 6)
>  #define HAS_LOGICAL_RING_CONTEXTS(dev) (INTEL_INFO(dev)->gen >= 8)
>  #define USES_PPGTT(dev)                (i915.enable_ppgtt)
> -#define USES_FULL_PPGTT(dev)   (i915.enable_ppgtt == 2)
> +#define USES_FULL_PPGTT(dev)   (i915.enable_ppgtt >= 2)
> +#ifdef CONFIG_64BIT
> +# define USES_FULL_48BIT_PPGTT(dev)    (i915.enable_ppgtt == 3)
> +#else
> +# define USES_FULL_48BIT_PPGTT(dev)    false
> +#endif
>
>  #define HAS_OVERLAY(dev)               (INTEL_INFO(dev)->has_overlay)
>  #define OVERLAY_NEEDS_PHYSICAL(dev)    (INTEL_INFO(dev)->overlay_needs_physical)
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index ff86501..489f8db 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -100,10 +100,17 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
>  {
>         bool has_aliasing_ppgtt;
>         bool has_full_ppgtt;
> +       bool has_full_64bit_ppgtt;
>
>         has_aliasing_ppgtt = INTEL_INFO(dev)->gen >= 6;
>         has_full_ppgtt = INTEL_INFO(dev)->gen >= 7;
>
> +#ifdef CONFIG_64BIT
> +       has_full_64bit_ppgtt = IS_BROADWELL(dev) && false; /* FIXME: 64b */
> +#else
> +       has_full_64bit_ppgtt = false;
> +#endif
> +
>         if (intel_vgpu_active(dev))
>                 has_full_ppgtt = false; /* emulation is too hard */
>
> @@ -121,6 +128,9 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
>         if (enable_ppgtt == 2 && has_full_ppgtt)
>                 return 2;
>
> +       if (enable_ppgtt == 3 && has_full_64bit_ppgtt)
> +               return 3;
> +
>  #ifdef CONFIG_INTEL_IOMMU
>         /* Disable ppgtt on SNB if VT-d is on. */
>         if (INTEL_INFO(dev)->gen == 6 && intel_iommu_gfx_mapped) {
> @@ -462,6 +472,45 @@ free_pd:
>         return ERR_PTR(ret);
>  }
>
> +static void __pdp_fini(struct i915_page_directory_pointer_entry *pdp)
> +{
> +       kfree(pdp->used_pdpes);
> +       kfree(pdp->page_directory);
> +       /* HACK */
> +       pdp->page_directory = NULL;
> +}
> +
> +static void unmap_and_free_pdp(struct i915_page_directory_pointer_entry *pdp,
> +                           struct drm_device *dev)
> +{
> +       __pdp_fini(pdp);
> +       if (USES_FULL_48BIT_PPGTT(dev))
> +               kfree(pdp);
> +}
> +
> +static int __pdp_init(struct i915_page_directory_pointer_entry *pdp,
> +                     struct drm_device *dev)
> +{
> +       size_t pdpes = I915_PDPES_PER_PDP(dev);
> +
> +       pdp->used_pdpes = kcalloc(BITS_TO_LONGS(pdpes),
> +                                 sizeof(unsigned long),
> +                                 GFP_KERNEL);
> +       if (!pdp->used_pdpes)
> +               return -ENOMEM;
> +
> +       pdp->page_directory = kcalloc(pdpes, sizeof(*pdp->page_directory), GFP_KERNEL);
> +       if (!pdp->page_directory) {
> +               kfree(pdp->used_pdpes);
> +               /* the PDP might be the statically allocated top level. Keep it
> +                * as clean as possible */
> +               pdp->used_pdpes = NULL;
> +               return -ENOMEM;
> +       }
> +
> +       return 0;
> +}
> +
>  /* Broadwell Page Directory Pointer Descriptors */
>  static int gen8_write_pdp(struct intel_engine_cs *ring,
>                           unsigned entry,
> @@ -491,7 +540,7 @@ static int gen8_mm_switch(struct i915_hw_ppgtt *ppgtt,
>  {
>         int i, ret;
>
> -       for (i = GEN8_LEGACY_PDPES - 1; i >= 0; i--) {
> +       for (i = 3; i >= 0; i--) {
>                 struct i915_page_directory_entry *pd = ppgtt->pdp.page_directory[i];
>                 dma_addr_t pd_daddr = pd ? pd->daddr : ppgtt->scratch_pd->daddr;
>                 /* The page directory might be NULL, but we need to clear out
> @@ -580,9 +629,6 @@ static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
>         pt_vaddr = NULL;
>
>         for_each_sg_page(pages->sgl, &sg_iter, pages->nents, 0) {
> -               if (WARN_ON(pdpe >= GEN8_LEGACY_PDPES))
> -                       break;
> -
>                 if (pt_vaddr == NULL) {
>                         struct i915_page_directory_entry *pd = ppgtt->pdp.page_directory[pdpe];
>                         struct i915_page_table_entry *pt = pd->page_tables[pde];
> @@ -664,7 +710,8 @@ static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt)
>         struct pci_dev *hwdev = ppgtt->base.dev->pdev;
>         int i, j;
>
> -       for_each_set_bit(i, ppgtt->pdp.used_pdpes, GEN8_LEGACY_PDPES) {
> +       for_each_set_bit(i, ppgtt->pdp.used_pdpes,
> +                       I915_PDPES_PER_PDP(ppgtt->base.dev)) {
>                 struct i915_page_directory_entry *pd;
>
>                 if (WARN_ON(!ppgtt->pdp.page_directory[i]))
> @@ -696,13 +743,15 @@ static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
>  {
>         int i;
>
> -       for_each_set_bit(i, ppgtt->pdp.used_pdpes, GEN8_LEGACY_PDPES) {
> +       for_each_set_bit(i, ppgtt->pdp.used_pdpes,
> +                               I915_PDPES_PER_PDP(ppgtt->base.dev)) {
>                 if (WARN_ON(!ppgtt->pdp.page_directory[i]))
>                         continue;
>
>                 gen8_free_page_tables(ppgtt->pdp.page_directory[i], ppgtt->base.dev);
>                 unmap_and_free_pd(ppgtt->pdp.page_directory[i], ppgtt->base.dev);
>         }
> +       unmap_and_free_pdp(&ppgtt->pdp, ppgtt->base.dev);

'ppgtt->scratch_pd' is not being de-allocated.

Probably it can be de-allocated explicitly from here, after the call
to  unmap_and_free_pdp or
from the gen8_ppgtt_cleanup function .

>  }
>
>  static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
> @@ -799,8 +848,9 @@ static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>         struct i915_page_directory_entry *pd;
>         uint64_t temp;
>         uint32_t pdpe;
> +       size_t pdpes =  I915_PDPES_PER_PDP(ppgtt->base.dev);
>
> -       BUG_ON(!bitmap_empty(new_pds, GEN8_LEGACY_PDPES));
> +       BUG_ON(!bitmap_empty(new_pds, pdpes));
>
>         /* FIXME: PPGTT container_of won't work for 64b */
>         BUG_ON((start + length) > 0x800000000ULL);
> @@ -820,18 +870,19 @@ static int gen8_ppgtt_alloc_page_directories(struct i915_hw_ppgtt *ppgtt,
>         return 0;
>
>  unwind_out:
> -       for_each_set_bit(pdpe, new_pds, GEN8_LEGACY_PDPES)
> +       for_each_set_bit(pdpe, new_pds, pdpes)
>                 unmap_and_free_pd(pdp->page_directory[pdpe], ppgtt->base.dev);
>
>         return -ENOMEM;
>  }
>
>  static inline void
> -free_gen8_temp_bitmaps(unsigned long *new_pds, unsigned long **new_pts)
> +free_gen8_temp_bitmaps(unsigned long *new_pds, unsigned long **new_pts,
> +                      size_t pdpes)
>  {
>         int i;
>
> -       for (i = 0; i < GEN8_LEGACY_PDPES; i++)
> +       for (i = 0; i < pdpes; i++)
>                 kfree(new_pts[i]);
>         kfree(new_pts);
>         kfree(new_pds);
> @@ -841,13 +892,14 @@ free_gen8_temp_bitmaps(unsigned long *new_pds, unsigned long **new_pts)
>   * of these are based on the number of PDPEs in the system.
>   */
>  int __must_check alloc_gen8_temp_bitmaps(unsigned long **new_pds,
> -                                        unsigned long ***new_pts)
> +                                        unsigned long ***new_pts,
> +                                        size_t pdpes)
>  {
>         int i;
>         unsigned long *pds;
>         unsigned long **pts;
>
> -       pds = kcalloc(BITS_TO_LONGS(GEN8_LEGACY_PDPES), sizeof(unsigned long), GFP_KERNEL);
> +       pds = kcalloc(BITS_TO_LONGS(pdpes), sizeof(unsigned long), GFP_KERNEL);
>         if (!pds)
>                 return -ENOMEM;
>
> @@ -857,7 +909,7 @@ int __must_check alloc_gen8_temp_bitmaps(unsigned long **new_pds,
>                 return -ENOMEM;
>         }
>
> -       for (i = 0; i < GEN8_LEGACY_PDPES; i++) {
> +       for (i = 0; i < pdpes; i++) {
>                 pts[i] = kcalloc(BITS_TO_LONGS(GEN8_PDES_PER_PAGE),
>                                  sizeof(unsigned long), GFP_KERNEL);
>                 if (!pts[i])
> @@ -870,7 +922,7 @@ int __must_check alloc_gen8_temp_bitmaps(unsigned long **new_pds,
>         return 0;
>
>  err_out:
> -       free_gen8_temp_bitmaps(pds, pts);
> +       free_gen8_temp_bitmaps(pds, pts, pdpes);
>         return -ENOMEM;
>  }
>
> @@ -886,6 +938,7 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
>         const uint64_t orig_length = length;
>         uint64_t temp;
>         uint32_t pdpe;
> +       size_t pdpes = I915_PDPES_PER_PDP(dev);
>         int ret;
>
>  #ifndef CONFIG_64BIT
> @@ -903,7 +956,7 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
>         if (WARN_ON(start + length < start))
>                 return -ERANGE;
>
> -       ret = alloc_gen8_temp_bitmaps(&new_page_dirs, &new_page_tables);
> +       ret = alloc_gen8_temp_bitmaps(&new_page_dirs, &new_page_tables, pdpes);
>         if (ret)
>                 return ret;
>
> @@ -911,7 +964,7 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
>         ret = gen8_ppgtt_alloc_page_directories(ppgtt, &ppgtt->pdp, start, length,
>                                         new_page_dirs);
>         if (ret) {
> -               free_gen8_temp_bitmaps(new_page_dirs, new_page_tables);
> +               free_gen8_temp_bitmaps(new_page_dirs, new_page_tables, pdpes);
>                 return ret;
>         }
>
> @@ -968,7 +1021,7 @@ static int gen8_alloc_va_range(struct i915_address_space *vm,
>                 set_bit(pdpe, ppgtt->pdp.used_pdpes);
>         }
>
> -       free_gen8_temp_bitmaps(new_page_dirs, new_page_tables);
> +       free_gen8_temp_bitmaps(new_page_dirs, new_page_tables, pdpes);
>         return 0;
>
>  err_out:
> @@ -977,13 +1030,19 @@ err_out:
>                         unmap_and_free_pt(pd->page_tables[temp], vm->dev);
>         }
>
> -       for_each_set_bit(pdpe, new_page_dirs, GEN8_LEGACY_PDPES)
> +       for_each_set_bit(pdpe, new_page_dirs, pdpes)
>                 unmap_and_free_pd(ppgtt->pdp.page_directory[pdpe], vm->dev);
>
> -       free_gen8_temp_bitmaps(new_page_dirs, new_page_tables);
> +       free_gen8_temp_bitmaps(new_page_dirs, new_page_tables, pdpes);
>         return ret;
>  }
>
> +static void gen8_ppgtt_fini_common(struct i915_hw_ppgtt *ppgtt)
> +{
> +       unmap_and_free_pt(ppgtt->scratch_pd, ppgtt->base.dev);
> +       unmap_and_free_pdp(&ppgtt->pdp, ppgtt->base.dev);
> +}
> +
>  /**
>   * GEN8 legacy ppgtt programming is accomplished through a max 4 PDP registers
>   * with a net effect resembling a 2-level page table in normal x86 terms. Each
> @@ -1004,6 +1063,15 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>
>         ppgtt->switch_mm = gen8_mm_switch;
>
> +       if (!USES_FULL_48BIT_PPGTT(ppgtt->base.dev)) {
> +               int ret = __pdp_init(&ppgtt->pdp, false);
> +               if (ret) {
> +                       unmap_and_free_pt(ppgtt->scratch_pd, ppgtt->base.dev);
> +                       return ret;
> +               }
> +       } else
> +               return -EPERM; /* Not yet implemented */
> +
>         return 0;
>  }
>
> @@ -1025,7 +1093,7 @@ static int gen8_aliasing_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>          * eventually. */
>         ret = gen8_alloc_va_range(&ppgtt->base, start, size);
>         if (ret) {
> -               unmap_and_free_pt(ppgtt->scratch_pd, ppgtt->base.dev);
> +               gen8_ppgtt_fini_common(ppgtt);
>                 return ret;
>         }
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index c68ec3a..a33c6e9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -85,8 +85,12 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
>   * The difference as compared to normal x86 3 level page table is the PDPEs are
>   * programmed via register.
>   */
> +#define GEN8_PML4ES_PER_PML4           512
> +#define GEN8_PML4E_SHIFT               39
>  #define GEN8_PDPE_SHIFT                        30
> -#define GEN8_PDPE_MASK                 0x3
> +/* NB: GEN8_PDPE_MASK is untrue for 32b platforms, but it has no impact on 32b page
> + * tables */
> +#define GEN8_PDPE_MASK                 0x1ff
>  #define GEN8_PDE_SHIFT                 21
>  #define GEN8_PDE_MASK                  0x1ff
>  #define GEN8_PTE_SHIFT                 12
> @@ -95,6 +99,13 @@ typedef gen8_gtt_pte_t gen8_ppgtt_pde_t;
>  #define GEN8_PTES_PER_PAGE             (PAGE_SIZE / sizeof(gen8_gtt_pte_t))
>  #define GEN8_PDES_PER_PAGE             (PAGE_SIZE / sizeof(gen8_ppgtt_pde_t))
>
> +#ifdef CONFIG_64BIT
> +# define I915_PDPES_PER_PDP(dev) (USES_FULL_48BIT_PPGTT(dev) ?\
> +               GEN8_PML4ES_PER_PML4 : GEN8_LEGACY_PDPES)
> +#else
> +# define I915_PDPES_PER_PDP            GEN8_LEGACY_PDPES
> +#endif
> +
>  #define PPAT_UNCACHED_INDEX            (_PAGE_PWT | _PAGE_PCD)
>  #define PPAT_CACHED_PDE_INDEX          0 /* WB LLC */
>  #define PPAT_CACHED_INDEX              _PAGE_PAT /* WB LLCeLLC */
> @@ -210,9 +221,17 @@ struct i915_page_directory_entry {
>  };
>
>  struct i915_page_directory_pointer_entry {
> -       /* struct page *page; */
> -       DECLARE_BITMAP(used_pdpes, GEN8_LEGACY_PDPES);
> -       struct i915_page_directory_entry *page_directory[GEN8_LEGACY_PDPES];
> +       struct page *page;
> +       dma_addr_t daddr;
> +       unsigned long *used_pdpes;
> +       struct i915_page_directory_entry **page_directory;
> +};
> +
> +struct i915_pml4 {
> +       struct page *page;
> +       dma_addr_t daddr;
> +       DECLARE_BITMAP(used_pml4es, GEN8_PML4ES_PER_PML4);
> +       struct i915_page_directory_pointer_entry *pdps[GEN8_PML4ES_PER_PML4];
>  };
>
>  struct i915_address_space {
> @@ -302,8 +321,9 @@ struct i915_hw_ppgtt {
>         struct drm_mm_node node;
>         unsigned long pd_dirty_rings;
>         union {
> -               struct i915_page_directory_pointer_entry pdp;
> -               struct i915_page_directory_entry pd;
> +               struct i915_pml4 pml4;          /* GEN8+ & 64b PPGTT */
> +               struct i915_page_directory_pointer_entry pdp;   /* GEN8+ */
> +               struct i915_page_directory_entry pd;            /* GEN6-7 */
>         };
>
>         union {
> @@ -399,14 +419,17 @@ static inline uint32_t gen6_pde_index(uint32_t addr)
>              temp = min(temp, length),                                  \
>              start += temp, length -= temp)
>
> -#define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter)         \
> -       for (iter = gen8_pdpe_index(start), pd = (pdp)->page_directory[iter];   \
> -            length > 0 && iter < GEN8_LEGACY_PDPES;                    \
> +#define gen8_for_each_pdpe_e(pd, pdp, start, length, temp, iter, b)    \
> +       for (iter = gen8_pdpe_index(start), pd = (pdp)->page_directory[iter]; \
> +            length > 0 && (iter < b);                                  \
>              pd = (pdp)->page_directory[++iter],                                \
>              temp = ALIGN(start+1, 1 << GEN8_PDPE_SHIFT) - start,       \
>              temp = min(temp, length),                                  \
>              start += temp, length -= temp)
>
> +#define gen8_for_each_pdpe(pd, pdp, start, length, temp, iter)         \
> +       gen8_for_each_pdpe_e(pd, pdp, start, length, temp, iter, I915_PDPES_PER_PDP(dev))
> +
>  /* Clamp length to the next page_directory boundary */
>  static inline uint64_t gen8_clamp_pd(uint64_t start, uint64_t length)
>  {
> --
> 2.1.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx