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

[Upadhyay, Tejas]

> -----Original Message-----
> From: Sundaresan, Sujaritha <sujaritha.sundaresan at intel.com>
> Sent: Monday, December 11, 2023 3:45 PM
> To: Tauro, Riana <riana.tauro at intel.com>; Upadhyay, Tejas
> <tejas.upadhyay at intel.com>; intel-xe at lists.freedesktop.org
> Cc: Gupta, Anshuman <anshuman.gupta at 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 at intel.com>
> >>>> Sent: Thursday, December 7, 2023 8:48 PM
> >>>> To: intel-xe at lists.freedesktop.org
> >>>> Cc: Upadhyay, Tejas <tejas.upadhyay at intel.com>; Tauro, Riana
> >>>> <riana.tauro at intel.com>; Gupta, Anshuman
> >>>> <anshuman.gupta at intel.com>; Sundaresan, Sujaritha
> >>>> <sujaritha.sundaresan at 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 at 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?

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