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

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

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.
> 
> Thanks,
> Tejas
>>>
>>> Thanks
>>> Riana
>>
>> I can have it added only if IS_DGFX if that works.
Sure.

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