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

[Matthew Auld]

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?

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