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

[Riana Tauro <riana.tauro@intel.com>]

On 12/7/2023 10:51 AM, Sundaresan, Sujaritha wrote:
> 
> 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.
Hi Suja

Removing tile won't be right, as there are other directories (gt#) 
dependent on it. Simple return should be good with a warn?

Thanks
Riana
> 
> 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