All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Christian König" <deathsimple@vodafone.de>
To: Matthew Dawson <matthew@mjdsystems.ca>, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/radeon: Avoid double gpu reset by adding a timeout on IB ring tests.
Date: Sun, 24 Jan 2016 10:49:00 +0100	[thread overview]
Message-ID: <56A49E0C.1060105@vodafone.de> (raw)
In-Reply-To: <1453616332-30763-1-git-send-email-matthew@mjdsystems.ca>

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 <matthew@mjdsystems.ca>

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/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 radeon_fence *fence, bool interruptible);
>   int radeon_fence_wait_next(struct radeon_device *rdev, int ring);
>   int radeon_fence_wait_empty(struct radeon_device *rdev, int ring);
> diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c
> index 05815c4..9fec805 100644
> --- a/drivers/gpu/drm/radeon/radeon_fence.c
> +++ b/drivers/gpu/drm/radeon/radeon_fence.c
> @@ -527,7 +527,7 @@ static long radeon_fence_wait_seq_timeout(struct radeon_device *rdev,
>   }
>   
>   /**
> - * radeon_fence_wait - wait for a fence to signal
> + * radeon_fence_wait_timeout - wait for a fence to signal with timeout
>    *
>    * @fence: radeon fence object
>    * @intr: use interruptible sleep
> @@ -535,9 +535,10 @@ static long radeon_fence_wait_seq_timeout(struct radeon_device *rdev,
>    * Wait for the requested fence to signal (all asics).
>    * @intr selects whether to use interruptable (true) or non-interruptable
>    * (false) sleep when waiting for the fence.
> + * @timeout: maximum time to wait, or MAX_SCHEDULE_TIMEOUT for infinite wait
>    * Returns 0 if the fence has passed, error for all other cases.
>    */
> -int radeon_fence_wait(struct radeon_fence *fence, bool intr)
> +int radeon_fence_wait_timeout(struct radeon_fence *fence, bool intr, long timeout)
>   {
>   	uint64_t seq[RADEON_NUM_RINGS] = {};
>   	long r;
> @@ -552,9 +553,11 @@ int radeon_fence_wait(struct radeon_fence *fence, bool intr)
>   		return fence_wait(&fence->base, intr);
>   
>   	seq[fence->ring] = fence->seq;
> -	r = radeon_fence_wait_seq_timeout(fence->rdev, seq, intr, MAX_SCHEDULE_TIMEOUT);
> +	r = radeon_fence_wait_seq_timeout(fence->rdev, seq, intr, timeout);
>   	if (r < 0) {
>   		return r;
> +	} else if (r == 0) {
> +		return -ETIMEDOUT;
>   	}
>   
>   	r = fence_signal(&fence->base);
> @@ -564,6 +567,22 @@ int radeon_fence_wait(struct radeon_fence *fence, bool intr)
>   }
>   
>   /**
> + * radeon_fence_wait - wait for a fence to signal
> + *
> + * @fence: radeon fence object
> + * @intr: use interruptible sleep
> + *
> + * Wait for the requested fence to signal (all asics).
> + * @intr selects whether to use interruptable (true) or non-interruptable
> + * (false) sleep when waiting for the fence.
> + * Returns 0 if the fence has passed, error for all other cases.
> + */
> +int radeon_fence_wait(struct radeon_fence *fence, bool intr)
> +{
> +	return radeon_fence_wait_timeout(fence, intr, MAX_SCHEDULE_TIMEOUT);
> +}
> +
> +/**
>    * radeon_fence_wait_any - wait for a fence to signal on any ring
>    *
>    * @rdev: radeon device pointer
> diff --git a/drivers/gpu/drm/radeon/radeon_vce.c b/drivers/gpu/drm/radeon/radeon_vce.c
> index 7eb1ae7..7d80dad 100644
> --- a/drivers/gpu/drm/radeon/radeon_vce.c
> +++ b/drivers/gpu/drm/radeon/radeon_vce.c
> @@ -810,7 +810,8 @@ int radeon_vce_ib_test(struct radeon_device *rdev, struct radeon_ring *ring)
>   		goto error;
>   	}
>   
> -	r = radeon_fence_wait(fence, false);
> +	r = radeon_fence_wait_timeout(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);
>   	} else {
> diff --git a/drivers/gpu/drm/radeon/uvd_v1_0.c b/drivers/gpu/drm/radeon/uvd_v1_0.c
> index c6b1cbc..35caa89 100644
> --- a/drivers/gpu/drm/radeon/uvd_v1_0.c
> +++ b/drivers/gpu/drm/radeon/uvd_v1_0.c
> @@ -522,7 +522,8 @@ int uvd_v1_0_ib_test(struct radeon_device *rdev, struct radeon_ring *ring)
>   		goto error;
>   	}
>   
> -	r = radeon_fence_wait(fence, false);
> +	r = radeon_fence_wait_timeout(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 error;

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2016-01-24  9:49 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-24  6:18 [PATCH] drm/radeon: Avoid double gpu reset by adding a timeout on IB ring tests Matthew Dawson
2016-01-24  9:49 ` Christian König [this message]
2016-01-31 18:50   ` Matthew Dawson
2016-02-03 12:24     ` Christian König
2016-02-08 14:51       ` Matthew Dawson
2016-02-08 16:55         ` Bruno Kant
  -- strict thread matches above, loose matches on Subject: below --
2016-02-07 21:51 Matthew Dawson
2016-02-08  9:38 ` Christian König
2016-02-08 15:42   ` Alex Deucher

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56A49E0C.1060105@vodafone.de \
    --to=deathsimple@vodafone.de \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=matthew@mjdsystems.ca \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.