drm/i915: Report correct GGTT space usage
diff mbox

Message ID 1435656482-12672-1-git-send-email-tvrtko.ursulin@linux.intel.com
State New
Headers show

Commit Message

Tvrtko Ursulin June 30, 2015, 9:28 a.m. UTC
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Currently only normal views were accounted which under-accounts
the usage as reported in debugfs.

Introduce new helper, i915_gem_obj_total_ggtt_size, and use it
from call sites which want to know how much GGTT space are
objects using.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  8 ++++----
 drivers/gpu/drm/i915/i915_drv.h     |  3 +++
 drivers/gpu/drm/i915/i915_gem.c     | 16 +++++++++++++++-
 3 files changed, 22 insertions(+), 5 deletions(-)

Comments

Chris Wilson June 30, 2015, 9:36 a.m. UTC | #1
On Tue, Jun 30, 2015 at 10:28:02AM +0100, Tvrtko Ursulin wrote:
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a2a4a27..7d69294 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -156,7 +156,7 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
>  	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);
> +			pinned += i915_gem_obj_total_ggtt_size(obj);

Please just rewrite this as a single vma walk, or review the patches I
sent to do so.
-Chris
Tvrtko Ursulin June 30, 2015, 9:50 a.m. UTC | #2
On 06/30/2015 10:36 AM, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 10:28:02AM +0100, Tvrtko Ursulin wrote:
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index a2a4a27..7d69294 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -156,7 +156,7 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
>>   	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);
>> +			pinned += i915_gem_obj_total_ggtt_size(obj);
>
> Please just rewrite this as a single vma walk, or review the patches I
> sent to do so.

Sorry there is a lot of patches floating around and I don't know which 
ones you refer too.

I just noticed debugfs displays wrong values when GGTT views other than 
normal are present.

So just open code the loop in get_aperture you say?

It is slightly ugly when there are helpers, but it is also true 
different callers want to know slightly different things. (Difference 
between GGTT usage reported in get_aperture vs debugfs - pinned or just 
allocated.)

Regards,

Tvrtko
Chris Wilson June 30, 2015, 10:01 a.m. UTC | #3
On Tue, Jun 30, 2015 at 10:50:15AM +0100, Tvrtko Ursulin wrote:
> 
> On 06/30/2015 10:36 AM, Chris Wilson wrote:
> >On Tue, Jun 30, 2015 at 10:28:02AM +0100, Tvrtko Ursulin wrote:
> >>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>index a2a4a27..7d69294 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>@@ -156,7 +156,7 @@ i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
> >>  	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);
> >>+			pinned += i915_gem_obj_total_ggtt_size(obj);
> >
> >Please just rewrite this as a single vma walk, or review the patches I
> >sent to do so.
> 
> Sorry there is a lot of patches floating around and I don't know
> which ones you refer too.
> 
> I just noticed debugfs displays wrong values when GGTT views other
> than normal are present.
> 
> So just open code the loop in get_aperture you say?

Yes.
 
> It is slightly ugly when there are helpers, but it is also true
> different callers want to know slightly different things.
> (Difference between GGTT usage reported in get_aperture vs debugfs -
> pinned or just allocated.)

I've stopped taking notice of the GGTT usage in the debugfs output, it's
archaic and doesn't help me anymore. The only case where it really
matters is debugfs/i915_gem_gtt.
-Chris
Shuang He July 1, 2015, 5:58 p.m. UTC | #4
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6676
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  345/345              345/345
BYT                 -3              287/287              284/287
HSW                                  382/382              382/382
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
*BYT  igt@gem_partial_pwrite_pread@reads-uncached      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 31d8768..3bb6c99 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -269,7 +269,7 @@  static int i915_gem_stolen_list_info(struct seq_file *m, void *data)
 		list_add(&obj->obj_exec_link, &stolen);
 
 		total_obj_size += obj->base.size;
-		total_gtt_size += i915_gem_obj_ggtt_size(obj);
+		total_gtt_size += i915_gem_obj_total_ggtt_size(obj);
 		count++;
 	}
 	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list) {
@@ -299,7 +299,7 @@  static int i915_gem_stolen_list_info(struct seq_file *m, void *data)
 
 #define count_objects(list, member) do { \
 	list_for_each_entry(obj, list, member) { \
-		size += i915_gem_obj_ggtt_size(obj); \
+		size += i915_gem_obj_total_ggtt_size(obj); \
 		++count; \
 		if (obj->map_and_fenceable) { \
 			mappable_size += i915_gem_obj_ggtt_size(obj); \
@@ -405,7 +405,7 @@  static void print_batch_pool_stats(struct seq_file *m,
 
 #define count_vmas(list, member) do { \
 	list_for_each_entry(vma, list, member) { \
-		size += i915_gem_obj_ggtt_size(vma->obj); \
+		size += i915_gem_obj_total_ggtt_size(vma->obj); \
 		++count; \
 		if (vma->obj->map_and_fenceable) { \
 			mappable_size += i915_gem_obj_ggtt_size(vma->obj); \
@@ -535,7 +535,7 @@  static int i915_gem_gtt_info(struct seq_file *m, void *data)
 		describe_obj(m, obj);
 		seq_putc(m, '\n');
 		total_obj_size += obj->base.size;
-		total_gtt_size += i915_gem_obj_ggtt_size(obj);
+		total_gtt_size += i915_gem_obj_total_ggtt_size(obj);
 		count++;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ea9caf2..690b541 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3010,6 +3010,9 @@  i915_gem_obj_ggtt_size(struct drm_i915_gem_object *obj)
 	return i915_gem_obj_size(obj, i915_obj_to_ggtt(obj));
 }
 
+unsigned long
+i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj);
+
 static inline int __must_check
 i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
 		      uint32_t alignment,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a2a4a27..7d69294 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -156,7 +156,7 @@  i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
 	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);
+			pinned += i915_gem_obj_total_ggtt_size(obj);
 	mutex_unlock(&dev->struct_mutex);
 
 	args->aper_size = dev_priv->gtt.base.total;
@@ -5469,3 +5469,17 @@  bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
 	return false;
 }
 
+unsigned long
+i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
+{
+	unsigned long size = 0;
+	struct i915_vma *vma;
+
+	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		if (i915_is_ggtt(vma->vm) &&
+		    drm_mm_node_allocated(&vma->node))
+			size += vma->node.size;
+	}
+
+	return size;
+}