Re: [Intel-gfx] [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug
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
> > > > > Cc: Thomas Hellström
> > > > > ---
> > > > > 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
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
> > > > Cc: Thomas Hellström
> > > > ---
> > > > 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
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
> > > > Cc: Thomas Hellström
> > > > ---
> > > > 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
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
> > > Cc: Thomas Hellström
> > > ---
> > > 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
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
> > Cc: Thomas Hellström
> > ---
> > 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
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
> Cc: Thomas Hellström
> ---
> 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
[Intel-gfx] [PATCH 2/2] drm/i915/debugfs: hook up ttm_resource_manager_debug
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
Cc: Thomas Hellström
---
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
