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

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

On 12/11/2023 4:43 PM, Riana Tauro wrote:
>
>
> On 12/11/2023 4:37 PM, Riana Tauro wrote:
>> Hi Suja
>>
>> On 12/11/2023 4:31 PM, Upadhyay, Tejas wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
>>>> Sent: Monday, December 11, 2023 3:45 PM
>>>> To: Tauro, Riana <riana.tauro@intel.com>; Upadhyay, Tejas
>>>> <tejas.upadhyay@intel.com>; intel-xe@lists.freedesktop.org
>>>> Cc: Gupta, Anshuman <anshuman.gupta@intel.com>
>>>> Subject: Re: [v3 1/2] drm/xe: Add a new memory directory under tile
>>>>
>>>>
>>>> On 12/11/2023 3:23 PM, Riana Tauro wrote:
>>>>> Hi Suja
>>>>>
>>>>> On 12/8/2023 1:58 PM, Sundaresan, Sujaritha wrote:
>>>>>>
>>>>>> On 12/8/2023 10:33 AM, Upadhyay, Tejas wrote:
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Sundaresan, Sujaritha <sujaritha.sundaresan@intel.com>
>>>>>>>> Sent: Thursday, December 7, 2023 8:48 PM
>>>>>>>> To: intel-xe@lists.freedesktop.org
>>>>>>>> Cc: Upadhyay, Tejas <tejas.upadhyay@intel.com>; Tauro, Riana
>>>>>>>> <riana.tauro@intel.com>; Gupta, Anshuman
>>>>>>>> <anshuman.gupta@intel.com>; Sundaresan, Sujaritha
>>>>>>>> <sujaritha.sundaresan@intel.com>
>>>>>>>> Subject: [v3 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
>>>>>>>>
>>>>>>>> v3: Fix reload error with kobject_put in place
>>>>>>>>       of kobject_del (Tejas)
>>>>>>>>
>>>>>>>> Signed-off-by: Sujaritha Sundaresan
>>>>>>>> <sujaritha.sundaresan@intel.com>
>>>>>>>> ---
>>>>>>>>    drivers/gpu/drm/xe/xe_tile_sysfs.c | 19 ++++++++++++++-----
>>>>>>>>    1 file changed, 14 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>>>>>>> b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>>>>>>> index 16376607c68f..64be1f3a38a9 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);
>>>>>>> As Christmas is approaching, you might want to adjust this in
>>>>>>> Christmas tree order.
>>>>>>>
>>>>>>>>        return sysfs_emit(buf, "%llu\n",
>>>>>>>> tile->mem.vram.actual_physical_size);
>>>>>>>>    }
>>>>>>>> @@ -36,8 +37,10 @@ static const struct attribute
>>>>>>>> *physical_memsize_attr =
>>>>>>>>
>>>>>>>>    static void tile_sysfs_fini(struct drm_device *drm, void 
>>>>>>>> *arg)  {
>>>>>>>> -    struct xe_tile *tile = arg;
>>>>>>>> +    struct kobject *kobj = arg;
>>>>>>>> +    struct xe_tile *tile = kobj_to_tile(kobj->parent);
>> Also, check if kobj is not null before conversion. As we are 
>> continuing with init even if kobj creation fails
>>>>>>> Ditto
>>>>>>>
>>>>>>>> +    kobject_put(kobj);
>>>>>>>>        kobject_put(tile->sysfs);
>>>>>>>>    }
>>>>>>>>
>>>>>>>> @@ -46,6 +49,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,12 +68,17 @@
>>>>>>>> void xe_tile_sysfs_init(struct xe_tile *tile)
>>>>>>>>
>>>>>>>>        tile->sysfs = &kt->base;
>>>>>>>>
>>>>>>>> -    if (IS_DGFX(xe) && xe->info.platform != XE_DG1 &&
>>>>>>>> -        sysfs_create_file(tile->sysfs, physical_memsize_attr))
>>>>>>>> +    kobj = kobject_create_and_add("memory", tile->sysfs);
>>>>>>>> +    if (!kobj) {
>>>>>>>> +        drm_warn(&xe->drm, "%s failed, err: %d\n", __func__, -
>>>>>>>> ENOMEM);
>>>>>>>> +    }
>>>>>
>>>>> There will be an empty memory directory in case of igfx. Should the
>>>>> memory directory be created only if there are attributes available?
>>>
>>> Yes I think it should be taking care of all conditions which 
>>> physical_memsize_attr sysfs file was created with.
>
> Yeah it needs to be as Tejas suggested.
> Should be under the same if

After discussions, I will have the memory directory created if DGFX and 
not DG1.

Will move the documentation for physical_memsize_attr to this patch.

But maybe changes in the cleanup need to be tested before committing to 
anything.

Not creating anything in IGFX may need more cleanup.

Thanks,

Suja

>
>>>
>>> Thanks,
>>> Tejas
>>>>>
>>>>> Thanks
>>>>> Riana
>>>>
>>>> I can have it added only if IS_DGFX if that works.
>>
>> Move the documentation of the physical_vram_size_bytes to this patch
>>
>> Thanks
>> Riana
>>>>
>>>> Thanks,
>>>>
>>>> Suja
>>>>
>>>>>>> { } is not required here.
>>>>>>>
>>>>>>> Tejas
>>>>>>
>>>>>> Will fix all of the above.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Suja
>>>>>>
>>>>>>>> +
>>>>>>>> +    if (kobj && IS_DGFX(xe) && xe->info.platform != XE_DG1 &&
>>>>>>>> +        sysfs_create_file(kobj, physical_memsize_attr))
>>>>>>>>            drm_warn(&xe->drm,
>>>>>>>>                 "Sysfs creation to read addr_range per tile
>>>>>>>> failed\n");
>>>>>>>>
>>>>>>>> -    err = drmm_add_action_or_reset(&xe->drm, tile_sysfs_fini,
>>>>>>>> tile);
>>>>>>>> +    err = drmm_add_action_or_reset(&xe->drm, tile_sysfs_fini,
>>>>>>>> +kobj);
>>>>>>>>        if (err) {
>>>>>>>>            drm_warn(&xe->drm, "%s: drmm_add_action_or_reset 
>>>>>>>> failed,
>>>>>>>> err: %d\n",
>>>>>>>>                 __func__, err);
>>>>>>>> -- 
>>>>>>>> 2.25.1