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

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

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@lists.freedesktop.org> On Behalf
> > > Of Rodrigo
> > > Vivi
> > > Sent: Tuesday, December 12, 2023 5:51 AM
> > > To: intel-xe@lists.freedesktop.org
> > > Cc: Roper, Matthew D <matthew.d.roper@intel.com>; Vivi, Rodrigo
> > > <rodrigo.vivi@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?

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

> 
> 
> Thanks,
> Aravind.
> > 
> > Tejas
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@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