Re: [PATCH 3/3] drm/amdgpu: Switch to interrupted wait to recover from ring hang.

2018-04-24 Thread Panariti, David
> Kind of dma_fence_wait_killable, except that we don't have such API
> (maybe worth adding ?)
Depends on how many places it would be called, or think it might be called.  
Can always factor on the 2nd time it's needed.
Factoring, IMO, rarely hurts.  The factored function can easily be visited 
using `M-.' ;->

Also, if the wait could be very long, would a log message, something like "xxx 
has run for Y seconds."  help?
I personally hate hanging w/no info.

regards,
davep


From: Grodzovsky, Andrey
Sent: Tuesday, April 24, 2018 11:58:19 AM
To: Panariti, David; linux-kernel@vger.kernel.org; amd-...@lists.freedesktop.org
Cc: Deucher, Alexander; Koenig, Christian; o...@redhat.com; 
a...@linux-foundation.org; ebied...@xmission.com
Subject: Re: [PATCH 3/3] drm/amdgpu: Switch to interrupted wait to recover from 
ring hang.



On 04/24/2018 11:52 AM, Panariti, David wrote:
> Hi,
>
> It looks like there can be an infinite loop if neither of the if()'s become 
> true.
> Is that an impossible condition?

That intended, we want to wait until either the fence signals or fatal
signal received, we don't want to terminate the wait if fence is not
signaled  even  when interrupted by non fatal signal.
Kind of dma_fence_wait_killable, except that we don't have such API
(maybe worth adding ?)

Andrey

>
> -Original Message-
> From: Andrey Grodzovsky <andrey.grodzov...@amd.com>
> Sent: Tuesday, April 24, 2018 11:31 AM
> To: linux-kernel@vger.kernel.org; amd-...@lists.freedesktop.org
> Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Koenig, Christian 
> <christian.koe...@amd.com>; Panariti, David <david.panar...@amd.com>; 
> o...@redhat.com; a...@linux-foundation.org; ebied...@xmission.com; 
> Grodzovsky, Andrey <andrey.grodzov...@amd.com>
> Subject: [PATCH 3/3] drm/amdgpu: Switch to interrupted wait to recover from 
> ring hang.
>
> If the ring is hanging for some reason allow to recover the waiting by 
> sending fatal signal.
>
> Originally-by: David Panariti <david.panar...@amd.com>
> Signed-off-by: Andrey Grodzovsky <andrey.grodzov...@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 14 ++
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index eb80edf..37a36af 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -421,10 +421,16 @@ int amdgpu_ctx_wait_prev_fence(struct amdgpu_ctx *ctx, 
> unsigned ring_id)
>
>   if (other) {
>   signed long r;
> - r = dma_fence_wait_timeout(other, false, MAX_SCHEDULE_TIMEOUT);
> - if (r < 0) {
> - DRM_ERROR("Error (%ld) waiting for fence!\n", r);
> - return r;
> +
> + while (true) {
> + if ((r = dma_fence_wait_timeout(other, true,
> + MAX_SCHEDULE_TIMEOUT)) >= 0)
> + return 0;
> +
> + if (fatal_signal_pending(current)) {
> + DRM_ERROR("Error (%ld) waiting for fence!\n", 
> r);
> + return r;
> + }
>   }
>   }
>
> --
> 2.7.4
>



Re: [PATCH 3/3] drm/amdgpu: Switch to interrupted wait to recover from ring hang.

2018-04-24 Thread Panariti, David
> Kind of dma_fence_wait_killable, except that we don't have such API
> (maybe worth adding ?)
Depends on how many places it would be called, or think it might be called.  
Can always factor on the 2nd time it's needed.
Factoring, IMO, rarely hurts.  The factored function can easily be visited 
using `M-.' ;->

Also, if the wait could be very long, would a log message, something like "xxx 
has run for Y seconds."  help?
I personally hate hanging w/no info.

regards,
davep


From: Grodzovsky, Andrey
Sent: Tuesday, April 24, 2018 11:58:19 AM
To: Panariti, David; linux-kernel@vger.kernel.org; amd-...@lists.freedesktop.org
Cc: Deucher, Alexander; Koenig, Christian; o...@redhat.com; 
a...@linux-foundation.org; ebied...@xmission.com
Subject: Re: [PATCH 3/3] drm/amdgpu: Switch to interrupted wait to recover from 
ring hang.



On 04/24/2018 11:52 AM, Panariti, David wrote:
> Hi,
>
> It looks like there can be an infinite loop if neither of the if()'s become 
> true.
> Is that an impossible condition?

That intended, we want to wait until either the fence signals or fatal
signal received, we don't want to terminate the wait if fence is not
signaled  even  when interrupted by non fatal signal.
Kind of dma_fence_wait_killable, except that we don't have such API
(maybe worth adding ?)

Andrey

>
> -Original Message-
> From: Andrey Grodzovsky 
> Sent: Tuesday, April 24, 2018 11:31 AM
> To: linux-kernel@vger.kernel.org; amd-...@lists.freedesktop.org
> Cc: Deucher, Alexander ; Koenig, Christian 
> ; Panariti, David ; 
> o...@redhat.com; a...@linux-foundation.org; ebied...@xmission.com; 
> Grodzovsky, Andrey 
> Subject: [PATCH 3/3] drm/amdgpu: Switch to interrupted wait to recover from 
> ring hang.
>
> If the ring is hanging for some reason allow to recover the waiting by 
> sending fatal signal.
>
> Originally-by: David Panariti 
> Signed-off-by: Andrey Grodzovsky 
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 14 ++
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index eb80edf..37a36af 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> @@ -421,10 +421,16 @@ int amdgpu_ctx_wait_prev_fence(struct amdgpu_ctx *ctx, 
> unsigned ring_id)
>
>   if (other) {
>   signed long r;
> - r = dma_fence_wait_timeout(other, false, MAX_SCHEDULE_TIMEOUT);
> - if (r < 0) {
> - DRM_ERROR("Error (%ld) waiting for fence!\n", r);
> - return r;
> +
> + while (true) {
> + if ((r = dma_fence_wait_timeout(other, true,
> + MAX_SCHEDULE_TIMEOUT)) >= 0)
> + return 0;
> +
> + if (fatal_signal_pending(current)) {
> + DRM_ERROR("Error (%ld) waiting for fence!\n", 
> r);
> + return r;
> + }
>   }
>   }
>
> --
> 2.7.4
>



RE: [PATCH 3/3] drm/amdgpu: Switch to interrupted wait to recover from ring hang.

2018-04-24 Thread Panariti, David
Hi,

It looks like there can be an infinite loop if neither of the if()'s become 
true.
Is that an impossible condition?

-Original Message-
From: Andrey Grodzovsky <andrey.grodzov...@amd.com> 
Sent: Tuesday, April 24, 2018 11:31 AM
To: linux-kernel@vger.kernel.org; amd-...@lists.freedesktop.org
Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Koenig, Christian 
<christian.koe...@amd.com>; Panariti, David <david.panar...@amd.com>; 
o...@redhat.com; a...@linux-foundation.org; ebied...@xmission.com; Grodzovsky, 
Andrey <andrey.grodzov...@amd.com>
Subject: [PATCH 3/3] drm/amdgpu: Switch to interrupted wait to recover from 
ring hang.

If the ring is hanging for some reason allow to recover the waiting by sending 
fatal signal.

Originally-by: David Panariti <david.panar...@amd.com>
Signed-off-by: Andrey Grodzovsky <andrey.grodzov...@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index eb80edf..37a36af 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -421,10 +421,16 @@ int amdgpu_ctx_wait_prev_fence(struct amdgpu_ctx *ctx, 
unsigned ring_id)
 
if (other) {
signed long r;
-   r = dma_fence_wait_timeout(other, false, MAX_SCHEDULE_TIMEOUT);
-   if (r < 0) {
-   DRM_ERROR("Error (%ld) waiting for fence!\n", r);
-   return r;
+
+   while (true) {
+   if ((r = dma_fence_wait_timeout(other, true,
+   MAX_SCHEDULE_TIMEOUT)) >= 0)
+   return 0;
+
+   if (fatal_signal_pending(current)) {
+   DRM_ERROR("Error (%ld) waiting for fence!\n", 
r);
+   return r;
+   }
}
}
 
--
2.7.4



RE: [PATCH 3/3] drm/amdgpu: Switch to interrupted wait to recover from ring hang.

2018-04-24 Thread Panariti, David
Hi,

It looks like there can be an infinite loop if neither of the if()'s become 
true.
Is that an impossible condition?

-Original Message-
From: Andrey Grodzovsky  
Sent: Tuesday, April 24, 2018 11:31 AM
To: linux-kernel@vger.kernel.org; amd-...@lists.freedesktop.org
Cc: Deucher, Alexander ; Koenig, Christian 
; Panariti, David ; 
o...@redhat.com; a...@linux-foundation.org; ebied...@xmission.com; Grodzovsky, 
Andrey 
Subject: [PATCH 3/3] drm/amdgpu: Switch to interrupted wait to recover from 
ring hang.

If the ring is hanging for some reason allow to recover the waiting by sending 
fatal signal.

Originally-by: David Panariti 
Signed-off-by: Andrey Grodzovsky 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index eb80edf..37a36af 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -421,10 +421,16 @@ int amdgpu_ctx_wait_prev_fence(struct amdgpu_ctx *ctx, 
unsigned ring_id)
 
if (other) {
signed long r;
-   r = dma_fence_wait_timeout(other, false, MAX_SCHEDULE_TIMEOUT);
-   if (r < 0) {
-   DRM_ERROR("Error (%ld) waiting for fence!\n", r);
-   return r;
+
+   while (true) {
+   if ((r = dma_fence_wait_timeout(other, true,
+   MAX_SCHEDULE_TIMEOUT)) >= 0)
+   return 0;
+
+   if (fatal_signal_pending(current)) {
+   DRM_ERROR("Error (%ld) waiting for fence!\n", 
r);
+   return r;
+   }
}
}
 
--
2.7.4