[Intel-xe] [v2 1/2] drm/xe: Add a new memory directory under tile
Upadhyay, Tejas
tejas.upadhyay at intel.com
Thu Dec 7 12:38:38 UTC 2023
> -----Original Message-----
> From: Sundaresan, Sujaritha <sujaritha.sundaresan at intel.com>
> Sent: Thursday, December 7, 2023 3:26 PM
> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>; Tauro, Riana
> <riana.tauro at intel.com>; Gupta, Anshuman <anshuman.gupta at intel.com>;
> intel-xe at lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Subject: Re: [Intel-xe] [v2 1/2] drm/xe: Add a new memory directory under
> tile
>
>
> On 12/7/2023 2:00 PM, Upadhyay, Tejas wrote:
> >
> >> -----Original Message-----
> >> From: Sundaresan, Sujaritha <sujaritha.sundaresan at intel.com>
> >> Sent: Thursday, December 7, 2023 12:58 PM
> >> To: Tauro, Riana <riana.tauro at intel.com>; Upadhyay, Tejas
> >> <tejas.upadhyay at intel.com>; Gupta, Anshuman
> >> <anshuman.gupta at intel.com>; intel-xe at lists.freedesktop.org
> >> Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> >> Subject: Re: [Intel-xe] [v2 1/2] drm/xe: Add a new memory directory
> >> under tile
> >>
> >>
> >> On 12/7/2023 12:08 PM, Sundaresan, Sujaritha wrote:
> >>> On 12/7/2023 11:36 AM, Riana Tauro wrote:
> >>>>
> >>>> On 12/7/2023 10:51 AM, Sundaresan, Sujaritha wrote:
> >>>>> On 12/7/2023 10:42 AM, Upadhyay, Tejas wrote:
> >>>>>>> -----Original Message-----
> >>>>>>> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On
> >>>>>>> Behalf Of Sundaresan, Sujaritha
> >>>>>>> Sent: Wednesday, December 6, 2023 5:44 PM
> >>>>>>> To: Gupta, Anshuman <anshuman.gupta at intel.com>; intel-
> >>>>>>> xe at lists.freedesktop.org
> >>>>>>> Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> >>>>>>> Subject: Re: [Intel-xe] [v2 1/2] drm/xe: Add a new memory
> >>>>>>> directory under tile
> >>>>>>>
> >>>>>>>
> >>>>>>> On 12/6/2023 5:38 PM, Sundaresan, Sujaritha wrote:
> >>>>>>>> On 12/6/2023 5:23 PM, Gupta, Anshuman wrote:
> >>>>>>>>>> -----Original Message-----
> >>>>>>>>>> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On
> >>>>>>>>>> Behalf Of Sujaritha Sundaresan
> >>>>>>>>>> Sent: Wednesday, December 6, 2023 10:18 AM
> >>>>>>>>>> To: intel-xe at lists.freedesktop.org
> >>>>>>>>>> Cc: Sundaresan, Sujaritha <sujaritha.sundaresan at intel.com>;
> >>>>>>>>>> Vivi, Rodrigo <rodrigo.vivi at intel.com>
> >>>>>>>>>> Subject: [Intel-xe] [v2 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
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Sujaritha Sundaresan
> >>>>>>>>>> <sujaritha.sundaresan at intel.com>
> >>>>>>>>>> ---
> >>>>>>>>>> drivers/gpu/drm/xe/xe_tile_sysfs.c | 15 ++++++++++++---
> >>>>>>>>>> 1 file changed, 12 insertions(+), 3 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/gpu/drm/xe/xe_tile_sysfs.c
> >>>>>>>>>> b/drivers/gpu/drm/xe/xe_tile_sysfs.c
> >>>>>>>>>> index 16376607c68f..e8ce4d9270e6 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);
> >>>>>>>>>>
> >>>>>>>>>> return sysfs_emit(buf, "%llu\n",
> >>>>>>>>>> tile->mem.vram.actual_physical_size);
> >>>>>>>>>> }
> >>>>>>>>>> @@ -38,7 +39,7 @@ static void tile_sysfs_fini(struct
> >>>>>>>>>> drm_device *drm, void
> >>>>>>>>>> *arg) {
> >>>>>>>>>> struct xe_tile *tile = arg;
> >>>>>>>>>>
> >>>>>>>>>> - kobject_put(tile->sysfs);
> >>>>>>>>>> + kobject_del(tile->sysfs);
> >>>>>>>>> Why kobekct_del instead of kobject_put?
> >>>>>>>>> Thanks,
> >>>>>>>>> Anshuman Gupta.
> >>>>>>>> Hi Anshuman,
> >>>>>>>>
> >>>>>>>> Basically when sanity checking, after reload we see that we are
> >>>>>>>> not doing a proper cleanup.
> >>>>>>>>
> >>>>>>>> kobject_put will only decrement the ref count and possibly free
> >>>>>>>> the kobject.
> >>>>>>>>
> >>>>>>>> But that is not happening in this case. There is a duplicate
> >>>>>>>> remaining of the tile directory.
> >>>>>>>>
> >>>>>>>> This required a clean unregister of the parent from sysfs hence
> >>>>>>>> the use of kobject_del.
> >>>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>>
> >>>>>>>> Suja
> >>>>>>> As a continuation of the above response;
> >>>>>>>
> >>>>>>> I can probably add a kobject_put call as well to ensure that we
> >>>>>>> are cleaning up the memory side of
> >>>>>>>
> >>>>>>> things as well. Will add.
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>>
> >>>>>>> Suja
> >>>>>>>
> >>>>>>>>>> }
> >>>>>>>>>>
> >>>>>>>>>> void xe_tile_sysfs_init(struct xe_tile *tile) @@ -46,6
> >>>>>>>>>> +47,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,8
> >>>>>>>>>> +66,15 @@ void xe_tile_sysfs_init(struct xe_tile *tile)
> >>>>>>>>>>
> >>>>>>>>>> tile->sysfs = &kt->base;
> >>>>>>>>>>
> >>>>>>>>>> + kobj = kobject_create_and_add("memory", tile->sysfs);
> >>>>>>>>>> + if (!kobj) {
> >>>>>>>>>> + kobject_put(kobj);
> >>>>>> Do you mean to put kobject_put(tile->sysfs) instead of
> >>>>>> kobject_put(kobj) ? as there was no Kobj created by the time you
> >>>>>> reached here!
> >>>>>>
> >>>>>> Tejas
> >>>>> Yup this should be fixed.
> >>>> Hi Suja
> >>>>
> >>>> Removing tile won't be right, as there are other directories (gt#)
> >>>> dependent on it. Simple return should be good with a warn?
> >>>>
> >>>> Thanks
> >>>> Riana
> >>> Sure. We can probably have the original cleanup in fini.
> > If you just give warn and return then you will never register tile_fini function
> and it will never be called on driver unload/reload. So either you remove tile
> using kobject_put() before return or don’t return and check kobj before
> creating files under it to avoid crash.
> >
> > Tejas
>
> Let me test out everything and see which approach will work the best.
>
> Thanks,
>
> Suja
Also please put reference of kobj created for "memory" in tile_sysfs_fini() . Like below :
kobject_put(kobj);
kobject_put(tile->sysfs);
Tejas
>
> >
> >>>>> Thanks.
> >>>>>
> >>>>> Suja
> >>>>>
> >>>>>>>>>> + drm_warn(&xe->drm, "%s failed, err: %d\n", __func__, -
> >>>>>>>>>> ENOMEM);
> >>>>>>>>>> + return;
> >>>>>>>>>> + }
> >>>>>>>>>> +
> >>>>>>>>>> if (IS_DGFX(xe) && xe->info.platform != XE_DG1 &&
> >>>>>>>>>> - sysfs_create_file(tile->sysfs,
> >>>>>>>>>> physical_memsize_attr))
> >>>>>>>>>> + sysfs_create_file(kobj, physical_memsize_attr))
> >>>>>>>>>> drm_warn(&xe->drm,
> >>>>>>>>>> "Sysfs creation to read addr_range per tile
> >>>>>>>>>> failed\n");
> >>>>>>>>>>
> >>>>>>>>>> --
> >>>>>>>>>> 2.25.1
> >> Hi all,
> >>
> >> So after all of this discussion, here's the final results after testing the
> cleanup.
> >>
> >> Regardless of anything we do in the init function, it looks like
> >> without having the
> >>
> >> two-step kobject_del and kobject_put cleanup in the sysfs_fini
> >> function we will
> >>
> >> see an error on reload with the tile directory not being fully
> >> cleaned up and reporting
> >>
> >> duplicate creation.
> >>
> >> The solution seems to be just to add a kobject_del before kobject_put
> >> in the fini
> >>
> >> function.
> >>
> >> I am only having warnings and simple returns on the init side.
> >>
> >> If there can be a consensus about this from the reviewers, I can
> >> float the next version
> >>
> >> accordingly.
> >>
> >> Thanks,
> >>
> >> Suja
More information about the Intel-xe
mailing list