Ping on this patch and patch 5. The rest already RBed. Andrey On 4/16/19 2:23 PM, Andrey Grodzovsky wrote: > From: Christian König <christian.koenig@xxxxxxx> > > We now destroy finished jobs from the worker thread to make sure that > we never destroy a job currently in timeout processing. > By this we avoid holding lock around ring mirror list in drm_sched_stop > which should solve a deadlock reported by a user. > > v2: Remove unused variable. > v4: Move guilty job free into sched code. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109692 > > Signed-off-by: Christian König <christian.koenig@xxxxxxx> > Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 9 +- > drivers/gpu/drm/etnaviv/etnaviv_dump.c | 4 - > drivers/gpu/drm/etnaviv/etnaviv_sched.c | 2 +- > drivers/gpu/drm/lima/lima_sched.c | 2 +- > drivers/gpu/drm/panfrost/panfrost_job.c | 2 +- > drivers/gpu/drm/scheduler/sched_main.c | 145 +++++++++++++++++------------ > drivers/gpu/drm/v3d/v3d_sched.c | 2 +- > include/drm/gpu_scheduler.h | 6 +- > 8 files changed, 94 insertions(+), 78 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 7cee269..a0e165c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -3334,7 +3334,7 @@ static int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev, > if (!ring || !ring->sched.thread) > continue; > > - drm_sched_stop(&ring->sched); > + drm_sched_stop(&ring->sched, &job->base); > > /* after all hw jobs are reset, hw fence is meaningless, so force_completion */ > amdgpu_fence_driver_force_completion(ring); > @@ -3343,8 +3343,6 @@ static int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev, > if(job) > drm_sched_increase_karma(&job->base); > > - > - > if (!amdgpu_sriov_vf(adev)) { > > if (!need_full_reset) > @@ -3482,8 +3480,7 @@ static int amdgpu_do_asic_reset(struct amdgpu_hive_info *hive, > return r; > } > > -static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev, > - struct amdgpu_job *job) > +static void amdgpu_device_post_asic_reset(struct amdgpu_device *adev) > { > int i; > > @@ -3623,7 +3620,7 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev, > > /* 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); > > if (r) { > /* bad news, how to tell it to userspace ? */ > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_dump.c b/drivers/gpu/drm/etnaviv/etnaviv_dump.c > index 33854c9..5778d9c 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_dump.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_dump.c > @@ -135,13 +135,11 @@ void etnaviv_core_dump(struct etnaviv_gpu *gpu) > mmu_size + gpu->buffer.size; > > /* Add in the active command buffers */ > - spin_lock_irqsave(&gpu->sched.job_list_lock, flags); > list_for_each_entry(s_job, &gpu->sched.ring_mirror_list, node) { > submit = to_etnaviv_submit(s_job); > file_size += submit->cmdbuf.size; > n_obj++; > } > - spin_unlock_irqrestore(&gpu->sched.job_list_lock, flags); > > /* Add in the active buffer objects */ > list_for_each_entry(vram, &gpu->mmu->mappings, mmu_node) { > @@ -183,14 +181,12 @@ void etnaviv_core_dump(struct etnaviv_gpu *gpu) > gpu->buffer.size, > etnaviv_cmdbuf_get_va(&gpu->buffer)); > > - spin_lock_irqsave(&gpu->sched.job_list_lock, flags); > list_for_each_entry(s_job, &gpu->sched.ring_mirror_list, node) { > submit = to_etnaviv_submit(s_job); > etnaviv_core_dump_mem(&iter, ETDUMP_BUF_CMD, > submit->cmdbuf.vaddr, submit->cmdbuf.size, > etnaviv_cmdbuf_get_va(&submit->cmdbuf)); > } > - spin_unlock_irqrestore(&gpu->sched.job_list_lock, flags); > > /* Reserve space for the bomap */ > if (n_bomap_pages) { > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c > index 6d24fea..a813c82 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); > + drm_sched_stop(&gpu->sched, sched_job); > > if(sched_job) > drm_sched_increase_karma(sched_job); > diff --git a/drivers/gpu/drm/lima/lima_sched.c b/drivers/gpu/drm/lima/lima_sched.c > index 97bd9c1..df98931 100644 > --- a/drivers/gpu/drm/lima/lima_sched.c > +++ b/drivers/gpu/drm/lima/lima_sched.c > @@ -300,7 +300,7 @@ static struct dma_fence *lima_sched_run_job(struct drm_sched_job *job) > static void lima_sched_handle_error_task(struct lima_sched_pipe *pipe, > struct lima_sched_task *task) > { > - drm_sched_stop(&pipe->base); > + drm_sched_stop(&pipe->base, &task->base); > > if (task) > drm_sched_increase_karma(&task->base); > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c > index 0a7ed04..c6336b7 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > @@ -385,7 +385,7 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job) > sched_job); > > for (i = 0; i < NUM_JOB_SLOTS; i++) > - drm_sched_stop(&pfdev->js->queue[i].sched); > + drm_sched_stop(&pfdev->js->queue[i].sched, sched_job); > > 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 19fc601..21e8734 100644 > --- a/drivers/gpu/drm/scheduler/sched_main.c > +++ b/drivers/gpu/drm/scheduler/sched_main.c > @@ -265,32 +265,6 @@ void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched, > } > EXPORT_SYMBOL(drm_sched_resume_timeout); > > -/* job_finish is called after hw fence signaled > - */ > -static void drm_sched_job_finish(struct work_struct *work) > -{ > - struct drm_sched_job *s_job = container_of(work, struct drm_sched_job, > - finish_work); > - struct drm_gpu_scheduler *sched = s_job->sched; > - 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. > - */ > - 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); > - > - sched->ops->free_job(s_job); > -} > - > static void drm_sched_job_begin(struct drm_sched_job *s_job) > { > struct drm_gpu_scheduler *sched = s_job->sched; > @@ -315,6 +289,13 @@ static void drm_sched_job_timedout(struct work_struct *work) > if (job) > job->sched->ops->timedout_job(job); > > + /* > + * Guilty job did complete and hence needs to be manually removed > + * See drm_sched_stop doc. > + */ > + if (list_empty(&job->node)) > + job->sched->ops->free_job(job); > + > spin_lock_irqsave(&sched->job_list_lock, flags); > drm_sched_start_timeout(sched); > spin_unlock_irqrestore(&sched->job_list_lock, flags); > @@ -371,23 +352,26 @@ EXPORT_SYMBOL(drm_sched_increase_karma); > * @sched: scheduler instance > * @bad: bad scheduler job > * > + * Stop the scheduler and also removes and frees all completed jobs. > + * Note: bad job will not be freed as it might be used later and so it's > + * callers responsibility to release it manually if it's not part of the > + * mirror list any more. > + * > */ > -void drm_sched_stop(struct drm_gpu_scheduler *sched) > +void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad) > { > - struct drm_sched_job *s_job; > + struct drm_sched_job *s_job, *tmp; > unsigned long flags; > - struct dma_fence *last_fence = NULL; > > 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. > + * Iterate the job list from later to earlier one and either deactive > + * their HW callbacks or remove them from mirror list if they already > + * signaled. > + * This iteration is thread safe as sched thread is stopped. > */ > - spin_lock_irqsave(&sched->job_list_lock, flags); > - list_for_each_entry_reverse(s_job, &sched->ring_mirror_list, node) { > + list_for_each_entry_safe_reverse(s_job, tmp, &sched->ring_mirror_list, node) { > if (s_job->s_fence->parent && > dma_fence_remove_callback(s_job->s_fence->parent, > &s_job->cb)) { > @@ -395,16 +379,30 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched) > s_job->s_fence->parent = NULL; > atomic_dec(&sched->hw_rq_count); > } else { > - last_fence = dma_fence_get(&s_job->s_fence->finished); > - break; > + /* > + * remove job from ring_mirror_list. > + * Locking here is for concurrent resume timeout > + */ > + spin_lock_irqsave(&sched->job_list_lock, flags); > + list_del_init(&s_job->node); > + spin_unlock_irqrestore(&sched->job_list_lock, flags); > + > + /* > + * Wait for job's HW fence callback to finish using s_job > + * before releasing it. > + * > + * Job is still alive so fence refcount at least 1 > + */ > + dma_fence_wait(&s_job->s_fence->finished, false); > + > + /* > + * We must keep bad job alive for later use during > + * recovery by some of the drivers > + */ > + if (bad != s_job) > + sched->ops->free_job(s_job); > } > } > - spin_unlock_irqrestore(&sched->job_list_lock, flags); > - > - if (last_fence) { > - dma_fence_wait(last_fence, false); > - dma_fence_put(last_fence); > - } > } > > EXPORT_SYMBOL(drm_sched_stop); > @@ -418,6 +416,7 @@ 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) > @@ -425,9 +424,7 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery) > > /* > * 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 > + * so no new jobs are being inserted or removed. Also concurrent > * GPU recovers can't run in parallel. > */ > list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) { > @@ -445,7 +442,9 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, bool full_recovery) > drm_sched_process_job(NULL, &s_job->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); > @@ -464,7 +463,6 @@ void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched) > 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; > > @@ -514,7 +512,6 @@ int drm_sched_job_init(struct drm_sched_job *job, > return -ENOMEM; > job->id = atomic64_inc_return(&sched->job_id_count); > > - INIT_WORK(&job->finish_work, drm_sched_job_finish); > INIT_LIST_HEAD(&job->node); > > return 0; > @@ -597,24 +594,53 @@ static void drm_sched_process_job(struct dma_fence *f, struct dma_fence_cb *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); > > 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); > + trace_drm_sched_process_job(s_fence); > > drm_sched_fence_finished(s_fence); > - > - trace_drm_sched_process_job(s_fence); > wake_up_interruptible(&sched->wake_up_worker); > +} > + > +/** > + * drm_sched_cleanup_jobs - destroy finished jobs > + * > + * @sched: scheduler instance > + * > + * Remove all finished jobs from the mirror list and destroy them. > + */ > +static void drm_sched_cleanup_jobs(struct drm_gpu_scheduler *sched) > +{ > + unsigned long flags; > + > + /* Don't destroy jobs while the timeout worker is running */ > + if (!cancel_delayed_work(&sched->work_tdr)) > + return; > + > + > + while (!list_empty(&sched->ring_mirror_list)) { > + struct drm_sched_job *job; > + > + job = list_first_entry(&sched->ring_mirror_list, > + struct drm_sched_job, node); > + if (!dma_fence_is_signaled(&job->s_fence->finished)) > + break; > + > + spin_lock_irqsave(&sched->job_list_lock, flags); > + /* remove job from ring_mirror_list */ > + list_del_init(&job->node); > + spin_unlock_irqrestore(&sched->job_list_lock, flags); > + > + sched->ops->free_job(job); > + } > + > + /* queue timeout for next job */ > + spin_lock_irqsave(&sched->job_list_lock, flags); > + drm_sched_start_timeout(sched); > + spin_unlock_irqrestore(&sched->job_list_lock, flags); > > - schedule_work(&s_job->finish_work); > } > > /** > @@ -656,9 +682,10 @@ static int drm_sched_main(void *param) > struct dma_fence *fence; > > wait_event_interruptible(sched->wake_up_worker, > + (drm_sched_cleanup_jobs(sched), > (!drm_sched_blocked(sched) && > (entity = drm_sched_select_entity(sched))) || > - kthread_should_stop()); > + kthread_should_stop())); > > if (!entity) > continue; > diff --git a/drivers/gpu/drm/v3d/v3d_sched.c b/drivers/gpu/drm/v3d/v3d_sched.c > index e740f3b..1a4abe7 100644 > --- a/drivers/gpu/drm/v3d/v3d_sched.c > +++ b/drivers/gpu/drm/v3d/v3d_sched.c > @@ -232,7 +232,7 @@ v3d_gpu_reset_for_timeout(struct v3d_dev *v3d, struct drm_sched_job *sched_job) > > /* block scheduler */ > for (q = 0; q < V3D_MAX_QUEUES; q++) > - drm_sched_stop(&v3d->queue[q].sched); > + drm_sched_stop(&v3d->queue[q].sched, sched_job); > > if (sched_job) > drm_sched_increase_karma(sched_job); > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > index 0daca4d..9ee0f27 100644 > --- a/include/drm/gpu_scheduler.h > +++ b/include/drm/gpu_scheduler.h > @@ -167,9 +167,6 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f); > * @sched: the scheduler instance on which this job is scheduled. > * @s_fence: contains the fences for the scheduling of job. > * @finish_cb: the callback for the finished fence. > - * @finish_work: schedules the function @drm_sched_job_finish once the job has > - * 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. > * @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 > @@ -188,7 +185,6 @@ struct drm_sched_job { > struct drm_gpu_scheduler *sched; > struct drm_sched_fence *s_fence; > struct dma_fence_cb finish_cb; > - struct work_struct finish_work; > struct list_head node; > uint64_t id; > atomic_t karma; > @@ -296,7 +292,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); > +void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad); > 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); _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
