Re: [PATCH 5/7] drm/i915/guc: Add stall timer to non blocking CTB send function

[John Harrison <john.c.harrison@xxxxxxxxx>]

On 7/1/2021 10:15, Matthew Brost wrote:
> Implement a stall timer which fails H2G CTBs once a period of time
> with no forward progress is reached to prevent deadlock.
>
> v2:
>   (Michal)
>    - Improve error message in ct_deadlock()
>    - Set broken when ct_deadlock() returns true
>    - Return -EPIPE on ct_deadlock()
> v3:
>   (Michal)
>    - Add ms to stall timer comment
>   (Matthew)
>    - Move broken check to intel_guc_ct_send()
>
> Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> Signed-off-by: Matthew Brost <matthew.brost@xxxxxxxxx>
Looks plausible to me.

Reviewed-by: John Harrison <John.C.Harrison@xxxxxxxxx>

> ---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 62 ++++++++++++++++++++---
>   drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h |  4 ++
>   2 files changed, 59 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> index fb825cc1d090..a9cb7b608520 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -4,6 +4,9 @@
>    */
>   
>   #include <linux/circ_buf.h>
> +#include <linux/ktime.h>
> +#include <linux/time64.h>
> +#include <linux/timekeeping.h>
>   
>   #include "i915_drv.h"
>   #include "intel_guc_ct.h"
> @@ -316,6 +319,7 @@ int intel_guc_ct_enable(struct intel_guc_ct *ct)
>   		goto err_deregister;
>   
>   	ct->enabled = true;
> +	ct->stall_time = KTIME_MAX;
>   
>   	return 0;
>   
> @@ -388,9 +392,6 @@ static int ct_write(struct intel_guc_ct *ct,
>   	u32 *cmds = ctb->cmds;
>   	unsigned int i;
>   
> -	if (unlikely(ctb->broken))
> -		return -EPIPE;
> -
>   	if (unlikely(desc->status))
>   		goto corrupted;
>   
> @@ -506,6 +507,25 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
>   	return err;
>   }
>   
> +#define GUC_CTB_TIMEOUT_MS	1500
> +static inline bool ct_deadlocked(struct intel_guc_ct *ct)
> +{
> +	long timeout = GUC_CTB_TIMEOUT_MS;
> +	bool ret = ktime_ms_delta(ktime_get(), ct->stall_time) > timeout;
> +
> +	if (unlikely(ret)) {
> +		struct guc_ct_buffer_desc *send = ct->ctbs.send.desc;
> +		struct guc_ct_buffer_desc *recv = ct->ctbs.send.desc;
> +
> +		CT_ERROR(ct, "Communication stalled for %lld ms, desc status=%#x,%#x\n",
> +			 ktime_ms_delta(ktime_get(), ct->stall_time),
> +			 send->status, recv->status);
> +		ct->ctbs.send.broken = true;
> +	}
> +
> +	return ret;
> +}
> +
>   static inline bool h2g_has_room(struct intel_guc_ct_buffer *ctb, u32 len_dw)
>   {
>   	struct guc_ct_buffer_desc *desc = ctb->desc;
> @@ -517,6 +537,26 @@ static inline bool h2g_has_room(struct intel_guc_ct_buffer *ctb, u32 len_dw)
>   	return space >= len_dw;
>   }
>   
> +static int has_room_nb(struct intel_guc_ct *ct, u32 len_dw)
> +{
> +	struct intel_guc_ct_buffer *ctb = &ct->ctbs.send;
> +
> +	lockdep_assert_held(&ct->ctbs.send.lock);
> +
> +	if (unlikely(!h2g_has_room(ctb, len_dw))) {
> +		if (ct->stall_time == KTIME_MAX)
> +			ct->stall_time = ktime_get();
> +
> +		if (unlikely(ct_deadlocked(ct)))
> +			return -EPIPE;
> +		else
> +			return -EBUSY;
> +	}
> +
> +	ct->stall_time = KTIME_MAX;
> +	return 0;
> +}
> +
>   static int ct_send_nb(struct intel_guc_ct *ct,
>   		      const u32 *action,
>   		      u32 len,
> @@ -529,11 +569,9 @@ static int ct_send_nb(struct intel_guc_ct *ct,
>   
>   	spin_lock_irqsave(&ctb->lock, spin_flags);
>   
> -	ret = h2g_has_room(ctb, len + GUC_CTB_HDR_LEN);
> -	if (unlikely(!ret)) {
> -		ret = -EBUSY;
> +	ret = has_room_nb(ct, len + GUC_CTB_HDR_LEN);
> +	if (unlikely(ret))
>   		goto out;
> -	}
>   
>   	fence = ct_get_next_fence(ct);
>   	ret = ct_write(ct, action, len, fence, flags);
> @@ -576,8 +614,13 @@ static int ct_send(struct intel_guc_ct *ct,
>   retry:
>   	spin_lock_irqsave(&ctb->lock, flags);
>   	if (unlikely(!h2g_has_room(ctb, len + GUC_CTB_HDR_LEN))) {
> +		if (ct->stall_time == KTIME_MAX)
> +			ct->stall_time = ktime_get();
>   		spin_unlock_irqrestore(&ctb->lock, flags);
>   
> +		if (unlikely(ct_deadlocked(ct)))
> +			return -EPIPE;
> +
>   		if (msleep_interruptible(sleep_period_ms))
>   			return -EINTR;
>   		sleep_period_ms = sleep_period_ms << 1;
> @@ -585,6 +628,8 @@ static int ct_send(struct intel_guc_ct *ct,
>   		goto retry;
>   	}
>   
> +	ct->stall_time = KTIME_MAX;
> +
>   	fence = ct_get_next_fence(ct);
>   	request.fence = fence;
>   	request.status = 0;
> @@ -647,6 +692,9 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32 *action, u32 len,
>   		return -ENODEV;
>   	}
>   
> +	if (unlikely(ct->ctbs.send.broken))
> +		return -EPIPE;
> +
>   	if (flags & INTEL_GUC_CT_SEND_NB)
>   		return ct_send_nb(ct, action, len, flags);
>   
> 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 5bb8bef024c8..bee03794c1eb 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h
> @@ -9,6 +9,7 @@
>   #include <linux/interrupt.h>
>   #include <linux/spinlock.h>
>   #include <linux/workqueue.h>
> +#include <linux/ktime.h>
>   
>   #include "intel_guc_fwif.h"
>   
> @@ -68,6 +69,9 @@ struct intel_guc_ct {
>   		struct list_head incoming; /* incoming requests */
>   		struct work_struct worker; /* handler for incoming requests */
>   	} requests;
> +
> +	/** @stall_time: time of first time a CTB submission is stalled */
> +	ktime_t stall_time;
>   };
>   
>   void intel_guc_ct_init_early(struct intel_guc_ct *ct);