having a delayed work item per job is redundant as we only need one per scheduler to track the time out the currently executing job.
v2: the first element of the ring mirror list is the currently executing job so we don't need a additional variable for it
Signed-off-by: Nayan Deshmukh nayan26deshmukh@gmail.com Suggested-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/scheduler/sched_main.c | 28 +++++++++++++++------------- include/drm/gpu_scheduler.h | 6 +++--- 2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 9ca741f3a0bc..88f6cff136f2 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -197,19 +197,16 @@ static void drm_sched_job_finish(struct work_struct *work) * manages to find this job as the next job in the list, the fence * signaled check below will prevent the timeout to be restarted. */ - cancel_delayed_work_sync(&s_job->work_tdr); + cancel_delayed_work_sync(&sched->work_tdr);
spin_lock(&sched->job_list_lock); /* queue TDR for next job */ + list_del(&s_job->node); if (sched->timeout != MAX_SCHEDULE_TIMEOUT && - !list_is_last(&s_job->node, &sched->ring_mirror_list)) { - struct drm_sched_job *next = list_next_entry(s_job, node); - - if (!dma_fence_is_signaled(&next->s_fence->finished)) - schedule_delayed_work(&next->work_tdr, sched->timeout); + !list_empty(&sched->ring_mirror_list)) { + schedule_delayed_work(&sched->work_tdr, sched->timeout); } /* remove job from ring_mirror_list */ - list_del(&s_job->node); spin_unlock(&sched->job_list_lock);
dma_fence_put(&s_job->s_fence->finished); @@ -236,16 +233,21 @@ static void drm_sched_job_begin(struct drm_sched_job *s_job) if (sched->timeout != MAX_SCHEDULE_TIMEOUT && list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node) == s_job) - schedule_delayed_work(&s_job->work_tdr, sched->timeout); + schedule_delayed_work(&sched->work_tdr, sched->timeout); spin_unlock(&sched->job_list_lock); }
static void drm_sched_job_timedout(struct work_struct *work) { - struct drm_sched_job *job = container_of(work, struct drm_sched_job, - work_tdr.work); + struct drm_gpu_scheduler *sched; + struct drm_sched_job *job; + + sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work); + job = list_first_entry_or_null(&sched->ring_mirror_list, + struct drm_sched_job, node);
- job->sched->ops->timedout_job(job); + if (job) + job->sched->ops->timedout_job(job); }
/** @@ -315,7 +317,7 @@ void drm_sched_job_recovery(struct drm_gpu_scheduler *sched) s_job = list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node); if (s_job && sched->timeout != MAX_SCHEDULE_TIMEOUT) - schedule_delayed_work(&s_job->work_tdr, sched->timeout); + schedule_delayed_work(&sched->work_tdr, sched->timeout);
list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) { struct drm_sched_fence *s_fence = s_job->s_fence; @@ -384,7 +386,6 @@ int drm_sched_job_init(struct drm_sched_job *job,
INIT_WORK(&job->finish_work, drm_sched_job_finish); INIT_LIST_HEAD(&job->node); - INIT_DELAYED_WORK(&job->work_tdr, drm_sched_job_timedout);
return 0; } @@ -575,6 +576,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, INIT_LIST_HEAD(&sched->ring_mirror_list); spin_lock_init(&sched->job_list_lock); atomic_set(&sched->hw_rq_count, 0); + INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout); atomic_set(&sched->num_jobs, 0); atomic64_set(&sched->job_id_count, 0);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index daec50f887b3..d87b268f1781 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -175,8 +175,6 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f); * finished to remove the job from the * @drm_gpu_scheduler.ring_mirror_list. * @node: used to append this struct to the @drm_gpu_scheduler.ring_mirror_list. - * @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the timeout - * interval is over. * @id: a unique id assigned to each job scheduled on the scheduler. * @karma: increment on every hang caused by this job. If this exceeds the hang * limit of the scheduler then the job is marked guilty and will not @@ -195,7 +193,6 @@ struct drm_sched_job { struct dma_fence_cb finish_cb; struct work_struct finish_work; struct list_head node; - struct delayed_work work_tdr; uint64_t id; atomic_t karma; enum drm_sched_priority s_priority; @@ -259,6 +256,8 @@ struct drm_sched_backend_ops { * finished. * @hw_rq_count: the number of jobs currently in the hardware queue. * @job_id_count: used to assign unique id to the each job. + * @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the + * timeout interval is over. * @thread: the kthread on which the scheduler which run. * @ring_mirror_list: the list of jobs which are currently in the job queue. * @job_list_lock: lock to protect the ring_mirror_list. @@ -278,6 +277,7 @@ struct drm_gpu_scheduler { wait_queue_head_t job_scheduled; atomic_t hw_rq_count; atomic64_t job_id_count; + struct delayed_work work_tdr; struct task_struct *thread; struct list_head ring_mirror_list; spinlock_t job_list_lock;
Am 21.09.2018 um 20:57 schrieb Nayan Deshmukh:
having a delayed work item per job is redundant as we only need one per scheduler to track the time out the currently executing job.
v2: the first element of the ring mirror list is the currently executing job so we don't need a additional variable for it
Signed-off-by: Nayan Deshmukh nayan26deshmukh@gmail.com Suggested-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/scheduler/sched_main.c | 28 +++++++++++++++------------- include/drm/gpu_scheduler.h | 6 +++--- 2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 9ca741f3a0bc..88f6cff136f2 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -197,19 +197,16 @@ static void drm_sched_job_finish(struct work_struct *work) * manages to find this job as the next job in the list, the fence * signaled check below will prevent the timeout to be restarted. */
- cancel_delayed_work_sync(&s_job->work_tdr);
cancel_delayed_work_sync(&sched->work_tdr);
spin_lock(&sched->job_list_lock); /* queue TDR for next job */
list_del(&s_job->node); if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_is_last(&s_job->node, &sched->ring_mirror_list)) {struct drm_sched_job *next = list_next_entry(s_job, node);if (!dma_fence_is_signaled(&next->s_fence->finished))schedule_delayed_work(&next->work_tdr, sched->timeout);
!list_empty(&sched->ring_mirror_list)) {schedule_delayed_work(&sched->work_tdr, sched->timeout);
You can remove the {} here as well.
/* remove job from ring_mirror_list */
That comment should move with the list_del().
Apart from that it looks good to me, Christian.
list_del(&s_job->node); spin_unlock(&sched->job_list_lock);
dma_fence_put(&s_job->s_fence->finished);
@@ -236,16 +233,21 @@ static void drm_sched_job_begin(struct drm_sched_job *s_job) if (sched->timeout != MAX_SCHEDULE_TIMEOUT && list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node) == s_job)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout);spin_unlock(&sched->job_list_lock); }
static void drm_sched_job_timedout(struct work_struct *work) {
- struct drm_sched_job *job = container_of(work, struct drm_sched_job,
work_tdr.work);
- struct drm_gpu_scheduler *sched;
- struct drm_sched_job *job;
- sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
- job = list_first_entry_or_null(&sched->ring_mirror_list,
struct drm_sched_job, node);
- job->sched->ops->timedout_job(job);
if (job)
job->sched->ops->timedout_job(job);}
/**
@@ -315,7 +317,7 @@ void drm_sched_job_recovery(struct drm_gpu_scheduler *sched) s_job = list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node); if (s_job && sched->timeout != MAX_SCHEDULE_TIMEOUT)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout);list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) { struct drm_sched_fence *s_fence = s_job->s_fence;
@@ -384,7 +386,6 @@ int drm_sched_job_init(struct drm_sched_job *job,
INIT_WORK(&job->finish_work, drm_sched_job_finish); INIT_LIST_HEAD(&job->node);
INIT_DELAYED_WORK(&job->work_tdr, drm_sched_job_timedout);
return 0; }
@@ -575,6 +576,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, INIT_LIST_HEAD(&sched->ring_mirror_list); spin_lock_init(&sched->job_list_lock); atomic_set(&sched->hw_rq_count, 0);
- INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout); atomic_set(&sched->num_jobs, 0); atomic64_set(&sched->job_id_count, 0);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index daec50f887b3..d87b268f1781 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -175,8 +175,6 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
finished to remove the job from the@drm_gpu_scheduler.ring_mirror_list.- @node: used to append this struct to the @drm_gpu_scheduler.ring_mirror_list.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the timeout
interval is over.- @id: a unique id assigned to each job scheduled on the scheduler.
- @karma: increment on every hang caused by this job. If this exceeds the hang
limit of the scheduler then the job is marked guilty and will not@@ -195,7 +193,6 @@ struct drm_sched_job { struct dma_fence_cb finish_cb; struct work_struct finish_work; struct list_head node;
- struct delayed_work work_tdr; uint64_t id; atomic_t karma; enum drm_sched_priority s_priority;
@@ -259,6 +256,8 @@ struct drm_sched_backend_ops {
finished.- @hw_rq_count: the number of jobs currently in the hardware queue.
- @job_id_count: used to assign unique id to the each job.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the
timeout interval is over.- @thread: the kthread on which the scheduler which run.
- @ring_mirror_list: the list of jobs which are currently in the job queue.
- @job_list_lock: lock to protect the ring_mirror_list.
@@ -278,6 +277,7 @@ struct drm_gpu_scheduler { wait_queue_head_t job_scheduled; atomic_t hw_rq_count; atomic64_t job_id_count;
- struct delayed_work work_tdr; struct task_struct *thread; struct list_head ring_mirror_list; spinlock_t job_list_lock;
having a delayed work item per job is redundant as we only need one per scheduler to track the time out the currently executing job.
v2: the first element of the ring mirror list is the currently executing job so we don't need a additional variable for it
Signed-off-by: Nayan Deshmukh nayan26deshmukh@gmail.com Suggested-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/scheduler/sched_main.c | 31 ++++++++++++++++--------------- include/drm/gpu_scheduler.h | 6 +++--- 2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 9ca741f3a0bc..4e8505d51795 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -197,19 +197,15 @@ static void drm_sched_job_finish(struct work_struct *work) * manages to find this job as the next job in the list, the fence * signaled check below will prevent the timeout to be restarted. */ - cancel_delayed_work_sync(&s_job->work_tdr); + cancel_delayed_work_sync(&sched->work_tdr);
spin_lock(&sched->job_list_lock); - /* queue TDR for next job */ - if (sched->timeout != MAX_SCHEDULE_TIMEOUT && - !list_is_last(&s_job->node, &sched->ring_mirror_list)) { - struct drm_sched_job *next = list_next_entry(s_job, node); - - if (!dma_fence_is_signaled(&next->s_fence->finished)) - schedule_delayed_work(&next->work_tdr, sched->timeout); - } /* remove job from ring_mirror_list */ list_del(&s_job->node); + /* queue TDR for next job */ + if (sched->timeout != MAX_SCHEDULE_TIMEOUT && + !list_empty(&sched->ring_mirror_list)) + schedule_delayed_work(&sched->work_tdr, sched->timeout); spin_unlock(&sched->job_list_lock);
dma_fence_put(&s_job->s_fence->finished); @@ -236,16 +232,21 @@ static void drm_sched_job_begin(struct drm_sched_job *s_job) if (sched->timeout != MAX_SCHEDULE_TIMEOUT && list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node) == s_job) - schedule_delayed_work(&s_job->work_tdr, sched->timeout); + schedule_delayed_work(&sched->work_tdr, sched->timeout); spin_unlock(&sched->job_list_lock); }
static void drm_sched_job_timedout(struct work_struct *work) { - struct drm_sched_job *job = container_of(work, struct drm_sched_job, - work_tdr.work); + struct drm_gpu_scheduler *sched; + struct drm_sched_job *job; + + sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work); + job = list_first_entry_or_null(&sched->ring_mirror_list, + struct drm_sched_job, node);
- job->sched->ops->timedout_job(job); + if (job) + job->sched->ops->timedout_job(job); }
/** @@ -315,7 +316,7 @@ void drm_sched_job_recovery(struct drm_gpu_scheduler *sched) s_job = list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node); if (s_job && sched->timeout != MAX_SCHEDULE_TIMEOUT) - schedule_delayed_work(&s_job->work_tdr, sched->timeout); + schedule_delayed_work(&sched->work_tdr, sched->timeout);
list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) { struct drm_sched_fence *s_fence = s_job->s_fence; @@ -384,7 +385,6 @@ int drm_sched_job_init(struct drm_sched_job *job,
INIT_WORK(&job->finish_work, drm_sched_job_finish); INIT_LIST_HEAD(&job->node); - INIT_DELAYED_WORK(&job->work_tdr, drm_sched_job_timedout);
return 0; } @@ -575,6 +575,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, INIT_LIST_HEAD(&sched->ring_mirror_list); spin_lock_init(&sched->job_list_lock); atomic_set(&sched->hw_rq_count, 0); + INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout); atomic_set(&sched->num_jobs, 0); atomic64_set(&sched->job_id_count, 0);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index daec50f887b3..d87b268f1781 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -175,8 +175,6 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f); * finished to remove the job from the * @drm_gpu_scheduler.ring_mirror_list. * @node: used to append this struct to the @drm_gpu_scheduler.ring_mirror_list. - * @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the timeout - * interval is over. * @id: a unique id assigned to each job scheduled on the scheduler. * @karma: increment on every hang caused by this job. If this exceeds the hang * limit of the scheduler then the job is marked guilty and will not @@ -195,7 +193,6 @@ struct drm_sched_job { struct dma_fence_cb finish_cb; struct work_struct finish_work; struct list_head node; - struct delayed_work work_tdr; uint64_t id; atomic_t karma; enum drm_sched_priority s_priority; @@ -259,6 +256,8 @@ struct drm_sched_backend_ops { * finished. * @hw_rq_count: the number of jobs currently in the hardware queue. * @job_id_count: used to assign unique id to the each job. + * @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the + * timeout interval is over. * @thread: the kthread on which the scheduler which run. * @ring_mirror_list: the list of jobs which are currently in the job queue. * @job_list_lock: lock to protect the ring_mirror_list. @@ -278,6 +277,7 @@ struct drm_gpu_scheduler { wait_queue_head_t job_scheduled; atomic_t hw_rq_count; atomic64_t job_id_count; + struct delayed_work work_tdr; struct task_struct *thread; struct list_head ring_mirror_list; spinlock_t job_list_lock;
Am 25.09.2018 um 19:09 schrieb Nayan Deshmukh:
having a delayed work item per job is redundant as we only need one per scheduler to track the time out the currently executing job.
v2: the first element of the ring mirror list is the currently executing job so we don't need a additional variable for it
Signed-off-by: Nayan Deshmukh nayan26deshmukh@gmail.com Suggested-by: Christian König christian.koenig@amd.com
Reviewed-by: Christian König christian.koenig@amd.com
Going to push that into our branch on the next best occasion.
Christian.
drivers/gpu/drm/scheduler/sched_main.c | 31 ++++++++++++++++--------------- include/drm/gpu_scheduler.h | 6 +++--- 2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 9ca741f3a0bc..4e8505d51795 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -197,19 +197,15 @@ static void drm_sched_job_finish(struct work_struct *work) * manages to find this job as the next job in the list, the fence * signaled check below will prevent the timeout to be restarted. */
- cancel_delayed_work_sync(&s_job->work_tdr);
cancel_delayed_work_sync(&sched->work_tdr);
spin_lock(&sched->job_list_lock);
- /* queue TDR for next job */
- if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_is_last(&s_job->node, &sched->ring_mirror_list)) {struct drm_sched_job *next = list_next_entry(s_job, node);if (!dma_fence_is_signaled(&next->s_fence->finished))schedule_delayed_work(&next->work_tdr, sched->timeout);- } /* remove job from ring_mirror_list */ list_del(&s_job->node);
/* queue TDR for next job */
if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_empty(&sched->ring_mirror_list))schedule_delayed_work(&sched->work_tdr, sched->timeout);spin_unlock(&sched->job_list_lock);
dma_fence_put(&s_job->s_fence->finished);
@@ -236,16 +232,21 @@ static void drm_sched_job_begin(struct drm_sched_job *s_job) if (sched->timeout != MAX_SCHEDULE_TIMEOUT && list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node) == s_job)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout);spin_unlock(&sched->job_list_lock); }
static void drm_sched_job_timedout(struct work_struct *work) {
- struct drm_sched_job *job = container_of(work, struct drm_sched_job,
work_tdr.work);
- struct drm_gpu_scheduler *sched;
- struct drm_sched_job *job;
- sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
- job = list_first_entry_or_null(&sched->ring_mirror_list,
struct drm_sched_job, node);
- job->sched->ops->timedout_job(job);
if (job)
job->sched->ops->timedout_job(job);}
/**
@@ -315,7 +316,7 @@ void drm_sched_job_recovery(struct drm_gpu_scheduler *sched) s_job = list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node); if (s_job && sched->timeout != MAX_SCHEDULE_TIMEOUT)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout);list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) { struct drm_sched_fence *s_fence = s_job->s_fence;
@@ -384,7 +385,6 @@ int drm_sched_job_init(struct drm_sched_job *job,
INIT_WORK(&job->finish_work, drm_sched_job_finish); INIT_LIST_HEAD(&job->node);
INIT_DELAYED_WORK(&job->work_tdr, drm_sched_job_timedout);
return 0; }
@@ -575,6 +575,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, INIT_LIST_HEAD(&sched->ring_mirror_list); spin_lock_init(&sched->job_list_lock); atomic_set(&sched->hw_rq_count, 0);
- INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout); atomic_set(&sched->num_jobs, 0); atomic64_set(&sched->job_id_count, 0);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index daec50f887b3..d87b268f1781 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -175,8 +175,6 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
finished to remove the job from the@drm_gpu_scheduler.ring_mirror_list.- @node: used to append this struct to the @drm_gpu_scheduler.ring_mirror_list.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the timeout
interval is over.- @id: a unique id assigned to each job scheduled on the scheduler.
- @karma: increment on every hang caused by this job. If this exceeds the hang
limit of the scheduler then the job is marked guilty and will not@@ -195,7 +193,6 @@ struct drm_sched_job { struct dma_fence_cb finish_cb; struct work_struct finish_work; struct list_head node;
- struct delayed_work work_tdr; uint64_t id; atomic_t karma; enum drm_sched_priority s_priority;
@@ -259,6 +256,8 @@ struct drm_sched_backend_ops {
finished.- @hw_rq_count: the number of jobs currently in the hardware queue.
- @job_id_count: used to assign unique id to the each job.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the
timeout interval is over.- @thread: the kthread on which the scheduler which run.
- @ring_mirror_list: the list of jobs which are currently in the job queue.
- @job_list_lock: lock to protect the ring_mirror_list.
@@ -278,6 +277,7 @@ struct drm_gpu_scheduler { wait_queue_head_t job_scheduled; atomic_t hw_rq_count; atomic64_t job_id_count;
- struct delayed_work work_tdr; struct task_struct *thread; struct list_head ring_mirror_list; spinlock_t job_list_lock;
Hi Nayan,
Am Mittwoch, den 26.09.2018, 02:09 +0900 schrieb Nayan Deshmukh:
having a delayed work item per job is redundant as we only need one per scheduler to track the time out the currently executing job.
v2: the first element of the ring mirror list is the currently executing job so we don't need a additional variable for it
Signed-off-by: Nayan Deshmukh nayan26deshmukh@gmail.com Suggested-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/scheduler/sched_main.c | 31 ++++++++++++++++--------------- include/drm/gpu_scheduler.h | 6 +++--- 2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 9ca741f3a0bc..4e8505d51795 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -197,19 +197,15 @@ static void drm_sched_job_finish(struct work_struct *work) * manages to find this job as the next job in the list, the fence * signaled check below will prevent the timeout to be restarted. */
- cancel_delayed_work_sync(&s_job->work_tdr);
cancel_delayed_work_sync(&sched->work_tdr);
spin_lock(&sched->job_list_lock);
- /* queue TDR for next job */
- if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_is_last(&s_job->node, &sched->ring_mirror_list)) {struct drm_sched_job *next = list_next_entry(s_job, node);if (!dma_fence_is_signaled(&next->s_fence->finished))schedule_delayed_work(&next->work_tdr, sched->timeout);- } /* remove job from ring_mirror_list */ list_del(&s_job->node);
/* queue TDR for next job */
if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_empty(&sched->ring_mirror_list))schedule_delayed_work(&sched->work_tdr, sched->timeout);spin_unlock(&sched->job_list_lock);
dma_fence_put(&s_job->s_fence->finished);
@@ -236,16 +232,21 @@ static void drm_sched_job_begin(struct drm_sched_job *s_job) if (sched->timeout != MAX_SCHEDULE_TIMEOUT && list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node) == s_job)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout);}
static void drm_sched_job_timedout(struct work_struct *work) {
- struct drm_sched_job *job = container_of(work, struct drm_sched_job,
work_tdr.work);
- struct drm_gpu_scheduler *sched;
- struct drm_sched_job *job;
- sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
- job = list_first_entry_or_null(&sched->ring_mirror_list,
struct drm_sched_job, node);
- job->sched->ops->timedout_job(job);
- if (job)
job->sched->ops->timedout_job(job);
I don't think this is fully robust. Jobs are only removed from the ring_mirror_list once the job_finish worker has run. If execution of this worker is delayed for any reason (though it's really unlikely for a delay as long as the job timeout to happen) you are blaming the wrong job here.
So I think what you need to to is find the first job in the ring mirror list with an unsignaled finish fence to robustly find the stuck job.
Regards, Lucas
}
/** @@ -315,7 +316,7 @@ void drm_sched_job_recovery(struct drm_gpu_scheduler *sched) s_job = list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node); if (s_job && sched->timeout != MAX_SCHEDULE_TIMEOUT)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout);list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) { struct drm_sched_fence *s_fence = s_job->s_fence;
@@ -384,7 +385,6 @@ int drm_sched_job_init(struct drm_sched_job *job,
INIT_WORK(&job->finish_work, drm_sched_job_finish); INIT_LIST_HEAD(&job->node);
INIT_DELAYED_WORK(&job->work_tdr, drm_sched_job_timedout);
return 0;
} @@ -575,6 +575,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, INIT_LIST_HEAD(&sched->ring_mirror_list); spin_lock_init(&sched->job_list_lock); atomic_set(&sched->hw_rq_count, 0);
- INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout); atomic_set(&sched->num_jobs, 0); atomic64_set(&sched->job_id_count, 0);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index daec50f887b3..d87b268f1781 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -175,8 +175,6 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
finished to remove the job from the@drm_gpu_scheduler.ring_mirror_list.- @node: used to append this struct to the @drm_gpu_scheduler.ring_mirror_list.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the timeout
interval is over.- @id: a unique id assigned to each job scheduled on the scheduler.
- @karma: increment on every hang caused by this job. If this exceeds the hang
limit of the scheduler then the job is marked guilty and will not@@ -195,7 +193,6 @@ struct drm_sched_job { struct dma_fence_cb finish_cb; struct work_struct finish_work; struct list_head node;
- struct delayed_work work_tdr; uint64_t id; atomic_t karma; enum drm_sched_priority s_priority;
@@ -259,6 +256,8 @@ struct drm_sched_backend_ops {
finished.- @hw_rq_count: the number of jobs currently in the hardware queue.
- @job_id_count: used to assign unique id to the each job.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the
timeout interval is over.- @thread: the kthread on which the scheduler which run.
- @ring_mirror_list: the list of jobs which are currently in the job queue.
- @job_list_lock: lock to protect the ring_mirror_list.
@@ -278,6 +277,7 @@ struct drm_gpu_scheduler { wait_queue_head_t job_scheduled; atomic_t hw_rq_count; atomic64_t job_id_count;
- struct delayed_work work_tdr; struct task_struct *thread; struct list_head ring_mirror_list; spinlock_t job_list_lock;
Am 26.09.2018 um 09:39 schrieb Lucas Stach:
Hi Nayan,
Am Mittwoch, den 26.09.2018, 02:09 +0900 schrieb Nayan Deshmukh:
having a delayed work item per job is redundant as we only need one per scheduler to track the time out the currently executing job.
v2: the first element of the ring mirror list is the currently executing job so we don't need a additional variable for it
Signed-off-by: Nayan Deshmukh nayan26deshmukh@gmail.com Suggested-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/scheduler/sched_main.c | 31 ++++++++++++++++--------------- include/drm/gpu_scheduler.h | 6 +++--- 2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 9ca741f3a0bc..4e8505d51795 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -197,19 +197,15 @@ static void drm_sched_job_finish(struct work_struct *work) * manages to find this job as the next job in the list, the fence * signaled check below will prevent the timeout to be restarted. */
- cancel_delayed_work_sync(&s_job->work_tdr);
cancel_delayed_work_sync(&sched->work_tdr);
spin_lock(&sched->job_list_lock);
- /* queue TDR for next job */
- if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_is_last(&s_job->node, &sched->ring_mirror_list)) {struct drm_sched_job *next = list_next_entry(s_job, node);if (!dma_fence_is_signaled(&next->s_fence->finished))schedule_delayed_work(&next->work_tdr, sched->timeout);- } /* remove job from ring_mirror_list */ list_del(&s_job->node);
/* queue TDR for next job */
if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_empty(&sched->ring_mirror_list))schedule_delayed_work(&sched->work_tdr, sched->timeout);spin_unlock(&sched->job_list_lock);
dma_fence_put(&s_job->s_fence->finished);
@@ -236,16 +232,21 @@ static void drm_sched_job_begin(struct drm_sched_job *s_job) if (sched->timeout != MAX_SCHEDULE_TIMEOUT && list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node) == s_job)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout);spin_unlock(&sched->job_list_lock); }
static void drm_sched_job_timedout(struct work_struct *work) {
- struct drm_sched_job *job = container_of(work, struct drm_sched_job,
work_tdr.work);
- struct drm_gpu_scheduler *sched;
- struct drm_sched_job *job;
- sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
- job = list_first_entry_or_null(&sched->ring_mirror_list,
struct drm_sched_job, node);
- job->sched->ops->timedout_job(job);
- if (job)
job->sched->ops->timedout_job(job);I don't think this is fully robust. Jobs are only removed from the ring_mirror_list once the job_finish worker has run. If execution of this worker is delayed for any reason (though it's really unlikely for a delay as long as the job timeout to happen) you are blaming the wrong job here.
So I think what you need to to is find the first job in the ring mirror list with an unsignaled finish fence to robustly find the stuck job.
Yeah, that is a known problem I've pointed out as well.
The issue is we have bug reports that this happened before the patch, but I'm not 100% sure how.
My suggestion is to move a good part of the logic from drm_sched_hw_job_reset() and drm_sched_job_recovery() into drm_sched_job_timedout().
E.g. we first call dma_fence_remove_callback() for each job and actually check the return value if the fence was already signaled.
If we find a signaled fence we abort and add the callback back to the ones where we removed it.
Nayan do you want to take care of this or should I take a look?
Regards, Christian.
Regards, Lucas
}
/** @@ -315,7 +316,7 @@ void drm_sched_job_recovery(struct drm_gpu_scheduler *sched) s_job = list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node); if (s_job && sched->timeout != MAX_SCHEDULE_TIMEOUT)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout);list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) { struct drm_sched_fence *s_fence = s_job->s_fence;
@@ -384,7 +385,6 @@ int drm_sched_job_init(struct drm_sched_job *job,
INIT_WORK(&job->finish_work, drm_sched_job_finish); INIT_LIST_HEAD(&job->node);
INIT_DELAYED_WORK(&job->work_tdr, drm_sched_job_timedout);
return 0; }
@@ -575,6 +575,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, INIT_LIST_HEAD(&sched->ring_mirror_list); spin_lock_init(&sched->job_list_lock); atomic_set(&sched->hw_rq_count, 0);
- INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout); atomic_set(&sched->num_jobs, 0); atomic64_set(&sched->job_id_count, 0);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index daec50f887b3..d87b268f1781 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -175,8 +175,6 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
finished to remove the job from the@drm_gpu_scheduler.ring_mirror_list.- @node: used to append this struct to the @drm_gpu_scheduler.ring_mirror_list.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the timeout
interval is over.- @id: a unique id assigned to each job scheduled on the scheduler.
- @karma: increment on every hang caused by this job. If this exceeds the hang
limit of the scheduler then the job is marked guilty and will not@@ -195,7 +193,6 @@ struct drm_sched_job { struct dma_fence_cb finish_cb; struct work_struct finish_work; struct list_head node;
- struct delayed_work work_tdr; uint64_t id; atomic_t karma; enum drm_sched_priority s_priority;
@@ -259,6 +256,8 @@ struct drm_sched_backend_ops {
finished.- @hw_rq_count: the number of jobs currently in the hardware queue.
- @job_id_count: used to assign unique id to the each job.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the
timeout interval is over.- @thread: the kthread on which the scheduler which run.
- @ring_mirror_list: the list of jobs which are currently in the job queue.
- @job_list_lock: lock to protect the ring_mirror_list.
@@ -278,6 +277,7 @@ struct drm_gpu_scheduler { wait_queue_head_t job_scheduled; atomic_t hw_rq_count; atomic64_t job_id_count;
- struct delayed_work work_tdr; struct task_struct *thread; struct list_head ring_mirror_list; spinlock_t job_list_lock;
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Christian,
On Wed, Sep 26, 2018, 10:13 AM Christian König < ckoenig.leichtzumerken@gmail.com> wrote:
Am 26.09.2018 um 09:39 schrieb Lucas Stach:
Hi Nayan,
Am Mittwoch, den 26.09.2018, 02:09 +0900 schrieb Nayan Deshmukh:
having a delayed work item per job is redundant as we only need one per scheduler to track the time out the currently executing job.
v2: the first element of the ring mirror list is the currently executing job so we don't need a additional variable for it
Signed-off-by: Nayan Deshmukh nayan26deshmukh@gmail.com Suggested-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/scheduler/sched_main.c | 31
++++++++++++++++---------------
include/drm/gpu_scheduler.h | 6 +++--- 2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c
b/drivers/gpu/drm/scheduler/sched_main.c
index 9ca741f3a0bc..4e8505d51795 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -197,19 +197,15 @@ static void drm_sched_job_finish(struct
work_struct *work)
* manages to find this job as the next job in the list, the fence * signaled check below will prevent the timeout to be restarted. */
- cancel_delayed_work_sync(&s_job->work_tdr);
cancel_delayed_work_sync(&sched->work_tdr);
spin_lock(&sched->job_list_lock);
- /* queue TDR for next job */
- if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_is_last(&s_job->node, &sched->ring_mirror_list)) {struct drm_sched_job *next = list_next_entry(s_job, node);if (!dma_fence_is_signaled(&next->s_fence->finished))schedule_delayed_work(&next->work_tdr,sched->timeout);
- } /* remove job from ring_mirror_list */ list_del(&s_job->node);
/* queue TDR for next job */
if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_empty(&sched->ring_mirror_list))schedule_delayed_work(&sched->work_tdr, sched->timeout);spin_unlock(&sched->job_list_lock);
dma_fence_put(&s_job->s_fence->finished);
@@ -236,16 +232,21 @@ static void drm_sched_job_begin(struct
drm_sched_job *s_job)
if (sched->timeout != MAX_SCHEDULE_TIMEOUT && list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node) == s_job)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout);spin_unlock(&sched->job_list_lock); }
static void drm_sched_job_timedout(struct work_struct *work) {
- struct drm_sched_job *job = container_of(work, struct
drm_sched_job,
work_tdr.work);
- struct drm_gpu_scheduler *sched;
- struct drm_sched_job *job;
- sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work
);
- job = list_first_entry_or_null(&sched->ring_mirror_list,
struct drm_sched_job, node);
- job->sched->ops->timedout_job(job);
- if (job)
job->sched->ops->timedout_job(job);I don't think this is fully robust. Jobs are only removed from the ring_mirror_list once the job_finish worker has run. If execution of this worker is delayed for any reason (though it's really unlikely for a delay as long as the job timeout to happen) you are blaming the wrong job here.
So I think what you need to to is find the first job in the ring mirror list with an unsignaled finish fence to robustly find the stuck job.
Yeah, that is a known problem I've pointed out as well.
The issue is we have bug reports that this happened before the patch, but I'm not 100% sure how.
My suggestion is to move a good part of the logic from drm_sched_hw_job_reset() and drm_sched_job_recovery() into drm_sched_job_timedout().
E.g. we first call dma_fence_remove_callback() for each job and actually check the return value if the fence was already signaled.
If we find a signaled fence we abort and add the callback back to the ones where we removed it.
Nayan do you want to take care of this or should I take a look?
I can take care of it.
Regards, Nayan
Regards, Christian.
Regards, Lucas
}
/** @@ -315,7 +316,7 @@ void drm_sched_job_recovery(struct
drm_gpu_scheduler *sched)
s_job = list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node); if (s_job && sched->timeout != MAX_SCHEDULE_TIMEOUT)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout);list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list,
node) {
struct drm_sched_fence *s_fence = s_job->s_fence;@@ -384,7 +385,6 @@ int drm_sched_job_init(struct drm_sched_job *job,
INIT_WORK(&job->finish_work, drm_sched_job_finish); INIT_LIST_HEAD(&job->node);
INIT_DELAYED_WORK(&job->work_tdr, drm_sched_job_timedout);
return 0; }
@@ -575,6 +575,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, INIT_LIST_HEAD(&sched->ring_mirror_list); spin_lock_init(&sched->job_list_lock); atomic_set(&sched->hw_rq_count, 0);
- INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout); atomic_set(&sched->num_jobs, 0); atomic64_set(&sched->job_id_count, 0);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index daec50f887b3..d87b268f1781 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -175,8 +175,6 @@ struct drm_sched_fence *to_drm_sched_fence(struct
dma_fence *f);
finished to remove the job from the@drm_gpu_scheduler.ring_mirror_list.- @node: used to append this struct to the
@drm_gpu_scheduler.ring_mirror_list.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout
after the timeout
interval is over.- @id: a unique id assigned to each job scheduled on the scheduler.
- @karma: increment on every hang caused by this job. If this
exceeds the hang
limit of the scheduler then the job is marked guilty andwill not
@@ -195,7 +193,6 @@ struct drm_sched_job { struct dma_fence_cb finish_cb; struct work_struct finish_work; struct list_head node;
- struct delayed_work work_tdr; uint64_t id; atomic_t karma; enum drm_sched_priority s_priority;
@@ -259,6 +256,8 @@ struct drm_sched_backend_ops {
finished.- @hw_rq_count: the number of jobs currently in the hardware queue.
- @job_id_count: used to assign unique id to the each job.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout
after the
timeout interval is over.- @thread: the kthread on which the scheduler which run.
- @ring_mirror_list: the list of jobs which are currently in the job
queue.
- @job_list_lock: lock to protect the ring_mirror_list.
@@ -278,6 +277,7 @@ struct drm_gpu_scheduler { wait_queue_head_t job_scheduled; atomic_t hw_rq_count; atomic64_t job_id_count;
- struct delayed_work work_tdr; struct task_struct *thread; struct list_head ring_mirror_list; spinlock_t job_list_lock;
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Christian,
On Thu, Sep 27, 2018 at 12:55 AM Nayan Deshmukh nayan26deshmukh@gmail.com wrote:
Hi Christian,
On Wed, Sep 26, 2018, 10:13 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 26.09.2018 um 09:39 schrieb Lucas Stach:
Hi Nayan,
Am Mittwoch, den 26.09.2018, 02:09 +0900 schrieb Nayan Deshmukh:
having a delayed work item per job is redundant as we only need one per scheduler to track the time out the currently executing job.
v2: the first element of the ring mirror list is the currently executing job so we don't need a additional variable for it
Signed-off-by: Nayan Deshmukh nayan26deshmukh@gmail.com Suggested-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/scheduler/sched_main.c | 31 ++++++++++++++++--------------- include/drm/gpu_scheduler.h | 6 +++--- 2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 9ca741f3a0bc..4e8505d51795 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -197,19 +197,15 @@ static void drm_sched_job_finish(struct work_struct *work) * manages to find this job as the next job in the list, the fence * signaled check below will prevent the timeout to be restarted. */
- cancel_delayed_work_sync(&s_job->work_tdr);
cancel_delayed_work_sync(&sched->work_tdr);
spin_lock(&sched->job_list_lock);
- /* queue TDR for next job */
- if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_is_last(&s_job->node, &sched->ring_mirror_list)) {struct drm_sched_job *next = list_next_entry(s_job, node);if (!dma_fence_is_signaled(&next->s_fence->finished))schedule_delayed_work(&next->work_tdr, sched->timeout);- } /* remove job from ring_mirror_list */ list_del(&s_job->node);
/* queue TDR for next job */
if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_empty(&sched->ring_mirror_list))schedule_delayed_work(&sched->work_tdr, sched->timeout);spin_unlock(&sched->job_list_lock);
dma_fence_put(&s_job->s_fence->finished);
@@ -236,16 +232,21 @@ static void drm_sched_job_begin(struct drm_sched_job *s_job) if (sched->timeout != MAX_SCHEDULE_TIMEOUT && list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node) == s_job)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout);spin_unlock(&sched->job_list_lock); }
static void drm_sched_job_timedout(struct work_struct *work) {
- struct drm_sched_job *job = container_of(work, struct drm_sched_job,
work_tdr.work);
- struct drm_gpu_scheduler *sched;
- struct drm_sched_job *job;
- sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
- job = list_first_entry_or_null(&sched->ring_mirror_list,
struct drm_sched_job, node);
- job->sched->ops->timedout_job(job);
- if (job)
job->sched->ops->timedout_job(job);I don't think this is fully robust. Jobs are only removed from the ring_mirror_list once the job_finish worker has run. If execution of this worker is delayed for any reason (though it's really unlikely for a delay as long as the job timeout to happen) you are blaming the wrong job here.
So I think what you need to to is find the first job in the ring mirror list with an unsignaled finish fence to robustly find the stuck job.
Yeah, that is a known problem I've pointed out as well.
The issue is we have bug reports that this happened before the patch, but I'm not 100% sure how.
My suggestion is to move a good part of the logic from drm_sched_hw_job_reset() and drm_sched_job_recovery() into drm_sched_job_timedout().
E.g. we first call dma_fence_remove_callback() for each job and actually check the return value if the fence was already signaled.
We can move this part to drm_sched_job_timedout().
If we find a signaled fence we abort and add the callback back to the ones where we removed it.
I was not able to find the abort part. In drm_sched_hw_job_reset() we don't take a action if the parent fence was already signaled.
We cannot shift a lot from drm_sched_job_recovery() to drm_sched_job_timedout(). The only part that seems shiftable is the one where we cancel the guilty job.
Regards, Nayan
Nayan do you want to take care of this or should I take a look?
I can take care of it.
Regards, Nayan
Regards, Christian.
Regards, Lucas
}
/** @@ -315,7 +316,7 @@ void drm_sched_job_recovery(struct drm_gpu_scheduler *sched) s_job = list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node); if (s_job && sched->timeout != MAX_SCHEDULE_TIMEOUT)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout);list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) { struct drm_sched_fence *s_fence = s_job->s_fence;
@@ -384,7 +385,6 @@ int drm_sched_job_init(struct drm_sched_job *job,
INIT_WORK(&job->finish_work, drm_sched_job_finish); INIT_LIST_HEAD(&job->node);
INIT_DELAYED_WORK(&job->work_tdr, drm_sched_job_timedout);
return 0; }
@@ -575,6 +575,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, INIT_LIST_HEAD(&sched->ring_mirror_list); spin_lock_init(&sched->job_list_lock); atomic_set(&sched->hw_rq_count, 0);
- INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout); atomic_set(&sched->num_jobs, 0); atomic64_set(&sched->job_id_count, 0);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index daec50f887b3..d87b268f1781 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -175,8 +175,6 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
finished to remove the job from the@drm_gpu_scheduler.ring_mirror_list.- @node: used to append this struct to the @drm_gpu_scheduler.ring_mirror_list.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the timeout
interval is over.- @id: a unique id assigned to each job scheduled on the scheduler.
- @karma: increment on every hang caused by this job. If this exceeds the hang
limit of the scheduler then the job is marked guilty and will not@@ -195,7 +193,6 @@ struct drm_sched_job { struct dma_fence_cb finish_cb; struct work_struct finish_work; struct list_head node;
- struct delayed_work work_tdr; uint64_t id; atomic_t karma; enum drm_sched_priority s_priority;
@@ -259,6 +256,8 @@ struct drm_sched_backend_ops {
finished.- @hw_rq_count: the number of jobs currently in the hardware queue.
- @job_id_count: used to assign unique id to the each job.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the
timeout interval is over.- @thread: the kthread on which the scheduler which run.
- @ring_mirror_list: the list of jobs which are currently in the job queue.
- @job_list_lock: lock to protect the ring_mirror_list.
@@ -278,6 +277,7 @@ struct drm_gpu_scheduler { wait_queue_head_t job_scheduled; atomic_t hw_rq_count; atomic64_t job_id_count;
- struct delayed_work work_tdr; struct task_struct *thread; struct list_head ring_mirror_list; spinlock_t job_list_lock;
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Am 04.10.2018 um 18:32 schrieb Nayan Deshmukh:
Hi Christian,
On Thu, Sep 27, 2018 at 12:55 AM Nayan Deshmukh nayan26deshmukh@gmail.com wrote:
Hi Christian,
On Wed, Sep 26, 2018, 10:13 AM Christian König ckoenig.leichtzumerken@gmail.com wrote:
Am 26.09.2018 um 09:39 schrieb Lucas Stach:
Hi Nayan,
Am Mittwoch, den 26.09.2018, 02:09 +0900 schrieb Nayan Deshmukh:
having a delayed work item per job is redundant as we only need one per scheduler to track the time out the currently executing job.
v2: the first element of the ring mirror list is the currently executing job so we don't need a additional variable for it
Signed-off-by: Nayan Deshmukh nayan26deshmukh@gmail.com Suggested-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/scheduler/sched_main.c | 31 ++++++++++++++++--------------- include/drm/gpu_scheduler.h | 6 +++--- 2 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 9ca741f3a0bc..4e8505d51795 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -197,19 +197,15 @@ static void drm_sched_job_finish(struct work_struct *work) * manages to find this job as the next job in the list, the fence * signaled check below will prevent the timeout to be restarted. */
- cancel_delayed_work_sync(&s_job->work_tdr);
cancel_delayed_work_sync(&sched->work_tdr);
spin_lock(&sched->job_list_lock);
- /* queue TDR for next job */
- if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_is_last(&s_job->node, &sched->ring_mirror_list)) {struct drm_sched_job *next = list_next_entry(s_job, node);if (!dma_fence_is_signaled(&next->s_fence->finished))schedule_delayed_work(&next->work_tdr, sched->timeout);- } /* remove job from ring_mirror_list */ list_del(&s_job->node);
/* queue TDR for next job */
if (sched->timeout != MAX_SCHEDULE_TIMEOUT &&
!list_empty(&sched->ring_mirror_list))schedule_delayed_work(&sched->work_tdr, sched->timeout); spin_unlock(&sched->job_list_lock); dma_fence_put(&s_job->s_fence->finished);@@ -236,16 +232,21 @@ static void drm_sched_job_begin(struct drm_sched_job *s_job) if (sched->timeout != MAX_SCHEDULE_TIMEOUT && list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node) == s_job)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout); spin_unlock(&sched->job_list_lock);}
static void drm_sched_job_timedout(struct work_struct *work) {
- struct drm_sched_job *job = container_of(work, struct drm_sched_job,
work_tdr.work);
- struct drm_gpu_scheduler *sched;
- struct drm_sched_job *job;
- sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
- job = list_first_entry_or_null(&sched->ring_mirror_list,
struct drm_sched_job, node);
- job->sched->ops->timedout_job(job);
- if (job)
job->sched->ops->timedout_job(job);I don't think this is fully robust. Jobs are only removed from the ring_mirror_list once the job_finish worker has run. If execution of this worker is delayed for any reason (though it's really unlikely for a delay as long as the job timeout to happen) you are blaming the wrong job here.
So I think what you need to to is find the first job in the ring mirror list with an unsignaled finish fence to robustly find the stuck job.
Yeah, that is a known problem I've pointed out as well.
The issue is we have bug reports that this happened before the patch, but I'm not 100% sure how.
My suggestion is to move a good part of the logic from drm_sched_hw_job_reset() and drm_sched_job_recovery() into drm_sched_job_timedout().
E.g. we first call dma_fence_remove_callback() for each job and actually check the return value if the fence was already signaled.
We can move this part to drm_sched_job_timedout().
If we find a signaled fence we abort and add the callback back to the ones where we removed it.
I was not able to find the abort part. In drm_sched_hw_job_reset() we don't take a action if the parent fence was already signaled.
That is a bug a swell.
We cannot shift a lot from drm_sched_job_recovery() to drm_sched_job_timedout(). The only part that seems shiftable is the one where we cancel the guilty job.
Actually I wanted to completely rework that part since it is rather unreliable as well.
Considering how buggy all of that is and how important it is to get it right I think I will rather do it myself.
Regards, Christian.
Regards, Nayan
Nayan do you want to take care of this or should I take a look?
I can take care of it.
Regards, Nayan
Regards, Christian.
Regards, Lucas
}
/** @@ -315,7 +316,7 @@ void drm_sched_job_recovery(struct drm_gpu_scheduler *sched) s_job = list_first_entry_or_null(&sched->ring_mirror_list, struct drm_sched_job, node); if (s_job && sched->timeout != MAX_SCHEDULE_TIMEOUT)
schedule_delayed_work(&s_job->work_tdr, sched->timeout);
schedule_delayed_work(&sched->work_tdr, sched->timeout); list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) { struct drm_sched_fence *s_fence = s_job->s_fence;@@ -384,7 +385,6 @@ int drm_sched_job_init(struct drm_sched_job *job,
INIT_WORK(&job->finish_work, drm_sched_job_finish); INIT_LIST_HEAD(&job->node);
INIT_DELAYED_WORK(&job->work_tdr, drm_sched_job_timedout);
return 0; }
@@ -575,6 +575,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, INIT_LIST_HEAD(&sched->ring_mirror_list); spin_lock_init(&sched->job_list_lock); atomic_set(&sched->hw_rq_count, 0);
- INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout); atomic_set(&sched->num_jobs, 0); atomic64_set(&sched->job_id_count, 0);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index daec50f887b3..d87b268f1781 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -175,8 +175,6 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f); * finished to remove the job from the * @drm_gpu_scheduler.ring_mirror_list. * @node: used to append this struct to the @drm_gpu_scheduler.ring_mirror_list.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the timeout
interval is over.
- @id: a unique id assigned to each job scheduled on the scheduler.
- @karma: increment on every hang caused by this job. If this exceeds the hang
limit of the scheduler then the job is marked guilty and will not@@ -195,7 +193,6 @@ struct drm_sched_job { struct dma_fence_cb finish_cb; struct work_struct finish_work; struct list_head node;
- struct delayed_work work_tdr; uint64_t id; atomic_t karma; enum drm_sched_priority s_priority;
@@ -259,6 +256,8 @@ struct drm_sched_backend_ops { * finished. * @hw_rq_count: the number of jobs currently in the hardware queue. * @job_id_count: used to assign unique id to the each job.
- @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the
timeout interval is over.
- @thread: the kthread on which the scheduler which run.
- @ring_mirror_list: the list of jobs which are currently in the job queue.
- @job_list_lock: lock to protect the ring_mirror_list.
@@ -278,6 +277,7 @@ struct drm_gpu_scheduler { wait_queue_head_t job_scheduled; atomic_t hw_rq_count; atomic64_t job_id_count;
- struct delayed_work work_tdr; struct task_struct *thread; struct list_head ring_mirror_list; spinlock_t job_list_lock;
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Christian König -
Lucas Stach -
Nayan Deshmukh