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

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

<snip>

I haven't gone through the code yet, but I wanted to ask some questions 
regarding the approach first.

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

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

> + * - Stop accepting new submissions

This is done as part of the above step and it isn't a separate one, right?

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

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

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

Daniele

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