On Tue, 8 Apr 2025 15:38:18 +0100
Adrián Larumbe <adrian.laru...@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.laru...@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 <r...@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.
