<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> On 10/10/2023 08:02, Jonathan Cavitt wrote: <blockquote type="cite" cite="mid:20231010150244.2021420-3-jonathan.cavitt@intel.com"> <pre class="moz-quote-pre" wrap="">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 <a class="moz-txt-link-rfc2396E" href="mailto:john.c.harrison@intel.com"><john.c.harrison@intel.com></a> Signed-off-by: Jonathan Cavitt <a class="moz-txt-link-rfc2396E" href="mailto:jonathan.cavitt@intel.com"><jonathan.cavitt@intel.com></a> --- 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. + */</pre> </blockquote> 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: <blockquote>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. </blockquote> <blockquote type="cite" cite="mid:20231010150244.2021420-3-jonathan.cavitt@intel.com"> <pre class="moz-quote-pre" wrap=""> +static inline long intel_guc_ct_expected_delay(struct intel_guc_ct *ct)</pre> </blockquote> 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? <blockquote type="cite" cite="mid:20231010150244.2021420-3-jonathan.cavitt@intel.com"> <pre class="moz-quote-pre" wrap=""> +{ + return HZ * 2;</pre> </blockquote> 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: <blockquote>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; } </blockquote> John. <blockquote type="cite" cite="mid:20231010150244.2021420-3-jonathan.cavitt@intel.com"> <pre class="moz-quote-pre" wrap=""> +} + #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) </pre> </blockquote> </body> </html>