On 10/10/2023 08:02, Jonathan Cavitt wrote:
Add a helper function to the GuC CT buffer that reports the expected
time to process all outstanding requests. As of now, there is no
functionality to check number of requests in the buffer, so the helper
function just reports 2 seconds, or 1ms per request up to the maximum
number of requests the CT buffer can store.
Suggested-by: John Harrison<[email protected]>
Signed-off-by: Jonathan Cavitt<[email protected]>
---
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
index 58e42901ff498..36afc1ce9fabd 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
@@ -120,6 +120,19 @@ static inline bool intel_guc_ct_enabled(struct
intel_guc_ct *ct)
return ct->enabled;
}
+/*
+ * GuC has a timeout of 1ms for a TLB invalidation response from GAM. On a
+ * timeout GuC drops the request and has no mechanism to notify the host about
+ * the timeout. There is also no mechanism for determining the number of
+ * outstanding requests in the CT buffer. Ergo, keep a larger timeout that
accounts
+ * for this individual timeout and the max number of outstanding requests that
+ * can be queued in CT buffer.
+ */
This feels like the wrong wording. TLB invalidations aren't even close
to the slowest thing that goes through the CT buffer. And the
description about dropping failed requests and such is irrelevant to the
implementation/purpose of this helper. That is specific detail about one
single use case of the helper. That might be the only user at this point
but the intention is that other parts of the driver will be updated to
call this as well rather than hard coding their own timeouts as they
currently do.
I would suggest:
Some H2G commands involve a synchronous response that the driver
needs to wait for. In such cases, a timeout is required to prevent
the driver from waiting forever in the case of an error (either no
error response is defined in the protocol or something has died and
requires a reset). The specific command may be defined as having a
time bound response but the CT is a queue and that time guarantee
only starts from the point when the command reaches the head of the
queue and is processed by GuC.
Ideally there would be a helper to report the progress of a given
command through the CT. However, that would require a significant
amount of work in the CT layer. In the meantime, provide a
reasonable estimation of the worst case latency it should take for
the entire queue to drain. And therefore, how long a caller should
wait before giving up on their request. The current estimate is
based on empirical measurement of a test that fills the buffer with
context creation and destruction requests as they seem to be the
slowest operation.
+static inline long intel_guc_ct_expected_delay(struct intel_guc_ct *ct)
This is not the 'expected' delay but the worst case maximum delay. Also,
no need to force the callers to know about ct structures. They
presumably have a intel_guc structure if they are sending H2G messages,
and that is all you should need to know about. Having said that, the
implementation isn't currently accessing any stored data, so why bother
with a parameter at all?
+{
+ return HZ * 2;
Also, this needs to be based on the buffer size so that if the size were
to increase then the time would as well.
My thought would be:
long intel_guc_ct_max_queue_time_jiffies(void) {
/*
* A 4KB buffer full of context destroy commands takes a little
over a second to process
* so bump that to 2s to be super safe.
*/
return (CTB_H2G_BUFFER_SIZE * HZ) / SZ_2K;
}
John.
+}
+
#define INTEL_GUC_CT_SEND_NB BIT(31)
#define INTEL_GUC_CT_SEND_G2H_DW_SHIFT 0
#define INTEL_GUC_CT_SEND_G2H_DW_MASK (0xff << INTEL_GUC_CT_SEND_G2H_DW_SHIFT)
