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

[Dong, Zhanjun]

Thanks for all comments, I will update code and prepare for next version.

Regards,
Zhanjun

-----Original Message-----
From: Dixit, Ashutosh <ashutosh.dixit at intel.com> 
Sent: June 14, 2022 12:28 PM
To: Dong, Zhanjun <zhanjun.dong at intel.com>
Cc: intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org; Teres Alexis, Alan Previn <alan.previn.teres.alexis at intel.com>; Wajdeczko, Michal <Michal.Wajdeczko at intel.com>
Subject: Re: [PATCH] drm/i915/guc: Check ctx while waiting for response

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