From patchwork Fri Apr 17 13:36:02 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mika Kuoppala X-Patchwork-Id: 6231701 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id C5723BF4A6 for ; Fri, 17 Apr 2015 13:36:47 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A3AAD2037F for ; Fri, 17 Apr 2015 13:36:46 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 5C5D420266 for ; Fri, 17 Apr 2015 13:36:45 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 621D66EB61; Fri, 17 Apr 2015 06:36:44 -0700 (PDT) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTP id 596D96EB61 for ; Fri, 17 Apr 2015 06:36:43 -0700 (PDT) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga103.fm.intel.com with ESMTP; 17 Apr 2015 06:36:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,594,1422950400"; d="scan'208";a="696847424" Received: from gaia.fi.intel.com (HELO gaia) ([10.237.72.111]) by fmsmga001.fm.intel.com with ESMTP; 17 Apr 2015 06:36:41 -0700 Received: by gaia (Postfix, from userid 1000) id 5B2F1405C8; Fri, 17 Apr 2015 16:36:02 +0300 (EEST) From: Mika Kuoppala To: Daniel Vetter , Intel Graphics Development In-Reply-To: <1429025727-15380-5-git-send-email-daniel.vetter@ffwll.ch> References: <1429025727-15380-1-git-send-email-daniel.vetter@ffwll.ch> <1429025727-15380-5-git-send-email-daniel.vetter@ffwll.ch> User-Agent: Notmuch/0.19+103~g294bb6d (http://notmuchmail.org) Emacs/23.4.1 (i686-pc-linux-gnu) Date: Fri, 17 Apr 2015 16:36:02 +0300 Message-ID: <87vbgu27y5.fsf@gaia.fi.intel.com> MIME-Version: 1.0 Cc: Daniel Vetter , Daniel Vetter Subject: Re: [Intel-gfx] [PATCH 04/17] drm/i915: Unify aliasing ppgtt handling X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_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 Daniel Vetter writes: > With the dynamic pagetable alloc code aliasing ppgtt special-cases > where again mixed in all over the place with the low-level init code. > > Extract the va preallocation and clearing again into the common code > where aliasing ppgtt gets set up. > > Note that with this we don't set the size of the aliasing ppgtt to the > size of the parent ggtt address space. Which isn't required at all > since except for the ppgtt setup/cleanup code no one ever looks at > this. > > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 134 +++++++----------------------------- > 1 file changed, 24 insertions(+), 110 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index abb11f139d25..75787f1d2751 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -387,50 +387,6 @@ fail_bitmap: > return ERR_PTR(ret); > } > > -/** > - * alloc_pt_range() - Allocate a multiple page tables > - * @pd: The page directory which will have at least @count entries > - * available to point to the allocated page tables. > - * @pde: First page directory entry for which we are allocating. > - * @count: Number of pages to allocate. > - * @dev: DRM device. > - * > - * Allocates multiple page table pages and sets the appropriate entries in the > - * page table structure within the page directory. Function cleans up after > - * itself on any failures. > - * > - * Return: 0 if allocation succeeded. > - */ > -static int alloc_pt_range(struct i915_page_directory *pd, uint16_t pde, size_t count, > - struct drm_device *dev) > -{ > - int i, ret; > - > - /* 512 is the max page tables per page_directory on any platform. */ > - if (WARN_ON(pde + count > I915_PDES)) > - return -EINVAL; > - > - for (i = pde; i < pde + count; i++) { > - struct i915_page_table *pt = alloc_pt_single(dev); > - > - if (IS_ERR(pt)) { > - ret = PTR_ERR(pt); > - goto err_out; > - } > - WARN(pd->page_table[i], > - "Leaking page directory entry %d (%p)\n", > - i, pd->page_table[i]); > - pd->page_table[i] = pt; > - } > - > - return 0; > - > -err_out: > - while (i-- > pde) > - unmap_and_free_pt(pd->page_table[i], dev); > - return ret; > -} > - > static void unmap_and_free_pd(struct i915_page_directory *pd, > struct drm_device *dev) > { > @@ -971,7 +927,7 @@ err_out: > * space. > * > */ > -static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size) > +static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > { > ppgtt->scratch_pt = alloc_pt_single(ppgtt->base.dev); > if (IS_ERR(ppgtt->scratch_pt)) > @@ -985,8 +941,9 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size) > gen8_initialize_pd(&ppgtt->base, ppgtt->scratch_pd); > > ppgtt->base.start = 0; > - ppgtt->base.total = size; > + ppgtt->base.total = 1ULL << 32; This warns on 32bit builds due to overflow. Seems that I have already tripped on this when I reviewed 4gb default ppgtt size patch. We should do a follow-up patch with: > ppgtt->base.cleanup = gen8_ppgtt_cleanup; > + ppgtt->base.allocate_va_range = gen8_alloc_va_range; > ppgtt->base.insert_entries = gen8_ppgtt_insert_entries; > ppgtt->base.clear_range = gen8_ppgtt_clear_range; > ppgtt->base.unbind_vma = ppgtt_unbind_vma; > @@ -997,46 +954,6 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size) > return 0; > } > > -static int gen8_aliasing_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > -{ > - struct drm_device *dev = ppgtt->base.dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - uint64_t start = 0, size = dev_priv->gtt.base.total; > - int ret; > - > - ret = gen8_ppgtt_init_common(ppgtt, dev_priv->gtt.base.total); > - if (ret) > - return ret; > - > - /* Aliasing PPGTT has to always work and be mapped because of the way we > - * use RESTORE_INHIBIT in the context switch. This will be fixed > - * eventually. */ > - ret = gen8_alloc_va_range(&ppgtt->base, start, size); > - if (ret) { > - unmap_and_free_pd(ppgtt->scratch_pd, ppgtt->base.dev); > - unmap_and_free_pt(ppgtt->scratch_pt, ppgtt->base.dev); > - return ret; > - } > - > - ppgtt->base.allocate_va_range = NULL; > - ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true); > - > - return 0; > -} > - > -static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > -{ > - int ret; > - > - ret = gen8_ppgtt_init_common(ppgtt, (1ULL << 32)); > - if (ret) > - return ret; > - > - ppgtt->base.allocate_va_range = gen8_alloc_va_range; > - > - return 0; > -} > - > static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m) > { > struct i915_address_space *vm = &ppgtt->base; > @@ -1533,7 +1450,7 @@ static void gen6_scratch_va_range(struct i915_hw_ppgtt *ppgtt, > ppgtt->pd.page_table[pde] = ppgtt->scratch_pt; > } > > -static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing) > +static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt) > { > struct drm_device *dev = ppgtt->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -1556,18 +1473,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing) > if (ret) > return ret; > > - if (aliasing) { > - /* preallocate all pts */ > - ret = alloc_pt_range(&ppgtt->pd, 0, I915_PDES, > - ppgtt->base.dev); > - > - if (ret) { > - gen6_ppgtt_cleanup(&ppgtt->base); > - return ret; > - } > - } > - > - ppgtt->base.allocate_va_range = aliasing ? NULL : gen6_alloc_va_range; > + ppgtt->base.allocate_va_range = gen6_alloc_va_range; > ppgtt->base.clear_range = gen6_ppgtt_clear_range; > ppgtt->base.insert_entries = gen6_ppgtt_insert_entries; > ppgtt->base.unbind_vma = ppgtt_unbind_vma; > @@ -1583,10 +1489,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing) > ppgtt->pd_addr = (gen6_pte_t __iomem *)dev_priv->gtt.gsm + > ppgtt->pd.pd_offset / sizeof(gen6_pte_t); > > - if (aliasing) > - ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true); > - else > - gen6_scratch_va_range(ppgtt, 0, ppgtt->base.total); > + gen6_scratch_va_range(ppgtt, 0, ppgtt->base.total); > > gen6_write_page_range(dev_priv, &ppgtt->pd, 0, ppgtt->base.total); > > @@ -1600,8 +1503,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing) > return 0; > } > > -static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt, > - bool aliasing) > +static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > @@ -1609,9 +1511,7 @@ static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt, > ppgtt->base.scratch = dev_priv->gtt.base.scratch; > > if (INTEL_INFO(dev)->gen < 8) > - return gen6_ppgtt_init(ppgtt, aliasing); > - else if (aliasing) > - return gen8_aliasing_ppgtt_init(ppgtt); > + return gen6_ppgtt_init(ppgtt); > else > return gen8_ppgtt_init(ppgtt); > } > @@ -1620,7 +1520,7 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt) > struct drm_i915_private *dev_priv = dev->dev_private; > int ret = 0; > > - ret = __hw_ppgtt_init(dev, ppgtt, false); > + ret = __hw_ppgtt_init(dev, ppgtt); > if (ret == 0) { > kref_init(&ppgtt->ref); > drm_mm_init(&ppgtt->base.mm, ppgtt->base.start, > @@ -2255,13 +2155,27 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev, > if (!ppgtt) > return -ENOMEM; > > - ret = __hw_ppgtt_init(dev, ppgtt, true); > + ret = __hw_ppgtt_init(dev, ppgtt); > + if (ret) { > + ppgtt->base.cleanup(&ppgtt->base); > + kfree(ppgtt); > + return ret; > + } > + > + if (ppgtt->base.allocate_va_range) > + ret = ppgtt->base.allocate_va_range(&ppgtt->base, 0, > + ppgtt->base.total); > if (ret) { > ppgtt->base.cleanup(&ppgtt->base); > kfree(ppgtt); > return ret; > } > > + ppgtt->base.clear_range(&ppgtt->base, > + ppgtt->base.start, > + ppgtt->base.total, > + true); > + > dev_priv->mm.aliasing_ppgtt = ppgtt; > } > > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -229,7 +229,7 @@ struct i915_address_space { struct drm_device *dev; struct list_head global_link; unsigned long start; /* Start offset always 0 for dri2 */ - size_t total; /* size addr space maps (ex. 2GB for ggtt) */ + u64 size; /* size addr space maps (ex. 2GB for ggtt) */ -Mika