Re: [PATCH v4 4/4] drm/panthor: show device-wide list of DRM GEM objects over DebugFS

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

On Tue, 8 Apr 2025 14:38:44 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
> > > index 44d027e6d664..2fc87be9b700 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_gem.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_gem.c
> > > @@ -2,6 +2,7 @@
> > >  /* Copyright 2019 Linaro, Ltd, Rob Herring <robh@kernel.org> */
> > >  /* Copyright 2023 Collabora ltd. */
> > >
> > > +#include <linux/cleanup.h>
> > >  #include <linux/dma-buf.h>
> > >  #include <linux/dma-mapping.h>
> > >  #include <linux/err.h>
> > > @@ -10,14 +11,65 @@
> > >  #include <drm/panthor_drm.h>
> > >
> > >  #include "panthor_device.h"
> > > +#include "panthor_fw.h"
> > >  #include "panthor_gem.h"
> > >  #include "panthor_mmu.h"
> > >
> > > +#ifdef CONFIG_DEBUG_FS
> > > +static void panthor_gem_debugfs_bo_init(struct panthor_gem_object *bo, u32 type_mask)
> > > +{
> > > +	INIT_LIST_HEAD(&bo->debugfs.node);  
> >
> > This should be called when the GEM object is created, otherwise the
> > list_empty() test done in panthor_gem_debugfs_bo_rm() will only work if
> > panthor_gem_debugfs_bo_add() is called, and depending on when this
> > happens, or whether it happens at all, the error path will do a NULL
> > deref.  
> 
> I'll be moving panthor_gem_debugfs_bo_add() back into panthor_gem_create_object() and
> inline panthor_gem_debugfs_bo_init() into it.

You mean moving the panthor_gem_debugfs_bo_add() call to
panthor_gem_create_object(), not inlining its content, right?

> > > +	} else {
> > > +		bo->debugfs.creator.tgid = 0;
> > > +		snprintf(bo->debugfs.creator.process_name,
> > > +			 sizeof(bo->debugfs.creator.process_name),
> > > +			 "kernel");
> > > +	}
> > > +
> > > +	bo->debugfs.bo_mask = type_mask;  
> >
> > Why not do that directly in panthor_gem_debugfs_bo_add()? The only bits
> > that might be useful to do early is the INIT_LIST_HEAD(), and I think
> > it can be inlined in panthor_gem_create_object().  
> 
> I'll be doing in this in the next revision, but because I've no access to the BO
> type mask from inside Panthor's drm_driver::gem_create_object() binding, then
> I'll have to assign the mask right after the object has been created.
> 
> I think this means there might be a short window after the object's been added to
> the DebugFS GEMs list in which it could be shown with the kernel mask field still
> set to 0, but I guess that's not too important either.

I think it's okay, as long as you don't crash when printing partially
initialized objects. Another solution would be to have a flag encoding
when the obj is initialized, so you can skip objects that don't have
this flag set yet.