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

[Raag Jadav <raag.jadav@intel.com>]

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?

> > + * - 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
> > + */
> > +
> > 
>