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

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

On 12/15/23 07:50, Upadhyay, Tejas wrote:
>
>> -----Original Message-----
>> From: Matthew Auld <matthew.william.auld@gmail.com>
>> Sent: Thursday, December 14, 2023 3:13 PM
>> To: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>; intel-xe@lists.freedesktop.org;
>> Upadhyay, Tejas <tejas.upadhyay@intel.com>; Roper, Matthew D
>> <matthew.d.roper@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@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?
> 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.

To reiterate it always suggested to expose sysfs if possible to avoid unnecessarily opening a device handle and waking the device at least for sysman needs.

Thanks,
Aravind.


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