| Message ID | 1372375867-1003-7-git-send-email-ben@bwidawsk.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 27 Jun 2013 16:30:07 -0700 Ben Widawsky <ben@bwidawsk.net> wrote: > The PPGTT PDEs serve as the guard page (as long as they remain at the > top) so we don't need yet another guard page. Note that there is a > potential issue if the aliasing PPGTT (and later, the default context) > relinquish this part of the GGTT. We should be able to assert that won't > happen however. > > While there, add some comments for the setup_global_gtt function which > started getting complicated. > > The reason I've opted not to leave out the guard_page argument is that > in order to support dri1, we call the setup function, and I didn't like > to have to clear the guard page in more than 1 location. > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net> > --- > drivers/gpu/drm/i915/i915_drv.h | 3 ++- > drivers/gpu/drm/i915/i915_gem.c | 4 ++-- > drivers/gpu/drm/i915/i915_gem_gtt.c | 27 ++++++++++++++++++++++----- > 3 files changed, 26 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index b709712..c677d6c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1852,7 +1852,8 @@ void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj); > void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj); > void i915_gem_init_global_gtt(struct drm_device *dev); > void i915_gem_setup_global_gtt(struct drm_device *dev, unsigned long start, > - unsigned long mappable_end, unsigned long end); > + unsigned long mappable_end, unsigned long end, > + unsigned long guard_size); > int i915_gem_gtt_init(struct drm_device *dev); > static inline void i915_gem_chipset_flush(struct drm_device *dev) > { > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 6806bb9..629e047 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -158,8 +158,8 @@ i915_gem_init_ioctl(struct drm_device *dev, void *data, > > mutex_lock(&dev->struct_mutex); > i915_gem_setup_global_gtt(dev, args->gtt_start, args->gtt_end, > - args->gtt_end); > - dev_priv->gtt.mappable_end = args->gtt_end; > + args->gtt_end, PAGE_SIZE); > + dev_priv->gtt.mappable_end = args->gtt_end - PAGE_SIZE; > mutex_unlock(&dev->struct_mutex); > > return 0; > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 0fce8d0..fb30d65 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -613,10 +613,23 @@ static void i915_gtt_color_adjust(struct drm_mm_node *node, > *end -= 4096; > } > } > + > +/** > + * i915_gem_setup_global_gtt() setup an allocator for the global GTT with the > + * given parameters and initialize all PTEs to point to the scratch page. > + * > + * @dev > + * @start - first offset of managed GGTT space > + * @mappable_end - Last offset of the aperture mapped region > + * @end - Last offset that can be accessed by the allocator > + * @guard_size - Size to initialize to scratch after end. (Currently only used > + * for prefetching case) > + */ > void i915_gem_setup_global_gtt(struct drm_device *dev, > unsigned long start, > unsigned long mappable_end, > - unsigned long end) > + unsigned long end, > + unsigned long guard_size) > { > /* Let GEM Manage all of the aperture. > * > @@ -634,8 +647,11 @@ void i915_gem_setup_global_gtt(struct drm_device *dev, > > BUG_ON(mappable_end > end); > > + if (WARN_ON(guard_size & ~PAGE_MASK)) > + guard_size = round_up(guard_size, PAGE_SIZE); > + > /* Subtract the guard page ... */ > - drm_mm_init(&dev_priv->mm.gtt_space, start, end - start - PAGE_SIZE); > + drm_mm_init(&dev_priv->mm.gtt_space, start, end - start - guard_size); > if (!HAS_LLC(dev)) > dev_priv->mm.gtt_space.color_adjust = i915_gtt_color_adjust; > > @@ -665,7 +681,8 @@ void i915_gem_setup_global_gtt(struct drm_device *dev, > } > > /* And finally clear the reserved guard page */ > - dev_priv->gtt.gtt_clear_range(dev, end / PAGE_SIZE - 1, 1); > + dev_priv->gtt.gtt_clear_range(dev, (end - guard_size) / PAGE_SIZE, > + guard_size / PAGE_SIZE); > } > > static bool > @@ -700,7 +717,7 @@ void i915_gem_init_global_gtt(struct drm_device *dev) > gtt_size -= GEN6_PPGTT_PD_ENTRIES * PAGE_SIZE; > } > > - i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); > + i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size, 0); > > ret = i915_gem_init_aliasing_ppgtt(dev); > if (!ret) > @@ -710,7 +727,7 @@ void i915_gem_init_global_gtt(struct drm_device *dev) > drm_mm_takedown(&dev_priv->mm.gtt_space); > gtt_size += GEN6_PPGTT_PD_ENTRIES * PAGE_SIZE; > } > - i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); > + i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size, PAGE_SIZE); > } > > static int setup_scratch_page(struct drm_device *dev) Just a nitpick that can be changed with a follow on patch if others agree: I'd rather see the WARN_ON made a BUG_ON when checking that the guard_size is a multiple of PAGE_SIZE (which, incidentally, is the wrong value to use, but that's also for another cleanup). Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b709712..c677d6c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1852,7 +1852,8 @@ void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj); void i915_gem_gtt_finish_object(struct drm_i915_gem_object *obj); void i915_gem_init_global_gtt(struct drm_device *dev); void i915_gem_setup_global_gtt(struct drm_device *dev, unsigned long start, - unsigned long mappable_end, unsigned long end); + unsigned long mappable_end, unsigned long end, + unsigned long guard_size); int i915_gem_gtt_init(struct drm_device *dev); static inline void i915_gem_chipset_flush(struct drm_device *dev) { diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 6806bb9..629e047 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -158,8 +158,8 @@ i915_gem_init_ioctl(struct drm_device *dev, void *data, mutex_lock(&dev->struct_mutex); i915_gem_setup_global_gtt(dev, args->gtt_start, args->gtt_end, - args->gtt_end); - dev_priv->gtt.mappable_end = args->gtt_end; + args->gtt_end, PAGE_SIZE); + dev_priv->gtt.mappable_end = args->gtt_end - PAGE_SIZE; mutex_unlock(&dev->struct_mutex); return 0; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 0fce8d0..fb30d65 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -613,10 +613,23 @@ static void i915_gtt_color_adjust(struct drm_mm_node *node, *end -= 4096; } } + +/** + * i915_gem_setup_global_gtt() setup an allocator for the global GTT with the + * given parameters and initialize all PTEs to point to the scratch page. + * + * @dev + * @start - first offset of managed GGTT space + * @mappable_end - Last offset of the aperture mapped region + * @end - Last offset that can be accessed by the allocator + * @guard_size - Size to initialize to scratch after end. (Currently only used + * for prefetching case) + */ void i915_gem_setup_global_gtt(struct drm_device *dev, unsigned long start, unsigned long mappable_end, - unsigned long end) + unsigned long end, + unsigned long guard_size) { /* Let GEM Manage all of the aperture. * @@ -634,8 +647,11 @@ void i915_gem_setup_global_gtt(struct drm_device *dev, BUG_ON(mappable_end > end); + if (WARN_ON(guard_size & ~PAGE_MASK)) + guard_size = round_up(guard_size, PAGE_SIZE); + /* Subtract the guard page ... */ - drm_mm_init(&dev_priv->mm.gtt_space, start, end - start - PAGE_SIZE); + drm_mm_init(&dev_priv->mm.gtt_space, start, end - start - guard_size); if (!HAS_LLC(dev)) dev_priv->mm.gtt_space.color_adjust = i915_gtt_color_adjust; @@ -665,7 +681,8 @@ void i915_gem_setup_global_gtt(struct drm_device *dev, } /* And finally clear the reserved guard page */ - dev_priv->gtt.gtt_clear_range(dev, end / PAGE_SIZE - 1, 1); + dev_priv->gtt.gtt_clear_range(dev, (end - guard_size) / PAGE_SIZE, + guard_size / PAGE_SIZE); } static bool @@ -700,7 +717,7 @@ void i915_gem_init_global_gtt(struct drm_device *dev) gtt_size -= GEN6_PPGTT_PD_ENTRIES * PAGE_SIZE; } - i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); + i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size, 0); ret = i915_gem_init_aliasing_ppgtt(dev); if (!ret) @@ -710,7 +727,7 @@ void i915_gem_init_global_gtt(struct drm_device *dev) drm_mm_takedown(&dev_priv->mm.gtt_space); gtt_size += GEN6_PPGTT_PD_ENTRIES * PAGE_SIZE; } - i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size); + i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size, PAGE_SIZE); } static int setup_scratch_page(struct drm_device *dev)
The PPGTT PDEs serve as the guard page (as long as they remain at the top) so we don't need yet another guard page. Note that there is a potential issue if the aliasing PPGTT (and later, the default context) relinquish this part of the GGTT. We should be able to assert that won't happen however. While there, add some comments for the setup_global_gtt function which started getting complicated. The reason I've opted not to leave out the guard_page argument is that in order to support dri1, we call the setup function, and I didn't like to have to clear the guard page in more than 1 location. Signed-off-by: Ben Widawsky <ben@bwidawsk.net> --- drivers/gpu/drm/i915/i915_drv.h | 3 ++- drivers/gpu/drm/i915/i915_gem.c | 4 ++-- drivers/gpu/drm/i915/i915_gem_gtt.c | 27 ++++++++++++++++++++++----- 3 files changed, 26 insertions(+), 8 deletions(-)