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: Wed, 19 Feb 2014 19:03:21 +0200 Message-ID: <1392829401.19792.34.camel@intelbox> References: <1392244132-6806-1-git-send-email-benjamin.widawsky@intel.com> <1392244132-6806-4-git-send-email-benjamin.widawsky@intel.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1395297170==" Return-path: Received: from mga14.intel.com (mga14.intel.com [143.182.124.37]) by gabe.freedesktop.org (Postfix) with ESMTP id 25EDBFAE0F for ; Wed, 19 Feb 2014 09:03:50 -0800 (PST) In-Reply-To: <1392244132-6806-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 --===============1395297170== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-UfSc81BXSdGaQTcHyRrG" --=-UfSc81BXSdGaQTcHyRrG Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, 2014-02-12 at 14:28 -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 > Signed-off-by: Ben Widawsky > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 182 +++++++++++++++++++++++++-----= ------ > 1 file changed, 126 insertions(+), 56 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i= 915_gem_gtt.c > index c6c221c..8a5cad9 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -366,91 +366,161 @@ static void gen8_ppgtt_cleanup(struct i915_address= _space *vm) > gen8_ppgtt_free(ppgtt); > } > =20 > -/** > - * 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) > +static int gen8_ppgtt_allocate_page_tables(struct i915_hw_ppgtt *ppgtt, > + const int max_pdp) > { > 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; > - int i, j, ret; > - > - if (size % (1<<30)) > - DRM_INFO("Pages will be wasted unless GTT size (%llu) is divisible by = 1GB\n", size); > - > - /* 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; > =20 > 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)); > + if (!pt_pages) > return -ENOMEM; > - } > =20 > 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); > =20 > - ppgtt->gen8_pt_dma_addr =3D kcalloc(max_pdp, > + return 0; > +} > + > +static int gen8_ppgtt_allocate_dma(struct i915_hw_ppgtt *ppgtt) > +{ > + int i; > + > + ppgtt->gen8_pt_dma_addr =3D kcalloc(ppgtt->num_pd_entries, > sizeof(*ppgtt->gen8_pt_dma_addr), > GFP_KERNEL); > - if (!ppgtt->gen8_pt_dma_addr) { > - ret =3D -ENOMEM; > - goto bail; > - } > + if (!ppgtt->gen8_pt_dma_addr) > + return -ENOMEM; > =20 > - for (i =3D 0; i < max_pdp; i++) { > + for (i =3D 0; i < ppgtt->num_pd_entries; 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; > + kfree(ppgtt->gen8_pt_dma_addr); > + while(i--) > + kfree(ppgtt->gen8_pt_dma_addr[i]); > + > + return -ENOMEM; > } > } > =20 > + 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); > + > + 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; > + BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPS); This check belongs to gen8_ppgtt_allocate_page_directories(). > + > + ret =3D gen8_ppgtt_allocate_dma(ppgtt); > + if (ret) > + gen8_ppgtt_free(ppgtt); Just for reference, this the same ppgtt->gen8_pt_dma_addr NULL deref issue on the error path as in 2/9. > + > + 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 > + * 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) > +{ > + const int max_pdp =3D DIV_ROUND_UP(size, 1 << 30); > + const int min_pt_pages =3D GEN8_PDES_PER_PAGE * max_pdp; > + int i, j, ret; > + > + if (size % (1<<30)) > + DRM_INFO("Pages will be wasted unless GTT size (%llu) is divisible by = 1GB\n", size); > + > + /* 1. Do all our allocations for page directories and page tables. */ > + ret =3D gen8_ppgtt_alloc(ppgtt, max_pdp); > + if (ret) > + return ret; > + > /* > - * 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 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(ppgtt->base.dev->pdev, > - p, 0, PAGE_SIZE, > - PCI_DMA_BIDIRECTIONAL); > - ret =3D pci_dma_mapping_error(ppgtt->base.dev->pdev, 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 > - /* 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); > + ret =3D gen8_ppgtt_setup_page_directories(ppgtt, i); > if (ret) > goto bail; Again only for reference the same leaked mappings for page tables on the error path as in 2/9. > - > - ppgtt->pd_dma_addr[i] =3D pd_addr; > } > =20 > /* > @@ -488,7 +558,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 --=-UfSc81BXSdGaQTcHyRrG 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) iQEcBAABAgAGBQJTBOPZAAoJEORIIAnNuWDFDAwH/1MRA3IZzv8EWXkOJbNa5Iuw S6Pw7aC8OEGNNJkEcgr982MAUqUJ2AOEXrHqVlqVQokszP2HigrCSPQaEQowQCaH BlIORFitPkmh5ITOLNSiptSE748wEiSA1ASPbFAHx1E+/b6heag01BY9fPLO5wDV P37969/BKwb41KInrlSar+AoZUcfq15Be6kAqL2YZmrTz4OqqPgDEbZo7UYPv3kR GJAEJ7Y9E/kbMGo1i7BVlrbPrGox3EGL/Fdv8qWeE/xbJjzQfOdi7SKimhscxP9X uHHHK0tGqtfs7ES+uDzc6mc2TgRd4cMPfBCq5SglkYye0HFcedMVltW7edtLAM0= =unY2 -----END PGP SIGNATURE----- --=-UfSc81BXSdGaQTcHyRrG-- --===============1395297170== 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 --===============1395297170==--