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

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

On Fri, 15 Jul 2022 14:13:13 -0700, Zhanjun Dong wrote:
>
> -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 = -ENODEV;

Good, -ENODEV seems to be the correct return value in this case.

>
>	*status = req->status;
>	return err;
> @@ -703,11 +708,18 @@ 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 == -ENODEV)
> +			/* wait_for_ct_request_update returns -ENODEV on reset/suspend in progress.
> +			 * In this case, output is debug rather than error info
> +			 */
> +			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);

A nit but I would probably prefer to move the CT_DEBUG() inside
wait_for_ct_request_update() (so we only keep 'if (err != -ENODEV)' checks
here) though it would mean adding the action argument also to
wait_for_ct_request_update().

In any case, since we have discussed this patch ad nauseam previously, this
is now:

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>