Re: [PATCH 06/66] drm/i915: Conditionally use guard page based on PPGTT

[Jesse Barnes <jbarnes@virtuousgeek.org>]

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>

-- 
Jesse Barnes, Intel Open Source Technology Center