On 06/01/2018 01:22 PM, Christian König wrote:
> Am 01.06.2018 um 19:11 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.
>>
>> v2:
>> Return the remaining timeout and use that as parameter for the next
>> call.
>> This way when we need to cleanup multiple queues we don't wait for the
>> entire TO period for each queue but rather in total.
>> Styling comments.
>> Rebase.
>>
>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
>> ---
>> Â drivers/gpu/drm/scheduler/gpu_scheduler.c | 74
>> ++++++++++++++++++++++++-------
>>  include/drm/gpu_scheduler.h              | 7 +--
>> Â 2 files changed, 61 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> index 8c1e80c..c594d17 100644
>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>> @@ -181,7 +181,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);
>> @@ -219,7 +218,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 || spsc_queue_peek(&entity->job_queue) == NULL)
>> Â Â Â Â Â Â Â Â Â return true;
>> Â Â Â Â Â Â return false;
>> @@ -260,25 +260,48 @@ static void
>> drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
>> Â Â *
>> Â Â * @sched: scheduler instance
>> Â Â * @entity: scheduler entity
>> + * @timeout: time to wait in ms for Q to become empty.
>> Â Â *
>> Â Â * Splitting drm_sched_entity_fini() into two functions, The first
>> one does the waiting,
>> Â Â * removes the entity from the runqueue and returns an error when
>> the process was killed.
>> + *
>> + * Returns amount of time spent in waiting for TO.
>> + * 0 if wait wasn't with time out.
>> + * MAX_WAIT_SCHED_ENTITY_Q_EMPTY_MS if wait timed out with condition
>> false
>> + * Number of MS spent in waiting before condition became true
>> + *
>> Â Â */
>> -void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_sched_entity *entity)
>> +unsigned drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_sched_entity *entity, unsigned timeout)
>
> Better use long for return type and timeout.
>
>> Â {
>> +Â Â Â unsigned ret = 0;
>
> Also use a long here and initialize it with timeout.
Please see bellow
>
>> +
>> Â Â Â Â Â if (!drm_sched_entity_is_initialized(sched, entity))
>> Â Â Â Â Â Â Â Â Â return;
>> Â Â Â Â Â /**
>> Â Â Â Â Â Â * 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;
>> -Â Â Â else
>> -Â Â Â Â Â Â Â entity->fini_status = wait_event_killable(sched->job_scheduled,
>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â drm_sched_entity_is_idle(entity));
>> -Â Â Â drm_sched_entity_set_rq(entity, NULL);
>> +Â Â Â if (current->flags & PF_EXITING) {
>> +Â Â Â Â Â Â Â if (timeout) {
>> +Â Â Â Â Â Â Â Â Â Â Â ret = jiffies_to_msecs(
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â wait_event_timeout(
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â sched->job_scheduled,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â drm_sched_entity_is_idle(entity),
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â msecs_to_jiffies(timeout)));
>
> Oh please don't use msecs as timeout, just use jiffies and let the
> caller do the conversion.
>
>> +
>> +Â Â Â Â Â Â Â Â Â Â Â if (!ret)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â ret = MAX_WAIT_SCHED_ENTITY_Q_EMPTY_MS;
>
> Why that? It is common coding style to return 0 when a timeout occurs.
>
> Christian.
What should i return when i do wait_event_killable, it's return values
are opposite to wait_event_timeout...
This way returning 0 has no impact on remaining waiting time, dong it
the other way will force the caller
to do some cumbersome logic instead of just
max_wait = max_wait >= ret ? max_wait - ret : 0;
like in amdgpu_ctx_mgr_entity_fini
Andrey
>
>> +Â Â Â Â Â Â Â }
>> +Â Â Â } else
>> +Â Â Â Â Â Â Â 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);
>> +
>> +Â Â Â return ret;
>> Â }
>> Â EXPORT_SYMBOL(drm_sched_entity_do_release);
>> Â @@ -290,11 +313,18 @@ EXPORT_SYMBOL(drm_sched_entity_do_release);
>> Â Â *
>> Â Â * This should be called after @drm_sched_entity_do_release. It
>> goes over the
>> Â Â * entity and signals all jobs with an error code if the process
>> was killed.
>> + *
>> Â Â */
>> Â 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;
>> Â @@ -314,12 +344,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);
>> +Â Â Â Â Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â Â Â }
>> Â Â Â Â Â }
>> Â @@ -339,7 +379,7 @@ EXPORT_SYMBOL(drm_sched_entity_cleanup);
>> Â void drm_sched_entity_fini(struct drm_gpu_scheduler *sched,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_sched_entity *entity)
>> Â {
>> -Â Â Â drm_sched_entity_do_release(sched, entity);
>> +Â Â Â drm_sched_entity_do_release(sched, entity,
>> MAX_WAIT_SCHED_ENTITY_Q_EMPTY_MS);
>> Â Â Â Â Â drm_sched_entity_cleanup(sched, entity);
>> Â }
>> Â EXPORT_SYMBOL(drm_sched_entity_fini);
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 496442f..af07875 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -27,6 +27,8 @@
>> Â #include <drm/spsc_queue.h>
>> Â #include <linux/dma-fence.h>
>> Â +#define MAX_WAIT_SCHED_ENTITY_Q_EMPTY_MS 1000
>
> I suggest to use msecs_to_jiffies(1000) here and drop the _MS postfix.
>
> Christian.
>
>> +
>> Â struct drm_gpu_scheduler;
>> Â struct drm_sched_rq;
>> Â @@ -84,7 +86,6 @@ struct drm_sched_entity {
>>      struct dma_fence       *dependency;
>>      struct dma_fence_cb       cb;
>>      atomic_t           *guilty;
>> -   int                            fini_status;
>>      struct dma_fence               *last_scheduled;
>> Â };
>> Â @@ -283,8 +284,8 @@ int drm_sched_entity_init(struct
>> drm_gpu_scheduler *sched,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_sched_entity *entity,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_sched_rq *rq,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â atomic_t *guilty);
>> -void drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_sched_entity *entity);
>> +unsigned drm_sched_entity_do_release(struct drm_gpu_scheduler *sched,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_sched_entity *entity, unsigned timeout);
>> Â void drm_sched_entity_cleanup(struct drm_gpu_scheduler *sched,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct drm_sched_entity *entity);
>> Â void drm_sched_entity_fini(struct drm_gpu_scheduler *sched,
>
