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 11:36 AM, Riana Tauro wrote:
>
>
> 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
Sure. We can probably have the original cleanup in fini.
>>
>> 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