Re: [Intel-xe] [v2 1/2] drm/xe: Add a new memory directory under tile

["Sundaresan, Sujaritha" <sujaritha.sundaresan@intel.com>]

On 12/6/2023 5:38 PM, Sundaresan, Sujaritha wrote:
>
> On 12/6/2023 5:23 PM, Gupta, Anshuman wrote:
>>
>>> -----Original Message-----
>>> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of 
>>> Sujaritha
>>> Sundaresan
>>> Sent: Wednesday, December 6, 2023 10:18 AM
>>> To: intel-xe@lists.freedesktop.org
>>> Cc: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>; Vivi, 
>>> Rodrigo
>>> <rodrigo.vivi@intel.com>
>>> Subject: [Intel-xe] [v2 1/2] drm/xe: Add a new memory directory 
>>> under tile
>>>
>>> Add a new memory directory under /device/tile<n> and move 
>>> physical_vram_size
>>> attribute to the new directory.
>>>
>>> New hierarchy:
>>>
>>> /device/tile<n>/memory/physical_vram_size_bytes
>>>
>>> v2: Fix heading typo (Riana)
>>>      Fix cleanup error on unload/reload cycle
>>>
>>> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_tile_sysfs.c | 15 ++++++++++++---
>>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>> b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>> index 16376607c68f..e8ce4d9270e6 100644
>>> --- a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>> +++ b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>> @@ -24,7 +24,8 @@ static ssize_t
>>>   physical_vram_size_bytes_show(struct device *kdev, struct 
>>> device_attribute
>>> *attr,
>>>                     char *buf)
>>>   {
>>> -    struct xe_tile *tile = kobj_to_tile(&kdev->kobj);
>>> +    struct kobject *kobj = &kdev->kobj;
>>> +    struct xe_tile *tile = kobj_to_tile(kobj->parent);
>>>
>>>       return sysfs_emit(buf, "%llu\n", 
>>> tile->mem.vram.actual_physical_size);
>>>   }
>>> @@ -38,7 +39,7 @@ static void tile_sysfs_fini(struct drm_device 
>>> *drm, void
>>> *arg)  {
>>>       struct xe_tile *tile = arg;
>>>
>>> -    kobject_put(tile->sysfs);
>>> +    kobject_del(tile->sysfs);
>> Why kobekct_del instead of kobject_put?
>> Thanks,
>> Anshuman Gupta.
>
> Hi Anshuman,
>
> Basically when sanity checking, after reload we see that we are not 
> doing a proper cleanup.
>
> kobject_put will only decrement the ref count and possibly free the 
> kobject.
>
> But that is not happening in this case. There is a duplicate remaining 
> of the tile directory.
>
> This required a clean unregister of the parent from sysfs hence the 
> use of kobject_del.
>
> Thanks,
>
> Suja

As a continuation of the above response;

I can probably add a kobject_put call as well to ensure that we are 
cleaning up the memory side of

things as well. Will add.

Thanks,

Suja

>
>>>   }
>>>
>>>   void xe_tile_sysfs_init(struct xe_tile *tile) @@ -46,6 +47,7 @@ void
>>> xe_tile_sysfs_init(struct xe_tile *tile)
>>>       struct xe_device *xe = tile_to_xe(tile);
>>>       struct device *dev = xe->drm.dev;
>>>       struct kobj_tile *kt;
>>> +    struct kobject *kobj;
>>>       int err;
>>>
>>>       kt = kzalloc(sizeof(*kt), GFP_KERNEL); @@ -64,8 +66,15 @@ void
>>> xe_tile_sysfs_init(struct xe_tile *tile)
>>>
>>>       tile->sysfs = &kt->base;
>>>
>>> +    kobj = kobject_create_and_add("memory", tile->sysfs);
>>> +    if (!kobj) {
>>> +        kobject_put(kobj);
>>> +        drm_warn(&xe->drm, "%s failed, err: %d\n", __func__, -
>>> ENOMEM);
>>> +        return;
>>> +    }
>>> +
>>>       if (IS_DGFX(xe) && xe->info.platform != XE_DG1 &&
>>> -        sysfs_create_file(tile->sysfs, physical_memsize_attr))
>>> +        sysfs_create_file(kobj, physical_memsize_attr))
>>>           drm_warn(&xe->drm,
>>>                "Sysfs creation to read addr_range per tile failed\n");
>>>
>>> -- 
>>> 2.25.1