| Message ID | 1366784140-2670-7-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 23 Apr 2013 23:15:34 -0700 Ben Widawsky <ben@bwidawsk.net> wrote: > I think this is a nice generalization on it's own, but it's primarily > prep work for my PPGTT support. Does this bother anyone? > > The only down side I can see is we waste 2k of cpu unmappable space > (unless we have something else that is <= 2k and doesn't need to be page > aligned). > > v2: Align PDEs to 64b in GTT > Allocate the node dynamically so we can use drm_mm_put_block > Now tested on IGT > Allocate node at the top to avoid fragmentation (Chris) > > v3: Use Chris' top down allocator > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem_gtt.c | 54 ++++++++++++++++++++++++++----------- > include/drm/drm_mm.h | 3 +++ > 3 files changed, 42 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 0a9c7cd..9ab6254 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -432,6 +432,7 @@ struct i915_gtt { > #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT) > > struct i915_hw_ppgtt { > + struct drm_mm_node *node; > struct drm_device *dev; > unsigned num_pd_entries; > struct page **pt_pages; > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 975adaa..b825d7b 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -219,6 +219,8 @@ static void gen6_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt) > { > int i; > > + drm_mm_put_block(ppgtt->node); > + > if (ppgtt->pt_dma_addr) { > for (i = 0; i < ppgtt->num_pd_entries; i++) > pci_unmap_page(ppgtt->dev->pdev, > @@ -235,16 +237,27 @@ static void gen6_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt) > > static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > { > +#define GEN6_PD_ALIGN (PAGE_SIZE * 16) > +#define GEN6_PD_SIZE (512 * PAGE_SIZE) > struct drm_device *dev = ppgtt->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - unsigned first_pd_entry_in_global_pt; > int i; > int ret = -ENOMEM; > > - /* ppgtt PDEs reside in the global gtt pagetable, which has 512*1024 > - * entries. For aliasing ppgtt support we just steal them at the end for > - * now. */ > - first_pd_entry_in_global_pt = gtt_total_entries(dev_priv->gtt) - 512; > + /* PPGTT PDEs reside in the GGTT stolen space, and consists of 512 > + * entries. The allocator works in address space sizes, so it's > + * multiplied by page size. We allocate at the top of the GTT to avoid > + * fragmentation. > + */ > + BUG_ON(!drm_mm_initialized(&dev_priv->mm.gtt_space)); > + ret = drm_mm_insert_node_in_range_generic(&dev_priv->mm.gtt_space, > + ppgtt->node, GEN6_PD_SIZE, > + GEN6_PD_ALIGN, 0, > + dev_priv->gtt.mappable_end, > + dev_priv->gtt.total - PAGE_SIZE, > + DRM_MM_TOPDOWN); > + if (ret) > + return ret; I guess I won't see why you're doing this until later; presumably it makes PPGTT management easier in subsequent patches? > > ppgtt->num_pd_entries = 512; > ppgtt->enable = gen6_ppgtt_enable; > @@ -253,8 +266,10 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > ppgtt->cleanup = gen6_ppgtt_cleanup; > ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries, > GFP_KERNEL); > - if (!ppgtt->pt_pages) > + if (!ppgtt->pt_pages) { > + drm_mm_put_block(ppgtt->node); > return -ENOMEM; > + } > > for (i = 0; i < ppgtt->num_pd_entries; i++) { > ppgtt->pt_pages[i] = alloc_page(GFP_KERNEL); > @@ -284,7 +299,10 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > ppgtt->clear_range(ppgtt, 0, > ppgtt->num_pd_entries*I915_PPGTT_PT_ENTRIES); > > - ppgtt->pd_offset = first_pd_entry_in_global_pt * sizeof(gen6_gtt_pte_t); > + DRM_DEBUG_DRIVER("Allocated pde space (%ldM) at GTT entry: %lx\n", > + ppgtt->node->size >> 20, > + ppgtt->node->start / PAGE_SIZE); > + ppgtt->pd_offset = ppgtt->node->start / PAGE_SIZE * sizeof(gen6_gtt_pte_t); > > return 0; > > @@ -315,6 +333,12 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) > if (!ppgtt) > return -ENOMEM; > > + ppgtt->node = kzalloc(sizeof(*ppgtt->node), GFP_KERNEL); > + if (!ppgtt->node) { > + kfree(ppgtt); > + return -ENOMEM; > + } > + Why not just put the drm_mm node directly into the hw_ppgtt struct instead of allocating it? Every context/ppgtt instance will need one, right? > ppgtt->dev = dev; > ppgtt->scratch_page_dma_addr = dev_priv->gtt.scratch_page_dma; > > @@ -323,10 +347,12 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) > else > BUG(); > > - if (ret) > + if (ret) { > + drm_mm_put_block(ppgtt->node); > kfree(ppgtt); > - else > + } else { > dev_priv->mm.aliasing_ppgtt = ppgtt; > + } > > return ret; > } > @@ -407,6 +433,9 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) > dev_priv->gtt.gtt_clear_range(dev, dev_priv->gtt.start / PAGE_SIZE, > dev_priv->gtt.total / PAGE_SIZE); > > + if (dev_priv->mm.aliasing_ppgtt) > + gen6_write_pdes(dev_priv->mm.aliasing_ppgtt); > + > list_for_each_entry(obj, &dev_priv->mm.bound_list, gtt_list) { > i915_gem_clflush_object(obj); > i915_gem_gtt_bind_object(obj, obj->cache_level); > @@ -651,12 +680,6 @@ void i915_gem_init_global_gtt(struct drm_device *dev) > if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) { > int ret; > > - if (INTEL_INFO(dev)->gen <= 7) { > - /* PPGTT pdes are stolen from global gtt ptes, so shrink the > - * aperture accordingly when using aliasing ppgtt. */ > - gtt_size -= 512 * PAGE_SIZE; > - } > - > i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); > > ret = i915_gem_init_aliasing_ppgtt(dev); > @@ -665,7 +688,6 @@ void i915_gem_init_global_gtt(struct drm_device *dev) > > DRM_ERROR("Aliased PPGTT setup failed %d\n", ret); > drm_mm_takedown(&dev_priv->mm.gtt_space); > - gtt_size += 512 * PAGE_SIZE; > } > i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); > } > diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h > index 752e846..ce7b13b 100644 > --- a/include/drm/drm_mm.h > +++ b/include/drm/drm_mm.h > @@ -53,6 +53,9 @@ enum drm_mm_search_flags { > DRM_MM_SEARCH_BELOW = 1<<1, > }; > > +#define DRM_MM_BOTTOMUP DRM_MM_CREATE_DEFAULT, DRM_MM_SEARCH_DEFAULT > +#define DRM_MM_TOPDOWN DRM_MM_CREATE_TOP, DRM_MM_SEARCH_BELOW > + > struct drm_mm_node { > struct list_head node_list; > struct list_head hole_stack; These last few hunks seem like they belong in different patches? With the above addressed: Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 0a9c7cd..9ab6254 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -432,6 +432,7 @@ struct i915_gtt { #define gtt_total_entries(gtt) ((gtt).total >> PAGE_SHIFT) struct i915_hw_ppgtt { + struct drm_mm_node *node; struct drm_device *dev; unsigned num_pd_entries; struct page **pt_pages; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 975adaa..b825d7b 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -219,6 +219,8 @@ static void gen6_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt) { int i; + drm_mm_put_block(ppgtt->node); + if (ppgtt->pt_dma_addr) { for (i = 0; i < ppgtt->num_pd_entries; i++) pci_unmap_page(ppgtt->dev->pdev, @@ -235,16 +237,27 @@ static void gen6_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt) static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) { +#define GEN6_PD_ALIGN (PAGE_SIZE * 16) +#define GEN6_PD_SIZE (512 * PAGE_SIZE) struct drm_device *dev = ppgtt->dev; struct drm_i915_private *dev_priv = dev->dev_private; - unsigned first_pd_entry_in_global_pt; int i; int ret = -ENOMEM; - /* ppgtt PDEs reside in the global gtt pagetable, which has 512*1024 - * entries. For aliasing ppgtt support we just steal them at the end for - * now. */ - first_pd_entry_in_global_pt = gtt_total_entries(dev_priv->gtt) - 512; + /* PPGTT PDEs reside in the GGTT stolen space, and consists of 512 + * entries. The allocator works in address space sizes, so it's + * multiplied by page size. We allocate at the top of the GTT to avoid + * fragmentation. + */ + BUG_ON(!drm_mm_initialized(&dev_priv->mm.gtt_space)); + ret = drm_mm_insert_node_in_range_generic(&dev_priv->mm.gtt_space, + ppgtt->node, GEN6_PD_SIZE, + GEN6_PD_ALIGN, 0, + dev_priv->gtt.mappable_end, + dev_priv->gtt.total - PAGE_SIZE, + DRM_MM_TOPDOWN); + if (ret) + return ret; ppgtt->num_pd_entries = 512; ppgtt->enable = gen6_ppgtt_enable; @@ -253,8 +266,10 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) ppgtt->cleanup = gen6_ppgtt_cleanup; ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries, GFP_KERNEL); - if (!ppgtt->pt_pages) + if (!ppgtt->pt_pages) { + drm_mm_put_block(ppgtt->node); return -ENOMEM; + } for (i = 0; i < ppgtt->num_pd_entries; i++) { ppgtt->pt_pages[i] = alloc_page(GFP_KERNEL); @@ -284,7 +299,10 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) ppgtt->clear_range(ppgtt, 0, ppgtt->num_pd_entries*I915_PPGTT_PT_ENTRIES); - ppgtt->pd_offset = first_pd_entry_in_global_pt * sizeof(gen6_gtt_pte_t); + DRM_DEBUG_DRIVER("Allocated pde space (%ldM) at GTT entry: %lx\n", + ppgtt->node->size >> 20, + ppgtt->node->start / PAGE_SIZE); + ppgtt->pd_offset = ppgtt->node->start / PAGE_SIZE * sizeof(gen6_gtt_pte_t); return 0; @@ -315,6 +333,12 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) if (!ppgtt) return -ENOMEM; + ppgtt->node = kzalloc(sizeof(*ppgtt->node), GFP_KERNEL); + if (!ppgtt->node) { + kfree(ppgtt); + return -ENOMEM; + } + ppgtt->dev = dev; ppgtt->scratch_page_dma_addr = dev_priv->gtt.scratch_page_dma; @@ -323,10 +347,12 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) else BUG(); - if (ret) + if (ret) { + drm_mm_put_block(ppgtt->node); kfree(ppgtt); - else + } else { dev_priv->mm.aliasing_ppgtt = ppgtt; + } return ret; } @@ -407,6 +433,9 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev) dev_priv->gtt.gtt_clear_range(dev, dev_priv->gtt.start / PAGE_SIZE, dev_priv->gtt.total / PAGE_SIZE); + if (dev_priv->mm.aliasing_ppgtt) + gen6_write_pdes(dev_priv->mm.aliasing_ppgtt); + list_for_each_entry(obj, &dev_priv->mm.bound_list, gtt_list) { i915_gem_clflush_object(obj); i915_gem_gtt_bind_object(obj, obj->cache_level); @@ -651,12 +680,6 @@ void i915_gem_init_global_gtt(struct drm_device *dev) if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) { int ret; - if (INTEL_INFO(dev)->gen <= 7) { - /* PPGTT pdes are stolen from global gtt ptes, so shrink the - * aperture accordingly when using aliasing ppgtt. */ - gtt_size -= 512 * PAGE_SIZE; - } - i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); ret = i915_gem_init_aliasing_ppgtt(dev); @@ -665,7 +688,6 @@ void i915_gem_init_global_gtt(struct drm_device *dev) DRM_ERROR("Aliased PPGTT setup failed %d\n", ret); drm_mm_takedown(&dev_priv->mm.gtt_space); - gtt_size += 512 * PAGE_SIZE; } i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); } diff --git a/include/drm/drm_mm.h b/include/drm/drm_mm.h index 752e846..ce7b13b 100644 --- a/include/drm/drm_mm.h +++ b/include/drm/drm_mm.h @@ -53,6 +53,9 @@ enum drm_mm_search_flags { DRM_MM_SEARCH_BELOW = 1<<1, }; +#define DRM_MM_BOTTOMUP DRM_MM_CREATE_DEFAULT, DRM_MM_SEARCH_DEFAULT +#define DRM_MM_TOPDOWN DRM_MM_CREATE_TOP, DRM_MM_SEARCH_BELOW + struct drm_mm_node { struct list_head node_list; struct list_head hole_stack;
I think this is a nice generalization on it's own, but it's primarily prep work for my PPGTT support. Does this bother anyone? The only down side I can see is we waste 2k of cpu unmappable space (unless we have something else that is <= 2k and doesn't need to be page aligned). v2: Align PDEs to 64b in GTT Allocate the node dynamically so we can use drm_mm_put_block Now tested on IGT Allocate node at the top to avoid fragmentation (Chris) v3: Use Chris' top down allocator Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem_gtt.c | 54 ++++++++++++++++++++++++++----------- include/drm/drm_mm.h | 3 +++ 3 files changed, 42 insertions(+), 16 deletions(-)