Re: [PATCH v2 4/4] drm/panthor: Display heap chunk entries in DebugFS GEMS file

[Boris Brezillon <boris.brezillon@collabora.com>]

On Thu, 27 Mar 2025 13:18:47 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> On 19.03.2025 18:00, Boris Brezillon wrote:
> > On Wed, 19 Mar 2025 15:03:19 +0000
> > Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> >  
> > > Expand the driver's DebugFS GEMS file to display entries for the heap
> > > chunks' GEM objects, both those allocated at heap creation time through an
> > > ioctl(), or in response to a tiler OOM event.
> > >
> > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > > ---
> > >  drivers/gpu/drm/panthor/panthor_gem.c  | 22 +++++++++++-----------
> > >  drivers/gpu/drm/panthor/panthor_gem.h  |  2 ++
> > >  drivers/gpu/drm/panthor/panthor_heap.c |  3 +++
> > >  3 files changed, 16 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> > > index f7eb413d88e7..252979473fdf 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_gem.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> > > @@ -22,16 +22,6 @@ static void panthor_gem_debugfs_bo_init(struct panthor_gem_object *bo)
> > >  	get_task_comm(bo->gems.creator.process_name, current->group_leader);
> > >  }
> > >
> > > -static void panthor_gem_debugfs_bo_add(struct panthor_gem_object *bo)
> > > -{
> > > -	struct panthor_device *ptdev =  container_of(bo->base.base.dev,
> > > -						     struct panthor_device, base);
> > > -
> > > -	mutex_lock(&ptdev->gems.lock);
> > > -	list_add_tail(&bo->gems.node, &ptdev->gems.node);
> > > -	mutex_unlock(&ptdev->gems.lock);
> > > -}
> > > -
> > >  static void panthor_gem_debugfs_bo_rm(struct panthor_gem_object *bo)
> > >  {
> > >  	struct panthor_device *ptdev = container_of(bo->base.base.dev,
> > > @@ -44,10 +34,20 @@ static void panthor_gem_debugfs_bo_rm(struct panthor_gem_object *bo)
> > >  	list_del_init(&bo->gems.node);
> > >  	mutex_unlock(&ptdev->gems.lock);
> > >  }
> > > +
> > > +void panthor_gem_debugfs_bo_add(struct panthor_gem_object *bo)
> > > +{
> > > +	struct panthor_device *ptdev =  container_of(bo->base.base.dev,
> > > +						     struct panthor_device, base);
> > > +
> > > +	mutex_lock(&ptdev->gems.lock);
> > > +	list_add_tail(&bo->gems.node, &ptdev->gems.node);
> > > +	mutex_unlock(&ptdev->gems.lock);
> > > +}
> > >  #else
> > >  static void panthor_gem_debugfs_bo_init(struct panthor_gem_object *bo) {}
> > > -static void panthor_gem_debugfs_bo_add(struct panthor_gem_object *bo) {}
> > >  static void panthor_gem_debugfs_bo_rm(struct panthor_gem_object *bo) {}
> > > +void panthor_gem_debugfs_bo_add(struct panthor_gem_object *bo) {}  
> >
> > Let's just define all these helpers as inline functions in
> > panthor_gem.h in patch 3.  
> 
> The only function that can so far be used from a different compilation unit is 'add'.
> The other two are internal to panthor_gem.c, so I'm incluned to keep them there as static
> functions, but move 'add' into the header file as a static inline function instead.

I'd really prefer if those were defined next to each other, and given
how short/simple they are, I don't see a good reason to hide them.

> 
> > >  #endif
> > >
> > >  static void panthor_gem_free_object(struct drm_gem_object *obj)
> > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h
> > > index 7c896ec35801..e132f14bbef8 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_gem.h
> > > +++ b/drivers/gpu/drm/panthor/panthor_gem.h
> > > @@ -132,6 +132,8 @@ panthor_gem_create_with_handle(struct drm_file *file,
> > >  void panthor_gem_bo_set_label(struct drm_gem_object *obj, const char *label);
> > >  void panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label);
> > >
> > > +void panthor_gem_debugfs_bo_add(struct panthor_gem_object *bo);
> > > +
> > >  static inline u64
> > >  panthor_kernel_bo_gpuva(struct panthor_kernel_bo *bo)
> > >  {
> > > diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> > > index db0285ce5812..73cf43eb4a7b 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_heap.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> > > @@ -180,6 +180,9 @@ static int panthor_alloc_heap_chunk(struct panthor_device *ptdev,
> > >  	heap->chunk_count++;
> > >  	mutex_unlock(&heap->lock);
> > >
> > > +	panthor_gem_kernel_bo_set_label(chunk->bo, "\"Tiler heap chunk\"");
> > > +	panthor_gem_debugfs_bo_add(to_panthor_bo(chunk->bo->obj));  
> >
> > I'd be tempted to label all the kernel BOs, not just the heap chunks,
> > and if we do that, we're probably better off changing the
> > kernel_bo_create() prototype to pass a label.  
> 
> I think is a good idea going forward, but in keeping things tight I'd say doing it now
> might be an overkill, since the only user of tagged BO's at the moment is the gem debugfs
> knob.
> 
> I think if in the future we find new ways of accounting or displaying labelled kernel BO's
> other than heap chunks, then we can expand the kernel_bo_create() argument list and then
> tag every single one of them at creation time.

Yeah, actually I'm questioning the fact we don't register all BOs to
the debugfs list now. If it's going to be a debugfs interface, I don't
really see a good reason for hiding some but exposing others.
important to show.