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

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

On 12/7/2023 10:42 AM, Upadhyay, Tejas wrote:
>
>> -----Original Message-----
>> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of
>> Sundaresan, Sujaritha
>> Sent: Wednesday, December 6, 2023 5:44 PM
>> To: Gupta, Anshuman <anshuman.gupta@intel.com>; intel-
>> xe@lists.freedesktop.org
>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>> Subject: Re: [Intel-xe] [v2 1/2] drm/xe: Add a new memory directory under
>> tile
>>
>>
>> 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);
> Do you mean to put kobject_put(tile->sysfs) instead of kobject_put(kobj) ? as there was no Kobj created by the time you reached here!
>
> Tejas

Yup this should be fixed.

Thanks.

Suja

>
>>>>> +        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