On 10/07/2025 09:53, Simona Vetter wrote:
> On Wed, Jul 09, 2025 at 04:48:21PM +0100, Steven Price wrote:
>> On 09/07/2025 14:52, Simona Vetter wrote:
>>> The object is potentially already gone after the drm_gem_object_put().
>>> In general the object should be fully constructed before calling
>>> drm_gem_handle_create(), except the debugfs tracking uses a separate
>>> lock and list and separate flag to denotate whether the object is
>>> actually initilized.
^^^^^^^^^^
Another nit (although checkpatch takes the credit for noticing): it
should have an 'a': initialized :p
I've fixed this and pushed to drm-misc-next.
Thanks,
Steve
>>>
>>> Since I'm touching this all anyway simplify this by only adding the
>>> object to the debugfs when it's ready for that, which allows us to
>>> delete that separate flag. panthor_gem_debugfs_bo_rm() already checks
>>> whether we've actually been added to the list or this is some error
>>> path cleanup.
>>>
>>> v2: Fix build issues for !CONFIG_DEBUGFS (Adrián)
>>>
>>> v3: Add linebreak and remove outdated comment (Liviu)
>>>
>>> Fixes: a3707f53eb3f ("drm/panthor: show device-wide list of DRM GEM objects
>>> over DebugFS")
>>> Cc: Adrián Larumbe <adrian.laru...@collabora.com>
>>> Cc: Boris Brezillon <boris.brezil...@collabora.com>
>>> Cc: Steven Price <steven.pr...@arm.com>
>>> Cc: Liviu Dudau <liviu.du...@arm.com>
>>> Reviewed-by: Liviu Dudau <liviu.du...@arm.com>
>>> Signed-off-by: Simona Vetter <simona.vet...@intel.com>
>>> Signed-off-by: Simona Vetter <simona.vet...@ffwll.ch>
>>
>> Reviewed-by: Steven Price <steven.pr...@arm.com>
>
> Thanks for the review!
>
>> Although a nit on the email subject - you're missing the "v3" tag ;)
>
> Yeah I often forget to set that, oops :-P
>
> Will you or someone from the panthor team land this, or should I push it
> to drm-misc-next myself?
> -Sima
>
>>
>> Steve
>>
>>> ---
>>> drivers/gpu/drm/panthor/panthor_gem.c | 31 +++++++++++++--------------
>>> drivers/gpu/drm/panthor/panthor_gem.h | 3 ---
>>> 2 files changed, 15 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_gem.c
>>> b/drivers/gpu/drm/panthor/panthor_gem.c
>>> index 7c00fd77758b..a123bc740ba1 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_gem.c
>>> +++ b/drivers/gpu/drm/panthor/panthor_gem.c
>>> @@ -16,10 +16,15 @@
>>> #include "panthor_mmu.h"
>>>
>>> #ifdef CONFIG_DEBUG_FS
>>> -static void panthor_gem_debugfs_bo_add(struct panthor_device *ptdev,
>>> - struct panthor_gem_object *bo)
>>> +static void panthor_gem_debugfs_bo_init(struct panthor_gem_object *bo)
>>> {
>>> INIT_LIST_HEAD(&bo->debugfs.node);
>>> +}
>>> +
>>> +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);
>>>
>>> bo->debugfs.creator.tgid = current->group_leader->pid;
>>> get_task_comm(bo->debugfs.creator.process_name, current->group_leader);
>>> @@ -44,14 +49,13 @@ static void panthor_gem_debugfs_bo_rm(struct
>>> panthor_gem_object *bo)
>>>
>>> static void panthor_gem_debugfs_set_usage_flags(struct panthor_gem_object
>>> *bo, u32 usage_flags)
>>> {
>>> - bo->debugfs.flags = usage_flags |
>>> PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED;
>>> + bo->debugfs.flags = usage_flags;
>>> + panthor_gem_debugfs_bo_add(bo);
>>> }
>>> #else
>>> -static void panthor_gem_debugfs_bo_add(struct panthor_device *ptdev,
>>> - struct panthor_gem_object *bo)
>>> -{}
>>> static void panthor_gem_debugfs_bo_rm(struct panthor_gem_object *bo) {}
>>> static void panthor_gem_debugfs_set_usage_flags(struct panthor_gem_object
>>> *bo, u32 usage_flags) {}
>>> +static void panthor_gem_debugfs_bo_init(struct panthor_gem_object *bo) {}
>>> #endif
>>>
>>> static void panthor_gem_free_object(struct drm_gem_object *obj)
>>> @@ -246,7 +250,7 @@ struct drm_gem_object *panthor_gem_create_object(struct
>>> drm_device *ddev, size_t
>>> drm_gem_gpuva_set_lock(&obj->base.base, &obj->gpuva_list_lock);
>>> mutex_init(&obj->label.lock);
>>>
>>> - panthor_gem_debugfs_bo_add(ptdev, obj);
>>> + panthor_gem_debugfs_bo_init(obj);
>>>
>>> return &obj->base.base;
>>> }
>>> @@ -285,6 +289,8 @@ panthor_gem_create_with_handle(struct drm_file *file,
>>> bo->base.base.resv = bo->exclusive_vm_root_gem->resv;
>>> }
>>>
>>> + panthor_gem_debugfs_set_usage_flags(bo, 0);
>>> +
>>> /*
>>> * Allocate an id of idr table where the obj is registered
>>> * and handle has the id what user can see.
>>> @@ -296,12 +302,6 @@ panthor_gem_create_with_handle(struct drm_file *file,
>>> /* drop reference from allocate - handle holds it now. */
>>> drm_gem_object_put(&shmem->base);
>>>
>>> - /*
>>> - * No explicit flags are needed in the call below, since the
>>> - * function internally sets the INITIALIZED bit for us.
>>> - */
>>> - panthor_gem_debugfs_set_usage_flags(bo, 0);
>>> -
>>> return ret;
>>> }
>>>
>>> @@ -387,7 +387,7 @@ static void panthor_gem_debugfs_bo_print(struct
>>> panthor_gem_object *bo,
>>> unsigned int refcount = kref_read(&bo->base.base.refcount);
>>> char creator_info[32] = {};
>>> size_t resident_size;
>>> - u32 gem_usage_flags = bo->debugfs.flags &
>>> (u32)~PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED;
>>> + u32 gem_usage_flags = bo->debugfs.flags;
>>> u32 gem_state_flags = 0;
>>>
>>> /* Skip BOs being destroyed. */
>>> @@ -436,8 +436,7 @@ void panthor_gem_debugfs_print_bos(struct
>>> panthor_device *ptdev,
>>>
>>> scoped_guard(mutex, &ptdev->gems.lock) {
>>> list_for_each_entry(bo, &ptdev->gems.node, debugfs.node) {
>>> - if (bo->debugfs.flags &
>>> PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED)
>>> - panthor_gem_debugfs_bo_print(bo, m, &totals);
>>> + panthor_gem_debugfs_bo_print(bo, m, &totals);
>>> }
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_gem.h
>>> b/drivers/gpu/drm/panthor/panthor_gem.h
>>> index 4dd732dcd59f..8fc7215e9b90 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_gem.h
>>> +++ b/drivers/gpu/drm/panthor/panthor_gem.h
>>> @@ -35,9 +35,6 @@ enum panthor_debugfs_gem_usage_flags {
>>>
>>> /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED: BO is mapped on the FW
>>> VM. */
>>> PANTHOR_DEBUGFS_GEM_USAGE_FLAG_FW_MAPPED =
>>> BIT(PANTHOR_DEBUGFS_GEM_USAGE_FW_MAPPED_BIT),
>>> -
>>> - /** @PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED: BO is ready for
>>> DebugFS display. */
>>> - PANTHOR_DEBUGFS_GEM_USAGE_FLAG_INITIALIZED = BIT(31),
>>> };
>>>
>>> /**
>>
>
