[PATCH] drm/i915: Make pinned object handling more specific

[PATCH] drm/i915: Make pinned object handling more specific

[Joonas Lahtinen]

Get rid of the over-generic i915_gem_obj_is_pinned and replace it
with i915_gem_obj_ggtt_is_pinned or more specific VMA handling.

Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Joonas Lahtinen
---
 drivers/gpu/drm/i915/i915_debugfs.c     |  8 +++++--
 drivers/gpu/drm/i915/i915_drv.h         |  9 +++++++-
 drivers/gpu/drm/i915/i915_gem.c         | 38 +++++++++++++++++++++------------
 drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c   |  4 +---
 drivers/gpu/drm/i915/intel_lrc.c        |  8 +++----
 6 files changed, 44 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 3e17210..5442a18 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -519,6 +519,7 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data)
 	uintptr_t list = (uintptr_t) node->info_ent->data;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
+	struct i915_vma *vma;
 	size_t total_obj_size, total_gtt_size;
 	int count, ret;
 
@@ -528,14 +529,17 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data)
 
 	total_obj_size = total_gtt_size = count = 0;
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
-		if (list == PINNED_LIST && !i915_gem_obj_is_pinned(obj))
+		if (list == PINNED_LIST && !i915_gem_obj_ggtt_is_pinned(obj))
 			continue;
 
 		seq_puts(m, "   ");
 		describe_obj(m, obj);
 		seq_putc(m, '\n');
 		total_obj_size += obj->base.size;
-		total_gtt_size += i915_gem_obj_ggtt_size(obj);
+		list_for_each_entry(vma, &obj->vma_list, vma_link)
+			if (i915_is_ggtt(vma->vm) &&
+			    (list != PINNED_LIST || vma->pin_count > 0))
+				total_gtt_size += vma->node.size;
 		count++;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 60aa962..be7bcc6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2960,7 +2960,14 @@ i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
 {
 	return i915_gem_obj_to_ggtt_view(obj, &i915_ggtt_view_normal);
 }
-bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj);
+
+bool
+i915_gem_obj_ggtt_is_pinned_view(struct drm_i915_gem_object *obj,
+				 const struct i915_ggtt_view *view);
+static inline bool
+i915_gem_obj_ggtt_is_pinned(struct drm_i915_gem_object *obj) {
+	return i915_gem_obj_ggtt_is_pinned_view(obj, &i915_ggtt_view_normal);
+}
 
 /* Some GGTT VM helpers */
 #define i915_obj_to_ggtt(obj) \
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index be35f04..75218c2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -150,13 +150,15 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_get_aperture *args = data;
 	struct drm_i915_gem_object *obj;
+	struct i915_vma *vma;
 	size_t pinned;
 
 	pinned = 0;
 	mutex_lock(&dev->struct_mutex);
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
-		if (i915_gem_obj_is_pinned(obj))
-			pinned += i915_gem_obj_ggtt_size(obj);
+		list_for_each_entry(vma, &obj->vma_list, vma_link)
+			if (vma->pin_count > 0)
+				pinned += vma->node.size;
 	mutex_unlock(&dev->struct_mutex);
 
 	args->aper_size = dev_priv->gtt.base.total;
@@ -3967,12 +3969,12 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 	if (obj->cache_level == cache_level)
 		return 0;
 
-	if (i915_gem_obj_is_pinned(obj)) {
-		DRM_DEBUG("can not change the cache level of pinned objects\n");
-		return -EBUSY;
-	}
-
 	list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
+		if (vma->pin_count > 0) {
+			DRM_DEBUG("can not change the cache level of pinned objects\n");
+			return -EBUSY;
+		}
+
 		if (!i915_gem_valid_gtt_space(vma, cache_level)) {
 			ret = i915_vma_unbind(vma);
 			if (ret)
@@ -4506,6 +4508,7 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_madvise *args = data;
 	struct drm_i915_gem_object *obj;
+	struct i915_vma *vma;
 	int ret;
 
 	switch (args->madv) {
@@ -4526,9 +4529,11 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
 		goto unlock;
 	}
 
-	if (i915_gem_obj_is_pinned(obj)) {
-		ret = -EINVAL;
-		goto out;
+	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		if (vma->pin_count > 0) {
+			ret = -EINVAL;
+			goto out;
+		}
 	}
 
 	if (obj->pages &&
@@ -5382,13 +5387,18 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
 	return 0;
 }
 
-bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
+bool
+i915_gem_obj_ggtt_is_pinned_view(struct drm_i915_gem_object *obj,
+				 const struct i915_ggtt_view *view)
 {
 	struct i915_vma *vma;
-	list_for_each_entry(vma, &obj->vma_list, vma_link)
-		if (vma->pin_count > 0)
+	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		if (!i915_is_ggtt(vma->vm))
+			continue;
+		if (i915_ggtt_view_equal(&vma->ggtt_view, view) &&
+		    vma->pin_count > 0)
 			return true;
-
+	}
 	return false;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 133afcf..ed4a16b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -634,7 +634,7 @@ static int do_switch(struct intel_engine_cs *ring,
 
 	if (from != NULL && ring == &dev_priv->ring[RCS]) {
 		BUG_ON(from->legacy_hw_ctx.rcs_state == NULL);
-		BUG_ON(!i915_gem_obj_is_pinned(from->legacy_hw_ctx.rcs_state));
+		BUG_ON(!i915_gem_obj_ggtt_is_pinned(from->legacy_hw_ctx.rcs_state));
 	}
 
 	if (should_skip_switch(ring, from, to))
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 6f42569..cc52f9c 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -697,9 +697,7 @@ static void capture_bo(struct drm_i915_error_buffer *err,
 	err->read_domains = obj->base.read_domains;
 	err->write_domain = obj->base.write_domain;
 	err->fence_reg = obj->fence_reg;
-	err->pinned = 0;
-	if (i915_gem_obj_is_pinned(obj))
-		err->pinned = 1;
+	err->pinned = vma->pin_count > 0;
 	err->tiling = obj->tiling_mode;
 	err->dirty = obj->dirty;
 	err->purgeable = obj->madv != I915_MADV_WILLNEED;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 9f5485d..dc595a0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -369,8 +369,8 @@ static void execlists_submit_contexts(struct intel_engine_cs *ring,
 	struct intel_ringbuffer *ringbuf1 = NULL;
 
 	BUG_ON(!ctx_obj0);
-	WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0));
-	WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj));
+	WARN_ON(!i915_gem_obj_ggtt_is_pinned(ctx_obj0));
+	WARN_ON(!i915_gem_obj_ggtt_is_pinned(ringbuf0->obj));
 
 	execlists_update_context(ctx_obj0, ringbuf0->obj, to0->ppgtt, tail0);
 
@@ -378,8 +378,8 @@ static void execlists_submit_contexts(struct intel_engine_cs *ring,
 		ringbuf1 = to1->engine[ring->id].ringbuf;
 		ctx_obj1 = to1->engine[ring->id].state;
 		BUG_ON(!ctx_obj1);
-		WARN_ON(!i915_gem_obj_is_pinned(ctx_obj1));
-		WARN_ON(!i915_gem_obj_is_pinned(ringbuf1->obj));
+		WARN_ON(!i915_gem_obj_ggtt_is_pinned(ctx_obj1));
+		WARN_ON(!i915_gem_obj_ggtt_is_pinned(ringbuf1->obj));
 
 		execlists_update_context(ctx_obj1, ringbuf1->obj, to1->ppgtt, tail1);
 	}
-- 
1.9.3



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make pinned object handling more specific

[Chris Wilson]

On Thu, Jun 04, 2015 at 11:49:06AM +0300, Joonas Lahtinen wrote:
> Get rid of the over-generic i915_gem_obj_is_pinned and replace it
> with i915_gem_obj_ggtt_is_pinned or more specific VMA handling.
> 
> Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Joonas Lahtinen

I take it you didn't read my patch to accomplish the same.

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     |  8 +++++--
>  drivers/gpu/drm/i915/i915_drv.h         |  9 +++++++-
>  drivers/gpu/drm/i915/i915_gem.c         | 38 +++++++++++++++++++++------------
>  drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
>  drivers/gpu/drm/i915/i915_gpu_error.c   |  4 +---
>  drivers/gpu/drm/i915/intel_lrc.c        |  8 +++----
>  6 files changed, 44 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3e17210..5442a18 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -519,6 +519,7 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data)
>  	uintptr_t list = (uintptr_t) node->info_ent->data;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj;
> +	struct i915_vma *vma;
>  	size_t total_obj_size, total_gtt_size;
>  	int count, ret;
>  
> @@ -528,14 +529,17 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data)
>  
>  	total_obj_size = total_gtt_size = count = 0;
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {

For instance here, we only care about the ggtt vma so lookup it first and
then use it directly.

> -		if (list == PINNED_LIST && !i915_gem_obj_is_pinned(obj))
> +		if (list == PINNED_LIST && !i915_gem_obj_ggtt_is_pinned(obj))
>  			continue;
>  
>  		seq_puts(m, "   ");
>  		describe_obj(m, obj);
>  		seq_putc(m, '\n');
>  		total_obj_size += obj->base.size;
> -		total_gtt_size += i915_gem_obj_ggtt_size(obj);
> +		list_for_each_entry(vma, &obj->vma_list, vma_link)
> +			if (i915_is_ggtt(vma->vm) &&
> +			    (list != PINNED_LIST || vma->pin_count > 0))
> +				total_gtt_size += vma->node.size;
>  		count++;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 60aa962..be7bcc6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2960,7 +2960,14 @@ i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
>  {
>  	return i915_gem_obj_to_ggtt_view(obj, &i915_ggtt_view_normal);
>  }
> -bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj);
> +
> +bool
> +i915_gem_obj_ggtt_is_pinned_view(struct drm_i915_gem_object *obj,
> +				 const struct i915_ggtt_view *view);
> +static inline bool
> +i915_gem_obj_ggtt_is_pinned(struct drm_i915_gem_object *obj) {
> +	return i915_gem_obj_ggtt_is_pinned_view(obj, &i915_ggtt_view_normal);
> +}
>  
>  /* Some GGTT VM helpers */
>  #define i915_obj_to_ggtt(obj) \
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index be35f04..75218c2 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -150,13 +150,15 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_get_aperture *args = data;
>  	struct drm_i915_gem_object *obj;
> +	struct i915_vma *vma;
>  	size_t pinned;
>  
>  	pinned = 0;
>  	mutex_lock(&dev->struct_mutex);
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
> -		if (i915_gem_obj_is_pinned(obj))
> -			pinned += i915_gem_obj_ggtt_size(obj);
> +		list_for_each_entry(vma, &obj->vma_list, vma_link)
> +			if (vma->pin_count > 0)
> +				pinned += vma->node.size;

Same again.

>  	mutex_unlock(&dev->struct_mutex);
>  
>  	args->aper_size = dev_priv->gtt.base.total;
> @@ -3967,12 +3969,12 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  	if (obj->cache_level == cache_level)
>  		return 0;
>  
> -	if (i915_gem_obj_is_pinned(obj)) {
> -		DRM_DEBUG("can not change the cache level of pinned objects\n");
> -		return -EBUSY;
> -	}
> -
>  	list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
> +		if (vma->pin_count > 0) {
> +			DRM_DEBUG("can not change the cache level of pinned objects\n");
> +			return -EBUSY;
> +		}
> +
>  		if (!i915_gem_valid_gtt_space(vma, cache_level)) {
>  			ret = i915_vma_unbind(vma);
>  			if (ret)
> @@ -4506,6 +4508,7 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_madvise *args = data;
>  	struct drm_i915_gem_object *obj;
> +	struct i915_vma *vma;
>  	int ret;
>  
>  	switch (args->madv) {
> @@ -4526,9 +4529,11 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
>  		goto unlock;
>  	}
>  
> -	if (i915_gem_obj_is_pinned(obj)) {
> -		ret = -EINVAL;
> -		goto out;
> +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +		if (vma->pin_count > 0) {
> +			ret = -EINVAL;
> +			goto out;
> +		}

This does not need the pinned check.

>  	}
>  
>  	if (obj->pages &&
> @@ -5382,13 +5387,18 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
>  	return 0;
>  }
>  
> -bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
> +bool
> +i915_gem_obj_ggtt_is_pinned_view(struct drm_i915_gem_object *obj,
> +				 const struct i915_ggtt_view *view)
>  {
>  	struct i915_vma *vma;
> -	list_for_each_entry(vma, &obj->vma_list, vma_link)
> -		if (vma->pin_count > 0)
> +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +		if (!i915_is_ggtt(vma->vm))
> +			continue;
> +		if (i915_ggtt_view_equal(&vma->ggtt_view, view) &&
> +		    vma->pin_count > 0)
>  			return true;

This function is not required when you succeed in removing the is-pinned
queries.

> -
> +	}
>  	return false;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 133afcf..ed4a16b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -634,7 +634,7 @@ static int do_switch(struct intel_engine_cs *ring,
>  
>  	if (from != NULL && ring == &dev_priv->ring[RCS]) {
>  		BUG_ON(from->legacy_hw_ctx.rcs_state == NULL);
> -		BUG_ON(!i915_gem_obj_is_pinned(from->legacy_hw_ctx.rcs_state));
> +		BUG_ON(!i915_gem_obj_ggtt_is_pinned(from->legacy_hw_ctx.rcs_state));

The API is at fault here.

>  	}
>  
>  	if (should_skip_switch(ring, from, to))
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index 6f42569..cc52f9c 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -697,9 +697,7 @@ static void capture_bo(struct drm_i915_error_buffer *err,
>  	err->read_domains = obj->base.read_domains;
>  	err->write_domain = obj->base.write_domain;
>  	err->fence_reg = obj->fence_reg;
> -	err->pinned = 0;
> -	if (i915_gem_obj_is_pinned(obj))
> -		err->pinned = 1;
> +	err->pinned = vma->pin_count > 0;

Fix gpu error capturing for. Patches have been on the list for years.

>  	err->tiling = obj->tiling_mode;
>  	err->dirty = obj->dirty;
>  	err->purgeable = obj->madv != I915_MADV_WILLNEED;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 9f5485d..dc595a0 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -369,8 +369,8 @@ static void execlists_submit_contexts(struct intel_engine_cs *ring,
>  	struct intel_ringbuffer *ringbuf1 = NULL;
>  
>  	BUG_ON(!ctx_obj0);
> -	WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0));
> -	WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj));
> +	WARN_ON(!i915_gem_obj_ggtt_is_pinned(ctx_obj0));
> +	WARN_ON(!i915_gem_obj_ggtt_is_pinned(ringbuf0->obj));

Fix execlists, this is simply lazy code.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make pinned object handling more specific

[Joonas Lahtinen]

On to, 2015-06-04 at 10:01 +0100, Chris Wilson wrote:
> On Thu, Jun 04, 2015 at 11:49:06AM +0300, Joonas Lahtinen wrote:
> > Get rid of the over-generic i915_gem_obj_is_pinned and replace it
> > with i915_gem_obj_ggtt_is_pinned or more specific VMA handling.
> > 
> > Requested-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Joonas Lahtinen
> 
> I take it you didn't read my patch to accomplish the same.
> 

Nope, do not recall seeing one.

> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c     |  8 +++++--
> >  drivers/gpu/drm/i915/i915_drv.h         |  9 +++++++-
> >  drivers/gpu/drm/i915/i915_gem.c         | 38 +++++++++++++++++++++------------
> >  drivers/gpu/drm/i915/i915_gem_context.c |  2 +-
> >  drivers/gpu/drm/i915/i915_gpu_error.c   |  4 +---
> >  drivers/gpu/drm/i915/intel_lrc.c        |  8 +++----
> >  6 files changed, 44 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 3e17210..5442a18 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -519,6 +519,7 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data)
> >  	uintptr_t list = (uintptr_t) node->info_ent->data;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_gem_object *obj;
> > +	struct i915_vma *vma;
> >  	size_t total_obj_size, total_gtt_size;
> >  	int count, ret;
> >  
> > @@ -528,14 +529,17 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data)
> >  
> >  	total_obj_size = total_gtt_size = count = 0;
> >  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> 
> For instance here, we only care about the ggtt vma so lookup it first and
> then use it directly.
> 

Ack. Although, depending on what this information is used for, they
might or might not be interested in how much of the GTT the
partial/rotated or other views consume.

> > -		if (list == PINNED_LIST && !i915_gem_obj_is_pinned(obj))
> > +		if (list == PINNED_LIST && !i915_gem_obj_ggtt_is_pinned(obj))
> >  			continue;
> >  
> >  		seq_puts(m, "   ");
> >  		describe_obj(m, obj);
> >  		seq_putc(m, '\n');
> >  		total_obj_size += obj->base.size;
> > -		total_gtt_size += i915_gem_obj_ggtt_size(obj);
> > +		list_for_each_entry(vma, &obj->vma_list, vma_link)
> > +			if (i915_is_ggtt(vma->vm) &&
> > +			    (list != PINNED_LIST || vma->pin_count > 0))
> > +				total_gtt_size += vma->node.size;
> >  		count++;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 60aa962..be7bcc6 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2960,7 +2960,14 @@ i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
> >  {
> >  	return i915_gem_obj_to_ggtt_view(obj, &i915_ggtt_view_normal);
> >  }
> > -bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj);
> > +
> > +bool
> > +i915_gem_obj_ggtt_is_pinned_view(struct drm_i915_gem_object *obj,
> > +				 const struct i915_ggtt_view *view);
> > +static inline bool
> > +i915_gem_obj_ggtt_is_pinned(struct drm_i915_gem_object *obj) {
> > +	return i915_gem_obj_ggtt_is_pinned_view(obj, &i915_ggtt_view_normal);
> > +}
> >  
> >  /* Some GGTT VM helpers */
> >  #define i915_obj_to_ggtt(obj) \
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index be35f04..75218c2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -150,13 +150,15 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_gem_get_aperture *args = data;
> >  	struct drm_i915_gem_object *obj;
> > +	struct i915_vma *vma;
> >  	size_t pinned;
> >  
> >  	pinned = 0;
> >  	mutex_lock(&dev->struct_mutex);
> >  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
> > -		if (i915_gem_obj_is_pinned(obj))
> > -			pinned += i915_gem_obj_ggtt_size(obj);
> > +		list_for_each_entry(vma, &obj->vma_list, vma_link)
> > +			if (vma->pin_count > 0)
> > +				pinned += vma->node.size;
> 
> Same again.

I think adding is_ggtt() test for the VMA prior checking for pinning
would be the right thing to do? Otherwise the pinned partial/rotated or
other views will be unaccounted for.

> 
> >  	mutex_unlock(&dev->struct_mutex);
> >  
> >  	args->aper_size = dev_priv->gtt.base.total;
> > @@ -3967,12 +3969,12 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
> >  	if (obj->cache_level == cache_level)
> >  		return 0;
> >  
> > -	if (i915_gem_obj_is_pinned(obj)) {
> > -		DRM_DEBUG("can not change the cache level of pinned objects\n");
> > -		return -EBUSY;
> > -	}
> > -
> >  	list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
> > +		if (vma->pin_count > 0) {
> > +			DRM_DEBUG("can not change the cache level of pinned objects\n");
> > +			return -EBUSY;
> > +		}
> > +
> >  		if (!i915_gem_valid_gtt_space(vma, cache_level)) {
> >  			ret = i915_vma_unbind(vma);
> >  			if (ret)
> > @@ -4506,6 +4508,7 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_gem_madvise *args = data;
> >  	struct drm_i915_gem_object *obj;
> > +	struct i915_vma *vma;
> >  	int ret;
> >  
> >  	switch (args->madv) {
> > @@ -4526,9 +4529,11 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
> >  		goto unlock;
> >  	}
> >  
> > -	if (i915_gem_obj_is_pinned(obj)) {
> > -		ret = -EINVAL;
> > -		goto out;
> > +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> > +		if (vma->pin_count > 0) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> 
> This does not need the pinned check.
> 

Right, I wonder why it was there to begin with?

> >  	}
> >  
> >  	if (obj->pages &&
> > @@ -5382,13 +5387,18 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
> >  	return 0;
> >  }
> >  
> > -bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
> > +bool
> > +i915_gem_obj_ggtt_is_pinned_view(struct drm_i915_gem_object *obj,
> > +				 const struct i915_ggtt_view *view)
> >  {
> >  	struct i915_vma *vma;
> > -	list_for_each_entry(vma, &obj->vma_list, vma_link)
> > -		if (vma->pin_count > 0)
> > +	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> > +		if (!i915_is_ggtt(vma->vm))
> > +			continue;
> > +		if (i915_ggtt_view_equal(&vma->ggtt_view, view) &&
> > +		    vma->pin_count > 0)
> >  			return true;
> 
> This function is not required when you succeed in removing the is-pinned
> queries.
> 

True, but I left it exist due to the contex/error/execlist patches. It's
a step in the right direction, and Ville agreed on that.

> > -
> > +	}
> >  	return false;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 133afcf..ed4a16b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -634,7 +634,7 @@ static int do_switch(struct intel_engine_cs *ring,
> >  
> >  	if (from != NULL && ring == &dev_priv->ring[RCS]) {
> >  		BUG_ON(from->legacy_hw_ctx.rcs_state == NULL);
> > -		BUG_ON(!i915_gem_obj_is_pinned(from->legacy_hw_ctx.rcs_state));
> > +		BUG_ON(!i915_gem_obj_ggtt_is_pinned(from->legacy_hw_ctx.rcs_state));
> 
> The API is at fault here.

Not sure what you mean by that.

> 
> >  	}
> >  
> >  	if (should_skip_switch(ring, from, to))
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index 6f42569..cc52f9c 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -697,9 +697,7 @@ static void capture_bo(struct drm_i915_error_buffer *err,
> >  	err->read_domains = obj->base.read_domains;
> >  	err->write_domain = obj->base.write_domain;
> >  	err->fence_reg = obj->fence_reg;
> > -	err->pinned = 0;
> > -	if (i915_gem_obj_is_pinned(obj))
> > -		err->pinned = 1;
> > +	err->pinned = vma->pin_count > 0;
> 
> Fix gpu error capturing for. Patches have been on the list for years.

Which patches exactly? I can have a look. I think we should refine the
review process a bit if such bits are still floating in the cyberspace.

> 
> >  	err->tiling = obj->tiling_mode;
> >  	err->dirty = obj->dirty;
> >  	err->purgeable = obj->madv != I915_MADV_WILLNEED;
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> > index 9f5485d..dc595a0 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -369,8 +369,8 @@ static void execlists_submit_contexts(struct intel_engine_cs *ring,
> >  	struct intel_ringbuffer *ringbuf1 = NULL;
> >  
> >  	BUG_ON(!ctx_obj0);
> > -	WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0));
> > -	WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj));
> > +	WARN_ON(!i915_gem_obj_ggtt_is_pinned(ctx_obj0));
> > +	WARN_ON(!i915_gem_obj_ggtt_is_pinned(ringbuf0->obj));
> 
> Fix execlists, this is simply lazy code.

Doesn't sound like a very small task? Can you elborate. This is also one
part discussed with Ville.

Regards, Joonas

> -Chris
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Make pinned object handling more specific

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6534
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  270/270              270/270
ILK                                  303/303              303/303
SNB                                  312/312              312/312
IVB                                  343/343              343/343
BYT                                  287/287              287/287
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx