* [PATCH v3 0/4] drm/xe: Support PCIe FLR
@ 2024-04-19 8:58 Aravind Iddamsetty
2024-04-19 8:58 ` [PATCH v2 1/4] drm: add devm release action Aravind Iddamsetty
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Aravind Iddamsetty @ 2024-04-19 8:58 UTC (permalink / raw)
To: dri-devel, daniel, maarten.lankhorst, airlied, mripard,
tzimmermann, rodrigo.vivi
Cc: intel-xe, Lucas De Marchi
PCI subsystem provides callbacks to inform the driver about a request to
do function level reset by user, initiated by writing to sysfs entry
/sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
without the need to do unbind and rebind as the driver needs to
reinitialize the device afresh post FLR.
v3: Trivial fix
v2:
1. Directly expose the devm_drm_dev_release_action instead of introducing
a helper (Rodrigo)
2. separate out gt idle and pci save/restore to a separate patch (Lucas)
3. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
Rodrigo, retaining your r-b's since there are no functional changes.
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
dmesg snip showing FLR recovery:
[ 590.486336] xe 0000:4d:00.0: enabling device (0140 -> 0142)
[ 590.506933] xe 0000:4d:00.0: [drm] Using GuC firmware from
xe/pvc_guc_70.20.0.bin version 70.20.0
[ 590.542355] xe 0000:4d:00.0: [drm] Using GuC firmware from
xe/pvc_guc_70.20.0.bin version 70.20.0
[ 590.578532] xe 0000:4d:00.0: [drm] VISIBLE VRAM: 0x0000202000000000,
0x0000002000000000
[ 590.578556] xe 0000:4d:00.0: [drm] VRAM[0, 0]: Actual physical size
0x0000001000000000, usable size exclude stolen 0x0000000fff000000, CPU
accessible size 0x0000000fff000000
[ 590.578560] xe 0000:4d:00.0: [drm] VRAM[0, 0]: DPA range:
[0x0000000000000000-1000000000], io range:
[0x0000202000000000-202fff000000]
[ 590.578585] xe 0000:4d:00.0: [drm] VRAM[1, 1]: Actual physical size
0x0000001000000000, usable size exclude stolen 0x0000000fff000000, CPU
accessible size 0x0000000fff000000
[ 590.578589] xe 0000:4d:00.0: [drm] VRAM[1, 1]: DPA range:
[0x0000001000000000-2000000000], io range:
[0x0000203000000000-203fff000000]
[ 590.578592] xe 0000:4d:00.0: [drm] Total VRAM: 0x0000202000000000,
0x0000002000000000
[ 590.578594] xe 0000:4d:00.0: [drm] Available VRAM:
0x0000202000000000, 0x0000001ffe000000
[ 590.738899] xe 0000:4d:00.0: [drm] GT0: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 590.889991] xe 0000:4d:00.0: [drm] GT1: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 590.892835] [drm] Initialized xe 1.1.0 20201103 for 0000:4d:00.0 on
minor 1
[ 590.900215] xe 0000:9a:00.0: enabling device (0140 -> 0142)
[ 590.915991] xe 0000:9a:00.0: [drm] Using GuC firmware from
xe/pvc_guc_70.20.0.bin version 70.20.0
[ 590.957450] xe 0000:9a:00.0: [drm] Using GuC firmware from
xe/pvc_guc_70.20.0.bin version 70.20.0
[ 590.989863] xe 0000:9a:00.0: [drm] VISIBLE VRAM: 0x000020e000000000,
0x0000002000000000
[ 590.989888] xe 0000:9a:00.0: [drm] VRAM[0, 0]: Actual physical size
0x0000001000000000, usable size exclude stolen 0x0000000fff000000, CPU
accessible size 0x0000000fff000000
[ 590.989893] xe 0000:9a:00.0: [drm] VRAM[0, 0]: DPA range:
[0x0000000000000000-1000000000], io range:
[0x000020e000000000-20efff000000]
[ 590.989918] xe 0000:9a:00.0: [drm] VRAM[1, 1]: Actual physical size
0x0000001000000000, usable size exclude stolen 0x0000000fff000000, CPU
accessible size 0x0000000fff000000
[ 590.989921] xe 0000:9a:00.0: [drm] VRAM[1, 1]: DPA range:
[0x0000001000000000-2000000000], io range:
[0x000020f000000000-20ffff000000]
[ 590.989924] xe 0000:9a:00.0: [drm] Total VRAM: 0x000020e000000000,
0x0000002000000000
[ 590.989927] xe 0000:9a:00.0: [drm] Available VRAM:
0x000020e000000000, 0x0000001ffe000000
[ 591.142061] xe 0000:9a:00.0: [drm] GT0: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 591.293505] xe 0000:9a:00.0: [drm] GT1: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 591.295487] [drm] Initialized xe 1.1.0 20201103 for 0000:9a:00.0 on
minor 2
[ 610.685993] Console: switching to colour dummy device 80x25
[ 610.686118] [IGT] xe_exec_basic: executing
[ 610.755398] xe 0000:4d:00.0: [drm] GT0: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 610.771783] xe 0000:4d:00.0: [drm] GT1: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 610.773542] [IGT] xe_exec_basic: starting subtest once-basic
[ 610.960251] [IGT] xe_exec_basic: finished subtest once-basic, SUCCESS
[ 610.962741] [IGT] xe_exec_basic: exiting, ret=0
[ 610.977203] Console: switching to colour frame buffer device 128x48
[ 611.006675] xe_exec_basic (3237) used greatest stack depth: 11128
bytes left
[ 644.682201] xe 0000:4d:00.0: [drm] GT0: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 644.699060] xe 0000:4d:00.0: [drm] GT1: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 644.699118] xe 0000:4d:00.0: preparing for PCIe FLR reset
[ 644.699149] xe 0000:4d:00.0: [drm] removing device access to
userspace
[ 644.928577] xe 0000:4d:00.0: PCI device went through FLR, reenabling
the device
[ 656.104233] xe 0000:4d:00.0: [drm] Using GuC firmware from
xe/pvc_guc_70.20.0.bin version 70.20.0
[ 656.149525] xe 0000:4d:00.0: [drm] Using GuC firmware from
xe/pvc_guc_70.20.0.bin version 70.20.0
[ 656.182711] xe 0000:4d:00.0: [drm] VISIBLE VRAM: 0x0000202000000000,
0x0000002000000000
[ 656.182737] xe 0000:4d:00.0: [drm] VRAM[0, 0]: Actual physical size
0x0000001000000000, usable size exclude stolen 0x0000000fff000000, CPU
accessible size 0x0000000fff000000
[ 656.182742] xe 0000:4d:00.0: [drm] VRAM[0, 0]: DPA range:
[0x0000000000000000-1000000000], io range:
[0x0000202000000000-202fff000000]
[ 656.182768] xe 0000:4d:00.0: [drm] VRAM[1, 1]: Actual physical size
0x0000001000000000, usable size exclude stolen 0x0000000fff000000, CPU
accessible size 0x0000000fff000000
[ 656.182772] xe 0000:4d:00.0: [drm] VRAM[1, 1]: DPA range:
[0x0000001000000000-2000000000], io range:
[0x0000203000000000-203fff000000]
[ 656.182775] xe 0000:4d:00.0: [drm] Total VRAM: 0x0000202000000000,
0x0000002000000000
[ 656.182778] xe 0000:4d:00.0: [drm] Available VRAM:
0x0000202000000000, 0x0000001ffe000000
[ 656.348657] xe 0000:4d:00.0: [drm] GT0: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 656.507619] xe 0000:4d:00.0: [drm] GT1: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 656.510848] [drm] Initialized xe 1.1.0 20201103 for 0000:4d:00.0 on
minor 1
[ 665.754402] Console: switching to colour dummy device 80x25
[ 665.754484] [IGT] xe_exec_basic: executing
[ 665.805853] xe 0000:4d:00.0: [drm] GT0: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 665.819825] xe 0000:4d:00.0: [drm] GT1: CCS_MODE=0 config:00400000,
num_engines:1, num_slices:4
[ 665.820359] [IGT] xe_exec_basic: starting subtest once-basic
[ 665.968899] [IGT] xe_exec_basic: finished subtest once-basic, SUCCESS
[ 665.969534] [IGT] xe_exec_basic: exiting, ret=0
[ 665.981027] Console: switching to colour frame buffer device 128x48
Aravind Iddamsetty (4):
drm: add devm release action
drm/xe: Save and restore PCI state
drm/xe: Extract xe_gt_idle() helper
drm/xe/FLR: Support PCIe FLR
drivers/gpu/drm/drm_drv.c | 6 ++
drivers/gpu/drm/xe/Makefile | 1 +
drivers/gpu/drm/xe/xe_device_types.h | 6 ++
drivers/gpu/drm/xe/xe_gt.c | 28 +++++----
drivers/gpu/drm/xe/xe_gt.h | 1 +
drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++
drivers/gpu/drm/xe/xe_pci.c | 57 ++++++++++++++++--
drivers/gpu/drm/xe/xe_pci.h | 6 +-
drivers/gpu/drm/xe/xe_pci_err.c | 88 ++++++++++++++++++++++++++++
drivers/gpu/drm/xe/xe_pci_err.h | 13 ++++
include/drm/drm_drv.h | 2 +
11 files changed, 197 insertions(+), 15 deletions(-)
create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
--
2.25.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/4] drm: add devm release action
2024-04-19 8:58 [PATCH v3 0/4] drm/xe: Support PCIe FLR Aravind Iddamsetty
@ 2024-04-19 8:58 ` Aravind Iddamsetty
2024-04-19 9:11 ` Maxime Ripard
2024-04-19 8:58 ` [PATCH 2/4] drm/xe: Save and restore PCI state Aravind Iddamsetty
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Aravind Iddamsetty @ 2024-04-19 8:58 UTC (permalink / raw)
To: dri-devel, daniel, maarten.lankhorst, airlied, mripard,
tzimmermann, rodrigo.vivi
Cc: intel-xe, Thomas Hellstr_m
In scenarios where drm_dev_put is directly called by driver we want to
release devm_drm_dev_init_release action associated with struct
drm_device.
v2: Directly expose the original function, instead of introducing a
helper (Rodrigo)
Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
---
drivers/gpu/drm/drm_drv.c | 6 ++++++
include/drm/drm_drv.h | 2 ++
2 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 243cacb3575c..ba60cbb0725f 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -714,6 +714,12 @@ static int devm_drm_dev_init(struct device *parent,
devm_drm_dev_init_release, dev);
}
+void devm_drm_dev_release_action(struct drm_device *dev)
+{
+ devm_release_action(dev->dev, devm_drm_dev_init_release, dev);
+}
+EXPORT_SYMBOL(devm_drm_dev_release_action);
+
void *__devm_drm_dev_alloc(struct device *parent,
const struct drm_driver *driver,
size_t size, size_t offset)
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 8878260d7529..fa9123684874 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -444,6 +444,8 @@ struct drm_driver {
const struct file_operations *fops;
};
+void devm_drm_dev_release_action(struct drm_device *dev);
+
void *__devm_drm_dev_alloc(struct device *parent,
const struct drm_driver *driver,
size_t size, size_t offset);
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/4] drm/xe: Save and restore PCI state
2024-04-19 8:58 [PATCH v3 0/4] drm/xe: Support PCIe FLR Aravind Iddamsetty
2024-04-19 8:58 ` [PATCH v2 1/4] drm: add devm release action Aravind Iddamsetty
@ 2024-04-19 8:58 ` Aravind Iddamsetty
2024-04-19 8:58 ` [PATCH v2 3/4] drm/xe: Extract xe_gt_idle() helper Aravind Iddamsetty
2024-04-19 8:58 ` [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR Aravind Iddamsetty
3 siblings, 0 replies; 18+ messages in thread
From: Aravind Iddamsetty @ 2024-04-19 8:58 UTC (permalink / raw)
To: dri-devel, daniel, maarten.lankhorst, airlied, mripard,
tzimmermann, rodrigo.vivi
Cc: intel-xe, Lucas De Marchi
Save and restore PCI states where ever needed.
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
---
drivers/gpu/drm/xe/xe_device_types.h | 3 ++
drivers/gpu/drm/xe/xe_pci.c | 48 ++++++++++++++++++++++++++--
drivers/gpu/drm/xe/xe_pci.h | 4 ++-
3 files changed, 51 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 8244b177a6a3..0a66555229e9 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -462,6 +462,9 @@ struct xe_device {
/** @needs_flr_on_fini: requests function-reset on fini */
bool needs_flr_on_fini;
+ /** @pci_state: PCI state of device */
+ struct pci_saved_state *pci_state;
+
/* private: */
#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index 65ea44672cab..fd45f235b35c 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -391,6 +391,41 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
#undef INTEL_VGA_DEVICE
+static bool xe_save_pci_state(struct pci_dev *pdev)
+{
+ struct xe_device *xe = pci_get_drvdata(pdev);
+
+ if (pci_save_state(pdev))
+ return false;
+
+ kfree(xe->pci_state);
+
+ xe->pci_state = pci_store_saved_state(pdev);
+ if (!xe->pci_state) {
+ drm_err(&xe->drm, "Failed to store PCI saved state\n");
+ return false;
+ }
+
+ return true;
+}
+
+void xe_load_pci_state(struct pci_dev *pdev)
+{
+ struct xe_device *xe = pci_get_drvdata(pdev);
+ int ret;
+
+ if (!xe->pci_state)
+ return;
+
+ ret = pci_load_saved_state(pdev, xe->pci_state);
+ if (ret) {
+ drm_warn(&xe->drm, "Failed to load PCI state err:%d\n", ret);
+ return;
+ }
+
+ pci_restore_state(pdev);
+}
+
/* is device_id present in comma separated list of ids */
static bool device_id_in_list(u16 device_id, const char *devices, bool negative)
{
@@ -696,6 +731,8 @@ static void xe_pci_remove(struct pci_dev *pdev)
xe_device_remove(xe);
xe_pm_runtime_fini(xe);
+
+ kfree(xe->pci_state);
pci_set_drvdata(pdev, NULL);
}
@@ -796,6 +833,9 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
drm_dbg(&xe->drm, "d3cold: capable=%s\n",
str_yes_no(xe->d3cold.capable));
+ if (xe_save_pci_state(pdev))
+ pci_restore_state(pdev);
+
return 0;
err_driver_cleanup:
@@ -847,7 +887,7 @@ static int xe_pci_suspend(struct device *dev)
*/
d3cold_toggle(pdev, D3COLD_ENABLE);
- pci_save_state(pdev);
+ xe_save_pci_state(pdev);
pci_disable_device(pdev);
return 0;
@@ -871,6 +911,8 @@ static int xe_pci_resume(struct device *dev)
pci_set_master(pdev);
+ xe_load_pci_state(pdev);
+
err = xe_pm_resume(pdev_to_xe_device(pdev));
if (err)
return err;
@@ -888,7 +930,7 @@ static int xe_pci_runtime_suspend(struct device *dev)
if (err)
return err;
- pci_save_state(pdev);
+ xe_save_pci_state(pdev);
if (xe->d3cold.allowed) {
d3cold_toggle(pdev, D3COLD_ENABLE);
@@ -913,7 +955,7 @@ static int xe_pci_runtime_resume(struct device *dev)
if (err)
return err;
- pci_restore_state(pdev);
+ xe_load_pci_state(pdev);
if (xe->d3cold.allowed) {
err = pci_enable_device(pdev);
diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
index 611c1209b14c..73b90a430d1f 100644
--- a/drivers/gpu/drm/xe/xe_pci.h
+++ b/drivers/gpu/drm/xe/xe_pci.h
@@ -6,7 +6,9 @@
#ifndef _XE_PCI_H_
#define _XE_PCI_H_
+struct pci_dev;
+
int xe_register_pci_driver(void);
void xe_unregister_pci_driver(void);
-
+void xe_load_pci_state(struct pci_dev *pdev);
#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 3/4] drm/xe: Extract xe_gt_idle() helper
2024-04-19 8:58 [PATCH v3 0/4] drm/xe: Support PCIe FLR Aravind Iddamsetty
2024-04-19 8:58 ` [PATCH v2 1/4] drm: add devm release action Aravind Iddamsetty
2024-04-19 8:58 ` [PATCH 2/4] drm/xe: Save and restore PCI state Aravind Iddamsetty
@ 2024-04-19 8:58 ` Aravind Iddamsetty
2024-04-23 4:21 ` Ghimiray, Himal Prasad
2024-04-19 8:58 ` [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR Aravind Iddamsetty
3 siblings, 1 reply; 18+ messages in thread
From: Aravind Iddamsetty @ 2024-04-19 8:58 UTC (permalink / raw)
To: dri-devel, daniel, maarten.lankhorst, airlied, mripard,
tzimmermann, rodrigo.vivi
Cc: intel-xe, Lucas De Marchi, Michal Wajdeczko,
Himal Prasad Ghimiray
This would be used in other places outside of gt_reset path.
v2:
1. Add kernel doc for xe_gt_idle (Michal)
2. fix return as no actual error is reported by xe_uc_stop (Himal)
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
---
drivers/gpu/drm/xe/xe_gt.c | 28 ++++++++++++++++++----------
drivers/gpu/drm/xe/xe_gt.h | 1 +
2 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
index 491d0413de15..a24340219c30 100644
--- a/drivers/gpu/drm/xe/xe_gt.c
+++ b/drivers/gpu/drm/xe/xe_gt.c
@@ -629,6 +629,23 @@ static int do_gt_restart(struct xe_gt *gt)
return 0;
}
+/**
+ * xe_gt_idle: Idle the GT.
+ * @gt: The gt object
+ *
+ * Typically called before initiating any resets.
+ *
+ */
+void xe_gt_idle(struct xe_gt *gt)
+{
+ xe_gt_sanitize(gt);
+ xe_uc_gucrc_disable(>->uc);
+ xe_uc_stop_prepare(>->uc);
+ xe_gt_pagefault_reset(gt);
+ xe_uc_stop(>->uc);
+ xe_gt_tlb_invalidation_reset(gt);
+}
+
static int gt_reset(struct xe_gt *gt)
{
int err;
@@ -645,21 +662,12 @@ static int gt_reset(struct xe_gt *gt)
}
xe_pm_runtime_get(gt_to_xe(gt));
- xe_gt_sanitize(gt);
err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
if (err)
goto err_msg;
- xe_uc_gucrc_disable(>->uc);
- xe_uc_stop_prepare(>->uc);
- xe_gt_pagefault_reset(gt);
-
- err = xe_uc_stop(>->uc);
- if (err)
- goto err_out;
-
- xe_gt_tlb_invalidation_reset(gt);
+ xe_gt_idle(gt);
err = do_gt_reset(gt);
if (err)
diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
index ed6ea8057e35..dcc1023d20e8 100644
--- a/drivers/gpu/drm/xe/xe_gt.h
+++ b/drivers/gpu/drm/xe/xe_gt.h
@@ -43,6 +43,7 @@ int xe_gt_resume(struct xe_gt *gt);
void xe_gt_reset_async(struct xe_gt *gt);
void xe_gt_sanitize(struct xe_gt *gt);
void xe_gt_remove(struct xe_gt *gt);
+void xe_gt_idle(struct xe_gt *gt);
/**
* xe_gt_any_hw_engine_by_reset_domain - scan the list of engines and return the
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
2024-04-19 8:58 [PATCH v3 0/4] drm/xe: Support PCIe FLR Aravind Iddamsetty
` (2 preceding siblings ...)
2024-04-19 8:58 ` [PATCH v2 3/4] drm/xe: Extract xe_gt_idle() helper Aravind Iddamsetty
@ 2024-04-19 8:58 ` Aravind Iddamsetty
3 siblings, 0 replies; 18+ messages in thread
From: Aravind Iddamsetty @ 2024-04-19 8:58 UTC (permalink / raw)
To: dri-devel, daniel, maarten.lankhorst, airlied, mripard,
tzimmermann, rodrigo.vivi
Cc: intel-xe, Lucas De Marchi, Michal Wajdeczko
PCI subsystem provides callbacks to inform the driver about a request to
do function level reset by user, initiated by writing to sysfs entry
/sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
without the need to do unbind and rebind as the driver needs to
reinitialize the device afresh post FLR.
v2:
1. separate out gt idle and pci save/restore to a separate patch (Lucas)
2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
v3: declare xe_pci_err_handlers as static(Michal)
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
---
drivers/gpu/drm/xe/Makefile | 1 +
drivers/gpu/drm/xe/xe_device_types.h | 3 +
drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++
drivers/gpu/drm/xe/xe_pci.c | 9 ++-
drivers/gpu/drm/xe/xe_pci.h | 2 +
drivers/gpu/drm/xe/xe_pci_err.c | 88 ++++++++++++++++++++++++++++
drivers/gpu/drm/xe/xe_pci_err.h | 13 ++++
7 files changed, 119 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 8bc62bfbc679..693971a1fac0 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -117,6 +117,7 @@ xe-y += xe_bb.o \
xe_module.o \
xe_pat.o \
xe_pci.o \
+ xe_pci_err.o \
xe_pcode.o \
xe_pm.o \
xe_preempt_fence.o \
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 0a66555229e9..8c749b378a92 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -465,6 +465,9 @@ struct xe_device {
/** @pci_state: PCI state of device */
struct pci_saved_state *pci_state;
+ /** @pci_device_is_reset: device went through PCIe FLR */
+ bool pci_device_is_reset;
+
/* private: */
#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
index 509649d0e65e..efba0fbe2f5c 100644
--- a/drivers/gpu/drm/xe/xe_guc_pc.c
+++ b/drivers/gpu/drm/xe/xe_guc_pc.c
@@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg)
return;
}
+ /* We already have done this before going through a reset, so skip here */
+ if (xe->pci_device_is_reset)
+ return;
+
XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL));
XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
XE_WARN_ON(xe_guc_pc_stop(pc));
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index fd45f235b35c..98170773a9e1 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -23,6 +23,7 @@
#include "xe_macros.h"
#include "xe_mmio.h"
#include "xe_module.h"
+#include "xe_pci_err.h"
#include "xe_pci_types.h"
#include "xe_pm.h"
#include "xe_sriov.h"
@@ -736,7 +737,7 @@ static void xe_pci_remove(struct pci_dev *pdev)
pci_set_drvdata(pdev, NULL);
}
-static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
const struct xe_device_desc *desc = (const void *)ent->driver_data;
const struct xe_subplatform_desc *subplatform_desc;
@@ -984,6 +985,11 @@ static const struct dev_pm_ops xe_pm_ops = {
};
#endif
+static const struct pci_error_handlers xe_pci_err_handlers = {
+ .reset_prepare = xe_pci_reset_prepare,
+ .reset_done = xe_pci_reset_done,
+};
+
static struct pci_driver xe_pci_driver = {
.name = DRIVER_NAME,
.id_table = pciidlist,
@@ -993,6 +999,7 @@ static struct pci_driver xe_pci_driver = {
#ifdef CONFIG_PM_SLEEP
.driver.pm = &xe_pm_ops,
#endif
+ .err_handler = &xe_pci_err_handlers,
};
int xe_register_pci_driver(void)
diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
index 73b90a430d1f..9faf5380a09e 100644
--- a/drivers/gpu/drm/xe/xe_pci.h
+++ b/drivers/gpu/drm/xe/xe_pci.h
@@ -7,8 +7,10 @@
#define _XE_PCI_H_
struct pci_dev;
+struct pci_device_id;
int xe_register_pci_driver(void);
void xe_unregister_pci_driver(void);
void xe_load_pci_state(struct pci_dev *pdev);
+int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
#endif
diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
new file mode 100644
index 000000000000..5306925ea2fa
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_pci_err.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#include <linux/pci.h>
+#include <drm/drm_drv.h>
+
+#include "xe_device.h"
+#include "xe_gt.h"
+#include "xe_gt_printk.h"
+#include "xe_pci.h"
+#include "xe_pci_err.h"
+#include "xe_pm.h"
+#include "xe_uc.h"
+
+/**
+ * xe_pci_reset_prepare - Called when user issued a PCIe reset
+ * via /sys/bus/pci/devices/.../reset.
+ * @pdev: PCI device struct
+ */
+void xe_pci_reset_prepare(struct pci_dev *pdev)
+{
+ struct xe_device *xe = pci_get_drvdata(pdev);
+ struct xe_gt *gt;
+ int id, err;
+
+ pci_warn(pdev, "preparing for PCIe reset\n");
+
+ drm_warn(&xe->drm, "removing device access to userspace\n");
+ drm_dev_unplug(&xe->drm);
+
+ xe_pm_runtime_get(xe);
+ /* idle the GTs */
+ for_each_gt(gt, xe, id) {
+ err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
+ if (err)
+ goto reset;
+ xe_uc_reset_prepare(>->uc);
+ xe_gt_idle(gt);
+ err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
+ XE_WARN_ON(err);
+ }
+ xe_pm_runtime_put(xe);
+
+reset:
+ pci_disable_device(pdev);
+}
+
+/**
+ * xe_pci_reset_done - Called when PCIe reset is done.
+ * @pdev: PCI device struct
+ */
+void xe_pci_reset_done(struct pci_dev *pdev)
+{
+ const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
+ struct xe_device *xe = pci_get_drvdata(pdev);
+
+ dev_info(&pdev->dev,
+ "device went through PCIe reset, reenabling the device\n");
+
+ if (pci_enable_device(pdev)) {
+ dev_err(&pdev->dev,
+ "Cannot re-enable PCI device after reset\n");
+ return;
+ }
+ pci_set_master(pdev);
+ xe_load_pci_state(pdev);
+
+ xe->pci_device_is_reset = true;
+ /*
+ * We want to completely clean the driver and even destroy
+ * the xe private data and reinitialize afresh similar to
+ * probe
+ */
+ pdev->driver->remove(pdev);
+ if (pci_dev_msi_enabled(pdev))
+ pci_free_irq_vectors(pdev);
+
+ devm_drm_dev_release_action(&xe->drm);
+ pci_disable_device(pdev);
+
+ /*
+ * if this fails the driver might be in a stale state, only option is
+ * to unbind and rebind
+ */
+ xe_pci_probe(pdev, ent);
+}
diff --git a/drivers/gpu/drm/xe/xe_pci_err.h b/drivers/gpu/drm/xe/xe_pci_err.h
new file mode 100644
index 000000000000..95a4c8ce9cf1
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_pci_err.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#ifndef _XE_PCI_ERR_H_
+#define _XE_PCI_ERR_H_
+
+struct pci_dev;
+
+void xe_pci_reset_prepare(struct pci_dev *pdev);
+void xe_pci_reset_done(struct pci_dev *pdev);
+#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] drm: add devm release action
2024-04-19 8:58 ` [PATCH v2 1/4] drm: add devm release action Aravind Iddamsetty
@ 2024-04-19 9:11 ` Maxime Ripard
2024-04-20 8:12 ` Aravind Iddamsetty
0 siblings, 1 reply; 18+ messages in thread
From: Maxime Ripard @ 2024-04-19 9:11 UTC (permalink / raw)
To: Aravind Iddamsetty
Cc: dri-devel, daniel, maarten.lankhorst, airlied, tzimmermann,
rodrigo.vivi, intel-xe, Thomas Hellstr_m
[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]
On Fri, Apr 19, 2024 at 02:28:23PM +0530, Aravind Iddamsetty wrote:
> In scenarios where drm_dev_put is directly called by driver we want to
> release devm_drm_dev_init_release action associated with struct
> drm_device.
>
> v2: Directly expose the original function, instead of introducing a
> helper (Rodrigo)
>
> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> ---
> drivers/gpu/drm/drm_drv.c | 6 ++++++
> include/drm/drm_drv.h | 2 ++
> 2 files changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 243cacb3575c..ba60cbb0725f 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -714,6 +714,12 @@ static int devm_drm_dev_init(struct device *parent,
> devm_drm_dev_init_release, dev);
> }
>
> +void devm_drm_dev_release_action(struct drm_device *dev)
> +{
> + devm_release_action(dev->dev, devm_drm_dev_init_release, dev);
> +}
> +EXPORT_SYMBOL(devm_drm_dev_release_action);
Again, this needs to be documented.
Maxime
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/4] drm: add devm release action
2024-04-19 9:11 ` Maxime Ripard
@ 2024-04-20 8:12 ` Aravind Iddamsetty
0 siblings, 0 replies; 18+ messages in thread
From: Aravind Iddamsetty @ 2024-04-20 8:12 UTC (permalink / raw)
To: Maxime Ripard
Cc: dri-devel, daniel, maarten.lankhorst, airlied, tzimmermann,
rodrigo.vivi, intel-xe, Thomas Hellstr_m
On 19/04/24 14:41, Maxime Ripard wrote:
> On Fri, Apr 19, 2024 at 02:28:23PM +0530, Aravind Iddamsetty wrote:
>> In scenarios where drm_dev_put is directly called by driver we want to
>> release devm_drm_dev_init_release action associated with struct
>> drm_device.
>>
>> v2: Directly expose the original function, instead of introducing a
>> helper (Rodrigo)
>>
>> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>
>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>> ---
>> drivers/gpu/drm/drm_drv.c | 6 ++++++
>> include/drm/drm_drv.h | 2 ++
>> 2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 243cacb3575c..ba60cbb0725f 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -714,6 +714,12 @@ static int devm_drm_dev_init(struct device *parent,
>> devm_drm_dev_init_release, dev);
>> }
>>
>> +void devm_drm_dev_release_action(struct drm_device *dev)
>> +{
>> + devm_release_action(dev->dev, devm_drm_dev_init_release, dev);
>> +}
>> +EXPORT_SYMBOL(devm_drm_dev_release_action);
> Again, this needs to be documented.
sorry I missed your earlier email, will address this.
Thanks,
Aravind.
>
> Maxime
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
2024-04-22 6:57 [PATCH v4 0/4] drm/xe: " Aravind Iddamsetty
@ 2024-04-22 6:57 ` Aravind Iddamsetty
2024-04-23 15:04 ` Nilawar, Badal
2024-04-23 23:49 ` Michał Winiarski
0 siblings, 2 replies; 18+ messages in thread
From: Aravind Iddamsetty @ 2024-04-22 6:57 UTC (permalink / raw)
To: dri-devel, daniel, maarten.lankhorst, airlied, mripard,
tzimmermann, rodrigo.vivi
Cc: intel-xe, Lucas De Marchi, Michal Wajdeczko
PCI subsystem provides callbacks to inform the driver about a request to
do function level reset by user, initiated by writing to sysfs entry
/sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
without the need to do unbind and rebind as the driver needs to
reinitialize the device afresh post FLR.
v2:
1. separate out gt idle and pci save/restore to a separate patch (Lucas)
2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
v3: declare xe_pci_err_handlers as static(Michal)
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
---
drivers/gpu/drm/xe/Makefile | 1 +
drivers/gpu/drm/xe/xe_device_types.h | 3 +
drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++
drivers/gpu/drm/xe/xe_pci.c | 9 ++-
drivers/gpu/drm/xe/xe_pci.h | 2 +
drivers/gpu/drm/xe/xe_pci_err.c | 88 ++++++++++++++++++++++++++++
drivers/gpu/drm/xe/xe_pci_err.h | 13 ++++
7 files changed, 119 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 8bc62bfbc679..693971a1fac0 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -117,6 +117,7 @@ xe-y += xe_bb.o \
xe_module.o \
xe_pat.o \
xe_pci.o \
+ xe_pci_err.o \
xe_pcode.o \
xe_pm.o \
xe_preempt_fence.o \
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 0a66555229e9..8c749b378a92 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -465,6 +465,9 @@ struct xe_device {
/** @pci_state: PCI state of device */
struct pci_saved_state *pci_state;
+ /** @pci_device_is_reset: device went through PCIe FLR */
+ bool pci_device_is_reset;
+
/* private: */
#if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
index 509649d0e65e..efba0fbe2f5c 100644
--- a/drivers/gpu/drm/xe/xe_guc_pc.c
+++ b/drivers/gpu/drm/xe/xe_guc_pc.c
@@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg)
return;
}
+ /* We already have done this before going through a reset, so skip here */
+ if (xe->pci_device_is_reset)
+ return;
+
XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL));
XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
XE_WARN_ON(xe_guc_pc_stop(pc));
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index a62300990e19..b5a582afc9e7 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -23,6 +23,7 @@
#include "xe_macros.h"
#include "xe_mmio.h"
#include "xe_module.h"
+#include "xe_pci_err.h"
#include "xe_pci_types.h"
#include "xe_pm.h"
#include "xe_sriov.h"
@@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev)
pci_set_drvdata(pdev, NULL);
}
-static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
const struct xe_device_desc *desc = (const void *)ent->driver_data;
const struct xe_subplatform_desc *subplatform_desc;
@@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = {
};
#endif
+static const struct pci_error_handlers xe_pci_err_handlers = {
+ .reset_prepare = xe_pci_reset_prepare,
+ .reset_done = xe_pci_reset_done,
+};
+
static struct pci_driver xe_pci_driver = {
.name = DRIVER_NAME,
.id_table = pciidlist,
@@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = {
#ifdef CONFIG_PM_SLEEP
.driver.pm = &xe_pm_ops,
#endif
+ .err_handler = &xe_pci_err_handlers,
};
int xe_register_pci_driver(void)
diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
index 73b90a430d1f..9faf5380a09e 100644
--- a/drivers/gpu/drm/xe/xe_pci.h
+++ b/drivers/gpu/drm/xe/xe_pci.h
@@ -7,8 +7,10 @@
#define _XE_PCI_H_
struct pci_dev;
+struct pci_device_id;
int xe_register_pci_driver(void);
void xe_unregister_pci_driver(void);
void xe_load_pci_state(struct pci_dev *pdev);
+int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
#endif
diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
new file mode 100644
index 000000000000..5306925ea2fa
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_pci_err.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#include <linux/pci.h>
+#include <drm/drm_drv.h>
+
+#include "xe_device.h"
+#include "xe_gt.h"
+#include "xe_gt_printk.h"
+#include "xe_pci.h"
+#include "xe_pci_err.h"
+#include "xe_pm.h"
+#include "xe_uc.h"
+
+/**
+ * xe_pci_reset_prepare - Called when user issued a PCIe reset
+ * via /sys/bus/pci/devices/.../reset.
+ * @pdev: PCI device struct
+ */
+void xe_pci_reset_prepare(struct pci_dev *pdev)
+{
+ struct xe_device *xe = pci_get_drvdata(pdev);
+ struct xe_gt *gt;
+ int id, err;
+
+ pci_warn(pdev, "preparing for PCIe reset\n");
+
+ drm_warn(&xe->drm, "removing device access to userspace\n");
+ drm_dev_unplug(&xe->drm);
+
+ xe_pm_runtime_get(xe);
+ /* idle the GTs */
+ for_each_gt(gt, xe, id) {
+ err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
+ if (err)
+ goto reset;
+ xe_uc_reset_prepare(>->uc);
+ xe_gt_idle(gt);
+ err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
+ XE_WARN_ON(err);
+ }
+ xe_pm_runtime_put(xe);
+
+reset:
+ pci_disable_device(pdev);
+}
+
+/**
+ * xe_pci_reset_done - Called when PCIe reset is done.
+ * @pdev: PCI device struct
+ */
+void xe_pci_reset_done(struct pci_dev *pdev)
+{
+ const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
+ struct xe_device *xe = pci_get_drvdata(pdev);
+
+ dev_info(&pdev->dev,
+ "device went through PCIe reset, reenabling the device\n");
+
+ if (pci_enable_device(pdev)) {
+ dev_err(&pdev->dev,
+ "Cannot re-enable PCI device after reset\n");
+ return;
+ }
+ pci_set_master(pdev);
+ xe_load_pci_state(pdev);
+
+ xe->pci_device_is_reset = true;
+ /*
+ * We want to completely clean the driver and even destroy
+ * the xe private data and reinitialize afresh similar to
+ * probe
+ */
+ pdev->driver->remove(pdev);
+ if (pci_dev_msi_enabled(pdev))
+ pci_free_irq_vectors(pdev);
+
+ devm_drm_dev_release_action(&xe->drm);
+ pci_disable_device(pdev);
+
+ /*
+ * if this fails the driver might be in a stale state, only option is
+ * to unbind and rebind
+ */
+ xe_pci_probe(pdev, ent);
+}
diff --git a/drivers/gpu/drm/xe/xe_pci_err.h b/drivers/gpu/drm/xe/xe_pci_err.h
new file mode 100644
index 000000000000..95a4c8ce9cf1
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_pci_err.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2024 Intel Corporation
+ */
+
+#ifndef _XE_PCI_ERR_H_
+#define _XE_PCI_ERR_H_
+
+struct pci_dev;
+
+void xe_pci_reset_prepare(struct pci_dev *pdev);
+void xe_pci_reset_done(struct pci_dev *pdev);
+#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/4] drm/xe: Extract xe_gt_idle() helper
2024-04-19 8:58 ` [PATCH v2 3/4] drm/xe: Extract xe_gt_idle() helper Aravind Iddamsetty
@ 2024-04-23 4:21 ` Ghimiray, Himal Prasad
0 siblings, 0 replies; 18+ messages in thread
From: Ghimiray, Himal Prasad @ 2024-04-23 4:21 UTC (permalink / raw)
To: Aravind Iddamsetty, dri-devel, daniel, maarten.lankhorst, airlied,
mripard, tzimmermann, rodrigo.vivi
Cc: intel-xe, Lucas De Marchi, Michal Wajdeczko
[-- Attachment #1: Type: text/plain, Size: 2596 bytes --]
On 19-04-2024 14:28, Aravind Iddamsetty wrote:
> This would be used in other places outside of gt_reset path.
>
> v2:
> 1. Add kernel doc for xe_gt_idle (Michal)
> 2. fix return as no actual error is reported by xe_uc_stop (Himal)
>
> Cc: Lucas De Marchi<lucas.demarchi@intel.com>
> Cc: Michal Wajdeczko<michal.wajdeczko@intel.com>
> Cc: Himal Prasad Ghimiray<himal.prasad.ghimiray@intel.com>
>
> Reviewed-by: Rodrigo Vivi<rodrigo.vivi@intel.com>
> Signed-off-by: Aravind Iddamsetty<aravind.iddamsetty@linux.intel.com>
> ---
> drivers/gpu/drm/xe/xe_gt.c | 28 ++++++++++++++++++----------
> drivers/gpu/drm/xe/xe_gt.h | 1 +
> 2 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index 491d0413de15..a24340219c30 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -629,6 +629,23 @@ static int do_gt_restart(struct xe_gt *gt)
> return 0;
> }
>
> +/**
> + * xe_gt_idle: Idle the GT.
> + * @gt: The gt object
> + *
> + * Typically called before initiating any resets.
> + *
> + */
> +void xe_gt_idle(struct xe_gt *gt)
> +{
> + xe_gt_sanitize(gt);
> + xe_uc_gucrc_disable(>->uc);
> + xe_uc_stop_prepare(>->uc);
> + xe_gt_pagefault_reset(gt);
> + xe_uc_stop(>->uc);
The cleanup of xe_uc_stop to return void needs to be addressed
independently of the patch.
This patch LGTM
Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> + xe_gt_tlb_invalidation_reset(gt);
> +}
> +
> static int gt_reset(struct xe_gt *gt)
> {
> int err;
> @@ -645,21 +662,12 @@ static int gt_reset(struct xe_gt *gt)
> }
>
> xe_pm_runtime_get(gt_to_xe(gt));
> - xe_gt_sanitize(gt);
>
> err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> if (err)
> goto err_msg;
>
> - xe_uc_gucrc_disable(>->uc);
> - xe_uc_stop_prepare(>->uc);
> - xe_gt_pagefault_reset(gt);
> -
> - err = xe_uc_stop(>->uc);
> - if (err)
> - goto err_out;
> -
> - xe_gt_tlb_invalidation_reset(gt);
> + xe_gt_idle(gt);
>
> err = do_gt_reset(gt);
> if (err)
> diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
> index ed6ea8057e35..dcc1023d20e8 100644
> --- a/drivers/gpu/drm/xe/xe_gt.h
> +++ b/drivers/gpu/drm/xe/xe_gt.h
> @@ -43,6 +43,7 @@ int xe_gt_resume(struct xe_gt *gt);
> void xe_gt_reset_async(struct xe_gt *gt);
> void xe_gt_sanitize(struct xe_gt *gt);
> void xe_gt_remove(struct xe_gt *gt);
> +void xe_gt_idle(struct xe_gt *gt);
>
> /**
> * xe_gt_any_hw_engine_by_reset_domain - scan the list of engines and return the
[-- Attachment #2: Type: text/html, Size: 39872 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
2024-04-22 6:57 ` [PATCH v3 4/4] drm/xe/FLR: " Aravind Iddamsetty
@ 2024-04-23 15:04 ` Nilawar, Badal
2024-04-24 3:12 ` Aravind Iddamsetty
2024-04-23 23:49 ` Michał Winiarski
1 sibling, 1 reply; 18+ messages in thread
From: Nilawar, Badal @ 2024-04-23 15:04 UTC (permalink / raw)
To: Aravind Iddamsetty, dri-devel, daniel, maarten.lankhorst, airlied,
mripard, tzimmermann, rodrigo.vivi
Cc: intel-xe, Lucas De Marchi, Michal Wajdeczko
On 22-04-2024 12:27, Aravind Iddamsetty wrote:
> PCI subsystem provides callbacks to inform the driver about a request to
> do function level reset by user, initiated by writing to sysfs entry
> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
> without the need to do unbind and rebind as the driver needs to
> reinitialize the device afresh post FLR.
>
> v2:
> 1. separate out gt idle and pci save/restore to a separate patch (Lucas)
> 2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
>
> v3: declare xe_pci_err_handlers as static(Michal)
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> ---
> drivers/gpu/drm/xe/Makefile | 1 +
> drivers/gpu/drm/xe/xe_device_types.h | 3 +
> drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++
> drivers/gpu/drm/xe/xe_pci.c | 9 ++-
> drivers/gpu/drm/xe/xe_pci.h | 2 +
> drivers/gpu/drm/xe/xe_pci_err.c | 88 ++++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_pci_err.h | 13 ++++
> 7 files changed, 119 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
>
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 8bc62bfbc679..693971a1fac0 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
> xe_module.o \
> xe_pat.o \
> xe_pci.o \
> + xe_pci_err.o \
> xe_pcode.o \
> xe_pm.o \
> xe_preempt_fence.o \
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 0a66555229e9..8c749b378a92 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -465,6 +465,9 @@ struct xe_device {
> /** @pci_state: PCI state of device */
> struct pci_saved_state *pci_state;
>
> + /** @pci_device_is_reset: device went through PCIe FLR */
> + bool pci_device_is_reset;
> +
> /* private: */
>
> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index 509649d0e65e..efba0fbe2f5c 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg)
> return;
> }
>
> + /* We already have done this before going through a reset, so skip here */
> + if (xe->pci_device_is_reset)
> + return;
> +
> XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL));
> XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
> XE_WARN_ON(xe_guc_pc_stop(pc));
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index a62300990e19..b5a582afc9e7 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -23,6 +23,7 @@
> #include "xe_macros.h"
> #include "xe_mmio.h"
> #include "xe_module.h"
> +#include "xe_pci_err.h"
> #include "xe_pci_types.h"
> #include "xe_pm.h"
> #include "xe_sriov.h"
> @@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev)
> pci_set_drvdata(pdev, NULL);
> }
>
> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> const struct xe_device_desc *desc = (const void *)ent->driver_data;
> const struct xe_subplatform_desc *subplatform_desc;
> @@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = {
> };
> #endif
>
> +static const struct pci_error_handlers xe_pci_err_handlers = {
> + .reset_prepare = xe_pci_reset_prepare,
> + .reset_done = xe_pci_reset_done,
> +};
> +
> static struct pci_driver xe_pci_driver = {
> .name = DRIVER_NAME,
> .id_table = pciidlist,
> @@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = {
> #ifdef CONFIG_PM_SLEEP
> .driver.pm = &xe_pm_ops,
> #endif
> + .err_handler = &xe_pci_err_handlers,
> };
>
> int xe_register_pci_driver(void)
> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
> index 73b90a430d1f..9faf5380a09e 100644
> --- a/drivers/gpu/drm/xe/xe_pci.h
> +++ b/drivers/gpu/drm/xe/xe_pci.h
> @@ -7,8 +7,10 @@
> #define _XE_PCI_H_
>
> struct pci_dev;
> +struct pci_device_id;
>
> int xe_register_pci_driver(void);
> void xe_unregister_pci_driver(void);
> void xe_load_pci_state(struct pci_dev *pdev);
> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
> #endif
> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
> new file mode 100644
> index 000000000000..5306925ea2fa
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include <linux/pci.h>
> +#include <drm/drm_drv.h>
> +
> +#include "xe_device.h"
> +#include "xe_gt.h"
> +#include "xe_gt_printk.h"
> +#include "xe_pci.h"
> +#include "xe_pci_err.h"
> +#include "xe_pm.h"
> +#include "xe_uc.h"
> +
> +/**
> + * xe_pci_reset_prepare - Called when user issued a PCIe reset
> + * via /sys/bus/pci/devices/.../reset.
> + * @pdev: PCI device struct
> + */
> +void xe_pci_reset_prepare(struct pci_dev *pdev)
> +{
> + struct xe_device *xe = pci_get_drvdata(pdev);
> + struct xe_gt *gt;
> + int id, err;
> +
> + pci_warn(pdev, "preparing for PCIe reset\n");
> +
> + drm_warn(&xe->drm, "removing device access to userspace\n");
> + drm_dev_unplug(&xe->drm);
> +
> + xe_pm_runtime_get(xe);
> + /* idle the GTs */
> + for_each_gt(gt, xe, id) {
> + err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> + if (err)
> + goto reset;
> + xe_uc_reset_prepare(>->uc);
> + xe_gt_idle(gt);
> + err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> + XE_WARN_ON(err);
> + }
> + xe_pm_runtime_put(xe);
Is xe_save_pci_state() needed here?
Regards,
Badal Nilawar> +
> +reset:
> + pci_disable_device(pdev);
> +}
> +
> +/**
> + * xe_pci_reset_done - Called when PCIe reset is done.
> + * @pdev: PCI device struct
> + */
> +void xe_pci_reset_done(struct pci_dev *pdev)
> +{
> + const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
> + struct xe_device *xe = pci_get_drvdata(pdev);
> +
> + dev_info(&pdev->dev,
> + "device went through PCIe reset, reenabling the device\n");
> +
> + if (pci_enable_device(pdev)) {
> + dev_err(&pdev->dev,
> + "Cannot re-enable PCI device after reset\n");
> + return;
> + }
> + pci_set_master(pdev);
> + xe_load_pci_state(pdev);
> +
> + xe->pci_device_is_reset = true;
> + /*
> + * We want to completely clean the driver and even destroy
> + * the xe private data and reinitialize afresh similar to
> + * probe
> + */
> + pdev->driver->remove(pdev);
> + if (pci_dev_msi_enabled(pdev))
> + pci_free_irq_vectors(pdev);
> +
> + devm_drm_dev_release_action(&xe->drm);
> + pci_disable_device(pdev);
> +
> + /*
> + * if this fails the driver might be in a stale state, only option is
> + * to unbind and rebind
> + */
> + xe_pci_probe(pdev, ent);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_pci_err.h b/drivers/gpu/drm/xe/xe_pci_err.h
> new file mode 100644
> index 000000000000..95a4c8ce9cf1
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_pci_err.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef _XE_PCI_ERR_H_
> +#define _XE_PCI_ERR_H_
> +
> +struct pci_dev;
> +
> +void xe_pci_reset_prepare(struct pci_dev *pdev);
> +void xe_pci_reset_done(struct pci_dev *pdev);
> +#endif
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
2024-04-22 6:57 ` [PATCH v3 4/4] drm/xe/FLR: " Aravind Iddamsetty
2024-04-23 15:04 ` Nilawar, Badal
@ 2024-04-23 23:49 ` Michał Winiarski
2024-04-24 5:12 ` Aravind Iddamsetty
1 sibling, 1 reply; 18+ messages in thread
From: Michał Winiarski @ 2024-04-23 23:49 UTC (permalink / raw)
To: Aravind Iddamsetty
Cc: dri-devel, daniel, maarten.lankhorst, airlied, mripard,
tzimmermann, rodrigo.vivi, intel-xe, Lucas De Marchi,
Michal Wajdeczko
On Mon, Apr 22, 2024 at 12:27:56PM +0530, Aravind Iddamsetty wrote:
> PCI subsystem provides callbacks to inform the driver about a request to
> do function level reset by user, initiated by writing to sysfs entry
> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
> without the need to do unbind and rebind as the driver needs to
> reinitialize the device afresh post FLR.
>
> v2:
> 1. separate out gt idle and pci save/restore to a separate patch (Lucas)
> 2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
>
> v3: declare xe_pci_err_handlers as static(Michal)
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> ---
> drivers/gpu/drm/xe/Makefile | 1 +
> drivers/gpu/drm/xe/xe_device_types.h | 3 +
> drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++
> drivers/gpu/drm/xe/xe_pci.c | 9 ++-
> drivers/gpu/drm/xe/xe_pci.h | 2 +
> drivers/gpu/drm/xe/xe_pci_err.c | 88 ++++++++++++++++++++++++++++
> drivers/gpu/drm/xe/xe_pci_err.h | 13 ++++
> 7 files changed, 119 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
>
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index 8bc62bfbc679..693971a1fac0 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
> xe_module.o \
> xe_pat.o \
> xe_pci.o \
> + xe_pci_err.o \
> xe_pcode.o \
> xe_pm.o \
> xe_preempt_fence.o \
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 0a66555229e9..8c749b378a92 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -465,6 +465,9 @@ struct xe_device {
> /** @pci_state: PCI state of device */
> struct pci_saved_state *pci_state;
>
> + /** @pci_device_is_reset: device went through PCIe FLR */
> + bool pci_device_is_reset;
> +
> /* private: */
>
> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> index 509649d0e65e..efba0fbe2f5c 100644
> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> @@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg)
> return;
> }
>
> + /* We already have done this before going through a reset, so skip here */
> + if (xe->pci_device_is_reset)
> + return;
> +
> XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL));
> XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
> XE_WARN_ON(xe_guc_pc_stop(pc));
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index a62300990e19..b5a582afc9e7 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -23,6 +23,7 @@
> #include "xe_macros.h"
> #include "xe_mmio.h"
> #include "xe_module.h"
> +#include "xe_pci_err.h"
> #include "xe_pci_types.h"
> #include "xe_pm.h"
> #include "xe_sriov.h"
> @@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev)
> pci_set_drvdata(pdev, NULL);
> }
>
> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> {
> const struct xe_device_desc *desc = (const void *)ent->driver_data;
> const struct xe_subplatform_desc *subplatform_desc;
> @@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = {
> };
> #endif
>
> +static const struct pci_error_handlers xe_pci_err_handlers = {
> + .reset_prepare = xe_pci_reset_prepare,
> + .reset_done = xe_pci_reset_done,
> +};
> +
> static struct pci_driver xe_pci_driver = {
> .name = DRIVER_NAME,
> .id_table = pciidlist,
> @@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = {
> #ifdef CONFIG_PM_SLEEP
> .driver.pm = &xe_pm_ops,
> #endif
> + .err_handler = &xe_pci_err_handlers,
> };
>
> int xe_register_pci_driver(void)
> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
> index 73b90a430d1f..9faf5380a09e 100644
> --- a/drivers/gpu/drm/xe/xe_pci.h
> +++ b/drivers/gpu/drm/xe/xe_pci.h
> @@ -7,8 +7,10 @@
> #define _XE_PCI_H_
>
> struct pci_dev;
> +struct pci_device_id;
>
> int xe_register_pci_driver(void);
> void xe_unregister_pci_driver(void);
> void xe_load_pci_state(struct pci_dev *pdev);
> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
> #endif
> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
> new file mode 100644
> index 000000000000..5306925ea2fa
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#include <linux/pci.h>
> +#include <drm/drm_drv.h>
> +
> +#include "xe_device.h"
> +#include "xe_gt.h"
> +#include "xe_gt_printk.h"
> +#include "xe_pci.h"
> +#include "xe_pci_err.h"
> +#include "xe_pm.h"
> +#include "xe_uc.h"
> +
> +/**
> + * xe_pci_reset_prepare - Called when user issued a PCIe reset
> + * via /sys/bus/pci/devices/.../reset.
> + * @pdev: PCI device struct
> + */
> +void xe_pci_reset_prepare(struct pci_dev *pdev)
> +{
> + struct xe_device *xe = pci_get_drvdata(pdev);
> + struct xe_gt *gt;
> + int id, err;
> +
> + pci_warn(pdev, "preparing for PCIe reset\n");
> +
> + drm_warn(&xe->drm, "removing device access to userspace\n");
Warn? Shouldn't it be info?
> + drm_dev_unplug(&xe->drm);
> +
> + xe_pm_runtime_get(xe);
> + /* idle the GTs */
> + for_each_gt(gt, xe, id) {
> + err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> + if (err)
> + goto reset;
> + xe_uc_reset_prepare(>->uc);
> + xe_gt_idle(gt);
> + err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> + XE_WARN_ON(err);
> + }
> + xe_pm_runtime_put(xe);
> +
> +reset:
> + pci_disable_device(pdev);
> +}
> +
> +/**
> + * xe_pci_reset_done - Called when PCIe reset is done.
> + * @pdev: PCI device struct
> + */
> +void xe_pci_reset_done(struct pci_dev *pdev)
> +{
> + const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
> + struct xe_device *xe = pci_get_drvdata(pdev);
> +
> + dev_info(&pdev->dev,
> + "device went through PCIe reset, reenabling the device\n");
> +
> + if (pci_enable_device(pdev)) {
> + dev_err(&pdev->dev,
> + "Cannot re-enable PCI device after reset\n");
> + return;
> + }
> + pci_set_master(pdev);
> + xe_load_pci_state(pdev);
> +
> + xe->pci_device_is_reset = true;
> + /*
> + * We want to completely clean the driver and even destroy
> + * the xe private data and reinitialize afresh similar to
> + * probe
> + */
> + pdev->driver->remove(pdev);
Do we really want to do that?
First of all, that fairly unusual - none of the other PCI drivers makes
changes in the device/driver model during reset, which makes me wonder
if this is really what the user expects.
I would expect that the device is reset, but the driver is not unloaded
and previously created FDs are still valid.
Note that messing with the driver model at reset also makes it difficult
to do a proper cleanup if SR-IOV is enabled, as PCI-core expects drivers
to remove VF devices during driver->remove.
Removal of said devices depends on pci_cfg_access_lock, which is already
held for the duration of the reset (which includes calling reset_done),
which will cause a deadlock.
In current form, it also causes WARN if there are open FDs to DRM
device during reset.
[29357.277699] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:02.0/tile0' 01:06:58 [142/47263]
[29357.286158] CPU: 7 PID: 3479 Comm: bash Not tainted 6.9.0-rc5-xe+ #78
[29357.305870] Call Trace:
[29357.308342] <TASK>
[29357.310453] dump_stack_lvl+0x139/0x1d0
[29357.314305] ? __pfx_dump_stack_lvl+0x10/0x10
[29357.318680] ? __pfx__printk+0x10/0x10
[29357.322450] ? kmalloc_trace+0x1c1/0x3a0
[29357.326394] ? sysfs_create_dir_ns+0x162/0x210
[29357.330861] sysfs_create_dir_ns+0x195/0x210
[29357.335154] ? __pfx_sysfs_create_dir_ns+0x10/0x10
[29357.339965] ? strcmp+0x2f/0x60
[29357.343134] kobject_add_internal+0x2af/0x600
[29357.347517] kobject_add+0xfb/0x190
[29357.351028] ? __link_object+0x1c7/0x280
[29357.354976] ? __pfx_kobject_add+0x10/0x10
[29357.359093] ? __create_object+0x62/0x140
[29357.363111] ? rcu_is_watching+0x20/0x50
[29357.367055] ? kmalloc_trace+0x1c1/0x3a0
[29357.370998] ? xe_tile_sysfs_init+0x4b/0x100 [xe]
[29357.376016] xe_tile_sysfs_init+0x91/0x100 [xe]
[29357.380868] xe_tile_init_noalloc+0xaf/0xc0 [xe]
[29357.385936] xe_device_probe+0x60c/0x750 [xe]
[29357.390674] ? __asan_memcpy+0x40/0x70
[29357.394461] ? __drmm_add_action+0x1ac/0x210 [drm]
[29357.399413] xe_pci_probe+0x5dc/0x680 [xe]
[29357.403882] pci_reset_function+0x108/0x140
[29357.408100] reset_store+0xb5/0x120
[29357.411623] ? __pfx_reset_store+0x10/0x10
[29357.415751] ? __pfx_sysfs_kf_write+0x10/0x10
[29357.420149] kernfs_fop_write_iter+0x1b8/0x260
[29357.424620] vfs_write+0x5ce/0x660
[29357.428067] ? __pfx_vfs_write+0x10/0x10
[29357.432028] ? __rcu_read_unlock+0x60/0x90
[29357.436181] ksys_write+0xe4/0x190
[29357.439612] ? __pfx_ksys_write+0x10/0x10
[29357.443657] do_syscall_64+0x7b/0x120
[29357.447348] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[29357.452423] RIP: 0033:0x7f4f1ff14887
[29357.456030] Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
[29357.474761] RSP: 002b:00007ffe54e95068 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[29357.482343] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f4f1ff14887
[29357.489487] RDX: 0000000000000002 RSI: 0000559eb4076e30 RDI: 0000000000000001
[29357.496630] RBP: 0000559eb4076e30 R08: 0000000000000000 R09: 0000559eb4076e30
[29357.503775] R10: 0000000000000077 R11: 0000000000000246 R12: 0000000000000002
[29357.510947] R13: 00007f4f2001b780 R14: 00007f4f20017600 R15: 00007f4f20016a00
[29357.518174] </TASK>
[29357.520539] kobject: kobject_add_internal failed for tile0 with -EEXIST, don't try to register things with the same name in the same directory.
-Michał
> + if (pci_dev_msi_enabled(pdev))
> + pci_free_irq_vectors(pdev);
> +
> + devm_drm_dev_release_action(&xe->drm);
> + pci_disable_device(pdev);
> +
> + /*
> + * if this fails the driver might be in a stale state, only option is
> + * to unbind and rebind
> + */
> + xe_pci_probe(pdev, ent);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_pci_err.h b/drivers/gpu/drm/xe/xe_pci_err.h
> new file mode 100644
> index 000000000000..95a4c8ce9cf1
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_pci_err.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 Intel Corporation
> + */
> +
> +#ifndef _XE_PCI_ERR_H_
> +#define _XE_PCI_ERR_H_
> +
> +struct pci_dev;
> +
> +void xe_pci_reset_prepare(struct pci_dev *pdev);
> +void xe_pci_reset_done(struct pci_dev *pdev);
> +#endif
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
2024-04-23 15:04 ` Nilawar, Badal
@ 2024-04-24 3:12 ` Aravind Iddamsetty
2024-04-24 11:12 ` Nilawar, Badal
0 siblings, 1 reply; 18+ messages in thread
From: Aravind Iddamsetty @ 2024-04-24 3:12 UTC (permalink / raw)
To: Nilawar, Badal, dri-devel, daniel, maarten.lankhorst, airlied,
mripard, tzimmermann, rodrigo.vivi
Cc: intel-xe, Lucas De Marchi, Michal Wajdeczko
On 23/04/24 20:34, Nilawar, Badal wrote:
>
>
> On 22-04-2024 12:27, Aravind Iddamsetty wrote:
>> PCI subsystem provides callbacks to inform the driver about a request to
>> do function level reset by user, initiated by writing to sysfs entry
>> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
>> without the need to do unbind and rebind as the driver needs to
>> reinitialize the device afresh post FLR.
>>
>> v2:
>> 1. separate out gt idle and pci save/restore to a separate patch (Lucas)
>> 2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
>>
>> v3: declare xe_pci_err_handlers as static(Michal)
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>
>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>> ---
>> drivers/gpu/drm/xe/Makefile | 1 +
>> drivers/gpu/drm/xe/xe_device_types.h | 3 +
>> drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++
>> drivers/gpu/drm/xe/xe_pci.c | 9 ++-
>> drivers/gpu/drm/xe/xe_pci.h | 2 +
>> drivers/gpu/drm/xe/xe_pci_err.c | 88 ++++++++++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_pci_err.h | 13 ++++
>> 7 files changed, 119 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index 8bc62bfbc679..693971a1fac0 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
>> xe_module.o \
>> xe_pat.o \
>> xe_pci.o \
>> + xe_pci_err.o \
>> xe_pcode.o \
>> xe_pm.o \
>> xe_preempt_fence.o \
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index 0a66555229e9..8c749b378a92 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -465,6 +465,9 @@ struct xe_device {
>> /** @pci_state: PCI state of device */
>> struct pci_saved_state *pci_state;
>> + /** @pci_device_is_reset: device went through PCIe FLR */
>> + bool pci_device_is_reset;
>> +
>> /* private: */
>> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
>> index 509649d0e65e..efba0fbe2f5c 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> @@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg)
>> return;
>> }
>> + /* We already have done this before going through a reset, so skip here */
>> + if (xe->pci_device_is_reset)
>> + return;
>> +
>> XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL));
>> XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
>> XE_WARN_ON(xe_guc_pc_stop(pc));
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index a62300990e19..b5a582afc9e7 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -23,6 +23,7 @@
>> #include "xe_macros.h"
>> #include "xe_mmio.h"
>> #include "xe_module.h"
>> +#include "xe_pci_err.h"
>> #include "xe_pci_types.h"
>> #include "xe_pm.h"
>> #include "xe_sriov.h"
>> @@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev)
>> pci_set_drvdata(pdev, NULL);
>> }
>> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> {
>> const struct xe_device_desc *desc = (const void *)ent->driver_data;
>> const struct xe_subplatform_desc *subplatform_desc;
>> @@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = {
>> };
>> #endif
>> +static const struct pci_error_handlers xe_pci_err_handlers = {
>> + .reset_prepare = xe_pci_reset_prepare,
>> + .reset_done = xe_pci_reset_done,
>> +};
>> +
>> static struct pci_driver xe_pci_driver = {
>> .name = DRIVER_NAME,
>> .id_table = pciidlist,
>> @@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = {
>> #ifdef CONFIG_PM_SLEEP
>> .driver.pm = &xe_pm_ops,
>> #endif
>> + .err_handler = &xe_pci_err_handlers,
>> };
>> int xe_register_pci_driver(void)
>> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
>> index 73b90a430d1f..9faf5380a09e 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.h
>> +++ b/drivers/gpu/drm/xe/xe_pci.h
>> @@ -7,8 +7,10 @@
>> #define _XE_PCI_H_
>> struct pci_dev;
>> +struct pci_device_id;
>> int xe_register_pci_driver(void);
>> void xe_unregister_pci_driver(void);
>> void xe_load_pci_state(struct pci_dev *pdev);
>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
>> #endif
>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
>> new file mode 100644
>> index 000000000000..5306925ea2fa
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
>> @@ -0,0 +1,88 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#include <linux/pci.h>
>> +#include <drm/drm_drv.h>
>> +
>> +#include "xe_device.h"
>> +#include "xe_gt.h"
>> +#include "xe_gt_printk.h"
>> +#include "xe_pci.h"
>> +#include "xe_pci_err.h"
>> +#include "xe_pm.h"
>> +#include "xe_uc.h"
>> +
>> +/**
>> + * xe_pci_reset_prepare - Called when user issued a PCIe reset
>> + * via /sys/bus/pci/devices/.../reset.
>> + * @pdev: PCI device struct
>> + */
>> +void xe_pci_reset_prepare(struct pci_dev *pdev)
>> +{
>> + struct xe_device *xe = pci_get_drvdata(pdev);
>> + struct xe_gt *gt;
>> + int id, err;
>> +
>> + pci_warn(pdev, "preparing for PCIe reset\n");
>> +
>> + drm_warn(&xe->drm, "removing device access to userspace\n");
>> + drm_dev_unplug(&xe->drm);
>> +
>> + xe_pm_runtime_get(xe);
>> + /* idle the GTs */
>> + for_each_gt(gt, xe, id) {
>> + err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> + if (err)
>> + goto reset;
>> + xe_uc_reset_prepare(>->uc);
>> + xe_gt_idle(gt);
>> + err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> + XE_WARN_ON(err);
>> + }
>> + xe_pm_runtime_put(xe);
> Is xe_save_pci_state() needed here?
No, as the state has already been saved post driver probe.
Regards,
Aravind.
>
> Regards,
> Badal Nilawar> +
>> +reset:
>> + pci_disable_device(pdev);
>> +}
>> +
>> +/**
>> + * xe_pci_reset_done - Called when PCIe reset is done.
>> + * @pdev: PCI device struct
>> + */
>> +void xe_pci_reset_done(struct pci_dev *pdev)
>> +{
>> + const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
>> + struct xe_device *xe = pci_get_drvdata(pdev);
>> +
>> + dev_info(&pdev->dev,
>> + "device went through PCIe reset, reenabling the device\n");
>> +
>> + if (pci_enable_device(pdev)) {
>> + dev_err(&pdev->dev,
>> + "Cannot re-enable PCI device after reset\n");
>> + return;
>> + }
>> + pci_set_master(pdev);
>> + xe_load_pci_state(pdev);
>> +
>> + xe->pci_device_is_reset = true;
>> + /*
>> + * We want to completely clean the driver and even destroy
>> + * the xe private data and reinitialize afresh similar to
>> + * probe
>> + */
>> + pdev->driver->remove(pdev);
>> + if (pci_dev_msi_enabled(pdev))
>> + pci_free_irq_vectors(pdev);
>> +
>> + devm_drm_dev_release_action(&xe->drm);
>> + pci_disable_device(pdev);
>> +
>> + /*
>> + * if this fails the driver might be in a stale state, only option is
>> + * to unbind and rebind
>> + */
>> + xe_pci_probe(pdev, ent);
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.h b/drivers/gpu/drm/xe/xe_pci_err.h
>> new file mode 100644
>> index 000000000000..95a4c8ce9cf1
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_pci_err.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_PCI_ERR_H_
>> +#define _XE_PCI_ERR_H_
>> +
>> +struct pci_dev;
>> +
>> +void xe_pci_reset_prepare(struct pci_dev *pdev);
>> +void xe_pci_reset_done(struct pci_dev *pdev);
>> +#endif
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
2024-04-23 23:49 ` Michał Winiarski
@ 2024-04-24 5:12 ` Aravind Iddamsetty
2024-04-24 23:29 ` Michał Winiarski
0 siblings, 1 reply; 18+ messages in thread
From: Aravind Iddamsetty @ 2024-04-24 5:12 UTC (permalink / raw)
To: Michał Winiarski
Cc: dri-devel, daniel, maarten.lankhorst, airlied, mripard,
tzimmermann, rodrigo.vivi, intel-xe, Lucas De Marchi,
Michal Wajdeczko
On 24/04/24 05:19, Michał Winiarski wrote:
> On Mon, Apr 22, 2024 at 12:27:56PM +0530, Aravind Iddamsetty wrote:
>> PCI subsystem provides callbacks to inform the driver about a request to
>> do function level reset by user, initiated by writing to sysfs entry
>> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
>> without the need to do unbind and rebind as the driver needs to
>> reinitialize the device afresh post FLR.
>>
>> v2:
>> 1. separate out gt idle and pci save/restore to a separate patch (Lucas)
>> 2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
>>
>> v3: declare xe_pci_err_handlers as static(Michal)
>>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>
>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>> ---
>> drivers/gpu/drm/xe/Makefile | 1 +
>> drivers/gpu/drm/xe/xe_device_types.h | 3 +
>> drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++
>> drivers/gpu/drm/xe/xe_pci.c | 9 ++-
>> drivers/gpu/drm/xe/xe_pci.h | 2 +
>> drivers/gpu/drm/xe/xe_pci_err.c | 88 ++++++++++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_pci_err.h | 13 ++++
>> 7 files changed, 119 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index 8bc62bfbc679..693971a1fac0 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
>> xe_module.o \
>> xe_pat.o \
>> xe_pci.o \
>> + xe_pci_err.o \
>> xe_pcode.o \
>> xe_pm.o \
>> xe_preempt_fence.o \
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index 0a66555229e9..8c749b378a92 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -465,6 +465,9 @@ struct xe_device {
>> /** @pci_state: PCI state of device */
>> struct pci_saved_state *pci_state;
>>
>> + /** @pci_device_is_reset: device went through PCIe FLR */
>> + bool pci_device_is_reset;
>> +
>> /* private: */
>>
>> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
>> index 509649d0e65e..efba0fbe2f5c 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>> @@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg)
>> return;
>> }
>>
>> + /* We already have done this before going through a reset, so skip here */
>> + if (xe->pci_device_is_reset)
>> + return;
>> +
>> XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL));
>> XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
>> XE_WARN_ON(xe_guc_pc_stop(pc));
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index a62300990e19..b5a582afc9e7 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -23,6 +23,7 @@
>> #include "xe_macros.h"
>> #include "xe_mmio.h"
>> #include "xe_module.h"
>> +#include "xe_pci_err.h"
>> #include "xe_pci_types.h"
>> #include "xe_pm.h"
>> #include "xe_sriov.h"
>> @@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev)
>> pci_set_drvdata(pdev, NULL);
>> }
>>
>> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> {
>> const struct xe_device_desc *desc = (const void *)ent->driver_data;
>> const struct xe_subplatform_desc *subplatform_desc;
>> @@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = {
>> };
>> #endif
>>
>> +static const struct pci_error_handlers xe_pci_err_handlers = {
>> + .reset_prepare = xe_pci_reset_prepare,
>> + .reset_done = xe_pci_reset_done,
>> +};
>> +
>> static struct pci_driver xe_pci_driver = {
>> .name = DRIVER_NAME,
>> .id_table = pciidlist,
>> @@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = {
>> #ifdef CONFIG_PM_SLEEP
>> .driver.pm = &xe_pm_ops,
>> #endif
>> + .err_handler = &xe_pci_err_handlers,
>> };
>>
>> int xe_register_pci_driver(void)
>> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
>> index 73b90a430d1f..9faf5380a09e 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.h
>> +++ b/drivers/gpu/drm/xe/xe_pci.h
>> @@ -7,8 +7,10 @@
>> #define _XE_PCI_H_
>>
>> struct pci_dev;
>> +struct pci_device_id;
>>
>> int xe_register_pci_driver(void);
>> void xe_unregister_pci_driver(void);
>> void xe_load_pci_state(struct pci_dev *pdev);
>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
>> #endif
>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
>> new file mode 100644
>> index 000000000000..5306925ea2fa
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
>> @@ -0,0 +1,88 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#include <linux/pci.h>
>> +#include <drm/drm_drv.h>
>> +
>> +#include "xe_device.h"
>> +#include "xe_gt.h"
>> +#include "xe_gt_printk.h"
>> +#include "xe_pci.h"
>> +#include "xe_pci_err.h"
>> +#include "xe_pm.h"
>> +#include "xe_uc.h"
>> +
>> +/**
>> + * xe_pci_reset_prepare - Called when user issued a PCIe reset
>> + * via /sys/bus/pci/devices/.../reset.
>> + * @pdev: PCI device struct
>> + */
>> +void xe_pci_reset_prepare(struct pci_dev *pdev)
>> +{
>> + struct xe_device *xe = pci_get_drvdata(pdev);
>> + struct xe_gt *gt;
>> + int id, err;
>> +
>> + pci_warn(pdev, "preparing for PCIe reset\n");
>> +
>> + drm_warn(&xe->drm, "removing device access to userspace\n");
> Warn? Shouldn't it be info?
I believe warn is appropriate as this is not a usual path the users apps expect
to hit and as they loose device connection.
>
>> + drm_dev_unplug(&xe->drm);
>> +
>> + xe_pm_runtime_get(xe);
>> + /* idle the GTs */
>> + for_each_gt(gt, xe, id) {
>> + err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> + if (err)
>> + goto reset;
>> + xe_uc_reset_prepare(>->uc);
>> + xe_gt_idle(gt);
>> + err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>> + XE_WARN_ON(err);
>> + }
>> + xe_pm_runtime_put(xe);
>> +
>> +reset:
>> + pci_disable_device(pdev);
>> +}
>> +
>> +/**
>> + * xe_pci_reset_done - Called when PCIe reset is done.
>> + * @pdev: PCI device struct
>> + */
>> +void xe_pci_reset_done(struct pci_dev *pdev)
>> +{
>> + const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
>> + struct xe_device *xe = pci_get_drvdata(pdev);
>> +
>> + dev_info(&pdev->dev,
>> + "device went through PCIe reset, reenabling the device\n");
>> +
>> + if (pci_enable_device(pdev)) {
>> + dev_err(&pdev->dev,
>> + "Cannot re-enable PCI device after reset\n");
>> + return;
>> + }
>> + pci_set_master(pdev);
>> + xe_load_pci_state(pdev);
>> +
>> + xe->pci_device_is_reset = true;
>> + /*
>> + * We want to completely clean the driver and even destroy
>> + * the xe private data and reinitialize afresh similar to
>> + * probe
>> + */
>> + pdev->driver->remove(pdev);
> Do we really want to do that?
> First of all, that fairly unusual - none of the other PCI drivers makes
> changes in the device/driver model during reset, which makes me wonder
> if this is really what the user expects.
> I would expect that the device is reset, but the driver is not unloaded
> and previously created FDs are still valid.
We cannot continue to work with previous instance of driver as post
reset GuC , LMEM and most of the soc is reset, so we need to reinitialize
device afresh like in driver probe, hence we remove to clean the previous
stale driver state and re-probe again.
All applications are expected to reopen the device handles afresh.
>
> Note that messing with the driver model at reset also makes it difficult
> to do a proper cleanup if SR-IOV is enabled, as PCI-core expects drivers
> to remove VF devices during driver->remove.
> Removal of said devices depends on pci_cfg_access_lock, which is already
> held for the duration of the reset (wh
I haven't verified SRIOV use case, i believe we can take this up as next step.
Also, since this is not an automatic reset but a user/admin initiated i believe
the onus can be on admin to close all VFs before initiating a reset.
> ich includes calling reset_done),
> which will cause a deadlock.
>
> In current form, it also causes WARN if there are open FDs to DRM
> device during reset.
I did verify using igt@device_reset@reset-bound which has a open connection haven't seen this error
but will re verify again but again certain warns are expected but driver reload should be successful.
similarly here as well we can expect the admin to close any applications using the device before
initiating a reset.
Thanks,
Aravind.
>
> [29357.277699] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:02.0/tile0' 01:06:58 [142/47263]
> [29357.286158] CPU: 7 PID: 3479 Comm: bash Not tainted 6.9.0-rc5-xe+ #78
> [29357.305870] Call Trace:
> [29357.308342] <TASK>
> [29357.310453] dump_stack_lvl+0x139/0x1d0
> [29357.314305] ? __pfx_dump_stack_lvl+0x10/0x10
> [29357.318680] ? __pfx__printk+0x10/0x10
> [29357.322450] ? kmalloc_trace+0x1c1/0x3a0
> [29357.326394] ? sysfs_create_dir_ns+0x162/0x210
> [29357.330861] sysfs_create_dir_ns+0x195/0x210
> [29357.335154] ? __pfx_sysfs_create_dir_ns+0x10/0x10
> [29357.339965] ? strcmp+0x2f/0x60
> [29357.343134] kobject_add_internal+0x2af/0x600
> [29357.347517] kobject_add+0xfb/0x190
> [29357.351028] ? __link_object+0x1c7/0x280
> [29357.354976] ? __pfx_kobject_add+0x10/0x10
> [29357.359093] ? __create_object+0x62/0x140
> [29357.363111] ? rcu_is_watching+0x20/0x50
> [29357.367055] ? kmalloc_trace+0x1c1/0x3a0
> [29357.370998] ? xe_tile_sysfs_init+0x4b/0x100 [xe]
> [29357.376016] xe_tile_sysfs_init+0x91/0x100 [xe]
> [29357.380868] xe_tile_init_noalloc+0xaf/0xc0 [xe]
> [29357.385936] xe_device_probe+0x60c/0x750 [xe]
> [29357.390674] ? __asan_memcpy+0x40/0x70
> [29357.394461] ? __drmm_add_action+0x1ac/0x210 [drm]
> [29357.399413] xe_pci_probe+0x5dc/0x680 [xe]
> [29357.403882] pci_reset_function+0x108/0x140
> [29357.408100] reset_store+0xb5/0x120
> [29357.411623] ? __pfx_reset_store+0x10/0x10
> [29357.415751] ? __pfx_sysfs_kf_write+0x10/0x10
> [29357.420149] kernfs_fop_write_iter+0x1b8/0x260
> [29357.424620] vfs_write+0x5ce/0x660
> [29357.428067] ? __pfx_vfs_write+0x10/0x10
> [29357.432028] ? __rcu_read_unlock+0x60/0x90
> [29357.436181] ksys_write+0xe4/0x190
> [29357.439612] ? __pfx_ksys_write+0x10/0x10
> [29357.443657] do_syscall_64+0x7b/0x120
> [29357.447348] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> [29357.452423] RIP: 0033:0x7f4f1ff14887
> [29357.456030] Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
> [29357.474761] RSP: 002b:00007ffe54e95068 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> [29357.482343] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f4f1ff14887
> [29357.489487] RDX: 0000000000000002 RSI: 0000559eb4076e30 RDI: 0000000000000001
> [29357.496630] RBP: 0000559eb4076e30 R08: 0000000000000000 R09: 0000559eb4076e30
> [29357.503775] R10: 0000000000000077 R11: 0000000000000246 R12: 0000000000000002
> [29357.510947] R13: 00007f4f2001b780 R14: 00007f4f20017600 R15: 00007f4f20016a00
> [29357.518174] </TASK>
> [29357.520539] kobject: kobject_add_internal failed for tile0 with -EEXIST, don't try to register things with the same name in the same directory.
>
> -Michał
>
>> + if (pci_dev_msi_enabled(pdev))
>> + pci_free_irq_vectors(pdev);
>> +
>> + devm_drm_dev_release_action(&xe->drm);
>> + pci_disable_device(pdev);
>> +
>> + /*
>> + * if this fails the driver might be in a stale state, only option is
>> + * to unbind and rebind
>> + */
>> + xe_pci_probe(pdev, ent);
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.h b/drivers/gpu/drm/xe/xe_pci_err.h
>> new file mode 100644
>> index 000000000000..95a4c8ce9cf1
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_pci_err.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2024 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_PCI_ERR_H_
>> +#define _XE_PCI_ERR_H_
>> +
>> +struct pci_dev;
>> +
>> +void xe_pci_reset_prepare(struct pci_dev *pdev);
>> +void xe_pci_reset_done(struct pci_dev *pdev);
>> +#endif
>> --
>> 2.25.1
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
2024-04-24 3:12 ` Aravind Iddamsetty
@ 2024-04-24 11:12 ` Nilawar, Badal
2024-04-25 4:07 ` Aravind Iddamsetty
0 siblings, 1 reply; 18+ messages in thread
From: Nilawar, Badal @ 2024-04-24 11:12 UTC (permalink / raw)
To: Aravind Iddamsetty, dri-devel, daniel, maarten.lankhorst, airlied,
mripard, tzimmermann, rodrigo.vivi
Cc: intel-xe, Lucas De Marchi, Michal Wajdeczko
On 24-04-2024 08:42, Aravind Iddamsetty wrote:
>
> On 23/04/24 20:34, Nilawar, Badal wrote:
>>
>>
>> On 22-04-2024 12:27, Aravind Iddamsetty wrote:
>>> PCI subsystem provides callbacks to inform the driver about a request to
>>> do function level reset by user, initiated by writing to sysfs entry
>>> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
>>> without the need to do unbind and rebind as the driver needs to
>>> reinitialize the device afresh post FLR.
>>>
>>> v2:
>>> 1. separate out gt idle and pci save/restore to a separate patch (Lucas)
>>> 2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
>>>
>>> v3: declare xe_pci_err_handlers as static(Michal)
>>>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>
>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>>> ---
>>> drivers/gpu/drm/xe/Makefile | 1 +
>>> drivers/gpu/drm/xe/xe_device_types.h | 3 +
>>> drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++
>>> drivers/gpu/drm/xe/xe_pci.c | 9 ++-
>>> drivers/gpu/drm/xe/xe_pci.h | 2 +
>>> drivers/gpu/drm/xe/xe_pci_err.c | 88 ++++++++++++++++++++++++++++
>>> drivers/gpu/drm/xe/xe_pci_err.h | 13 ++++
>>> 7 files changed, 119 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
>>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
>>>
>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>> index 8bc62bfbc679..693971a1fac0 100644
>>> --- a/drivers/gpu/drm/xe/Makefile
>>> +++ b/drivers/gpu/drm/xe/Makefile
>>> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
>>> xe_module.o \
>>> xe_pat.o \
>>> xe_pci.o \
>>> + xe_pci_err.o \
>>> xe_pcode.o \
>>> xe_pm.o \
>>> xe_preempt_fence.o \
>>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>>> index 0a66555229e9..8c749b378a92 100644
>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>>> @@ -465,6 +465,9 @@ struct xe_device {
>>> /** @pci_state: PCI state of device */
>>> struct pci_saved_state *pci_state;
>>> + /** @pci_device_is_reset: device went through PCIe FLR */
>>> + bool pci_device_is_reset;
>>> +
>>> /* private: */
>>> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
>>> index 509649d0e65e..efba0fbe2f5c 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>>> @@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg)
>>> return;
>>> }
>>> + /* We already have done this before going through a reset, so skip here */
>>> + if (xe->pci_device_is_reset)
>>> + return;
>>> +
>>> XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL));
>>> XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
>>> XE_WARN_ON(xe_guc_pc_stop(pc));
>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>> index a62300990e19..b5a582afc9e7 100644
>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>> @@ -23,6 +23,7 @@
>>> #include "xe_macros.h"
>>> #include "xe_mmio.h"
>>> #include "xe_module.h"
>>> +#include "xe_pci_err.h"
>>> #include "xe_pci_types.h"
>>> #include "xe_pm.h"
>>> #include "xe_sriov.h"
>>> @@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev)
>>> pci_set_drvdata(pdev, NULL);
>>> }
>>> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>> {
>>> const struct xe_device_desc *desc = (const void *)ent->driver_data;
>>> const struct xe_subplatform_desc *subplatform_desc;
>>> @@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = {
>>> };
>>> #endif
>>> +static const struct pci_error_handlers xe_pci_err_handlers = {
>>> + .reset_prepare = xe_pci_reset_prepare,
>>> + .reset_done = xe_pci_reset_done,
>>> +};
>>> +
>>> static struct pci_driver xe_pci_driver = {
>>> .name = DRIVER_NAME,
>>> .id_table = pciidlist,
>>> @@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = {
>>> #ifdef CONFIG_PM_SLEEP
>>> .driver.pm = &xe_pm_ops,
>>> #endif
>>> + .err_handler = &xe_pci_err_handlers,
>>> };
>>> int xe_register_pci_driver(void)
>>> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
>>> index 73b90a430d1f..9faf5380a09e 100644
>>> --- a/drivers/gpu/drm/xe/xe_pci.h
>>> +++ b/drivers/gpu/drm/xe/xe_pci.h
>>> @@ -7,8 +7,10 @@
>>> #define _XE_PCI_H_
>>> struct pci_dev;
>>> +struct pci_device_id;
>>> int xe_register_pci_driver(void);
>>> void xe_unregister_pci_driver(void);
>>> void xe_load_pci_state(struct pci_dev *pdev);
>>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
>>> #endif
>>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
>>> new file mode 100644
>>> index 000000000000..5306925ea2fa
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
>>> @@ -0,0 +1,88 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2024 Intel Corporation
>>> + */
>>> +
>>> +#include <linux/pci.h>
>>> +#include <drm/drm_drv.h>
>>> +
>>> +#include "xe_device.h"
>>> +#include "xe_gt.h"
>>> +#include "xe_gt_printk.h"
>>> +#include "xe_pci.h"
>>> +#include "xe_pci_err.h"
>>> +#include "xe_pm.h"
>>> +#include "xe_uc.h"
>>> +
>>> +/**
>>> + * xe_pci_reset_prepare - Called when user issued a PCIe reset
>>> + * via /sys/bus/pci/devices/.../reset.
>>> + * @pdev: PCI device struct
>>> + */
>>> +void xe_pci_reset_prepare(struct pci_dev *pdev)
>>> +{
>>> + struct xe_device *xe = pci_get_drvdata(pdev);
>>> + struct xe_gt *gt;
>>> + int id, err;
>>> +
>>> + pci_warn(pdev, "preparing for PCIe reset\n");
>>> +
>>> + drm_warn(&xe->drm, "removing device access to userspace\n");
>>> + drm_dev_unplug(&xe->drm);
>>> +
>>> + xe_pm_runtime_get(xe);
>>> + /* idle the GTs */
>>> + for_each_gt(gt, xe, id) {
>>> + err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>> + if (err)
>>> + goto reset;
>>> + xe_uc_reset_prepare(>->uc);
>>> + xe_gt_idle(gt);
>>> + err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>> + XE_WARN_ON(err);
>>> + }
>>> + xe_pm_runtime_put(xe);
>> Is xe_save_pci_state() needed here?
>
> No, as the state has already been saved post driver probe.
Thanks for clarification. One more doubt I have is in FLR flow what will
happen to buffer objects especially mmaped BOs. Will all the BOs get
destroyed? I couldn't figure out it in the code.
Badal
>
> Regards,
> Aravind.
>>
>> Regards,
>> Badal Nilawar> +
>>> +reset:
>>> + pci_disable_device(pdev);
>>> +}
>>> +
>>> +/**
>>> + * xe_pci_reset_done - Called when PCIe reset is done.
>>> + * @pdev: PCI device struct
>>> + */
>>> +void xe_pci_reset_done(struct pci_dev *pdev)
>>> +{
>>> + const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
>>> + struct xe_device *xe = pci_get_drvdata(pdev);
>>> +
>>> + dev_info(&pdev->dev,
>>> + "device went through PCIe reset, reenabling the device\n");
>>> +
>>> + if (pci_enable_device(pdev)) {
>>> + dev_err(&pdev->dev,
>>> + "Cannot re-enable PCI device after reset\n");
>>> + return;
>>> + }
>>> + pci_set_master(pdev);
>>> + xe_load_pci_state(pdev);
>>> +
>>> + xe->pci_device_is_reset = true;
>>> + /*
>>> + * We want to completely clean the driver and even destroy
>>> + * the xe private data and reinitialize afresh similar to
>>> + * probe
>>> + */
>>> + pdev->driver->remove(pdev);
>>> + if (pci_dev_msi_enabled(pdev))
>>> + pci_free_irq_vectors(pdev);
>>> +
>>> + devm_drm_dev_release_action(&xe->drm);
>>> + pci_disable_device(pdev);
>>> +
>>> + /*
>>> + * if this fails the driver might be in a stale state, only option is
>>> + * to unbind and rebind
>>> + */
>>> + xe_pci_probe(pdev, ent);
>>> +}
>>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.h b/drivers/gpu/drm/xe/xe_pci_err.h
>>> new file mode 100644
>>> index 000000000000..95a4c8ce9cf1
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/xe_pci_err.h
>>> @@ -0,0 +1,13 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2024 Intel Corporation
>>> + */
>>> +
>>> +#ifndef _XE_PCI_ERR_H_
>>> +#define _XE_PCI_ERR_H_
>>> +
>>> +struct pci_dev;
>>> +
>>> +void xe_pci_reset_prepare(struct pci_dev *pdev);
>>> +void xe_pci_reset_done(struct pci_dev *pdev);
>>> +#endif
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
2024-04-24 5:12 ` Aravind Iddamsetty
@ 2024-04-24 23:29 ` Michał Winiarski
2024-04-25 6:17 ` Aravind Iddamsetty
0 siblings, 1 reply; 18+ messages in thread
From: Michał Winiarski @ 2024-04-24 23:29 UTC (permalink / raw)
To: Aravind Iddamsetty
Cc: dri-devel, daniel, maarten.lankhorst, airlied, mripard,
tzimmermann, rodrigo.vivi, intel-xe, Lucas De Marchi,
Michal Wajdeczko
On Wed, Apr 24, 2024 at 10:42:59AM +0530, Aravind Iddamsetty wrote:
>
> On 24/04/24 05:19, Michał Winiarski wrote:
> > On Mon, Apr 22, 2024 at 12:27:56PM +0530, Aravind Iddamsetty wrote:
> >> PCI subsystem provides callbacks to inform the driver about a request to
> >> do function level reset by user, initiated by writing to sysfs entry
> >> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
> >> without the need to do unbind and rebind as the driver needs to
> >> reinitialize the device afresh post FLR.
> >>
> >> v2:
> >> 1. separate out gt idle and pci save/restore to a separate patch (Lucas)
> >> 2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
> >>
> >> v3: declare xe_pci_err_handlers as static(Michal)
> >>
> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>
> >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> >> ---
> >> drivers/gpu/drm/xe/Makefile | 1 +
> >> drivers/gpu/drm/xe/xe_device_types.h | 3 +
> >> drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++
> >> drivers/gpu/drm/xe/xe_pci.c | 9 ++-
> >> drivers/gpu/drm/xe/xe_pci.h | 2 +
> >> drivers/gpu/drm/xe/xe_pci_err.c | 88 ++++++++++++++++++++++++++++
> >> drivers/gpu/drm/xe/xe_pci_err.h | 13 ++++
> >> 7 files changed, 119 insertions(+), 1 deletion(-)
> >> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
> >> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
> >>
> >> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> >> index 8bc62bfbc679..693971a1fac0 100644
> >> --- a/drivers/gpu/drm/xe/Makefile
> >> +++ b/drivers/gpu/drm/xe/Makefile
> >> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
> >> xe_module.o \
> >> xe_pat.o \
> >> xe_pci.o \
> >> + xe_pci_err.o \
> >> xe_pcode.o \
> >> xe_pm.o \
> >> xe_preempt_fence.o \
> >> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> >> index 0a66555229e9..8c749b378a92 100644
> >> --- a/drivers/gpu/drm/xe/xe_device_types.h
> >> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> >> @@ -465,6 +465,9 @@ struct xe_device {
> >> /** @pci_state: PCI state of device */
> >> struct pci_saved_state *pci_state;
> >>
> >> + /** @pci_device_is_reset: device went through PCIe FLR */
> >> + bool pci_device_is_reset;
> >> +
> >> /* private: */
> >>
> >> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> >> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> >> index 509649d0e65e..efba0fbe2f5c 100644
> >> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> >> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> >> @@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg)
> >> return;
> >> }
> >>
> >> + /* We already have done this before going through a reset, so skip here */
> >> + if (xe->pci_device_is_reset)
> >> + return;
> >> +
> >> XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL));
> >> XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
> >> XE_WARN_ON(xe_guc_pc_stop(pc));
> >> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> >> index a62300990e19..b5a582afc9e7 100644
> >> --- a/drivers/gpu/drm/xe/xe_pci.c
> >> +++ b/drivers/gpu/drm/xe/xe_pci.c
> >> @@ -23,6 +23,7 @@
> >> #include "xe_macros.h"
> >> #include "xe_mmio.h"
> >> #include "xe_module.h"
> >> +#include "xe_pci_err.h"
> >> #include "xe_pci_types.h"
> >> #include "xe_pm.h"
> >> #include "xe_sriov.h"
> >> @@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev)
> >> pci_set_drvdata(pdev, NULL);
> >> }
> >>
> >> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >> {
> >> const struct xe_device_desc *desc = (const void *)ent->driver_data;
> >> const struct xe_subplatform_desc *subplatform_desc;
> >> @@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = {
> >> };
> >> #endif
> >>
> >> +static const struct pci_error_handlers xe_pci_err_handlers = {
> >> + .reset_prepare = xe_pci_reset_prepare,
> >> + .reset_done = xe_pci_reset_done,
> >> +};
> >> +
> >> static struct pci_driver xe_pci_driver = {
> >> .name = DRIVER_NAME,
> >> .id_table = pciidlist,
> >> @@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = {
> >> #ifdef CONFIG_PM_SLEEP
> >> .driver.pm = &xe_pm_ops,
> >> #endif
> >> + .err_handler = &xe_pci_err_handlers,
> >> };
> >>
> >> int xe_register_pci_driver(void)
> >> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
> >> index 73b90a430d1f..9faf5380a09e 100644
> >> --- a/drivers/gpu/drm/xe/xe_pci.h
> >> +++ b/drivers/gpu/drm/xe/xe_pci.h
> >> @@ -7,8 +7,10 @@
> >> #define _XE_PCI_H_
> >>
> >> struct pci_dev;
> >> +struct pci_device_id;
> >>
> >> int xe_register_pci_driver(void);
> >> void xe_unregister_pci_driver(void);
> >> void xe_load_pci_state(struct pci_dev *pdev);
> >> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
> >> #endif
> >> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
> >> new file mode 100644
> >> index 000000000000..5306925ea2fa
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
> >> @@ -0,0 +1,88 @@
> >> +// SPDX-License-Identifier: MIT
> >> +/*
> >> + * Copyright © 2024 Intel Corporation
> >> + */
> >> +
> >> +#include <linux/pci.h>
> >> +#include <drm/drm_drv.h>
> >> +
> >> +#include "xe_device.h"
> >> +#include "xe_gt.h"
> >> +#include "xe_gt_printk.h"
> >> +#include "xe_pci.h"
> >> +#include "xe_pci_err.h"
> >> +#include "xe_pm.h"
> >> +#include "xe_uc.h"
> >> +
> >> +/**
> >> + * xe_pci_reset_prepare - Called when user issued a PCIe reset
> >> + * via /sys/bus/pci/devices/.../reset.
> >> + * @pdev: PCI device struct
> >> + */
> >> +void xe_pci_reset_prepare(struct pci_dev *pdev)
> >> +{
> >> + struct xe_device *xe = pci_get_drvdata(pdev);
> >> + struct xe_gt *gt;
> >> + int id, err;
> >> +
> >> + pci_warn(pdev, "preparing for PCIe reset\n");
> >> +
> >> + drm_warn(&xe->drm, "removing device access to userspace\n");
> > Warn? Shouldn't it be info?
> I believe warn is appropriate as this is not a usual path the users apps expect
> to hit and as they loose device connection.
It's an expected path after issuing a reset via /sys/bus/pci/devices/.../reset.
DRM device disappearing - yeah, that's probably not expected, fully
agree on that.
> >
> >> + drm_dev_unplug(&xe->drm);
> >> +
> >> + xe_pm_runtime_get(xe);
> >> + /* idle the GTs */
> >> + for_each_gt(gt, xe, id) {
> >> + err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> >> + if (err)
> >> + goto reset;
> >> + xe_uc_reset_prepare(>->uc);
> >> + xe_gt_idle(gt);
> >> + err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> >> + XE_WARN_ON(err);
> >> + }
> >> + xe_pm_runtime_put(xe);
> >> +
> >> +reset:
> >> + pci_disable_device(pdev);
> >> +}
> >> +
> >> +/**
> >> + * xe_pci_reset_done - Called when PCIe reset is done.
> >> + * @pdev: PCI device struct
> >> + */
> >> +void xe_pci_reset_done(struct pci_dev *pdev)
> >> +{
> >> + const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
> >> + struct xe_device *xe = pci_get_drvdata(pdev);
> >> +
> >> + dev_info(&pdev->dev,
> >> + "device went through PCIe reset, reenabling the device\n");
> >> +
> >> + if (pci_enable_device(pdev)) {
> >> + dev_err(&pdev->dev,
> >> + "Cannot re-enable PCI device after reset\n");
> >> + return;
> >> + }
> >> + pci_set_master(pdev);
> >> + xe_load_pci_state(pdev);
> >> +
> >> + xe->pci_device_is_reset = true;
> >> + /*
> >> + * We want to completely clean the driver and even destroy
> >> + * the xe private data and reinitialize afresh similar to
> >> + * probe
> >> + */
> >> + pdev->driver->remove(pdev);
> > Do we really want to do that?
> > First of all, that fairly unusual - none of the other PCI drivers makes
> > changes in the device/driver model during reset, which makes me wonder
> > if this is really what the user expects.
> > I would expect that the device is reset, but the driver is not unloaded
> > and previously created FDs are still valid.
> We cannot continue to work with previous instance of driver as post
> reset GuC , LMEM and most of the soc is reset, so we need to reinitialize
> device afresh like in driver probe, hence we remove to clean the previous
> stale driver state and re-probe again.
GuC, LMEM and most of the soc is reset when we're doing S3 / S4, and
yet, we're not removing the driver in this scenario.
>
> All applications are expected to reopen the device handles afresh.
Current UMDs are not handling that case.
>
> >
> > Note that messing with the driver model at reset also makes it difficult
> > to do a proper cleanup if SR-IOV is enabled, as PCI-core expects drivers
> > to remove VF devices during driver->remove.
> > Removal of said devices depends on pci_cfg_access_lock, which is already
> > held for the duration of the reset (wh
> I haven't verified SRIOV use case, i believe we can take this up as next step.
> Also, since this is not an automatic reset but a user/admin initiated i believe
> the onus can be on admin to close all VFs before initiating a reset.
That's not the contract for reset. It's not a "user/admin" interface,
there doesn't necessarily have to be an "interactive" user interfacing
with it through shell.
It's just a regular stable sysfs uAPI - it could very well be called by
application as well.
> > ich includes calling reset_done),
> > which will cause a deadlock.
> >
> > In current form, it also causes WARN if there are open FDs to DRM
> > device during reset.
> I did verify using igt@device_reset@reset-bound which has a open connection haven't seen this error
> but will re verify again but again certain warns are expected but driver reload should be successful.
> similarly here as well we can expect the admin to close any applications using the device before
> initiating a reset.
Can we expect this?
If we can - why are you stating that "all applications are expected to
reopen the device handles afresh"?
I believe that the expectation for reset is that it behaves simiarly to
the PM flows, which is mostly transparent to the users.
Thanks,
-Michał
>
> Thanks,
>
> Aravind.
> >
> > [29357.277699] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:02.0/tile0' 01:06:58 [142/47263]
> > [29357.286158] CPU: 7 PID: 3479 Comm: bash Not tainted 6.9.0-rc5-xe+ #78
> > [29357.305870] Call Trace:
> > [29357.308342] <TASK>
> > [29357.310453] dump_stack_lvl+0x139/0x1d0
> > [29357.314305] ? __pfx_dump_stack_lvl+0x10/0x10
> > [29357.318680] ? __pfx__printk+0x10/0x10
> > [29357.322450] ? kmalloc_trace+0x1c1/0x3a0
> > [29357.326394] ? sysfs_create_dir_ns+0x162/0x210
> > [29357.330861] sysfs_create_dir_ns+0x195/0x210
> > [29357.335154] ? __pfx_sysfs_create_dir_ns+0x10/0x10
> > [29357.339965] ? strcmp+0x2f/0x60
> > [29357.343134] kobject_add_internal+0x2af/0x600
> > [29357.347517] kobject_add+0xfb/0x190
> > [29357.351028] ? __link_object+0x1c7/0x280
> > [29357.354976] ? __pfx_kobject_add+0x10/0x10
> > [29357.359093] ? __create_object+0x62/0x140
> > [29357.363111] ? rcu_is_watching+0x20/0x50
> > [29357.367055] ? kmalloc_trace+0x1c1/0x3a0
> > [29357.370998] ? xe_tile_sysfs_init+0x4b/0x100 [xe]
> > [29357.376016] xe_tile_sysfs_init+0x91/0x100 [xe]
> > [29357.380868] xe_tile_init_noalloc+0xaf/0xc0 [xe]
> > [29357.385936] xe_device_probe+0x60c/0x750 [xe]
> > [29357.390674] ? __asan_memcpy+0x40/0x70
> > [29357.394461] ? __drmm_add_action+0x1ac/0x210 [drm]
> > [29357.399413] xe_pci_probe+0x5dc/0x680 [xe]
> > [29357.403882] pci_reset_function+0x108/0x140
> > [29357.408100] reset_store+0xb5/0x120
> > [29357.411623] ? __pfx_reset_store+0x10/0x10
> > [29357.415751] ? __pfx_sysfs_kf_write+0x10/0x10
> > [29357.420149] kernfs_fop_write_iter+0x1b8/0x260
> > [29357.424620] vfs_write+0x5ce/0x660
> > [29357.428067] ? __pfx_vfs_write+0x10/0x10
> > [29357.432028] ? __rcu_read_unlock+0x60/0x90
> > [29357.436181] ksys_write+0xe4/0x190
> > [29357.439612] ? __pfx_ksys_write+0x10/0x10
> > [29357.443657] do_syscall_64+0x7b/0x120
> > [29357.447348] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> > [29357.452423] RIP: 0033:0x7f4f1ff14887
> > [29357.456030] Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
> > [29357.474761] RSP: 002b:00007ffe54e95068 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> > [29357.482343] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f4f1ff14887
> > [29357.489487] RDX: 0000000000000002 RSI: 0000559eb4076e30 RDI: 0000000000000001
> > [29357.496630] RBP: 0000559eb4076e30 R08: 0000000000000000 R09: 0000559eb4076e30
> > [29357.503775] R10: 0000000000000077 R11: 0000000000000246 R12: 0000000000000002
> > [29357.510947] R13: 00007f4f2001b780 R14: 00007f4f20017600 R15: 00007f4f20016a00
> > [29357.518174] </TASK>
> > [29357.520539] kobject: kobject_add_internal failed for tile0 with -EEXIST, don't try to register things with the same name in the same directory.
> >
> > -Michał
> >
> >> + if (pci_dev_msi_enabled(pdev))
> >> + pci_free_irq_vectors(pdev);
> >> +
> >> + devm_drm_dev_release_action(&xe->drm);
> >> + pci_disable_device(pdev);
> >> +
> >> + /*
> >> + * if this fails the driver might be in a stale state, only option is
> >> + * to unbind and rebind
> >> + */
> >> + xe_pci_probe(pdev, ent);
> >> +}
> >> diff --git a/drivers/gpu/drm/xe/xe_pci_err.h b/drivers/gpu/drm/xe/xe_pci_err.h
> >> new file mode 100644
> >> index 000000000000..95a4c8ce9cf1
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/xe/xe_pci_err.h
> >> @@ -0,0 +1,13 @@
> >> +/* SPDX-License-Identifier: MIT */
> >> +/*
> >> + * Copyright © 2024 Intel Corporation
> >> + */
> >> +
> >> +#ifndef _XE_PCI_ERR_H_
> >> +#define _XE_PCI_ERR_H_
> >> +
> >> +struct pci_dev;
> >> +
> >> +void xe_pci_reset_prepare(struct pci_dev *pdev);
> >> +void xe_pci_reset_done(struct pci_dev *pdev);
> >> +#endif
> >> --
> >> 2.25.1
> >>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
2024-04-24 11:12 ` Nilawar, Badal
@ 2024-04-25 4:07 ` Aravind Iddamsetty
0 siblings, 0 replies; 18+ messages in thread
From: Aravind Iddamsetty @ 2024-04-25 4:07 UTC (permalink / raw)
To: Nilawar, Badal, dri-devel, daniel, maarten.lankhorst, airlied,
mripard, tzimmermann, rodrigo.vivi
Cc: intel-xe, Lucas De Marchi, Michal Wajdeczko
On 24/04/24 16:42, Nilawar, Badal wrote:
>
>
> On 24-04-2024 08:42, Aravind Iddamsetty wrote:
>>
>> On 23/04/24 20:34, Nilawar, Badal wrote:
>>>
>>>
>>> On 22-04-2024 12:27, Aravind Iddamsetty wrote:
>>>> PCI subsystem provides callbacks to inform the driver about a request to
>>>> do function level reset by user, initiated by writing to sysfs entry
>>>> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
>>>> without the need to do unbind and rebind as the driver needs to
>>>> reinitialize the device afresh post FLR.
>>>>
>>>> v2:
>>>> 1. separate out gt idle and pci save/restore to a separate patch (Lucas)
>>>> 2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
>>>>
>>>> v3: declare xe_pci_err_handlers as static(Michal)
>>>>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>>
>>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/xe/Makefile | 1 +
>>>> drivers/gpu/drm/xe/xe_device_types.h | 3 +
>>>> drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++
>>>> drivers/gpu/drm/xe/xe_pci.c | 9 ++-
>>>> drivers/gpu/drm/xe/xe_pci.h | 2 +
>>>> drivers/gpu/drm/xe/xe_pci_err.c | 88 ++++++++++++++++++++++++++++
>>>> drivers/gpu/drm/xe/xe_pci_err.h | 13 ++++
>>>> 7 files changed, 119 insertions(+), 1 deletion(-)
>>>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
>>>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>>> index 8bc62bfbc679..693971a1fac0 100644
>>>> --- a/drivers/gpu/drm/xe/Makefile
>>>> +++ b/drivers/gpu/drm/xe/Makefile
>>>> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
>>>> xe_module.o \
>>>> xe_pat.o \
>>>> xe_pci.o \
>>>> + xe_pci_err.o \
>>>> xe_pcode.o \
>>>> xe_pm.o \
>>>> xe_preempt_fence.o \
>>>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>>>> index 0a66555229e9..8c749b378a92 100644
>>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>>>> @@ -465,6 +465,9 @@ struct xe_device {
>>>> /** @pci_state: PCI state of device */
>>>> struct pci_saved_state *pci_state;
>>>> + /** @pci_device_is_reset: device went through PCIe FLR */
>>>> + bool pci_device_is_reset;
>>>> +
>>>> /* private: */
>>>> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> index 509649d0e65e..efba0fbe2f5c 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> @@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg)
>>>> return;
>>>> }
>>>> + /* We already have done this before going through a reset, so skip here */
>>>> + if (xe->pci_device_is_reset)
>>>> + return;
>>>> +
>>>> XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL));
>>>> XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
>>>> XE_WARN_ON(xe_guc_pc_stop(pc));
>>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>>> index a62300990e19..b5a582afc9e7 100644
>>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>>> @@ -23,6 +23,7 @@
>>>> #include "xe_macros.h"
>>>> #include "xe_mmio.h"
>>>> #include "xe_module.h"
>>>> +#include "xe_pci_err.h"
>>>> #include "xe_pci_types.h"
>>>> #include "xe_pm.h"
>>>> #include "xe_sriov.h"
>>>> @@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev)
>>>> pci_set_drvdata(pdev, NULL);
>>>> }
>>>> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>> {
>>>> const struct xe_device_desc *desc = (const void *)ent->driver_data;
>>>> const struct xe_subplatform_desc *subplatform_desc;
>>>> @@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = {
>>>> };
>>>> #endif
>>>> +static const struct pci_error_handlers xe_pci_err_handlers = {
>>>> + .reset_prepare = xe_pci_reset_prepare,
>>>> + .reset_done = xe_pci_reset_done,
>>>> +};
>>>> +
>>>> static struct pci_driver xe_pci_driver = {
>>>> .name = DRIVER_NAME,
>>>> .id_table = pciidlist,
>>>> @@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = {
>>>> #ifdef CONFIG_PM_SLEEP
>>>> .driver.pm = &xe_pm_ops,
>>>> #endif
>>>> + .err_handler = &xe_pci_err_handlers,
>>>> };
>>>> int xe_register_pci_driver(void)
>>>> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
>>>> index 73b90a430d1f..9faf5380a09e 100644
>>>> --- a/drivers/gpu/drm/xe/xe_pci.h
>>>> +++ b/drivers/gpu/drm/xe/xe_pci.h
>>>> @@ -7,8 +7,10 @@
>>>> #define _XE_PCI_H_
>>>> struct pci_dev;
>>>> +struct pci_device_id;
>>>> int xe_register_pci_driver(void);
>>>> void xe_unregister_pci_driver(void);
>>>> void xe_load_pci_state(struct pci_dev *pdev);
>>>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
>>>> #endif
>>>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
>>>> new file mode 100644
>>>> index 000000000000..5306925ea2fa
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
>>>> @@ -0,0 +1,88 @@
>>>> +// SPDX-License-Identifier: MIT
>>>> +/*
>>>> + * Copyright © 2024 Intel Corporation
>>>> + */
>>>> +
>>>> +#include <linux/pci.h>
>>>> +#include <drm/drm_drv.h>
>>>> +
>>>> +#include "xe_device.h"
>>>> +#include "xe_gt.h"
>>>> +#include "xe_gt_printk.h"
>>>> +#include "xe_pci.h"
>>>> +#include "xe_pci_err.h"
>>>> +#include "xe_pm.h"
>>>> +#include "xe_uc.h"
>>>> +
>>>> +/**
>>>> + * xe_pci_reset_prepare - Called when user issued a PCIe reset
>>>> + * via /sys/bus/pci/devices/.../reset.
>>>> + * @pdev: PCI device struct
>>>> + */
>>>> +void xe_pci_reset_prepare(struct pci_dev *pdev)
>>>> +{
>>>> + struct xe_device *xe = pci_get_drvdata(pdev);
>>>> + struct xe_gt *gt;
>>>> + int id, err;
>>>> +
>>>> + pci_warn(pdev, "preparing for PCIe reset\n");
>>>> +
>>>> + drm_warn(&xe->drm, "removing device access to userspace\n");
>>>> + drm_dev_unplug(&xe->drm);
>>>> +
>>>> + xe_pm_runtime_get(xe);
>>>> + /* idle the GTs */
>>>> + for_each_gt(gt, xe, id) {
>>>> + err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>>> + if (err)
>>>> + goto reset;
>>>> + xe_uc_reset_prepare(>->uc);
>>>> + xe_gt_idle(gt);
>>>> + err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>>> + XE_WARN_ON(err);
>>>> + }
>>>> + xe_pm_runtime_put(xe);
>>> Is xe_save_pci_state() needed here?
>>
>> No, as the state has already been saved post driver probe.
>
> Thanks for clarification. One more doubt I have is in FLR flow what will happen to buffer objects especially mmaped BOs. Will all the BOs get destroyed? I couldn't figure out it in the code.
buffer ummaping is handled in drm_dev_unplug called above.
Thanks,
Aravind.
>
> Badal
>>
>> Regards,
>> Aravind.
>>>
>>> Regards,
>>> Badal Nilawar> +
>>>> +reset:
>>>> + pci_disable_device(pdev);
>>>> +}
>>>> +
>>>> +/**
>>>> + * xe_pci_reset_done - Called when PCIe reset is done.
>>>> + * @pdev: PCI device struct
>>>> + */
>>>> +void xe_pci_reset_done(struct pci_dev *pdev)
>>>> +{
>>>> + const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
>>>> + struct xe_device *xe = pci_get_drvdata(pdev);
>>>> +
>>>> + dev_info(&pdev->dev,
>>>> + "device went through PCIe reset, reenabling the device\n");
>>>> +
>>>> + if (pci_enable_device(pdev)) {
>>>> + dev_err(&pdev->dev,
>>>> + "Cannot re-enable PCI device after reset\n");
>>>> + return;
>>>> + }
>>>> + pci_set_master(pdev);
>>>> + xe_load_pci_state(pdev);
>>>> +
>>>> + xe->pci_device_is_reset = true;
>>>> + /*
>>>> + * We want to completely clean the driver and even destroy
>>>> + * the xe private data and reinitialize afresh similar to
>>>> + * probe
>>>> + */
>>>> + pdev->driver->remove(pdev);
>>>> + if (pci_dev_msi_enabled(pdev))
>>>> + pci_free_irq_vectors(pdev);
>>>> +
>>>> + devm_drm_dev_release_action(&xe->drm);
>>>> + pci_disable_device(pdev);
>>>> +
>>>> + /*
>>>> + * if this fails the driver might be in a stale state, only option is
>>>> + * to unbind and rebind
>>>> + */
>>>> + xe_pci_probe(pdev, ent);
>>>> +}
>>>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.h b/drivers/gpu/drm/xe/xe_pci_err.h
>>>> new file mode 100644
>>>> index 000000000000..95a4c8ce9cf1
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/xe/xe_pci_err.h
>>>> @@ -0,0 +1,13 @@
>>>> +/* SPDX-License-Identifier: MIT */
>>>> +/*
>>>> + * Copyright © 2024 Intel Corporation
>>>> + */
>>>> +
>>>> +#ifndef _XE_PCI_ERR_H_
>>>> +#define _XE_PCI_ERR_H_
>>>> +
>>>> +struct pci_dev;
>>>> +
>>>> +void xe_pci_reset_prepare(struct pci_dev *pdev);
>>>> +void xe_pci_reset_done(struct pci_dev *pdev);
>>>> +#endif
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
2024-04-24 23:29 ` Michał Winiarski
@ 2024-04-25 6:17 ` Aravind Iddamsetty
2024-04-26 0:53 ` Michał Winiarski
0 siblings, 1 reply; 18+ messages in thread
From: Aravind Iddamsetty @ 2024-04-25 6:17 UTC (permalink / raw)
To: Michał Winiarski
Cc: dri-devel, daniel, maarten.lankhorst, airlied, mripard,
tzimmermann, rodrigo.vivi, intel-xe, Lucas De Marchi,
Michal Wajdeczko
On 25/04/24 04:59, Michał Winiarski wrote:
> On Wed, Apr 24, 2024 at 10:42:59AM +0530, Aravind Iddamsetty wrote:
>> On 24/04/24 05:19, Michał Winiarski wrote:
>>> On Mon, Apr 22, 2024 at 12:27:56PM +0530, Aravind Iddamsetty wrote:
>>>> PCI subsystem provides callbacks to inform the driver about a request to
>>>> do function level reset by user, initiated by writing to sysfs entry
>>>> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
>>>> without the need to do unbind and rebind as the driver needs to
>>>> reinitialize the device afresh post FLR.
>>>>
>>>> v2:
>>>> 1. separate out gt idle and pci save/restore to a separate patch (Lucas)
>>>> 2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
>>>>
>>>> v3: declare xe_pci_err_handlers as static(Michal)
>>>>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>>
>>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>>>> ---
>>>> drivers/gpu/drm/xe/Makefile | 1 +
>>>> drivers/gpu/drm/xe/xe_device_types.h | 3 +
>>>> drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++
>>>> drivers/gpu/drm/xe/xe_pci.c | 9 ++-
>>>> drivers/gpu/drm/xe/xe_pci.h | 2 +
>>>> drivers/gpu/drm/xe/xe_pci_err.c | 88 ++++++++++++++++++++++++++++
>>>> drivers/gpu/drm/xe/xe_pci_err.h | 13 ++++
>>>> 7 files changed, 119 insertions(+), 1 deletion(-)
>>>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
>>>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>>> index 8bc62bfbc679..693971a1fac0 100644
>>>> --- a/drivers/gpu/drm/xe/Makefile
>>>> +++ b/drivers/gpu/drm/xe/Makefile
>>>> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
>>>> xe_module.o \
>>>> xe_pat.o \
>>>> xe_pci.o \
>>>> + xe_pci_err.o \
>>>> xe_pcode.o \
>>>> xe_pm.o \
>>>> xe_preempt_fence.o \
>>>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>>>> index 0a66555229e9..8c749b378a92 100644
>>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>>>> @@ -465,6 +465,9 @@ struct xe_device {
>>>> /** @pci_state: PCI state of device */
>>>> struct pci_saved_state *pci_state;
>>>>
>>>> + /** @pci_device_is_reset: device went through PCIe FLR */
>>>> + bool pci_device_is_reset;
>>>> +
>>>> /* private: */
>>>>
>>>> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> index 509649d0e65e..efba0fbe2f5c 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
>>>> @@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg)
>>>> return;
>>>> }
>>>>
>>>> + /* We already have done this before going through a reset, so skip here */
>>>> + if (xe->pci_device_is_reset)
>>>> + return;
>>>> +
>>>> XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL));
>>>> XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
>>>> XE_WARN_ON(xe_guc_pc_stop(pc));
>>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>>>> index a62300990e19..b5a582afc9e7 100644
>>>> --- a/drivers/gpu/drm/xe/xe_pci.c
>>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>>>> @@ -23,6 +23,7 @@
>>>> #include "xe_macros.h"
>>>> #include "xe_mmio.h"
>>>> #include "xe_module.h"
>>>> +#include "xe_pci_err.h"
>>>> #include "xe_pci_types.h"
>>>> #include "xe_pm.h"
>>>> #include "xe_sriov.h"
>>>> @@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev)
>>>> pci_set_drvdata(pdev, NULL);
>>>> }
>>>>
>>>> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>>> {
>>>> const struct xe_device_desc *desc = (const void *)ent->driver_data;
>>>> const struct xe_subplatform_desc *subplatform_desc;
>>>> @@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = {
>>>> };
>>>> #endif
>>>>
>>>> +static const struct pci_error_handlers xe_pci_err_handlers = {
>>>> + .reset_prepare = xe_pci_reset_prepare,
>>>> + .reset_done = xe_pci_reset_done,
>>>> +};
>>>> +
>>>> static struct pci_driver xe_pci_driver = {
>>>> .name = DRIVER_NAME,
>>>> .id_table = pciidlist,
>>>> @@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = {
>>>> #ifdef CONFIG_PM_SLEEP
>>>> .driver.pm = &xe_pm_ops,
>>>> #endif
>>>> + .err_handler = &xe_pci_err_handlers,
>>>> };
>>>>
>>>> int xe_register_pci_driver(void)
>>>> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
>>>> index 73b90a430d1f..9faf5380a09e 100644
>>>> --- a/drivers/gpu/drm/xe/xe_pci.h
>>>> +++ b/drivers/gpu/drm/xe/xe_pci.h
>>>> @@ -7,8 +7,10 @@
>>>> #define _XE_PCI_H_
>>>>
>>>> struct pci_dev;
>>>> +struct pci_device_id;
>>>>
>>>> int xe_register_pci_driver(void);
>>>> void xe_unregister_pci_driver(void);
>>>> void xe_load_pci_state(struct pci_dev *pdev);
>>>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
>>>> #endif
>>>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
>>>> new file mode 100644
>>>> index 000000000000..5306925ea2fa
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
>>>> @@ -0,0 +1,88 @@
>>>> +// SPDX-License-Identifier: MIT
>>>> +/*
>>>> + * Copyright © 2024 Intel Corporation
>>>> + */
>>>> +
>>>> +#include <linux/pci.h>
>>>> +#include <drm/drm_drv.h>
>>>> +
>>>> +#include "xe_device.h"
>>>> +#include "xe_gt.h"
>>>> +#include "xe_gt_printk.h"
>>>> +#include "xe_pci.h"
>>>> +#include "xe_pci_err.h"
>>>> +#include "xe_pm.h"
>>>> +#include "xe_uc.h"
>>>> +
>>>> +/**
>>>> + * xe_pci_reset_prepare - Called when user issued a PCIe reset
>>>> + * via /sys/bus/pci/devices/.../reset.
>>>> + * @pdev: PCI device struct
>>>> + */
>>>> +void xe_pci_reset_prepare(struct pci_dev *pdev)
>>>> +{
>>>> + struct xe_device *xe = pci_get_drvdata(pdev);
>>>> + struct xe_gt *gt;
>>>> + int id, err;
>>>> +
>>>> + pci_warn(pdev, "preparing for PCIe reset\n");
>>>> +
>>>> + drm_warn(&xe->drm, "removing device access to userspace\n");
>>> Warn? Shouldn't it be info?
>> I believe warn is appropriate as this is not a usual path the users apps expect
>> to hit and as they loose device connection.
> It's an expected path after issuing a reset via /sys/bus/pci/devices/.../reset.
> DRM device disappearing - yeah, that's probably not expected, fully
> agree on that.
>
>>>> + drm_dev_unplug(&xe->drm);
>>>> +
>>>> + xe_pm_runtime_get(xe);
>>>> + /* idle the GTs */
>>>> + for_each_gt(gt, xe, id) {
>>>> + err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>>> + if (err)
>>>> + goto reset;
>>>> + xe_uc_reset_prepare(>->uc);
>>>> + xe_gt_idle(gt);
>>>> + err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
>>>> + XE_WARN_ON(err);
>>>> + }
>>>> + xe_pm_runtime_put(xe);
>>>> +
>>>> +reset:
>>>> + pci_disable_device(pdev);
>>>> +}
>>>> +
>>>> +/**
>>>> + * xe_pci_reset_done - Called when PCIe reset is done.
>>>> + * @pdev: PCI device struct
>>>> + */
>>>> +void xe_pci_reset_done(struct pci_dev *pdev)
>>>> +{
>>>> + const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
>>>> + struct xe_device *xe = pci_get_drvdata(pdev);
>>>> +
>>>> + dev_info(&pdev->dev,
>>>> + "device went through PCIe reset, reenabling the device\n");
>>>> +
>>>> + if (pci_enable_device(pdev)) {
>>>> + dev_err(&pdev->dev,
>>>> + "Cannot re-enable PCI device after reset\n");
>>>> + return;
>>>> + }
>>>> + pci_set_master(pdev);
>>>> + xe_load_pci_state(pdev);
>>>> +
>>>> + xe->pci_device_is_reset = true;
>>>> + /*
>>>> + * We want to completely clean the driver and even destroy
>>>> + * the xe private data and reinitialize afresh similar to
>>>> + * probe
>>>> + */
>>>> + pdev->driver->remove(pdev);
>>> Do we really want to do that?
>>> First of all, that fairly unusual - none of the other PCI drivers makes
>>> changes in the device/driver model during reset, which makes me wonder
>>> if this is really what the user expects.
>>> I would expect that the device is reset, but the driver is not unloaded
>>> and previously created FDs are still valid.
>> We cannot continue to work with previous instance of driver as post
>> reset GuC , LMEM and most of the soc is reset, so we need to reinitialize
>> device afresh like in driver probe, hence we remove to clean the previous
>> stale driver state and re-probe again.
> GuC, LMEM and most of the soc is reset when we're doing S3 / S4, and
> yet, we're not removing the driver in this scenario.
ya there we are migrating the buffer from device memory to system memory, but I'm little
doubtful if we can do that in case of reset as device might already be a bad state or error state.
Also, that this is a reset and expectation is to have clean slate not resume from previous state.
>
>> All applications are expected to reopen the device handles afresh.
> Current UMDs are not handling that case.
strange then how is it handled today, without these callback implemented
the only way to do FLR and recover from it is to unbind the driver->initiate FLR->rebind.
also i do not think it is automatically handled the admin or privileged application initiating a
reset should re launch those UMDS.
>
>>> Note that messing with the driver model at reset also makes it difficult
>>> to do a proper cleanup if SR-IOV is enabled, as PCI-core expects drivers
>>> to remove VF devices during driver->remove.
>>> Removal of said devices depends on pci_cfg_access_lock, which is already
>>> held for the duration of the reset (wh
>> I haven't verified SRIOV use case, i believe we can take this up as next step.
>> Also, since this is not an automatic reset but a user/admin initiated i believe
>> the onus can be on admin to close all VFs before initiating a reset.
> That's not the contract for reset. It's not a "user/admin" interface,
> there doesn't necessarily have to be an "interactive" user interfacing
> with it through shell.
> It's just a regular stable sysfs uAPI - it could very well be called by
> application as well.
the application issuing a reset cannot be a normal one it should be a privileged one,
and not every application is granted that access eg; L0 sysman who initiates the reset
takes care of closing all open connections before initiating a reset.
>
>>> ich includes calling reset_done),
>>> which will cause a deadlock.
>>>
>>> In current form, it also causes WARN if there are open FDs to DRM
>>> device during reset.
>> I did verify using igt@device_reset@reset-bound which has a open connection haven't seen this error
>> but will re verify again but again certain warns are expected but driver reload should be successful.
>> similarly here as well we can expect the admin to close any applications using the device before
>> initiating a reset.
> Can we expect this?
> If we can - why are you stating that "all applications are expected to
> reopen the device handles afresh"?
> I believe that the expectation for reset is that it behaves simiarly to
> the PM flows, which is mostly transparent to the users.
Like I mentioned the definition of a reset is to start afresh from clean state
and since we cleared previous state of device applications cannot continue to work
without re initializing device connections.
and today that is the case even without these callbacks.
Thanks,
Aravind.
>
> Thanks,
> -Michał
>
>> Thanks,
>>
>> Aravind.
>>> [29357.277699] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:02.0/tile0' 01:06:58 [142/47263]
>>> [29357.286158] CPU: 7 PID: 3479 Comm: bash Not tainted 6.9.0-rc5-xe+ #78
>>> [29357.305870] Call Trace:
>>> [29357.308342] <TASK>
>>> [29357.310453] dump_stack_lvl+0x139/0x1d0
>>> [29357.314305] ? __pfx_dump_stack_lvl+0x10/0x10
>>> [29357.318680] ? __pfx__printk+0x10/0x10
>>> [29357.322450] ? kmalloc_trace+0x1c1/0x3a0
>>> [29357.326394] ? sysfs_create_dir_ns+0x162/0x210
>>> [29357.330861] sysfs_create_dir_ns+0x195/0x210
>>> [29357.335154] ? __pfx_sysfs_create_dir_ns+0x10/0x10
>>> [29357.339965] ? strcmp+0x2f/0x60
>>> [29357.343134] kobject_add_internal+0x2af/0x600
>>> [29357.347517] kobject_add+0xfb/0x190
>>> [29357.351028] ? __link_object+0x1c7/0x280
>>> [29357.354976] ? __pfx_kobject_add+0x10/0x10
>>> [29357.359093] ? __create_object+0x62/0x140
>>> [29357.363111] ? rcu_is_watching+0x20/0x50
>>> [29357.367055] ? kmalloc_trace+0x1c1/0x3a0
>>> [29357.370998] ? xe_tile_sysfs_init+0x4b/0x100 [xe]
>>> [29357.376016] xe_tile_sysfs_init+0x91/0x100 [xe]
>>> [29357.380868] xe_tile_init_noalloc+0xaf/0xc0 [xe]
>>> [29357.385936] xe_device_probe+0x60c/0x750 [xe]
>>> [29357.390674] ? __asan_memcpy+0x40/0x70
>>> [29357.394461] ? __drmm_add_action+0x1ac/0x210 [drm]
>>> [29357.399413] xe_pci_probe+0x5dc/0x680 [xe]
>>> [29357.403882] pci_reset_function+0x108/0x140
>>> [29357.408100] reset_store+0xb5/0x120
>>> [29357.411623] ? __pfx_reset_store+0x10/0x10
>>> [29357.415751] ? __pfx_sysfs_kf_write+0x10/0x10
>>> [29357.420149] kernfs_fop_write_iter+0x1b8/0x260
>>> [29357.424620] vfs_write+0x5ce/0x660
>>> [29357.428067] ? __pfx_vfs_write+0x10/0x10
>>> [29357.432028] ? __rcu_read_unlock+0x60/0x90
>>> [29357.436181] ksys_write+0xe4/0x190
>>> [29357.439612] ? __pfx_ksys_write+0x10/0x10
>>> [29357.443657] do_syscall_64+0x7b/0x120
>>> [29357.447348] entry_SYSCALL_64_after_hwframe+0x76/0x7e
>>> [29357.452423] RIP: 0033:0x7f4f1ff14887
>>> [29357.456030] Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
>>> [29357.474761] RSP: 002b:00007ffe54e95068 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
>>> [29357.482343] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f4f1ff14887
>>> [29357.489487] RDX: 0000000000000002 RSI: 0000559eb4076e30 RDI: 0000000000000001
>>> [29357.496630] RBP: 0000559eb4076e30 R08: 0000000000000000 R09: 0000559eb4076e30
>>> [29357.503775] R10: 0000000000000077 R11: 0000000000000246 R12: 0000000000000002
>>> [29357.510947] R13: 00007f4f2001b780 R14: 00007f4f20017600 R15: 00007f4f20016a00
>>> [29357.518174] </TASK>
>>> [29357.520539] kobject: kobject_add_internal failed for tile0 with -EEXIST, don't try to register things with the same name in the same directory.
>>>
>>> -Michał
>>>
>>>> + if (pci_dev_msi_enabled(pdev))
>>>> + pci_free_irq_vectors(pdev);
>>>> +
>>>> + devm_drm_dev_release_action(&xe->drm);
>>>> + pci_disable_device(pdev);
>>>> +
>>>> + /*
>>>> + * if this fails the driver might be in a stale state, only option is
>>>> + * to unbind and rebind
>>>> + */
>>>> + xe_pci_probe(pdev, ent);
>>>> +}
>>>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.h b/drivers/gpu/drm/xe/xe_pci_err.h
>>>> new file mode 100644
>>>> index 000000000000..95a4c8ce9cf1
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/xe/xe_pci_err.h
>>>> @@ -0,0 +1,13 @@
>>>> +/* SPDX-License-Identifier: MIT */
>>>> +/*
>>>> + * Copyright © 2024 Intel Corporation
>>>> + */
>>>> +
>>>> +#ifndef _XE_PCI_ERR_H_
>>>> +#define _XE_PCI_ERR_H_
>>>> +
>>>> +struct pci_dev;
>>>> +
>>>> +void xe_pci_reset_prepare(struct pci_dev *pdev);
>>>> +void xe_pci_reset_done(struct pci_dev *pdev);
>>>> +#endif
>>>> --
>>>> 2.25.1
>>>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR
2024-04-25 6:17 ` Aravind Iddamsetty
@ 2024-04-26 0:53 ` Michał Winiarski
0 siblings, 0 replies; 18+ messages in thread
From: Michał Winiarski @ 2024-04-26 0:53 UTC (permalink / raw)
To: Aravind Iddamsetty
Cc: dri-devel, daniel, maarten.lankhorst, airlied, mripard,
tzimmermann, rodrigo.vivi, intel-xe, Lucas De Marchi,
Michal Wajdeczko
On Thu, Apr 25, 2024 at 11:47:46AM +0530, Aravind Iddamsetty wrote:
>
> On 25/04/24 04:59, Michał Winiarski wrote:
> > On Wed, Apr 24, 2024 at 10:42:59AM +0530, Aravind Iddamsetty wrote:
> >> On 24/04/24 05:19, Michał Winiarski wrote:
> >>> On Mon, Apr 22, 2024 at 12:27:56PM +0530, Aravind Iddamsetty wrote:
> >>>> PCI subsystem provides callbacks to inform the driver about a request to
> >>>> do function level reset by user, initiated by writing to sysfs entry
> >>>> /sys/bus/pci/devices/.../reset. This will allow the driver to handle FLR
> >>>> without the need to do unbind and rebind as the driver needs to
> >>>> reinitialize the device afresh post FLR.
> >>>>
> >>>> v2:
> >>>> 1. separate out gt idle and pci save/restore to a separate patch (Lucas)
> >>>> 2. Fixed the warnings seen around xe_guc_submit_stop, xe_guc_puc_fini
> >>>>
> >>>> v3: declare xe_pci_err_handlers as static(Michal)
> >>>>
> >>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> >>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>>>
> >>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> >>>> ---
> >>>> drivers/gpu/drm/xe/Makefile | 1 +
> >>>> drivers/gpu/drm/xe/xe_device_types.h | 3 +
> >>>> drivers/gpu/drm/xe/xe_guc_pc.c | 4 ++
> >>>> drivers/gpu/drm/xe/xe_pci.c | 9 ++-
> >>>> drivers/gpu/drm/xe/xe_pci.h | 2 +
> >>>> drivers/gpu/drm/xe/xe_pci_err.c | 88 ++++++++++++++++++++++++++++
> >>>> drivers/gpu/drm/xe/xe_pci_err.h | 13 ++++
> >>>> 7 files changed, 119 insertions(+), 1 deletion(-)
> >>>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
> >>>> create mode 100644 drivers/gpu/drm/xe/xe_pci_err.h
> >>>>
> >>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> >>>> index 8bc62bfbc679..693971a1fac0 100644
> >>>> --- a/drivers/gpu/drm/xe/Makefile
> >>>> +++ b/drivers/gpu/drm/xe/Makefile
> >>>> @@ -117,6 +117,7 @@ xe-y += xe_bb.o \
> >>>> xe_module.o \
> >>>> xe_pat.o \
> >>>> xe_pci.o \
> >>>> + xe_pci_err.o \
> >>>> xe_pcode.o \
> >>>> xe_pm.o \
> >>>> xe_preempt_fence.o \
> >>>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> >>>> index 0a66555229e9..8c749b378a92 100644
> >>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
> >>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> >>>> @@ -465,6 +465,9 @@ struct xe_device {
> >>>> /** @pci_state: PCI state of device */
> >>>> struct pci_saved_state *pci_state;
> >>>>
> >>>> + /** @pci_device_is_reset: device went through PCIe FLR */
> >>>> + bool pci_device_is_reset;
> >>>> +
> >>>> /* private: */
> >>>>
> >>>> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> >>>> diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c b/drivers/gpu/drm/xe/xe_guc_pc.c
> >>>> index 509649d0e65e..efba0fbe2f5c 100644
> >>>> --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> >>>> +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> >>>> @@ -902,6 +902,10 @@ static void xe_guc_pc_fini(struct drm_device *drm, void *arg)
> >>>> return;
> >>>> }
> >>>>
> >>>> + /* We already have done this before going through a reset, so skip here */
> >>>> + if (xe->pci_device_is_reset)
> >>>> + return;
> >>>> +
> >>>> XE_WARN_ON(xe_force_wake_get(gt_to_fw(pc_to_gt(pc)), XE_FORCEWAKE_ALL));
> >>>> XE_WARN_ON(xe_guc_pc_gucrc_disable(pc));
> >>>> XE_WARN_ON(xe_guc_pc_stop(pc));
> >>>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> >>>> index a62300990e19..b5a582afc9e7 100644
> >>>> --- a/drivers/gpu/drm/xe/xe_pci.c
> >>>> +++ b/drivers/gpu/drm/xe/xe_pci.c
> >>>> @@ -23,6 +23,7 @@
> >>>> #include "xe_macros.h"
> >>>> #include "xe_mmio.h"
> >>>> #include "xe_module.h"
> >>>> +#include "xe_pci_err.h"
> >>>> #include "xe_pci_types.h"
> >>>> #include "xe_pm.h"
> >>>> #include "xe_sriov.h"
> >>>> @@ -738,7 +739,7 @@ static void xe_pci_remove(struct pci_dev *pdev)
> >>>> pci_set_drvdata(pdev, NULL);
> >>>> }
> >>>>
> >>>> -static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >>>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >>>> {
> >>>> const struct xe_device_desc *desc = (const void *)ent->driver_data;
> >>>> const struct xe_subplatform_desc *subplatform_desc;
> >>>> @@ -986,6 +987,11 @@ static const struct dev_pm_ops xe_pm_ops = {
> >>>> };
> >>>> #endif
> >>>>
> >>>> +static const struct pci_error_handlers xe_pci_err_handlers = {
> >>>> + .reset_prepare = xe_pci_reset_prepare,
> >>>> + .reset_done = xe_pci_reset_done,
> >>>> +};
> >>>> +
> >>>> static struct pci_driver xe_pci_driver = {
> >>>> .name = DRIVER_NAME,
> >>>> .id_table = pciidlist,
> >>>> @@ -995,6 +1001,7 @@ static struct pci_driver xe_pci_driver = {
> >>>> #ifdef CONFIG_PM_SLEEP
> >>>> .driver.pm = &xe_pm_ops,
> >>>> #endif
> >>>> + .err_handler = &xe_pci_err_handlers,
> >>>> };
> >>>>
> >>>> int xe_register_pci_driver(void)
> >>>> diff --git a/drivers/gpu/drm/xe/xe_pci.h b/drivers/gpu/drm/xe/xe_pci.h
> >>>> index 73b90a430d1f..9faf5380a09e 100644
> >>>> --- a/drivers/gpu/drm/xe/xe_pci.h
> >>>> +++ b/drivers/gpu/drm/xe/xe_pci.h
> >>>> @@ -7,8 +7,10 @@
> >>>> #define _XE_PCI_H_
> >>>>
> >>>> struct pci_dev;
> >>>> +struct pci_device_id;
> >>>>
> >>>> int xe_register_pci_driver(void);
> >>>> void xe_unregister_pci_driver(void);
> >>>> void xe_load_pci_state(struct pci_dev *pdev);
> >>>> +int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent);
> >>>> #endif
> >>>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.c b/drivers/gpu/drm/xe/xe_pci_err.c
> >>>> new file mode 100644
> >>>> index 000000000000..5306925ea2fa
> >>>> --- /dev/null
> >>>> +++ b/drivers/gpu/drm/xe/xe_pci_err.c
> >>>> @@ -0,0 +1,88 @@
> >>>> +// SPDX-License-Identifier: MIT
> >>>> +/*
> >>>> + * Copyright © 2024 Intel Corporation
> >>>> + */
> >>>> +
> >>>> +#include <linux/pci.h>
> >>>> +#include <drm/drm_drv.h>
> >>>> +
> >>>> +#include "xe_device.h"
> >>>> +#include "xe_gt.h"
> >>>> +#include "xe_gt_printk.h"
> >>>> +#include "xe_pci.h"
> >>>> +#include "xe_pci_err.h"
> >>>> +#include "xe_pm.h"
> >>>> +#include "xe_uc.h"
> >>>> +
> >>>> +/**
> >>>> + * xe_pci_reset_prepare - Called when user issued a PCIe reset
> >>>> + * via /sys/bus/pci/devices/.../reset.
> >>>> + * @pdev: PCI device struct
> >>>> + */
> >>>> +void xe_pci_reset_prepare(struct pci_dev *pdev)
> >>>> +{
> >>>> + struct xe_device *xe = pci_get_drvdata(pdev);
> >>>> + struct xe_gt *gt;
> >>>> + int id, err;
> >>>> +
> >>>> + pci_warn(pdev, "preparing for PCIe reset\n");
> >>>> +
> >>>> + drm_warn(&xe->drm, "removing device access to userspace\n");
> >>> Warn? Shouldn't it be info?
> >> I believe warn is appropriate as this is not a usual path the users apps expect
> >> to hit and as they loose device connection.
> > It's an expected path after issuing a reset via /sys/bus/pci/devices/.../reset.
> > DRM device disappearing - yeah, that's probably not expected, fully
> > agree on that.
> >
> >>>> + drm_dev_unplug(&xe->drm);
> >>>> +
> >>>> + xe_pm_runtime_get(xe);
> >>>> + /* idle the GTs */
> >>>> + for_each_gt(gt, xe, id) {
> >>>> + err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> >>>> + if (err)
> >>>> + goto reset;
> >>>> + xe_uc_reset_prepare(>->uc);
> >>>> + xe_gt_idle(gt);
> >>>> + err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> >>>> + XE_WARN_ON(err);
> >>>> + }
> >>>> + xe_pm_runtime_put(xe);
> >>>> +
> >>>> +reset:
> >>>> + pci_disable_device(pdev);
> >>>> +}
> >>>> +
> >>>> +/**
> >>>> + * xe_pci_reset_done - Called when PCIe reset is done.
> >>>> + * @pdev: PCI device struct
> >>>> + */
> >>>> +void xe_pci_reset_done(struct pci_dev *pdev)
> >>>> +{
> >>>> + const struct pci_device_id *ent = pci_match_id(pdev->driver->id_table, pdev);
> >>>> + struct xe_device *xe = pci_get_drvdata(pdev);
> >>>> +
> >>>> + dev_info(&pdev->dev,
> >>>> + "device went through PCIe reset, reenabling the device\n");
> >>>> +
> >>>> + if (pci_enable_device(pdev)) {
> >>>> + dev_err(&pdev->dev,
> >>>> + "Cannot re-enable PCI device after reset\n");
> >>>> + return;
> >>>> + }
> >>>> + pci_set_master(pdev);
> >>>> + xe_load_pci_state(pdev);
> >>>> +
> >>>> + xe->pci_device_is_reset = true;
> >>>> + /*
> >>>> + * We want to completely clean the driver and even destroy
> >>>> + * the xe private data and reinitialize afresh similar to
> >>>> + * probe
> >>>> + */
> >>>> + pdev->driver->remove(pdev);
> >>> Do we really want to do that?
> >>> First of all, that fairly unusual - none of the other PCI drivers makes
> >>> changes in the device/driver model during reset, which makes me wonder
> >>> if this is really what the user expects.
> >>> I would expect that the device is reset, but the driver is not unloaded
> >>> and previously created FDs are still valid.
> >> We cannot continue to work with previous instance of driver as post
> >> reset GuC , LMEM and most of the soc is reset, so we need to reinitialize
> >> device afresh like in driver probe, hence we remove to clean the previous
> >> stale driver state and re-probe again.
> > GuC, LMEM and most of the soc is reset when we're doing S3 / S4, and
> > yet, we're not removing the driver in this scenario.
> ya there we are migrating the buffer from device memory to system memory, but I'm little
> doubtful if we can do that in case of reset as device might already be a bad state or error state.
> Also, that this is a reset and expectation is to have clean slate not resume from previous state.
We don't necessarily have to migrate - I'm just trying to highlight that
we do have most of the code needed to reinitialize the device extracted
as part of PM flows - there's no need to detach the driver from the
device in order to achieve that.
> >
> >> All applications are expected to reopen the device handles afresh.
> > Current UMDs are not handling that case.
>
> strange then how is it handled today, without these callback implemented
> the only way to do FLR and recover from it is to unbind the driver->initiate FLR->rebind.
> also i do not think it is automatically handled the admin or privileged application initiating a
> reset should re launch those UMDS.
It's possible to just call reset "today" without unbinding the driver,
and it's not a user error to do so.
The fact that the driver can't deal with it is a bug and the unbind ->
reset -> bind dance is just a way to workaround it.
If it is absolutely necessary to kill all clients - the driver could
just do whatever it needs in order to reinitialize on its own instead of
setting up driver-specific expectations for generic PCI-level sysfs
interface,
See e.g. what habanalabs driver is doing as part of its .reset_done()
handler:
https://elixir.bootlin.com/linux/latest/source/drivers/accel/habanalabs/common/device.c#L1381
> >
> >>> Note that messing with the driver model at reset also makes it difficult
> >>> to do a proper cleanup if SR-IOV is enabled, as PCI-core expects drivers
> >>> to remove VF devices during driver->remove.
> >>> Removal of said devices depends on pci_cfg_access_lock, which is already
> >>> held for the duration of the reset (wh
> >> I haven't verified SRIOV use case, i believe we can take this up as next step.
> >> Also, since this is not an automatic reset but a user/admin initiated i believe
> >> the onus can be on admin to close all VFs before initiating a reset.
> > That's not the contract for reset. It's not a "user/admin" interface,
> > there doesn't necessarily have to be an "interactive" user interfacing
> > with it through shell.
> > It's just a regular stable sysfs uAPI - it could very well be called by
> > application as well.
> the application issuing a reset cannot be a normal one it should be a privileged one,
> and not every application is granted that access eg; L0 sysman who initiates the reset
> takes care of closing all open connections before initiating a reset.
It's the kernel's job to make sure the reset goes smoothly - not
the userspace application that's using the interface (privileged or not).
> >
> >>> ich includes calling reset_done),
> >>> which will cause a deadlock.
> >>>
> >>> In current form, it also causes WARN if there are open FDs to DRM
> >>> device during reset.
> >> I did verify using igt@device_reset@reset-bound which has a open connection haven't seen this error
> >> but will re verify again but again certain warns are expected but driver reload should be successful.
> >> similarly here as well we can expect the admin to close any applications using the device before
> >> initiating a reset.
> > Can we expect this?
> > If we can - why are you stating that "all applications are expected to
> > reopen the device handles afresh"?
> > I believe that the expectation for reset is that it behaves simiarly to
> > the PM flows, which is mostly transparent to the users.
> Like I mentioned the definition of a reset is to start afresh from clean state
> and since we cleared previous state of device applications cannot continue to work
> without re initializing device connections.
>
> and today that is the case even without these callbacks.
It's not. Doing reset today will result in both existing clients and new
clients to not be able to submit any work. Driver will just throw
multitude of errors, warnings and generally won't be able to recover
itself. Some of those warnings are WARNs, which means that the kernel is
now tainted (or it gets promoted to panic, on panic_on_warn systems).
There is a workaround that allows the user to avoid this behavior - just
unbind -> reset -> bind, and that allows new clients to be able to
submit work.
The series is just automating that - calling reset does the unbind and
bind for the user. It moves the workaround (unbind -> reset -> bind)
from userspace to kernel but that's not what the user wanted. If the
user wanted to destroy the device and create a new one - there's an
interface for that (unbind -> bind).
Thanks,
-Michał
>
> Thanks,
> Aravind.
> >
> > Thanks,
> > -Michał
> >
> >> Thanks,
> >>
> >> Aravind.
> >>> [29357.277699] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:02.0/tile0' 01:06:58 [142/47263]
> >>> [29357.286158] CPU: 7 PID: 3479 Comm: bash Not tainted 6.9.0-rc5-xe+ #78
> >>> [29357.305870] Call Trace:
> >>> [29357.308342] <TASK>
> >>> [29357.310453] dump_stack_lvl+0x139/0x1d0
> >>> [29357.314305] ? __pfx_dump_stack_lvl+0x10/0x10
> >>> [29357.318680] ? __pfx__printk+0x10/0x10
> >>> [29357.322450] ? kmalloc_trace+0x1c1/0x3a0
> >>> [29357.326394] ? sysfs_create_dir_ns+0x162/0x210
> >>> [29357.330861] sysfs_create_dir_ns+0x195/0x210
> >>> [29357.335154] ? __pfx_sysfs_create_dir_ns+0x10/0x10
> >>> [29357.339965] ? strcmp+0x2f/0x60
> >>> [29357.343134] kobject_add_internal+0x2af/0x600
> >>> [29357.347517] kobject_add+0xfb/0x190
> >>> [29357.351028] ? __link_object+0x1c7/0x280
> >>> [29357.354976] ? __pfx_kobject_add+0x10/0x10
> >>> [29357.359093] ? __create_object+0x62/0x140
> >>> [29357.363111] ? rcu_is_watching+0x20/0x50
> >>> [29357.367055] ? kmalloc_trace+0x1c1/0x3a0
> >>> [29357.370998] ? xe_tile_sysfs_init+0x4b/0x100 [xe]
> >>> [29357.376016] xe_tile_sysfs_init+0x91/0x100 [xe]
> >>> [29357.380868] xe_tile_init_noalloc+0xaf/0xc0 [xe]
> >>> [29357.385936] xe_device_probe+0x60c/0x750 [xe]
> >>> [29357.390674] ? __asan_memcpy+0x40/0x70
> >>> [29357.394461] ? __drmm_add_action+0x1ac/0x210 [drm]
> >>> [29357.399413] xe_pci_probe+0x5dc/0x680 [xe]
> >>> [29357.403882] pci_reset_function+0x108/0x140
> >>> [29357.408100] reset_store+0xb5/0x120
> >>> [29357.411623] ? __pfx_reset_store+0x10/0x10
> >>> [29357.415751] ? __pfx_sysfs_kf_write+0x10/0x10
> >>> [29357.420149] kernfs_fop_write_iter+0x1b8/0x260
> >>> [29357.424620] vfs_write+0x5ce/0x660
> >>> [29357.428067] ? __pfx_vfs_write+0x10/0x10
> >>> [29357.432028] ? __rcu_read_unlock+0x60/0x90
> >>> [29357.436181] ksys_write+0xe4/0x190
> >>> [29357.439612] ? __pfx_ksys_write+0x10/0x10
> >>> [29357.443657] do_syscall_64+0x7b/0x120
> >>> [29357.447348] entry_SYSCALL_64_after_hwframe+0x76/0x7e
> >>> [29357.452423] RIP: 0033:0x7f4f1ff14887
> >>> [29357.456030] Code: 10 00 f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb b7 0f 1f 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
> >>> [29357.474761] RSP: 002b:00007ffe54e95068 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
> >>> [29357.482343] RAX: ffffffffffffffda RBX: 0000000000000002 RCX: 00007f4f1ff14887
> >>> [29357.489487] RDX: 0000000000000002 RSI: 0000559eb4076e30 RDI: 0000000000000001
> >>> [29357.496630] RBP: 0000559eb4076e30 R08: 0000000000000000 R09: 0000559eb4076e30
> >>> [29357.503775] R10: 0000000000000077 R11: 0000000000000246 R12: 0000000000000002
> >>> [29357.510947] R13: 00007f4f2001b780 R14: 00007f4f20017600 R15: 00007f4f20016a00
> >>> [29357.518174] </TASK>
> >>> [29357.520539] kobject: kobject_add_internal failed for tile0 with -EEXIST, don't try to register things with the same name in the same directory.
> >>>
> >>> -Michał
> >>>
> >>>> + if (pci_dev_msi_enabled(pdev))
> >>>> + pci_free_irq_vectors(pdev);
> >>>> +
> >>>> + devm_drm_dev_release_action(&xe->drm);
> >>>> + pci_disable_device(pdev);
> >>>> +
> >>>> + /*
> >>>> + * if this fails the driver might be in a stale state, only option is
> >>>> + * to unbind and rebind
> >>>> + */
> >>>> + xe_pci_probe(pdev, ent);
> >>>> +}
> >>>> diff --git a/drivers/gpu/drm/xe/xe_pci_err.h b/drivers/gpu/drm/xe/xe_pci_err.h
> >>>> new file mode 100644
> >>>> index 000000000000..95a4c8ce9cf1
> >>>> --- /dev/null
> >>>> +++ b/drivers/gpu/drm/xe/xe_pci_err.h
> >>>> @@ -0,0 +1,13 @@
> >>>> +/* SPDX-License-Identifier: MIT */
> >>>> +/*
> >>>> + * Copyright © 2024 Intel Corporation
> >>>> + */
> >>>> +
> >>>> +#ifndef _XE_PCI_ERR_H_
> >>>> +#define _XE_PCI_ERR_H_
> >>>> +
> >>>> +struct pci_dev;
> >>>> +
> >>>> +void xe_pci_reset_prepare(struct pci_dev *pdev);
> >>>> +void xe_pci_reset_done(struct pci_dev *pdev);
> >>>> +#endif
> >>>> --
> >>>> 2.25.1
> >>>>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2024-04-26 0:53 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-19 8:58 [PATCH v3 0/4] drm/xe: Support PCIe FLR Aravind Iddamsetty
2024-04-19 8:58 ` [PATCH v2 1/4] drm: add devm release action Aravind Iddamsetty
2024-04-19 9:11 ` Maxime Ripard
2024-04-20 8:12 ` Aravind Iddamsetty
2024-04-19 8:58 ` [PATCH 2/4] drm/xe: Save and restore PCI state Aravind Iddamsetty
2024-04-19 8:58 ` [PATCH v2 3/4] drm/xe: Extract xe_gt_idle() helper Aravind Iddamsetty
2024-04-23 4:21 ` Ghimiray, Himal Prasad
2024-04-19 8:58 ` [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR Aravind Iddamsetty
-- strict thread matches above, loose matches on Subject: below --
2024-04-22 6:57 [PATCH v4 0/4] drm/xe: " Aravind Iddamsetty
2024-04-22 6:57 ` [PATCH v3 4/4] drm/xe/FLR: " Aravind Iddamsetty
2024-04-23 15:04 ` Nilawar, Badal
2024-04-24 3:12 ` Aravind Iddamsetty
2024-04-24 11:12 ` Nilawar, Badal
2024-04-25 4:07 ` Aravind Iddamsetty
2024-04-23 23:49 ` Michał Winiarski
2024-04-24 5:12 ` Aravind Iddamsetty
2024-04-24 23:29 ` Michał Winiarski
2024-04-25 6:17 ` Aravind Iddamsetty
2024-04-26 0:53 ` Michał Winiarski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox