On Thu, Jun 24, 2021 at 07:37:01PM +0200, Michal Wajdeczko wrote:
>
>
> On 24.06.2021 09:04, 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.
> >
> > Also update to ct_write to return -EIO rather than -EPIPE on a
> > corrupted descriptor.
>
> by doing so you will have the same error code for two different problems:
>
> a) corrupted CTB descriptor (definitely unrecoverable)
> b) long stall in CTB processing (still recoverable)
>
Already discussed both are treated exactly the same by the rest of the
stack so we return a single error code.
> while caller is explicitly instructed to retry only on:
>
> c) temporary stall in CTB processing (likely recoverable)
>
> so why do we want to limit our diagnostics?
>
> >
> > 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>
> > ---
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 47 +++++++++++++++++++++--
> > drivers/gpu/drm/i915/gt/uc/intel_guc_ct.h | 4 ++
> > 2 files changed, 48 insertions(+), 3 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 c9a65d05911f..27ec30b5ef47 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > @@ -319,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;
> >
> > @@ -392,7 +393,7 @@ static int ct_write(struct intel_guc_ct *ct,
> > unsigned int i;
> >
> > if (unlikely(ctb->broken))
> > - return -EPIPE;
> > + return -EIO;
> >
> > if (unlikely(desc->status))
> > goto corrupted;
> > @@ -464,7 +465,7 @@ static int ct_write(struct intel_guc_ct *ct,
> > CT_ERROR(ct, "Corrupted descriptor head=%u tail=%u status=%#x\n",
> > desc->head, desc->tail, desc->status);
> > ctb->broken = true;
> > - return -EPIPE;
> > + return -EIO;
> > }
> >
> > /**
> > @@ -507,6 +508,18 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
> > return err;
> > }
> >
> > +#define GUC_CTB_TIMEOUT_MS 1500
>
> it's 150% of core CTB timeout, maybe we should correlate them ?
>
Seems overkill.
> > +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))
> > + CT_ERROR(ct, "CT deadlocked\n");
>
> nit: in commit message you said all these changes are to "prevent
> deadlock" so maybe this message should rather be:
>
> int delta = ktime_ms_delta(ktime_get(), ct->stall_time);
>
> CT_ERROR(ct, "Communication stalled for %dms\n", delta);
>
Sure.
> (note that CT_ERROR already adds "CT" prefix)
>
> > +
> > + 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;
> > @@ -518,6 +531,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)))
>
> and maybe above message should be printed somewhere around here when we
> detect "deadlock" for the first time?
>
Not sure I follow. The error message is in the correct place if ask me.
Probably should set the broken flag though when the message is printed
though.
> > + return -EIO;
> > + 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,
> > @@ -530,7 +563,7 @@ static int ct_send_nb(struct intel_guc_ct *ct,
> >
> > spin_lock_irqsave(&ctb->lock, spin_flags);
> >
> > - ret = h2g_has_room(ctb, len + 1);
> > + ret = has_room_nb(ct, len + 1);
> > if (unlikely(ret))
> > goto out;
> >
> > @@ -574,11 +607,19 @@ static int ct_send(struct intel_guc_ct *ct,
> > retry:
> > spin_lock_irqsave(&ct->ctbs.send.lock, flags);
> > if (unlikely(!h2g_has_room(ctb, len + 1))) {
> > + if (ct->stall_time == KTIME_MAX)
> > + ct->stall_time = ktime_get();
>
> as this is a repeated pattern, maybe it should be moved to h2g_has_room
> or other wrapper ?
>
Once we check G2H credits the pattern changes, hence the reason for this
outside of the wrapper. Also IMO sometimes we go a little overboard with
wrappers and it makes the code harder to understand.
> > spin_unlock_irqrestore(&ct->ctbs.send.lock, flags);
> > +
> > + if (unlikely(ct_deadlocked(ct)))
> > + return -EIO;
> > +
> > cond_resched();
> > goto retry;
> > }
> >
> > + ct->stall_time = KTIME_MAX;
>
> this one too
>
Same as above.
Matt
> > +
> > fence = ct_get_next_fence(ct);
> > request.fence = fence;
> > request.status = 0;
> > 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 eb69263324ba..55ef7c52472f 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);
> >
