From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 4/9] drm/i915: Make clear/insert vfuncs args absolute Date: Wed, 19 Feb 2014 19:26:53 +0200 Message-ID: <1392830813.19792.38.camel@intelbox> References: <1392244132-6806-1-git-send-email-benjamin.widawsky@intel.com> <1392244132-6806-5-git-send-email-benjamin.widawsky@intel.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0354245372==" Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 99A07FAE9F for ; Wed, 19 Feb 2014 09:28:01 -0800 (PST) In-Reply-To: <1392244132-6806-5-git-send-email-benjamin.widawsky@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Ben Widawsky Cc: Intel GFX , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org --===============0354245372== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-u74YtOZAIBx1iO0Nc5ff" --=-u74YtOZAIBx1iO0Nc5ff Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2014-02-12 at 14:28 -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. >=20 > 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. >=20 > Signed-off-by: Ben Widawsky I haven't found any issues with this patch, but Chris' comment on size_t makes sense. So with that changed: Reviewed-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_drv.h | 6 +-- > drivers/gpu/drm/i915/i915_gem_gtt.c | 80 +++++++++++++++++++++----------= ------ > 2 files changed, 49 insertions(+), 37 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_= drv.h > index cecbb9a..2ebad96 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, > + size_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/i= 915_gem_gtt.c > index 8a5cad9..5bfc6ff 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 *ppg= tt, > } > =20 > static void gen8_ppgtt_clear_range(struct i915_address_space *vm, > - unsigned first_entry, > - unsigned num_entries, > + uint64_t start, > + size_t length, > bool use_scratch) > { > struct i915_hw_ppgtt *ppgtt =3D > container_of(vm, struct i915_hw_ppgtt, base); > gen8_gtt_pte_t *pt_vaddr, scratch_pte; > + unsigned first_entry =3D start >> PAGE_SHIFT; > + unsigned num_entries =3D length >> PAGE_SHIFT; > unsigned act_pt =3D first_entry / GEN8_PTES_PER_PAGE; > unsigned first_pte =3D first_entry % GEN8_PTES_PER_PAGE; > unsigned last_pte, i; > @@ -290,12 +292,13 @@ static void gen8_ppgtt_clear_range(struct i915_addr= ess_space *vm, > =20 > 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 =3D > container_of(vm, struct i915_hw_ppgtt, base); > gen8_gtt_pte_t *pt_vaddr; > + unsigned first_entry =3D start >> PAGE_SHIFT; > unsigned act_pt =3D first_entry / GEN8_PTES_PER_PAGE; > unsigned act_pte =3D first_entry % GEN8_PTES_PER_PAGE; > struct sg_page_iter sg_iter; > @@ -866,13 +869,15 @@ static int gen6_ppgtt_enable(struct i915_hw_ppgtt *= ppgtt) > =20 > /* 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, > + size_t length, > bool use_scratch) > { > struct i915_hw_ppgtt *ppgtt =3D > container_of(vm, struct i915_hw_ppgtt, base); > gen6_gtt_pte_t *pt_vaddr, scratch_pte; > + unsigned first_entry =3D start >> PAGE_SHIFT; > + unsigned num_entries =3D length >> PAGE_SHIFT; > unsigned act_pt =3D first_entry / I915_PPGTT_PT_ENTRIES; > unsigned first_pte =3D first_entry % I915_PPGTT_PT_ENTRIES; > unsigned last_pte, i; > @@ -899,12 +904,13 @@ static void gen6_ppgtt_clear_range(struct i915_addr= ess_space *vm, > =20 > 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 =3D > container_of(vm, struct i915_hw_ppgtt, base); > gen6_gtt_pte_t *pt_vaddr; > + unsigned first_entry =3D start >> PAGE_SHIFT; > unsigned act_pt =3D first_entry / I915_PPGTT_PT_ENTRIES; > unsigned act_pte =3D first_entry % I915_PPGTT_PT_ENTRIES; > struct sg_page_iter sg_iter; > @@ -1037,8 +1043,7 @@ alloc: > ppgtt->pt_dma_addr[i] =3D pt_addr; > } > =20 > - 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 =3D gen6_dump_ppgtt; > =20 > DRM_DEBUG_DRIVER("Allocated pde space (%ldM) at GTT entry: %lx\n", > @@ -1102,20 +1107,17 @@ ppgtt_bind_vma(struct i915_vma *vma, > enum i915_cache_level cache_level, > u32 flags) > { > - const unsigned long entry =3D vma->node.start >> PAGE_SHIFT; > - > WARN_ON(flags); > =20 > - 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); > } > =20 > static void ppgtt_unbind_vma(struct i915_vma *vma) > { > - const unsigned long entry =3D 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); > } > =20 > @@ -1276,10 +1278,11 @@ static inline void gen8_set_pte(void __iomem *add= r, gen8_gtt_pte_t pte) > =20 > 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 =3D vm->dev->dev_private; > + unsigned first_entry =3D start >> PAGE_SHIFT; > gen8_gtt_pte_t __iomem *gtt_entries =3D > (gen8_gtt_pte_t __iomem *)dev_priv->gtt.gsm + first_entry; > int i =3D 0; > @@ -1321,10 +1324,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 =3D vm->dev->dev_private; > + unsigned first_entry =3D start >> PAGE_SHIFT; > gen6_gtt_pte_t __iomem *gtt_entries =3D > (gen6_gtt_pte_t __iomem *)dev_priv->gtt.gsm + first_entry; > int i =3D 0; > @@ -1356,11 +1360,13 @@ static void gen6_ggtt_insert_entries(struct i915_= address_space *vm, > } > =20 > static void gen8_ggtt_clear_range(struct i915_address_space *vm, > - unsigned int first_entry, > - unsigned int num_entries, > + uint64_t start, > + size_t length, > bool use_scratch) > { > struct drm_i915_private *dev_priv =3D vm->dev->dev_private; > + unsigned first_entry =3D start >> PAGE_SHIFT; > + unsigned num_entries =3D length >> PAGE_SHIFT; > gen8_gtt_pte_t scratch_pte, __iomem *gtt_base =3D > (gen8_gtt_pte_t __iomem *) dev_priv->gtt.gsm + first_entry; > const int max_entries =3D gtt_total_entries(dev_priv->gtt) - first_entr= y; > @@ -1380,11 +1386,13 @@ static void gen8_ggtt_clear_range(struct i915_add= ress_space *vm, > } > =20 > static void gen6_ggtt_clear_range(struct i915_address_space *vm, > - unsigned int first_entry, > - unsigned int num_entries, > + uint64_t start, > + size_t length, > bool use_scratch) > { > struct drm_i915_private *dev_priv =3D vm->dev->dev_private; > + unsigned first_entry =3D start >> PAGE_SHIFT; > + unsigned num_entries =3D length >> PAGE_SHIFT; > gen6_gtt_pte_t scratch_pte, __iomem *gtt_base =3D > (gen6_gtt_pte_t __iomem *) dev_priv->gtt.gsm + first_entry; > const int max_entries =3D gtt_total_entries(dev_priv->gtt) - first_entr= y; > @@ -1417,10 +1425,12 @@ static void i915_ggtt_bind_vma(struct i915_vma *v= ma, > } > =20 > static void i915_ggtt_clear_range(struct i915_address_space *vm, > - unsigned int first_entry, > - unsigned int num_entries, > + uint64_t start, > + size_t length, > bool unused) > { > + unsigned first_entry =3D start >> PAGE_SHIFT; > + unsigned num_entries =3D length >> PAGE_SHIFT; > intel_gtt_clear_range(first_entry, num_entries); > } > =20 > @@ -1441,7 +1451,6 @@ static void ggtt_bind_vma(struct i915_vma *vma, > struct drm_device *dev =3D vma->vm->dev; > struct drm_i915_private *dev_priv =3D dev->dev_private; > struct drm_i915_gem_object *obj =3D vma->obj; > - const unsigned long entry =3D vma->node.start >> PAGE_SHIFT; > =20 > /* 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 > @@ -1457,7 +1466,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 !=3D 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 =3D 1; > } > @@ -1468,7 +1478,9 @@ static void ggtt_bind_vma(struct i915_vma *vma, > (cache_level !=3D obj->cache_level))) { > struct i915_hw_ppgtt *appgtt =3D 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 =3D 1; > } > } > @@ -1478,11 +1490,11 @@ static void ggtt_unbind_vma(struct i915_vma *vma) > struct drm_device *dev =3D vma->vm->dev; > struct drm_i915_private *dev_priv =3D dev->dev_private; > struct drm_i915_gem_object *obj =3D vma->obj; > - const unsigned long entry =3D vma->node.start >> PAGE_SHIFT; > =20 > 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 =3D 0; > } > @@ -1490,8 +1502,8 @@ static void ggtt_unbind_vma(struct i915_vma *vma) > if (obj->has_aliasing_ppgtt_mapping) { > struct i915_hw_ppgtt *appgtt =3D 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 =3D 0; > } > @@ -1576,14 +1588,14 @@ void i915_gem_setup_global_gtt(struct drm_device = *dev, > =20 > /* Clear any non-preallocated blocks */ > drm_mm_for_each_hole(entry, &ggtt_vm->mm, hole_start, hole_end) { > - const unsigned long count =3D (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); > } > =20 > /* 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); > } > =20 > void i915_gem_init_global_gtt(struct drm_device *dev) --=-u74YtOZAIBx1iO0Nc5ff Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQEcBAABAgAGBQJTBOldAAoJEORIIAnNuWDFnpsIAK6wy4TfekH1CxlVZqDmKKZv uvhmrZ/AL5+SA4b5kNuSYDb1/xkcW7i0MLTBfhwmspj5bw3PIW4WKTGfSNN9SHqk H6H76yx98rIBmC3pxwAo1DcY4wGXABm3Kd0gibLHPhijBwFb9ZBQKyXi9aWxC4P7 eCADs6r9WrPiRIGs4sqEMGhPUenBDhDdiUAPYPkw99mkAJ/bHYggn4dcUaWRQS7V f6STIPRjhhvrJJP39H2u42ckakO7L31/+oNIh+0zlu7wzeco7jSAZiVC5AgdePcg IH+7G6xylVvVHH9GBSAusZ2udvXke0o1o8i+CWWDyhJKflzKDZ3QpWHjP6QW+lI= =yaGZ -----END PGP SIGNATURE----- --=-u74YtOZAIBx1iO0Nc5ff-- --===============0354245372== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0354245372==--