Re: [PATCH V16 10/12] drm/xe: Add sysfs interface for bad gpu vram pages

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

On 18-08-2026 18:25, Rodrigo Vivi wrote:
> On Tue, Aug 18, 2026 at 09:08:46AM +0000, Upadhyay, Tejas wrote:
>>
>>> -----Original Message-----
>>> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>>> Sent: 18 August 2026 01:01
>>> To: Wajdeczko, Michal <michal.wajdeczko@intel.com>
>>> Cc: Upadhyay, Tejas <tejas.upadhyay@intel.com>; intel-
>>> xe@lists.freedesktop.org; Thomas Hellström
>>> <thomas.hellstrom@linux.intel.com>; Ghimiray, Himal Prasad
>>> <himal.prasad.ghimiray@intel.com>
>>> Subject: Re: [PATCH V16 10/12] drm/xe: Add sysfs interface for bad gpu vram
>>> pages
>>>
>>> On Mon, Aug 17, 2026 at 07:09:46PM +0200, Michal Wajdeczko wrote:
>>>>
>>>> On 8/17/2026 6:06 PM, Rodrigo Vivi wrote:
>>>>> On Mon, Aug 17, 2026 at 02:58:31PM +0000, Upadhyay, Tejas wrote:
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Wajdeczko, Michal <michal.wajdeczko@intel.com>
>>>>>>> Sent: 17 August 2026 16:57
>>>>>>> To: Upadhyay, Tejas <tejas.upadhyay@intel.com>; intel-
>>>>>>> xe@lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
>>>>>>> Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>>>>> Cc: Ghimiray, Himal Prasad <himal.prasad.ghimiray@intel.com>
>>>>>>> Subject: Re: [PATCH V16 10/12] drm/xe: Add sysfs interface for bad
>>>>>>> gpu vram pages
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 8/17/2026 8:51 AM, Tejas Upadhyay wrote:
>>>>>>>> Include a sysfs interface designed to expose information about
>>>>>>>> bad VRAM pages — those identified as having hardware faults
>>>>>>>> (e.g., ECC errors). This interface allows userspace tools and
>>>>>>>> administrators to monitor the health of the GPU's local memory
>>>>>>>> and track the status of page retirement. Details on bad gpu vram
>>>>>>>> pages can be found under
>>> /sys/bus/pci/devices/<bdf>/vram_bad_pages.
>>>>>>> since those new files are xe driver specific, shouldn't we refer
>>>>>>> to them using
>>>>>>>
>>>>>>> 	/sys/bus/pci/drivers/xe/<bdf>/vram...
>>>>>>>
>>>>>>>> The format is: pfn : gpu_page_size : flags
>>>>>>> kernel documentation [1] says
>>>>>>>
>>>>>>> 	"Mixing types, expressing multiple lines of data, and doing
>>>>>>> 	fancy formatting of data is heavily frowned upon"
>>>>>>>
>>>>>>> [1] https://docs.kernel.org/filesystems/sysfs.html#attributes
>>>>>>>
>>>>>>> so to follow the guidelines maybe we expose the separate files:
>>>>>>>
>>>>>>> /sys/bus/pci/drivers/xe/<bdf>/vram_page_size		u64
>>>>>>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_count	u64
>>>>>>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_reserved	u64[]
>>>>>>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_pending	u64[]
>>>>>>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_failed	u64[]
>>>>>>>
>>>>>>> or
>>>>>>>
>>>>>>> /sys/bus/pci/drivers/xe/<bdf>
>>>>>>> |
>>>>>>> +-- vram/
>>>>>>>     +-- page_size	u64
>>>>>>>     +-- bad_pages/
>>>>>>>         +-- count	u64
>>>>>>>         +-- reserved	u64[]
>>>>>>>         +-- pending	u64[]
>>>>>>>         +-- failed	u64[]
>>>>>>>
>>>>>>> then
>>>>>>>
>>>>>>> /sys/bus/pci/drivers/xe/<bdf>/vram_page_size:0x1000
>>>>>>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_count:5
>>>>>>>
>>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_reserved:0x0000000000
>>>>>>> 00
>>>>>>> 0000
>>>>>>>
>>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_pending:0x00000000012
>>>>>>> 34
>>>>>>> 000
>>>>>>>
>>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_pending:0x00000000012
>>>>>>> 35
>>>>>>> 000
>>>>>>>
>>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_pending:0x00000000012
>>>>>>> 36
>>>>>>> 000
>>>>>>>
>>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_pending:0x00000000012
>>>>>>> 37
>>>>>>> 000
>>>>>> Thanks for comment, this is documented format by design doc. Sysman
>>> also depending on this format. So I don’t see this can be done without design
>>> being changed for everyone.
>>>>> Internal design docs don't superseed upstream documentation.
>>>>> It is the other way around.
>>>>>
>>>>> But also, the files will be there one way or another. Both paths are
>>>>> valid, so I don't believe that change in here force changes in the
>>>>> userspace. Although, yes consistency is good...
>>>>>
>>>>> That said, I don't have a strong feeling for one way or the other.
>>>>>
>>>>> Since we are adding to the device level anyway, I believe it should
>>>>> be okay. But Michal, do you know any doc or any precedence that kind
>>>>> of force us to go the other way?
>>>> hmm, are we talking here about the attribute format or folder layout?
>>>>
>>>> if about the latter, no strong feeling either ("files will be there
>>>> one way or another")
>>>>
>>>> but if about the former, then the same documentation [1] earlier says:
>>>>
>>>> 	"Attributes should be ASCII text files, preferably with only
>>>> 	"one value per file. It is noted that it may not be efficient
>>>> 	"to contain only one value per file, so it is socially acceptable
>>>> 	"to express an array of values of the same type.
>>>>
>>>> and my proposal with separate files meets that expectations (there
>>>> will be either single value in the file or array of values of the same
>>>> type), opposed to original idea of array of offset:page_size:flag
>>>> tuples
>>> doh! I'm sorry... my comment was purely driven by the other sentence above:
>>> "since those new files are xe driver specific, shouldn't we refer to them using"
>>>
>>> But now I looked at the content o the patch itself. This patch as is is a BIG NO!
>>> It is against the sysfs rules. Period. Internal spec and other components need
>>> to adjust.
>>>
>>> Also please do not repeat the same PVC mistakes with tenths of lingering sysfs
>>> entries. Organize this per directory as Michal told.
>>>
>>> Another thing, make a design that is future ready, use 'vram0/' as the name of
>>> the directory with vram0 stuff. Like we have freq0/ for instance.
>>>
>>> Perhaps even
>>>
>>> +-- vram0/
>>>   +-- pages/
>>>      +-- size u64
>>>      +-- bad_pages/
>>>          +-- count u64
>>>          +-- reserved      u64[]
>>>          +-- pending       u64[]
>>>          +-- failed        u64[]
>> Currently information shown under vram_bad_pages(looks similar to what other competitor's  bad pages info shows), actual gives data which consumer can extract directly meaningful info out of it. With above approach, consumer need to make one, which we need to discuss with other folks. Lets discuss in a group.
> 2 wrongs don't make 1 right!
>
> If you don't believe in the reviewers check the documentation yourself:
>
> https://www.kernel.org/doc/html/latest/filesystems/sysfs.html
>
> "Mixing types, expressing multiple lines of data, and doing fancy formatting
> of data is heavily frowned upon. Doing these things may get you publicly
> humiliated and your code rewritten without notice."

Is my understanding correct that the PAGE_SIZE limit and the "one value
per file" guidance apply to regular attributes only, and that a
bin_attribute is the sanctioned mechanism for streaming output larger
than one page (via the off/count arguments)?

Thanks,
Aravind.


>
>> Tejas
>>> Thanks,
>>> Rodrigo.
>>>
>>>>>> Tejas
>>>>>>>> flags:
>>>>>>>>   R: reserved, this gpu page is reserved.
>>>>>>>>   P: pending for reserve, this gpu page is marked as bad, will be
>>>>>>>>      reserved in next window of page_reserve.
>>>>>>>>   F: unable to reserve, this gpu page can't be reserved due to some
>>>>>>>>      reasons.
>>>>>>>>
>>>>>>>> For example, cat /sys/bus/pci/devices/<bdf>/vram_bad_pages:
>>>>>>>>   max_pages : 10000
>>>>>>>>   0x0000000000000000 : 0x0000000000001000 : R
>>>>>>>>   0x0000000000001234 : 0x0000000000001000 : P
>>>>>>>>
>>>>>>>> The sysfs binary attribute is created under the PCI device
>>>>>>>> kobject when the platform supports it and the configfs
>>>>>>>> bad_page_reservation policy is enabled. Uses RCU-protected list
>>>>>>>> traversal so reads never block normal VRAM allocation operations.
>>>>>>>>