On Wed, Mar 20, 2024 at 04:14:26PM +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.
Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty@xxxxxxxxxxxxxxx>
---
drivers/gpu/drm/xe/Makefile | 1 +
drivers/gpu/drm/xe/xe_device_types.h | 3 +
drivers/gpu/drm/xe/xe_gt.c | 31 ++++++---
drivers/gpu/drm/xe/xe_gt.h | 1 +
drivers/gpu/drm/xe/xe_pci.c | 53 ++++++++++++++--
drivers/gpu/drm/xe/xe_pci.h | 6 +-
drivers/gpu/drm/xe/xe_pci_err.c | 94 ++++++++++++++++++++++++++++
7 files changed, 174 insertions(+), 15 deletions(-)
create mode 100644 drivers/gpu/drm/xe/xe_pci_err.c
diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index 3c3e67885559..1447712fec65 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -114,6 +114,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 9785eef2e5a4..e9b8c7cbb428 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -455,6 +455,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_gt.c b/drivers/gpu/drm/xe/xe_gt.c
index 85408e7a932b..437874a9a5a0 100644
--- a/drivers/gpu/drm/xe/xe_gt.c
+++ b/drivers/gpu/drm/xe/xe_gt.c
@@ -629,6 +629,26 @@ static int do_gt_restart(struct xe_gt *gt)
return 0;
}
+/* Idle the GT */
+int xe_idle_gt(struct xe_gt *gt)
any non-static function should use xe_gt_ prefix.
+{
+ int err;
+
+ xe_gt_sanitize(gt);
+
+ xe_uc_gucrc_disable(>->uc);
+ xe_uc_stop_prepare(>->uc);
+ xe_gt_pagefault_reset(gt);
+
+ err = xe_uc_stop(>->uc);
+ if (err)
+ return err;
+
+ xe_gt_tlb_invalidation_reset(gt);
+
+ return err;
+}
+
static int gt_reset(struct xe_gt *gt)
{
int err;
@@ -645,21 +665,12 @@ static int gt_reset(struct xe_gt *gt)
}
xe_pm_runtime_get(gt_to_xe(gt));
- xe_gt_sanitize(gt);
err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
if (err)
goto err_msg;
- xe_uc_gucrc_disable(>->uc);
- xe_uc_stop_prepare(>->uc);
- xe_gt_pagefault_reset(gt);
-
- err = xe_uc_stop(>->uc);
- if (err)
- goto err_out;
-
- xe_gt_tlb_invalidation_reset(gt);
+ xe_idle_gt(gt);
this and the above should be in a commit alone
"drm/xe: Extract xe_gt_idle() helper" with explanation it will be used
in other places outside of gt_reset path.
but I'm a little bit confused here... why do you need to remove
xe_gt_sanitize() inside the function to make gt idle?
Lucas De Marchi
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..77df919199cc 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);
+int xe_idle_gt(struct xe_gt *gt);
/**
* xe_gt_any_hw_engine_by_reset_domain - scan the list of engines and return the
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index c401d4890386..fcd2a7f66f7b 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -383,6 +383,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) {
+ pci_restore_state(pdev);
+ } else {
+ drm_warn(&xe->drm, "Failed to load PCI state err:%d\n", ret);
+ }
please check coding style here and invert condition so you keep the
return-early pattern you started in the xe->pci_state check.
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;
}
+}
+
/* is device_id present in comma separated list of ids */
static bool device_id_in_list(u16 device_id, const char *devices, bool negative)
{
@@ -688,10 +723,12 @@ 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);
}
-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;
@@ -786,6 +823,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);
why do we save and restore in sequence when probing? If we only have to
have a saved stated for possible errors later, why do we restore on
success? /me confused
+
return 0;
}
@@ -833,7 +873,7 @@ static int xe_pci_suspend(struct device *dev)
*/
d3cold_toggle(pdev, D3COLD_ENABLE);
- pci_save_state(pdev);
+ xe_save_pci_state(pdev);
all these no change in behavior with xe_ functions wrapping the the pci
ones could be delegated to a prep patch so it's easier to separate the
noise from the actual changes.
pci_disable_device(pdev);
return 0;
@@ -857,6 +897,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;
@@ -874,7 +916,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);
@@ -899,7 +941,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);
@@ -928,6 +970,8 @@ static const struct dev_pm_ops xe_pm_ops = {
};
#endif
+extern const struct pci_error_handlers xe_pci_err_handlers;
not sure this really belongs into a separate compilation unit. Just
leave it in xe_pci.c? Adding the extern here doesn't make it pretty.
if we are adding a separate .c, then the struct definition should
probably be here, and you do it like
static const struct pci_error_handlers xe_pci_err_handlers = {
.reset_prepare = xe_pci_err_reset_prepare,
.reset_done = xe_pci_err_reset_done,
};
See xe_pm_ops, that was left here rather than in xe_pm.c.
Lucas De Marchi
+
static struct pci_driver xe_pci_driver = {
.name = DRIVER_NAME,
.id_table = pciidlist,
@@ -937,6 +981,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 611c1209b14c..9faf5380a09e 100644
--- a/drivers/gpu/drm/xe/xe_pci.h
+++ b/drivers/gpu/drm/xe/xe_pci.h
@@ -6,7 +6,11 @@
#ifndef _XE_PCI_H_
#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..ecc467a9c3af
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_pci_err.c
@@ -0,0 +1,94 @@
+// 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_pm.h"
+
+/**
+ * xe_pci_reset_prepare - Called when user issued a function level reset
+ * via /sys/bus/pci/devices/.../reset.
+ * @pdev: PCI device struct
+ */
+static 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 FLR 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;
+ err = xe_idle_gt(gt);
+ if (err) {
+ xe_gt_err(gt, "failed to idle gt (%pe)\n", ERR_PTR(err));
+ goto reset;
+ }
+
+ 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 function level reset is done.
+ * @pdev: PCI device struct
+ */
+static 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,
+ "PCI device went through FLR, 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);
+
+ /*
+ * 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_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);
+}
+
+const struct pci_error_handlers xe_pci_err_handlers = {
+ .reset_prepare = xe_pci_reset_prepare,
+ .reset_done = xe_pci_reset_done,
+};
--
2.25.1
