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

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

On 12/14/23 15:13, Matthew Auld wrote:
> On Thu, 14 Dec 2023 at 05:56, Aravind Iddamsetty
> <aravind.iddamsetty@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@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.
> 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?

But also for some device properties the intention is to expose via sysfs
so that it doesn't mandate any open handle to the driver.

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