Re: [PATCH v6 8/8] drm/xe/pci: Introduce PCIe FLR

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Wed, Apr 29, 2026 at 06:33:55AM +0200, Raag Jadav wrote:
> On Tue, Apr 28, 2026 at 04:28:15PM -0700, Daniele Ceraolo Spurio wrote:
> > <snip>
> > 
> > I haven't gone through the code yet, but I wanted to ask some questions
> > regarding the approach first.
> 
> Sure.
> 
> > > +
> > > +/**
> > > + * DOC: PCI Error Handling
> > > + *
> > > + * Xe driver registers PCI callbacks which are called by PCI core in case of
> > > + * bus errors or resets.
> > > + *
> > > + * Currently only PCI Function Level Reset (FLR) callbacks are supported. Since
> > > + * most of the Endpoint Function state is lost on PCIe FLR, the flow is pretty
> > > + * much similar to system suspend/resume flow with a few notable exceptions.
> > 
> > IMO we need a couple of lines to describe what the impact of FLR is on the
> > HW. Something like:
> > 
> > "PCI FLR clears VRAM and resets the state of all the HW units. Therefore,
> > the contents of all exec queues and BOs in VRAM are lost and the HW needs a
> > full re-init".
> 
> Makes sense.
> 
> > > + *
> > > + * Prepare phase:
> > > + * - Temporarily wedge the device to prevent userspace access
> > 
> > I'm not convinced that wedging is the correct approach here, because the
> > expectation from the apps POV is that wedging is permanent, so they won't
> > try again later. Maybe we can have a separate flr_in_progress flag and
> > return something like -EBUSY or -EAGAIN when the FLR is in progress?
> 
> This was my initial plan but during implementation I realized that much
> of the code paths that need handling based new flag are already handled
> by wedged flag. Like IOCTLs, dummy page faulting, GT reset worker, GuC
> submission, GuC PC and TLB invalidation corner cases, SRIOV races and so
> on. So I decided to reuse it here.
> 
> In my understand wedging is permanent only when we choose to send the
> uevent and expect device recovery from userspace, which IIUC we're not.
> So I hope that's okay?

Right, it should be okay.

But we have 2 different users on top.

Runtime (NEO/Level0-core and Apps):

UMDs will send DEVICE_LOST to application in the case of any kind of reset.
Nothing prevents App to go and try it again. It will just receive error.

Admin (Level0-sysman and XPUManager):

As Raag told, to them it is only permanent if we ask for help through the
wedge uevent hints. Otherwise they should still be able to re-enumerate
the devices whenever needed.

> 
> > > + * - Stop accepting new submissions
> > 
> > This is done as part of the above step and it isn't a separate one, right?
> 
> We explicitly xe_guc_submit_disable() inside flr_prepare() so I thought it
> was worth spelling out. Will drop.
> 
> > > + * - Kill exec queues which signals all fences and frees in-flight jobs
> > > + * - Skip memory eviction due to untrustworthy VRAM contents
> > 
> > Note that the VRAM contents are not necessarily untrustworthy at this points
> > since the FLR hasn't happened yet. However, if the admin is triggering an
> > FLR it is likely that something is broken (whether memory, GuC, GT or
> > something else), so we shouldn't try to touch the HW anyway.
> 
> Yes, that's what I meant here but your phrasing is better. Will update.
> 
> > > + * - Remove all memory mappings since VRAM contents will be lost
> > 
> > Dumb question, but what happens if a userspace app has an object mapped and
> > they try to access it from the CPU after this step?
> 
> I'm not much familiar with MM parts but from what I understand it'll
> cause a fault which should be redirected to dummy page. I've tried to
> handle it with commit c020fff70d75 but I'm not sure if that's sufficient.
> This is why I've marked MM corner cases as TODO.
> 
> > > + *
> > > + * Re-initialization phase:
> > > + * - Recreate kernel bos due to skipped eviction in prepare phase
> > > + * - Restore kernel queues which were killed in prepare phase
> > > + * - Reload all uC firmwares
> > > + * - Bring up GT and unwedge to allow userspace access
> > > + *
> > > + * Since VRAM contents are lost, the user is expected to recreate user memory
> > > + * and reload context.
> > 
> > How is the user expected to realize that they need to re-create their BOs? A
> > queue can be killed for different reasons and normally that doesn't imply
> > that any associated BO is now invalid.
> 
> We return -ECANCELED if wedged flag is set and the dummy page data will
> read all 0s. This would be the indication to the application that it needs
> to recreate user memory and reload context.
> 
> Raag
> 
> > > + *
> > > + * TODO: Add PCIe error handling callbacks using similar flow.
> > > + *
> > > + * Current implementation is only limited to re-initializing GT.
> > > + * This needs to be extended for a lot of components listed below.
> > > + *
> > > + * - Proper re-initialization of GSC and PXP for integrated platforms
> > > + * - SRIOV cases which need synchronization between PF and VF
> > > + * - Re-initialization of all child devices of Xe
> > > + * - User memory handling and MM corner cases
> > > + * - Display
> > > + */
> > > +
> > > 
> >