[PATCH] amdgpu_device_recover_vram always failed if only one node in shadow_list

[Lou, Wentao]

Hi Christian,

Sometimes shadow->parent would be NULL in my testbed, but not reproduce today...
Just sent out another patch following your advice.
Thanks.

BR,
Wentao


-----Original Message-----
From: Christian König <ckoenig.leichtzumerken at gmail.com> 
Sent: Tuesday, April 2, 2019 6:36 PM
To: Lou, Wentao <Wentao.Lou at amd.com>; amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH] amdgpu_device_recover_vram always failed if only one node in shadow_list

Am 02.04.19 um 11:19 schrieb wentalou:
> amdgpu_bo_restore_shadow would assign zero to r if succeeded.
> r would remain zero if there is only one node in shadow_list.
> current code would always return failure when r <= 0.
> restart the timeout for each wait was a rather problematic bug as well.
> The value of tmo SHOULD be changed, otherwise we wait tmo jiffies on each loop.
> meanwhile, fix Call Trace by NULL of shadow->parent.
>
> Change-Id: I7e836ec7ab6cd0f069aac24f88e454e906637541
> Signed-off-by: Wentao Lou <Wentao.Lou at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index c4c61e9..5a2dc44 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3183,7 +3183,7 @@ static int amdgpu_device_recover_vram(struct 
> amdgpu_device *adev)
>   
>   		/* No need to recover an evicted BO */
>   		if (shadow->tbo.mem.mem_type != TTM_PL_TT ||
> -		    shadow->parent->tbo.mem.mem_type != TTM_PL_VRAM)
> +		    shadow->parent == NULL || shadow->parent->tbo.mem.mem_type != 
> +TTM_PL_VRAM)

That doesn't looks like a good idea to me. Did you actually run into this issue?

>   			continue;
>   
>   		r = amdgpu_bo_restore_shadow(shadow, &next); @@ -3191,11 +3191,16 
> @@ static int amdgpu_device_recover_vram(struct amdgpu_device *adev)
>   			break;
>   
>   		if (fence) {
> -			r = dma_fence_wait_timeout(fence, false, tmo);
> +			tmo = dma_fence_wait_timeout(fence, false, tmo);
>   			dma_fence_put(fence);
>   			fence = next;
> -			if (r <= 0)
> +			if (tmo == 0) {
> +				r = -ETIMEDOUT;
>   				break;
> +			} else if (tmo < 0) {
> +				r = tmo;
> +				break;
> +			}
>   		} else {
>   			fence = next;
>   		}
> @@ -3206,8 +3211,8 @@ static int amdgpu_device_recover_vram(struct amdgpu_device *adev)
>   		tmo = dma_fence_wait_timeout(fence, false, tmo);
>   	dma_fence_put(fence);
>   
> -	if (r <= 0 || tmo <= 0) {
> -		DRM_ERROR("recover vram bo from shadow failed\n");
> +	if (r < 0 || tmo <= 0) {
> +		DRM_ERROR("recover vram bo from shadow failed, tmo is %d\n", tmo);

Maybe print both r and tmo in the message.

Regards,
Christian.

>   		return -EIO;
>   	}
>