Re: [Intel-gfx] [PATCH 4/4] drm/i915/guc: Move and improve error message for missed CTB reply

From: Daniel Vetter <daniel@ffwll.ch>
To: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 4/4] drm/i915/guc: Move and improve error message for missed CTB reply
Date: Wed, 18 Aug 2021 16:24:51 +0200	[thread overview]
Message-ID: <YR0YMwmNHfzXT9XP@phenom.ffwll.local> (raw)
In-Reply-To: <20210701155513.2024-5-michal.wajdeczko@intel.com>

On Thu, Jul 01, 2021 at 05:55:13PM +0200, Michal Wajdeczko wrote:
> If we timeout waiting for a CT reply we print very simple error
> message. Improve that and by moving error reporting to the caller
> we can use CT_ERROR instead of DRM_ERROR and report just fence
> as error code will be reported later anyway.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>

Look reasonable.

Btw for within the driver we generally never document static inline
functions with full kerneldoc. That's overkill and they get stale real
fast. What would be useful to document is the interface with the driver at
large (i.e. non-static functions), especially for something that's used
all over like CTB will be. But then we're back to responsibilities and
especialy aroung gpu reset, so not sure whether documenting the current
code before that's sorted is the best idea.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 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 8110586ce1fd..f488a51e1ebe 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> @@ -490,9 +490,6 @@ static int wait_for_ct_request_update(struct ct_request *req, u32 *status)
>  		err = wait_for(done, 10);
>  #undef done
>  
> -	if (unlikely(err))
> -		DRM_ERROR("CT: fence %u err %d\n", req->fence, err);
> -
>  	*status = req->status;
>  	return err;
>  }
> @@ -536,8 +533,11 @@ static int ct_send(struct intel_guc_ct *ct,
>  	intel_guc_notify(ct_to_guc(ct));
>  
>  	err = wait_for_ct_request_update(&request, status);
> -	if (unlikely(err))
> +	if (unlikely(err)) {
> +		CT_ERROR(ct, "No response for request %#x (fence %u)\n",
> +			 action[0], request.fence);
>  		goto unlink;
> +	}
>  
>  	if (FIELD_GET(GUC_HXG_MSG_0_TYPE, *status) != GUC_HXG_TYPE_RESPONSE_SUCCESS) {
>  		err = -EIO;
> -- 
> 2.25.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch