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

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

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

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