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

[Daniel Vetter <daniel@ffwll.ch>]

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