From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 2/9] drm/i915/bdw: Reorganize PPGTT init Date: Wed, 19 Feb 2014 16:59:49 +0200 Message-ID: <1392821989.19792.13.camel@intelbox> References: <1392244132-6806-1-git-send-email-benjamin.widawsky@intel.com> <1392244132-6806-3-git-send-email-benjamin.widawsky@intel.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0734203569==" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 14067FAE0C for ; Wed, 19 Feb 2014 07:00:27 -0800 (PST) In-Reply-To: <1392244132-6806-3-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 --===============0734203569== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-eO3cn4UdoUdfTbwcK8C1" --=-eO3cn4UdoUdfTbwcK8C1 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2014-02-12 at 14:28 -0800, Ben Widawsky wrote: > Create 3 clear stages in PPGTT init. This will help with upcoming > changes be more readable. The 3 stages are, allocation, dma mapping, and > writing the P[DT]Es >=20 > One nice benefit to the patches is that it makes 2 very clear error > points, allocation, and mapping, and avoids having to do any handling > after writing PTEs (something which was likely buggy before). This > simplified error handling I suspect will be helpful when we move to > deferred/dynamic page table allocation and mapping. >=20 > The patches also attempts to break up some of the steps into more > logical reviewable chunks, particularly when we free. >=20 > v2: Don't call cleanup on the error path since that takes down the > drm_mm and list entry, which aren't setup at this point. >=20 > Signed-off-by: Ben Widawsky > --- > drivers/gpu/drm/i915/i915_drv.h | 2 +- > drivers/gpu/drm/i915/i915_gem_gtt.c | 124 +++++++++++++++++++++---------= ------ > 2 files changed, 73 insertions(+), 53 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_= drv.h > index 2572a95..cecbb9a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -709,7 +709,7 @@ struct i915_hw_ppgtt { > }; > union { > dma_addr_t *pt_dma_addr; > - dma_addr_t *gen8_pt_dma_addr[4]; > + dma_addr_t **gen8_pt_dma_addr; If there isn't any reason to allocate this dynamically I'd just leave the static array. This would make the error path a bit simpler and be more symmetric wrt. pd_dma_addr which is also a static array. > }; > =20 > int (*enable)(struct i915_hw_ppgtt *ppgtt); > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i= 915_gem_gtt.c > index ee38faf..c6c221c 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -326,12 +326,14 @@ static void gen8_ppgtt_free(struct i915_hw_ppgtt *p= pgtt) > for (i =3D 0; i < ppgtt->num_pd_pages ; i++) > kfree(ppgtt->gen8_pt_dma_addr[i]); > =20 > + kfree(ppgtt->gen8_pt_dma_addr); > __free_pages(ppgtt->gen8_pt_pages, get_order(ppgtt->num_pt_pages << PAG= E_SHIFT)); > __free_pages(ppgtt->pd_pages, get_order(ppgtt->num_pd_pages << PAGE_SHI= FT)); > } > =20 > static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt) > { > + struct pci_dev *hwdev =3D ppgtt->base.dev->pdev; > int i, j; > =20 > for (i =3D 0; i < ppgtt->num_pd_pages; i++) { > @@ -340,18 +342,14 @@ static void gen8_ppgtt_unmap_pages(struct i915_hw_p= pgtt *ppgtt) > if (!ppgtt->pd_dma_addr[i]) > continue; > =20 > - pci_unmap_page(ppgtt->base.dev->pdev, > - ppgtt->pd_dma_addr[i], > - PAGE_SIZE, PCI_DMA_BIDIRECTIONAL); > + pci_unmap_page(hwdev, ppgtt->pd_dma_addr[i], PAGE_SIZE, > + PCI_DMA_BIDIRECTIONAL); > =20 > for (j =3D 0; j < GEN8_PDES_PER_PAGE; j++) { > dma_addr_t addr =3D ppgtt->gen8_pt_dma_addr[i][j]; > if (addr) > - pci_unmap_page(ppgtt->base.dev->pdev, > - addr, > - PAGE_SIZE, > - PCI_DMA_BIDIRECTIONAL); > - > + pci_unmap_page(hwdev, addr, PAGE_SIZE, > + PCI_DMA_BIDIRECTIONAL); > } > } > } > @@ -369,27 +367,26 @@ static void gen8_ppgtt_cleanup(struct i915_address_= space *vm) > } > =20 > /** > - * GEN8 legacy ppgtt programming is accomplished through 4 PDP registers= with a > - * net effect resembling a 2-level page table in normal x86 terms. Each = PDP > - * represents 1GB of memory > - * 4 * 512 * 512 * 4096 =3D 4GB legacy 32b address space. > + * GEN8 legacy ppgtt programming is accomplished through a max 4 PDP reg= isters > + * with a net effect resembling a 2-level page table in normal x86 terms= . Each > + * PDP represents 1GB of memory 4 * 512 * 512 * 4096 =3D 4GB legacy 32b = address > + * space. > * > + * FIXME: split allocation into smaller pieces. For now we only ever do = this > + * once, but with full PPGTT, the multiple contiguous allocations will b= e bad. > * TODO: Do something with the size parameter > - **/ > + */ > static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size) > { > struct page *pt_pages; > - int i, j, ret =3D -ENOMEM; > const int max_pdp =3D DIV_ROUND_UP(size, 1 << 30); > const int num_pt_pages =3D GEN8_PDES_PER_PAGE * max_pdp; > + int i, j, ret; > =20 > if (size % (1<<30)) > DRM_INFO("Pages will be wasted unless GTT size (%llu) is divisible by = 1GB\n", size); > =20 > - /* FIXME: split allocation into smaller pieces. For now we only ever do > - * this once, but with full PPGTT, the multiple contiguous allocations > - * will be bad. > - */ > + /* 1. Do all our allocations for page directories and page tables */ > ppgtt->pd_pages =3D alloc_pages(GFP_KERNEL, get_order(max_pdp << PAGE_S= HIFT)); > if (!ppgtt->pd_pages) > return -ENOMEM; > @@ -404,52 +401,66 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *pp= gtt, uint64_t size) > ppgtt->num_pd_pages =3D 1 << get_order(max_pdp << PAGE_SHIFT); > ppgtt->num_pt_pages =3D 1 << get_order(num_pt_pages << PAGE_SHIFT); > ppgtt->num_pd_entries =3D max_pdp * GEN8_PDES_PER_PAGE; > - ppgtt->enable =3D gen8_ppgtt_enable; > - ppgtt->switch_mm =3D gen8_mm_switch; > - ppgtt->base.clear_range =3D gen8_ppgtt_clear_range; > - ppgtt->base.insert_entries =3D gen8_ppgtt_insert_entries; > - ppgtt->base.cleanup =3D gen8_ppgtt_cleanup; > - ppgtt->base.start =3D 0; > - ppgtt->base.total =3D ppgtt->num_pt_pages * GEN8_PTES_PER_PAGE * PAGE_S= IZE; > - > BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPS); > =20 > + ppgtt->gen8_pt_dma_addr =3D kcalloc(max_pdp, > + sizeof(*ppgtt->gen8_pt_dma_addr), > + GFP_KERNEL); > + if (!ppgtt->gen8_pt_dma_addr) { > + ret =3D -ENOMEM; > + goto bail; On the error path, in gen8_ppgtt_free() we'd dereference the above NULL ptr. > + } > + > + for (i =3D 0; i < max_pdp; i++) { > + ppgtt->gen8_pt_dma_addr[i] =3D kcalloc(GEN8_PDES_PER_PAGE, > + sizeof(dma_addr_t), > + GFP_KERNEL); > + if (!ppgtt->gen8_pt_dma_addr[i]) { > + ret =3D -ENOMEM; > + goto bail; > + } > + } > + > /* > - * - Create a mapping for the page directories. > - * - For each page directory: > - * allocate space for page table mappings. > - * map each page table > + * 2. Create all the DMA mappings for the page directories and page > + * tables > */ > for (i =3D 0; i < max_pdp; i++) { > - dma_addr_t temp; > - temp =3D pci_map_page(ppgtt->base.dev->pdev, > - &ppgtt->pd_pages[i], 0, > - PAGE_SIZE, PCI_DMA_BIDIRECTIONAL); > - if (pci_dma_mapping_error(ppgtt->base.dev->pdev, temp)) > - goto err_out; > - > - ppgtt->pd_dma_addr[i] =3D temp; > - > - ppgtt->gen8_pt_dma_addr[i] =3D kmalloc(sizeof(dma_addr_t) * GEN8_PDES_= PER_PAGE, GFP_KERNEL); > - if (!ppgtt->gen8_pt_dma_addr[i]) > - goto err_out; > + dma_addr_t pd_addr, pt_addr; > =20 > + /* Get the page table mappings per page directory */ > for (j =3D 0; j < GEN8_PDES_PER_PAGE; j++) { > struct page *p =3D &pt_pages[i * GEN8_PDES_PER_PAGE + j]; > - temp =3D pci_map_page(ppgtt->base.dev->pdev, > - p, 0, PAGE_SIZE, > - PCI_DMA_BIDIRECTIONAL); > =20 > - if (pci_dma_mapping_error(ppgtt->base.dev->pdev, temp)) > - goto err_out; > + pt_addr =3D pci_map_page(ppgtt->base.dev->pdev, > + p, 0, PAGE_SIZE, > + PCI_DMA_BIDIRECTIONAL); > + ret =3D pci_dma_mapping_error(ppgtt->base.dev->pdev, pt_addr); > + if (ret) > + goto bail; > =20 > - ppgtt->gen8_pt_dma_addr[i][j] =3D temp; > + ppgtt->gen8_pt_dma_addr[i][j] =3D pt_addr; > } > + > + /* And the page directory mappings */ > + pd_addr =3D pci_map_page(ppgtt->base.dev->pdev, > + &ppgtt->pd_pages[i], 0, > + PAGE_SIZE, PCI_DMA_BIDIRECTIONAL); > + ret =3D pci_dma_mapping_error(ppgtt->base.dev->pdev, pd_addr); > + if (ret) > + goto bail; The error path here would leak the above page table mappings, since ppgtt->pd_dma_addr[i] is still zero, but in gen8_ppgtt_unmap_pages() we do a if (!ppgtt->pd_dma_addr[i]) continue; skipping the page table unmap part. This is reworked in your later patches, but the issue is still there in the final version.=20 > + > + ppgtt->pd_dma_addr[i] =3D pd_addr; > } > =20 > - /* For now, the PPGTT helper functions all require that the PDEs are > + /* > + * 3. Map all the page directory entires to point to the page tables > + * we've allocated. > + * > + * For now, the PPGTT helper functions all require that the PDEs are > * plugged in correctly. So we do that now/here. For aliasing PPGTT, we > - * will never need to touch the PDEs again */ > + * will never need to touch the PDEs again. > + */ > for (i =3D 0; i < max_pdp; i++) { > gen8_ppgtt_pde_t *pd_vaddr; > pd_vaddr =3D kmap_atomic(&ppgtt->pd_pages[i]); > @@ -461,6 +472,14 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppg= tt, uint64_t size) > kunmap_atomic(pd_vaddr); > } > =20 > + ppgtt->enable =3D gen8_ppgtt_enable; > + ppgtt->switch_mm =3D gen8_mm_switch; > + ppgtt->base.clear_range =3D gen8_ppgtt_clear_range; > + ppgtt->base.insert_entries =3D gen8_ppgtt_insert_entries; > + ppgtt->base.cleanup =3D gen8_ppgtt_cleanup; > + ppgtt->base.start =3D 0; > + ppgtt->base.total =3D ppgtt->num_pt_pages * GEN8_PTES_PER_PAGE * PAGE_S= IZE; > + > ppgtt->base.clear_range(&ppgtt->base, 0, > ppgtt->num_pd_entries * GEN8_PTES_PER_PAGE, > true); > @@ -473,8 +492,9 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgt= t, uint64_t size) > size % (1<<30)); > return 0; > =20 > -err_out: > - ppgtt->base.cleanup(&ppgtt->base); > +bail: > + gen8_ppgtt_unmap_pages(ppgtt); > + gen8_ppgtt_free(ppgtt); > return ret; > } > =20 --=-eO3cn4UdoUdfTbwcK8C1 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) iQEcBAABAgAGBQJTBMblAAoJEORIIAnNuWDFcgAH/03mnC87bEI+q+Z0SFETesFk 0qvpczs9TzHFmAZRdeDlM2tDNcnklO2TrKzOWqR2AZ6f6Bo8VXKJJTDvu4JXAZcW iMOAqdw3/dcHLpsNxJxX/P4F4lT0Ugmq5nFOAAEezdHXXMWgRWF/wHb3sNyaUoec iqdLbkjMMiInSVKXtSjXigFYuNi0Efo2SUWBhhet6tovm46n1r8RqHn5MRZYGv/F cPgK2xj3ibwO4lwyWK9YBUO3S7gfnQ6beid0RcXkUsoGgEUWca174qpjz9Pm9kvW Gopr1xgOTd9KGI3FK9z7JgXR+l+MTJgUBJ8+JDFEuh7m3cYa0G7K/D7bogZD0bE= =DIEF -----END PGP SIGNATURE----- --=-eO3cn4UdoUdfTbwcK8C1-- --===============0734203569== 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 --===============0734203569==--