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

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 02 Jun 2022 10:21:19 -0700, Zhanjun Dong wrote:
>

Hi Zhanjun,

> 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.

IMO the patch title should be changed: which ctx are we checking while
waiting for response? Something like "check for ct enabled while waiting
for response"?

> @@ -481,12 +481,14 @@ 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)) == \
> +	(!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 (!intel_guc_ct_enabled(ct))
> +		err = -ECANCELED;

Also, I really don't like intel_guc_ct_enabled() being called in two
places. Is there a possibility that intel_guc_ct_enabled() can return false
in the first place (causing the wait to exit) and then return true in the
second place (so we don't return -ECANCELED)?

Is it possible to change the status of the request to something else from
intel_guc_ct_disable() (or wherever ct->enabled is set to false) rather
than introducing intel_guc_ct_enabled() checks here. Changing the status of
the request when CT goes down would cause the wait's to exit here. And then
we can check that special request status signifying CT went down?

Thanks.
--
Ashutosh