Attached series is the first 2 patches we already discussed about ring
mirror list handling racing with all your comments fixed (still not
committed). The third patch is a prototype based on the first 2 patches
and on our discussion.
Please take a look.
Andrey
On 01/18/2019 01:32 PM, Koenig, Christian wrote:
> Am 18.01.19 um 18:34 schrieb Grodzovsky, Andrey:
>> On 01/18/2019 12:10 PM, Koenig, Christian wrote:
>>> Am 18.01.19 um 16:21 schrieb Grodzovsky, Andrey:
>>>> On 01/18/2019 04:25 AM, Koenig, Christian wrote:
>>>>> [SNIP]
>>>>>>>>> Re-arming the timeout should probably have a much reduced value
>>>>>>>>> when the job hasn't changed. E.g. something like a few ms.
>>>>>> Now i got thinking about non hanged job in progress (job A) and let's
>>>>>> say it's a long job , it just started executing but due to time out of
>>>>>> another job (job B) on another (or this scheduler) it's parent cb got
>>>>>> disconnected, we disarmed the tdr timer for the job's scheduler,
>>>>>> meanwhile the timed out job did manage to complete before HW reset
>>>>>> check and hence we skip HW reset, attach back the cb and rearm job's A
>>>>>> tdr timer with a future value of few ms only - aren't we going to get
>>>>>> false tdr triggered on job B now because we didn't let it enough time
>>>>>> to run and complete ? I would prefer the other extreme of longer time
>>>>>> for time out to trigger then false TDR. Optimally we would have per
>>>>>> job timer and rearm to exactly the reminder of it's time out value -
>>>>>> but we gave up on per job tdr work long ago.
>>>>> Well we only re-arm the timeout with a shorter period if it already
>>>>> triggered once. If we just suspend the timeout then we should still use
>>>>> the longer period.
>>>> Can you explain more on this ? I don't get it.
>>> See drm_sched_job_timedout(), we re-arm the timeout at the end of the
>>> procedure.
>>>
>>> We should change that and re-arm the timer with a much lower timeout if
>>> the job is still not finished.
>>>
>>> Christian.
>> I still don't see how this can fix the problem of of long job in
>> progress triggering false tdr if no HW reset was done, but maybe I am
>> missing other pieces you have in mind, I will finish the patch and send
>> it and then we can be more specific based on the code.
> Ok sounds good. We should probably discuss less on details and prototype
> a bit more.
>
> Might be that I'm missing something here as well, so probably good to
> have some code to talk about things more directly.
>
> Christian.
>
>> Andrey
>>
>>>> Andrey
>>>>
>>>>>> In general the more i think about it (correct me if I am wrong) I am
>>>>>> less sure how much the optimization feature is useful - if job's time
>>>>>> out did trigger what are the chances that the little more time we give
>>>>>> it between beginning of tdr function and the time we do start the
>>>>>> actual HW reset will be exactly what it needed to complete. Also, this
>>>>>> is still not water proof as the job might complete and signal it's HW
>>>>>> fence exactly after we checked for completion but before starting the
>>>>>> HW reset code.
>>>>> I don't see this as an optimization, but rather as mandatory for correct
>>>>> operation.
>>>>>
>>>>> See without this we can run into issues because we execute jobs multiple
>>>>> times. That can still happen with this clean handling, but it is much
>>>>> more unlikely.
>>>>>
>>>>> Christian.
>>>>>
>>>>>> Andrey
>>>>>>
>>>>>>>> By unchanged you mean when we didn't resubmit the job because of the
>>>>>>>> optimized non HW reset, right ?
>>>>>>> Correct, yes.
>>>>>>>
>>>>>>>>>> About flushing tdr jobs in progress from .free_job cb - looks like
>>>>>>>>>> drm_sched_job_finish->cancel_delayed_work_sync is not enough, we
>>>>>>>>>> still need to take care of flushing all sced->work_tdr for a
>>>>>>>>>> device and for all devices in hive for XGMI.
>>>>>>>>>> What do you think ?
>>>>>>>>> Why should that be necessary? We only wait for the delayed work to
>>>>>>>>> make sure that the job is not destroyed while dealing with it.
>>>>>>>>>
>>>>>>>>> Christian.
>>>>>>>> But we might not be waiting for the correct sched->work_tdr, we do
>>>>>>>> the reset routine for all schedulers in a device accessing their
>>>>>>>> jobs too and not only for the scheduler to which the job belongs.
>>>>>>>> For XGMI not only that, we reset all the devices in the hive.
>>>>>>> That is harmless you only need to wait for the work_tdr of the
>>>>>>> current scheduler, not for all of them.
>>>>>>>
>>>>>>>> I was thinking, amdgpu driver is not even interested in allowing
>>>>>>>> multiple sced->tdr to execute together - we have to serialize all of
>>>>>>>> them anyway with the trylock mutex (even without XGMI), v3d in
>>>>>>>> v3d_job_timedout seems also to reset all of his schedulers from the
>>>>>>>> tdr work. Would it make sense to provide the sched->work_td as init
>>>>>>>> parameter to scheduler (same one for all schedulers) so we can
>>>>>>>> enforce serialization by disallowing more then 1 tdr work to execute
>>>>>>>> in the same time ? Other drivers interested to do in parallel can
>>>>>>>> provide unique sched->work_tdr per scheduler. This does imply
>>>>>>>> drm_sched_job_timedout has to removed and delegated to specific
>>>>>>>> driver implementation as probably other code dealing with
>>>>>>>> sched->work_tdr... Maybe even move tdr handling to the driver all
>>>>>>>> together ?
>>>>>>> Yeah, I was thinking something similar. The problem with this
>>>>>>> approach is that a delayed work item can have only one delay, but for
>>>>>>> multiple engines we need multiple delays.
>>>>>>>
>>>>>>> What we could do is to make it a timer instead and raise the work
>>>>>>> item from the device specific callback.
>>>>>>>
>>>>>>> But that doesn't really saves us the stop all schedulers trouble, so
>>>>>>> it doesn't buy us much in the end if I see this correctly.
>>>>>>>
>>>>>>> Christian.
>>>>> _______________________________________________
>>>>> amd-gfx mailing list
>>>>> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
From 29bbbb8a45388e2e8c81056ae2284e84234a6990 Mon Sep 17 00:00:00 2001
From: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
Date: Tue, 4 Dec 2018 16:56:14 -0500
Subject: drm/sched: Refactor ring mirror list handling.
Decauple sched threads stop and start and ring mirror
list handling from the policy of what to do about the
guilty jobs.
When stoppping the sched thread and detaching sched fences
from non signaled HW fenes wait for all signaled HW fences
to complete before rerunning the jobs.
v2: Fix resubmission of guilty job into HW after refactoring.
v4:
Full restart for all the jobs, not only from guilty ring.
Extract karma increase into standalone function.
v5:
Rework waiting for signaled jobs without relying on the job
struct itself as those might already be freed for non 'guilty'
job's schedulers.
Expose karma increase to drivers.
v6:
Use list_for_each_entry_safe_continue and drm_sched_process_job
in case fence already signaled.
Call drm_sched_increase_karma only once for amdgpu and add documentation.
v7:
Wait only for the latest job's fence.
Suggested-by: Christian Koenig <Christian.Koenig@xxxxxxx>
Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 20 ++--
drivers/gpu/drm/etnaviv/etnaviv_sched.c | 11 +-
drivers/gpu/drm/scheduler/sched_main.c | 172 ++++++++++++++++++-----------
drivers/gpu/drm/v3d/v3d_sched.c | 12 +-
include/drm/gpu_scheduler.h | 8 +-
5 files changed, 134 insertions(+), 89 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 8a61764..e2f578a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3313,17 +3313,15 @@ static int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- kthread_park(ring->sched.thread);
-
- if (job && job->base.sched != &ring->sched)
- continue;
-
- drm_sched_hw_job_reset(&ring->sched, job ? &job->base : NULL);
+ drm_sched_stop(&ring->sched, job ? &job->base : NULL);
/* after all hw jobs are reset, hw fence is meaningless, so force_completion */
amdgpu_fence_driver_force_completion(ring);
}
+ if(job)
+ drm_sched_increase_karma(&job->base);
+
if (!amdgpu_sriov_vf(adev)) {
@@ -3469,14 +3467,10 @@ static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- /* only need recovery sched of the given job's ring
- * or all rings (in the case @job is NULL)
- * after above amdgpu_reset accomplished
- */
- if ((!job || job->base.sched == &ring->sched) && !adev->asic_reset_res)
- drm_sched_job_recovery(&ring->sched);
+ if (!adev->asic_reset_res)
+ drm_sched_resubmit_jobs(&ring->sched);
- kthread_unpark(ring->sched.thread);
+ drm_sched_start(&ring->sched, !adev->asic_reset_res);
}
if (!amdgpu_device_has_dc_support(adev)) {
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
index 49a6763..6f1268f 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
@@ -109,16 +109,19 @@ static void etnaviv_sched_timedout_job(struct drm_sched_job *sched_job)
}
/* block scheduler */
- kthread_park(gpu->sched.thread);
- drm_sched_hw_job_reset(&gpu->sched, sched_job);
+ drm_sched_stop(&gpu->sched, sched_job);
+
+ if(sched_job)
+ drm_sched_increase_karma(sched_job);
/* get the GPU back into the init state */
etnaviv_core_dump(gpu);
etnaviv_gpu_recover_hang(gpu);
+ drm_sched_resubmit_jobs(&gpu->sched);
+
/* restart scheduler after GPU is usable again */
- drm_sched_job_recovery(&gpu->sched);
- kthread_unpark(gpu->sched.thread);
+ drm_sched_start(&gpu->sched, true);
}
static void etnaviv_sched_free_job(struct drm_sched_job *sched_job)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index dbb6906..e758fc6 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -60,8 +60,6 @@
static void drm_sched_process_job(struct dma_fence *f, struct dma_fence_cb *cb);
-static void drm_sched_expel_job_unlocked(struct drm_sched_job *s_job);
-
/**
* drm_sched_rq_init - initialize a given run queue struct
*
@@ -335,6 +333,51 @@ static void drm_sched_job_timedout(struct work_struct *work)
spin_unlock_irqrestore(&sched->job_list_lock, flags);
}
+ /**
+ * drm_sched_increase_karma - Update sched_entity guilty flag
+ *
+ * @bad: The job guilty of time out
+ *
+ * Increment on every hang caused by the 'bad' job. If this exceeds the hang
+ * limit of the scheduler then the respective sched entity is marked guilty and
+ * jobs from it will not be scheduled further
+ */
+void drm_sched_increase_karma(struct drm_sched_job *bad)
+{
+ int i;
+ struct drm_sched_entity *tmp;
+ struct drm_sched_entity *entity;
+ struct drm_gpu_scheduler *sched = bad->sched;
+
+ /* don't increase @bad's karma if it's from KERNEL RQ,
+ * because sometimes GPU hang would cause kernel jobs (like VM updating jobs)
+ * corrupt but keep in mind that kernel jobs always considered good.
+ */
+ if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
+ atomic_inc(&bad->karma);
+ for (i = DRM_SCHED_PRIORITY_MIN; i < DRM_SCHED_PRIORITY_KERNEL;
+ i++) {
+ struct drm_sched_rq *rq = &sched->sched_rq[i];
+
+ spin_lock(&rq->lock);
+ list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
+ if (bad->s_fence->scheduled.context ==
+ entity->fence_context) {
+ if (atomic_read(&bad->karma) >
+ bad->sched->hang_limit)
+ if (entity->guilty)
+ atomic_set(entity->guilty, 1);
+ break;
+ }
+ }
+ spin_unlock(&rq->lock);
+ if (&entity->list != &rq->entities)
+ break;
+ }
+ }
+}
+EXPORT_SYMBOL(drm_sched_increase_karma);
+
/**
* drm_sched_hw_job_reset - stop the scheduler if it contains the bad job
*
@@ -342,13 +385,22 @@ static void drm_sched_job_timedout(struct work_struct *work)
* @bad: bad scheduler job
*
*/
-void drm_sched_hw_job_reset(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
+void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
{
struct drm_sched_job *s_job;
- struct drm_sched_entity *entity, *tmp;
unsigned long flags;
- int i;
+ struct dma_fence *last_fence = NULL;
+ int r;
+
+ kthread_park(sched->thread);
+ /*
+ * Verify all the signaled jobs in mirror list are removed from the ring
+ * by waiting for the latest job to enter the list. This should insure that
+ * also all the previous jobs that were in flight also already singaled
+ * and removed from the list.
+ */
+retry_wait:
spin_lock_irqsave(&sched->job_list_lock, flags);
list_for_each_entry_reverse(s_job, &sched->ring_mirror_list, node) {
if (s_job->s_fence->parent &&
@@ -357,35 +409,20 @@ void drm_sched_hw_job_reset(struct drm_gpu_scheduler *sched, struct drm_sched_jo
dma_fence_put(s_job->s_fence->parent);
s_job->s_fence->parent = NULL;
atomic_dec(&sched->hw_rq_count);
+ } else {
+ last_fence = dma_fence_get(&s_job->s_fence->finished);
+ break;
}
}
spin_unlock_irqrestore(&sched->job_list_lock, flags);
- if (bad && bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
- atomic_inc(&bad->karma);
- /* don't increase @bad's karma if it's from KERNEL RQ,
- * becuase sometimes GPU hang would cause kernel jobs (like VM updating jobs)
- * corrupt but keep in mind that kernel jobs always considered good.
- */
- for (i = DRM_SCHED_PRIORITY_MIN; i < DRM_SCHED_PRIORITY_KERNEL; i++ ) {
- struct drm_sched_rq *rq = &sched->sched_rq[i];
-
- spin_lock(&rq->lock);
- list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
- if (bad->s_fence->scheduled.context == entity->fence_context) {
- if (atomic_read(&bad->karma) > bad->sched->hang_limit)
- if (entity->guilty)
- atomic_set(entity->guilty, 1);
- break;
- }
- }
- spin_unlock(&rq->lock);
- if (&entity->list != &rq->entities)
- break;
- }
+ if (last_fence) {
+ dma_fence_wait(last_fence, false);
+ dma_fence_put(last_fence);
}
}
-EXPORT_SYMBOL(drm_sched_hw_job_reset);
+
+EXPORT_SYMBOL(drm_sched_stop);
/**
* drm_sched_job_recovery - recover jobs after a reset
@@ -393,33 +430,21 @@ EXPORT_SYMBOL(drm_sched_hw_job_reset);
* @sched: scheduler instance
*
*/
-void drm_sched_job_recovery(struct drm_gpu_scheduler *sched)
+void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
{
struct drm_sched_job *s_job, *tmp;
- bool found_guilty = false;
unsigned long flags;
int r;
+ if (!full_recovery)
+ goto unpark;
+
spin_lock_irqsave(&sched->job_list_lock, flags);
list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) {
struct drm_sched_fence *s_fence = s_job->s_fence;
- struct dma_fence *fence;
- uint64_t guilty_context;
-
- if (!found_guilty && atomic_read(&s_job->karma) > sched->hang_limit) {
- found_guilty = true;
- guilty_context = s_job->s_fence->scheduled.context;
- }
-
- if (found_guilty && s_job->s_fence->scheduled.context == guilty_context)
- dma_fence_set_error(&s_fence->finished, -ECANCELED);
-
- spin_unlock_irqrestore(&sched->job_list_lock, flags);
- fence = sched->ops->run_job(s_job);
- atomic_inc(&sched->hw_rq_count);
+ struct dma_fence *fence = s_job->s_fence->parent;
if (fence) {
- s_fence->parent = dma_fence_get(fence);
r = dma_fence_add_callback(fence, &s_fence->cb,
drm_sched_process_job);
if (r == -ENOENT)
@@ -427,18 +452,47 @@ void drm_sched_job_recovery(struct drm_gpu_scheduler *sched)
else if (r)
DRM_ERROR("fence add callback failed (%d)\n",
r);
- dma_fence_put(fence);
- } else {
- if (s_fence->finished.error < 0)
- drm_sched_expel_job_unlocked(s_job);
+ } else
drm_sched_process_job(NULL, &s_fence->cb);
- }
- spin_lock_irqsave(&sched->job_list_lock, flags);
}
+
drm_sched_start_timeout(sched);
spin_unlock_irqrestore(&sched->job_list_lock, flags);
+
+unpark:
+ kthread_unpark(sched->thread);
}
-EXPORT_SYMBOL(drm_sched_job_recovery);
+EXPORT_SYMBOL(drm_sched_start);
+
+/**
+ * drm_sched_resubmit_jobs - helper to relunch job from mirror ring list
+ *
+ * @sched: scheduler instance
+ *
+ */
+void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)
+{
+ struct drm_sched_job *s_job, *tmp;
+ uint64_t guilty_context;
+ bool found_guilty = false;
+
+ /*TODO DO we need spinlock here ? */
+ list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) {
+ struct drm_sched_fence *s_fence = s_job->s_fence;
+
+ if (!found_guilty && atomic_read(&s_job->karma) > sched->hang_limit) {
+ found_guilty = true;
+ guilty_context = s_job->s_fence->scheduled.context;
+ }
+
+ if (found_guilty && s_job->s_fence->scheduled.context == guilty_context)
+ dma_fence_set_error(&s_fence->finished, -ECANCELED);
+
+ s_job->s_fence->parent = sched->ops->run_job(s_job);
+ atomic_inc(&sched->hw_rq_count);
+ }
+}
+EXPORT_SYMBOL(drm_sched_resubmit_jobs);
/**
* drm_sched_job_init - init a scheduler job
@@ -634,26 +688,14 @@ static int drm_sched_main(void *param)
DRM_ERROR("fence add callback failed (%d)\n",
r);
dma_fence_put(fence);
- } else {
- if (s_fence->finished.error < 0)
- drm_sched_expel_job_unlocked(sched_job);
+ } else
drm_sched_process_job(NULL, &s_fence->cb);
- }
wake_up(&sched->job_scheduled);
}
return 0;
}
-static void drm_sched_expel_job_unlocked(struct drm_sched_job *s_job)
-{
- struct drm_gpu_scheduler *sched = s_job->sched;
-
- spin_lock(&sched->job_list_lock);
- list_del_init(&s_job->node);
- spin_unlock(&sched->job_list_lock);
-}
-
/**
* drm_sched_init - Init a gpu scheduler instance
*
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index 445b2ef..f76d9ed 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -178,18 +178,22 @@ v3d_job_timedout(struct drm_sched_job *sched_job)
for (q = 0; q < V3D_MAX_QUEUES; q++) {
struct drm_gpu_scheduler *sched = &v3d->queue[q].sched;
- kthread_park(sched->thread);
- drm_sched_hw_job_reset(sched, (sched_job->sched == sched ?
+ drm_sched_stop(sched, (sched_job->sched == sched ?
sched_job : NULL));
+
+ if(sched_job)
+ drm_sched_increase_karma(sched_job);
}
/* get the GPU back into the init state */
v3d_reset(v3d);
+ for (q = 0; q < V3D_MAX_QUEUES; q++)
+ drm_sched_resubmit_jobs(sched_job->sched);
+
/* Unblock schedulers and restart their jobs. */
for (q = 0; q < V3D_MAX_QUEUES; q++) {
- drm_sched_job_recovery(&v3d->queue[q].sched);
- kthread_unpark(v3d->queue[q].sched.thread);
+ drm_sched_start(&v3d->queue[q].sched, true);
}
mutex_unlock(&v3d->reset_lock);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 47e1979..4f21faf 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -298,9 +298,11 @@ int drm_sched_job_init(struct drm_sched_job *job,
void *owner);
void drm_sched_job_cleanup(struct drm_sched_job *job);
void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
-void drm_sched_hw_job_reset(struct drm_gpu_scheduler *sched,
- struct drm_sched_job *job);
-void drm_sched_job_recovery(struct drm_gpu_scheduler *sched);
+void drm_sched_stop(struct drm_gpu_scheduler *sched,
+ struct drm_sched_job *job);
+void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery);
+void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
+void drm_sched_increase_karma(struct drm_sched_job *bad);
bool drm_sched_dependency_optimized(struct dma_fence* fence,
struct drm_sched_entity *entity);
void drm_sched_fault(struct drm_gpu_scheduler *sched);
--
2.7.4
From 38ee1954555d78a7003558a68d537e6e6fd47501 Mon Sep 17 00:00:00 2001
From: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
Date: Wed, 5 Dec 2018 14:21:28 -0500
Subject: drm/sched: Rework HW fence processing.
Expedite job deletion from ring mirror list to the HW fence signal
callback instead from finish_work, together with waiting for all
such fences to signal in drm_sched_stop we garantee that
already signaled job will not be processed twice.
Remove the sched finish fence callback and just submit finish_work
directly from the HW fence callback.
v2: Fix comments.
v3: Attach hw fence cb to sched_job
v5: Rebase
Suggested-by: Christian Koenig <Christian.Koenig@xxxxxxx>
Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
---
drivers/gpu/drm/scheduler/sched_main.c | 55 +++++++++++++++++-----------------
include/drm/gpu_scheduler.h | 6 ++--
2 files changed, 29 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index e758fc6..16c6363 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -284,8 +284,6 @@ static void drm_sched_job_finish(struct work_struct *work)
cancel_delayed_work_sync(&sched->work_tdr);
spin_lock_irqsave(&sched->job_list_lock, flags);
- /* remove job from ring_mirror_list */
- list_del_init(&s_job->node);
/* queue TDR for next job */
drm_sched_start_timeout(sched);
spin_unlock_irqrestore(&sched->job_list_lock, flags);
@@ -293,22 +291,11 @@ static void drm_sched_job_finish(struct work_struct *work)
sched->ops->free_job(s_job);
}
-static void drm_sched_job_finish_cb(struct dma_fence *f,
- struct dma_fence_cb *cb)
-{
- struct drm_sched_job *job = container_of(cb, struct drm_sched_job,
- finish_cb);
- schedule_work(&job->finish_work);
-}
-
static void drm_sched_job_begin(struct drm_sched_job *s_job)
{
struct drm_gpu_scheduler *sched = s_job->sched;
unsigned long flags;
- dma_fence_add_callback(&s_job->s_fence->finished, &s_job->finish_cb,
- drm_sched_job_finish_cb);
-
spin_lock_irqsave(&sched->job_list_lock, flags);
list_add_tail(&s_job->node, &sched->ring_mirror_list);
drm_sched_start_timeout(sched);
@@ -405,7 +392,7 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
list_for_each_entry_reverse(s_job, &sched->ring_mirror_list, node) {
if (s_job->s_fence->parent &&
dma_fence_remove_callback(s_job->s_fence->parent,
- &s_job->s_fence->cb)) {
+ &s_job->cb)) {
dma_fence_put(s_job->s_fence->parent);
s_job->s_fence->parent = NULL;
atomic_dec(&sched->hw_rq_count);
@@ -433,31 +420,34 @@ EXPORT_SYMBOL(drm_sched_stop);
void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
{
struct drm_sched_job *s_job, *tmp;
- unsigned long flags;
int r;
if (!full_recovery)
goto unpark;
- spin_lock_irqsave(&sched->job_list_lock, flags);
+ /*
+ * Locking the list is not required here as the sched thread is parked
+ * so no new jobs are being pushed in to HW and in drm_sched_stop we
+ * flushed all the jobs who were still in mirror list but who already
+ * signaled and removed them self from the list. Also concurrent
+ * GPU recovers can't run in parallel.
+ */
list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) {
- struct drm_sched_fence *s_fence = s_job->s_fence;
struct dma_fence *fence = s_job->s_fence->parent;
if (fence) {
- r = dma_fence_add_callback(fence, &s_fence->cb,
+ r = dma_fence_add_callback(fence, &s_job->cb,
drm_sched_process_job);
if (r == -ENOENT)
- drm_sched_process_job(fence, &s_fence->cb);
+ drm_sched_process_job(fence, &s_job->cb);
else if (r)
DRM_ERROR("fence add callback failed (%d)\n",
r);
} else
- drm_sched_process_job(NULL, &s_fence->cb);
+ drm_sched_process_job(NULL, &s_job->cb);
}
drm_sched_start_timeout(sched);
- spin_unlock_irqrestore(&sched->job_list_lock, flags);
unpark:
kthread_unpark(sched->thread);
@@ -606,18 +596,27 @@ drm_sched_select_entity(struct drm_gpu_scheduler *sched)
*/
static void drm_sched_process_job(struct dma_fence *f, struct dma_fence_cb *cb)
{
- struct drm_sched_fence *s_fence =
- container_of(cb, struct drm_sched_fence, cb);
+ struct drm_sched_job *s_job = container_of(cb, struct drm_sched_job, cb);
+ struct drm_sched_fence *s_fence = s_job->s_fence;
struct drm_gpu_scheduler *sched = s_fence->sched;
+ unsigned long flags;
+
+ cancel_delayed_work(&sched->work_tdr);
- dma_fence_get(&s_fence->finished);
atomic_dec(&sched->hw_rq_count);
atomic_dec(&sched->num_jobs);
+
+ spin_lock_irqsave(&sched->job_list_lock, flags);
+ /* remove job from ring_mirror_list */
+ list_del_init(&s_job->node);
+ spin_unlock_irqrestore(&sched->job_list_lock, flags);
+
drm_sched_fence_finished(s_fence);
trace_drm_sched_process_job(s_fence);
- dma_fence_put(&s_fence->finished);
wake_up_interruptible(&sched->wake_up_worker);
+
+ schedule_work(&s_job->finish_work);
}
/**
@@ -680,16 +679,16 @@ static int drm_sched_main(void *param)
if (fence) {
s_fence->parent = dma_fence_get(fence);
- r = dma_fence_add_callback(fence, &s_fence->cb,
+ r = dma_fence_add_callback(fence, &sched_job->cb,
drm_sched_process_job);
if (r == -ENOENT)
- drm_sched_process_job(fence, &s_fence->cb);
+ drm_sched_process_job(fence, &sched_job->cb);
else if (r)
DRM_ERROR("fence add callback failed (%d)\n",
r);
dma_fence_put(fence);
} else
- drm_sched_process_job(NULL, &s_fence->cb);
+ drm_sched_process_job(NULL, &sched_job->cb);
wake_up(&sched->job_scheduled);
}
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 4f21faf..62c2352 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -138,10 +138,6 @@ struct drm_sched_fence {
struct dma_fence finished;
/**
- * @cb: the callback for the parent fence below.
- */
- struct dma_fence_cb cb;
- /**
* @parent: the fence returned by &drm_sched_backend_ops.run_job
* when scheduling the job on hardware. We signal the
* &drm_sched_fence.finished fence once parent is signalled.
@@ -181,6 +177,7 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
* be scheduled further.
* @s_priority: the priority of the job.
* @entity: the entity to which this job belongs.
+ * @cb: the callback for the parent fence in s_fence.
*
* A job is created by the driver using drm_sched_job_init(), and
* should call drm_sched_entity_push_job() once it wants the scheduler
@@ -197,6 +194,7 @@ struct drm_sched_job {
atomic_t karma;
enum drm_sched_priority s_priority;
struct drm_sched_entity *entity;
+ struct dma_fence_cb cb;
};
static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
--
2.7.4
From e8886a4d425b5cb915acfb7da165e42b917b7626 Mon Sep 17 00:00:00 2001
From: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
Date: Fri, 18 Jan 2019 14:06:25 -0500
Subject: drm/sched: DRAFT: drm/sched: Skip HW reset if guilty job is signaled.
Also reject other tdr jobs while current in progress as it is going
to take care of all the schedulres for a device and all devices for
XGMI hive.
Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 65 +++++++++++++++++++-----------
drivers/gpu/drm/amd/amdgpu/amdgpu_job.h | 1 +
drivers/gpu/drm/etnaviv/etnaviv_sched.c | 2 +-
drivers/gpu/drm/scheduler/sched_main.c | 47 ++++++++++++---------
drivers/gpu/drm/v3d/v3d_sched.c | 3 +-
include/drm/gpu_scheduler.h | 9 ++++-
6 files changed, 79 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index e2f578a..db2b984 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3313,7 +3313,7 @@ static int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- drm_sched_stop(&ring->sched, job ? &job->base : NULL);
+ drm_sched_stop(&ring->sched);
/* after all hw jobs are reset, hw fence is meaningless, so force_completion */
amdgpu_fence_driver_force_completion(ring);
@@ -3457,7 +3457,7 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive,
}
static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
- struct amdgpu_job *job)
+ bool resubmit)
{
int i;
@@ -3467,7 +3467,7 @@ static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
if (!ring || !ring->sched.thread)
continue;
- if (!adev->asic_reset_res)
+ if (!adev->asic_reset_res && resubmit)
drm_sched_resubmit_jobs(&ring->sched);
drm_sched_start(&ring->sched, !adev->asic_reset_res);
@@ -3480,14 +3480,22 @@ static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev,
adev->asic_reset_res = 0;
}
-static void amdgpu_device_lock_adev(struct amdgpu_device *adev)
+static bool amdgpu_device_lock_adev(struct amdgpu_device *adev, bool trylock_reset)
{
- mutex_lock(&adev->lock_reset);
+ if (trylock_reset) {
+ if (!mutex_trylock(&adev->lock_reset))
+ return false;
+ }
+ else
+ mutex_lock(&adev->lock_reset);
+
atomic_inc(&adev->gpu_reset_counter);
adev->in_gpu_reset = 1;
/* Block kfd: SRIOV would do it separately */
if (!amdgpu_sriov_vf(adev))
amdgpu_amdkfd_pre_reset(adev);
+
+ return true;
}
static void amdgpu_device_unlock_adev(struct amdgpu_device *adev)
@@ -3520,23 +3528,29 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
bool need_full_reset = false;
struct amdgpu_device *tmp_adev = NULL;
struct list_head device_list, *device_list_handle = NULL;
+ bool job_finished, xgmi_topology_present;
INIT_LIST_HEAD(&device_list);
dev_info(adev->dev, "GPU reset begin!\n");
+ hive = amdgpu_get_xgmi_hive(adev, 0);
+ xgmi_topology_present = hive && adev->gmc.xgmi.num_physical_nodes > 1;
+
/*
- * In case of XGMI hive disallow concurrent resets to be triggered
- * by different nodes. No point also since the one node already executing
- * reset will also reset all the other nodes in the hive.
+ * Here and bellow we trylock to avoid chain of resets executing from
+ * either trigger by jobs on different adevs in XGMI hive or jobs on
+ * different schedulers for same device while this tdr is running.
+ * We always reset all schedulers for device and all devices for XGMI
+ * hive so that should take care of them too.
*/
- hive = amdgpu_get_xgmi_hive(adev, 0);
- if (hive && adev->gmc.xgmi.num_physical_nodes > 1 &&
- !mutex_trylock(&hive->reset_lock))
+ if (xgmi_topology_present && !mutex_trylock(&hive->reset_lock))
return 0;
/* Start with adev pre asic reset first for soft reset check.*/
- amdgpu_device_lock_adev(adev);
+ if (!amdgpu_device_lock_adev(adev, !xgmi_topology_present))
+ return 0;
+
r = amdgpu_device_pre_asic_reset(adev,
job,
&need_full_reset);
@@ -3571,7 +3585,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
if (tmp_adev == adev)
continue;
- amdgpu_device_lock_adev(tmp_adev);
+ amdgpu_device_lock_adev(tmp_adev, false);
r = amdgpu_device_pre_asic_reset(tmp_adev,
NULL,
&need_full_reset);
@@ -3583,21 +3597,26 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
}
}
+
+
/* Actual ASIC resets if needed.*/
/* TODO Implement XGMI hive reset logic for SRIOV */
- if (amdgpu_sriov_vf(adev)) {
- r = amdgpu_device_reset_sriov(adev, job ? false : true);
- if (r)
- adev->asic_reset_res = r;
- } else {
- r = amdgpu_do_asic_reset(hive, device_list_handle, &need_full_reset);
- if (r && r == -EAGAIN)
- goto retry;
+
+ if (!(job_finished = drm_sched_job_finished(&job->base))) {
+ if (amdgpu_sriov_vf(adev)) {
+ r = amdgpu_device_reset_sriov(adev, job ? false : true);
+ if (r)
+ adev->asic_reset_res = r;
+ } else {
+ r = amdgpu_do_asic_reset(hive, device_list_handle, &need_full_reset);
+ if (r && r == -EAGAIN)
+ goto retry;
+ }
}
/* Post ASIC reset for all devs .*/
list_for_each_entry(tmp_adev, device_list_handle, gmc.xgmi.head) {
- amdgpu_device_post_asic_reset(tmp_adev, tmp_adev == adev ? job : NULL);
+ amdgpu_device_post_asic_reset(tmp_adev, job_finished);
if (r) {
/* bad news, how to tell it to userspace ? */
@@ -3610,7 +3629,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
amdgpu_device_unlock_adev(tmp_adev);
}
- if (hive && adev->gmc.xgmi.num_physical_nodes > 1)
+ if (xgmi_topology_present)
mutex_unlock(&hive->reset_lock);
if (r)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
index e1b46a6..f4195b9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
@@ -73,4 +73,5 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,
void *owner, struct dma_fence **f);
int amdgpu_job_submit_direct(struct amdgpu_job *job, struct amdgpu_ring *ring,
struct dma_fence **fence);
+
#endif
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
index 6f1268f..67ae266 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c
@@ -109,7 +109,7 @@ static void etnaviv_sched_timedout_job(struct drm_sched_job *sched_job)
}
/* block scheduler */
- drm_sched_stop(&gpu->sched, sched_job);
+ drm_sched_stop(&gpu->sched);
if(sched_job)
drm_sched_increase_karma(sched_job);
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 16c6363..4617fd9 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -189,11 +189,13 @@ EXPORT_SYMBOL(drm_sched_dependency_optimized);
*
* Start the timeout for the given scheduler.
*/
-static void drm_sched_start_timeout(struct drm_gpu_scheduler *sched)
+static void drm_sched_start_timeout(struct drm_gpu_scheduler *sched, long delay)
{
+ long timeout = delay >= 0 ? delay : sched->timeout;
+
if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_empty(&sched->ring_mirror_list))
- schedule_delayed_work(&sched->work_tdr, sched->timeout);
+ mod_delayed_work(system_wq, &sched->work_tdr, timeout);
}
/**
@@ -275,18 +277,16 @@ static void drm_sched_job_finish(struct work_struct *work)
unsigned long flags;
/*
- * Canceling the timeout without removing our job from the ring mirror
- * list is safe, as we will only end up in this worker if our jobs
- * finished fence has been signaled. So even if some another worker
- * manages to find this job as the next job in the list, the fence
- * signaled check below will prevent the timeout to be restarted.
+ * Avoid restarting timer if you are racing against tdr in progress
+ * as tdr tasks will stop and restart the timer itself otherwise you
+ * can end up rearming timer which the tdr just stopped.
*/
- cancel_delayed_work_sync(&sched->work_tdr);
-
- spin_lock_irqsave(&sched->job_list_lock, flags);
- /* queue TDR for next job */
- drm_sched_start_timeout(sched);
- spin_unlock_irqrestore(&sched->job_list_lock, flags);
+ if (!flush_delayed_work(&sched->work_tdr)) {
+ spin_lock_irqsave(&sched->job_list_lock, flags);
+ /* queue TDR for next job */
+ drm_sched_start_timeout(sched, -1);
+ spin_unlock_irqrestore(&sched->job_list_lock, flags);
+ }
sched->ops->free_job(s_job);
}
@@ -298,7 +298,7 @@ static void drm_sched_job_begin(struct drm_sched_job *s_job)
spin_lock_irqsave(&sched->job_list_lock, flags);
list_add_tail(&s_job->node, &sched->ring_mirror_list);
- drm_sched_start_timeout(sched);
+ drm_sched_start_timeout(sched, -1);
spin_unlock_irqrestore(&sched->job_list_lock, flags);
}
@@ -307,6 +307,7 @@ static void drm_sched_job_timedout(struct work_struct *work)
struct drm_gpu_scheduler *sched;
struct drm_sched_job *job;
unsigned long flags;
+ long delay = -1;
sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
job = list_first_entry_or_null(&sched->ring_mirror_list,
@@ -316,7 +317,9 @@ static void drm_sched_job_timedout(struct work_struct *work)
job->sched->ops->timedout_job(job);
spin_lock_irqsave(&sched->job_list_lock, flags);
- drm_sched_start_timeout(sched);
+ if (job && job->s_fence->parent && !drm_sched_job_finished(job))
+ delay = msecs_to_jiffies(2);
+ drm_sched_start_timeout(sched, delay);
spin_unlock_irqrestore(&sched->job_list_lock, flags);
}
@@ -372,12 +375,11 @@ EXPORT_SYMBOL(drm_sched_increase_karma);
* @bad: bad scheduler job
*
*/
-void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
+void drm_sched_stop(struct drm_gpu_scheduler *sched)
{
struct drm_sched_job *s_job;
unsigned long flags;
struct dma_fence *last_fence = NULL;
- int r;
kthread_park(sched->thread);
@@ -387,14 +389,12 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
* also all the previous jobs that were in flight also already singaled
* and removed from the list.
*/
-retry_wait:
spin_lock_irqsave(&sched->job_list_lock, flags);
list_for_each_entry_reverse(s_job, &sched->ring_mirror_list, node) {
if (s_job->s_fence->parent &&
dma_fence_remove_callback(s_job->s_fence->parent,
&s_job->cb)) {
dma_fence_put(s_job->s_fence->parent);
- s_job->s_fence->parent = NULL;
atomic_dec(&sched->hw_rq_count);
} else {
last_fence = dma_fence_get(&s_job->s_fence->finished);
@@ -407,6 +407,13 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
dma_fence_wait(last_fence, false);
dma_fence_put(last_fence);
}
+
+ /*
+ * We parked the sched thread so no new job can rearm the timer now,
+ * was also are flushing work_tdr from drm_sched_job_finish so no job
+ * finishing work in progress can rearm the timer.
+ */
+ cancel_delayed_work(&sched->work_tdr);
}
EXPORT_SYMBOL(drm_sched_stop);
@@ -447,7 +454,7 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery)
drm_sched_process_job(NULL, &s_job->cb);
}
- drm_sched_start_timeout(sched);
+ drm_sched_start_timeout(sched, -1);
unpark:
kthread_unpark(sched->thread);
diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c
index f76d9ed..9424388 100644
--- a/drivers/gpu/drm/v3d/v3d_sched.c
+++ b/drivers/gpu/drm/v3d/v3d_sched.c
@@ -178,8 +178,7 @@ v3d_job_timedout(struct drm_sched_job *sched_job)
for (q = 0; q < V3D_MAX_QUEUES; q++) {
struct drm_gpu_scheduler *sched = &v3d->queue[q].sched;
- drm_sched_stop(sched, (sched_job->sched == sched ?
- sched_job : NULL));
+ drm_sched_stop(sched);
if(sched_job)
drm_sched_increase_karma(sched_job);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 62c2352..24077f6 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -203,6 +203,12 @@ static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
return (s_job && atomic_inc_return(&s_job->karma) > threshold);
}
+static inline bool drm_sched_job_finished(struct drm_sched_job *s_job)
+{
+ WARN_ON(!s_job->s_fence->parent);
+ return dma_fence_is_signaled(s_job->s_fence->parent);
+}
+
/**
* struct drm_sched_backend_ops
*
@@ -296,8 +302,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
void *owner);
void drm_sched_job_cleanup(struct drm_sched_job *job);
void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
-void drm_sched_stop(struct drm_gpu_scheduler *sched,
- struct drm_sched_job *job);
+void drm_sched_stop(struct drm_gpu_scheduler *sched);
void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery);
void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
void drm_sched_increase_karma(struct drm_sched_job *bad);
--
2.7.4
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
