[PATCH 1/2] drm/i915/buddy: add some pretty printing

[PATCH 1/2] drm/i915/buddy: add some pretty printing

[Matthew Auld]

Implement the debug hook for the buddy resource manager. For this we
want to print out the status of the memory manager, including how much
memory is still allocatable, what page sizes we have etc. This will be
triggered when TTM is unable to fulfil an allocation request for device
local-memory.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_buddy.c             | 45 +++++++++++++++++++
 drivers/gpu/drm/i915/i915_buddy.h             |  8 ++++
 drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 20 ++++++++-
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_buddy.c b/drivers/gpu/drm/i915/i915_buddy.c
index 7b274c51cac0..240e881d9eb0 100644
--- a/drivers/gpu/drm/i915/i915_buddy.c
+++ b/drivers/gpu/drm/i915/i915_buddy.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/kmemleak.h>
+#include <linux/sizes.h>
 
 #include "i915_buddy.h"
 
@@ -82,6 +83,7 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size)
 	size = round_down(size, chunk_size);
 
 	mm->size = size;
+	mm->avail = size;
 	mm->chunk_size = chunk_size;
 	mm->max_order = ilog2(size) - ilog2(chunk_size);
 
@@ -155,6 +157,8 @@ void i915_buddy_fini(struct i915_buddy_mm *mm)
 		i915_block_free(mm, mm->roots[i]);
 	}
 
+	GEM_WARN_ON(mm->avail != mm->size);
+
 	kfree(mm->roots);
 	kfree(mm->free_list);
 }
@@ -230,6 +234,7 @@ void i915_buddy_free(struct i915_buddy_mm *mm,
 		     struct i915_buddy_block *block)
 {
 	GEM_BUG_ON(!i915_buddy_block_is_allocated(block));
+	mm->avail += i915_buddy_block_size(mm, block);
 	__i915_buddy_free(mm, block);
 }
 
@@ -283,6 +288,7 @@ i915_buddy_alloc(struct i915_buddy_mm *mm, unsigned int order)
 	}
 
 	mark_allocated(block);
+	mm->avail -= i915_buddy_block_size(mm, block);
 	kmemleak_update_trace(block);
 	return block;
 
@@ -368,6 +374,7 @@ int i915_buddy_alloc_range(struct i915_buddy_mm *mm,
 			}
 
 			mark_allocated(block);
+			mm->avail -= i915_buddy_block_size(mm, block);
 			list_add_tail(&block->link, &allocated);
 			continue;
 		}
@@ -402,6 +409,44 @@ int i915_buddy_alloc_range(struct i915_buddy_mm *mm,
 	return err;
 }
 
+void i915_buddy_block_print(struct i915_buddy_mm *mm,
+			    struct i915_buddy_block *block,
+			    struct drm_printer *p)
+{
+	u64 start = i915_buddy_block_offset(block);
+	u64 size = i915_buddy_block_size(mm, block);
+
+	drm_printf(p, "%#018llx-%#018llx: %llu\n", start, start + size, size);
+}
+
+void i915_buddy_print(struct i915_buddy_mm *mm, struct drm_printer *p)
+{
+	int order;
+
+	drm_printf(p, "chunk_size: %lluKB, total: %lluMB, free: %lluMB\n",
+		   mm->chunk_size >> 10, mm->size >> 20, mm->avail >> 20);
+
+	for (order = mm->max_order; order >= 0; order--) {
+		struct i915_buddy_block *block;
+		u64 count = 0, free;
+
+		list_for_each_entry(block, &mm->free_list[order], link) {
+			GEM_BUG_ON(!i915_buddy_block_is_free(block));
+			count++;
+		}
+
+		drm_printf(p, "order-%d ", order);
+
+		free = count * (mm->chunk_size << order);
+		if (free < SZ_1M)
+			drm_printf(p, "free: %lluKB", free >> 10);
+		else
+			drm_printf(p, "free: %lluMB", free >> 20);
+
+		drm_printf(p, ", pages: %llu\n", count);
+	}
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/i915_buddy.c"
 #endif
diff --git a/drivers/gpu/drm/i915/i915_buddy.h b/drivers/gpu/drm/i915/i915_buddy.h
index 3940d632f208..7077742112ac 100644
--- a/drivers/gpu/drm/i915/i915_buddy.h
+++ b/drivers/gpu/drm/i915/i915_buddy.h
@@ -10,6 +10,8 @@
 #include <linux/list.h>
 #include <linux/slab.h>
 
+#include <drm/drm_print.h>
+
 struct i915_buddy_block {
 #define I915_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12)
 #define I915_BUDDY_HEADER_STATE  GENMASK_ULL(11, 10)
@@ -69,6 +71,7 @@ struct i915_buddy_mm {
 	/* Must be at least PAGE_SIZE */
 	u64 chunk_size;
 	u64 size;
+	u64 avail;
 };
 
 static inline u64
@@ -129,6 +132,11 @@ void i915_buddy_free(struct i915_buddy_mm *mm, struct i915_buddy_block *block);
 
 void i915_buddy_free_list(struct i915_buddy_mm *mm, struct list_head *objects);
 
+void i915_buddy_print(struct i915_buddy_mm *mm, struct drm_printer *p);
+void i915_buddy_block_print(struct i915_buddy_mm *mm,
+			    struct i915_buddy_block *block,
+			    struct drm_printer *p);
+
 void i915_buddy_module_exit(void);
 int i915_buddy_module_init(void);
 
diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
index 6877362f6b85..95ab786a1fe4 100644
--- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
+++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
@@ -126,12 +126,30 @@ static void i915_ttm_buddy_man_free(struct ttm_resource_manager *man,
 	kfree(bman_res);
 }
 
+static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
+				     struct drm_printer *printer)
+{
+	struct i915_ttm_buddy_manager *bman = to_buddy_manager(man);
+	struct i915_buddy_block *block;
+
+	mutex_lock(&bman->lock);
+	drm_printf(printer, "default_page_size: %lluKB\n",
+		   bman->default_page_size >> 10);
+
+	i915_buddy_print(&bman->mm, printer);
+
+	drm_printf(printer, "reserved:\n");
+	list_for_each_entry(block, &bman->reserved, link)
+		i915_buddy_block_print(&bman->mm, block, printer);
+	mutex_unlock(&bman->lock);
+}
+
 static const struct ttm_resource_manager_func i915_ttm_buddy_manager_func = {
 	.alloc = i915_ttm_buddy_man_alloc,
 	.free = i915_ttm_buddy_man_free,
+	.debug = i915_ttm_buddy_man_debug,
 };
 
-
 /**
  * i915_ttm_buddy_man_init - Setup buddy allocator based ttm manager
  * @bdev: The ttm device
-- 
2.26.3

[Intel-gfx] [PATCH 1/2] drm/i915/buddy: add some pretty printing

[Matthew Auld]

Implement the debug hook for the buddy resource manager. For this we
want to print out the status of the memory manager, including how much
memory is still allocatable, what page sizes we have etc. This will be
triggered when TTM is unable to fulfil an allocation request for device
local-memory.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_buddy.c             | 45 +++++++++++++++++++
 drivers/gpu/drm/i915/i915_buddy.h             |  8 ++++
 drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 20 ++++++++-
 3 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_buddy.c b/drivers/gpu/drm/i915/i915_buddy.c
index 7b274c51cac0..240e881d9eb0 100644
--- a/drivers/gpu/drm/i915/i915_buddy.c
+++ b/drivers/gpu/drm/i915/i915_buddy.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/kmemleak.h>
+#include <linux/sizes.h>
 
 #include "i915_buddy.h"
 
@@ -82,6 +83,7 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64 size, u64 chunk_size)
 	size = round_down(size, chunk_size);
 
 	mm->size = size;
+	mm->avail = size;
 	mm->chunk_size = chunk_size;
 	mm->max_order = ilog2(size) - ilog2(chunk_size);
 
@@ -155,6 +157,8 @@ void i915_buddy_fini(struct i915_buddy_mm *mm)
 		i915_block_free(mm, mm->roots[i]);
 	}
 
+	GEM_WARN_ON(mm->avail != mm->size);
+
 	kfree(mm->roots);
 	kfree(mm->free_list);
 }
@@ -230,6 +234,7 @@ void i915_buddy_free(struct i915_buddy_mm *mm,
 		     struct i915_buddy_block *block)
 {
 	GEM_BUG_ON(!i915_buddy_block_is_allocated(block));
+	mm->avail += i915_buddy_block_size(mm, block);
 	__i915_buddy_free(mm, block);
 }
 
@@ -283,6 +288,7 @@ i915_buddy_alloc(struct i915_buddy_mm *mm, unsigned int order)
 	}
 
 	mark_allocated(block);
+	mm->avail -= i915_buddy_block_size(mm, block);
 	kmemleak_update_trace(block);
 	return block;
 
@@ -368,6 +374,7 @@ int i915_buddy_alloc_range(struct i915_buddy_mm *mm,
 			}
 
 			mark_allocated(block);
+			mm->avail -= i915_buddy_block_size(mm, block);
 			list_add_tail(&block->link, &allocated);
 			continue;
 		}
@@ -402,6 +409,44 @@ int i915_buddy_alloc_range(struct i915_buddy_mm *mm,
 	return err;
 }
 
+void i915_buddy_block_print(struct i915_buddy_mm *mm,
+			    struct i915_buddy_block *block,
+			    struct drm_printer *p)
+{
+	u64 start = i915_buddy_block_offset(block);
+	u64 size = i915_buddy_block_size(mm, block);
+
+	drm_printf(p, "%#018llx-%#018llx: %llu\n", start, start + size, size);
+}
+
+void i915_buddy_print(struct i915_buddy_mm *mm, struct drm_printer *p)
+{
+	int order;
+
+	drm_printf(p, "chunk_size: %lluKB, total: %lluMB, free: %lluMB\n",
+		   mm->chunk_size >> 10, mm->size >> 20, mm->avail >> 20);
+
+	for (order = mm->max_order; order >= 0; order--) {
+		struct i915_buddy_block *block;
+		u64 count = 0, free;
+
+		list_for_each_entry(block, &mm->free_list[order], link) {
+			GEM_BUG_ON(!i915_buddy_block_is_free(block));
+			count++;
+		}
+
+		drm_printf(p, "order-%d ", order);
+
+		free = count * (mm->chunk_size << order);
+		if (free < SZ_1M)
+			drm_printf(p, "free: %lluKB", free >> 10);
+		else
+			drm_printf(p, "free: %lluMB", free >> 20);
+
+		drm_printf(p, ", pages: %llu\n", count);
+	}
+}
+
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
 #include "selftests/i915_buddy.c"
 #endif
diff --git a/drivers/gpu/drm/i915/i915_buddy.h b/drivers/gpu/drm/i915/i915_buddy.h
index 3940d632f208..7077742112ac 100644
--- a/drivers/gpu/drm/i915/i915_buddy.h
+++ b/drivers/gpu/drm/i915/i915_buddy.h
@@ -10,6 +10,8 @@
 #include <linux/list.h>
 #include <linux/slab.h>
 
+#include <drm/drm_print.h>
+
 struct i915_buddy_block {
 #define I915_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12)
 #define I915_BUDDY_HEADER_STATE  GENMASK_ULL(11, 10)
@@ -69,6 +71,7 @@ struct i915_buddy_mm {
 	/* Must be at least PAGE_SIZE */
 	u64 chunk_size;
 	u64 size;
+	u64 avail;
 };
 
 static inline u64
@@ -129,6 +132,11 @@ void i915_buddy_free(struct i915_buddy_mm *mm, struct i915_buddy_block *block);
 
 void i915_buddy_free_list(struct i915_buddy_mm *mm, struct list_head *objects);
 
+void i915_buddy_print(struct i915_buddy_mm *mm, struct drm_printer *p);
+void i915_buddy_block_print(struct i915_buddy_mm *mm,
+			    struct i915_buddy_block *block,
+			    struct drm_printer *p);
+
 void i915_buddy_module_exit(void);
 int i915_buddy_module_init(void);
 
diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
index 6877362f6b85..95ab786a1fe4 100644
--- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
+++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
@@ -126,12 +126,30 @@ static void i915_ttm_buddy_man_free(struct ttm_resource_manager *man,
 	kfree(bman_res);
 }
 
+static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
+				     struct drm_printer *printer)
+{
+	struct i915_ttm_buddy_manager *bman = to_buddy_manager(man);
+	struct i915_buddy_block *block;
+
+	mutex_lock(&bman->lock);
+	drm_printf(printer, "default_page_size: %lluKB\n",
+		   bman->default_page_size >> 10);
+
+	i915_buddy_print(&bman->mm, printer);
+
+	drm_printf(printer, "reserved:\n");
+	list_for_each_entry(block, &bman->reserved, link)
+		i915_buddy_block_print(&bman->mm, block, printer);
+	mutex_unlock(&bman->lock);
+}
+
 static const struct ttm_resource_manager_func i915_ttm_buddy_manager_func = {
 	.alloc = i915_ttm_buddy_man_alloc,
 	.free = i915_ttm_buddy_man_free,
+	.debug = i915_ttm_buddy_man_debug,
 };
 
-
 /**
  * i915_ttm_buddy_man_init - Setup buddy allocator based ttm manager
  * @bdev: The ttm device
-- 
2.26.3

[PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Matthew Auld]

This should give a more complete view of the various bits of internal
resource manager state, for device local-memory.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index eec0d349ea6a..109e6feed6be 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 static int i915_gem_object_info(struct seq_file *m, void *data)
 {
 	struct drm_i915_private *i915 = node_to_i915(m->private);
+	struct drm_printer p = drm_seq_file_printer(m);
 	struct intel_memory_region *mr;
 	enum intel_region_id id;
 
@@ -245,9 +246,14 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
 		   i915->mm.shrink_count,
 		   atomic_read(&i915->mm.free_count),
 		   i915->mm.shrink_memory);
-	for_each_memory_region(mr, i915, id)
-		seq_printf(m, "%s: total:%pa, available:%pa bytes\n",
-			   mr->name, &mr->total, &mr->avail);
+	for_each_memory_region(mr, i915, id) {
+		seq_printf(m, "%s: ", mr->name);
+		if (mr->region_private)
+			ttm_resource_manager_debug(mr->region_private, &p);
+		else
+			seq_printf(m, "total:%pa, available:%pa bytes\n",
+				   &mr->total, &mr->avail);
+	}
 
 	return 0;
 }
-- 
2.26.3

[Intel-gfx] [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Matthew Auld]

This should give a more complete view of the various bits of internal
resource manager state, for device local-memory.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index eec0d349ea6a..109e6feed6be 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 static int i915_gem_object_info(struct seq_file *m, void *data)
 {
 	struct drm_i915_private *i915 = node_to_i915(m->private);
+	struct drm_printer p = drm_seq_file_printer(m);
 	struct intel_memory_region *mr;
 	enum intel_region_id id;
 
@@ -245,9 +246,14 @@ static int i915_gem_object_info(struct seq_file *m, void *data)
 		   i915->mm.shrink_count,
 		   atomic_read(&i915->mm.free_count),
 		   i915->mm.shrink_memory);
-	for_each_memory_region(mr, i915, id)
-		seq_printf(m, "%s: total:%pa, available:%pa bytes\n",
-			   mr->name, &mr->total, &mr->avail);
+	for_each_memory_region(mr, i915, id) {
+		seq_printf(m, "%s: ", mr->name);
+		if (mr->region_private)
+			ttm_resource_manager_debug(mr->region_private, &p);
+		else
+			seq_printf(m, "total:%pa, available:%pa bytes\n",
+				   &mr->total, &mr->avail);
+	}
 
 	return 0;
 }
-- 
2.26.3

Re: [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Thomas Hellström]

On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> This should give a more complete view of the various bits of internal
> resource manager state, for device local-memory.
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index eec0d349ea6a..109e6feed6be 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m,
> struct drm_i915_gem_object *obj)
>  static int i915_gem_object_info(struct seq_file *m, void *data)
>  {
>         struct drm_i915_private *i915 = node_to_i915(m->private);
> +       struct drm_printer p = drm_seq_file_printer(m);
>         struct intel_memory_region *mr;
>         enum intel_region_id id;
>  
> @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct seq_file
> *m, void *data)
>                    i915->mm.shrink_count,
>                    atomic_read(&i915->mm.free_count),
>                    i915->mm.shrink_memory);
> -       for_each_memory_region(mr, i915, id)
> -               seq_printf(m, "%s: total:%pa, available:%pa bytes\n",
> -                          mr->name, &mr->total, &mr->avail);
> +       for_each_memory_region(mr, i915, id) {
> +               seq_printf(m, "%s: ", mr->name);
> +               if (mr->region_private)
> +                       ttm_resource_manager_debug(mr-
> >region_private, &p);
> +               else
> +                       seq_printf(m, "total:%pa, available:%pa
> bytes\n",
> +                                  &mr->total, &mr->avail);

Hm. Shouldn't we make the above intel_memory_region_debug() or perhaps
intel_memory_region_info() to avoid using memory region internals
directly here?

/Thomas

Re: [Intel-gfx] [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Thomas Hellström]

On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> This should give a more complete view of the various bits of internal
> resource manager state, for device local-memory.
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index eec0d349ea6a..109e6feed6be 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m,
> struct drm_i915_gem_object *obj)
>  static int i915_gem_object_info(struct seq_file *m, void *data)
>  {
>         struct drm_i915_private *i915 = node_to_i915(m->private);
> +       struct drm_printer p = drm_seq_file_printer(m);
>         struct intel_memory_region *mr;
>         enum intel_region_id id;
>  
> @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct seq_file
> *m, void *data)
>                    i915->mm.shrink_count,
>                    atomic_read(&i915->mm.free_count),
>                    i915->mm.shrink_memory);
> -       for_each_memory_region(mr, i915, id)
> -               seq_printf(m, "%s: total:%pa, available:%pa bytes\n",
> -                          mr->name, &mr->total, &mr->avail);
> +       for_each_memory_region(mr, i915, id) {
> +               seq_printf(m, "%s: ", mr->name);
> +               if (mr->region_private)
> +                       ttm_resource_manager_debug(mr-
> >region_private, &p);
> +               else
> +                       seq_printf(m, "total:%pa, available:%pa
> bytes\n",
> +                                  &mr->total, &mr->avail);

Hm. Shouldn't we make the above intel_memory_region_debug() or perhaps
intel_memory_region_info() to avoid using memory region internals
directly here?

/Thomas

Re: [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Daniel Vetter]

On Thu, Aug 19, 2021 at 09:32:20AM +0200, Thomas Hellström wrote:
> On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> > This should give a more complete view of the various bits of internal
> > resource manager state, for device local-memory.
> > 
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index eec0d349ea6a..109e6feed6be 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m,
> > struct drm_i915_gem_object *obj)
> >  static int i915_gem_object_info(struct seq_file *m, void *data)
> >  {
> >         struct drm_i915_private *i915 = node_to_i915(m->private);
> > +       struct drm_printer p = drm_seq_file_printer(m);
> >         struct intel_memory_region *mr;
> >         enum intel_region_id id;
> >  
> > @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct seq_file
> > *m, void *data)
> >                    i915->mm.shrink_count,
> >                    atomic_read(&i915->mm.free_count),
> >                    i915->mm.shrink_memory);
> > -       for_each_memory_region(mr, i915, id)
> > -               seq_printf(m, "%s: total:%pa, available:%pa bytes\n",
> > -                          mr->name, &mr->total, &mr->avail);
> > +       for_each_memory_region(mr, i915, id) {
> > +               seq_printf(m, "%s: ", mr->name);
> > +               if (mr->region_private)
> > +                       ttm_resource_manager_debug(mr-
> > >region_private, &p);
> > +               else
> > +                       seq_printf(m, "total:%pa, available:%pa
> > bytes\n",
> > +                                  &mr->total, &mr->avail);
> 
> Hm. Shouldn't we make the above intel_memory_region_debug() or perhaps
> intel_memory_region_info() to avoid using memory region internals
> directly here?

Imo we should just emebed ttm_resource_mager into our own and not try to
abstract this all away that much. At least in upstream there is just not
going to be another memory region implementation, and for backporting I'm
not sure these abstractions really help that much - we're touching all the
same code still in the end.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Daniel Vetter]

On Thu, Aug 19, 2021 at 09:32:20AM +0200, Thomas Hellström wrote:
> On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> > This should give a more complete view of the various bits of internal
> > resource manager state, for device local-memory.
> > 
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index eec0d349ea6a..109e6feed6be 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m,
> > struct drm_i915_gem_object *obj)
> >  static int i915_gem_object_info(struct seq_file *m, void *data)
> >  {
> >         struct drm_i915_private *i915 = node_to_i915(m->private);
> > +       struct drm_printer p = drm_seq_file_printer(m);
> >         struct intel_memory_region *mr;
> >         enum intel_region_id id;
> >  
> > @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct seq_file
> > *m, void *data)
> >                    i915->mm.shrink_count,
> >                    atomic_read(&i915->mm.free_count),
> >                    i915->mm.shrink_memory);
> > -       for_each_memory_region(mr, i915, id)
> > -               seq_printf(m, "%s: total:%pa, available:%pa bytes\n",
> > -                          mr->name, &mr->total, &mr->avail);
> > +       for_each_memory_region(mr, i915, id) {
> > +               seq_printf(m, "%s: ", mr->name);
> > +               if (mr->region_private)
> > +                       ttm_resource_manager_debug(mr-
> > >region_private, &p);
> > +               else
> > +                       seq_printf(m, "total:%pa, available:%pa
> > bytes\n",
> > +                                  &mr->total, &mr->avail);
> 
> Hm. Shouldn't we make the above intel_memory_region_debug() or perhaps
> intel_memory_region_info() to avoid using memory region internals
> directly here?

Imo we should just emebed ttm_resource_mager into our own and not try to
abstract this all away that much. At least in upstream there is just not
going to be another memory region implementation, and for backporting I'm
not sure these abstractions really help that much - we're touching all the
same code still in the end.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Thomas Hellström]

On Thu, 2021-08-26 at 11:16 +0200, Daniel Vetter wrote:
> On Thu, Aug 19, 2021 at 09:32:20AM +0200, Thomas Hellström wrote:
> > On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> > > This should give a more complete view of the various bits of
> > > internal
> > > resource manager state, for device local-memory.
> > > 
> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index eec0d349ea6a..109e6feed6be 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m,
> > > struct drm_i915_gem_object *obj)
> > >  static int i915_gem_object_info(struct seq_file *m, void *data)
> > >  {
> > >         struct drm_i915_private *i915 = node_to_i915(m->private);
> > > +       struct drm_printer p = drm_seq_file_printer(m);
> > >         struct intel_memory_region *mr;
> > >         enum intel_region_id id;
> > >  
> > > @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct
> > > seq_file
> > > *m, void *data)
> > >                    i915->mm.shrink_count,
> > >                    atomic_read(&i915->mm.free_count),
> > >                    i915->mm.shrink_memory);
> > > -       for_each_memory_region(mr, i915, id)
> > > -               seq_printf(m, "%s: total:%pa, available:%pa
> > > bytes\n",
> > > -                          mr->name, &mr->total, &mr->avail);
> > > +       for_each_memory_region(mr, i915, id) {
> > > +               seq_printf(m, "%s: ", mr->name);
> > > +               if (mr->region_private)
> > > +                       ttm_resource_manager_debug(mr-
> > > > region_private, &p);
> > > +               else
> > > +                       seq_printf(m, "total:%pa, available:%pa
> > > bytes\n",
> > > +                                  &mr->total, &mr->avail);
> > 
> > Hm. Shouldn't we make the above intel_memory_region_debug() or
> > perhaps
> > intel_memory_region_info() to avoid using memory region internals
> > directly here?
> 
> Imo we should just emebed ttm_resource_mager into our own and not try
> to
> abstract this all away that much. At least in upstream there is just
> not
> going to be another memory region implementation, and for backporting
> I'm
> not sure these abstractions really help that much - we're touching
> all the
> same code still in the end.

Hmm, yes. Here I was seeing the separation between the debugfs code and
the intel_memory_region code, rather between the latter and TTM.

The i915 driver is currently much "everything uses everything" which
IMHO is not really good for code understanding and maintainance.

/Thomas

> -Daniel

Re: [Intel-gfx] [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Thomas Hellström]

On Thu, 2021-08-26 at 11:16 +0200, Daniel Vetter wrote:
> On Thu, Aug 19, 2021 at 09:32:20AM +0200, Thomas Hellström wrote:
> > On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> > > This should give a more complete view of the various bits of
> > > internal
> > > resource manager state, for device local-memory.
> > > 
> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index eec0d349ea6a..109e6feed6be 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m,
> > > struct drm_i915_gem_object *obj)
> > >  static int i915_gem_object_info(struct seq_file *m, void *data)
> > >  {
> > >         struct drm_i915_private *i915 = node_to_i915(m->private);
> > > +       struct drm_printer p = drm_seq_file_printer(m);
> > >         struct intel_memory_region *mr;
> > >         enum intel_region_id id;
> > >  
> > > @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct
> > > seq_file
> > > *m, void *data)
> > >                    i915->mm.shrink_count,
> > >                    atomic_read(&i915->mm.free_count),
> > >                    i915->mm.shrink_memory);
> > > -       for_each_memory_region(mr, i915, id)
> > > -               seq_printf(m, "%s: total:%pa, available:%pa
> > > bytes\n",
> > > -                          mr->name, &mr->total, &mr->avail);
> > > +       for_each_memory_region(mr, i915, id) {
> > > +               seq_printf(m, "%s: ", mr->name);
> > > +               if (mr->region_private)
> > > +                       ttm_resource_manager_debug(mr-
> > > > region_private, &p);
> > > +               else
> > > +                       seq_printf(m, "total:%pa, available:%pa
> > > bytes\n",
> > > +                                  &mr->total, &mr->avail);
> > 
> > Hm. Shouldn't we make the above intel_memory_region_debug() or
> > perhaps
> > intel_memory_region_info() to avoid using memory region internals
> > directly here?
> 
> Imo we should just emebed ttm_resource_mager into our own and not try
> to
> abstract this all away that much. At least in upstream there is just
> not
> going to be another memory region implementation, and for backporting
> I'm
> not sure these abstractions really help that much - we're touching
> all the
> same code still in the end.

Hmm, yes. Here I was seeing the separation between the debugfs code and
the intel_memory_region code, rather between the latter and TTM.

The i915 driver is currently much "everything uses everything" which
IMHO is not really good for code understanding and maintainance.

/Thomas

> -Daniel

Re: [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Thomas Hellström]

On Thu, 2021-08-26 at 11:51 +0200, Thomas Hellström wrote:
> On Thu, 2021-08-26 at 11:16 +0200, Daniel Vetter wrote:
> > On Thu, Aug 19, 2021 at 09:32:20AM +0200, Thomas Hellström wrote:
> > > On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> > > > This should give a more complete view of the various bits of
> > > > internal
> > > > resource manager state, for device local-memory.
> > > > 
> > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
> > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index eec0d349ea6a..109e6feed6be 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file
> > > > *m,
> > > > struct drm_i915_gem_object *obj)
> > > >  static int i915_gem_object_info(struct seq_file *m, void
> > > > *data)
> > > >  {
> > > >         struct drm_i915_private *i915 = node_to_i915(m-
> > > > >private);
> > > > +       struct drm_printer p = drm_seq_file_printer(m);
> > > >         struct intel_memory_region *mr;
> > > >         enum intel_region_id id;
> > > >  
> > > > @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct
> > > > seq_file
> > > > *m, void *data)
> > > >                    i915->mm.shrink_count,
> > > >                    atomic_read(&i915->mm.free_count),
> > > >                    i915->mm.shrink_memory);
> > > > -       for_each_memory_region(mr, i915, id)
> > > > -               seq_printf(m, "%s: total:%pa, available:%pa
> > > > bytes\n",
> > > > -                          mr->name, &mr->total, &mr->avail);
> > > > +       for_each_memory_region(mr, i915, id) {
> > > > +               seq_printf(m, "%s: ", mr->name);
> > > > +               if (mr->region_private)
> > > > +                       ttm_resource_manager_debug(mr-
> > > > > region_private, &p);
> > > > +               else
> > > > +                       seq_printf(m, "total:%pa, available:%pa
> > > > bytes\n",
> > > > +                                  &mr->total, &mr->avail);
> > > 
> > > Hm. Shouldn't we make the above intel_memory_region_debug() or
> > > perhaps
> > > intel_memory_region_info() to avoid using memory region internals
> > > directly here?
> > 
> > Imo we should just emebed ttm_resource_mager into our own and not
> > try
> > to
> > abstract this all away that much. At least in upstream there is
> > just
> > not
> > going to be another memory region implementation, and for
> > backporting
> > I'm
> > not sure these abstractions really help that much - we're touching
> > all the
> > same code still in the end.
> 
> Hmm, yes. Here I was seeing the separation between the debugfs code
> and
> the intel_memory_region code, rather between the latter and TTM.
> 
> The i915 driver is currently much "everything uses everything" which
> IMHO is not really good for code understanding and maintainance.
> 
> /Thomas
> 
> > -Daniel
> 
But yes, agreed, on the memory region backends it doesn't make much
sense. It was helpful during bringup but yes we probably won't be
adding another backend hopefully.

/Thomas

Re: [Intel-gfx] [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Thomas Hellström]

On Thu, 2021-08-26 at 11:51 +0200, Thomas Hellström wrote:
> On Thu, 2021-08-26 at 11:16 +0200, Daniel Vetter wrote:
> > On Thu, Aug 19, 2021 at 09:32:20AM +0200, Thomas Hellström wrote:
> > > On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> > > > This should give a more complete view of the various bits of
> > > > internal
> > > > resource manager state, for device local-memory.
> > > > 
> > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
> > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index eec0d349ea6a..109e6feed6be 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file
> > > > *m,
> > > > struct drm_i915_gem_object *obj)
> > > >  static int i915_gem_object_info(struct seq_file *m, void
> > > > *data)
> > > >  {
> > > >         struct drm_i915_private *i915 = node_to_i915(m-
> > > > >private);
> > > > +       struct drm_printer p = drm_seq_file_printer(m);
> > > >         struct intel_memory_region *mr;
> > > >         enum intel_region_id id;
> > > >  
> > > > @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct
> > > > seq_file
> > > > *m, void *data)
> > > >                    i915->mm.shrink_count,
> > > >                    atomic_read(&i915->mm.free_count),
> > > >                    i915->mm.shrink_memory);
> > > > -       for_each_memory_region(mr, i915, id)
> > > > -               seq_printf(m, "%s: total:%pa, available:%pa
> > > > bytes\n",
> > > > -                          mr->name, &mr->total, &mr->avail);
> > > > +       for_each_memory_region(mr, i915, id) {
> > > > +               seq_printf(m, "%s: ", mr->name);
> > > > +               if (mr->region_private)
> > > > +                       ttm_resource_manager_debug(mr-
> > > > > region_private, &p);
> > > > +               else
> > > > +                       seq_printf(m, "total:%pa, available:%pa
> > > > bytes\n",
> > > > +                                  &mr->total, &mr->avail);
> > > 
> > > Hm. Shouldn't we make the above intel_memory_region_debug() or
> > > perhaps
> > > intel_memory_region_info() to avoid using memory region internals
> > > directly here?
> > 
> > Imo we should just emebed ttm_resource_mager into our own and not
> > try
> > to
> > abstract this all away that much. At least in upstream there is
> > just
> > not
> > going to be another memory region implementation, and for
> > backporting
> > I'm
> > not sure these abstractions really help that much - we're touching
> > all the
> > same code still in the end.
> 
> Hmm, yes. Here I was seeing the separation between the debugfs code
> and
> the intel_memory_region code, rather between the latter and TTM.
> 
> The i915 driver is currently much "everything uses everything" which
> IMHO is not really good for code understanding and maintainance.
> 
> /Thomas
> 
> > -Daniel
> 
But yes, agreed, on the memory region backends it doesn't make much
sense. It was helpful during bringup but yes we probably won't be
adding another backend hopefully.

/Thomas

Re: [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Daniel Vetter]

On Thu, Aug 26, 2021 at 11:51:44AM +0200, Thomas Hellström wrote:
> On Thu, 2021-08-26 at 11:16 +0200, Daniel Vetter wrote:
> > On Thu, Aug 19, 2021 at 09:32:20AM +0200, Thomas Hellström wrote:
> > > On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> > > > This should give a more complete view of the various bits of
> > > > internal
> > > > resource manager state, for device local-memory.
> > > > 
> > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
> > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index eec0d349ea6a..109e6feed6be 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m,
> > > > struct drm_i915_gem_object *obj)
> > > >  static int i915_gem_object_info(struct seq_file *m, void *data)
> > > >  {
> > > >         struct drm_i915_private *i915 = node_to_i915(m->private);
> > > > +       struct drm_printer p = drm_seq_file_printer(m);
> > > >         struct intel_memory_region *mr;
> > > >         enum intel_region_id id;
> > > >  
> > > > @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct
> > > > seq_file
> > > > *m, void *data)
> > > >                    i915->mm.shrink_count,
> > > >                    atomic_read(&i915->mm.free_count),
> > > >                    i915->mm.shrink_memory);
> > > > -       for_each_memory_region(mr, i915, id)
> > > > -               seq_printf(m, "%s: total:%pa, available:%pa
> > > > bytes\n",
> > > > -                          mr->name, &mr->total, &mr->avail);
> > > > +       for_each_memory_region(mr, i915, id) {
> > > > +               seq_printf(m, "%s: ", mr->name);
> > > > +               if (mr->region_private)
> > > > +                       ttm_resource_manager_debug(mr-
> > > > > region_private, &p);
> > > > +               else
> > > > +                       seq_printf(m, "total:%pa, available:%pa
> > > > bytes\n",
> > > > +                                  &mr->total, &mr->avail);
> > > 
> > > Hm. Shouldn't we make the above intel_memory_region_debug() or
> > > perhaps
> > > intel_memory_region_info() to avoid using memory region internals
> > > directly here?
> > 
> > Imo we should just emebed ttm_resource_mager into our own and not try
> > to
> > abstract this all away that much. At least in upstream there is just
> > not
> > going to be another memory region implementation, and for backporting
> > I'm
> > not sure these abstractions really help that much - we're touching
> > all the
> > same code still in the end.
> 
> Hmm, yes. Here I was seeing the separation between the debugfs code and
> the intel_memory_region code, rather between the latter and TTM.
> 
> The i915 driver is currently much "everything uses everything" which
> IMHO is not really good for code understanding and maintainance.

Ah yes I agree, we don't have clear seperation of concerns really, and
debugfs is all over. I got confused a bit with the ->region_private
pointer and thought you'd be talking about that.

My experience has been that going over the interface functions and trying
to kerneldoc helps a lot with this, because instead of documenting some
major confusion you can just clean it up first. We should definitely try
to componentize stuff more and not leak internal details all over the
place.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Daniel Vetter]

On Thu, Aug 26, 2021 at 11:51:44AM +0200, Thomas Hellström wrote:
> On Thu, 2021-08-26 at 11:16 +0200, Daniel Vetter wrote:
> > On Thu, Aug 19, 2021 at 09:32:20AM +0200, Thomas Hellström wrote:
> > > On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> > > > This should give a more complete view of the various bits of
> > > > internal
> > > > resource manager state, for device local-memory.
> > > > 
> > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
> > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index eec0d349ea6a..109e6feed6be 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m,
> > > > struct drm_i915_gem_object *obj)
> > > >  static int i915_gem_object_info(struct seq_file *m, void *data)
> > > >  {
> > > >         struct drm_i915_private *i915 = node_to_i915(m->private);
> > > > +       struct drm_printer p = drm_seq_file_printer(m);
> > > >         struct intel_memory_region *mr;
> > > >         enum intel_region_id id;
> > > >  
> > > > @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct
> > > > seq_file
> > > > *m, void *data)
> > > >                    i915->mm.shrink_count,
> > > >                    atomic_read(&i915->mm.free_count),
> > > >                    i915->mm.shrink_memory);
> > > > -       for_each_memory_region(mr, i915, id)
> > > > -               seq_printf(m, "%s: total:%pa, available:%pa
> > > > bytes\n",
> > > > -                          mr->name, &mr->total, &mr->avail);
> > > > +       for_each_memory_region(mr, i915, id) {
> > > > +               seq_printf(m, "%s: ", mr->name);
> > > > +               if (mr->region_private)
> > > > +                       ttm_resource_manager_debug(mr-
> > > > > region_private, &p);
> > > > +               else
> > > > +                       seq_printf(m, "total:%pa, available:%pa
> > > > bytes\n",
> > > > +                                  &mr->total, &mr->avail);
> > > 
> > > Hm. Shouldn't we make the above intel_memory_region_debug() or
> > > perhaps
> > > intel_memory_region_info() to avoid using memory region internals
> > > directly here?
> > 
> > Imo we should just emebed ttm_resource_mager into our own and not try
> > to
> > abstract this all away that much. At least in upstream there is just
> > not
> > going to be another memory region implementation, and for backporting
> > I'm
> > not sure these abstractions really help that much - we're touching
> > all the
> > same code still in the end.
> 
> Hmm, yes. Here I was seeing the separation between the debugfs code and
> the intel_memory_region code, rather between the latter and TTM.
> 
> The i915 driver is currently much "everything uses everything" which
> IMHO is not really good for code understanding and maintainance.

Ah yes I agree, we don't have clear seperation of concerns really, and
debugfs is all over. I got confused a bit with the ->region_private
pointer and thought you'd be talking about that.

My experience has been that going over the interface functions and trying
to kerneldoc helps a lot with this, because instead of documenting some
major confusion you can just clean it up first. We should definitely try
to componentize stuff more and not leak internal details all over the
place.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Daniel Vetter]

On Thu, Aug 26, 2021 at 12:03:29PM +0200, Daniel Vetter wrote:
> On Thu, Aug 26, 2021 at 11:51:44AM +0200, Thomas Hellström wrote:
> > On Thu, 2021-08-26 at 11:16 +0200, Daniel Vetter wrote:
> > > On Thu, Aug 19, 2021 at 09:32:20AM +0200, Thomas Hellström wrote:
> > > > On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> > > > > This should give a more complete view of the various bits of
> > > > > internal
> > > > > resource manager state, for device local-memory.
> > > > > 
> > > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
> > > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > index eec0d349ea6a..109e6feed6be 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m,
> > > > > struct drm_i915_gem_object *obj)
> > > > >  static int i915_gem_object_info(struct seq_file *m, void *data)
> > > > >  {
> > > > >         struct drm_i915_private *i915 = node_to_i915(m->private);
> > > > > +       struct drm_printer p = drm_seq_file_printer(m);
> > > > >         struct intel_memory_region *mr;
> > > > >         enum intel_region_id id;
> > > > >  
> > > > > @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct
> > > > > seq_file
> > > > > *m, void *data)
> > > > >                    i915->mm.shrink_count,
> > > > >                    atomic_read(&i915->mm.free_count),
> > > > >                    i915->mm.shrink_memory);
> > > > > -       for_each_memory_region(mr, i915, id)
> > > > > -               seq_printf(m, "%s: total:%pa, available:%pa
> > > > > bytes\n",
> > > > > -                          mr->name, &mr->total, &mr->avail);
> > > > > +       for_each_memory_region(mr, i915, id) {
> > > > > +               seq_printf(m, "%s: ", mr->name);
> > > > > +               if (mr->region_private)
> > > > > +                       ttm_resource_manager_debug(mr-
> > > > > > region_private, &p);
> > > > > +               else
> > > > > +                       seq_printf(m, "total:%pa, available:%pa
> > > > > bytes\n",
> > > > > +                                  &mr->total, &mr->avail);
> > > > 
> > > > Hm. Shouldn't we make the above intel_memory_region_debug() or
> > > > perhaps
> > > > intel_memory_region_info() to avoid using memory region internals
> > > > directly here?
> > > 
> > > Imo we should just emebed ttm_resource_mager into our own and not try
> > > to
> > > abstract this all away that much. At least in upstream there is just
> > > not
> > > going to be another memory region implementation, and for backporting
> > > I'm
> > > not sure these abstractions really help that much - we're touching
> > > all the
> > > same code still in the end.
> > 
> > Hmm, yes. Here I was seeing the separation between the debugfs code and
> > the intel_memory_region code, rather between the latter and TTM.
> > 
> > The i915 driver is currently much "everything uses everything" which
> > IMHO is not really good for code understanding and maintainance.
> 
> Ah yes I agree, we don't have clear seperation of concerns really, and
> debugfs is all over. I got confused a bit with the ->region_private
> pointer and thought you'd be talking about that.
> 
> My experience has been that going over the interface functions and trying
> to kerneldoc helps a lot with this, because instead of documenting some
> major confusion you can just clean it up first. We should definitely try
> to componentize stuff more and not leak internal details all over the
> place.

While we discuss this: For debugfs functions I recommend using drm_printer
and not seq_file directly, it's a nice bit of abstraction so that you can
also dump to debugfs. Or anything else really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug

[Daniel Vetter]

On Thu, Aug 26, 2021 at 12:03:29PM +0200, Daniel Vetter wrote:
> On Thu, Aug 26, 2021 at 11:51:44AM +0200, Thomas Hellström wrote:
> > On Thu, 2021-08-26 at 11:16 +0200, Daniel Vetter wrote:
> > > On Thu, Aug 19, 2021 at 09:32:20AM +0200, Thomas Hellström wrote:
> > > > On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> > > > > This should give a more complete view of the various bits of
> > > > > internal
> > > > > resource manager state, for device local-memory.
> > > > > 
> > > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > > > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++++---
> > > > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > index eec0d349ea6a..109e6feed6be 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > @@ -238,6 +238,7 @@ i915_debugfs_describe_obj(struct seq_file *m,
> > > > > struct drm_i915_gem_object *obj)
> > > > >  static int i915_gem_object_info(struct seq_file *m, void *data)
> > > > >  {
> > > > >         struct drm_i915_private *i915 = node_to_i915(m->private);
> > > > > +       struct drm_printer p = drm_seq_file_printer(m);
> > > > >         struct intel_memory_region *mr;
> > > > >         enum intel_region_id id;
> > > > >  
> > > > > @@ -245,9 +246,14 @@ static int i915_gem_object_info(struct
> > > > > seq_file
> > > > > *m, void *data)
> > > > >                    i915->mm.shrink_count,
> > > > >                    atomic_read(&i915->mm.free_count),
> > > > >                    i915->mm.shrink_memory);
> > > > > -       for_each_memory_region(mr, i915, id)
> > > > > -               seq_printf(m, "%s: total:%pa, available:%pa
> > > > > bytes\n",
> > > > > -                          mr->name, &mr->total, &mr->avail);
> > > > > +       for_each_memory_region(mr, i915, id) {
> > > > > +               seq_printf(m, "%s: ", mr->name);
> > > > > +               if (mr->region_private)
> > > > > +                       ttm_resource_manager_debug(mr-
> > > > > > region_private, &p);
> > > > > +               else
> > > > > +                       seq_printf(m, "total:%pa, available:%pa
> > > > > bytes\n",
> > > > > +                                  &mr->total, &mr->avail);
> > > > 
> > > > Hm. Shouldn't we make the above intel_memory_region_debug() or
> > > > perhaps
> > > > intel_memory_region_info() to avoid using memory region internals
> > > > directly here?
> > > 
> > > Imo we should just emebed ttm_resource_mager into our own and not try
> > > to
> > > abstract this all away that much. At least in upstream there is just
> > > not
> > > going to be another memory region implementation, and for backporting
> > > I'm
> > > not sure these abstractions really help that much - we're touching
> > > all the
> > > same code still in the end.
> > 
> > Hmm, yes. Here I was seeing the separation between the debugfs code and
> > the intel_memory_region code, rather between the latter and TTM.
> > 
> > The i915 driver is currently much "everything uses everything" which
> > IMHO is not really good for code understanding and maintainance.
> 
> Ah yes I agree, we don't have clear seperation of concerns really, and
> debugfs is all over. I got confused a bit with the ->region_private
> pointer and thought you'd be talking about that.
> 
> My experience has been that going over the interface functions and trying
> to kerneldoc helps a lot with this, because instead of documenting some
> major confusion you can just clean it up first. We should definitely try
> to componentize stuff more and not leak internal details all over the
> place.

While we discuss this: For debugfs functions I recommend using drm_printer
and not seq_file directly, it's a nice bit of abstraction so that you can
also dump to debugfs. Or anything else really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/2] drm/i915/buddy: add some pretty printing

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 6237 bytes --]

== Series Details ==

Series: series starting with [1/2] drm/i915/buddy: add some pretty printing
URL   : https://patchwork.freedesktop.org/series/93790/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_10496 -> Patchwork_20846
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_20846 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_20846, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_20846:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_pm_rpm@module-reload:
    - fi-tgl-1115g4:      NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-tgl-1115g4/igt@i915_pm_rpm@module-reload.html

  * igt@kms_addfb_basic@too-wide:
    - fi-tgl-1115g4:      NOTRUN -> [DMESG-WARN][2] +91 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-tgl-1115g4/igt@kms_addfb_basic@too-wide.html

  
Known issues
------------

  Here are the changes found in Patchwork_20846 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][3] ([i915#2190])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-tgl-1115g4/igt@gem_huc_copy@huc-copy.html

  * igt@i915_pm_backlight@basic-brightness:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][4] ([i915#1155])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-tgl-1115g4/igt@i915_pm_backlight@basic-brightness.html

  * igt@i915_selftest@live@execlists:
    - fi-bsw-nick:        [PASS][5] -> [INCOMPLETE][6] ([i915#2940])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10496/fi-bsw-nick/igt@i915_selftest@live@execlists.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-bsw-nick/igt@i915_selftest@live@execlists.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][7] ([fdo#111827]) +8 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-tgl-1115g4/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       [PASS][8] -> [FAIL][9] ([i915#1372])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10496/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-icl-u2:          [PASS][10] -> [DMESG-WARN][11] ([i915#2868])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10496/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][12] ([fdo#109285])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-tgl-1115g4/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_psr@primary_mmap_gtt:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][13] ([i915#1072]) +3 similar issues
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-tgl-1115g4/igt@kms_psr@primary_mmap_gtt.html

  * igt@prime_vgem@basic-userptr:
    - fi-tgl-1115g4:      NOTRUN -> [SKIP][14] ([i915#3301])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-tgl-1115g4/igt@prime_vgem@basic-userptr.html

  * igt@runner@aborted:
    - fi-tgl-1115g4:      NOTRUN -> [FAIL][15] ([i915#2722] / [i915#3834])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-tgl-1115g4/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_lrc:
    - fi-rkl-guc:         [DMESG-WARN][16] ([i915#3958]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_10496/fi-rkl-guc/igt@i915_selftest@live@gt_lrc.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/fi-rkl-guc/igt@i915_selftest@live@gt_lrc.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
  [i915#1372]: https://gitlab.freedesktop.org/drm/intel/issues/1372
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2722]: https://gitlab.freedesktop.org/drm/intel/issues/2722
  [i915#2868]: https://gitlab.freedesktop.org/drm/intel/issues/2868
  [i915#2940]: https://gitlab.freedesktop.org/drm/intel/issues/2940
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3303]: https://gitlab.freedesktop.org/drm/intel/issues/3303
  [i915#3834]: https://gitlab.freedesktop.org/drm/intel/issues/3834
  [i915#3958]: https://gitlab.freedesktop.org/drm/intel/issues/3958


Participating hosts (34 -> 34)
------------------------------

  Additional (1): fi-tgl-1115g4 
  Missing    (1): fi-bsw-cyan 


Build changes
-------------

  * Linux: CI_DRM_10496 -> Patchwork_20846

  CI-20190529: 20190529
  CI_DRM_10496: e4f0d7fe9eeff06828925fc08a58a0084a205e50 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6178: 146260200f9a6d4536e48a195e2ab49a07d4f0c1 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_20846: 0994869bcc7bd0c1c73588db397a0699a261f2c2 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

0994869bcc7b drm/i915/debugfs: hook up ttm_resource_manager_debug
f378ff74fe10 drm/i915/buddy: add some pretty printing

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_20846/index.html

[-- Attachment #2: Type: text/html, Size: 7131 bytes --]

Re: [PATCH 1/2] drm/i915/buddy: add some pretty printing

[Thomas Hellström]

On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> Implement the debug hook for the buddy resource manager. For this we
> want to print out the status of the memory manager, including how much
> memory is still allocatable, what page sizes we have etc. This will be
> triggered when TTM is unable to fulfil an allocation request for device
> local-memory.
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_buddy.c             | 45 +++++++++++++++++++
>  drivers/gpu/drm/i915/i915_buddy.h             |  8 ++++
>  drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 20 ++++++++-
>  3 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_buddy.c
> b/drivers/gpu/drm/i915/i915_buddy.c
> index 7b274c51cac0..240e881d9eb0 100644
> --- a/drivers/gpu/drm/i915/i915_buddy.c
> +++ b/drivers/gpu/drm/i915/i915_buddy.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/kmemleak.h>
> +#include <linux/sizes.h>
>  
>  #include "i915_buddy.h"
>  
> @@ -82,6 +83,7 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64
> size, u64 chunk_size)
>         size = round_down(size, chunk_size);
>  
>         mm->size = size;
> +       mm->avail = size;
>         mm->chunk_size = chunk_size;
>         mm->max_order = ilog2(size) - ilog2(chunk_size);
>  
> @@ -155,6 +157,8 @@ void i915_buddy_fini(struct i915_buddy_mm *mm)
>                 i915_block_free(mm, mm->roots[i]);
>         }
>  
> +       GEM_WARN_ON(mm->avail != mm->size);
> +
>         kfree(mm->roots);
>         kfree(mm->free_list);
>  }
> @@ -230,6 +234,7 @@ void i915_buddy_free(struct i915_buddy_mm *mm,
>                      struct i915_buddy_block *block)
>  {
>         GEM_BUG_ON(!i915_buddy_block_is_allocated(block));
> +       mm->avail += i915_buddy_block_size(mm, block);
>         __i915_buddy_free(mm, block);
>  }
>  
> @@ -283,6 +288,7 @@ i915_buddy_alloc(struct i915_buddy_mm *mm, unsigned
> int order)
>         }
>  
>         mark_allocated(block);
> +       mm->avail -= i915_buddy_block_size(mm, block);
>         kmemleak_update_trace(block);
>         return block;
>  
> @@ -368,6 +374,7 @@ int i915_buddy_alloc_range(struct i915_buddy_mm
> *mm,
>                         }
>  
>                         mark_allocated(block);
> +                       mm->avail -= i915_buddy_block_size(mm, block);
>                         list_add_tail(&block->link, &allocated);
>                         continue;
>                 }
> @@ -402,6 +409,44 @@ int i915_buddy_alloc_range(struct i915_buddy_mm
> *mm,
>         return err;
>  }
>  
> +void i915_buddy_block_print(struct i915_buddy_mm *mm,
> +                           struct i915_buddy_block *block,
> +                           struct drm_printer *p)
> +{
> +       u64 start = i915_buddy_block_offset(block);
> +       u64 size = i915_buddy_block_size(mm, block);
> +
> +       drm_printf(p, "%#018llx-%#018llx: %llu\n", start, start + size,
> size);
> +}
> +
> +void i915_buddy_print(struct i915_buddy_mm *mm, struct drm_printer *p)
> +{
> +       int order;
> +
> +       drm_printf(p, "chunk_size: %lluKB, total: %lluMB, free:
> %lluMB\n",
> +                  mm->chunk_size >> 10, mm->size >> 20, mm->avail >>
> 20);
> +
> +       for (order = mm->max_order; order >= 0; order--) {
> +               struct i915_buddy_block *block;
> +               u64 count = 0, free;
> +
> +               list_for_each_entry(block, &mm->free_list[order], link)
> {
> +                       GEM_BUG_ON(!i915_buddy_block_is_free(block));
> +                       count++;
> +               }
> +
> +               drm_printf(p, "order-%d ", order);
> +
> +               free = count * (mm->chunk_size << order);
> +               if (free < SZ_1M)
> +                       drm_printf(p, "free: %lluKB", free >> 10);
> +               else
> +                       drm_printf(p, "free: %lluMB", free >> 20);

Use KiB and MiB instead of KB and MB? Also below.


> +
> +               drm_printf(p, ", pages: %llu\n", count);
> +       }
> +}
> +
>  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>  #include "selftests/i915_buddy.c"
>  #endif
> diff --git a/drivers/gpu/drm/i915/i915_buddy.h
> b/drivers/gpu/drm/i915/i915_buddy.h
> index 3940d632f208..7077742112ac 100644
> --- a/drivers/gpu/drm/i915/i915_buddy.h
> +++ b/drivers/gpu/drm/i915/i915_buddy.h
> @@ -10,6 +10,8 @@
>  #include <linux/list.h>
>  #include <linux/slab.h>
>  
> +#include <drm/drm_print.h>
> +
>  struct i915_buddy_block {
>  #define I915_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12)
>  #define I915_BUDDY_HEADER_STATE  GENMASK_ULL(11, 10)
> @@ -69,6 +71,7 @@ struct i915_buddy_mm {
>         /* Must be at least PAGE_SIZE */
>         u64 chunk_size;
>         u64 size;
> +       u64 avail;
>  };
>  
>  static inline u64
> @@ -129,6 +132,11 @@ void i915_buddy_free(struct i915_buddy_mm *mm,
> struct i915_buddy_block *block);
>  
>  void i915_buddy_free_list(struct i915_buddy_mm *mm, struct list_head
> *objects);
>  
> +void i915_buddy_print(struct i915_buddy_mm *mm, struct drm_printer
> *p);
> +void i915_buddy_block_print(struct i915_buddy_mm *mm,
> +                           struct i915_buddy_block *block,
> +                           struct drm_printer *p);
> +
>  void i915_buddy_module_exit(void);
>  int i915_buddy_module_init(void);
>  
> diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> index 6877362f6b85..95ab786a1fe4 100644
> --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> @@ -126,12 +126,30 @@ static void i915_ttm_buddy_man_free(struct
> ttm_resource_manager *man,
>         kfree(bman_res);
>  }
>  
> +static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
> +                                    struct drm_printer *printer)
> +{
> +       struct i915_ttm_buddy_manager *bman = to_buddy_manager(man);
> +       struct i915_buddy_block *block;
> +
> +       mutex_lock(&bman->lock);
> +       drm_printf(printer, "default_page_size: %lluKB\n",
> +                  bman->default_page_size >> 10);
> +
> +       i915_buddy_print(&bman->mm, printer);
> +
> +       drm_printf(printer, "reserved:\n");
> +       list_for_each_entry(block, &bman->reserved, link)
> +               i915_buddy_block_print(&bman->mm, block, printer);
> +       mutex_unlock(&bman->lock);
> +}
> +
>  static const struct ttm_resource_manager_func
> i915_ttm_buddy_manager_func = {
>         .alloc = i915_ttm_buddy_man_alloc,
>         .free = i915_ttm_buddy_man_free,
> +       .debug = i915_ttm_buddy_man_debug,
>  };
>  
> -
>  /**
>   * i915_ttm_buddy_man_init - Setup buddy allocator based ttm manager
>   * @bdev: The ttm device

Otherwise LGTM,
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Re: [Intel-gfx] [PATCH 1/2] drm/i915/buddy: add some pretty printing

[Thomas Hellström]

On Wed, 2021-08-18 at 15:58 +0100, Matthew Auld wrote:
> Implement the debug hook for the buddy resource manager. For this we
> want to print out the status of the memory manager, including how much
> memory is still allocatable, what page sizes we have etc. This will be
> triggered when TTM is unable to fulfil an allocation request for device
> local-memory.
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_buddy.c             | 45 +++++++++++++++++++
>  drivers/gpu/drm/i915/i915_buddy.h             |  8 ++++
>  drivers/gpu/drm/i915/i915_ttm_buddy_manager.c | 20 ++++++++-
>  3 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_buddy.c
> b/drivers/gpu/drm/i915/i915_buddy.c
> index 7b274c51cac0..240e881d9eb0 100644
> --- a/drivers/gpu/drm/i915/i915_buddy.c
> +++ b/drivers/gpu/drm/i915/i915_buddy.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/kmemleak.h>
> +#include <linux/sizes.h>
>  
>  #include "i915_buddy.h"
>  
> @@ -82,6 +83,7 @@ int i915_buddy_init(struct i915_buddy_mm *mm, u64
> size, u64 chunk_size)
>         size = round_down(size, chunk_size);
>  
>         mm->size = size;
> +       mm->avail = size;
>         mm->chunk_size = chunk_size;
>         mm->max_order = ilog2(size) - ilog2(chunk_size);
>  
> @@ -155,6 +157,8 @@ void i915_buddy_fini(struct i915_buddy_mm *mm)
>                 i915_block_free(mm, mm->roots[i]);
>         }
>  
> +       GEM_WARN_ON(mm->avail != mm->size);
> +
>         kfree(mm->roots);
>         kfree(mm->free_list);
>  }
> @@ -230,6 +234,7 @@ void i915_buddy_free(struct i915_buddy_mm *mm,
>                      struct i915_buddy_block *block)
>  {
>         GEM_BUG_ON(!i915_buddy_block_is_allocated(block));
> +       mm->avail += i915_buddy_block_size(mm, block);
>         __i915_buddy_free(mm, block);
>  }
>  
> @@ -283,6 +288,7 @@ i915_buddy_alloc(struct i915_buddy_mm *mm, unsigned
> int order)
>         }
>  
>         mark_allocated(block);
> +       mm->avail -= i915_buddy_block_size(mm, block);
>         kmemleak_update_trace(block);
>         return block;
>  
> @@ -368,6 +374,7 @@ int i915_buddy_alloc_range(struct i915_buddy_mm
> *mm,
>                         }
>  
>                         mark_allocated(block);
> +                       mm->avail -= i915_buddy_block_size(mm, block);
>                         list_add_tail(&block->link, &allocated);
>                         continue;
>                 }
> @@ -402,6 +409,44 @@ int i915_buddy_alloc_range(struct i915_buddy_mm
> *mm,
>         return err;
>  }
>  
> +void i915_buddy_block_print(struct i915_buddy_mm *mm,
> +                           struct i915_buddy_block *block,
> +                           struct drm_printer *p)
> +{
> +       u64 start = i915_buddy_block_offset(block);
> +       u64 size = i915_buddy_block_size(mm, block);
> +
> +       drm_printf(p, "%#018llx-%#018llx: %llu\n", start, start + size,
> size);
> +}
> +
> +void i915_buddy_print(struct i915_buddy_mm *mm, struct drm_printer *p)
> +{
> +       int order;
> +
> +       drm_printf(p, "chunk_size: %lluKB, total: %lluMB, free:
> %lluMB\n",
> +                  mm->chunk_size >> 10, mm->size >> 20, mm->avail >>
> 20);
> +
> +       for (order = mm->max_order; order >= 0; order--) {
> +               struct i915_buddy_block *block;
> +               u64 count = 0, free;
> +
> +               list_for_each_entry(block, &mm->free_list[order], link)
> {
> +                       GEM_BUG_ON(!i915_buddy_block_is_free(block));
> +                       count++;
> +               }
> +
> +               drm_printf(p, "order-%d ", order);
> +
> +               free = count * (mm->chunk_size << order);
> +               if (free < SZ_1M)
> +                       drm_printf(p, "free: %lluKB", free >> 10);
> +               else
> +                       drm_printf(p, "free: %lluMB", free >> 20);

Use KiB and MiB instead of KB and MB? Also below.


> +
> +               drm_printf(p, ", pages: %llu\n", count);
> +       }
> +}
> +
>  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>  #include "selftests/i915_buddy.c"
>  #endif
> diff --git a/drivers/gpu/drm/i915/i915_buddy.h
> b/drivers/gpu/drm/i915/i915_buddy.h
> index 3940d632f208..7077742112ac 100644
> --- a/drivers/gpu/drm/i915/i915_buddy.h
> +++ b/drivers/gpu/drm/i915/i915_buddy.h
> @@ -10,6 +10,8 @@
>  #include <linux/list.h>
>  #include <linux/slab.h>
>  
> +#include <drm/drm_print.h>
> +
>  struct i915_buddy_block {
>  #define I915_BUDDY_HEADER_OFFSET GENMASK_ULL(63, 12)
>  #define I915_BUDDY_HEADER_STATE  GENMASK_ULL(11, 10)
> @@ -69,6 +71,7 @@ struct i915_buddy_mm {
>         /* Must be at least PAGE_SIZE */
>         u64 chunk_size;
>         u64 size;
> +       u64 avail;
>  };
>  
>  static inline u64
> @@ -129,6 +132,11 @@ void i915_buddy_free(struct i915_buddy_mm *mm,
> struct i915_buddy_block *block);
>  
>  void i915_buddy_free_list(struct i915_buddy_mm *mm, struct list_head
> *objects);
>  
> +void i915_buddy_print(struct i915_buddy_mm *mm, struct drm_printer
> *p);
> +void i915_buddy_block_print(struct i915_buddy_mm *mm,
> +                           struct i915_buddy_block *block,
> +                           struct drm_printer *p);
> +
>  void i915_buddy_module_exit(void);
>  int i915_buddy_module_init(void);
>  
> diff --git a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> index 6877362f6b85..95ab786a1fe4 100644
> --- a/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> +++ b/drivers/gpu/drm/i915/i915_ttm_buddy_manager.c
> @@ -126,12 +126,30 @@ static void i915_ttm_buddy_man_free(struct
> ttm_resource_manager *man,
>         kfree(bman_res);
>  }
>  
> +static void i915_ttm_buddy_man_debug(struct ttm_resource_manager *man,
> +                                    struct drm_printer *printer)
> +{
> +       struct i915_ttm_buddy_manager *bman = to_buddy_manager(man);
> +       struct i915_buddy_block *block;
> +
> +       mutex_lock(&bman->lock);
> +       drm_printf(printer, "default_page_size: %lluKB\n",
> +                  bman->default_page_size >> 10);
> +
> +       i915_buddy_print(&bman->mm, printer);
> +
> +       drm_printf(printer, "reserved:\n");
> +       list_for_each_entry(block, &bman->reserved, link)
> +               i915_buddy_block_print(&bman->mm, block, printer);
> +       mutex_unlock(&bman->lock);
> +}
> +
>  static const struct ttm_resource_manager_func
> i915_ttm_buddy_manager_func = {
>         .alloc = i915_ttm_buddy_man_alloc,
>         .free = i915_ttm_buddy_man_free,
> +       .debug = i915_ttm_buddy_man_debug,
>  };
>  
> -
>  /**
>   * i915_ttm_buddy_man_init - Setup buddy allocator based ttm manager
>   * @bdev: The ttm device

Otherwise LGTM,
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>