Am 03.08.2018 um 19:31 schrieb Lucas Stach:
> Am Montag, den 06.08.2018, 14:57 +0200 schrieb Christian König:
>> Am 03.08.2018 um 16:29 schrieb Lucas Stach:
>>> drm_sched_job_finish() is a work item scheduled for each finished job on
>>> a unbound system workqueue. This means the workers can execute out of order
>>> with regard to the real hardware job completions.
>>>
>>> If this happens queueing a timeout worker for the first job on the ring
>>> mirror list is wrong, as this may be a job which has already finished
>>> executing. Fix this by reorganizing the code to always queue the worker
>>> for the next job on the list, if this job hasn't finished yet. This is
>>> robust against a potential reordering of the finish workers.
>>>
>>> Also move out the timeout worker cancelling, so that we don't need to
>>> take the job list lock twice. As a small optimization list_del is used
>>> to remove the job from the ring mirror list, as there is no need to
>>> reinit the list head in the job we are about to free.
>>>
>>>>> Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
>>> ---
>>> v2: - properly handle last job in the ring
>>> Â Â Â Â Â - check correct fence for compeletion
>>> ---
>>> Â drivers/gpu/drm/scheduler/gpu_scheduler.c | 22 ++++++++++------------
>>> Â 1 file changed, 10 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler.c b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>> index 44d480768dfe..574875e2c206 100644
>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler.c
>>> @@ -452,24 +452,22 @@ static void drm_sched_job_finish(struct work_struct *work)
>>>>> Â Â Â Â Â finish_work);
>>>>> Â Â struct drm_gpu_scheduler *sched = s_job->sched;
>>>
>>>>> - /* remove job from ring_mirror_list */
>>>>> - spin_lock(&sched->job_list_lock);
>>>>> - list_del_init(&s_job->node);
>>>>> - if (sched->timeout != MAX_SCHEDULE_TIMEOUT) {
>>>>> - struct drm_sched_job *next;
>>> -
>>>>> - spin_unlock(&sched->job_list_lock);
>>>>> + if (sched->timeout != MAX_SCHEDULE_TIMEOUT)
>>> Â Â cancel_delayed_work_sync(&s_job->work_tdr);
>> That is unfortunately still racy here.
>>
>> Between canceling the job and removing it from the list someone could
>> actually start the time (in theory) :)
>>
>> Cancel it, remove it from the list and cancel it again.
> I don't see how. If we end up in this worker the finished fence of the
> job is already certainly signaled (as this is what triggers queueing of
> the worker). So even if some other worker manages to find this job as
> the next job in the list, the dma_fence_is_signaled check should
> prevent the timeout worker from getting scheduled again.
Well that makes sense, but is a bit hard to understand.
Anyway, please remove the extra "if" check. With that done the patch is
Reviewed-by: Christian König <christian.koenig at amd.com>.
Thanks,
Christian.
>
>> BTW: You could completely drop the "if (sched->timeout !=
>> MAX_SCHEDULE_TIMEOUT)" here cause canceling is harmless as long as the
>> structure is initialized.
> Right.
>
> Regards,
> Lucas
>
>> Christian.
>>
>>>>> - spin_lock(&sched->job_list_lock);
>>>
>>>>> - /* queue TDR for next job */
>>>>> - next = list_first_entry_or_null(&sched->ring_mirror_list,
>>>>> - struct drm_sched_job, node);
>>>>> + 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 (next)
>>>>> + 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);
>>>>> Â Â spin_unlock(&sched->job_list_lock);
>>> +
>>>>> Â Â dma_fence_put(&s_job->s_fence->finished);
>>>>> Â Â sched->ops->free_job(s_job);
>>> Â }
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
