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 15:38:18 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> On 08.04.2025 15:47, Boris Brezillon wrote:
> 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?  
> 
> Yes, inlining panthor_gem_debugfs_bo_init() into panthor_gem_debugfs_bo_add() and moving
> panthor_gem_debugfs_bo_add() into panthor_gem_create_object().
> 
> > > > > +	} 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.  
> 
> I think what I'll do is set the mask to a poison value, maybe 0xFF, and only when
> it's overwritten with a legitimate value, display the object in the DebugFS GEMS file.

Well, it's just as simple to leave it to zero at bo_add() time, and
have an INITIALIZED flag that you set along the other flags in the
caller of gem_shmem_create(). If you make it a poison value, you'll
need to make sure it never conflicts with a valid flag combination,
which might be annoying.