From patchwork Wed Feb 19 20:06:28 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ben Widawsky X-Patchwork-Id: 3682841 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id CB81C9F1EE for ; Wed, 19 Feb 2014 20:06:47 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id B5C4D201BA for ; Wed, 19 Feb 2014 20:06:46 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 725F1201F7 for ; Wed, 19 Feb 2014 20:06:45 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2F04FFAF1F; Wed, 19 Feb 2014 12:06:44 -0800 (PST) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 9CAF4FAF1F for ; Wed, 19 Feb 2014 12:06:41 -0800 (PST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 19 Feb 2014 12:02:17 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.97,507,1389772800"; d="scan'208";a="458331148" Received: from ironside.jf.intel.com ([10.7.197.77]) by orsmga001.jf.intel.com with ESMTP; 19 Feb 2014 12:06:29 -0800 From: Ben Widawsky To: Intel GFX Date: Wed, 19 Feb 2014 12:06:28 -0800 Message-Id: <1392840388-6531-1-git-send-email-benjamin.widawsky@intel.com> X-Mailer: git-send-email 1.9.0 In-Reply-To: <1392821989.19792.13.camel@intelbox> References: <1392821989.19792.13.camel@intelbox> Cc: Ben Widawsky , Ben Widawsky Subject: [Intel-gfx] [PATCH] [v3] drm/i915/bdw: Reorganize PPGTT init X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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 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. The patches also attempts to break up some of the steps into more logical reviewable chunks, particularly when we free. 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. v3: Fixes addressing Imre's comments from: <1392821989.19792.13.camel@intelbox> Don't do dynamic allocation for the page table DMA addresses. I can't remember why I did it in the first place. This addresses one of Imre's other issues. Fix error path leak of page tables. Signed-off-by: Ben Widawsky Reviewed-by: Imre Deak --- drivers/gpu/drm/i915/i915_gem_gtt.c | 114 ++++++++++++++++++++---------------- 1 file changed, 64 insertions(+), 50 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index e414d7e..03f586aa 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -333,6 +333,7 @@ static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt) static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt) { + struct pci_dev *hwdev = ppgtt->base.dev->pdev; int i, j; for (i = 0; i < ppgtt->num_pd_pages; i++) { @@ -341,18 +342,14 @@ static void gen8_ppgtt_unmap_pages(struct i915_hw_ppgtt *ppgtt) if (!ppgtt->pd_dma_addr[i]) continue; - 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); for (j = 0; j < GEN8_PDES_PER_PAGE; j++) { dma_addr_t addr = 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); } } } @@ -370,27 +367,27 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm) } /** - * 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 = 4GB legacy 32b address space. + * GEN8 legacy ppgtt programming is accomplished through a max 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 = 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 be 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 = -ENOMEM; const int max_pdp = DIV_ROUND_UP(size, 1 << 30); const int num_pt_pages = GEN8_PDES_PER_PAGE * max_pdp; + struct pci_dev *hwdev = ppgtt->base.dev->pdev; + int i, j, ret; if (size % (1<<30)) DRM_INFO("Pages will be wasted unless GTT size (%llu) is divisible by 1GB\n", size); - /* 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 = alloc_pages(GFP_KERNEL, get_order(max_pdp << PAGE_SHIFT)); if (!ppgtt->pd_pages) return -ENOMEM; @@ -405,52 +402,60 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size) ppgtt->num_pd_pages = 1 << get_order(max_pdp << PAGE_SHIFT); ppgtt->num_pt_pages = 1 << get_order(num_pt_pages << PAGE_SHIFT); ppgtt->num_pd_entries = max_pdp * GEN8_PDES_PER_PAGE; - ppgtt->enable = gen8_ppgtt_enable; - ppgtt->switch_mm = gen8_mm_switch; - ppgtt->base.clear_range = gen8_ppgtt_clear_range; - ppgtt->base.insert_entries = gen8_ppgtt_insert_entries; - ppgtt->base.cleanup = gen8_ppgtt_cleanup; - ppgtt->base.start = 0; - ppgtt->base.total = ppgtt->num_pt_pages * GEN8_PTES_PER_PAGE * PAGE_SIZE; - BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPS); + for (i = 0; i < max_pdp; i++) { + ppgtt->gen8_pt_dma_addr[i] = kcalloc(GEN8_PDES_PER_PAGE, + sizeof(dma_addr_t), + GFP_KERNEL); + if (!ppgtt->gen8_pt_dma_addr[i]) { + ret = -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 = 0; i < max_pdp; i++) { - dma_addr_t temp; - temp = 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; + dma_addr_t pd_addr, pt_addr; - ppgtt->pd_dma_addr[i] = temp; + /* And the page directory mappings */ + pd_addr = pci_map_page(hwdev, &ppgtt->pd_pages[i], 0, + PAGE_SIZE, PCI_DMA_BIDIRECTIONAL); + ret = pci_dma_mapping_error(ppgtt->base.dev->pdev, pd_addr); + if (ret) + goto bail; - ppgtt->gen8_pt_dma_addr[i] = kmalloc(sizeof(dma_addr_t) * GEN8_PDES_PER_PAGE, GFP_KERNEL); - if (!ppgtt->gen8_pt_dma_addr[i]) - goto err_out; + ppgtt->pd_dma_addr[i] = pd_addr; + /* Get the page table mappings per page directory */ for (j = 0; j < GEN8_PDES_PER_PAGE; j++) { struct page *p = &pt_pages[i * GEN8_PDES_PER_PAGE + j]; - temp = pci_map_page(ppgtt->base.dev->pdev, - p, 0, PAGE_SIZE, - PCI_DMA_BIDIRECTIONAL); - if (pci_dma_mapping_error(ppgtt->base.dev->pdev, temp)) - goto err_out; + pt_addr = pci_map_page(hwdev, p, 0, PAGE_SIZE, + PCI_DMA_BIDIRECTIONAL); + ret = pci_dma_mapping_error(hwdev, pt_addr); + if (ret) { + ppgtt->pd_dma_addr[i] = 0; + pci_unmap_page(hwdev, pd_addr, PAGE_SIZE, + PCI_DMA_BIDIRECTIONAL); + goto bail; + } - ppgtt->gen8_pt_dma_addr[i][j] = temp; + ppgtt->gen8_pt_dma_addr[i][j] = pt_addr; } } - /* 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 = 0; i < max_pdp; i++) { gen8_ppgtt_pde_t *pd_vaddr; pd_vaddr = kmap_atomic(&ppgtt->pd_pages[i]); @@ -462,6 +467,14 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size) kunmap_atomic(pd_vaddr); } + ppgtt->enable = gen8_ppgtt_enable; + ppgtt->switch_mm = gen8_mm_switch; + ppgtt->base.clear_range = gen8_ppgtt_clear_range; + ppgtt->base.insert_entries = gen8_ppgtt_insert_entries; + ppgtt->base.cleanup = gen8_ppgtt_cleanup; + ppgtt->base.start = 0; + 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, true); @@ -474,8 +487,9 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size) size % (1<<30)); return 0; -err_out: - ppgtt->base.cleanup(&ppgtt->base); +bail: + gen8_ppgtt_unmap_pages(ppgtt); + gen8_ppgtt_free(ppgtt); return ret; }