On Thu, 27 Jun 2013 16:30:07 -0700
Ben Widawsky <ben at 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 at 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 at virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center
