[PATCH] drm/xe: Remove vram size info from sysfs

[Upadhyay, Tejas]

> -----Original Message-----
> From: Matthew Auld <matthew.william.auld at gmail.com>
> Sent: Thursday, December 14, 2023 3:13 PM
> To: Aravind Iddamsetty <aravind.iddamsetty at linux.intel.com>
> Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>; intel-xe at lists.freedesktop.org;
> Upadhyay, Tejas <tejas.upadhyay at intel.com>; Roper, Matthew D
> <matthew.d.roper at intel.com>
> Subject: Re: [PATCH] drm/xe: Remove vram size info from sysfs
> 
> On Thu, 14 Dec 2023 at 05:56, Aravind Iddamsetty
> <aravind.iddamsetty at linux.intel.com> wrote:
> >
> >
> > On 12/13/23 21:23, Vivi, Rodrigo wrote:
> > > On Wed, 2023-12-13 at 10:48 +0530, Aravind Iddamsetty wrote:
> > >> On 12/13/23 09:43, Upadhyay, Tejas wrote:
> > >>>> -----Original Message-----
> > >>>> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf
> > >>>> Of Rodrigo Vivi
> > >>>> Sent: Tuesday, December 12, 2023 5:51 AM
> > >>>> To: intel-xe at lists.freedesktop.org
> > >>>> Cc: Roper, Matthew D <matthew.d.roper at intel.com>; Vivi, Rodrigo
> > >>>> <rodrigo.vivi at intel.com>
> > >>>> Subject: [PATCH] drm/xe: Remove vram size info from sysfs
> > >>>>
> > >>>> This information is already part of the query IOCTL.
> > >>>> Let's not duplicate it here in the sysfs.
> > >>> This patch was on request from L0 as they wanted actual VRAM size
> > >>> including reserved portion. We should check with UMD team if they
> > >>> require this sysfs or not. I will check.
> > >> Hi Rodrigo,
> > >>
> > >> As Tejas mentioned the sysfs interface exposes the total physical
> > >> vram including the stolen size which is needed by sysman, in the
> > >> IOCTL only usable size(without stolen) is exposed.
> > > I have removed it now so we can discuss better and ensure that we
> > > are taking the best uapi decisions that are acceptable and won't
> > > break the compatibility later after we are merged upstream.
> > >
> > > The time to break was now, so I removed.
> > >
> > > But we can continue with the discussion and see what is the best
> > > approach.
> > >
> > > My first question would be: why does Level0 needs to know the stolen
> > > size? So, why we don't add that information along with the query uapi?
> > As part of their memory property API(device property) it needs the
> > total memory on the card not just the usable.
> >
> > https://spec.oneapi.io/level-zero/latest/sysman/api.html#_CPPv4N20zes_
> > mem_properties_t12physicalSizeE
> >
> > >
> > > It would be really awkward to see a value in the sysfs as total
> > > memory and a completely different value on the query ioclt? this
> > > stolen part apparently needs to be more transparent then, no?!
> > The query IOCTL is to show the usable memory for the application to know
> how much it can request hence stolen is removed.
> > while the sysfs is like a device property to show total memory on card.
> 
> What about making region.total_size be the actual physical size, and then
> s/region.used/region.avail/ which is then just the static usable size, or if
> perfmon_capable() it is the live usable size?

I think it should be ok KMD pow, but need agreement with consumer of this, I have email thread going on this as well as on JIRA filed. I will loop everyone there.

Tejas
> 
> >
> >
> > Thanks,
> > Aravind.
> > >
> > >>
> > >> Thanks,
> > >> Aravind.
> > >>> Tejas
> > >>>> Cc: Matt Roper <matthew.d.roper at intel.com>
> > >>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > >>>> ---
> > >>>>  drivers/gpu/drm/xe/xe_tile_sysfs.c | 23 +----------------------
> > >>>>  1 file changed, 1 insertion(+), 22 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/gpu/drm/xe/xe_tile_sysfs.c
> > >>>> b/drivers/gpu/drm/xe/xe_tile_sysfs.c
> > >>>> index 16376607c68f..0f8d3e7fce46 100644
> > >>>> --- a/drivers/gpu/drm/xe/xe_tile_sysfs.c
> > >>>> +++ b/drivers/gpu/drm/xe/xe_tile_sysfs.c
> > >>>> @@ -20,20 +20,6 @@ static const struct kobj_type
> > >>>> xe_tile_sysfs_kobj_type = {
> > >>>>         .sysfs_ops = &kobj_sysfs_ops,  };
> > >>>>
> > >>>> -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);
> > >>>> -
> > >>>> -       return sysfs_emit(buf, "%llu\n", tile-
> > >>>>> mem.vram.actual_physical_size);
> > >>>> -}
> > >>>> -
> > >>>> -static DEVICE_ATTR_RO(physical_vram_size_bytes);
> > >>>> -
> > >>>> -static const struct attribute *physical_memsize_attr =
> > >>>> -       &dev_attr_physical_vram_size_bytes.attr;
> > >>>> -
> > >>>>  static void tile_sysfs_fini(struct drm_device *drm, void *arg) {
> > >>>>         struct xe_tile *tile = arg; @@ -64,15 +50,8 @@ 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))
> > >>>> -               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);
> > >>>> -       if (err) {
> > >>>> +       if (err)
> > >>>>                 drm_warn(&xe->drm, "%s: drmm_add_action_or_reset
> > >>>> failed, err: %d\n",
> > >>>>                          __func__, err);
> > >>>> -               return;
> > >>>> -       }
> > >>>>  }
> > >>>> --
> > >>>> 2.43.0