[PATCH 1/4] drm/i915: Demote unknown param to DRM_DEBUG

[PATCH 1/4] drm/i915: Demote unknown param to DRM_DEBUG

[Ben Widawsky]

It's not terribly interesting to know that a parameter doesn't exist,
and it can get in the way of interesting messages, especially with the
staggered VECS merging as we've done.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 3cd2b60..57d2ba9 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -999,8 +999,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		value = 1;
 		break;
 	default:
-		DRM_DEBUG_DRIVER("Unknown parameter %d\n",
-				 param->param);
+		DRM_DEBUG("Unknown parameter %d\n", param->param);
 		return -EINVAL;
 	}
 
-- 
1.8.3

[PATCH 2/4] drm/i915: Unbind the fb

[Ben Widawsky]

This is the only place I could find where we don't already unbind the
object before freeing it. This change will allow us to have a nice
assertion on free about whether or not an object still has pinned pages.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_fb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 6b7c3ca..cd92889 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -209,6 +209,7 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 	drm_framebuffer_unregister_private(&ifb->base);
 	drm_framebuffer_cleanup(&ifb->base);
 	if (ifb->obj) {
+		i915_gem_object_unbind(to_intel_bo(&ifb->obj->base));
 		drm_gem_object_unreference_unlocked(&ifb->obj->base);
 		ifb->obj = NULL;
 	}
-- 
1.8.3

[PATCH 3/4] drm/i915: unpin pages at unbind

[Ben Widawsky]

If we properly keep track of the pages_pin_count, then when we later add
multiple address spaces, the put_pages doesn't need any special checks
to be able to perform it's job.

CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e5b6a92..ad0c69d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2508,6 +2508,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
 		obj->has_aliasing_ppgtt_mapping = 0;
 	}
 	i915_gem_gtt_finish_object(obj);
+	i915_gem_object_unpin_pages(obj);
 
 	list_del(&obj->mm_list);
 	list_move_tail(&obj->gtt_list, &dev_priv->mm.unbound_list);
@@ -3057,7 +3058,6 @@ search_free:
 
 	obj->map_and_fenceable = mappable && fenceable;
 
-	i915_gem_object_unpin_pages(obj);
 	trace_i915_gem_object_bind(obj, map_and_fenceable);
 	i915_gem_verify_gtt(dev);
 	return 0;
@@ -3857,7 +3857,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 		dev_priv->mm.interruptible = was_interruptible;
 	}
 
-	obj->pages_pin_count = 0;
+	if (WARN_ON(obj->pages_pin_count))
+		obj->pages_pin_count = 0;
 	i915_gem_object_put_pages(obj);
 	i915_gem_object_free_mmap_offset(obj);
 	i915_gem_object_release_stolen(obj);
-- 
1.8.3

[PATCH 4/4] drm/i915: Rename the gtt_list to global_list

[Ben Widawsky]

Since it will be used for the global bound/unbound list with full PPGTT,
this helps clarify things for upcoming code rework.

Recommended-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c    | 11 ++++++-----
 drivers/gpu/drm/i915/i915_drv.h        |  2 +-
 drivers/gpu/drm/i915/i915_gem.c        | 23 ++++++++++++-----------
 drivers/gpu/drm/i915/i915_gem_gtt.c    |  4 ++--
 drivers/gpu/drm/i915/i915_gem_stolen.c |  2 +-
 drivers/gpu/drm/i915/i915_irq.c        |  6 +++---
 6 files changed, 25 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2eb572a..750e13e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -215,7 +215,7 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
 		   dev_priv->mm.object_memory);
 
 	size = count = mappable_size = mappable_count = 0;
-	count_objects(&dev_priv->mm.bound_list, gtt_list);
+	count_objects(&dev_priv->mm.bound_list, global_list);
 	seq_printf(m, "%u [%u] objects, %zu [%zu] bytes in gtt\n",
 		   count, mappable_count, size, mappable_size);
 
@@ -230,7 +230,7 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
 		   count, mappable_count, size, mappable_size);
 
 	size = count = purgeable_size = purgeable_count = 0;
-	list_for_each_entry(obj, &dev_priv->mm.unbound_list, gtt_list) {
+	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list) {
 		size += obj->base.size, ++count;
 		if (obj->madv == I915_MADV_DONTNEED)
 			purgeable_size += obj->base.size, ++purgeable_count;
@@ -238,7 +238,7 @@ static int i915_gem_object_info(struct seq_file *m, void* data)
 	seq_printf(m, "%u unbound objects, %zu bytes\n", count, size);
 
 	size = count = mappable_size = mappable_count = 0;
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, gtt_list) {
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		if (obj->fault_mappable) {
 			size += obj->gtt_space->size;
 			++count;
@@ -283,7 +283,7 @@ static int i915_gem_gtt_info(struct seq_file *m, void* data)
 		return ret;
 
 	total_obj_size = total_gtt_size = count = 0;
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, gtt_list) {
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		if (list == PINNED_LIST && obj->pin_count == 0)
 			continue;
 
@@ -1924,7 +1924,8 @@ i915_drop_caches_set(void *data, u64 val)
 	}
 
 	if (val & DROP_UNBOUND) {
-		list_for_each_entry_safe(obj, next, &dev_priv->mm.unbound_list, gtt_list)
+		list_for_each_entry_safe(obj, next, &dev_priv->mm.unbound_list,
+					 global_list)
 			if (obj->pages_pin_count == 0) {
 				ret = i915_gem_object_put_pages(obj);
 				if (ret)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b6f2968..8153873 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1154,7 +1154,7 @@ struct drm_i915_gem_object {
 	struct drm_mm_node *gtt_space;
 	/** Stolen memory for this object, instead of being backed by shmem. */
 	struct drm_mm_node *stolen;
-	struct list_head gtt_list;
+	struct list_head global_list;
 
 	/** This object's place on the active/inactive lists */
 	struct list_head ring_list;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ad0c69d..22499e7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -176,7 +176,7 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
 
 	pinned = 0;
 	mutex_lock(&dev->struct_mutex);
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, gtt_list)
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
 		if (obj->pin_count)
 			pinned += obj->gtt_space->size;
 	mutex_unlock(&dev->struct_mutex);
@@ -1676,7 +1676,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 	/* ->put_pages might need to allocate memory for the bit17 swizzle
 	 * array, hence protect them from being reaped by removing them from gtt
 	 * lists early. */
-	list_del(&obj->gtt_list);
+	list_del(&obj->global_list);
 
 	ops->put_pages(obj);
 	obj->pages = NULL;
@@ -1696,7 +1696,7 @@ __i915_gem_shrink(struct drm_i915_private *dev_priv, long target,
 
 	list_for_each_entry_safe(obj, next,
 				 &dev_priv->mm.unbound_list,
-				 gtt_list) {
+				 global_list) {
 		if ((i915_gem_object_is_purgeable(obj) || !purgeable_only) &&
 		    i915_gem_object_put_pages(obj) == 0) {
 			count += obj->base.size >> PAGE_SHIFT;
@@ -1733,7 +1733,8 @@ i915_gem_shrink_all(struct drm_i915_private *dev_priv)
 
 	i915_gem_evict_everything(dev_priv->dev);
 
-	list_for_each_entry_safe(obj, next, &dev_priv->mm.unbound_list, gtt_list)
+	list_for_each_entry_safe(obj, next, &dev_priv->mm.unbound_list,
+				 global_list)
 		i915_gem_object_put_pages(obj);
 }
 
@@ -1858,7 +1859,7 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 	if (ret)
 		return ret;
 
-	list_add_tail(&obj->gtt_list, &dev_priv->mm.unbound_list);
+	list_add_tail(&obj->global_list, &dev_priv->mm.unbound_list);
 	return 0;
 }
 
@@ -2511,7 +2512,7 @@ i915_gem_object_unbind(struct drm_i915_gem_object *obj)
 	i915_gem_object_unpin_pages(obj);
 
 	list_del(&obj->mm_list);
-	list_move_tail(&obj->gtt_list, &dev_priv->mm.unbound_list);
+	list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
 	/* Avoid an unnecessary call to unbind on rebind. */
 	obj->map_and_fenceable = true;
 
@@ -2919,7 +2920,7 @@ static void i915_gem_verify_gtt(struct drm_device *dev)
 	struct drm_i915_gem_object *obj;
 	int err = 0;
 
-	list_for_each_entry(obj, &dev_priv->mm.gtt_list, gtt_list) {
+	list_for_each_entry(obj, &dev_priv->mm.gtt_list, global_list) {
 		if (obj->gtt_space == NULL) {
 			printk(KERN_ERR "object found on GTT list with no space reserved\n");
 			err++;
@@ -3043,7 +3044,7 @@ search_free:
 		return ret;
 	}
 
-	list_move_tail(&obj->gtt_list, &dev_priv->mm.bound_list);
+	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
 	list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
 
 	obj->gtt_space = node;
@@ -3757,7 +3758,7 @@ void i915_gem_object_init(struct drm_i915_gem_object *obj,
 			  const struct drm_i915_gem_object_ops *ops)
 {
 	INIT_LIST_HEAD(&obj->mm_list);
-	INIT_LIST_HEAD(&obj->gtt_list);
+	INIT_LIST_HEAD(&obj->global_list);
 	INIT_LIST_HEAD(&obj->ring_list);
 	INIT_LIST_HEAD(&obj->exec_list);
 
@@ -4499,10 +4500,10 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)
 	}
 
 	cnt = 0;
-	list_for_each_entry(obj, &dev_priv->mm.unbound_list, gtt_list)
+	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
 		if (obj->pages_pin_count == 0)
 			cnt += obj->base.size >> PAGE_SHIFT;
-	list_for_each_entry(obj, &dev_priv->mm.inactive_list, gtt_list)
+	list_for_each_entry(obj, &dev_priv->mm.inactive_list, global_list)
 		if (obj->pin_count == 0 && obj->pages_pin_count == 0)
 			cnt += obj->base.size >> PAGE_SHIFT;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ddad13f..5101ab6 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -439,7 +439,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 	dev_priv->gtt.gtt_clear_range(dev, dev_priv->gtt.start / PAGE_SIZE,
 				      dev_priv->gtt.total / PAGE_SIZE);
 
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, gtt_list) {
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		i915_gem_clflush_object(obj);
 		i915_gem_gtt_bind_object(obj, obj->cache_level);
 	}
@@ -631,7 +631,7 @@ void i915_gem_setup_global_gtt(struct drm_device *dev,
 		dev_priv->mm.gtt_space.color_adjust = i915_gtt_color_adjust;
 
 	/* Mark any preallocated objects as occupied */
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, gtt_list) {
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		DRM_DEBUG_KMS("reserving preallocated space: %x + %zx\n",
 			      obj->gtt_offset, obj->base.size);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 89cbfab..689e291 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -383,7 +383,7 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	obj->gtt_offset = gtt_offset;
 	obj->has_global_gtt_mapping = 1;
 
-	list_add_tail(&obj->gtt_list, &dev_priv->mm.bound_list);
+	list_add_tail(&obj->global_list, &dev_priv->mm.bound_list);
 	list_add_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
 
 	return obj;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index da5c9ab..83ec610 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1647,7 +1647,7 @@ static u32 capture_pinned_bo(struct drm_i915_error_buffer *err,
 	struct drm_i915_gem_object *obj;
 	int i = 0;
 
-	list_for_each_entry(obj, head, gtt_list) {
+	list_for_each_entry(obj, head, global_list) {
 		if (obj->pin_count == 0)
 			continue;
 
@@ -1789,7 +1789,7 @@ static void i915_gem_record_active_context(struct intel_ring_buffer *ring,
 	if (ring->id != RCS || !error->ccid)
 		return;
 
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, gtt_list) {
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		if ((error->ccid & PAGE_MASK) == obj->gtt_offset) {
 			ering->ctx = i915_error_object_create_sized(dev_priv,
 								    obj, 1);
@@ -1926,7 +1926,7 @@ static void i915_capture_error_state(struct drm_device *dev)
 	list_for_each_entry(obj, &dev_priv->mm.active_list, mm_list)
 		i++;
 	error->active_bo_count = i;
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, gtt_list)
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
 		if (obj->pin_count)
 			i++;
 	error->pinned_bo_count = i - error->active_bo_count;
-- 
1.8.3

Re: [PATCH 1/4] drm/i915: Demote unknown param to DRM_DEBUG

[Chris Wilson]

On Fri, May 31, 2013 at 11:28:45AM -0700, Ben Widawsky wrote:
> It's not terribly interesting to know that a parameter doesn't exist,
> and it can get in the way of interesting messages, especially with the
> staggered VECS merging as we've done.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

The idiom we have is DRM_DEBUG() for informative messages after user
error, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Rename the gtt_list to global_list

[Chris Wilson]

On Fri, May 31, 2013 at 11:28:48AM -0700, Ben Widawsky wrote:
> Since it will be used for the global bound/unbound list with full PPGTT,
> this helps clarify things for upcoming code rework.
> 
> Recommended-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/4] [v2] drm/i915: Unbind the fb

[Ben Widawsky]

This is the only place I could find where we don't already unbind the
object before freeing it. This change will allow us to have a nice
assertion on free about whether or not an object still has pinned pages.

v2: Just use the i915 object instead of casting

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_fb.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c
index 6b7c3ca..bc81de6 100644
--- a/drivers/gpu/drm/i915/intel_fb.c
+++ b/drivers/gpu/drm/i915/intel_fb.c
@@ -209,6 +209,7 @@ static void intel_fbdev_destroy(struct drm_device *dev,
 	drm_framebuffer_unregister_private(&ifb->base);
 	drm_framebuffer_cleanup(&ifb->base);
 	if (ifb->obj) {
+		i915_gem_object_unbind(ifb->obj);
 		drm_gem_object_unreference_unlocked(&ifb->obj->base);
 		ifb->obj = NULL;
 	}
-- 
1.8.3

Re: [PATCH 3/4] drm/i915: unpin pages at unbind

[Chris Wilson]

On Fri, May 31, 2013 at 11:28:47AM -0700, Ben Widawsky wrote:
> If we properly keep track of the pages_pin_count, then when we later add
> multiple address spaces, the put_pages doesn't need any special checks
> to be able to perform it's job.
> 
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Like. Since we don't expose the pages_pin_count to the user (unlike the
regular pin_count & pin_ioctl), then we should never have to perform a
cleanup in free_object(). Though as you have seen, we obviously need to
let this patch soak for a bit in case we missed some.

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] [v2] drm/i915: Unbind the fb

[Chris Wilson]

On Fri, May 31, 2013 at 11:45:29AM -0700, Ben Widawsky wrote:
> This is the only place I could find where we don't already unbind the
> object before freeing it. This change will allow us to have a nice
> assertion on free about whether or not an object still has pinned pages.
> 
> v2: Just use the i915 object instead of casting

The patch is flawed in that we try to do the unbind without mutex
protection. But it should already be unbound in free_object() anyway -
and doing so here is unlikely to fix the WARN_ON (presuming that is
what you are chasing).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/4] [v2] drm/i915: Unbind the fb

[Ben Widawsky]

On Fri, May 31, 2013 at 08:39:32PM +0100, Chris Wilson wrote:
> On Fri, May 31, 2013 at 11:45:29AM -0700, Ben Widawsky wrote:
> > This is the only place I could find where we don't already unbind the
> > object before freeing it. This change will allow us to have a nice
> > assertion on free about whether or not an object still has pinned pages.
> > 
> > v2: Just use the i915 object instead of casting
> 
> The patch is flawed in that we try to do the unbind without mutex
> protection. But it should already be unbound in free_object() anyway -
> and doing so here is unlikely to fix the WARN_ON (presuming that is
> what you are chasing).
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

Agreed. This patch is junk. I made the mistake of working on it when I
should have been in bed.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 1/2] drm/i915: Make stolen use pin pages

[Ben Widawsky]

This should have been in reply to patch 2, sorry...

On Fri, May 31, 2013 at 02:46:19PM -0700, Ben Widawsky wrote:
> This makes it easier to catch leaks.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 89cbfab..bc593e3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -279,7 +279,7 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
>  		goto cleanup;
>  
>  	obj->has_dma_mapping = true;
> -	obj->pages_pin_count = 1;
> +	i915_gem_object_pin_pages(obj);
>  	obj->stolen = stolen;
>  
>  	obj->base.write_domain = I915_GEM_DOMAIN_GTT;
> -- 
> 1.8.3
> 

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 1/2] drm/i915: Make stolen use pin pages

[Ben Widawsky]

This makes it easier to catch leaks.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 89cbfab..bc593e3 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -279,7 +279,7 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
 		goto cleanup;
 
 	obj->has_dma_mapping = true;
-	obj->pages_pin_count = 1;
+	i915_gem_object_pin_pages(obj);
 	obj->stolen = stolen;
 
 	obj->base.write_domain = I915_GEM_DOMAIN_GTT;
-- 
1.8.3

[PATCH 2/2] drm/i915: Unpin stolen pages

[Ben Widawsky]

The way the stolen handling works is we take a pin on the backing pages,
but we never actually get a reference to the bo. On freeing objects
allocated with stolen memory, the final unref will end up freeing the
object with pinned pages count left. To enable an assertion to catch
bugs in this code path, this patch cleans up that remaining pin.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e5b6a92..a18a485 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3857,6 +3857,11 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 		dev_priv->mm.interruptible = was_interruptible;
 	}
 
+	/* Stolen objects don't hold a ref, but do hold pin count. Fix that up
+	 * before progressing. */
+	if (obj->stolen)
+		i915_gem_object_unpin_pages(obj);
+
 	obj->pages_pin_count = 0;
 	i915_gem_object_put_pages(obj);
 	i915_gem_object_free_mmap_offset(obj);
-- 
1.8.3

Re: [PATCH 2/2] drm/i915: Unpin stolen pages

[Chris Wilson]

On Fri, May 31, 2013 at 02:46:20PM -0700, Ben Widawsky wrote:
> The way the stolen handling works is we take a pin on the backing pages,
> but we never actually get a reference to the bo. On freeing objects
> allocated with stolen memory, the final unref will end up freeing the
> object with pinned pages count left. To enable an assertion to catch
> bugs in this code path, this patch cleans up that remaining pin.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

That neatly explains the WARN. Not too happy about accumulating lots of
backing storage specific processing into free_object, but that can be
fixed up later (there is an obj->ops->release() pending).

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Unpin stolen pages

[Daniel Vetter]

On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, May 31, 2013 at 02:46:20PM -0700, Ben Widawsky wrote:
>> The way the stolen handling works is we take a pin on the backing pages,
>> but we never actually get a reference to the bo. On freeing objects
>> allocated with stolen memory, the final unref will end up freeing the
>> object with pinned pages count left. To enable an assertion to catch
>> bugs in this code path, this patch cleans up that remaining pin.
>>
>> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>
> That neatly explains the WARN. Not too happy about accumulating lots of
> backing storage specific processing into free_object, but that can be
> fixed up later (there is an obj->ops->release() pending).

I'm more irked with the semantic overloading of object pinning. Might
be cleaner to otherwise mark stolen obejcts as not shrinkable instead
of pinning them for their entire lifetime. But we can bikeshed that
later on ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: Unpin stolen pages

[Chris Wilson]

On Sat, Jun 01, 2013 at 11:17:10AM +0200, Daniel Vetter wrote:
> On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > That neatly explains the WARN. Not too happy about accumulating lots of
> > backing storage specific processing into free_object, but that can be
> > fixed up later (there is an obj->ops->release() pending).
> 
> I'm more irked with the semantic overloading of object pinning. Might
> be cleaner to otherwise mark stolen obejcts as not shrinkable instead
> of pinning them for their entire lifetime. But we can bikeshed that
> later on ;-)

Some merit to that argument, but it still feels correct to say that the
stolen pages are pinned for their lifetime. Given obj->ops->release(),
it does actually become simpler to not mess around with pin_count. So
later it is.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Unpin stolen pages

[Daniel Vetter]

On Sat, Jun 1, 2013 at 1:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sat, Jun 01, 2013 at 11:17:10AM +0200, Daniel Vetter wrote:
>> On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > That neatly explains the WARN. Not too happy about accumulating lots of
>> > backing storage specific processing into free_object, but that can be
>> > fixed up later (there is an obj->ops->release() pending).
>>
>> I'm more irked with the semantic overloading of object pinning. Might
>> be cleaner to otherwise mark stolen obejcts as not shrinkable instead
>> of pinning them for their entire lifetime. But we can bikeshed that
>> later on ;-)
>
> Some merit to that argument, but it still feels correct to say that the
> stolen pages are pinned for their lifetime. Given obj->ops->release(),
> it does actually become simpler to not mess around with pin_count. So
> later it is.

I was more unhappy that pin_count has different meanings, until I've
noticed that we've fixed that up already with the introduction of
->pages_pin_count. Shouldn't stolen mem just hold a reference on that
one? After all unbinding from the gtt is ok with stolen memory, but
dropping the backing storage in the shrinker won't work. Not that we
currently use stolen for anything else than permanently pinned bos.

Reading through the code we have a bit an unclear relationship between
pages_pin_count and the (gtt) pin_count, so this approach would need
some clarification (especially around the shrinker).

And it would still leave us with a special case in free, so your
->release callback still has uses ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: Unpin stolen pages

[Chris Wilson]

On Sat, Jun 01, 2013 at 03:13:04PM +0200, Daniel Vetter wrote:
> On Sat, Jun 1, 2013 at 1:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Sat, Jun 01, 2013 at 11:17:10AM +0200, Daniel Vetter wrote:
> >> On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> > That neatly explains the WARN. Not too happy about accumulating lots of
> >> > backing storage specific processing into free_object, but that can be
> >> > fixed up later (there is an obj->ops->release() pending).
> >>
> >> I'm more irked with the semantic overloading of object pinning. Might
> >> be cleaner to otherwise mark stolen obejcts as not shrinkable instead
> >> of pinning them for their entire lifetime. But we can bikeshed that
> >> later on ;-)
> >
> > Some merit to that argument, but it still feels correct to say that the
> > stolen pages are pinned for their lifetime. Given obj->ops->release(),
> > it does actually become simpler to not mess around with pin_count. So
> > later it is.
> 
> I was more unhappy that pin_count has different meanings, until I've
> noticed that we've fixed that up already with the introduction of
> ->pages_pin_count. Shouldn't stolen mem just hold a reference on that
> one? After all unbinding from the gtt is ok with stolen memory, but
> dropping the backing storage in the shrinker won't work. Not that we
> currently use stolen for anything else than permanently pinned bos.

As mentioned on irc, stolen does use the pages_pin_count for its
purposes. The purpose of this patch is purely to allow sanity checking
the pages_pin_count with a WARN_ON during free which seems sensible but
not strictly required.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Unpin stolen pages

[Daniel Vetter]

On Mon, Jun 03, 2013 at 09:36:48AM +0100, Chris Wilson wrote:
> On Sat, Jun 01, 2013 at 03:13:04PM +0200, Daniel Vetter wrote:
> > On Sat, Jun 1, 2013 at 1:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Sat, Jun 01, 2013 at 11:17:10AM +0200, Daniel Vetter wrote:
> > >> On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > >> > That neatly explains the WARN. Not too happy about accumulating lots of
> > >> > backing storage specific processing into free_object, but that can be
> > >> > fixed up later (there is an obj->ops->release() pending).
> > >>
> > >> I'm more irked with the semantic overloading of object pinning. Might
> > >> be cleaner to otherwise mark stolen obejcts as not shrinkable instead
> > >> of pinning them for their entire lifetime. But we can bikeshed that
> > >> later on ;-)
> > >
> > > Some merit to that argument, but it still feels correct to say that the
> > > stolen pages are pinned for their lifetime. Given obj->ops->release(),
> > > it does actually become simpler to not mess around with pin_count. So
> > > later it is.
> > 
> > I was more unhappy that pin_count has different meanings, until I've
> > noticed that we've fixed that up already with the introduction of
> > ->pages_pin_count. Shouldn't stolen mem just hold a reference on that
> > one? After all unbinding from the gtt is ok with stolen memory, but
> > dropping the backing storage in the shrinker won't work. Not that we
> > currently use stolen for anything else than permanently pinned bos.
> 
> As mentioned on irc, stolen does use the pages_pin_count for its
> purposes. The purpose of this patch is purely to allow sanity checking
> the pages_pin_count with a WARN_ON during free which seems sensible but
> not strictly required.

Yeah, silly me didn't read the code before sending a knee-jerk mail ;-)

Series merged (hopefully in the right order and all, please check), thanks
for the patches and review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: Unpin stolen pages

[Ben Widawsky]

On Mon, Jun 03, 2013 at 10:52:12AM +0200, Daniel Vetter wrote:
> On Mon, Jun 03, 2013 at 09:36:48AM +0100, Chris Wilson wrote:
> > On Sat, Jun 01, 2013 at 03:13:04PM +0200, Daniel Vetter wrote:
> > > On Sat, Jun 1, 2013 at 1:34 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > > On Sat, Jun 01, 2013 at 11:17:10AM +0200, Daniel Vetter wrote:
> > > >> On Sat, Jun 1, 2013 at 1:51 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > >> > That neatly explains the WARN. Not too happy about accumulating lots of
> > > >> > backing storage specific processing into free_object, but that can be
> > > >> > fixed up later (there is an obj->ops->release() pending).
> > > >>
> > > >> I'm more irked with the semantic overloading of object pinning. Might
> > > >> be cleaner to otherwise mark stolen obejcts as not shrinkable instead
> > > >> of pinning them for their entire lifetime. But we can bikeshed that
> > > >> later on ;-)
> > > >
> > > > Some merit to that argument, but it still feels correct to say that the
> > > > stolen pages are pinned for their lifetime. Given obj->ops->release(),
> > > > it does actually become simpler to not mess around with pin_count. So
> > > > later it is.
> > > 
> > > I was more unhappy that pin_count has different meanings, until I've
> > > noticed that we've fixed that up already with the introduction of
> > > ->pages_pin_count. Shouldn't stolen mem just hold a reference on that
> > > one? After all unbinding from the gtt is ok with stolen memory, but
> > > dropping the backing storage in the shrinker won't work. Not that we
> > > currently use stolen for anything else than permanently pinned bos.
> > 
> > As mentioned on irc, stolen does use the pages_pin_count for its
> > purposes. The purpose of this patch is purely to allow sanity checking
> > the pages_pin_count with a WARN_ON during free which seems sensible but
> > not strictly required.
> 
> Yeah, silly me didn't read the code before sending a knee-jerk mail ;-)
> 
> Series merged (hopefully in the right order and all, please check), thanks
> for the patches and review.
> -Daniel
>
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Order looks good. Thanks.

-- 
Ben Widawsky, Intel Open Source Technology Center