On 10/16/2018 3:26 AM, Michal Wajdeczko wrote:
On Tue, 16 Oct 2018 00:10:08 +0200, Daniele Ceraolo Spurio
<daniele.ceraolospurio@xxxxxxxxx> wrote:
The ENTER/EXIT_S_STATE actions queue the save/restore operation in GuC
FW and then return, so waiting on the H2H is not enough to guarantee
GuC is done.
When all the processing is done, GuC writes 0 to scratch register 14,
so we can poll on that. Note that GuC does not ensure that the value
in the register is different from 0 while the action is in progress
so we need to take care of that ourselves as well.
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_guc.c | 28 +++++++++++++++++++++++++--
drivers/gpu/drm/i915/intel_guc_fwif.h | 6 ++++++
2 files changed, 32 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc.c
b/drivers/gpu/drm/i915/intel_guc.c
index 230aea69385d..f238cd7a9dcf 100644
--- a/drivers/gpu/drm/i915/intel_guc.c
+++ b/drivers/gpu/drm/i915/intel_guc.c
@@ -521,6 +521,30 @@ int intel_guc_auth_huc(struct intel_guc *guc,
u32 rsa_offset)
return intel_guc_send(guc, action, ARRAY_SIZE(action));
}
+/*
+ * The ENTER/EXIT_S_STATE actions queue the save/restore operation
in GuC FW and
+ * then return, so waiting on the H2H is not enough to guarantee GuC
is done.
+ * When all the processing is done, GuC writes 0 to scratch register
14, so we
s/writes 0/writes INTEL_GUC_SLEEP_STATE_SUCCESS ?
+ * can poll on that. Note that GuC does not ensure that the value in
the
+ * register is different from 0 while the action is in progress so
we need to
+ * take care of that ourselves as well.
+ */
+static int guc_sleep_state_action(struct intel_guc *guc,
+ const u32 *action, u32 len)
+{
+ struct drm_i915_private *dev_priv = guc_to_i915(guc);
+ int ret;
+
+ I915_WRITE(SOFT_SCRATCH(14), ~0x0);
Do we want to add dedicated name for that scratch register?
As I replied on your patch, the register is used for other purposes in
other actions so I don't think it is a good idea to rename it.
Also, as GuC is using scratch[14] then we need to remove it from our send
register pool - patch is here [1]
+
+ ret = intel_guc_send(guc, action, len);
+ if (ret)
+ return ret;
+
+ return intel_wait_for_register(dev_priv, SOFT_SCRATCH(14), ~0x0,
+ INTEL_GUC_SLEEP_STATE_SUCCESS, 10);
Maybe it is worth to use __intel_wait_for_register() and print sleep
state error code in case of failure ? And do we really need to wait
full 10ms for SUCCESS if GuC already has reported PREEMPT_TO_IDLE_FAILED
or ENGINE_RESET_FAILED ?
u32 state;
ret = __intel_wait_for_register(dev_priv, SOFT_SCRATCH(14),
0x80000000,
0, 10, &state);
if (ret)
return ret;
if (status != INTEL_GUC_SLEEP_STATE_SUCCESS) {
DRM_ERROR("... %u\n", state);
return -EIO;
}
return 0;
It should be impossible for us to get PREEMPT_TO_IDLE_FAILED or
ENGINE_RESET_FAILED because those only come in play if guc_suspend() is
called while there are outstanding request inside GuC. However, no real
downsides in going with your solution either so I'll do the changes ;)
+}
+
/**
* intel_guc_suspend() - notify GuC entering suspend state
* @guc: the guc
@@ -533,7 +557,7 @@ int intel_guc_suspend(struct intel_guc *guc)
intel_guc_ggtt_offset(guc, guc->shared_data)
};
- return intel_guc_send(guc, data, ARRAY_SIZE(data));
+ return guc_sleep_state_action(guc, data, ARRAY_SIZE(data));
}
/**
@@ -571,7 +595,7 @@ int intel_guc_resume(struct intel_guc *guc)
intel_guc_ggtt_offset(guc, guc->shared_data)
};
- return intel_guc_send(guc, data, ARRAY_SIZE(data));
+ return guc_sleep_state_action(guc, data, ARRAY_SIZE(data));
}
/**
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h
b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 8382d591c784..b0eb5aabe0a7 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -687,6 +687,12 @@ enum intel_guc_report_status {
INTEL_GUC_REPORT_STATUS_COMPLETE = 0x4,
};
+enum intel_guc_sleep_state_status {
+ INTEL_GUC_SLEEP_STATE_SUCCESS = 0x0,
+ INTEL_GUC_SLEEP_STATE_PREEMPT_TO_IDLE_FAILED = 0x1,
+ INTEL_GUC_SLEEP_STATE_ENGINE_RESET_FAILED = 0x2
as we waiting for state change, maybe we should explicitly define
INTEL_GUC_SLEEP_STATE_INVALID_MASK = 0x80000000,
and use it in __intel_wait_for_register ?
ack
+};
+
#define GUC_LOG_CONTROL_LOGGING_ENABLED (1 << 0)
#define GUC_LOG_CONTROL_VERBOSITY_SHIFT 4
#define GUC_LOG_CONTROL_VERBOSITY_MASK (0xF <<
GUC_LOG_CONTROL_VERBOSITY_SHIFT)
Note for you while I'm it: I've been told that the gen11 GuC FW has a
simplified and improved flow for suspend/resume, so some changes will be
required in your series. Not sure about the details.
Thanks,
Daniele
Thanks,
Michal
[1] https://patchwork.freedesktop.org/patch/256921/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
