[Intel-gfx] [PATCH 4/9] drm/i915: Make clear/insert vfuncs args absolute

Ben Widawsky ben at bwidawsk.net
Thu Feb 20 20:35:21 CET 2014


On Thu, Feb 20, 2014 at 12:37:19PM +0200, Imre Deak wrote:
> On Wed, 2014-02-19 at 22:05 -0800, Ben Widawsky wrote:
> > This patch converts insert_entries and clear_range, both functions which
> > are specific to the VM. These functions tend to encapsulate the gen
> > specific PTE writes. Passing absolute addresses to the insert_entries,
> > and clear_range will help make the logic clearer within the functions as
> > to what's going on. Currently, all callers simply do the appropriate
> > page shift, which IMO, ends up looking weird with an upcoming change for
> > the gen8 page table allocations.
> > 
> > Up until now, the PPGTT was a funky 2 level page table. GEN8 changes
> > this to look more like a 3 level page table, and to that extent we need
> > a significant amount more memory simply for the page tables. To address
> > this, the allocations will be split up in finer amounts.
> > 
> > v2: Replace size_t with uint64_t (Chris, Imre)
> > 
> > Reviewed-by: Imre Deak <imre.deak at intel.com>
> 
> One more thing I haven't noticed,
> i915_gem_suspend_gtt_mappings()/i915_gem_restore_gtt_mappings() needs a
> fixup too.
> 

Thanks for spotting this. gen8_ppgtt_init() is also broken. I don't see
any others:

@@ -542,7 +542,7 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
        ppgtt->base.total = ppgtt->num_pt_pages * GEN8_PTES_PER_PAGE * PAGE_SIZE;
 
        ppgtt->base.clear_range(&ppgtt->base, 0,
-                               ppgtt->num_pd_entries * GEN8_PTES_PER_PAGE,
+                               ppgtt->num_pd_entries * GEN8_PTES_PER_PAGE * PAGE_SIZE,
                                true);
 
        DRM_DEBUG_DRIVER("Allocated %d pages for page directories (%d wasted)\n",

> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> 
> 
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h     |  6 +--
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 80 +++++++++++++++++++++----------------
> >  2 files changed, 49 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 8c64831..f3379ea 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -652,12 +652,12 @@ struct i915_address_space {
> >  				     enum i915_cache_level level,
> >  				     bool valid); /* Create a valid PTE */
> >  	void (*clear_range)(struct i915_address_space *vm,
> > -			    unsigned int first_entry,
> > -			    unsigned int num_entries,
> > +			    uint64_t start,
> > +			    uint64_t length,
> >  			    bool use_scratch);
> >  	void (*insert_entries)(struct i915_address_space *vm,
> >  			       struct sg_table *st,
> > -			       unsigned int first_entry,
> > +			       uint64_t start,
> >  			       enum i915_cache_level cache_level);
> >  	void (*cleanup)(struct i915_address_space *vm);
> >  };
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 0af3587..ef5e90c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -254,13 +254,15 @@ static int gen8_mm_switch(struct i915_hw_ppgtt *ppgtt,
> >  }
> >  
> >  static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
> > -				   unsigned first_entry,
> > -				   unsigned num_entries,
> > +				   uint64_t start,
> > +				   uint64_t length,
> >  				   bool use_scratch)
> >  {
> >  	struct i915_hw_ppgtt *ppgtt =
> >  		container_of(vm, struct i915_hw_ppgtt, base);
> >  	gen8_gtt_pte_t *pt_vaddr, scratch_pte;
> > +	unsigned first_entry = start >> PAGE_SHIFT;
> > +	unsigned num_entries = length >> PAGE_SHIFT;
> >  	unsigned act_pt = first_entry / GEN8_PTES_PER_PAGE;
> >  	unsigned first_pte = first_entry % GEN8_PTES_PER_PAGE;
> >  	unsigned last_pte, i;
> > @@ -290,12 +292,13 @@ static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
> >  
> >  static void gen8_ppgtt_insert_entries(struct i915_address_space *vm,
> >  				      struct sg_table *pages,
> > -				      unsigned first_entry,
> > +				      uint64_t start,
> >  				      enum i915_cache_level cache_level)
> >  {
> >  	struct i915_hw_ppgtt *ppgtt =
> >  		container_of(vm, struct i915_hw_ppgtt, base);
> >  	gen8_gtt_pte_t *pt_vaddr;
> > +	unsigned first_entry = start >> PAGE_SHIFT;
> >  	unsigned act_pt = first_entry / GEN8_PTES_PER_PAGE;
> >  	unsigned act_pte = first_entry % GEN8_PTES_PER_PAGE;
> >  	struct sg_page_iter sg_iter;
> > @@ -855,13 +858,15 @@ static int gen6_ppgtt_enable(struct i915_hw_ppgtt *ppgtt)
> >  
> >  /* PPGTT support for Sandybdrige/Gen6 and later */
> >  static void gen6_ppgtt_clear_range(struct i915_address_space *vm,
> > -				   unsigned first_entry,
> > -				   unsigned num_entries,
> > +				   uint64_t start,
> > +				   uint64_t length,
> >  				   bool use_scratch)
> >  {
> >  	struct i915_hw_ppgtt *ppgtt =
> >  		container_of(vm, struct i915_hw_ppgtt, base);
> >  	gen6_gtt_pte_t *pt_vaddr, scratch_pte;
> > +	unsigned first_entry = start >> PAGE_SHIFT;
> > +	unsigned num_entries = length >> PAGE_SHIFT;
> >  	unsigned act_pt = first_entry / I915_PPGTT_PT_ENTRIES;
> >  	unsigned first_pte = first_entry % I915_PPGTT_PT_ENTRIES;
> >  	unsigned last_pte, i;
> > @@ -888,12 +893,13 @@ static void gen6_ppgtt_clear_range(struct i915_address_space *vm,
> >  
> >  static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
> >  				      struct sg_table *pages,
> > -				      unsigned first_entry,
> > +				      uint64_t start,
> >  				      enum i915_cache_level cache_level)
> >  {
> >  	struct i915_hw_ppgtt *ppgtt =
> >  		container_of(vm, struct i915_hw_ppgtt, base);
> >  	gen6_gtt_pte_t *pt_vaddr;
> > +	unsigned first_entry = start >> PAGE_SHIFT;
> >  	unsigned act_pt = first_entry / I915_PPGTT_PT_ENTRIES;
> >  	unsigned act_pte = first_entry % I915_PPGTT_PT_ENTRIES;
> >  	struct sg_page_iter sg_iter;
> > @@ -1026,8 +1032,7 @@ alloc:
> >  		ppgtt->pt_dma_addr[i] = pt_addr;
> >  	}
> >  
> > -	ppgtt->base.clear_range(&ppgtt->base, 0,
> > -				ppgtt->num_pd_entries * I915_PPGTT_PT_ENTRIES, true);
> > +	ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
> >  	ppgtt->debug_dump = gen6_dump_ppgtt;
> >  
> >  	DRM_DEBUG_DRIVER("Allocated pde space (%ldM) at GTT entry: %lx\n",
> > @@ -1091,20 +1096,17 @@ ppgtt_bind_vma(struct i915_vma *vma,
> >  	       enum i915_cache_level cache_level,
> >  	       u32 flags)
> >  {
> > -	const unsigned long entry = vma->node.start >> PAGE_SHIFT;
> > -
> >  	WARN_ON(flags);
> >  
> > -	vma->vm->insert_entries(vma->vm, vma->obj->pages, entry, cache_level);
> > +	vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start,
> > +				cache_level);
> >  }
> >  
> >  static void ppgtt_unbind_vma(struct i915_vma *vma)
> >  {
> > -	const unsigned long entry = vma->node.start >> PAGE_SHIFT;
> > -
> >  	vma->vm->clear_range(vma->vm,
> > -			     entry,
> > -			     vma->obj->base.size >> PAGE_SHIFT,
> > +			     vma->node.start,
> > +			     vma->obj->base.size,
> >  			     true);
> >  }
> >  
> > @@ -1265,10 +1267,11 @@ static inline void gen8_set_pte(void __iomem *addr, gen8_gtt_pte_t pte)
> >  
> >  static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
> >  				     struct sg_table *st,
> > -				     unsigned int first_entry,
> > +				     uint64_t start,
> >  				     enum i915_cache_level level)
> >  {
> >  	struct drm_i915_private *dev_priv = vm->dev->dev_private;
> > +	unsigned first_entry = start >> PAGE_SHIFT;
> >  	gen8_gtt_pte_t __iomem *gtt_entries =
> >  		(gen8_gtt_pte_t __iomem *)dev_priv->gtt.gsm + first_entry;
> >  	int i = 0;
> > @@ -1310,10 +1313,11 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
> >   */
> >  static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
> >  				     struct sg_table *st,
> > -				     unsigned int first_entry,
> > +				     uint64_t start,
> >  				     enum i915_cache_level level)
> >  {
> >  	struct drm_i915_private *dev_priv = vm->dev->dev_private;
> > +	unsigned first_entry = start >> PAGE_SHIFT;
> >  	gen6_gtt_pte_t __iomem *gtt_entries =
> >  		(gen6_gtt_pte_t __iomem *)dev_priv->gtt.gsm + first_entry;
> >  	int i = 0;
> > @@ -1345,11 +1349,13 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
> >  }
> >  
> >  static void gen8_ggtt_clear_range(struct i915_address_space *vm,
> > -				  unsigned int first_entry,
> > -				  unsigned int num_entries,
> > +				  uint64_t start,
> > +				  uint64_t length,
> >  				  bool use_scratch)
> >  {
> >  	struct drm_i915_private *dev_priv = vm->dev->dev_private;
> > +	unsigned first_entry = start >> PAGE_SHIFT;
> > +	unsigned num_entries = length >> PAGE_SHIFT;
> >  	gen8_gtt_pte_t scratch_pte, __iomem *gtt_base =
> >  		(gen8_gtt_pte_t __iomem *) dev_priv->gtt.gsm + first_entry;
> >  	const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry;
> > @@ -1369,11 +1375,13 @@ static void gen8_ggtt_clear_range(struct i915_address_space *vm,
> >  }
> >  
> >  static void gen6_ggtt_clear_range(struct i915_address_space *vm,
> > -				  unsigned int first_entry,
> > -				  unsigned int num_entries,
> > +				  uint64_t start,
> > +				  uint64_t length,
> >  				  bool use_scratch)
> >  {
> >  	struct drm_i915_private *dev_priv = vm->dev->dev_private;
> > +	unsigned first_entry = start >> PAGE_SHIFT;
> > +	unsigned num_entries = length >> PAGE_SHIFT;
> >  	gen6_gtt_pte_t scratch_pte, __iomem *gtt_base =
> >  		(gen6_gtt_pte_t __iomem *) dev_priv->gtt.gsm + first_entry;
> >  	const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry;
> > @@ -1406,10 +1414,12 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
> >  }
> >  
> >  static void i915_ggtt_clear_range(struct i915_address_space *vm,
> > -				  unsigned int first_entry,
> > -				  unsigned int num_entries,
> > +				  uint64_t start,
> > +				  uint64_t length,
> >  				  bool unused)
> >  {
> > +	unsigned first_entry = start >> PAGE_SHIFT;
> > +	unsigned num_entries = length >> PAGE_SHIFT;
> >  	intel_gtt_clear_range(first_entry, num_entries);
> >  }
> >  
> > @@ -1430,7 +1440,6 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> >  	struct drm_device *dev = vma->vm->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_gem_object *obj = vma->obj;
> > -	const unsigned long entry = vma->node.start >> PAGE_SHIFT;
> >  
> >  	/* If there is no aliasing PPGTT, or the caller needs a global mapping,
> >  	 * or we have a global mapping already but the cacheability flags have
> > @@ -1446,7 +1455,8 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> >  	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
> >  		if (!obj->has_global_gtt_mapping ||
> >  		    (cache_level != obj->cache_level)) {
> > -			vma->vm->insert_entries(vma->vm, obj->pages, entry,
> > +			vma->vm->insert_entries(vma->vm, obj->pages,
> > +						vma->node.start,
> >  						cache_level);
> >  			obj->has_global_gtt_mapping = 1;
> >  		}
> > @@ -1457,7 +1467,9 @@ static void ggtt_bind_vma(struct i915_vma *vma,
> >  	     (cache_level != obj->cache_level))) {
> >  		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
> >  		appgtt->base.insert_entries(&appgtt->base,
> > -					    vma->obj->pages, entry, cache_level);
> > +					    vma->obj->pages,
> > +					    vma->node.start,
> > +					    cache_level);
> >  		vma->obj->has_aliasing_ppgtt_mapping = 1;
> >  	}
> >  }
> > @@ -1467,11 +1479,11 @@ static void ggtt_unbind_vma(struct i915_vma *vma)
> >  	struct drm_device *dev = vma->vm->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_gem_object *obj = vma->obj;
> > -	const unsigned long entry = vma->node.start >> PAGE_SHIFT;
> >  
> >  	if (obj->has_global_gtt_mapping) {
> > -		vma->vm->clear_range(vma->vm, entry,
> > -				     vma->obj->base.size >> PAGE_SHIFT,
> > +		vma->vm->clear_range(vma->vm,
> > +				     vma->node.start,
> > +				     obj->base.size,
> >  				     true);
> >  		obj->has_global_gtt_mapping = 0;
> >  	}
> > @@ -1479,8 +1491,8 @@ static void ggtt_unbind_vma(struct i915_vma *vma)
> >  	if (obj->has_aliasing_ppgtt_mapping) {
> >  		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
> >  		appgtt->base.clear_range(&appgtt->base,
> > -					 entry,
> > -					 obj->base.size >> PAGE_SHIFT,
> > +					 vma->node.start,
> > +					 obj->base.size,
> >  					 true);
> >  		obj->has_aliasing_ppgtt_mapping = 0;
> >  	}
> > @@ -1565,14 +1577,14 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
> >  
> >  	/* Clear any non-preallocated blocks */
> >  	drm_mm_for_each_hole(entry, &ggtt_vm->mm, hole_start, hole_end) {
> > -		const unsigned long count = (hole_end - hole_start) / PAGE_SIZE;
> >  		DRM_DEBUG_KMS("clearing unused GTT space: [%lx, %lx]\n",
> >  			      hole_start, hole_end);
> > -		ggtt_vm->clear_range(ggtt_vm, hole_start / PAGE_SIZE, count, true);
> > +		ggtt_vm->clear_range(ggtt_vm, hole_start,
> > +				     hole_end - hole_start, true);
> >  	}
> >  
> >  	/* And finally clear the reserved guard page */
> > -	ggtt_vm->clear_range(ggtt_vm, end / PAGE_SIZE - 1, 1, true);
> > +	ggtt_vm->clear_range(ggtt_vm, end - PAGE_SIZE, PAGE_SIZE, true);
> >  }
> >  
> >  void i915_gem_init_global_gtt(struct drm_device *dev)
> 
> 

-- 
Ben Widawsky, Intel Open Source Technology Center



More information about the Intel-gfx mailing list