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

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

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);
>>> 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?
>
> Thanks
> Riana

I can have it added only if IS_DGFX if that works.

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