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.
The convention we follow in drm is to record the version of the patch and
what changed in that version.
Generally I am ok with this version of the patch but still have a couple of
questions.
> -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;
So we have the choice of either setting the request status here as I was
suggesting earlier, e.g. as follows:
#define GUC_HXG_TYPE_REQUEST_CANCELED 4u // unused value
if (!ct_enabled)
req->status = GUC_HXG_TYPE_REQUEST_CANCELED;
We would return 0 in this case and would check for the req->status value
above where needed.
Or we can return -ECANCELED. I don't know if -ECANCELED is the right value
to return but whatever we return will have to be unique (ununsed elsewhere)
since we are relying on the return value. -ECANCELED is unique so that part
is ok.
Do other reviewers have a preference whether we should set req->status or
return a unique return value?
> *status = req->status;
> return err;
> @@ -703,11 +708,15 @@ static int ct_send(struct intel_guc_ct *ct,
>
> intel_guc_notify(ct_to_guc(ct));
>
> - err = wait_for_ct_request_update(&request, status);
> + err = wait_for_ct_request_update(ct, &request, status);
> g2h_release_space(ct, GUC_CTB_HXG_MSG_MAX_LEN);
> if (unlikely(err)) {
> - CT_ERROR(ct, "No response for request %#x (fence %u)\n",
> - action[0], request.fence);
> + if (err == -ECANCELED)
> + CT_DEBUG(ct, "Request %#x (fence %u) cancelled as CTB
> is disabled\n",
> + action[0], request.fence);
> + else
> + CT_ERROR(ct, "No response for request %#x (fence %u)\n",
> + action[0], request.fence);
> goto unlink;
> }
>
> @@ -771,8 +780,9 @@ int intel_guc_ct_send(struct intel_guc_ct *ct, const u32
> *action, u32 len,
>
> ret = ct_send(ct, action, len, response_buf, response_buf_size,
> &status);
> if (unlikely(ret < 0)) {
> - CT_ERROR(ct, "Sending action %#x failed (%pe) status=%#X\n",
> - action[0], ERR_PTR(ret), status);
> + if (ret != -ECANCELED)
> + CT_ERROR(ct, "Sending action %#x failed (%pe)
> status=%#X\n",
> + action[0], ERR_PTR(ret), status);
I am wondering why we even have this print and should we just delete it or
convert it to CT_DEBUG(). The reason is that only error prints closest to
where the actual error occurs are useful since they pin-point the error
clearly. This to be seems to be a "second" print from a higher level
function which does not seem particularly useful.
> } else if (unlikely(ret)) {
> CT_DEBUG(ct, "send action %#x returned %d (%#x)\n",
> action[0], ret, ret);
> --
> 2.36.0
>
