Dying process might be blocked from receiving any more signals so avoid using it.
Also retire enity->fini_status and just check the SW queue, if it's not empty do the fallback cleanup.
Also handle entity->last_scheduled == NULL use case which happens when HW ring is already hangged whem a new entity tried to enqeue jobs.
Signed-off-by: Andrey Grodzovsky andrey.grodzovsky@amd.com --- drivers/gpu/drm/scheduler/gpu_scheduler.c | 47 ++++++++++++++++++++++--------- include/drm/gpu_scheduler.h | 1 - 2 files changed, 34 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c index 44d4807..4d038f9 100644 --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c @@ -135,7 +135,6 @@ int drm_sched_entity_init(struct drm_gpu_scheduler *sched, entity->rq = rq; entity->sched = sched; entity->guilty = guilty; - entity->fini_status = 0; entity->last_scheduled = NULL;
spin_lock_init(&entity->rq_lock); @@ -173,7 +172,8 @@ static bool drm_sched_entity_is_initialized(struct drm_gpu_scheduler *sched, static bool drm_sched_entity_is_idle(struct drm_sched_entity *entity) { rmb(); - if (spsc_queue_peek(&entity->job_queue) == NULL) + + if (entity->rq == NULL || spsc_queue_peek(&entity->job_queue) == NULL) return true;
return false; @@ -227,12 +227,16 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched, * The client will not queue more IBs during this fini, consume existing * queued IBs or discard them on SIGKILL */ - if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL) - entity->fini_status = -ERESTARTSYS; + if ((current->flags & PF_EXITING)) + wait_event_timeout(sched->job_scheduled, + drm_sched_entity_is_idle(entity), msecs_to_jiffies(1000)); else - entity->fini_status = wait_event_killable(sched->job_scheduled, - drm_sched_entity_is_idle(entity)); - drm_sched_entity_set_rq(entity, NULL); + wait_event_killable(sched->job_scheduled, drm_sched_entity_is_idle(entity)); + + + /* For killed process disable any more IBs enqueue right now */ + if ((current->flags & PF_EXITING) && (current->exit_code == SIGKILL)) + drm_sched_entity_set_rq(entity, NULL); } EXPORT_SYMBOL(drm_sched_entity_do_release);
@@ -247,7 +251,13 @@ EXPORT_SYMBOL(drm_sched_entity_do_release); void drm_sched_entity_cleanup(struct drm_gpu_scheduler *sched, struct drm_sched_entity *entity) { - if (entity->fini_status) { + + drm_sched_entity_set_rq(entity, NULL); + + /* Consumption of existing IBs wasn't completed. Forcefully + * remove them here. + */ + if (spsc_queue_peek(&entity->job_queue)) { struct drm_sched_job *job; int r;
@@ -267,12 +277,22 @@ void drm_sched_entity_cleanup(struct drm_gpu_scheduler *sched, struct drm_sched_fence *s_fence = job->s_fence; drm_sched_fence_scheduled(s_fence); dma_fence_set_error(&s_fence->finished, -ESRCH); - r = dma_fence_add_callback(entity->last_scheduled, &job->finish_cb, - drm_sched_entity_kill_jobs_cb); - if (r == -ENOENT) + + /* + * When pipe is hanged by older entity, new entity might + * not even have chance to submit it's first job to HW + * and so entity->last_scheduled will remain NULL + */ + if (!entity->last_scheduled) drm_sched_entity_kill_jobs_cb(NULL, &job->finish_cb); - else if (r) - DRM_ERROR("fence add callback failed (%d)\n", r); + else { + r = dma_fence_add_callback(entity->last_scheduled, &job->finish_cb, + drm_sched_entity_kill_jobs_cb); + if (r == -ENOENT) + drm_sched_entity_kill_jobs_cb(NULL, &job->finish_cb); + else if (r) + DRM_ERROR("fence add callback failed (%d)\n", r); + } } }
@@ -713,6 +733,7 @@ static int drm_sched_main(void *param) continue;
sched_job = drm_sched_entity_pop_job(entity); + if (!sched_job) continue;
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index dec6558..d220ac9 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -64,7 +64,6 @@ struct drm_sched_entity { struct dma_fence *dependency; struct dma_fence_cb cb; atomic_t *guilty; /* points to ctx's guilty */ - int fini_status; struct dma_fence *last_scheduled; };
With this we can now terminate jobs enqueue into SW queue the moment the task is being killed instead of waiting for last user of drm file to release it.
Also stop checking for kref_read(&ctx->refcount) == 1 when calling drm_sched_entity_do_release since other task might still hold a reference to this entity but we don't care since KILL means terminate job submission regardless of what other tasks are doing.
Signed-off-by: Andrey Grodzovsky andrey.grodzovsky@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 13 +++++++------ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 ++++++++++++ drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 1 - 3 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c index c5bb362..db69045 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c @@ -452,23 +452,24 @@ void amdgpu_ctx_mgr_entity_fini(struct amdgpu_ctx_mgr *mgr)
idp = &mgr->ctx_handles;
+ mutex_lock(&mgr->lock); idr_for_each_entry(idp, ctx, id) {
- if (!ctx->adev) + if (!ctx->adev) { + mutex_unlock(&mgr->lock); return; + }
for (i = 0; i < ctx->adev->num_rings; i++) {
if (ctx->adev->rings[i] == &ctx->adev->gfx.kiq.ring) continue;
- if (kref_read(&ctx->refcount) == 1) - drm_sched_entity_do_release(&ctx->adev->rings[i]->sched, - &ctx->rings[i].entity); - else - DRM_ERROR("ctx %p is still alive\n", ctx); + drm_sched_entity_do_release(&ctx->adev->rings[i]->sched, + &ctx->rings[i].entity); } } + mutex_unlock(&mgr->lock); }
void amdgpu_ctx_mgr_entity_cleanup(struct amdgpu_ctx_mgr *mgr) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index b0bf2f2..36a9acf 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -855,9 +855,21 @@ static const struct dev_pm_ops amdgpu_pm_ops = { .runtime_idle = amdgpu_pmops_runtime_idle, };
+static int amdgpu_flush(struct file *f, fl_owner_t id) +{ + struct drm_file *file_priv = f->private_data; + struct amdgpu_fpriv *fpriv = file_priv->driver_priv; + + amdgpu_ctx_mgr_entity_fini(&fpriv->ctx_mgr); + + return 0; +} + + static const struct file_operations amdgpu_driver_kms_fops = { .owner = THIS_MODULE, .open = drm_open, + .flush = amdgpu_flush, .release = drm_release, .unlocked_ioctl = amdgpu_drm_ioctl, .mmap = amdgpu_mmap, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index ca21549..1239384 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -930,7 +930,6 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev, return;
pm_runtime_get_sync(dev->dev); - amdgpu_ctx_mgr_entity_fini(&fpriv->ctx_mgr);
if (adev->asic_type != CHIP_RAVEN) { amdgpu_uvd_free_handles(adev, file_priv);
Am 30.05.2018 um 21:54 schrieb Andrey Grodzovsky:
Dying process might be blocked from receiving any more signals so avoid using it.
Also retire enity->fini_status and just check the SW queue, if it's not empty do the fallback cleanup.
Also handle entity->last_scheduled == NULL use case which happens when HW ring is already hangged whem a new entity tried to enqeue jobs.
Signed-off-by: Andrey Grodzovsky andrey.grodzovsky@amd.com
Would be nice to have to split that patch up further, but not sure if that is actually doable.
Additional to that a few more nitpicks below.
drivers/gpu/drm/scheduler/gpu_scheduler.c | 47 ++++++++++++++++++++++--------- include/drm/gpu_scheduler.h | 1 - 2 files changed, 34 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c index 44d4807..4d038f9 100644 --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c @@ -135,7 +135,6 @@ int drm_sched_entity_init(struct drm_gpu_scheduler *sched, entity->rq = rq; entity->sched = sched; entity->guilty = guilty;
entity->fini_status = 0; entity->last_scheduled = NULL;
spin_lock_init(&entity->rq_lock);
@@ -173,7 +172,8 @@ static bool drm_sched_entity_is_initialized(struct drm_gpu_scheduler *sched, static bool drm_sched_entity_is_idle(struct drm_sched_entity *entity) { rmb();
- if (spsc_queue_peek(&entity->job_queue) == NULL)
- if (entity->rq == NULL || spsc_queue_peek(&entity->job_queue) == NULL)
Usual kernel coding style is more to use "!entity->rq" instead of "entity->rq == NULL".
return true;return false; @@ -227,12 +227,16 @@ void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched, * The client will not queue more IBs during this fini, consume existing * queued IBs or discard them on SIGKILL */
- if ((current->flags & PF_SIGNALED) && current->exit_code == SIGKILL)
entity->fini_status = -ERESTARTSYS;
- if ((current->flags & PF_EXITING))
wait_event_timeout(sched->job_scheduled,drm_sched_entity_is_idle(entity), msecs_to_jiffies(1000));
Mhm, making the timeout a parameter or at least use a define would be nice to have.
Additional to that it might be a good idea to return the remaining timeout and use that as parameter for the next call.
E.g. this way when we need to cleanup multiple queues we don't wait for 1000 ms each, but rather only 1000 ms in total.
else
entity->fini_status = wait_event_killable(sched->job_scheduled,drm_sched_entity_is_idle(entity));- drm_sched_entity_set_rq(entity, NULL);
wait_event_killable(sched->job_scheduled, drm_sched_entity_is_idle(entity));- /* For killed process disable any more IBs enqueue right now */
- if ((current->flags & PF_EXITING) && (current->exit_code == SIGKILL))
drm_sched_entity_set_rq(entity, NULL);@@ -247,7 +251,13 @@ EXPORT_SYMBOL(drm_sched_entity_do_release); void drm_sched_entity_cleanup(struct drm_gpu_scheduler *sched, struct drm_sched_entity *entity) {
- if (entity->fini_status) {
- drm_sched_entity_set_rq(entity, NULL);
- /* Consumption of existing IBs wasn't completed. Forcefully
* remove them here.*/- if (spsc_queue_peek(&entity->job_queue)) { struct drm_sched_job *job; int r;
@@ -267,12 +277,22 @@ void drm_sched_entity_cleanup(struct drm_gpu_scheduler *sched, struct drm_sched_fence *s_fence = job->s_fence; drm_sched_fence_scheduled(s_fence); dma_fence_set_error(&s_fence->finished, -ESRCH);
r = dma_fence_add_callback(entity->last_scheduled, &job->finish_cb,drm_sched_entity_kill_jobs_cb);if (r == -ENOENT)
/** When pipe is hanged by older entity, new entity might* not even have chance to submit it's first job to HW* and so entity->last_scheduled will remain NULL*/if (!entity->last_scheduled) drm_sched_entity_kill_jobs_cb(NULL, &job->finish_cb);
else if (r)DRM_ERROR("fence add callback failed (%d)\n", r);
else {
Coding style nit pick when one side of an else has { or } the other side should have it as well.
r = dma_fence_add_callback(entity->last_scheduled, &job->finish_cb,drm_sched_entity_kill_jobs_cb);if (r == -ENOENT)drm_sched_entity_kill_jobs_cb(NULL, &job->finish_cb);else if (r)DRM_ERROR("fence add callback failed (%d)\n", r);}@@ -713,6 +733,7 @@ static int drm_sched_main(void *param) continue;
sched_job = drm_sched_entity_pop_job(entity);
Unnecessary blank line added.
Apart from that thanks a lot for taking care of that, that is really appreciated.
Thanks, Christian.
if (!sched_job) continue;diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index dec6558..d220ac9 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -64,7 +64,6 @@ struct drm_sched_entity { struct dma_fence *dependency; struct dma_fence_cb cb; atomic_t *guilty; /* points to ctx's guilty */
- int fini_status; struct dma_fence *last_scheduled; };
dri-devel@lists.freedesktop.org
-
Andrey Grodzovsky -
Christian König