[PATCH] drm/radeon: Avoid double gpu reset by adding a timeout on IB ring tests.

[Bruno Kant]

Le 2016-02-08 15:51, Matthew Dawson a écrit :
>> Yeah, 1s works as well. But that 100ms shouldn't be enough sounds like
>> another bug to me.
> Probably, but it has a tendency to crash my entire system, so I'm not 
> sure how
> to debug it further.  Netconsole maybe?  I probably won't be able to 
> dig into
> this due to time since 1s does work.
> 

Hello,

netconsole won't help if the box gets killed. IP, USB, everything should 
be dead on a hang. I have a netconsole running, but I see nothing on it 
when my box hangs.

I'm now trying to use crash dumps, they could help you also. It dumps a 
core, plus a short text file with the main kernel panic message. If your 
box panics, you would get at least the text file (sort of main cause).

The core dump itself must later be read with kernel debug symbols, this 
can be more difficult to set up. But try kdump, this was easy to set up 
on my Fedora box, look at this tuto:

https://fedoraproject.org/wiki/How_to_use_kdump_to_debug_kernel_crashes

You may also later need a custom built kernel, or a debug kernel (which 
could slow down your box). I have noticed some kernel debug options 
which generate panics for different kind of kernel locks or oops.

Best regards

[PATCH] drm/radeon: Avoid double gpu reset by adding a timeout on IB ring tests.

[Alex Deucher]

On Mon, Feb 8, 2016 at 4:38 AM, Christian König  
wrote:
> Am 07.02.2016 um 22:51 schrieb Matthew Dawson:
>>
>> When the radeon driver resets a gpu, it attempts to test whether all the
>> rings can successfully handle an IB.  If these rings fail to respond, the
>> process will wait forever.  Another gpu reset can't happen at this point,
>> as the current reset holds a lock required to do so.  Instead, make all
>> the IB tests run with a timeout, so the system can attempt to recover
>> in this case.
>>
>> While this doesn't fix the underlying issue with card resets failing, it
>> gives the system a higher chance of recovering.  These timeouts have been
>> confirmed to help both a Tathi and Hawaii card recover after a gpu reset.
>>
>> This also adds a new function, radeon_fence_wait_timeout, that behaves
>> like
>> fence_wait_timeout.  It is used instead of fence_wait_timeout as it
>> continues
>> to work during a reset.  radeon_fence_wait is changed to be implemented
>> using this function.
>>
>> V2:
>>   - Changed the timeout to 1s, as the default 10s from radeon_wait_timeout
>> was
>> too long.  A timeout of 100ms was tested and found to be too short.
>>   - Changed radeon_fence_wait_timeout to behave more like
>> fence_wait_timeout.
>>
>> Signed-off-by: Matthew Dawson 
>
>
> Reviewed-by: Christian König 


Applied.  Thanks!

Alex

>
> Regards,
> Christian.
>
>
>> ---
>>   drivers/gpu/drm/radeon/cik.c  | 11 --
>>   drivers/gpu/drm/radeon/cik_sdma.c |  9 ++--
>>   drivers/gpu/drm/radeon/r100.c | 10 +++--
>>   drivers/gpu/drm/radeon/r600.c | 10 +++--
>>   drivers/gpu/drm/radeon/r600_dma.c |  9 ++--
>>   drivers/gpu/drm/radeon/radeon.h   |  2 ++
>>   drivers/gpu/drm/radeon/radeon_fence.c | 40
>> ---
>>   drivers/gpu/drm/radeon/radeon_vce.c   | 11 +++---
>>   drivers/gpu/drm/radeon/uvd_v1_0.c | 10 +++--
>>   9 files changed, 89 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
>> index 4c30d8c..0600140 100644
>> --- a/drivers/gpu/drm/radeon/cik.c
>> +++ b/drivers/gpu/drm/radeon/cik.c
>> @@ -4219,13 +4219,20 @@ int cik_ib_test(struct radeon_device *rdev, struct
>> radeon_ring *ring)
>> DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
>> return r;
>> }
>> -   r = radeon_fence_wait(ib.fence, false);
>> -   if (r) {
>> +   r = radeon_fence_wait_timeout(ib.fence, false, usecs_to_jiffies(
>> +   RADEON_USEC_IB_TEST_TIMEOUT));
>> +   if (r < 0) {
>> DRM_ERROR("radeon: fence wait failed (%d).\n", r);
>> radeon_scratch_free(rdev, scratch);
>> radeon_ib_free(rdev, &ib);
>> return r;
>> +   } else if (r == 0) {
>> +   DRM_ERROR("radeon: fence wait timed out.\n");
>> +   radeon_scratch_free(rdev, scratch);
>> +   radeon_ib_free(rdev, &ib);
>> +   return -ETIMEDOUT;
>> }
>> +   r = 0;
>> for (i = 0; i < rdev->usec_timeout; i++) {
>> tmp = RREG32(scratch);
>> if (tmp == 0xDEADBEEF)
>> diff --git a/drivers/gpu/drm/radeon/cik_sdma.c
>> b/drivers/gpu/drm/radeon/cik_sdma.c
>> index d16f2ee..9c351dc 100644
>> --- a/drivers/gpu/drm/radeon/cik_sdma.c
>> +++ b/drivers/gpu/drm/radeon/cik_sdma.c
>> @@ -737,11 +737,16 @@ int cik_sdma_ib_test(struct radeon_device *rdev,
>> struct radeon_ring *ring)
>> DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
>> return r;
>> }
>> -   r = radeon_fence_wait(ib.fence, false);
>> -   if (r) {
>> +   r = radeon_fence_wait_timeout(ib.fence, false, usecs_to_jiffies(
>> +   RADEON_USEC_IB_TEST_TIMEOUT));
>> +   if (r < 0) {
>> DRM_ERROR("radeon: fence wait failed (%d).\n", r);
>> return r;
>> +   } else if (r == 0) {
>> +   DRM_ERROR("radeon: fence wait timed out.\n");
>> +   return -ETIMEDOUT;
>> }
>> +   r = 0;
>> for (i = 0; i < rdev->usec_timeout; i++) {
>> tmp = le32_to_cpu(rdev->wb.wb[index/4]);
>> if (tmp == 0xDEADBEEF)
>> diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
>> index 5eae0a8..6e478a2 100644
>> --- a/drivers/gpu/drm/radeon/r100.c
>> +++ b/drivers/gpu/drm/radeon/r100.c
>> @@ -3732,11 +3732,17 @@ int r100_ib_test(struct radeon_device *rdev,
>> struct radeon_ring *ring)
>> DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
>> goto free_ib;
>> }
>> -   r = radeon_fence_wait(ib.fence, false);
>> -   if (r) {
>> +   r = radeon_fence_wait_timeout(ib.fence, false, usecs_to_jiffies(
>> +   RADEON_USEC_IB_TEST_TIMEOUT));
>> +   if (r < 0) {
>> DRM_ERROR("radeon: fence wait failed (%d).\n", r);
>>   

[PATCH] drm/radeon: Avoid double gpu reset by adding a timeout on IB ring tests.

[Christian König]

Am 07.02.2016 um 22:51 schrieb Matthew Dawson:
> When the radeon driver resets a gpu, it attempts to test whether all the
> rings can successfully handle an IB.  If these rings fail to respond, the
> process will wait forever.  Another gpu reset can't happen at this point,
> as the current reset holds a lock required to do so.  Instead, make all
> the IB tests run with a timeout, so the system can attempt to recover
> in this case.
>
> While this doesn't fix the underlying issue with card resets failing, it
> gives the system a higher chance of recovering.  These timeouts have been
> confirmed to help both a Tathi and Hawaii card recover after a gpu reset.
>
> This also adds a new function, radeon_fence_wait_timeout, that behaves like
> fence_wait_timeout.  It is used instead of fence_wait_timeout as it continues
> to work during a reset.  radeon_fence_wait is changed to be implemented
> using this function.
>
> V2:
>   - Changed the timeout to 1s, as the default 10s from radeon_wait_timeout was
> too long.  A timeout of 100ms was tested and found to be too short.
>   - Changed radeon_fence_wait_timeout to behave more like fence_wait_timeout.
>
> Signed-off-by: Matthew Dawson 

Reviewed-by: Christian König 

Regards,
Christian.

> ---
>   drivers/gpu/drm/radeon/cik.c  | 11 --
>   drivers/gpu/drm/radeon/cik_sdma.c |  9 ++--
>   drivers/gpu/drm/radeon/r100.c | 10 +++--
>   drivers/gpu/drm/radeon/r600.c | 10 +++--
>   drivers/gpu/drm/radeon/r600_dma.c |  9 ++--
>   drivers/gpu/drm/radeon/radeon.h   |  2 ++
>   drivers/gpu/drm/radeon/radeon_fence.c | 40 
> ---
>   drivers/gpu/drm/radeon/radeon_vce.c   | 11 +++---
>   drivers/gpu/drm/radeon/uvd_v1_0.c | 10 +++--
>   9 files changed, 89 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
> index 4c30d8c..0600140 100644
> --- a/drivers/gpu/drm/radeon/cik.c
> +++ b/drivers/gpu/drm/radeon/cik.c
> @@ -4219,13 +4219,20 @@ int cik_ib_test(struct radeon_device *rdev, struct 
> radeon_ring *ring)
>   DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
>   return r;
>   }
> - r = radeon_fence_wait(ib.fence, false);
> - if (r) {
> + r = radeon_fence_wait_timeout(ib.fence, false, usecs_to_jiffies(
> + RADEON_USEC_IB_TEST_TIMEOUT));
> + if (r < 0) {
>   DRM_ERROR("radeon: fence wait failed (%d).\n", r);
>   radeon_scratch_free(rdev, scratch);
>   radeon_ib_free(rdev, &ib);
>   return r;
> + } else if (r == 0) {
> + DRM_ERROR("radeon: fence wait timed out.\n");
> + radeon_scratch_free(rdev, scratch);
> + radeon_ib_free(rdev, &ib);
> + return -ETIMEDOUT;
>   }
> + r = 0;
>   for (i = 0; i < rdev->usec_timeout; i++) {
>   tmp = RREG32(scratch);
>   if (tmp == 0xDEADBEEF)
> diff --git a/drivers/gpu/drm/radeon/cik_sdma.c 
> b/drivers/gpu/drm/radeon/cik_sdma.c
> index d16f2ee..9c351dc 100644
> --- a/drivers/gpu/drm/radeon/cik_sdma.c
> +++ b/drivers/gpu/drm/radeon/cik_sdma.c
> @@ -737,11 +737,16 @@ int cik_sdma_ib_test(struct radeon_device *rdev, struct 
> radeon_ring *ring)
>   DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
>   return r;
>   }
> - r = radeon_fence_wait(ib.fence, false);
> - if (r) {
> + r = radeon_fence_wait_timeout(ib.fence, false, usecs_to_jiffies(
> + RADEON_USEC_IB_TEST_TIMEOUT));
> + if (r < 0) {
>   DRM_ERROR("radeon: fence wait failed (%d).\n", r);
>   return r;
> + } else if (r == 0) {
> + DRM_ERROR("radeon: fence wait timed out.\n");
> + return -ETIMEDOUT;
>   }
> + r = 0;
>   for (i = 0; i < rdev->usec_timeout; i++) {
>   tmp = le32_to_cpu(rdev->wb.wb[index/4]);
>   if (tmp == 0xDEADBEEF)
> diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
> index 5eae0a8..6e478a2 100644
> --- a/drivers/gpu/drm/radeon/r100.c
> +++ b/drivers/gpu/drm/radeon/r100.c
> @@ -3732,11 +3732,17 @@ int r100_ib_test(struct radeon_device *rdev, struct 
> radeon_ring *ring)
>   DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
>   goto free_ib;
>   }
> - r = radeon_fence_wait(ib.fence, false);
> - if (r) {
> + r = radeon_fence_wait_timeout(ib.fence, false, usecs_to_jiffies(
> + RADEON_USEC_IB_TEST_TIMEOUT));
> + if (r < 0) {
>   DRM_ERROR("radeon: fence wait failed (%d).\n", r);
>   goto free_ib;
> + } else if (r == 0) {
> + DRM_ERROR("radeon: fence wait timed out.\n");
> + r = -ETIMEDOUT;
> + goto free_ib;
>   }
> + r = 0;
>   for (i = 0; i < rdev->usec_timeout; i++) {
>   tmp = RREG32(scratch);
>   if

[PATCH] drm/radeon: Avoid double gpu reset by adding a timeout on IB ring tests.

[Matthew Dawson]

On Wednesday, February 3, 2016 1:24:49 PM EST Christian König wrote:
> Am 31.01.2016 um 19:50 schrieb Matthew Dawson:
> > On Sunday, January 24, 2016 10:49:00 AM EST Christian König wrote:
> >> Am 24.01.2016 um 07:18 schrieb Matthew Dawson:
> >>> When the radeon driver resets a gpu, it attempts to test whether all the
> >>> rings can successfully handle an IB.  If these rings fail to respond,
> >>> the
> >>> process will wait forever.  Another gpu reset can't happen at this
> >>> point,
> >>> as the current reset holds a lock required to do so.  Instead, make all
> >>> the IB tests run with a timeout, so the system can attempt to recover
> >>> in this case.
> >>> 
> >>> While this doesn't fix the underlying issue with card resets failing, it
> >>> gives the system a higher chance of recovering.  These timeouts have
> >>> been
> >>> confirmed to help both a Tathi and Hawaii card recover after a gpu
> >>> reset.
> >>> 
> >>> I haven't been able to test this on other cards, but everything
> >>> compiles.
> >>> I wasn't sure what to use for a timeout value, so for now I've used
> >>> radeon_lockup_timeout.  When that is 0, the timeout functionality in
> >>> this
> >>> patch is also disabled.
> >>> 
> >>> This also adds a new function, radeon_fence_wait_timeout, that behaves
> >>> like
> >>> radeon_fence_wait except allowing a different timeout value to be passed
> >>> to
> >>> radeon_fence_wait_seq_timeout.
> >>> 
> >>> Signed-off-by: Matthew Dawson 
> >> 
> >> Really good idea, but please don't use radeon_lockup_timeout for this
> >> cause the 10 seconds default of that is way to long. Instead just use a
> >> fixed, let's say 100ms timeout defined in radeon.h.
> > 
> > I originally tried 100ms, but I found that to be too short (my system
> > would
> > just lockup completely (no network even)).  I found 1s is long enough, and
> > isn't so long to be annoying.  Would that be ok?
> 
> Yeah, 1s works as well. But that 100ms shouldn't be enough sounds like
> another bug to me.
Probably, but it has a tendency to crash my entire system, so I'm not sure how 
to debug it further.  Netconsole maybe?  I probably won't be able to dig into 
this due to time since 1s does work.

> 
> >> Also don't define our own radeon_fence_wait_timeout, just use
> >> fence_wait_timeout(&fence->base, RADEON_IB_TEST_TIMEOUT) for this.
> > 
> > I switched everything over to this, however the ib test always times out
> > after a gpu reset (on startup, everything is normal).  I'm not sure why
> > fence_wait_timeout isn't being signaled while
> > radeon_fence_wait_seq_timeout
> > is.  I'm suspicious that either radeon_fence_enable_signaling is not doing
> > something necessary to get the fence completion to actual signal
> > radeon_fence_default_wait, or because we hold the exclusive lock during a
> > reset radeon_fence_check_lockup never runs and thus never triggers the
> > fence or enables some irq lines.
> > 
> > Is it worth digging through the interactions here to get dma-buf fences to
> > work correctly during a lockup, or would it be better to just keep
> > radeon_fence_wait_timeout?  If option 1 is preferred, I'm happy to learn
> > but I need some help learning how it is supposed to work.
> 
> In this case feel free to add the radeon_fence_wait_timeout. It's
> probably a good idea to get rid of the exclusive lock sooner or later in
> radeon as well, but that really is a long term project.
> 
> If you're really bored I can give you tons of input where to start with
> that.
That sounds like a good project to learn more about the driver, so I'd be 
interested.  I don't have a lot of time right now to play with it though.  I 
do want to finish off the tf2 crasher first as well.  Once I have time I'll 
poke the list to get started.

Thanks for the reviews!
-- 
Matthew
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: 

[PATCH] drm/radeon: Avoid double gpu reset by adding a timeout on IB ring tests.

[Matthew Dawson]

When the radeon driver resets a gpu, it attempts to test whether all the
rings can successfully handle an IB.  If these rings fail to respond, the
process will wait forever.  Another gpu reset can't happen at this point,
as the current reset holds a lock required to do so.  Instead, make all
the IB tests run with a timeout, so the system can attempt to recover
in this case.

While this doesn't fix the underlying issue with card resets failing, it
gives the system a higher chance of recovering.  These timeouts have been
confirmed to help both a Tathi and Hawaii card recover after a gpu reset.

This also adds a new function, radeon_fence_wait_timeout, that behaves like
fence_wait_timeout.  It is used instead of fence_wait_timeout as it continues
to work during a reset.  radeon_fence_wait is changed to be implemented
using this function.

V2:
 - Changed the timeout to 1s, as the default 10s from radeon_wait_timeout was
too long.  A timeout of 100ms was tested and found to be too short.
 - Changed radeon_fence_wait_timeout to behave more like fence_wait_timeout.

Signed-off-by: Matthew Dawson 
---
 drivers/gpu/drm/radeon/cik.c  | 11 --
 drivers/gpu/drm/radeon/cik_sdma.c |  9 ++--
 drivers/gpu/drm/radeon/r100.c | 10 +++--
 drivers/gpu/drm/radeon/r600.c | 10 +++--
 drivers/gpu/drm/radeon/r600_dma.c |  9 ++--
 drivers/gpu/drm/radeon/radeon.h   |  2 ++
 drivers/gpu/drm/radeon/radeon_fence.c | 40 ---
 drivers/gpu/drm/radeon/radeon_vce.c   | 11 +++---
 drivers/gpu/drm/radeon/uvd_v1_0.c | 10 +++--
 9 files changed, 89 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
index 4c30d8c..0600140 100644
--- a/drivers/gpu/drm/radeon/cik.c
+++ b/drivers/gpu/drm/radeon/cik.c
@@ -4219,13 +4219,20 @@ int cik_ib_test(struct radeon_device *rdev, struct 
radeon_ring *ring)
DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
return r;
}
-   r = radeon_fence_wait(ib.fence, false);
-   if (r) {
+   r = radeon_fence_wait_timeout(ib.fence, false, usecs_to_jiffies(
+   RADEON_USEC_IB_TEST_TIMEOUT));
+   if (r < 0) {
DRM_ERROR("radeon: fence wait failed (%d).\n", r);
radeon_scratch_free(rdev, scratch);
radeon_ib_free(rdev, &ib);
return r;
+   } else if (r == 0) {
+   DRM_ERROR("radeon: fence wait timed out.\n");
+   radeon_scratch_free(rdev, scratch);
+   radeon_ib_free(rdev, &ib);
+   return -ETIMEDOUT;
}
+   r = 0;
for (i = 0; i < rdev->usec_timeout; i++) {
tmp = RREG32(scratch);
if (tmp == 0xDEADBEEF)
diff --git a/drivers/gpu/drm/radeon/cik_sdma.c 
b/drivers/gpu/drm/radeon/cik_sdma.c
index d16f2ee..9c351dc 100644
--- a/drivers/gpu/drm/radeon/cik_sdma.c
+++ b/drivers/gpu/drm/radeon/cik_sdma.c
@@ -737,11 +737,16 @@ int cik_sdma_ib_test(struct radeon_device *rdev, struct 
radeon_ring *ring)
DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
return r;
}
-   r = radeon_fence_wait(ib.fence, false);
-   if (r) {
+   r = radeon_fence_wait_timeout(ib.fence, false, usecs_to_jiffies(
+   RADEON_USEC_IB_TEST_TIMEOUT));
+   if (r < 0) {
DRM_ERROR("radeon: fence wait failed (%d).\n", r);
return r;
+   } else if (r == 0) {
+   DRM_ERROR("radeon: fence wait timed out.\n");
+   return -ETIMEDOUT;
}
+   r = 0;
for (i = 0; i < rdev->usec_timeout; i++) {
tmp = le32_to_cpu(rdev->wb.wb[index/4]);
if (tmp == 0xDEADBEEF)
diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
index 5eae0a8..6e478a2 100644
--- a/drivers/gpu/drm/radeon/r100.c
+++ b/drivers/gpu/drm/radeon/r100.c
@@ -3732,11 +3732,17 @@ int r100_ib_test(struct radeon_device *rdev, struct 
radeon_ring *ring)
DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
goto free_ib;
}
-   r = radeon_fence_wait(ib.fence, false);
-   if (r) {
+   r = radeon_fence_wait_timeout(ib.fence, false, usecs_to_jiffies(
+   RADEON_USEC_IB_TEST_TIMEOUT));
+   if (r < 0) {
DRM_ERROR("radeon: fence wait failed (%d).\n", r);
goto free_ib;
+   } else if (r == 0) {
+   DRM_ERROR("radeon: fence wait timed out.\n");
+   r = -ETIMEDOUT;
+   goto free_ib;
}
+   r = 0;
for (i = 0; i < rdev->usec_timeout; i++) {
tmp = RREG32(scratch);
if (tmp == 0xDEADBEEF) {
diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
index cc2fdf0..ed12104 100644
--- a/drivers/gpu/drm/radeon/r600.c
+++ b/drivers/gpu/drm/radeon/r600.c
@@ -3381,11 +3381

[PATCH] drm/radeon: Avoid double gpu reset by adding a timeout on IB ring tests.

[Christian König]

Am 31.01.2016 um 19:50 schrieb Matthew Dawson:
> On Sunday, January 24, 2016 10:49:00 AM EST Christian König wrote:
>> Am 24.01.2016 um 07:18 schrieb Matthew Dawson:
>>> When the radeon driver resets a gpu, it attempts to test whether all the
>>> rings can successfully handle an IB.  If these rings fail to respond, the
>>> process will wait forever.  Another gpu reset can't happen at this point,
>>> as the current reset holds a lock required to do so.  Instead, make all
>>> the IB tests run with a timeout, so the system can attempt to recover
>>> in this case.
>>>
>>> While this doesn't fix the underlying issue with card resets failing, it
>>> gives the system a higher chance of recovering.  These timeouts have been
>>> confirmed to help both a Tathi and Hawaii card recover after a gpu reset.
>>>
>>> I haven't been able to test this on other cards, but everything compiles.
>>> I wasn't sure what to use for a timeout value, so for now I've used
>>> radeon_lockup_timeout.  When that is 0, the timeout functionality in this
>>> patch is also disabled.
>>>
>>> This also adds a new function, radeon_fence_wait_timeout, that behaves
>>> like
>>> radeon_fence_wait except allowing a different timeout value to be passed
>>> to
>>> radeon_fence_wait_seq_timeout.
>>>
>>> Signed-off-by: Matthew Dawson 
>> Really good idea, but please don't use radeon_lockup_timeout for this
>> cause the 10 seconds default of that is way to long. Instead just use a
>> fixed, let's say 100ms timeout defined in radeon.h.
> I originally tried 100ms, but I found that to be too short (my system would
> just lockup completely (no network even)).  I found 1s is long enough, and
> isn't so long to be annoying.  Would that be ok?

Yeah, 1s works as well. But that 100ms shouldn't be enough sounds like 
another bug to me.

>
>> Also don't define our own radeon_fence_wait_timeout, just use
>> fence_wait_timeout(&fence->base, RADEON_IB_TEST_TIMEOUT) for this.
> I switched everything over to this, however the ib test always times out after
> a gpu reset (on startup, everything is normal).  I'm not sure why
> fence_wait_timeout isn't being signaled while radeon_fence_wait_seq_timeout
> is.  I'm suspicious that either radeon_fence_enable_signaling is not doing
> something necessary to get the fence completion to actual signal
> radeon_fence_default_wait, or because we hold the exclusive lock during a
> reset radeon_fence_check_lockup never runs and thus never triggers the fence
> or enables some irq lines.
>
> Is it worth digging through the interactions here to get dma-buf fences to
> work correctly during a lockup, or would it be better to just keep
> radeon_fence_wait_timeout?  If option 1 is preferred, I'm happy to learn but I
> need some help learning how it is supposed to work.

In this case feel free to add the radeon_fence_wait_timeout. It's 
probably a good idea to get rid of the exclusive lock sooner or later in 
radeon as well, but that really is a long term project.

If you're really bored I can give you tons of input where to start with 
that.

Regards,
Christian.

>
>> Apart from that the idea looks really good to me.
>>
>> Regards,
>> Christian.
>>
> Thanks,
>
> Matthew
>
>>> ---
>>>
>>>drivers/gpu/drm/radeon/cik_sdma.c |  3 ++-
>>>drivers/gpu/drm/radeon/r100.c |  3 ++-
>>>drivers/gpu/drm/radeon/r600.c |  3 ++-
>>>drivers/gpu/drm/radeon/r600_dma.c |  3 ++-
>>>drivers/gpu/drm/radeon/radeon.h   |  1 +
>>>drivers/gpu/drm/radeon/radeon_fence.c | 25 ++---
>>>drivers/gpu/drm/radeon/radeon_vce.c   |  3 ++-
>>>drivers/gpu/drm/radeon/uvd_v1_0.c |  3 ++-
>>>8 files changed, 35 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/cik_sdma.c
>>> b/drivers/gpu/drm/radeon/cik_sdma.c index d16f2ee..014f5ca 100644
>>> --- a/drivers/gpu/drm/radeon/cik_sdma.c
>>> +++ b/drivers/gpu/drm/radeon/cik_sdma.c
>>> @@ -737,7 +737,8 @@ int cik_sdma_ib_test(struct radeon_device *rdev,
>>> struct radeon_ring *ring)>
>>> DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
>>> return r;
>>> 
>>> }
>>>
>>> -   r = radeon_fence_wait(ib.fence, false);
>>> +   r = radeon_fence_wait_timeout(ib.fence, false, msecs_to_jiffies(
>>> +   (radeon_lockup_timeout) ? radeon_lockup_timeout :
>>> MAX_SCHEDULE_TIMEOUT));>
>>> if (r) {
>>> 
>>> DRM_ERROR("radeon: fence wait failed (%d).\n", r);
>>> return r;
>>>
>>> diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
>>> index 5eae0a8..5386c09 100644
>>> --- a/drivers/gpu/drm/radeon/r100.c
>>> +++ b/drivers/gpu/drm/radeon/r100.c
>>> @@ -3732,7 +3732,8 @@ int r100_ib_test(struct radeon_device *rdev, struct
>>> radeon_ring *ring)>
>>> DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
>>> goto free_ib;
>>> 
>>> }
>>>
>>> -   r = radeon_fence_wait(ib.fence, false);
>>> +   r = radeon_fence_wait_timeou

[PATCH] drm/radeon: Avoid double gpu reset by adding a timeout on IB ring tests.

[Matthew Dawson]

On Sunday, January 24, 2016 10:49:00 AM EST Christian König wrote:
> Am 24.01.2016 um 07:18 schrieb Matthew Dawson:
> > When the radeon driver resets a gpu, it attempts to test whether all the
> > rings can successfully handle an IB.  If these rings fail to respond, the
> > process will wait forever.  Another gpu reset can't happen at this point,
> > as the current reset holds a lock required to do so.  Instead, make all
> > the IB tests run with a timeout, so the system can attempt to recover
> > in this case.
> > 
> > While this doesn't fix the underlying issue with card resets failing, it
> > gives the system a higher chance of recovering.  These timeouts have been
> > confirmed to help both a Tathi and Hawaii card recover after a gpu reset.
> > 
> > I haven't been able to test this on other cards, but everything compiles.
> > I wasn't sure what to use for a timeout value, so for now I've used
> > radeon_lockup_timeout.  When that is 0, the timeout functionality in this
> > patch is also disabled.
> > 
> > This also adds a new function, radeon_fence_wait_timeout, that behaves
> > like
> > radeon_fence_wait except allowing a different timeout value to be passed
> > to
> > radeon_fence_wait_seq_timeout.
> > 
> > Signed-off-by: Matthew Dawson 
> 
> Really good idea, but please don't use radeon_lockup_timeout for this
> cause the 10 seconds default of that is way to long. Instead just use a
> fixed, let's say 100ms timeout defined in radeon.h.
I originally tried 100ms, but I found that to be too short (my system would 
just lockup completely (no network even)).  I found 1s is long enough, and 
isn't so long to be annoying.  Would that be ok?

> 
> Also don't define our own radeon_fence_wait_timeout, just use
> fence_wait_timeout(&fence->base, RADEON_IB_TEST_TIMEOUT) for this.
I switched everything over to this, however the ib test always times out after 
a gpu reset (on startup, everything is normal).  I'm not sure why 
fence_wait_timeout isn't being signaled while radeon_fence_wait_seq_timeout 
is.  I'm suspicious that either radeon_fence_enable_signaling is not doing 
something necessary to get the fence completion to actual signal 
radeon_fence_default_wait, or because we hold the exclusive lock during a 
reset radeon_fence_check_lockup never runs and thus never triggers the fence 
or enables some irq lines.

Is it worth digging through the interactions here to get dma-buf fences to 
work correctly during a lockup, or would it be better to just keep 
radeon_fence_wait_timeout?  If option 1 is preferred, I'm happy to learn but I 
need some help learning how it is supposed to work.

> 
> Apart from that the idea looks really good to me.
> 
> Regards,
> Christian.
> 
Thanks,

Matthew

> > ---
> > 
> >   drivers/gpu/drm/radeon/cik_sdma.c |  3 ++-
> >   drivers/gpu/drm/radeon/r100.c |  3 ++-
> >   drivers/gpu/drm/radeon/r600.c |  3 ++-
> >   drivers/gpu/drm/radeon/r600_dma.c |  3 ++-
> >   drivers/gpu/drm/radeon/radeon.h   |  1 +
> >   drivers/gpu/drm/radeon/radeon_fence.c | 25 ++---
> >   drivers/gpu/drm/radeon/radeon_vce.c   |  3 ++-
> >   drivers/gpu/drm/radeon/uvd_v1_0.c |  3 ++-
> >   8 files changed, 35 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/radeon/cik_sdma.c
> > b/drivers/gpu/drm/radeon/cik_sdma.c index d16f2ee..014f5ca 100644
> > --- a/drivers/gpu/drm/radeon/cik_sdma.c
> > +++ b/drivers/gpu/drm/radeon/cik_sdma.c
> > @@ -737,7 +737,8 @@ int cik_sdma_ib_test(struct radeon_device *rdev,
> > struct radeon_ring *ring)> 
> > DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
> > return r;
> > 
> > }
> > 
> > -   r = radeon_fence_wait(ib.fence, false);
> > +   r = radeon_fence_wait_timeout(ib.fence, false, msecs_to_jiffies(
> > +   (radeon_lockup_timeout) ? radeon_lockup_timeout :
> > MAX_SCHEDULE_TIMEOUT));> 
> > if (r) {
> > 
> > DRM_ERROR("radeon: fence wait failed (%d).\n", r);
> > return r;
> > 
> > diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
> > index 5eae0a8..5386c09 100644
> > --- a/drivers/gpu/drm/radeon/r100.c
> > +++ b/drivers/gpu/drm/radeon/r100.c
> > @@ -3732,7 +3732,8 @@ int r100_ib_test(struct radeon_device *rdev, struct
> > radeon_ring *ring)> 
> > DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
> > goto free_ib;
> > 
> > }
> > 
> > -   r = radeon_fence_wait(ib.fence, false);
> > +   r = radeon_fence_wait_timeout(ib.fence, false, msecs_to_jiffies(
> > +   (radeon_lockup_timeout) ? radeon_lockup_timeout :
> > MAX_SCHEDULE_TIMEOUT));> 
> > if (r) {
> > 
> > DRM_ERROR("radeon: fence wait failed (%d).\n", r);
> > goto free_ib;
> > 
> > diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> > index cc2fdf0..8fbe5d7 100644
> > --- a/drivers/gpu/drm/radeon/r600.c
> > +++ b/drivers/gpu/drm/radeon/r600.c
> > @@ -3381

[PATCH] drm/radeon: Avoid double gpu reset by adding a timeout on IB ring tests.

[Christian König]

Am 24.01.2016 um 07:18 schrieb Matthew Dawson:
> When the radeon driver resets a gpu, it attempts to test whether all the
> rings can successfully handle an IB.  If these rings fail to respond, the
> process will wait forever.  Another gpu reset can't happen at this point,
> as the current reset holds a lock required to do so.  Instead, make all
> the IB tests run with a timeout, so the system can attempt to recover
> in this case.
>
> While this doesn't fix the underlying issue with card resets failing, it
> gives the system a higher chance of recovering.  These timeouts have been
> confirmed to help both a Tathi and Hawaii card recover after a gpu reset.
>
> I haven't been able to test this on other cards, but everything compiles.
> I wasn't sure what to use for a timeout value, so for now I've used
> radeon_lockup_timeout.  When that is 0, the timeout functionality in this
> patch is also disabled.
>
> This also adds a new function, radeon_fence_wait_timeout, that behaves like
> radeon_fence_wait except allowing a different timeout value to be passed to
> radeon_fence_wait_seq_timeout.
>
> Signed-off-by: Matthew Dawson 

Really good idea, but please don't use radeon_lockup_timeout for this 
cause the 10 seconds default of that is way to long. Instead just use a 
fixed, let's say 100ms timeout defined in radeon.h.

Also don't define our own radeon_fence_wait_timeout, just use 
fence_wait_timeout(&fence->base, RADEON_IB_TEST_TIMEOUT) for this.

Apart from that the idea looks really good to me.

Regards,
Christian.

> ---
>   drivers/gpu/drm/radeon/cik_sdma.c |  3 ++-
>   drivers/gpu/drm/radeon/r100.c |  3 ++-
>   drivers/gpu/drm/radeon/r600.c |  3 ++-
>   drivers/gpu/drm/radeon/r600_dma.c |  3 ++-
>   drivers/gpu/drm/radeon/radeon.h   |  1 +
>   drivers/gpu/drm/radeon/radeon_fence.c | 25 ++---
>   drivers/gpu/drm/radeon/radeon_vce.c   |  3 ++-
>   drivers/gpu/drm/radeon/uvd_v1_0.c |  3 ++-
>   8 files changed, 35 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/cik_sdma.c 
> b/drivers/gpu/drm/radeon/cik_sdma.c
> index d16f2ee..014f5ca 100644
> --- a/drivers/gpu/drm/radeon/cik_sdma.c
> +++ b/drivers/gpu/drm/radeon/cik_sdma.c
> @@ -737,7 +737,8 @@ int cik_sdma_ib_test(struct radeon_device *rdev, struct 
> radeon_ring *ring)
>   DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
>   return r;
>   }
> - r = radeon_fence_wait(ib.fence, false);
> + r = radeon_fence_wait_timeout(ib.fence, false, msecs_to_jiffies(
> + (radeon_lockup_timeout) ? radeon_lockup_timeout : 
> MAX_SCHEDULE_TIMEOUT));
>   if (r) {
>   DRM_ERROR("radeon: fence wait failed (%d).\n", r);
>   return r;
> diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
> index 5eae0a8..5386c09 100644
> --- a/drivers/gpu/drm/radeon/r100.c
> +++ b/drivers/gpu/drm/radeon/r100.c
> @@ -3732,7 +3732,8 @@ int r100_ib_test(struct radeon_device *rdev, struct 
> radeon_ring *ring)
>   DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
>   goto free_ib;
>   }
> - r = radeon_fence_wait(ib.fence, false);
> + r = radeon_fence_wait_timeout(ib.fence, false, msecs_to_jiffies(
> + (radeon_lockup_timeout) ? radeon_lockup_timeout : 
> MAX_SCHEDULE_TIMEOUT));
>   if (r) {
>   DRM_ERROR("radeon: fence wait failed (%d).\n", r);
>   goto free_ib;
> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
> index cc2fdf0..8fbe5d7 100644
> --- a/drivers/gpu/drm/radeon/r600.c
> +++ b/drivers/gpu/drm/radeon/r600.c
> @@ -3381,7 +3381,8 @@ int r600_ib_test(struct radeon_device *rdev, struct 
> radeon_ring *ring)
>   DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
>   goto free_ib;
>   }
> - r = radeon_fence_wait(ib.fence, false);
> + r = radeon_fence_wait_timeout(ib.fence, false, msecs_to_jiffies(
> + (radeon_lockup_timeout) ? radeon_lockup_timeout : 
> MAX_SCHEDULE_TIMEOUT));
>   if (r) {
>   DRM_ERROR("radeon: fence wait failed (%d).\n", r);
>   goto free_ib;
> diff --git a/drivers/gpu/drm/radeon/r600_dma.c 
> b/drivers/gpu/drm/radeon/r600_dma.c
> index d2dd29a..3ff614d 100644
> --- a/drivers/gpu/drm/radeon/r600_dma.c
> +++ b/drivers/gpu/drm/radeon/r600_dma.c
> @@ -368,7 +368,8 @@ int r600_dma_ib_test(struct radeon_device *rdev, struct 
> radeon_ring *ring)
>   DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
>   return r;
>   }
> - r = radeon_fence_wait(ib.fence, false);
> + r = radeon_fence_wait_timeout(ib.fence, false, msecs_to_jiffies(
> + (radeon_lockup_timeout) ? radeon_lockup_timeout : 
> MAX_SCHEDULE_TIMEOUT));
>   if (r) {
>   DRM_ERROR("radeon: fence wait failed (%d).\n", r);
>   return r;
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/driv

[PATCH] drm/radeon: Avoid double gpu reset by adding a timeout on IB ring tests.

[Matthew Dawson]

When the radeon driver resets a gpu, it attempts to test whether all the
rings can successfully handle an IB.  If these rings fail to respond, the
process will wait forever.  Another gpu reset can't happen at this point,
as the current reset holds a lock required to do so.  Instead, make all
the IB tests run with a timeout, so the system can attempt to recover
in this case.

While this doesn't fix the underlying issue with card resets failing, it
gives the system a higher chance of recovering.  These timeouts have been
confirmed to help both a Tathi and Hawaii card recover after a gpu reset.

I haven't been able to test this on other cards, but everything compiles.
I wasn't sure what to use for a timeout value, so for now I've used
radeon_lockup_timeout.  When that is 0, the timeout functionality in this
patch is also disabled.

This also adds a new function, radeon_fence_wait_timeout, that behaves like
radeon_fence_wait except allowing a different timeout value to be passed to
radeon_fence_wait_seq_timeout.

Signed-off-by: Matthew Dawson 
---
 drivers/gpu/drm/radeon/cik_sdma.c |  3 ++-
 drivers/gpu/drm/radeon/r100.c |  3 ++-
 drivers/gpu/drm/radeon/r600.c |  3 ++-
 drivers/gpu/drm/radeon/r600_dma.c |  3 ++-
 drivers/gpu/drm/radeon/radeon.h   |  1 +
 drivers/gpu/drm/radeon/radeon_fence.c | 25 ++---
 drivers/gpu/drm/radeon/radeon_vce.c   |  3 ++-
 drivers/gpu/drm/radeon/uvd_v1_0.c |  3 ++-
 8 files changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/radeon/cik_sdma.c 
b/drivers/gpu/drm/radeon/cik_sdma.c
index d16f2ee..014f5ca 100644
--- a/drivers/gpu/drm/radeon/cik_sdma.c
+++ b/drivers/gpu/drm/radeon/cik_sdma.c
@@ -737,7 +737,8 @@ int cik_sdma_ib_test(struct radeon_device *rdev, struct 
radeon_ring *ring)
DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
return r;
}
-   r = radeon_fence_wait(ib.fence, false);
+   r = radeon_fence_wait_timeout(ib.fence, false, msecs_to_jiffies(
+   (radeon_lockup_timeout) ? radeon_lockup_timeout : 
MAX_SCHEDULE_TIMEOUT));
if (r) {
DRM_ERROR("radeon: fence wait failed (%d).\n", r);
return r;
diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
index 5eae0a8..5386c09 100644
--- a/drivers/gpu/drm/radeon/r100.c
+++ b/drivers/gpu/drm/radeon/r100.c
@@ -3732,7 +3732,8 @@ int r100_ib_test(struct radeon_device *rdev, struct 
radeon_ring *ring)
DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
goto free_ib;
}
-   r = radeon_fence_wait(ib.fence, false);
+   r = radeon_fence_wait_timeout(ib.fence, false, msecs_to_jiffies(
+   (radeon_lockup_timeout) ? radeon_lockup_timeout : 
MAX_SCHEDULE_TIMEOUT));
if (r) {
DRM_ERROR("radeon: fence wait failed (%d).\n", r);
goto free_ib;
diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c
index cc2fdf0..8fbe5d7 100644
--- a/drivers/gpu/drm/radeon/r600.c
+++ b/drivers/gpu/drm/radeon/r600.c
@@ -3381,7 +3381,8 @@ int r600_ib_test(struct radeon_device *rdev, struct 
radeon_ring *ring)
DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
goto free_ib;
}
-   r = radeon_fence_wait(ib.fence, false);
+   r = radeon_fence_wait_timeout(ib.fence, false, msecs_to_jiffies(
+   (radeon_lockup_timeout) ? radeon_lockup_timeout : 
MAX_SCHEDULE_TIMEOUT));
if (r) {
DRM_ERROR("radeon: fence wait failed (%d).\n", r);
goto free_ib;
diff --git a/drivers/gpu/drm/radeon/r600_dma.c 
b/drivers/gpu/drm/radeon/r600_dma.c
index d2dd29a..3ff614d 100644
--- a/drivers/gpu/drm/radeon/r600_dma.c
+++ b/drivers/gpu/drm/radeon/r600_dma.c
@@ -368,7 +368,8 @@ int r600_dma_ib_test(struct radeon_device *rdev, struct 
radeon_ring *ring)
DRM_ERROR("radeon: failed to schedule ib (%d).\n", r);
return r;
}
-   r = radeon_fence_wait(ib.fence, false);
+   r = radeon_fence_wait_timeout(ib.fence, false, msecs_to_jiffies(
+   (radeon_lockup_timeout) ? radeon_lockup_timeout : 
MAX_SCHEDULE_TIMEOUT));
if (r) {
DRM_ERROR("radeon: fence wait failed (%d).\n", r);
return r;
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 5ae6db9..0b698b6 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -382,6 +382,7 @@ void radeon_fence_driver_force_completion(struct 
radeon_device *rdev, int ring);
 int radeon_fence_emit(struct radeon_device *rdev, struct radeon_fence **fence, 
int ring);
 void radeon_fence_process(struct radeon_device *rdev, int ring);
 bool radeon_fence_signaled(struct radeon_fence *fence);
+int radeon_fence_wait_timeout(struct radeon_fence *fence, bool interruptible, 
long timeout);
 int radeon_fence_wait(struct ra