Re: [Intel-gfx] [PATCH] drm/i915/guc: Check for ct enabled while waiting for response

[Dixit, Ashutosh]

On Thu, 16 Jun 2022 21:50:55 -0700, Dixit, Ashutosh wrote:
>
> On Thu, 16 Jun 2022 15:01:59 -0700, Zhanjun Dong wrote:
> >
> > We are seeing error message of "No response for request". Some cases
> > happened while waiting for response and reset/suspend action was triggered.
> > In this case, no response is not an error, active requests will be
> > cancelled.
> >
> > This patch will handle this condition and change the error message into
> > debug message.
> >
> > Signed-off-by: Zhanjun Dong <zhanjun.d...@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 24 ++++++++++++++++-------
> >  1 file changed, 17 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 f01325cd1b62..f07a7666b1ad 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> > @@ -455,6 +455,7 @@ static int ct_write(struct intel_guc_ct *ct,
> >
> >  /**
> >   * wait_for_ct_request_update - Wait for CT request state update.
> > + * @ct:            pointer to CT
> >   * @req:   pointer to pending request
> >   * @status:        placeholder for status
> >   *
> > @@ -467,9 +468,10 @@ static int ct_write(struct intel_guc_ct *ct,
> >   * *       0 response received (status is valid)
> >   * *       -ETIMEDOUT no response within hardcoded timeout
> >   */
> > -static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
> > +static int wait_for_ct_request_update(struct intel_guc_ct *ct, struct 
> > ct_request *req, u32 *status)
> >  {
> >     int err;
> > +   bool ct_enabled;
> >
> >     /*
> >      * Fast commands should complete in less than 10us, so sample quickly
> > @@ -481,12 +483,15 @@ static int wait_for_ct_request_update(struct 
> > ct_request *req, u32 *status)
> >  #define GUC_CTB_RESPONSE_TIMEOUT_SHORT_MS 10
> >  #define GUC_CTB_RESPONSE_TIMEOUT_LONG_MS 1000
> >  #define done \
> > -   (FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \
> > +   (!(ct_enabled = intel_guc_ct_enabled(ct)) || \
> > +    FIELD_GET(GUC_HXG_MSG_0_ORIGIN, READ_ONCE(req->status)) == \
> >      GUC_HXG_ORIGIN_GUC)
> >     err = wait_for_us(done, GUC_CTB_RESPONSE_TIMEOUT_SHORT_MS);
> >     if (err)
> >             err = wait_for(done, GUC_CTB_RESPONSE_TIMEOUT_LONG_MS);
> >  #undef done
> > +   if (!ct_enabled)
> > +           err = -ECANCELED;
>
> Actually here's an even simpler suggestion. We could just do:
>
>       if (!ct_enabled)
>               CT_DEBUG(ct, "Request %#x (fence %u) cancelled as CTB is 
> disabled\n", ...);
>
> And return 0 as before. This way we won't have to make any changes in
> either ct_send() or intel_guc_ct_send(). So intel_guc_ct_enabled() just
> serves to get us out of the wait early and prevent the -ETIMEDOUT return
> (and 0 return avoids all the error messages we are trying to eliminate).

Actually will need to unlink the request too, so it will be something like:

        if (!ct_enabled) {
                CT_DEBUG(ct, "Request %#x (fence %u) cancelled as CTB is 
disabled\n", ...);

                spin_lock_irqsave(&ct->requests.lock, flags);
                list_del(&request.link);
                spin_unlock_irqrestore(&ct->requests.lock, flags);
        }