[PATCH] drm/xe: Check return values of functions in xe_gt_shutdown()

[Matt Roper]

On Thu, Sep 26, 2024 at 04:30:26PM +0530, apoorva.singh at intel.com wrote:
> From: Apoorva Singh <apoorva.singh at intel.com>
> 
> Check the return values of the functions xe_force_wake_get()
> and xe_force_wake_put() to prevent mistakenly treating them as
> void returns.

But the patch below isn't actually checking them in any meaningful way.
It's just printing a (redundant) message in dmesg and then blindly
moving forward without actually acting upon the failure in a sensible
way.  We already printed a message in __domain_wait() (at notice level;
you could potentially boost the severity of those messages if you want).

If we truly failed to grab forcewake for the GT domain, then our attempt
to trigger a reset via the GDRST register is also going to fail.  If
we're going to check the return value here, then the expectation is that
we'd actually take a meaningful action (e.g., wedging the GT since in
this case we appear to have lost our ability to talk to the hardware).
Some heavier action like a user-initiated FLR is probably going to be
necessary to get things working again, and just adding a xe_gt_WARN
doesn't really do anything meaningful.

Just adding extra WARN messages doesn't seem to help anything here.


Matt

> 
> Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> Signed-off-by: Apoorva Singh <apoorva.singh at intel.com>
> Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index 274737417b0f..317640554310 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -890,9 +890,12 @@ int xe_gt_suspend(struct xe_gt *gt)
>  
>  void xe_gt_shutdown(struct xe_gt *gt)
>  {
> -	xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> +	int err;
> +	err = xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> +	xe_gt_WARN(gt, err, "Acknowledgment for domain awake timedout");
>  	do_gt_reset(gt);
> -	xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> +	err = xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> +	xe_gt_WARN(gt, err, "Acknowledgment for domain sleep timedout");
>  }
>  
>  /**
> -- 
> 2.34.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation