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 <[email protected]>
> >>> Sent: 17 August 2026 16:57
> >>> To: Upadhyay, Tejas <[email protected]>; intel-
> >>> [email protected]; Vivi, Rodrigo <[email protected]>; Thomas
> >>> Hellström <[email protected]>
> >>> Cc: Ghimiray, Himal Prasad <[email protected]>
> >>> 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:0x000000000000
> >>> 0000
> >>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_pending:0x0000000001234
> >>> 000
> >>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_pending:0x0000000001235
> >>> 000
> >>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_pending:0x0000000001236
> >>> 000
> >>> /sys/bus/pci/drivers/xe/<bdf>/vram_bad_pages_pending:0x0000000001237
> >>> 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[]

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