[RFC 2/2] drm/xe/FLR: Support PCIe FLR

Lucas De Marchi lucas.demarchi at intel.com
Wed Mar 20 23:01:22 UTC 2024


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 at intel.com>
>Cc: Lucas De Marchi <lucas.demarchi at intel.com>
>Signed-off-by: Aravind Iddamsetty <aravind.iddamsetty at linux.intel.com>
>---
> 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(&gt->uc);
>+	xe_uc_stop_prepare(&gt->uc);
>+	xe_gt_pagefault_reset(gt);
>+
>+	err = xe_uc_stop(&gt->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(&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_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
>


More information about the Intel-xe mailing list