| Message ID | 1359160867-780-5-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 26 Jan 2013, Ben Widawsky <ben@bwidawsk.net> wrote: > Aside from being a nice prep for some work I'm doing in PPGTT, this also > leaves us with a more accurate size in our gtt_total_entries member. > Since we can't actually use the PPGTT reserved part of the GGTT. > > This will help some of the existing assertions find issues which would > overwrite the PPGTT PDEs. > > Another benefit is for platforms which don't use the entire 2GB GTT, > this will save GTT space. For example if a platform has 1 1GB GTT, this > patch should save 1MB of GTT space (512MB has an even greater savings). > I don't have a platform to easily test this however. > > If/when we ever move to real PPGTT, I think allocating the PDEs as > objects will make the most sense. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 2 ++ > drivers/gpu/drm/i915/i915_drv.h | 3 ++- > drivers/gpu/drm/i915/i915_gem_gtt.c | 35 ++++++++++++++++++++++++++--------- > 3 files changed, 30 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 384f193..4545357 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1605,6 +1605,8 @@ static int i915_ppgtt_info(struct seq_file *m, void *data) > > seq_printf(m, "aliasing PPGTT:\n"); > seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd_offset); > + seq_printf(m, "mapped size: %lldM\n", > + ppgtt->mapped_size>>20); > } > seq_printf(m, "ECOCHK: 0x%08x\n", I915_READ(GAM_ECOCHK)); > mutex_unlock(&dev->struct_mutex); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 1af8e73..60ba4ab 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -388,10 +388,11 @@ struct i915_gtt { > struct page *scratch_page; > }; > > -#define I915_PPGTT_PD_ENTRIES 512 > +#define GEN6_MAX_PD_ENTRIES 512 > #define I915_PPGTT_PT_ENTRIES 1024 > struct i915_hw_ppgtt { > struct drm_device *dev; > + uint64_t mapped_size; > unsigned num_pd_entries; > struct page **pt_pages; > uint32_t pd_offset; > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index f79f427..d9af815 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -108,7 +108,7 @@ static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt, > } > } > > -static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt) > +static int gen6_init_ppgtt(struct i915_hw_ppgtt *ppgtt, uint64_t size) > { > struct drm_device *dev = ppgtt->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -116,15 +116,22 @@ static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt) > int i; > int ret = -ENOMEM; > > + if (dev_priv->mm.aliasing_ppgtt) > + return -EBUSY; > + > + /* Each PDE maps 4MB (1k PAGE_SIZE pages)*/ > + ppgtt->mapped_size = round_up(size, 1<<22); > + ppgtt->num_pd_entries = ppgtt->mapped_size >> 22; > + BUG_ON(ppgtt->num_pd_entries > GEN6_MAX_PD_ENTRIES); > + > /* ppgtt PDEs reside in the global gtt pagetable, which has 512*1024 > * entries. For aliasing ppgtt support we just steal them at the end for > * now. */ > - first_pd_entry_in_global_pt = dev_priv->mm.gtt->gtt_total_entries - I915_PPGTT_PD_ENTRIES; > + first_pd_entry_in_global_pt = dev_priv->mm.gtt->gtt_total_entries - > + ppgtt->num_pd_entries; > > - ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES; > ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries, > GFP_KERNEL); > - > if (!ppgtt->pt_pages) > return -ENOMEM; > > @@ -156,7 +163,10 @@ static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt) > i915_ppgtt_clear_range(ppgtt, 0, > ppgtt->num_pd_entries*I915_PPGTT_PT_ENTRIES); > > - ppgtt->pd_offset = (first_pd_entry_in_global_pt)*sizeof(gtt_pte_t); > + ppgtt->pd_offset = first_pd_entry_in_global_pt * sizeof(gtt_pte_t); > + > + DRM_INFO("Allocated %d PDEs for PPGTT\n", ppgtt->num_pd_entries); > + DRM_DEBUG_DRIVER("First PDE: %d\n", first_pd_entry_in_global_pt); > > return 0; > > @@ -192,7 +202,7 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) > ppgtt->dev = dev; > ppgtt->scratch_page_dma_addr = dev_priv->gtt.scratch_page_dma; > > - ret = gen6_init_aliasing_ppgtt(ppgtt); > + ret = gen6_init_ppgtt(ppgtt, dev_priv->gtt.total); > if (ret) > kfree(ppgtt); > else > @@ -625,20 +635,27 @@ void i915_gem_init_global_gtt(struct drm_device *dev) > mappable_size = dev_priv->gtt.mappable_end; > > if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) { > + /* Each PDE represents 1k PAGE_SIZE objects */ > + const int reserve_pd_space = gtt_size >> 10; Maybe (likely) I'm being dense, but care to explain this shift a bit? Should it match ppgtt->num_pd_entries * PAGE_SIZE after i915_gem_init_aliasing_ppgtt()? BR, Jani. > int ret; > + > + BUG_ON(!reserve_pd_space); > + > /* PPGTT pdes are stolen from global gtt ptes, so shrink the > * aperture accordingly when using aliasing ppgtt. */ > - gtt_size -= I915_PPGTT_PD_ENTRIES*PAGE_SIZE; > + gtt_size -= reserve_pd_space; > > i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); > > ret = i915_gem_init_aliasing_ppgtt(dev); > - if (!ret) > + if (!ret) { > + dev_priv->mm.gtt->gtt_total_entries = gtt_size >> PAGE_SHIFT; > return; > + } > > DRM_ERROR("Aliased PPGTT setup failed %d\n", ret); > drm_mm_takedown(&dev_priv->mm.gtt_space); > - gtt_size += I915_PPGTT_PD_ENTRIES*PAGE_SIZE; > + gtt_size += reserve_pd_space; > } > i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); > } > -- > 1.8.1.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jan 28, 2013 at 02:19:15PM +0200, Jani Nikula wrote: > On Sat, 26 Jan 2013, Ben Widawsky <ben@bwidawsk.net> wrote: > > Aside from being a nice prep for some work I'm doing in PPGTT, this also > > leaves us with a more accurate size in our gtt_total_entries member. > > Since we can't actually use the PPGTT reserved part of the GGTT. > > > > This will help some of the existing assertions find issues which would > > overwrite the PPGTT PDEs. > > > > Another benefit is for platforms which don't use the entire 2GB GTT, > > this will save GTT space. For example if a platform has 1 1GB GTT, this > > patch should save 1MB of GTT space (512MB has an even greater savings). > > I don't have a platform to easily test this however. > > > > If/when we ever move to real PPGTT, I think allocating the PDEs as > > objects will make the most sense. > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 2 ++ > > drivers/gpu/drm/i915/i915_drv.h | 3 ++- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 35 ++++++++++++++++++++++++++--------- > > 3 files changed, 30 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > > index 384f193..4545357 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -1605,6 +1605,8 @@ static int i915_ppgtt_info(struct seq_file *m, void *data) > > > > seq_printf(m, "aliasing PPGTT:\n"); > > seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd_offset); > > + seq_printf(m, "mapped size: %lldM\n", > > + ppgtt->mapped_size>>20); > > } > > seq_printf(m, "ECOCHK: 0x%08x\n", I915_READ(GAM_ECOCHK)); > > mutex_unlock(&dev->struct_mutex); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 1af8e73..60ba4ab 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -388,10 +388,11 @@ struct i915_gtt { > > struct page *scratch_page; > > }; > > > > -#define I915_PPGTT_PD_ENTRIES 512 > > +#define GEN6_MAX_PD_ENTRIES 512 > > #define I915_PPGTT_PT_ENTRIES 1024 > > struct i915_hw_ppgtt { > > struct drm_device *dev; > > + uint64_t mapped_size; > > unsigned num_pd_entries; > > struct page **pt_pages; > > uint32_t pd_offset; > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > > index f79f427..d9af815 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > @@ -108,7 +108,7 @@ static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt, > > } > > } > > > > -static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt) > > +static int gen6_init_ppgtt(struct i915_hw_ppgtt *ppgtt, uint64_t size) > > { > > struct drm_device *dev = ppgtt->dev; > > struct drm_i915_private *dev_priv = dev->dev_private; > > @@ -116,15 +116,22 @@ static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt) > > int i; > > int ret = -ENOMEM; > > > > + if (dev_priv->mm.aliasing_ppgtt) > > + return -EBUSY; > > + > > + /* Each PDE maps 4MB (1k PAGE_SIZE pages)*/ > > + ppgtt->mapped_size = round_up(size, 1<<22); > > + ppgtt->num_pd_entries = ppgtt->mapped_size >> 22; > > + BUG_ON(ppgtt->num_pd_entries > GEN6_MAX_PD_ENTRIES); > > + > > /* ppgtt PDEs reside in the global gtt pagetable, which has 512*1024 > > * entries. For aliasing ppgtt support we just steal them at the end for > > * now. */ > > - first_pd_entry_in_global_pt = dev_priv->mm.gtt->gtt_total_entries - I915_PPGTT_PD_ENTRIES; > > + first_pd_entry_in_global_pt = dev_priv->mm.gtt->gtt_total_entries - > > + ppgtt->num_pd_entries; > > > > - ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES; > > ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries, > > GFP_KERNEL); > > - > > if (!ppgtt->pt_pages) > > return -ENOMEM; > > > > @@ -156,7 +163,10 @@ static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt) > > i915_ppgtt_clear_range(ppgtt, 0, > > ppgtt->num_pd_entries*I915_PPGTT_PT_ENTRIES); > > > > - ppgtt->pd_offset = (first_pd_entry_in_global_pt)*sizeof(gtt_pte_t); > > + ppgtt->pd_offset = first_pd_entry_in_global_pt * sizeof(gtt_pte_t); > > + > > + DRM_INFO("Allocated %d PDEs for PPGTT\n", ppgtt->num_pd_entries); > > + DRM_DEBUG_DRIVER("First PDE: %d\n", first_pd_entry_in_global_pt); > > > > return 0; > > > > @@ -192,7 +202,7 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) > > ppgtt->dev = dev; > > ppgtt->scratch_page_dma_addr = dev_priv->gtt.scratch_page_dma; > > > > - ret = gen6_init_aliasing_ppgtt(ppgtt); > > + ret = gen6_init_ppgtt(ppgtt, dev_priv->gtt.total); > > if (ret) > > kfree(ppgtt); > > else > > @@ -625,20 +635,27 @@ void i915_gem_init_global_gtt(struct drm_device *dev) > > mappable_size = dev_priv->gtt.mappable_end; > > > > if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) { > > + /* Each PDE represents 1k PAGE_SIZE objects */ > > + const int reserve_pd_space = gtt_size >> 10; > > Maybe (likely) I'm being dense, but care to explain this shift a bit? > Should it match ppgtt->num_pd_entries * PAGE_SIZE after > i915_gem_init_aliasing_ppgtt()? > > BR, > Jani. > yeah, it's horribly confusing until you actually need to write code yourself. I actually think Daniel should have reviewed this patch, but I guess he is lazy. In this context, gtt_size refers to the amount of GPU mappable memory. (I actually had another patch which renamed all of this stuff, but whatever). We want to be able to map the entire space the gtt maps with the ppgtt. Each page directory holds 1024 PTEs, which each map a 4k pfn. Therefore, each PDEs is effecitvely mapping (1 << (10 + 12)). Since the PDs are stored in the GGTT (suck), we're trying to carve out the necessary memory from the GGTT. The PDEs are special, see the DCLV stuff. The number of PDEs required is: gtt_size >> 22 But as I said above, we really need to calculate how much PD space we need. Each PD has PAGE_SIZE of space required gtt_size >> 22 >> PAGE_SIZE = gtt_size >> 10 In hindsight, maybe doing that calculation in the code would have been better. Anyway, to answer the question, I think yes, it should match ppgtt->num_pd_entries * PAGE_SIZE > > int ret; > > + > > + BUG_ON(!reserve_pd_space); > > + > > /* PPGTT pdes are stolen from global gtt ptes, so shrink the > > * aperture accordingly when using aliasing ppgtt. */ > > - gtt_size -= I915_PPGTT_PD_ENTRIES*PAGE_SIZE; > > + gtt_size -= reserve_pd_space; > > > > i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); > > > > ret = i915_gem_init_aliasing_ppgtt(dev); > > - if (!ret) > > + if (!ret) { > > + dev_priv->mm.gtt->gtt_total_entries = gtt_size >> PAGE_SHIFT; > > return; > > + } > > > > DRM_ERROR("Aliased PPGTT setup failed %d\n", ret); > > drm_mm_takedown(&dev_priv->mm.gtt_space); > > - gtt_size += I915_PPGTT_PD_ENTRIES*PAGE_SIZE; > > + gtt_size += reserve_pd_space; > > } > > i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); > > } > > -- > > 1.8.1.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jan 28, 2013 at 7:26 PM, Ben Widawsky <ben@bwidawsk.net> wrote: > But as I said above, we really need to calculate how much PD space we > need. Each PD has PAGE_SIZE of space required > gtt_size >> 22 >> PAGE_SIZE = gtt_size >> 10 gtt_size >> 22 * PAGE_SIZE > In hindsight, maybe doing that calculation in the code would have been > better. > > Anyway, to answer the question, I think yes, it should match > ppgtt->num_pd_entries * PAGE_SIZE That looks much clearer imo and spells out what happens much better (we still a pte = full page in the address space for each ppgtt pd). Can you please apply this bikeshed to the patch? -Daniel
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 384f193..4545357 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1605,6 +1605,8 @@ static int i915_ppgtt_info(struct seq_file *m, void *data) seq_printf(m, "aliasing PPGTT:\n"); seq_printf(m, "pd gtt offset: 0x%08x\n", ppgtt->pd_offset); + seq_printf(m, "mapped size: %lldM\n", + ppgtt->mapped_size>>20); } seq_printf(m, "ECOCHK: 0x%08x\n", I915_READ(GAM_ECOCHK)); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 1af8e73..60ba4ab 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -388,10 +388,11 @@ struct i915_gtt { struct page *scratch_page; }; -#define I915_PPGTT_PD_ENTRIES 512 +#define GEN6_MAX_PD_ENTRIES 512 #define I915_PPGTT_PT_ENTRIES 1024 struct i915_hw_ppgtt { struct drm_device *dev; + uint64_t mapped_size; unsigned num_pd_entries; struct page **pt_pages; uint32_t pd_offset; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index f79f427..d9af815 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -108,7 +108,7 @@ static void i915_ppgtt_clear_range(struct i915_hw_ppgtt *ppgtt, } } -static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt) +static int gen6_init_ppgtt(struct i915_hw_ppgtt *ppgtt, uint64_t size) { struct drm_device *dev = ppgtt->dev; struct drm_i915_private *dev_priv = dev->dev_private; @@ -116,15 +116,22 @@ static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt) int i; int ret = -ENOMEM; + if (dev_priv->mm.aliasing_ppgtt) + return -EBUSY; + + /* Each PDE maps 4MB (1k PAGE_SIZE pages)*/ + ppgtt->mapped_size = round_up(size, 1<<22); + ppgtt->num_pd_entries = ppgtt->mapped_size >> 22; + BUG_ON(ppgtt->num_pd_entries > GEN6_MAX_PD_ENTRIES); + /* ppgtt PDEs reside in the global gtt pagetable, which has 512*1024 * entries. For aliasing ppgtt support we just steal them at the end for * now. */ - first_pd_entry_in_global_pt = dev_priv->mm.gtt->gtt_total_entries - I915_PPGTT_PD_ENTRIES; + first_pd_entry_in_global_pt = dev_priv->mm.gtt->gtt_total_entries - + ppgtt->num_pd_entries; - ppgtt->num_pd_entries = I915_PPGTT_PD_ENTRIES; ppgtt->pt_pages = kzalloc(sizeof(struct page *)*ppgtt->num_pd_entries, GFP_KERNEL); - if (!ppgtt->pt_pages) return -ENOMEM; @@ -156,7 +163,10 @@ static int gen6_init_aliasing_ppgtt(struct i915_hw_ppgtt *ppgtt) i915_ppgtt_clear_range(ppgtt, 0, ppgtt->num_pd_entries*I915_PPGTT_PT_ENTRIES); - ppgtt->pd_offset = (first_pd_entry_in_global_pt)*sizeof(gtt_pte_t); + ppgtt->pd_offset = first_pd_entry_in_global_pt * sizeof(gtt_pte_t); + + DRM_INFO("Allocated %d PDEs for PPGTT\n", ppgtt->num_pd_entries); + DRM_DEBUG_DRIVER("First PDE: %d\n", first_pd_entry_in_global_pt); return 0; @@ -192,7 +202,7 @@ static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev) ppgtt->dev = dev; ppgtt->scratch_page_dma_addr = dev_priv->gtt.scratch_page_dma; - ret = gen6_init_aliasing_ppgtt(ppgtt); + ret = gen6_init_ppgtt(ppgtt, dev_priv->gtt.total); if (ret) kfree(ppgtt); else @@ -625,20 +635,27 @@ void i915_gem_init_global_gtt(struct drm_device *dev) mappable_size = dev_priv->gtt.mappable_end; if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) { + /* Each PDE represents 1k PAGE_SIZE objects */ + const int reserve_pd_space = gtt_size >> 10; int ret; + + BUG_ON(!reserve_pd_space); + /* PPGTT pdes are stolen from global gtt ptes, so shrink the * aperture accordingly when using aliasing ppgtt. */ - gtt_size -= I915_PPGTT_PD_ENTRIES*PAGE_SIZE; + gtt_size -= reserve_pd_space; i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); ret = i915_gem_init_aliasing_ppgtt(dev); - if (!ret) + if (!ret) { + dev_priv->mm.gtt->gtt_total_entries = gtt_size >> PAGE_SHIFT; return; + } DRM_ERROR("Aliased PPGTT setup failed %d\n", ret); drm_mm_takedown(&dev_priv->mm.gtt_space); - gtt_size += I915_PPGTT_PD_ENTRIES*PAGE_SIZE; + gtt_size += reserve_pd_space; } i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); }
Aside from being a nice prep for some work I'm doing in PPGTT, this also leaves us with a more accurate size in our gtt_total_entries member. Since we can't actually use the PPGTT reserved part of the GGTT. This will help some of the existing assertions find issues which would overwrite the PPGTT PDEs. Another benefit is for platforms which don't use the entire 2GB GTT, this will save GTT space. For example if a platform has 1 1GB GTT, this patch should save 1MB of GTT space (512MB has an even greater savings). I don't have a platform to easily test this however. If/when we ever move to real PPGTT, I think allocating the PDEs as objects will make the most sense. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_debugfs.c | 2 ++ drivers/gpu/drm/i915/i915_drv.h | 3 ++- drivers/gpu/drm/i915/i915_gem_gtt.c | 35 ++++++++++++++++++++++++++--------- 3 files changed, 30 insertions(+), 10 deletions(-)