On Thu, 27 Mar 2025 13:18:47 +0000
Adrián Larumbe <adrian.laru...@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.laru...@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.laru...@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.
