[PATCH 1/4] drm/xe/gsc: Do not attempt to load the GSC multiple times

[Lucas De Marchi]

On Tue, Aug 13, 2024 at 04:11:44PM GMT, Daniele Ceraolo Spurio wrote:
>The GSC HW is only reset by driver FLR or D3cold entry. We don't support
>the former at runtime, while the latter is only supported on DGFX, for
>which we don't support GSC. Therefore, if GSC failed to load previously
>there is no need to try again because the HW is stuck in the error state.
>
>An assert has been added so that if we ever add DGFX support we'll know
>we need to handle the D3 case.
>
>Fixes: dd0e89e5edc2 ("drm/xe/gsc: GSC FW load")
>Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>Cc: John Harrison <John.C.Harrison at Intel.com>
>Cc: Alan Previn <alan.previn.teres.alexis at intel.com>
>---
> drivers/gpu/drm/xe/xe_gsc.c   | 12 ++++++++++++
> drivers/gpu/drm/xe/xe_uc_fw.h |  9 +++++++--
> 2 files changed, 19 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_gsc.c b/drivers/gpu/drm/xe/xe_gsc.c
>index 77ce44e845c5..66963e26c574 100644
>--- a/drivers/gpu/drm/xe/xe_gsc.c
>+++ b/drivers/gpu/drm/xe/xe_gsc.c
>@@ -519,10 +519,22 @@ int xe_gsc_init_post_hwconfig(struct xe_gsc *gsc)
> void xe_gsc_load_start(struct xe_gsc *gsc)
> {
> 	struct xe_gt *gt = gsc_to_gt(gsc);
>+	struct xe_device *xe = gt_to_xe(gt);
>
> 	if (!xe_uc_fw_is_loadable(&gsc->fw) || !gsc->q)
> 		return;
>
>+	/*
>+	 * The GSC HW is only reset by driver FLR or D3cold entry. We don't
>+	 * support the former at runtime, while the latter is only supported on
>+	 * DGFX, for which we don't support GSC. Therefore, if GSC failed to
>+	 * load previously there is no need to try again because the HW is
>+	 * stuck in the error state.
>+	 */
>+	xe_assert(xe, !IS_DGFX(xe));
>+	if (xe_uc_fw_is_in_error_state(&gsc->fw))
>+		return;
>+
> 	/* GSC FW survives GT reset and D3Hot */
> 	if (gsc_fw_is_loaded(gt)) {
> 		xe_uc_fw_change_status(&gsc->fw, XE_UC_FIRMWARE_TRANSFERRED);
>diff --git a/drivers/gpu/drm/xe/xe_uc_fw.h b/drivers/gpu/drm/xe/xe_uc_fw.h
>index c108e9d08e70..e4709d9b380d 100644
>--- a/drivers/gpu/drm/xe/xe_uc_fw.h
>+++ b/drivers/gpu/drm/xe/xe_uc_fw.h
>@@ -65,7 +65,7 @@ const char *xe_uc_fw_status_repr(enum xe_uc_fw_status status)
> 	return "<invalid>";
> }
>
>-static inline int xe_uc_fw_status_to_error(enum xe_uc_fw_status status)
>+static inline int xe_uc_fw_status_to_error(const enum xe_uc_fw_status status)

this const addition here looks gratuitous. Even if uc_fw is const in the
caller, this doesn't need to be.

I'll leave the review to the Cc list

Lucas De Marchi


> {
> 	switch (status) {
> 	case XE_UC_FIRMWARE_NOT_SUPPORTED:
>@@ -108,7 +108,7 @@ static inline const char *xe_uc_fw_type_repr(enum xe_uc_fw_type type)
> }
>
> static inline enum xe_uc_fw_status
>-__xe_uc_fw_status(struct xe_uc_fw *uc_fw)
>+__xe_uc_fw_status(const struct xe_uc_fw *uc_fw)
> {
> 	/* shouldn't call this before checking hw/blob availability */
> 	XE_WARN_ON(uc_fw->status == XE_UC_FIRMWARE_UNINITIALIZED);
>@@ -156,6 +156,11 @@ static inline bool xe_uc_fw_is_overridden(const struct xe_uc_fw *uc_fw)
> 	return uc_fw->user_overridden;
> }
>
>+static inline bool xe_uc_fw_is_in_error_state(const struct xe_uc_fw *uc_fw)
>+{
>+	return xe_uc_fw_status_to_error(__xe_uc_fw_status(uc_fw)) != 0;
>+}
>+
> static inline void xe_uc_fw_sanitize(struct xe_uc_fw *uc_fw)
> {
> 	if (xe_uc_fw_is_loadable(uc_fw))
>-- 
>2.43.0
>