[PATCH 1/2] drm/i915: trivial: kill-agp collateral cleanups

[PATCH 1/2] drm/i915: trivial: kill-agp collateral cleanups

[Ben Widawsky]

- i915_gem_init_aliasing_ppgtt should now be static
- move i915_gem_init_ppgtt declaration to the right place

I expected sparse to catch the first one, but somehow it didn't.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h     | 3 +--
 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d2b93a4..9a57e66 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1522,7 +1522,6 @@ int __must_check i915_gem_init(struct drm_device *dev);
 int __must_check i915_gem_init_hw(struct drm_device *dev);
 void i915_gem_l3_remap(struct drm_device *dev);
 void i915_gem_init_swizzling(struct drm_device *dev);
-void i915_gem_init_ppgtt(struct drm_device *dev);
 void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
 int __must_check i915_gem_idle(struct drm_device *dev);
@@ -1576,7 +1575,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file);
 
 /* i915_gem_gtt.c */
-int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
+void i915_gem_init_ppgtt(struct drm_device *dev);
 void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev);
 void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
 			    struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index eac2cec..597efbd 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,
 	}
 }
 
-int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
+static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct i915_hw_ppgtt *ppgtt;
-- 
1.8.0.2

[PATCH 2/2] drm/i915: Reclaim GGTT space for failed ppgtt

[Ben Widawsky]

When the ppgtt init fails, we may as well reuse the space that were were
reserving for the ppgtt PDEs.

This also fixes an extraneous mutex_unlock.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 597efbd..94d21aa 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -525,12 +525,20 @@ static void i915_gtt_color_adjust(struct drm_mm_node *node,
 			*end -= 4096;
 	}
 }
-
 void i915_gem_setup_global_gtt(struct drm_device *dev,
 			       unsigned long start,
 			       unsigned long mappable_end,
 			       unsigned long end)
 {
+	/* Let GEM Manage all of the aperture.
+	 *
+	 * However, leave one page at the end still bound to the scratch page.
+	 * There are a number of places where the hardware apparently prefetches
+	 * past the end of the object, and we've seen multiple hangs with the
+	 * GPU head pointer stuck in a batchbuffer bound at the last page of the
+	 * aperture.  One page should be enough to keep any prefetching inside
+	 * of the aperture.
+	 */
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct drm_mm_node *entry;
 	struct drm_i915_gem_object *obj;
@@ -593,12 +601,12 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long gtt_size, mappable_size;
-	int ret;
 
 	gtt_size = dev_priv->mm.gtt->gtt_total_entries << PAGE_SHIFT;
 	mappable_size = dev_priv->mm.gtt->gtt_mappable_entries << PAGE_SHIFT;
 
 	if (intel_enable_ppgtt(dev) && HAS_ALIASING_PPGTT(dev)) {
+		int ret;
 		/* 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;
@@ -606,23 +614,14 @@ void i915_gem_init_global_gtt(struct drm_device *dev)
 		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
 
 		ret = i915_gem_init_aliasing_ppgtt(dev);
-		if (ret) {
-			mutex_unlock(&dev->struct_mutex);
+		if (!ret)
 			return;
-		}
-	} else {
-		/* Let GEM Manage all of the aperture.
-		 *
-		 * However, leave one page at the end still bound to the scratch
-		 * page.  There are a number of places where the hardware
-		 * apparently prefetches past the end of the object, and we've
-		 * seen multiple hangs with the GPU head pointer stuck in a
-		 * batchbuffer bound at the last page of the aperture.  One page
-		 * should be enough to keep any prefetching inside of the
-		 * aperture.
-		 */
-		i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
+
+		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;
 	}
+	i915_gem_setup_global_gtt(dev, 0, mappable_size, gtt_size);
 }
 
 static int setup_scratch_page(struct drm_device *dev)
-- 
1.8.0.2

Re: [PATCH 2/2] drm/i915: Reclaim GGTT space for failed ppgtt

[Chris Wilson]

On Thu, 27 Dec 2012 18:27:22 -0800, Ben Widawsky <ben@bwidawsk.net> wrote:
> When the ppgtt init fails, we may as well reuse the space that were were
> reserving for the ppgtt PDEs.
> 
> This also fixes an extraneous mutex_unlock.

Hear, hear.

Could I ask for the int ret; to be killed off entirely?

[both] Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: trivial: kill-agp collateral cleanups

[Jani Nikula]

On Fri, 28 Dec 2012, Ben Widawsky <ben@bwidawsk.net> wrote:
> - i915_gem_init_aliasing_ppgtt should now be static
> - move i915_gem_init_ppgtt declaration to the right place
>
> I expected sparse to catch the first one, but somehow it didn't.

FWIW, it only complains if you don't declare the function first. But you
still had the declaration sitting in i915_drv.h. I'm sure we'd have
plenty of cleaning up to do if we did have a tool to detect these...

BR,
Jani.

>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     | 3 +--
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d2b93a4..9a57e66 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1522,7 +1522,6 @@ int __must_check i915_gem_init(struct drm_device *dev);
>  int __must_check i915_gem_init_hw(struct drm_device *dev);
>  void i915_gem_l3_remap(struct drm_device *dev);
>  void i915_gem_init_swizzling(struct drm_device *dev);
> -void i915_gem_init_ppgtt(struct drm_device *dev);
>  void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
>  int __must_check i915_gpu_idle(struct drm_device *dev);
>  int __must_check i915_gem_idle(struct drm_device *dev);
> @@ -1576,7 +1575,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
>  				   struct drm_file *file);
>  
>  /* i915_gem_gtt.c */
> -int __must_check i915_gem_init_aliasing_ppgtt(struct drm_device *dev);
> +void i915_gem_init_ppgtt(struct drm_device *dev);
>  void i915_gem_cleanup_aliasing_ppgtt(struct drm_device *dev);
>  void i915_ppgtt_bind_object(struct i915_hw_ppgtt *ppgtt,
>  			    struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index eac2cec..597efbd 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,
>  	}
>  }
>  
> -int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
> +static int i915_gem_init_aliasing_ppgtt(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct i915_hw_ppgtt *ppgtt;
> -- 
> 1.8.0.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx