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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 4/29/2026 9:22 AM, Rodrigo Vivi wrote:
> 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.

Those are very specific to server use-cases. While that's what we're 
currently implementing FLR for, there might be other use-cases in the 
future that require us to implement this on the client side (there is 
already at least one case where we wedge but we could instead recover 
via driver-triggered FLR), where the apps can be less curated.

I'm a bit lost on how a random app is supposed to tell the difference 
between temporary and permanent wedges if they get a DEVICE_LOST error 
in both cases. Are we expecting all apps to register to the uevent? Or 
are the UMD drivers expected to return a different code if the wedge is 
permanent? Because I don't think that an app should just keep trying 
again non-stop.

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

Maybe instead of dropping it, reword it as "stop all submissions to the 
GuC".

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

AFAICS that patch only redirects to dummy page while the wedged flag is 
set. What happens after the FLR is completed and we've removed the 
wedged flag? If we've dropped the mapping to the memory, where is that 
access going to go?

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

Applications don't usually check their memory to see if it is still 
good. Are we expecting them to start doing this? or are we expecting all 
memory to get thrown out every time an application gets an -ECANCELED error?
In either case I'd like an ack from the UMD teams on this.

Daniele

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