From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 3/9] drm/i915/bdw: Split ppgtt initialization up Date: Thu, 20 Feb 2014 15:10:58 +0200 Message-ID: <1392901858.14149.4.camel@intelbox> References: <1392244132-6806-1-git-send-email-benjamin.widawsky@intel.com> <1392876349-24684-4-git-send-email-benjamin.widawsky@intel.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0629968687==" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 02B1DFB23B for ; Thu, 20 Feb 2014 05:11:02 -0800 (PST) In-Reply-To: <1392876349-24684-4-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 --===============0629968687== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-/GPhb8hdmR+rA3R7209W" --=-/GPhb8hdmR+rA3R7209W Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2014-02-19 at 22:05 -0800, Ben Widawsky wrote: > Like cleanup in an earlier patch, the code becomes much more readable, > and easier to extend if we extract out helper functions for the various > stages of init. >=20 > Note that with this patch it becomes really simple, and tempting to begin > using the 'goto out' idiom with explicit free/fini semantics. I've > kept the error path as similar as possible to the cleanup() function to > make sure cleanup is as robust as possible >=20 > v2: Remove comment "NB:From here on, ppgtt->base.cleanup() should > function properly" > Update commit message to reflect above >=20 > v3: Rebased on top of bugfixes found in the previous patch by Imre > Moved number of pd pages assertion to the proper place (Imre) >=20 > v4: > Allocate dma address space for num_pd_pages, not num_pd_entries (Ben) > Don't use gen8_pt_dma_addr after free on error path (Imre) > With new fix from v4 of the previous patch. >=20 > Signed-off-by: Ben Widawsky Looks ok to me: Reviewed-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 164 +++++++++++++++++++++++++-----= ------ > 1 file changed, 116 insertions(+), 48 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i= 915_gem_gtt.c > index 7956659..0af3587 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -366,6 +366,113 @@ static void gen8_ppgtt_cleanup(struct i915_address_= space *vm) > gen8_ppgtt_free(ppgtt); > } > =20 > +static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt, > + const int max_pdp) > +{ > + struct page *pt_pages; > + const int num_pt_pages =3D GEN8_PDES_PER_PAGE * max_pdp; > + > + pt_pages =3D alloc_pages(GFP_KERNEL, get_order(num_pt_pages << PAGE_SHI= FT)); > + if (!pt_pages) > + return -ENOMEM; > + > + ppgtt->gen8_pt_pages =3D pt_pages; > + ppgtt->num_pt_pages =3D 1 << get_order(num_pt_pages << PAGE_SHIFT); > + > + return 0; > +} > + > +static int gen8_ppgtt_allocate_dma(struct i915_hw_ppgtt *ppgtt) > +{ > + int i; > + > + for (i =3D 0; i < ppgtt->num_pd_pages; 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]) > + return -ENOMEM; > + } > + > + return 0; > +} > + > +static int gen8_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *pp= gtt, > + const int max_pdp) > +{ > + ppgtt->pd_pages =3D alloc_pages(GFP_KERNEL, get_order(max_pdp << PAGE_S= HIFT)); > + if (!ppgtt->pd_pages) > + return -ENOMEM; > + > + ppgtt->num_pd_pages =3D 1 << get_order(max_pdp << PAGE_SHIFT); > + BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPS); > + > + return 0; > +} > + > +static int gen8_ppgtt_alloc(struct i915_hw_ppgtt *ppgtt, > + const int max_pdp) > +{ > + int ret; > + > + ret =3D gen8_ppgtt_allocate_page_directories(ppgtt, max_pdp); > + if (ret) > + return ret; > + > + ret =3D gen8_ppgtt_allocate_page_tables(ppgtt, max_pdp); > + if (ret) { > + __free_pages(ppgtt->pd_pages, get_order(max_pdp << PAGE_SHIFT)); > + return ret; > + } > + > + ppgtt->num_pd_entries =3D max_pdp * GEN8_PDES_PER_PAGE; > + > + ret =3D gen8_ppgtt_allocate_dma(ppgtt); > + if (ret) > + gen8_ppgtt_free(ppgtt); > + > + return ret; > +} > + > +static int gen8_ppgtt_setup_page_directories(struct i915_hw_ppgtt *ppgtt= , > + const int pd) > +{ > + dma_addr_t pd_addr; > + int ret; > + > + pd_addr =3D pci_map_page(ppgtt->base.dev->pdev, > + &ppgtt->pd_pages[pd], 0, > + PAGE_SIZE, PCI_DMA_BIDIRECTIONAL); > + > + ret =3D pci_dma_mapping_error(ppgtt->base.dev->pdev, pd_addr); > + if (ret) > + return ret; > + > + ppgtt->pd_dma_addr[pd] =3D pd_addr; > + > + return 0; > +} > + > +static int gen8_ppgtt_setup_page_tables(struct i915_hw_ppgtt *ppgtt, > + const int pd, > + const int pt) > +{ > + dma_addr_t pt_addr; > + struct page *p; > + int ret; > + > + p =3D &ppgtt->gen8_pt_pages[pd * GEN8_PDES_PER_PAGE + pt]; > + 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) > + return ret; > + > + ppgtt->gen8_pt_dma_addr[pd][pt] =3D pt_addr; > + > + return 0; > +} > + > /** > * 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 > @@ -378,69 +485,30 @@ static void gen8_ppgtt_cleanup(struct i915_address_= space *vm) > */ > static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size) > { > - struct page *pt_pages; > const int max_pdp =3D DIV_ROUND_UP(size, 1 << 30); > - const int num_pt_pages =3D GEN8_PDES_PER_PAGE * max_pdp; > - struct pci_dev *hwdev =3D ppgtt->base.dev->pdev; > + const int min_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 > - /* 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; > - > - pt_pages =3D alloc_pages(GFP_KERNEL, get_order(num_pt_pages << PAGE_SHI= FT)); > - if (!pt_pages) { > - __free_pages(ppgtt->pd_pages, get_order(max_pdp << PAGE_SHIFT)); > - return -ENOMEM; > - } > - > - ppgtt->gen8_pt_pages =3D pt_pages; > - 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; > - BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPS); > - > - 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; > - } > - } > + /* 1. Do all our allocations for page directories and page tables. */ > + ret =3D gen8_ppgtt_alloc(ppgtt, max_pdp); > + if (ret) > + return ret; > =20 > /* > - * 2. Create all the DMA mappings for the page directories and page > - * tables > + * 2. Create DMA mappings for the page directories and page tables. > */ > for (i =3D 0; i < max_pdp; i++) { > - dma_addr_t pd_addr, pt_addr; > - > - /* Get the page directory mappings */ > - pd_addr =3D pci_map_page(hwdev, &ppgtt->pd_pages[i], 0, > - PAGE_SIZE, PCI_DMA_BIDIRECTIONAL); > - ret =3D pci_dma_mapping_error(ppgtt->base.dev->pdev, pd_addr); > + ret =3D gen8_ppgtt_setup_page_directories(ppgtt, i); > if (ret) > goto bail; > =20 > - ppgtt->pd_dma_addr[i] =3D pd_addr; > - > - /* And 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]; > - > - pt_addr =3D pci_map_page(hwdev, p, 0, PAGE_SIZE, > - PCI_DMA_BIDIRECTIONAL); > - ret =3D pci_dma_mapping_error(hwdev, pt_addr); > + ret =3D gen8_ppgtt_setup_page_tables(ppgtt, i, j); > if (ret) > goto bail; > - > - ppgtt->gen8_pt_dma_addr[i][j] =3D pt_addr; > } > } > =20 > @@ -479,7 +547,7 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgt= t, uint64_t size) > ppgtt->num_pd_pages, ppgtt->num_pd_pages - max_pdp); > DRM_DEBUG_DRIVER("Allocated %d pages for page tables (%lld wasted)\n", > ppgtt->num_pt_pages, > - (ppgtt->num_pt_pages - num_pt_pages) + > + (ppgtt->num_pt_pages - min_pt_pages) + > size % (1<<30)); > return 0; > =20 --=-/GPhb8hdmR+rA3R7209W 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) iQEcBAABAgAGBQJTBf7iAAoJEORIIAnNuWDF/pgH/3rHP0Vb6ef9dfr7392ghG6O CrSKlLlCM3VHMJhHgyTwBg6pCMSKEn7cpfb3RONLcN4wHIG5L4Y6EtF5iL/hoT2V zczbshfo0nJY8tVU9hrvKO2KEy8Pr35qklQ6NnEEi8t7QLOJ2QTjiyVSiVcn0Lvb gFnOrMO47qf7ySAoapVwXKV0a77C4uvtkqubZs3u7XFDaC3bmckrMXzHcLR3qn8N st4zGnOBtniDn2FS2+eftU7VvP3KhcNn4xKEcbhxau7Cljtt/hN+4wzXjuJpf8JG o1EM98XR5dzqd+HpQ9XNs+JjsYuLfTGwIYCqNRTWamNdGGooeTrJ5RiXVsaZ/Dw= =OqIZ -----END PGP SIGNATURE----- --=-/GPhb8hdmR+rA3R7209W-- --===============0629968687== 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 --===============0629968687==--