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

[Daniel Vetter <daniel@ffwll.ch>]

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