[Intel-gfx] [PATCH v10 2/7] drm/i915/guc: Add CT size delay helper

[John Harrison]

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<john.c.harrison at intel.com>
> Signed-off-by: Jonathan Cavitt<jonathan.cavitt at intel.com>
> ---
>   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)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20231010/958cc119/attachment.htm>