[PATCH v4 0/4] drm/xe: Support PCIe FLR

[PATCH v4 0/4] drm/xe: Support PCIe FLR

[Aravind Iddamsetty]

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.

v4: add kernel-doc

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            | 13 ++++
 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, 204 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

[PATCH v3 1/4] drm: add devm release action

[Aravind Iddamsetty]

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)

v3: add kernel-doc (Maxime Ripard)

Cc: Maxime Ripard <mripard@kernel.org>
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 | 13 +++++++++++++
 include/drm/drm_drv.h     |  2 ++
 2 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 243cacb3575c..9d0409165f1e 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
 					devm_drm_dev_init_release, dev);
 }
 
+/**
+ * devm_drm_dev_release_action - Call the final release action of the device
+ * @dev: DRM device
+ *
+ * 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.
+ */
+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

[PATCH 2/4] drm/xe: Save and restore PCI state

[Aravind Iddamsetty]

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 fb20c9828563..a62300990e19 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -393,6 +393,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)
 {
@@ -698,6 +733,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);
 }
 
@@ -798,6 +835,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:
@@ -849,7 +889,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;
@@ -873,6 +913,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;
@@ -890,7 +932,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);
@@ -915,7 +957,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

[PATCH v2 3/4] drm/xe: Extract xe_gt_idle() helper

[Aravind Iddamsetty]

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(&gt->uc);
+	xe_uc_stop_prepare(&gt->uc);
+	xe_gt_pagefault_reset(gt);
+	xe_uc_stop(&gt->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(&gt->uc);
-	xe_uc_stop_prepare(&gt->uc);
-	xe_gt_pagefault_reset(gt);
-
-	err = xe_uc_stop(&gt->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

[PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR

[Aravind Iddamsetty]

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(&gt->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

Re: [PATCH v3 1/4] drm: add devm release action

[Rodrigo Vivi]

On Mon, Apr 22, 2024 at 12:27:53PM +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)
> 
> v3: add kernel-doc (Maxime Ripard)
> 
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 

please avoid these empty lines here.... cc, rv-b, sign-offs, links,
etc are all in the same block.

> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_drv.c | 13 +++++++++++++
>  include/drm/drm_drv.h     |  2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 243cacb3575c..9d0409165f1e 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
>  					devm_drm_dev_init_release, dev);
>  }
>  
> +/**
> + * devm_drm_dev_release_action - Call the final release action of the device

Seeing the doc here gave me a second thought....

the original release should be renamed to _devm_drm_dev_release
and this should be called devm_drm_dev_release without the 'action' word.

> + * @dev: DRM device
> + *
> + * 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.

But also, this made me more confusing on why this is needed.
Why can't we call put and get back?

The next needs to make it clear on why we need to release in these
scenarios regarless of the number of counters. But do we really
want this?

Can we block the flr if we have users? Perhaps this is the reason
that on my side the flr was not that clean? beucase I had display
so I had clients connected?

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

Re: [PATCH v3 1/4] drm: add devm release action

[Aravind Iddamsetty]

On 23/04/24 02:24, Rodrigo Vivi wrote:
> On Mon, Apr 22, 2024 at 12:27:53PM +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)
>>
>> v3: add kernel-doc (Maxime Ripard)
>>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>
> please avoid these empty lines here.... cc, rv-b, sign-offs, links,
> etc are all in the same block.
ok.
>
>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_drv.c | 13 +++++++++++++
>>  include/drm/drm_drv.h     |  2 ++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 243cacb3575c..9d0409165f1e 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
>>  					devm_drm_dev_init_release, dev);
>>  }
>>  
>> +/**
>> + * devm_drm_dev_release_action - Call the final release action of the device
> Seeing the doc here gave me a second thought....
>
> the original release should be renamed to _devm_drm_dev_release
> and this should be called devm_drm_dev_release without the 'action' word.
i believe, was suggested earlier to directly expose the main function, is 
there any reason to have a __ version ?
>
>> + * @dev: DRM device
>> + *
>> + * 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.
> But also, this made me more confusing on why this is needed.
> Why can't we call put and get back?
IIUC, the ownership of drm_dev_get is with devm_drm_dev_alloc
and the release is tied to a devm action hence we needed this.

>
> The next needs to make it clear on why we need to release in these
> scenarios regarless of the number of counters. But do we really
> want this?
in our case post tFLR we need to reprobe the device, but the previousdrm instance
is not destroyed with just calling pci_remove as it is tied to PDEV lifetime
which is not destroyed hence we need to call the last release action ourself
so that the final release is called.
>
> Can we block the flr if we have users? Perhaps this is the reason
> that on my side the flr was not that clean? beucase I had display
> so I had clients connected?
the display side error is due to power wells, post FLR the power wells are already
disabled while we try to disable them from driver again it is throwing warnings.

Thanks,

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

Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR

[Nilawar, Badal]

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(&gt->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

Re: [PATCH v3 1/4] drm: add devm release action

[Rodrigo Vivi]

On Tue, Apr 23, 2024 at 02:25:06PM +0530, Aravind Iddamsetty wrote:
> 
> On 23/04/24 02:24, Rodrigo Vivi wrote:
> > On Mon, Apr 22, 2024 at 12:27:53PM +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)
> >>
> >> v3: add kernel-doc (Maxime Ripard)
> >>
> >> Cc: Maxime Ripard <mripard@kernel.org>
> >> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>
> > please avoid these empty lines here.... cc, rv-b, sign-offs, links,
> > etc are all in the same block.
> ok.
> >
> >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_drv.c | 13 +++++++++++++
> >>  include/drm/drm_drv.h     |  2 ++
> >>  2 files changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index 243cacb3575c..9d0409165f1e 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
> >>  					devm_drm_dev_init_release, dev);
> >>  }
> >>  
> >> +/**
> >> + * devm_drm_dev_release_action - Call the final release action of the device
> > Seeing the doc here gave me a second thought....
> >
> > the original release should be renamed to _devm_drm_dev_release
> > and this should be called devm_drm_dev_release without the 'action' word.
> i believe, was suggested earlier to directly expose the main function, is 
> there any reason to have a __ version ?

No no, just ignore me. Just remove the '_action' and don't change the other.

I don't like exposing the a function with '__'. what would '__' that mean?
This is what I meant on the first comment.

Now, I believe that we don't need the '_action'. What does the 'action' mean?

the devm_drm_dev_release should be enough. But then I got confused and
I thought it would conflict with the original released function name.
But I misread it.

This also made me ask myself if we really should use 'devm' prefix there
as well.

> >
> >> + * @dev: DRM device
> >> + *
> >> + * 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.
> > But also, this made me more confusing on why this is needed.
> > Why can't we call put and get back?
> IIUC, the ownership of drm_dev_get is with devm_drm_dev_alloc
> and the release is tied to a devm action hence we needed this.

you are right, but it is just a refcount. you are putting that one
back on behalf of the init path, to force the refcount to 0, and
then grabbing it back on init behalf after the flr. So in the
end it is the same.

Then with this flow we also can check if we are really force the
disconnection of eveybody before we are ready to put the last
ref that would call the release fn.

but well, I'm just brainstorming some thoughts on possibilities
of a clear release without forcing that...  it would be good
to run some experiments to see if that is possible. if not,
then let's go with this forced one.

> 
> >
> > The next needs to make it clear on why we need to release in these
> > scenarios regarless of the number of counters. But do we really
> > want this?
> in our case post tFLR we need to reprobe the device, but the previousdrm instance
> is not destroyed with just calling pci_remove as it is tied to PDEV lifetime
> which is not destroyed hence we need to call the last release action ourself
> so that the final release is called.
> >
> > Can we block the flr if we have users? Perhaps this is the reason
> > that on my side the flr was not that clean? beucase I had display
> > so I had clients connected?
> the display side error is due to power wells, post FLR the power wells are already
> disabled while we try to disable them from driver again it is throwing warnings.

so we probably need to tell display that we are going to be disabled.
probably the same patch that we need for d3cold:

https://lore.kernel.org/all/20240227183725.505699-3-rodrigo.vivi@intel.com/

> 
> Thanks,
> 
> Aravind.
> >
> >> + */
> >> +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
> >>

Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR

[Michał Winiarski]

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(&gt->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
> 

Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR

[Aravind Iddamsetty]

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(&gt->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

Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR

[Aravind Iddamsetty]

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(&gt->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
>>

Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR

[Nilawar, Badal]

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(&gt->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

Re: [PATCH v3 1/4] drm: add devm release action

[Aravind Iddamsetty]

On 23/04/24 23:12, Rodrigo Vivi wrote:
> On Tue, Apr 23, 2024 at 02:25:06PM +0530, Aravind Iddamsetty wrote:
>> On 23/04/24 02:24, Rodrigo Vivi wrote:
>>> On Mon, Apr 22, 2024 at 12:27:53PM +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)
>>>>
>>>> v3: add kernel-doc (Maxime Ripard)
>>>>
>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>
>>> please avoid these empty lines here.... cc, rv-b, sign-offs, links,
>>> etc are all in the same block.
>> ok.
>>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_drv.c | 13 +++++++++++++
>>>>  include/drm/drm_drv.h     |  2 ++
>>>>  2 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>> index 243cacb3575c..9d0409165f1e 100644
>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>> @@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
>>>>  					devm_drm_dev_init_release, dev);
>>>>  }
>>>>  
>>>> +/**
>>>> + * devm_drm_dev_release_action - Call the final release action of the device
>>> Seeing the doc here gave me a second thought....
>>>
>>> the original release should be renamed to _devm_drm_dev_release
>>> and this should be called devm_drm_dev_release without the 'action' word.
>> i believe, was suggested earlier to directly expose the main function, is 
>> there any reason to have a __ version ?
> No no, just ignore me. Just remove the '_action' and don't change the other.
>
> I don't like exposing the a function with '__'. what would '__' that mean?
> This is what I meant on the first comment.
>
> Now, I believe that we don't need the '_action'. What does the 'action' mean?
action is taken from here devm_release_action, but unlike here there isn't
any direct action call
>
> the devm_drm_dev_release should be enough. But then I got confused and
> I thought it would conflict with the original released function name.
> But I misread it.
>
> This also made me ask myself if we really should use 'devm' prefix there
> as well.
similar to devm in devm_drm_dev_alloc as it releases the action registered by it.
>
>>>> + * @dev: DRM device
>>>> + *
>>>> + * 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.
>>> But also, this made me more confusing on why this is needed.
>>> Why can't we call put and get back?
>> IIUC, the ownership of drm_dev_get is with devm_drm_dev_alloc
>> and the release is tied to a devm action hence we needed this.
> you are right, but it is just a refcount. you are putting that one
> back on behalf of the init path, to force the refcount to 0, and
> then grabbing it back on init behalf after the flr. So in the
> end it is the same.
>
> Then with this flow we also can check if we are really force the
> disconnection of eveybody before we are ready to put the last
> ref that would call the release fn.
>
> but well, I'm just brainstorming some thoughts on possibilities
> of a clear release without forcing that...  it would be good
> to run some experiments to see if that is possible. if not,
> then let's go with this forced one.
even if we close all clients we ought to call this as the ref taken
during alloc is released only when pdev is destroyed.But on the 
contrary can we expect the onus to be on admin to close all clients
before initiating a reset as this a manual trigger not an automatic one.
>
>>> The next needs to make it clear on why we need to release in these
>>> scenarios regarless of the number of counters. But do we really
>>> want this?
>> in our case post tFLR we need to reprobe the device, but the previousdrm instance
>> is not destroyed with just calling pci_remove as it is tied to PDEV lifetime
>> which is not destroyed hence we need to call the last release action ourself
>> so that the final release is called.
>>> Can we block the flr if we have users? Perhaps this is the reason
>>> that on my side the flr was not that clean? beucase I had display
>>> so I had clients connected?
>> the display side error is due to power wells, post FLR the power wells are already
>> disabled while we try to disable them from driver again it is throwing warnings.
> so we probably need to tell display that we are going to be disabled.
> probably the same patch that we need for d3cold:
>
> https://lore.kernel.org/all/20240227183725.505699-3-rodrigo.vivi@intel.com/

will try this one and get back.

Thanks,
Aravind.
>
>> Thanks,
>>
>> Aravind.
>>>> + */
>>>> +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
>>>>

Re: [PATCH v3 1/4] drm: add devm release action

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2747 bytes --]

On Tue, Apr 23, 2024 at 01:42:22PM -0400, Rodrigo Vivi wrote:
> On Tue, Apr 23, 2024 at 02:25:06PM +0530, Aravind Iddamsetty wrote:
> > 
> > On 23/04/24 02:24, Rodrigo Vivi wrote:
> > > On Mon, Apr 22, 2024 at 12:27:53PM +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)
> > >>
> > >> v3: add kernel-doc (Maxime Ripard)
> > >>
> > >> Cc: Maxime Ripard <mripard@kernel.org>
> > >> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
> > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > >>
> > > please avoid these empty lines here.... cc, rv-b, sign-offs, links,
> > > etc are all in the same block.
> > ok.
> > >
> > >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > >> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> > >> ---
> > >>  drivers/gpu/drm/drm_drv.c | 13 +++++++++++++
> > >>  include/drm/drm_drv.h     |  2 ++
> > >>  2 files changed, 15 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > >> index 243cacb3575c..9d0409165f1e 100644
> > >> --- a/drivers/gpu/drm/drm_drv.c
> > >> +++ b/drivers/gpu/drm/drm_drv.c
> > >> @@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
> > >>  					devm_drm_dev_init_release, dev);
> > >>  }
> > >>  
> > >> +/**
> > >> + * devm_drm_dev_release_action - Call the final release action of the device
> > > Seeing the doc here gave me a second thought....
> > >
> > > the original release should be renamed to _devm_drm_dev_release
> > > and this should be called devm_drm_dev_release without the 'action' word.
> > i believe, was suggested earlier to directly expose the main function, is 
> > there any reason to have a __ version ?
> 
> No no, just ignore me. Just remove the '_action' and don't change the other.
> 
> I don't like exposing the a function with '__'. what would '__' that mean?
> This is what I meant on the first comment.
> 
> Now, I believe that we don't need the '_action'. What does the 'action' mean?
> 
> the devm_drm_dev_release should be enough. But then I got confused and
> I thought it would conflict with the original released function name.
> But I misread it.

I don't think devm_drm_dev_release is a good name either. Just like any
other devm_* function that cancels what a previous one has been doing
(devm_kfree, devm_backlight_device_unregister, devm_nvmem_device_put,
etc.) it should be called devm_drm_dev_put or something similar.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/4] drm: add devm release action

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]

On Mon, Apr 22, 2024 at 12:27:53PM +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)
> 
> v3: add kernel-doc (Maxime Ripard)
> 
> Cc: Maxime Ripard <mripard@kernel.org>
> 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 | 13 +++++++++++++
>  include/drm/drm_drv.h     |  2 ++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 243cacb3575c..9d0409165f1e 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
>  					devm_drm_dev_init_release, dev);
>  }
>  
> +/**
> + * devm_drm_dev_release_action - Call the final release action of the device
> + * @dev: DRM device
> + *
> + * 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.
> + */

I'm not entirely sure what you mean by that documentation. "In scenarios
where drm_dev_put is directly called by the driver", we wouldn't need to
consider that function at all, right?

Also, we should reference it in drm_dev_put and devm_drm_dev_alloc too.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/4] drm: add devm release action

[Rodrigo Vivi]

On Wed, Apr 24, 2024 at 01:49:16PM +0200, Maxime Ripard wrote:
> On Tue, Apr 23, 2024 at 01:42:22PM -0400, Rodrigo Vivi wrote:
> > On Tue, Apr 23, 2024 at 02:25:06PM +0530, Aravind Iddamsetty wrote:
> > > 
> > > On 23/04/24 02:24, Rodrigo Vivi wrote:
> > > > On Mon, Apr 22, 2024 at 12:27:53PM +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)
> > > >>
> > > >> v3: add kernel-doc (Maxime Ripard)
> > > >>
> > > >> Cc: Maxime Ripard <mripard@kernel.org>
> > > >> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
> > > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > >>
> > > > please avoid these empty lines here.... cc, rv-b, sign-offs, links,
> > > > etc are all in the same block.
> > > ok.
> > > >
> > > >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > >> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> > > >> ---
> > > >>  drivers/gpu/drm/drm_drv.c | 13 +++++++++++++
> > > >>  include/drm/drm_drv.h     |  2 ++
> > > >>  2 files changed, 15 insertions(+)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > >> index 243cacb3575c..9d0409165f1e 100644
> > > >> --- a/drivers/gpu/drm/drm_drv.c
> > > >> +++ b/drivers/gpu/drm/drm_drv.c
> > > >> @@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
> > > >>  					devm_drm_dev_init_release, dev);
> > > >>  }
> > > >>  
> > > >> +/**
> > > >> + * devm_drm_dev_release_action - Call the final release action of the device
> > > > Seeing the doc here gave me a second thought....
> > > >
> > > > the original release should be renamed to _devm_drm_dev_release
> > > > and this should be called devm_drm_dev_release without the 'action' word.
> > > i believe, was suggested earlier to directly expose the main function, is 
> > > there any reason to have a __ version ?
> > 
> > No no, just ignore me. Just remove the '_action' and don't change the other.
> > 
> > I don't like exposing the a function with '__'. what would '__' that mean?
> > This is what I meant on the first comment.
> > 
> > Now, I believe that we don't need the '_action'. What does the 'action' mean?
> > 
> > the devm_drm_dev_release should be enough. But then I got confused and
> > I thought it would conflict with the original released function name.
> > But I misread it.
> 
> I don't think devm_drm_dev_release is a good name either. Just like any
> other devm_* function that cancels what a previous one has been doing
> (devm_kfree, devm_backlight_device_unregister, devm_nvmem_device_put,
> etc.) it should be called devm_drm_dev_put or something similar.

I see what you mean, but I don't believe the 'put' is the best option,
for 2 reasons:
- in general, we have put paired with gets and this has not get equivalent
- this bypass the regular get/put mechanism and forces the releases that
  would be done only after all drm_dev_put() taking ref to zero.

> 
> Maxime

Re: [PATCH v3 1/4] drm: add devm release action

[Aravind Iddamsetty]

On 24/04/24 17:21, Maxime Ripard wrote:
> On Mon, Apr 22, 2024 at 12:27:53PM +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)
>>
>> v3: add kernel-doc (Maxime Ripard)
>>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> 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 | 13 +++++++++++++
>>  include/drm/drm_drv.h     |  2 ++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>> index 243cacb3575c..9d0409165f1e 100644
>> --- a/drivers/gpu/drm/drm_drv.c
>> +++ b/drivers/gpu/drm/drm_drv.c
>> @@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
>>  					devm_drm_dev_init_release, dev);
>>  }
>>  
>> +/**
>> + * devm_drm_dev_release_action - Call the final release action of the device
>> + * @dev: DRM device
>> + *
>> + * 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.
>> + */
> I'm not entirely sure what you mean by that documentation. "In scenarios
> where drm_dev_put is directly called by the driver", we wouldn't need to
> consider that function at all, right?

the drm_dev_put is not being invoked by drivers directly but that is
associated with devres releases and the scenario here I meant if
drivers want to have that called by themselves.
>
> Also, we should reference it in drm_dev_put and devm_drm_dev_alloc too.

sorry I didn't get this can you please elaborate.

Thanks,
Aravind.
>
> Maxime

Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR

[Michał Winiarski]

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(&gt->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
> >>

Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR

[Aravind Iddamsetty]

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(&gt->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

Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR

[Aravind Iddamsetty]

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(&gt->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
>>>>

Re: [PATCH v3 1/4] drm: add devm release action

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3734 bytes --]

On Wed, Apr 24, 2024 at 08:20:32AM -0400, Rodrigo Vivi wrote:
> On Wed, Apr 24, 2024 at 01:49:16PM +0200, Maxime Ripard wrote:
> > On Tue, Apr 23, 2024 at 01:42:22PM -0400, Rodrigo Vivi wrote:
> > > On Tue, Apr 23, 2024 at 02:25:06PM +0530, Aravind Iddamsetty wrote:
> > > > 
> > > > On 23/04/24 02:24, Rodrigo Vivi wrote:
> > > > > On Mon, Apr 22, 2024 at 12:27:53PM +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)
> > > > >>
> > > > >> v3: add kernel-doc (Maxime Ripard)
> > > > >>
> > > > >> Cc: Maxime Ripard <mripard@kernel.org>
> > > > >> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
> > > > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > >>
> > > > > please avoid these empty lines here.... cc, rv-b, sign-offs, links,
> > > > > etc are all in the same block.
> > > > ok.
> > > > >
> > > > >> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > >> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
> > > > >> ---
> > > > >>  drivers/gpu/drm/drm_drv.c | 13 +++++++++++++
> > > > >>  include/drm/drm_drv.h     |  2 ++
> > > > >>  2 files changed, 15 insertions(+)
> > > > >>
> > > > >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > > >> index 243cacb3575c..9d0409165f1e 100644
> > > > >> --- a/drivers/gpu/drm/drm_drv.c
> > > > >> +++ b/drivers/gpu/drm/drm_drv.c
> > > > >> @@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
> > > > >>  					devm_drm_dev_init_release, dev);
> > > > >>  }
> > > > >>  
> > > > >> +/**
> > > > >> + * devm_drm_dev_release_action - Call the final release action of the device
> > > > > Seeing the doc here gave me a second thought....
> > > > >
> > > > > the original release should be renamed to _devm_drm_dev_release
> > > > > and this should be called devm_drm_dev_release without the 'action' word.
> > > > i believe, was suggested earlier to directly expose the main function, is 
> > > > there any reason to have a __ version ?
> > > 
> > > No no, just ignore me. Just remove the '_action' and don't change the other.
> > > 
> > > I don't like exposing the a function with '__'. what would '__' that mean?
> > > This is what I meant on the first comment.
> > > 
> > > Now, I believe that we don't need the '_action'. What does the 'action' mean?
> > > 
> > > the devm_drm_dev_release should be enough. But then I got confused and
> > > I thought it would conflict with the original released function name.
> > > But I misread it.
> > 
> > I don't think devm_drm_dev_release is a good name either. Just like any
> > other devm_* function that cancels what a previous one has been doing
> > (devm_kfree, devm_backlight_device_unregister, devm_nvmem_device_put,
> > etc.) it should be called devm_drm_dev_put or something similar.
> 
> I see what you mean, but I don't believe the 'put' is the best option,
> for 2 reasons:
> - in general, we have put paired with gets and this has not get equivalent

Yeah, that's true. _release is fine then I guess.

> - this bypass the regular get/put mechanism and forces the releases that
>   would be done only after all drm_dev_put() taking ref to zero.

I don't think it does? devm_release_action will only remove the devm
action and execute it directly, but this action here is a call to
drm_dev_put, so we might still have other references taken that would
defer the device being freed.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v3 1/4] drm: add devm release action

[Aravind Iddamsetty]

On 25/04/24 18:22, Maxime Ripard wrote:
> On Wed, Apr 24, 2024 at 08:20:32AM -0400, Rodrigo Vivi wrote:
>> On Wed, Apr 24, 2024 at 01:49:16PM +0200, Maxime Ripard wrote:
>>> On Tue, Apr 23, 2024 at 01:42:22PM -0400, Rodrigo Vivi wrote:
>>>> On Tue, Apr 23, 2024 at 02:25:06PM +0530, Aravind Iddamsetty wrote:
>>>>> On 23/04/24 02:24, Rodrigo Vivi wrote:
>>>>>> On Mon, Apr 22, 2024 at 12:27:53PM +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)
>>>>>>>
>>>>>>> v3: add kernel-doc (Maxime Ripard)
>>>>>>>
>>>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>>>> Cc: Thomas Hellstr_m <thomas.hellstrom@linux.intel.com>
>>>>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>>>>
>>>>>> please avoid these empty lines here.... cc, rv-b, sign-offs, links,
>>>>>> etc are all in the same block.
>>>>> ok.
>>>>>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>>>>> Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@linux.intel.com>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/drm_drv.c | 13 +++++++++++++
>>>>>>>  include/drm/drm_drv.h     |  2 ++
>>>>>>>  2 files changed, 15 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
>>>>>>> index 243cacb3575c..9d0409165f1e 100644
>>>>>>> --- a/drivers/gpu/drm/drm_drv.c
>>>>>>> +++ b/drivers/gpu/drm/drm_drv.c
>>>>>>> @@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
>>>>>>>  					devm_drm_dev_init_release, dev);
>>>>>>>  }
>>>>>>>  
>>>>>>> +/**
>>>>>>> + * devm_drm_dev_release_action - Call the final release action of the device
>>>>>> Seeing the doc here gave me a second thought....
>>>>>>
>>>>>> the original release should be renamed to _devm_drm_dev_release
>>>>>> and this should be called devm_drm_dev_release without the 'action' word.
>>>>> i believe, was suggested earlier to directly expose the main function, is 
>>>>> there any reason to have a __ version ?
>>>> No no, just ignore me. Just remove the '_action' and don't change the other.
>>>>
>>>> I don't like exposing the a function with '__'. what would '__' that mean?
>>>> This is what I meant on the first comment.
>>>>
>>>> Now, I believe that we don't need the '_action'. What does the 'action' mean?
>>>>
>>>> the devm_drm_dev_release should be enough. But then I got confused and
>>>> I thought it would conflict with the original released function name.
>>>> But I misread it.
>>> I don't think devm_drm_dev_release is a good name either. Just like any
>>> other devm_* function that cancels what a previous one has been doing
>>> (devm_kfree, devm_backlight_device_unregister, devm_nvmem_device_put,
>>> etc.) it should be called devm_drm_dev_put or something similar.
>> I see what you mean, but I don't believe the 'put' is the best option,
>> for 2 reasons:
>> - in general, we have put paired with gets and this has not get equivalent
> Yeah, that's true. _release is fine then I guess.
>
>> - this bypass the regular get/put mechanism and forces the releases that
>>   would be done only after all drm_dev_put() taking ref to zero.
> I don't think it does? devm_release_action will only remove the devm
> action and execute it directly, but this action here is a call to
> drm_dev_put, so we might still have other references taken that would
> defer the device being freed.
yes i.e right, i assumed drm_dev_unplug would close all client handles but no.
So i was thinking if it is ok to iterate over  no of clients and call drm_dev_put in either
drm_dev_unplug or as part of this devm_release.


Thanks,
Aravind.
>
> Maxime

Re: [PATCH v3 4/4] drm/xe/FLR: Support PCIe FLR

[Michał Winiarski]

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(&gt->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
> >>>>

Re: [PATCH v3 1/4] drm: add devm release action

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2599 bytes --]

On Wed, Apr 24, 2024 at 06:06:52PM +0530, Aravind Iddamsetty wrote:
> 
> On 24/04/24 17:21, Maxime Ripard wrote:
> > On Mon, Apr 22, 2024 at 12:27:53PM +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)
> >>
> >> v3: add kernel-doc (Maxime Ripard)
> >>
> >> Cc: Maxime Ripard <mripard@kernel.org>
> >> 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 | 13 +++++++++++++
> >>  include/drm/drm_drv.h     |  2 ++
> >>  2 files changed, 15 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index 243cacb3575c..9d0409165f1e 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -714,6 +714,19 @@ static int devm_drm_dev_init(struct device *parent,
> >>  					devm_drm_dev_init_release, dev);
> >>  }
> >>  
> >> +/**
> >> + * devm_drm_dev_release_action - Call the final release action of the device
> >> + * @dev: DRM device
> >> + *
> >> + * 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.
> >> + */
> > I'm not entirely sure what you mean by that documentation. "In scenarios
> > where drm_dev_put is directly called by the driver", we wouldn't need to
> > consider that function at all, right?
> 
> the drm_dev_put is not being invoked by drivers directly but that is
> associated with devres releases and the scenario here I meant if
> drivers want to have that called by themselves.

Then that needs to be rephrased to mention both that it applies only to
drivers using devm_drm_dev_alloc, and if they want to drop their
reference earlier than anticipated.

> > Also, we should reference it in drm_dev_put and devm_drm_dev_alloc too.
> 
> sorry I didn't get this can you please elaborate.

devm_drm_dev_alloc needs to point at this new function to mention that
we can drop the reference explicitly. And drm_dev_put needs to mention
that it only applies to non-devm drivers, and if we want to drop the
reference on a devm driver, we need to call this new function.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]