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

[Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>]

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


Thanks,
Aravind.
>
>>
>> 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