[v5,1/2] drm/sched: Refactor ring mirror list handling.

Submitted by Grodzovsky, Andrey on Jan. 9, 2019, 3:18 p.m.

Details

Message ID ea37c55f-ee56-cce7-a4e1-1a599642c54c@amd.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Grodzovsky, Andrey Jan. 9, 2019, 3:18 p.m.
On 01/09/2019 05:22 AM, Christian König wrote:
> Am 07.01.19 um 20:47 schrieb Grodzovsky, Andrey:

>>

>> On 01/07/2019 09:13 AM, Christian König wrote:

>>> Am 03.01.19 um 18:42 schrieb Grodzovsky, Andrey:

>>>> On 01/03/2019 11:20 AM, Grodzovsky, Andrey wrote:

>>>>> On 01/03/2019 03:54 AM, Koenig, Christian wrote:

>>>>>> Am 21.12.18 um 21:36 schrieb Grodzovsky, Andrey:

>>>>>>> On 12/21/2018 01:37 PM, Christian König wrote:

>>>>>>>> Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:

>>>>>>>>> Decauple sched threads stop and start and ring mirror

>>>>>>>>> list handling from the policy of what to do about the

>>>>>>>>> guilty jobs.

>>>>>>>>> When stoppping the sched thread and detaching sched fences

>>>>>>>>> from non signaled HW fenes wait for all signaled HW fences

>>>>>>>>> to complete before rerunning the jobs.

>>>>>>>>>

>>>>>>>>> v2: Fix resubmission of guilty job into HW after refactoring.

>>>>>>>>>

>>>>>>>>> v4:

>>>>>>>>> Full restart for all the jobs, not only from guilty ring.

>>>>>>>>> Extract karma increase into standalone function.

>>>>>>>>>

>>>>>>>>> v5:

>>>>>>>>> Rework waiting for signaled jobs without relying on the job

>>>>>>>>> struct itself as those might already be freed for non 'guilty'

>>>>>>>>> job's schedulers.

>>>>>>>>> Expose karma increase to drivers.

>>>>>>>>>

>>>>>>>>> Suggested-by: Christian Koenig <Christian.Koenig@amd.com>

>>>>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>

>>>>>>>>> ---

>>>>>>>>>        drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  18 +--

>>>>>>>>>        drivers/gpu/drm/etnaviv/etnaviv_sched.c |  11 +-

>>>>>>>>>        drivers/gpu/drm/scheduler/sched_main.c | 188

>>>>>>>>> +++++++++++++++++++----------

>>>>>>>>>        drivers/gpu/drm/v3d/v3d_sched.c |  12 +-

>>>>>>>>>        include/drm/gpu_scheduler.h |  10 +-

>>>>>>>>>        5 files changed, 151 insertions(+), 88 deletions(-)

>>>>>>>>>

>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c

>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c

>>>>>>>>> index 8a078f4..a4bd2d3 100644

>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c

>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c

>>>>>>>>> @@ -3298,12 +3298,10 @@ static int

>>>>>>>>> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,

>>>>>>>>>                if (!ring || !ring->sched.thread)

>>>>>>>>>                    continue;

>>>>>>>>>        - kthread_park(ring->sched.thread);

>>>>>>>>> +        drm_sched_stop(&ring->sched, job ? &job->base : NULL);

>>>>>>>>>        -        if (job && job->base.sched != &ring->sched)

>>>>>>>>> -            continue;

>>>>>>>>> -

>>>>>>>>> - drm_sched_hw_job_reset(&ring->sched, job ? &job->base :

>>>>>>>>> NULL);

>>>>>>>>> +        if(job)

>>>>>>>>> + drm_sched_increase_karma(&job->base);

>>>>>>>> Since we dropped the "job && job->base.sched != &ring->sched" 

>>>>>>>> check

>>>>>>>> above this will now increase the jobs karma multiple times.

>>>>>>>>

>>>>>>>> Maybe just move that outside of the loop.

>>>>>>>>

>>>>>>>>>                  /* after all hw jobs are reset, hw fence is

>>>>>>>>> meaningless,

>>>>>>>>> so force_completion */

>>>>>>>>> amdgpu_fence_driver_force_completion(ring);

>>>>>>>>> @@ -3454,14 +3452,10 @@ static void

>>>>>>>>> amdgpu_device_post_asic_reset(struct amdgpu_device *adev,

>>>>>>>>>                if (!ring || !ring->sched.thread)

>>>>>>>>>                    continue;

>>>>>>>>>        -        /* only need recovery sched of the given job's 

>>>>>>>>> ring

>>>>>>>>> -         * or all rings (in the case @job is NULL)

>>>>>>>>> -         * after above amdgpu_reset accomplished

>>>>>>>>> -         */

>>>>>>>>> -        if ((!job || job->base.sched == &ring->sched) &&

>>>>>>>>> !adev->asic_reset_res)

>>>>>>>>> - drm_sched_job_recovery(&ring->sched);

>>>>>>>>> +        if (!adev->asic_reset_res)

>>>>>>>>> + drm_sched_resubmit_jobs(&ring->sched);

>>>>>>>>>        - kthread_unpark(ring->sched.thread);

>>>>>>>>> +        drm_sched_start(&ring->sched, !adev->asic_reset_res);

>>>>>>>>>            }

>>>>>>>>>              if (!amdgpu_device_has_dc_support(adev)) {

>>>>>>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c

>>>>>>>>> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c

>>>>>>>>> index 49a6763..6f1268f 100644

>>>>>>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c

>>>>>>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c

>>>>>>>>> @@ -109,16 +109,19 @@ static void 

>>>>>>>>> etnaviv_sched_timedout_job(struct

>>>>>>>>> drm_sched_job *sched_job)

>>>>>>>>>            }

>>>>>>>>>              /* block scheduler */

>>>>>>>>> -    kthread_park(gpu->sched.thread);

>>>>>>>>> -    drm_sched_hw_job_reset(&gpu->sched, sched_job);

>>>>>>>>> +    drm_sched_stop(&gpu->sched, sched_job);

>>>>>>>>> +

>>>>>>>>> +    if(sched_job)

>>>>>>>>> +        drm_sched_increase_karma(sched_job);

>>>>>>>>>              /* get the GPU back into the init state */

>>>>>>>>>            etnaviv_core_dump(gpu);

>>>>>>>>>            etnaviv_gpu_recover_hang(gpu);

>>>>>>>>>        + drm_sched_resubmit_jobs(&gpu->sched);

>>>>>>>>> +

>>>>>>>>>            /* restart scheduler after GPU is usable again */

>>>>>>>>> -    drm_sched_job_recovery(&gpu->sched);

>>>>>>>>> -    kthread_unpark(gpu->sched.thread);

>>>>>>>>> +    drm_sched_start(&gpu->sched, true);

>>>>>>>>>        }

>>>>>>>>>          static void etnaviv_sched_free_job(struct drm_sched_job

>>>>>>>>> *sched_job)

>>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c

>>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c

>>>>>>>>> index dbb6906..b5c5bee 100644

>>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c

>>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c

>>>>>>>>> @@ -60,8 +60,6 @@

>>>>>>>>>          static void drm_sched_process_job(struct dma_fence *f,

>>>>>>>>> struct

>>>>>>>>> dma_fence_cb *cb);

>>>>>>>>>        -static void drm_sched_expel_job_unlocked(struct

>>>>>>>>> drm_sched_job

>>>>>>>>> *s_job);

>>>>>>>>> -

>>>>>>>>>        /**

>>>>>>>>>         * drm_sched_rq_init - initialize a given run queue struct

>>>>>>>>>         *

>>>>>>>>> @@ -335,6 +333,42 @@ static void drm_sched_job_timedout(struct

>>>>>>>>> work_struct *work)

>>>>>>>>> spin_unlock_irqrestore(&sched->job_list_lock, flags);

>>>>>>>>>        }

>>>>>>>> Kernel doc here would be nice to have.

>>>>>>>>

>>>>>>>>> +void drm_sched_increase_karma(struct drm_sched_job *bad)

>>>>>>>>> +{

>>>>>>>>> +    int i;

>>>>>>>>> +    struct drm_sched_entity *tmp;

>>>>>>>>> +    struct drm_sched_entity *entity;

>>>>>>>>> +    struct drm_gpu_scheduler *sched = bad->sched;

>>>>>>>>> +

>>>>>>>>> +    /* don't increase @bad's karma if it's from KERNEL RQ,

>>>>>>>>> +     * because sometimes GPU hang would cause kernel jobs 

>>>>>>>>> (like VM

>>>>>>>>> updating jobs)

>>>>>>>>> +     * corrupt but keep in mind that kernel jobs always 

>>>>>>>>> considered

>>>>>>>>> good.

>>>>>>>>> +     */

>>>>>>>>> +    if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {

>>>>>>>>> +        atomic_inc(&bad->karma);

>>>>>>>>> +        for (i = DRM_SCHED_PRIORITY_MIN; i <

>>>>>>>>> DRM_SCHED_PRIORITY_KERNEL;

>>>>>>>>> +             i++) {

>>>>>>>>> +            struct drm_sched_rq *rq = &sched->sched_rq[i];

>>>>>>>>> +

>>>>>>>>> +            spin_lock(&rq->lock);

>>>>>>>>> +            list_for_each_entry_safe(entity, tmp, &rq->entities,

>>>>>>>>> list) {

>>>>>>>>> +                if (bad->s_fence->scheduled.context ==

>>>>>>>>> +                    entity->fence_context) {

>>>>>>>>> +                    if (atomic_read(&bad->karma) >

>>>>>>>>> + bad->sched->hang_limit)

>>>>>>>>> +                        if (entity->guilty)

>>>>>>>>> + atomic_set(entity->guilty, 1);

>>>>>>>>> +                    break;

>>>>>>>>> +                }

>>>>>>>>> +            }

>>>>>>>>> +            spin_unlock(&rq->lock);

>>>>>>>>> +            if (&entity->list != &rq->entities)

>>>>>>>>> +                break;

>>>>>>>>> +        }

>>>>>>>>> +    }

>>>>>>>>> +}

>>>>>>>>> +EXPORT_SYMBOL(drm_sched_increase_karma);

>>>>>>>>> +

>>>>>>>>>        /**

>>>>>>>>>         * drm_sched_hw_job_reset - stop the scheduler if it

>>>>>>>>> contains the

>>>>>>>>> bad job

>>>>>>>>>         *

>>>>>>>>> @@ -342,13 +376,22 @@ static void drm_sched_job_timedout(struct

>>>>>>>>> work_struct *work)

>>>>>>>>>         * @bad: bad scheduler job

>>>>>>>>>         *

>>>>>>>>>         */

>>>>>>>>> -void drm_sched_hw_job_reset(struct drm_gpu_scheduler *sched,

>>>>>>>>> struct

>>>>>>>>> drm_sched_job *bad)

>>>>>>>>> +void drm_sched_stop(struct drm_gpu_scheduler *sched, struct

>>>>>>>>> drm_sched_job *bad)

>>>>>>>>>        {

>>>>>>>>> -    struct drm_sched_job *s_job;

>>>>>>>>> -    struct drm_sched_entity *entity, *tmp;

>>>>>>>>> +    struct drm_sched_job *s_job, *last_job;

>>>>>>>>>            unsigned long flags;

>>>>>>>>> -    int i;

>>>>>>>>> +    struct dma_fence *wait_fence =  NULL;

>>>>>>>>> +    int r;

>>>>>>>>> +

>>>>>>>>> +    kthread_park(sched->thread);

>>>>>>>>>        +    /*

>>>>>>>>> +     * Verify all the signaled jobs in mirror list are removed

>>>>>>>>> from

>>>>>>>>> the ring

>>>>>>>>> +     * by waiting for their respective scheduler fences to 

>>>>>>>>> signal.

>>>>>>>>> +     * Continually  repeat traversing the ring mirror list

>>>>>>>>> until no

>>>>>>>>> more signaled

>>>>>>>>> +     * fences are found

>>>>>>>>> +     */

>>>>>>>>> +retry_wait:

>>>>>>>>> spin_lock_irqsave(&sched->job_list_lock, flags);

>>>>>>>>>            list_for_each_entry_reverse(s_job,

>>>>>>>>> &sched->ring_mirror_list,

>>>>>>>>> node) {

>>>>>>>>>                if (s_job->s_fence->parent &&

>>>>>>>>> @@ -357,35 +400,45 @@ void drm_sched_hw_job_reset(struct

>>>>>>>>> drm_gpu_scheduler *sched, struct drm_sched_jo

>>>>>>>>> dma_fence_put(s_job->s_fence->parent);

>>>>>>>>>                    s_job->s_fence->parent = NULL;

>>>>>>>>> atomic_dec(&sched->hw_rq_count);

>>>>>>>>> +        } else {

>>>>>>>>> +             wait_fence =

>>>>>>>>> dma_fence_get(&s_job->s_fence->finished);

>>>>>>>>> +             last_job = s_job;

>>>>>>>>> +             break;

>>>>>>>>>                }

>>>>>>>>>            }

>>>>>>>>> - spin_unlock_irqrestore(&sched->job_list_lock, flags);

>>>>>>>>>        -    if (bad && bad->s_priority !=

>>>>>>>>> DRM_SCHED_PRIORITY_KERNEL) {

>>>>>>>>> -        atomic_inc(&bad->karma);

>>>>>>>>> -        /* don't increase @bad's karma if it's from KERNEL RQ,

>>>>>>>>> -         * becuase sometimes GPU hang would cause kernel jobs

>>>>>>>>> (like

>>>>>>>>> VM updating jobs)

>>>>>>>>> -         * corrupt but keep in mind that kernel jobs always

>>>>>>>>> considered good.

>>>>>>>>> -         */

>>>>>>>>> -        for (i = DRM_SCHED_PRIORITY_MIN; i <

>>>>>>>>> DRM_SCHED_PRIORITY_KERNEL; i++ ) {

>>>>>>>>> -            struct drm_sched_rq *rq = &sched->sched_rq[i];

>>>>>>>>> +    /* No signaled jobs in the ring, its safe to proceed to ASIC

>>>>>>>>> reset */

>>>>>>>>> +    if (!wait_fence) {

>>>>>>>>> + spin_unlock_irqrestore(&sched->job_list_lock, flags);

>>>>>>>>> +        goto done;

>>>>>>>>> +    }

>>>>>>>>>        -            spin_lock(&rq->lock);

>>>>>>>>> -            list_for_each_entry_safe(entity, tmp, &rq->entities,

>>>>>>>>> list) {

>>>>>>>>> -                if (bad->s_fence->scheduled.context ==

>>>>>>>>> entity->fence_context) {

>>>>>>>>> -                    if (atomic_read(&bad->karma) >

>>>>>>>>> bad->sched->hang_limit)

>>>>>>>>> -                        if (entity->guilty)

>>>>>>>>> - atomic_set(entity->guilty, 1);

>>>>>>>>> -                    break;

>>>>>>>>> -                }

>>>>>>>>> -            }

>>>>>>>>> -            spin_unlock(&rq->lock);

>>>>>>>>> -            if (&entity->list != &rq->entities)

>>>>>>>>> -                break;

>>>>>>>>> +    /* Restore removed cb since removing again already removed

>>>>>>>>> cb is

>>>>>>>>> undefined */

>>>>>>>>> +    list_for_each_entry_reverse(s_job, &sched->ring_mirror_list,

>>>>>>>>> node) {

>>>>>>>>> +        if(s_job == last_job)

>>>>>>>>> +            break;

>>>>>>>> Need to double check after the holidays, but you should be able

>>>>>>>> to use

>>>>>>>> list_for_each_entry_continue here.

>>>>>>> I think it should work - kind of traversing back on all the jobs

>>>>>>> we just

>>>>>>> removed their callbacks.

>>>>>> Wrapping your head around stuff again after the holidays sometimes

>>>>>> shows

>>>>>> new problems we have completely missed :)

>>>>>>

>>>>>> Adding the callbacks again won't work because we have freed up our

>>>>>> reference to the hardware fence above:

>>>>>>> dma_fence_put(s_job->s_fence->parent);

>>>>>>>                    s_job->s_fence->parent = NULL;

>>>>>> We need to drop this or we would never be able to re-add the fences

>>>>> Yea, that a big miss on my side...

>>>>>

>>>>>> .

>>>>>>

>>>>>> But I'm still not sure if we shouldn't rely on the correct order of

>>>>>> signaling and simplify this instead.

>>>>> As you said before, once we switched to signaling the parent from the

>>>>> interrupt context instead of scheduled work no danger of race there

>>>> Correction here - once we switched removing the job from mirror_ring

>>>> list directly in interrupt context instead later from scheduled work

>>> Ok, so let's stick with the approach of only waiting for the first

>>> signaled one found.

>>>

>>> But we need to remove setting the parent fence to NULL or otherwise we

>>> won't be able to add the callback ever again.

>>>

>>> Christian.

>> But we will not be adding the cb back in drm_sched_stop anymore, now we

>> are only going to add back the cb in drm_sched_startr after rerunning

>> those jobs in drm_sched_resubmit_jobs and assign them a new parent there

>> anyway.

>

> Yeah, but when we find that we don't need to reset anything anymore 

> then adding the callbacks again won't be possible any more.

>

> Christian.


I am not sure I understand it, can u point me to example of how this 
will happen ? I am attaching my latest patches with waiting only for the 
last job's fence here just so we are on same page regarding the code.

Andrey

>

>>

>> Andrey

>>

>>>> Andrey

>>>>

>>>>> , so

>>>>> what if we submit job A and after it job B and B completes before A

>>>>> (like the sync dependency test in libdrm amdgpu tests but without

>>>>> adding

>>>>> explicit dependency to the second command on the first) I believe 

>>>>> that

>>>>> still in this case job B's parent (HW) fence will not be signaled

>>>>> before

>>>>> job A completes since EOP event is not signaled until the entire pipe

>>>>> completed and flushed it's cashes including job A. So from this

>>>>> seems to

>>>>> me that indeed it's enough to wait for the last inserted job's parent

>>>>> (HW) fence in ring mirror list to signal.

>>>>> Let me know what you think on that.

>>>>>

>>>>> P.S V5 is not the last iteration and there was V6 series.

>>>>>

>>>>> Andrey

>>>>>

>>>>>> Regards,

>>>>>> Christian.

>>>>>>

>>>>>>> Andrey

>>>>>>>

>>>>>>>>> +

>>>>>>>>> +        if (s_job->s_fence->parent) {

>>>>>>>>> +            r = dma_fence_add_callback(s_job->s_fence->parent,

>>>>>>>>> + &s_job->s_fence->cb,

>>>>>>>>> + drm_sched_process_job);

>>>>>>>>> +            if (r)

>>>>>>>>> +                DRM_ERROR("fence restore callback failed 

>>>>>>>>> (%d)\n",

>>>>>>>>> +                                      r);

>>>>>>>> When you fail to add the callback this means that you need to call

>>>>>>>> call drm_sched_process_job manually here.

>>>>>>>>

>>>>>>>>>                }

>>>>>>>>>            }

>>>>>>>>> + spin_unlock_irqrestore(&sched->job_list_lock, flags);

>>>>>>>>> +

>>>>>>>>> +    dma_fence_wait(wait_fence, false);

>>>>>>>>> +    dma_fence_put(wait_fence);

>>>>>>>>> +    wait_fence = NULL;

>>>>>>>>> +

>>>>>>>>> +    goto retry_wait;

>>>>>>>>> +

>>>>>>>>> +done:

>>>>>>>>> +    return;

>>>>>>>> Drop the done: label and return directly above.

>>>>>>>>

>>>>>>>> Apart from all those nit picks that starts to look like it should

>>>>>>>> work,

>>>>>>>> Christian.

>>>>>>>>

>>>>>>>>>        }

>>>>>>>>> -EXPORT_SYMBOL(drm_sched_hw_job_reset);

>>>>>>>>> +EXPORT_SYMBOL(drm_sched_stop);

>>>>>>>>>          /**

>>>>>>>>>         * drm_sched_job_recovery - recover jobs after a reset

>>>>>>>>> @@ -393,33 +446,21 @@ EXPORT_SYMBOL(drm_sched_hw_job_reset);

>>>>>>>>>         * @sched: scheduler instance

>>>>>>>>>         *

>>>>>>>>>         */

>>>>>>>>> -void drm_sched_job_recovery(struct drm_gpu_scheduler *sched)

>>>>>>>>> +void drm_sched_start(struct drm_gpu_scheduler *sched, bool

>>>>>>>>> full_recovery)

>>>>>>>>>        {

>>>>>>>>>            struct drm_sched_job *s_job, *tmp;

>>>>>>>>> -    bool found_guilty = false;

>>>>>>>>>            unsigned long flags;

>>>>>>>>>            int r;

>>>>>>>>>        +    if (!full_recovery)

>>>>>>>>> +        goto unpark;

>>>>>>>>> +

>>>>>>>>> spin_lock_irqsave(&sched->job_list_lock, flags);

>>>>>>>>>            list_for_each_entry_safe(s_job, tmp,

>>>>>>>>> &sched->ring_mirror_list,

>>>>>>>>> node) {

>>>>>>>>>                struct drm_sched_fence *s_fence = s_job->s_fence;

>>>>>>>>> -        struct dma_fence *fence;

>>>>>>>>> -        uint64_t guilty_context;

>>>>>>>>> -

>>>>>>>>> -        if (!found_guilty && atomic_read(&s_job->karma) >

>>>>>>>>> sched->hang_limit) {

>>>>>>>>> -            found_guilty = true;

>>>>>>>>> -            guilty_context = s_job->s_fence->scheduled.context;

>>>>>>>>> -        }

>>>>>>>>> -

>>>>>>>>> -        if (found_guilty && s_job->s_fence->scheduled.context ==

>>>>>>>>> guilty_context)

>>>>>>>>> - dma_fence_set_error(&s_fence->finished, -ECANCELED);

>>>>>>>>> -

>>>>>>>>> - spin_unlock_irqrestore(&sched->job_list_lock, flags);

>>>>>>>>> -        fence = sched->ops->run_job(s_job);

>>>>>>>>> -        atomic_inc(&sched->hw_rq_count);

>>>>>>>>> +        struct dma_fence *fence = s_job->s_fence->parent;

>>>>>>>>>                  if (fence) {

>>>>>>>>> -            s_fence->parent = dma_fence_get(fence);

>>>>>>>>>                    r = dma_fence_add_callback(fence, 

>>>>>>>>> &s_fence->cb,

>>>>>>>>> drm_sched_process_job);

>>>>>>>>>                    if (r == -ENOENT)

>>>>>>>>> @@ -427,18 +468,47 @@ void drm_sched_job_recovery(struct

>>>>>>>>> drm_gpu_scheduler *sched)

>>>>>>>>>                    else if (r)

>>>>>>>>>                        DRM_ERROR("fence add callback failed 

>>>>>>>>> (%d)\n",

>>>>>>>>>                              r);

>>>>>>>>> -            dma_fence_put(fence);

>>>>>>>>> -        } else {

>>>>>>>>> -            if (s_fence->finished.error < 0)

>>>>>>>>> - drm_sched_expel_job_unlocked(s_job);

>>>>>>>>> +        } else

>>>>>>>>>                    drm_sched_process_job(NULL, &s_fence->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);

>>>>>>>>> +}

>>>>>>>>> +EXPORT_SYMBOL(drm_sched_start);

>>>>>>>>> +

>>>>>>>>> +/**

>>>>>>>>> + * drm_sched_resubmit_jobs - helper to relunch job from mirror

>>>>>>>>> ring

>>>>>>>>> list

>>>>>>>>> + *

>>>>>>>>> + * @sched: scheduler instance

>>>>>>>>> + *

>>>>>>>>> + */

>>>>>>>>> +void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)

>>>>>>>>> +{

>>>>>>>>> +    struct drm_sched_job *s_job, *tmp;

>>>>>>>>> +    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;

>>>>>>>>> +

>>>>>>>>> +        if (!found_guilty && atomic_read(&s_job->karma) >

>>>>>>>>> sched->hang_limit) {

>>>>>>>>> +            found_guilty = true;

>>>>>>>>> +            guilty_context = s_job->s_fence->scheduled.context;

>>>>>>>>> +        }

>>>>>>>>> +

>>>>>>>>> +        if (found_guilty && s_job->s_fence->scheduled.context ==

>>>>>>>>> guilty_context)

>>>>>>>>> + dma_fence_set_error(&s_fence->finished, -ECANCELED);

>>>>>>>>> +

>>>>>>>>> +        s_job->s_fence->parent = sched->ops->run_job(s_job);

>>>>>>>>> +        atomic_inc(&sched->hw_rq_count);

>>>>>>>>> +    }

>>>>>>>>>        }

>>>>>>>>> -EXPORT_SYMBOL(drm_sched_job_recovery);

>>>>>>>>> +EXPORT_SYMBOL(drm_sched_resubmit_jobs);

>>>>>>>>>          /**

>>>>>>>>>         * drm_sched_job_init - init a scheduler job

>>>>>>>>> @@ -634,26 +704,14 @@ static int drm_sched_main(void *param)

>>>>>>>>>                        DRM_ERROR("fence add callback failed 

>>>>>>>>> (%d)\n",

>>>>>>>>>                              r);

>>>>>>>>>                    dma_fence_put(fence);

>>>>>>>>> -        } else {

>>>>>>>>> -            if (s_fence->finished.error < 0)

>>>>>>>>> - drm_sched_expel_job_unlocked(sched_job);

>>>>>>>>> +        } else

>>>>>>>>>                    drm_sched_process_job(NULL, &s_fence->cb);

>>>>>>>>> -        }

>>>>>>>>> wake_up(&sched->job_scheduled);

>>>>>>>>>            }

>>>>>>>>>            return 0;

>>>>>>>>>        }

>>>>>>>>>        -static void drm_sched_expel_job_unlocked(struct

>>>>>>>>> drm_sched_job *s_job)

>>>>>>>>> -{

>>>>>>>>> -    struct drm_gpu_scheduler *sched = s_job->sched;

>>>>>>>>> -

>>>>>>>>> -    spin_lock(&sched->job_list_lock);

>>>>>>>>> -    list_del_init(&s_job->node);

>>>>>>>>> -    spin_unlock(&sched->job_list_lock);

>>>>>>>>> -}

>>>>>>>>> -

>>>>>>>>>        /**

>>>>>>>>>         * drm_sched_init - Init a gpu scheduler instance

>>>>>>>>>         *

>>>>>>>>> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c

>>>>>>>>> b/drivers/gpu/drm/v3d/v3d_sched.c

>>>>>>>>> index 445b2ef..f76d9ed 100644

>>>>>>>>> --- a/drivers/gpu/drm/v3d/v3d_sched.c

>>>>>>>>> +++ b/drivers/gpu/drm/v3d/v3d_sched.c

>>>>>>>>> @@ -178,18 +178,22 @@ v3d_job_timedout(struct drm_sched_job

>>>>>>>>> *sched_job)

>>>>>>>>>            for (q = 0; q < V3D_MAX_QUEUES; q++) {

>>>>>>>>>                struct drm_gpu_scheduler *sched =

>>>>>>>>> &v3d->queue[q].sched;

>>>>>>>>>        -        kthread_park(sched->thread);

>>>>>>>>> -        drm_sched_hw_job_reset(sched, (sched_job->sched == 

>>>>>>>>> sched ?

>>>>>>>>> +        drm_sched_stop(sched, (sched_job->sched == sched ?

>>>>>>>>>                                   sched_job : NULL));

>>>>>>>>> +

>>>>>>>>> +        if(sched_job)

>>>>>>>>> +            drm_sched_increase_karma(sched_job);

>>>>>>>>>            }

>>>>>>>>>              /* get the GPU back into the init state */

>>>>>>>>>            v3d_reset(v3d);

>>>>>>>>>        +    for (q = 0; q < V3D_MAX_QUEUES; q++)

>>>>>>>>> + drm_sched_resubmit_jobs(sched_job->sched);

>>>>>>>>> +

>>>>>>>>>            /* Unblock schedulers and restart their jobs. */

>>>>>>>>>            for (q = 0; q < V3D_MAX_QUEUES; q++) {

>>>>>>>>> - drm_sched_job_recovery(&v3d->queue[q].sched);

>>>>>>>>> - kthread_unpark(v3d->queue[q].sched.thread);

>>>>>>>>> + drm_sched_start(&v3d->queue[q].sched, true);

>>>>>>>>>            }

>>>>>>>>> mutex_unlock(&v3d->reset_lock);

>>>>>>>>> diff --git a/include/drm/gpu_scheduler.h

>>>>>>>>> b/include/drm/gpu_scheduler.h

>>>>>>>>> index 47e1979..5ab2d97 100644

>>>>>>>>> --- a/include/drm/gpu_scheduler.h

>>>>>>>>> +++ b/include/drm/gpu_scheduler.h

>>>>>>>>> @@ -175,6 +175,7 @@ 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.

>>>>>>>>> + * @finish_node: used in a list to wait on before resetting the

>>>>>>>>> scheduler

>>>>>>>>>         * @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

>>>>>>>>> @@ -193,6 +194,7 @@ struct drm_sched_job {

>>>>>>>>>            struct dma_fence_cb        finish_cb;

>>>>>>>>>            struct work_struct        finish_work;

>>>>>>>>>            struct list_head        node;

>>>>>>>>> +    struct list_head        finish_node;

>>>>>>>>>            uint64_t            id;

>>>>>>>>>            atomic_t            karma;

>>>>>>>>>            enum drm_sched_priority s_priority;

>>>>>>>>> @@ -298,9 +300,11 @@ 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_hw_job_reset(struct drm_gpu_scheduler *sched,

>>>>>>>>> -                struct drm_sched_job *job);

>>>>>>>>> -void drm_sched_job_recovery(struct drm_gpu_scheduler *sched);

>>>>>>>>> +void drm_sched_stop(struct drm_gpu_scheduler *sched,

>>>>>>>>> +            struct drm_sched_job *job);

>>>>>>>>> +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);

>>>>>>>>>        bool drm_sched_dependency_optimized(struct dma_fence* 

>>>>>>>>> fence,

>>>>>>>>>                            struct drm_sched_entity *entity);

>>>>>>>>>        void drm_sched_fault(struct drm_gpu_scheduler *sched);

>>>>>>>> _______________________________________________

>>>>>>>> amd-gfx mailing list

>>>>>>>> amd-gfx@lists.freedesktop.org

>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

>>>>>> _______________________________________________

>>>>>> amd-gfx mailing list

>>>>>> amd-gfx@lists.freedesktop.org

>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

>>>>> _______________________________________________

>>>>> 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

>> _______________________________________________

>> amd-gfx mailing list

>> amd-gfx@lists.freedesktop.org

>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

>

Patch hide | download patch | download mbox

From 82f84f1995f30513dd926b0f3ca7b047ff7168ab Mon Sep 17 00:00:00 2001
From: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Date: Wed, 5 Dec 2018 14:21:28 -0500
Subject: drm/sched: Rework HW fence processing.

Expedite job deletion from ring mirror list to the HW fence signal
callback instead from finish_work, together with waiting for all
such fences to signal in drm_sched_stop we garantee that
already signaled job will not be processed twice.
Remove the sched finish fence callback and just submit finish_work
directly from the HW fence callback.

v2: Fix comments.
v3: Attach  hw fence cb to sched_job
v5: Rebase

Suggested-by: Christian Koenig <Christian.Koenig@amd.com>
Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 55 +++++++++++++++++-----------------
 include/drm/gpu_scheduler.h            |  6 ++--
 2 files changed, 29 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index e758fc6..16c6363 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -284,8 +284,6 @@  static void drm_sched_job_finish(struct work_struct *work)
 	cancel_delayed_work_sync(&sched->work_tdr);
 
 	spin_lock_irqsave(&sched->job_list_lock, flags);
-	/* remove job from ring_mirror_list */
-	list_del_init(&s_job->node);
 	/* queue TDR for next job */
 	drm_sched_start_timeout(sched);
 	spin_unlock_irqrestore(&sched->job_list_lock, flags);
@@ -293,22 +291,11 @@  static void drm_sched_job_finish(struct work_struct *work)
 	sched->ops->free_job(s_job);
 }
 
-static void drm_sched_job_finish_cb(struct dma_fence *f,
-				    struct dma_fence_cb *cb)
-{
-	struct drm_sched_job *job = container_of(cb, struct drm_sched_job,
-						 finish_cb);
-	schedule_work(&job->finish_work);
-}
-
 static void drm_sched_job_begin(struct drm_sched_job *s_job)
 {
 	struct drm_gpu_scheduler *sched = s_job->sched;
 	unsigned long flags;
 
-	dma_fence_add_callback(&s_job->s_fence->finished, &s_job->finish_cb,
-			       drm_sched_job_finish_cb);
-
 	spin_lock_irqsave(&sched->job_list_lock, flags);
 	list_add_tail(&s_job->node, &sched->ring_mirror_list);
 	drm_sched_start_timeout(sched);
@@ -405,7 +392,7 @@  void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad)
 	list_for_each_entry_reverse(s_job, &sched->ring_mirror_list, node) {
 		if (s_job->s_fence->parent &&
 		    dma_fence_remove_callback(s_job->s_fence->parent,
-					      &s_job->s_fence->cb)) {
+					      &s_job->cb)) {
 			dma_fence_put(s_job->s_fence->parent);
 			s_job->s_fence->parent = NULL;
 			atomic_dec(&sched->hw_rq_count);
@@ -433,31 +420,34 @@  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)
 		goto unpark;
 
-	spin_lock_irqsave(&sched->job_list_lock, flags);
+	/*
+	 * 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
+	 * GPU recovers can't run in parallel.
+	 */
 	list_for_each_entry_safe(s_job, tmp, &sched->ring_mirror_list, node) {
-		struct drm_sched_fence *s_fence = s_job->s_fence;
 		struct dma_fence *fence = s_job->s_fence->parent;
 
 		if (fence) {
-			r = dma_fence_add_callback(fence, &s_fence->cb,
+			r = dma_fence_add_callback(fence, &s_job->cb,
 						   drm_sched_process_job);
 			if (r == -ENOENT)
-				drm_sched_process_job(fence, &s_fence->cb);
+				drm_sched_process_job(fence, &s_job->cb);
 			else if (r)
 				DRM_ERROR("fence add callback failed (%d)\n",
 					  r);
 		} else
-			drm_sched_process_job(NULL, &s_fence->cb);
+			drm_sched_process_job(NULL, &s_job->cb);
 	}
 
 	drm_sched_start_timeout(sched);
-	spin_unlock_irqrestore(&sched->job_list_lock, flags);
 
 unpark:
 	kthread_unpark(sched->thread);
@@ -606,18 +596,27 @@  drm_sched_select_entity(struct drm_gpu_scheduler *sched)
  */
 static void drm_sched_process_job(struct dma_fence *f, struct dma_fence_cb *cb)
 {
-	struct drm_sched_fence *s_fence =
-		container_of(cb, struct drm_sched_fence, 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);
 
-	dma_fence_get(&s_fence->finished);
 	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);
+
 	drm_sched_fence_finished(s_fence);
 
 	trace_drm_sched_process_job(s_fence);
-	dma_fence_put(&s_fence->finished);
 	wake_up_interruptible(&sched->wake_up_worker);
+
+	schedule_work(&s_job->finish_work);
 }
 
 /**
@@ -680,16 +679,16 @@  static int drm_sched_main(void *param)
 
 		if (fence) {
 			s_fence->parent = dma_fence_get(fence);
-			r = dma_fence_add_callback(fence, &s_fence->cb,
+			r = dma_fence_add_callback(fence, &sched_job->cb,
 						   drm_sched_process_job);
 			if (r == -ENOENT)
-				drm_sched_process_job(fence, &s_fence->cb);
+				drm_sched_process_job(fence, &sched_job->cb);
 			else if (r)
 				DRM_ERROR("fence add callback failed (%d)\n",
 					  r);
 			dma_fence_put(fence);
 		} else
-			drm_sched_process_job(NULL, &s_fence->cb);
+			drm_sched_process_job(NULL, &sched_job->cb);
 
 		wake_up(&sched->job_scheduled);
 	}
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 4f21faf..62c2352 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -138,10 +138,6 @@  struct drm_sched_fence {
 	struct dma_fence		finished;
 
         /**
-         * @cb: the callback for the parent fence below.
-         */
-	struct dma_fence_cb		cb;
-        /**
          * @parent: the fence returned by &drm_sched_backend_ops.run_job
          * when scheduling the job on hardware. We signal the
          * &drm_sched_fence.finished fence once parent is signalled.
@@ -181,6 +177,7 @@  struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
  *         be scheduled further.
  * @s_priority: the priority of the job.
  * @entity: the entity to which this job belongs.
+ * @cb: the callback for the parent fence in s_fence.
  *
  * A job is created by the driver using drm_sched_job_init(), and
  * should call drm_sched_entity_push_job() once it wants the scheduler
@@ -197,6 +194,7 @@  struct drm_sched_job {
 	atomic_t			karma;
 	enum drm_sched_priority		s_priority;
 	struct drm_sched_entity  *entity;
+	struct dma_fence_cb		cb;
 };
 
 static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
-- 
2.7.4


Comments

Grodzovsky, Andrey Jan. 10, 2019, 3:56 p.m.
Just a ping.

Andrey


On 01/09/2019 10:18 AM, Andrey Grodzovsky wrote:
>

>

> On 01/09/2019 05:22 AM, Christian König wrote:

>> Am 07.01.19 um 20:47 schrieb Grodzovsky, Andrey:

>>>

>>> On 01/07/2019 09:13 AM, Christian König wrote:

>>>> Am 03.01.19 um 18:42 schrieb Grodzovsky, Andrey:

>>>>> On 01/03/2019 11:20 AM, Grodzovsky, Andrey wrote:

>>>>>> On 01/03/2019 03:54 AM, Koenig, Christian wrote:

>>>>>>> Am 21.12.18 um 21:36 schrieb Grodzovsky, Andrey:

>>>>>>>> On 12/21/2018 01:37 PM, Christian König wrote:

>>>>>>>>> Am 20.12.18 um 20:23 schrieb Andrey Grodzovsky:

>>>>>>>>>> Decauple sched threads stop and start and ring mirror

>>>>>>>>>> list handling from the policy of what to do about the

>>>>>>>>>> guilty jobs.

>>>>>>>>>> When stoppping the sched thread and detaching sched fences

>>>>>>>>>> from non signaled HW fenes wait for all signaled HW fences

>>>>>>>>>> to complete before rerunning the jobs.

>>>>>>>>>>

>>>>>>>>>> v2: Fix resubmission of guilty job into HW after refactoring.

>>>>>>>>>>

>>>>>>>>>> v4:

>>>>>>>>>> Full restart for all the jobs, not only from guilty ring.

>>>>>>>>>> Extract karma increase into standalone function.

>>>>>>>>>>

>>>>>>>>>> v5:

>>>>>>>>>> Rework waiting for signaled jobs without relying on the job

>>>>>>>>>> struct itself as those might already be freed for non 'guilty'

>>>>>>>>>> job's schedulers.

>>>>>>>>>> Expose karma increase to drivers.

>>>>>>>>>>

>>>>>>>>>> Suggested-by: Christian Koenig <Christian.Koenig@amd.com>

>>>>>>>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>

>>>>>>>>>> ---

>>>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  18 +--

>>>>>>>>>>        drivers/gpu/drm/etnaviv/etnaviv_sched.c |  11 +-

>>>>>>>>>>        drivers/gpu/drm/scheduler/sched_main.c | 188

>>>>>>>>>> +++++++++++++++++++----------

>>>>>>>>>>        drivers/gpu/drm/v3d/v3d_sched.c |  12 +-

>>>>>>>>>>        include/drm/gpu_scheduler.h |  10 +-

>>>>>>>>>>        5 files changed, 151 insertions(+), 88 deletions(-)

>>>>>>>>>>

>>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c

>>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c

>>>>>>>>>> index 8a078f4..a4bd2d3 100644

>>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c

>>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c

>>>>>>>>>> @@ -3298,12 +3298,10 @@ static int

>>>>>>>>>> amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,

>>>>>>>>>>                if (!ring || !ring->sched.thread)

>>>>>>>>>>                    continue;

>>>>>>>>>>        - kthread_park(ring->sched.thread);

>>>>>>>>>> +        drm_sched_stop(&ring->sched, job ? &job->base : NULL);

>>>>>>>>>>        -        if (job && job->base.sched != &ring->sched)

>>>>>>>>>> -            continue;

>>>>>>>>>> -

>>>>>>>>>> - drm_sched_hw_job_reset(&ring->sched, job ? &job->base :

>>>>>>>>>> NULL);

>>>>>>>>>> +        if(job)

>>>>>>>>>> + drm_sched_increase_karma(&job->base);

>>>>>>>>> Since we dropped the "job && job->base.sched != &ring->sched" 

>>>>>>>>> check

>>>>>>>>> above this will now increase the jobs karma multiple times.

>>>>>>>>>

>>>>>>>>> Maybe just move that outside of the loop.

>>>>>>>>>

>>>>>>>>>>                  /* after all hw jobs are reset, hw fence is

>>>>>>>>>> meaningless,

>>>>>>>>>> so force_completion */

>>>>>>>>>> amdgpu_fence_driver_force_completion(ring);

>>>>>>>>>> @@ -3454,14 +3452,10 @@ static void

>>>>>>>>>> amdgpu_device_post_asic_reset(struct amdgpu_device *adev,

>>>>>>>>>>                if (!ring || !ring->sched.thread)

>>>>>>>>>>                    continue;

>>>>>>>>>>        -        /* only need recovery sched of the given 

>>>>>>>>>> job's ring

>>>>>>>>>> -         * or all rings (in the case @job is NULL)

>>>>>>>>>> -         * after above amdgpu_reset accomplished

>>>>>>>>>> -         */

>>>>>>>>>> -        if ((!job || job->base.sched == &ring->sched) &&

>>>>>>>>>> !adev->asic_reset_res)

>>>>>>>>>> - drm_sched_job_recovery(&ring->sched);

>>>>>>>>>> +        if (!adev->asic_reset_res)

>>>>>>>>>> + drm_sched_resubmit_jobs(&ring->sched);

>>>>>>>>>>        - kthread_unpark(ring->sched.thread);

>>>>>>>>>> +        drm_sched_start(&ring->sched, !adev->asic_reset_res);

>>>>>>>>>>            }

>>>>>>>>>>              if (!amdgpu_device_has_dc_support(adev)) {

>>>>>>>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c

>>>>>>>>>> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c

>>>>>>>>>> index 49a6763..6f1268f 100644

>>>>>>>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c

>>>>>>>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c

>>>>>>>>>> @@ -109,16 +109,19 @@ static void 

>>>>>>>>>> etnaviv_sched_timedout_job(struct

>>>>>>>>>> drm_sched_job *sched_job)

>>>>>>>>>>            }

>>>>>>>>>>              /* block scheduler */

>>>>>>>>>> -    kthread_park(gpu->sched.thread);

>>>>>>>>>> -    drm_sched_hw_job_reset(&gpu->sched, sched_job);

>>>>>>>>>> +    drm_sched_stop(&gpu->sched, sched_job);

>>>>>>>>>> +

>>>>>>>>>> +    if(sched_job)

>>>>>>>>>> +        drm_sched_increase_karma(sched_job);

>>>>>>>>>>              /* get the GPU back into the init state */

>>>>>>>>>>            etnaviv_core_dump(gpu);

>>>>>>>>>>            etnaviv_gpu_recover_hang(gpu);

>>>>>>>>>>        + drm_sched_resubmit_jobs(&gpu->sched);

>>>>>>>>>> +

>>>>>>>>>>            /* restart scheduler after GPU is usable again */

>>>>>>>>>> -    drm_sched_job_recovery(&gpu->sched);

>>>>>>>>>> -    kthread_unpark(gpu->sched.thread);

>>>>>>>>>> +    drm_sched_start(&gpu->sched, true);

>>>>>>>>>>        }

>>>>>>>>>>          static void etnaviv_sched_free_job(struct drm_sched_job

>>>>>>>>>> *sched_job)

>>>>>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c

>>>>>>>>>> b/drivers/gpu/drm/scheduler/sched_main.c

>>>>>>>>>> index dbb6906..b5c5bee 100644

>>>>>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c

>>>>>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c

>>>>>>>>>> @@ -60,8 +60,6 @@

>>>>>>>>>>          static void drm_sched_process_job(struct dma_fence *f,

>>>>>>>>>> struct

>>>>>>>>>> dma_fence_cb *cb);

>>>>>>>>>>        -static void drm_sched_expel_job_unlocked(struct

>>>>>>>>>> drm_sched_job

>>>>>>>>>> *s_job);

>>>>>>>>>> -

>>>>>>>>>>        /**

>>>>>>>>>>         * drm_sched_rq_init - initialize a given run queue 

>>>>>>>>>> struct

>>>>>>>>>>         *

>>>>>>>>>> @@ -335,6 +333,42 @@ static void drm_sched_job_timedout(struct

>>>>>>>>>> work_struct *work)

>>>>>>>>>> spin_unlock_irqrestore(&sched->job_list_lock, flags);

>>>>>>>>>>        }

>>>>>>>>> Kernel doc here would be nice to have.

>>>>>>>>>

>>>>>>>>>> +void drm_sched_increase_karma(struct drm_sched_job *bad)

>>>>>>>>>> +{

>>>>>>>>>> +    int i;

>>>>>>>>>> +    struct drm_sched_entity *tmp;

>>>>>>>>>> +    struct drm_sched_entity *entity;

>>>>>>>>>> +    struct drm_gpu_scheduler *sched = bad->sched;

>>>>>>>>>> +

>>>>>>>>>> +    /* don't increase @bad's karma if it's from KERNEL RQ,

>>>>>>>>>> +     * because sometimes GPU hang would cause kernel jobs 

>>>>>>>>>> (like VM

>>>>>>>>>> updating jobs)

>>>>>>>>>> +     * corrupt but keep in mind that kernel jobs always 

>>>>>>>>>> considered

>>>>>>>>>> good.

>>>>>>>>>> +     */

>>>>>>>>>> +    if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {

>>>>>>>>>> +        atomic_inc(&bad->karma);

>>>>>>>>>> +        for (i = DRM_SCHED_PRIORITY_MIN; i <

>>>>>>>>>> DRM_SCHED_PRIORITY_KERNEL;

>>>>>>>>>> +             i++) {

>>>>>>>>>> +            struct drm_sched_rq *rq = &sched->sched_rq[i];

>>>>>>>>>> +

>>>>>>>>>> +            spin_lock(&rq->lock);

>>>>>>>>>> +            list_for_each_entry_safe(entity, tmp, 

>>>>>>>>>> &rq->entities,

>>>>>>>>>> list) {

>>>>>>>>>> +                if (bad->s_fence->scheduled.context ==

>>>>>>>>>> +                    entity->fence_context) {

>>>>>>>>>> +                    if (atomic_read(&bad->karma) >

>>>>>>>>>> + bad->sched->hang_limit)

>>>>>>>>>> +                        if (entity->guilty)

>>>>>>>>>> + atomic_set(entity->guilty, 1);

>>>>>>>>>> +                    break;

>>>>>>>>>> +                }

>>>>>>>>>> +            }

>>>>>>>>>> +            spin_unlock(&rq->lock);

>>>>>>>>>> +            if (&entity->list != &rq->entities)

>>>>>>>>>> +                break;

>>>>>>>>>> +        }

>>>>>>>>>> +    }

>>>>>>>>>> +}

>>>>>>>>>> +EXPORT_SYMBOL(drm_sched_increase_karma);

>>>>>>>>>> +

>>>>>>>>>>        /**

>>>>>>>>>>         * drm_sched_hw_job_reset - stop the scheduler if it

>>>>>>>>>> contains the

>>>>>>>>>> bad job

>>>>>>>>>>         *

>>>>>>>>>> @@ -342,13 +376,22 @@ static void drm_sched_job_timedout(struct

>>>>>>>>>> work_struct *work)

>>>>>>>>>>         * @bad: bad scheduler job

>>>>>>>>>>         *

>>>>>>>>>>         */

>>>>>>>>>> -void drm_sched_hw_job_reset(struct drm_gpu_scheduler *sched,

>>>>>>>>>> struct

>>>>>>>>>> drm_sched_job *bad)

>>>>>>>>>> +void drm_sched_stop(struct drm_gpu_scheduler *sched, struct

>>>>>>>>>> drm_sched_job *bad)

>>>>>>>>>>        {

>>>>>>>>>> -    struct drm_sched_job *s_job;

>>>>>>>>>> -    struct drm_sched_entity *entity, *tmp;

>>>>>>>>>> +    struct drm_sched_job *s_job, *last_job;

>>>>>>>>>>            unsigned long flags;

>>>>>>>>>> -    int i;

>>>>>>>>>> +    struct dma_fence *wait_fence =  NULL;

>>>>>>>>>> +    int r;

>>>>>>>>>> +

>>>>>>>>>> +    kthread_park(sched->thread);

>>>>>>>>>>        +    /*

>>>>>>>>>> +     * Verify all the signaled jobs in mirror list are removed

>>>>>>>>>> from

>>>>>>>>>> the ring

>>>>>>>>>> +     * by waiting for their respective scheduler fences to 

>>>>>>>>>> signal.

>>>>>>>>>> +     * Continually  repeat traversing the ring mirror list

>>>>>>>>>> until no

>>>>>>>>>> more signaled

>>>>>>>>>> +     * fences are found

>>>>>>>>>> +     */

>>>>>>>>>> +retry_wait:

>>>>>>>>>> spin_lock_irqsave(&sched->job_list_lock, flags);

>>>>>>>>>>            list_for_each_entry_reverse(s_job,

>>>>>>>>>> &sched->ring_mirror_list,

>>>>>>>>>> node) {

>>>>>>>>>>                if (s_job->s_fence->parent &&

>>>>>>>>>> @@ -357,35 +400,45 @@ void drm_sched_hw_job_reset(struct

>>>>>>>>>> drm_gpu_scheduler *sched, struct drm_sched_jo

>>>>>>>>>> dma_fence_put(s_job->s_fence->parent);

>>>>>>>>>>                    s_job->s_fence->parent = NULL;

>>>>>>>>>> atomic_dec(&sched->hw_rq_count);

>>>>>>>>>> +        } else {

>>>>>>>>>> +             wait_fence =

>>>>>>>>>> dma_fence_get(&s_job->s_fence->finished);

>>>>>>>>>> +             last_job = s_job;

>>>>>>>>>> +             break;

>>>>>>>>>>                }

>>>>>>>>>>            }

>>>>>>>>>> - spin_unlock_irqrestore(&sched->job_list_lock, flags);

>>>>>>>>>>        -    if (bad && bad->s_priority !=

>>>>>>>>>> DRM_SCHED_PRIORITY_KERNEL) {

>>>>>>>>>> -        atomic_inc(&bad->karma);

>>>>>>>>>> -        /* don't increase @bad's karma if it's from KERNEL RQ,

>>>>>>>>>> -         * becuase sometimes GPU hang would cause kernel jobs

>>>>>>>>>> (like

>>>>>>>>>> VM updating jobs)

>>>>>>>>>> -         * corrupt but keep in mind that kernel jobs always

>>>>>>>>>> considered good.

>>>>>>>>>> -         */

>>>>>>>>>> -        for (i = DRM_SCHED_PRIORITY_MIN; i <

>>>>>>>>>> DRM_SCHED_PRIORITY_KERNEL; i++ ) {

>>>>>>>>>> -            struct drm_sched_rq *rq = &sched->sched_rq[i];

>>>>>>>>>> +    /* No signaled jobs in the ring, its safe to proceed to 

>>>>>>>>>> ASIC

>>>>>>>>>> reset */

>>>>>>>>>> +    if (!wait_fence) {

>>>>>>>>>> + spin_unlock_irqrestore(&sched->job_list_lock, flags);

>>>>>>>>>> +        goto done;

>>>>>>>>>> +    }

>>>>>>>>>>        -            spin_lock(&rq->lock);

>>>>>>>>>> -            list_for_each_entry_safe(entity, tmp, 

>>>>>>>>>> &rq->entities,

>>>>>>>>>> list) {

>>>>>>>>>> -                if (bad->s_fence->scheduled.context ==

>>>>>>>>>> entity->fence_context) {

>>>>>>>>>> -                    if (atomic_read(&bad->karma) >

>>>>>>>>>> bad->sched->hang_limit)

>>>>>>>>>> -                        if (entity->guilty)

>>>>>>>>>> - atomic_set(entity->guilty, 1);

>>>>>>>>>> -                    break;

>>>>>>>>>> -                }

>>>>>>>>>> -            }

>>>>>>>>>> -            spin_unlock(&rq->lock);

>>>>>>>>>> -            if (&entity->list != &rq->entities)

>>>>>>>>>> -                break;

>>>>>>>>>> +    /* Restore removed cb since removing again already removed

>>>>>>>>>> cb is

>>>>>>>>>> undefined */

>>>>>>>>>> +    list_for_each_entry_reverse(s_job, 

>>>>>>>>>> &sched->ring_mirror_list,

>>>>>>>>>> node) {

>>>>>>>>>> +        if(s_job == last_job)

>>>>>>>>>> +            break;

>>>>>>>>> Need to double check after the holidays, but you should be able

>>>>>>>>> to use

>>>>>>>>> list_for_each_entry_continue here.

>>>>>>>> I think it should work - kind of traversing back on all the jobs

>>>>>>>> we just

>>>>>>>> removed their callbacks.

>>>>>>> Wrapping your head around stuff again after the holidays sometimes

>>>>>>> shows

>>>>>>> new problems we have completely missed :)

>>>>>>>

>>>>>>> Adding the callbacks again won't work because we have freed up our

>>>>>>> reference to the hardware fence above:

>>>>>>>> dma_fence_put(s_job->s_fence->parent);

>>>>>>>>                    s_job->s_fence->parent = NULL;

>>>>>>> We need to drop this or we would never be able to re-add the fences

>>>>>> Yea, that a big miss on my side...

>>>>>>

>>>>>>> .

>>>>>>>

>>>>>>> But I'm still not sure if we shouldn't rely on the correct order of

>>>>>>> signaling and simplify this instead.

>>>>>> As you said before, once we switched to signaling the parent from 

>>>>>> the

>>>>>> interrupt context instead of scheduled work no danger of race there

>>>>> Correction here - once we switched removing the job from mirror_ring

>>>>> list directly in interrupt context instead later from scheduled work

>>>> Ok, so let's stick with the approach of only waiting for the first

>>>> signaled one found.

>>>>

>>>> But we need to remove setting the parent fence to NULL or otherwise we

>>>> won't be able to add the callback ever again.

>>>>

>>>> Christian.

>>> But we will not be adding the cb back in drm_sched_stop anymore, now we

>>> are only going to add back the cb in drm_sched_startr after rerunning

>>> those jobs in drm_sched_resubmit_jobs and assign them a new parent 

>>> there

>>> anyway.

>>

>> Yeah, but when we find that we don't need to reset anything anymore 

>> then adding the callbacks again won't be possible any more.

>>

>> Christian.

>

> I am not sure I understand it, can u point me to example of how this 

> will happen ? I am attaching my latest patches with waiting only for 

> the last job's fence here just so we are on same page regarding the code.

>

> Andrey

>

>>

>>>

>>> Andrey

>>>

>>>>> Andrey

>>>>>

>>>>>> , so

>>>>>> what if we submit job A and after it job B and B completes before A

>>>>>> (like the sync dependency test in libdrm amdgpu tests but without

>>>>>> adding

>>>>>> explicit dependency to the second command on the first) I believe 

>>>>>> that

>>>>>> still in this case job B's parent (HW) fence will not be signaled

>>>>>> before

>>>>>> job A completes since EOP event is not signaled until the entire 

>>>>>> pipe

>>>>>> completed and flushed it's cashes including job A. So from this

>>>>>> seems to

>>>>>> me that indeed it's enough to wait for the last inserted job's 

>>>>>> parent

>>>>>> (HW) fence in ring mirror list to signal.

>>>>>> Let me know what you think on that.

>>>>>>

>>>>>> P.S V5 is not the last iteration and there was V6 series.

>>>>>>

>>>>>> Andrey

>>>>>>

>>>>>>> Regards,

>>>>>>> Christian.

>>>>>>>

>>>>>>>> Andrey

>>>>>>>>

>>>>>>>>>> +

>>>>>>>>>> +        if (s_job->s_fence->parent) {

>>>>>>>>>> +            r = dma_fence_add_callback(s_job->s_fence->parent,

>>>>>>>>>> + &s_job->s_fence->cb,

>>>>>>>>>> + drm_sched_process_job);

>>>>>>>>>> +            if (r)

>>>>>>>>>> +                DRM_ERROR("fence restore callback failed 

>>>>>>>>>> (%d)\n",

>>>>>>>>>> +                                      r);

>>>>>>>>> When you fail to add the callback this means that you need to 

>>>>>>>>> call

>>>>>>>>> call drm_sched_process_job manually here.

>>>>>>>>>

>>>>>>>>>>                }

>>>>>>>>>>            }

>>>>>>>>>> + spin_unlock_irqrestore(&sched->job_list_lock, flags);

>>>>>>>>>> +

>>>>>>>>>> +    dma_fence_wait(wait_fence, false);

>>>>>>>>>> +    dma_fence_put(wait_fence);

>>>>>>>>>> +    wait_fence = NULL;

>>>>>>>>>> +

>>>>>>>>>> +    goto retry_wait;

>>>>>>>>>> +

>>>>>>>>>> +done:

>>>>>>>>>> +    return;

>>>>>>>>> Drop the done: label and return directly above.

>>>>>>>>>

>>>>>>>>> Apart from all those nit picks that starts to look like it should

>>>>>>>>> work,

>>>>>>>>> Christian.

>>>>>>>>>

>>>>>>>>>>        }

>>>>>>>>>> -EXPORT_SYMBOL(drm_sched_hw_job_reset);

>>>>>>>>>> +EXPORT_SYMBOL(drm_sched_stop);

>>>>>>>>>>          /**

>>>>>>>>>>         * drm_sched_job_recovery - recover jobs after a reset

>>>>>>>>>> @@ -393,33 +446,21 @@ EXPORT_SYMBOL(drm_sched_hw_job_reset);

>>>>>>>>>>         * @sched: scheduler instance

>>>>>>>>>>         *

>>>>>>>>>>         */

>>>>>>>>>> -void drm_sched_job_recovery(struct drm_gpu_scheduler *sched)

>>>>>>>>>> +void drm_sched_start(struct drm_gpu_scheduler *sched, bool

>>>>>>>>>> full_recovery)

>>>>>>>>>>        {

>>>>>>>>>>            struct drm_sched_job *s_job, *tmp;

>>>>>>>>>> -    bool found_guilty = false;

>>>>>>>>>>            unsigned long flags;

>>>>>>>>>>            int r;

>>>>>>>>>>        +    if (!full_recovery)

>>>>>>>>>> +        goto unpark;

>>>>>>>>>> +

>>>>>>>>>> spin_lock_irqsave(&sched->job_list_lock, flags);

>>>>>>>>>>            list_for_each_entry_safe(s_job, tmp,

>>>>>>>>>> &sched->ring_mirror_list,

>>>>>>>>>> node) {

>>>>>>>>>>                struct drm_sched_fence *s_fence = s_job->s_fence;

>>>>>>>>>> -        struct dma_fence *fence;

>>>>>>>>>> -        uint64_t guilty_context;

>>>>>>>>>> -

>>>>>>>>>> -        if (!found_guilty && atomic_read(&s_job->karma) >

>>>>>>>>>> sched->hang_limit) {

>>>>>>>>>> -            found_guilty = true;

>>>>>>>>>> -            guilty_context = s_job->s_fence->scheduled.context;

>>>>>>>>>> -        }

>>>>>>>>>> -

>>>>>>>>>> -        if (found_guilty && 

>>>>>>>>>> s_job->s_fence->scheduled.context ==

>>>>>>>>>> guilty_context)

>>>>>>>>>> - dma_fence_set_error(&s_fence->finished, -ECANCELED);

>>>>>>>>>> -

>>>>>>>>>> - spin_unlock_irqrestore(&sched->job_list_lock, flags);

>>>>>>>>>> -        fence = sched->ops->run_job(s_job);

>>>>>>>>>> -        atomic_inc(&sched->hw_rq_count);

>>>>>>>>>> +        struct dma_fence *fence = s_job->s_fence->parent;

>>>>>>>>>>                  if (fence) {

>>>>>>>>>> -            s_fence->parent = dma_fence_get(fence);

>>>>>>>>>>                    r = dma_fence_add_callback(fence, 

>>>>>>>>>> &s_fence->cb,

>>>>>>>>>> drm_sched_process_job);

>>>>>>>>>>                    if (r == -ENOENT)

>>>>>>>>>> @@ -427,18 +468,47 @@ void drm_sched_job_recovery(struct

>>>>>>>>>> drm_gpu_scheduler *sched)

>>>>>>>>>>                    else if (r)

>>>>>>>>>>                        DRM_ERROR("fence add callback failed 

>>>>>>>>>> (%d)\n",

>>>>>>>>>>                              r);

>>>>>>>>>> -            dma_fence_put(fence);

>>>>>>>>>> -        } else {

>>>>>>>>>> -            if (s_fence->finished.error < 0)

>>>>>>>>>> - drm_sched_expel_job_unlocked(s_job);

>>>>>>>>>> +        } else

>>>>>>>>>>                    drm_sched_process_job(NULL, &s_fence->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);

>>>>>>>>>> +}

>>>>>>>>>> +EXPORT_SYMBOL(drm_sched_start);

>>>>>>>>>> +

>>>>>>>>>> +/**

>>>>>>>>>> + * drm_sched_resubmit_jobs - helper to relunch job from mirror

>>>>>>>>>> ring

>>>>>>>>>> list

>>>>>>>>>> + *

>>>>>>>>>> + * @sched: scheduler instance

>>>>>>>>>> + *

>>>>>>>>>> + */

>>>>>>>>>> +void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched)

>>>>>>>>>> +{

>>>>>>>>>> +    struct drm_sched_job *s_job, *tmp;

>>>>>>>>>> +    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;

>>>>>>>>>> +

>>>>>>>>>> +        if (!found_guilty && atomic_read(&s_job->karma) >

>>>>>>>>>> sched->hang_limit) {

>>>>>>>>>> +            found_guilty = true;

>>>>>>>>>> +            guilty_context = s_job->s_fence->scheduled.context;

>>>>>>>>>> +        }

>>>>>>>>>> +

>>>>>>>>>> +        if (found_guilty && 

>>>>>>>>>> s_job->s_fence->scheduled.context ==

>>>>>>>>>> guilty_context)

>>>>>>>>>> + dma_fence_set_error(&s_fence->finished, -ECANCELED);

>>>>>>>>>> +

>>>>>>>>>> +        s_job->s_fence->parent = sched->ops->run_job(s_job);

>>>>>>>>>> +        atomic_inc(&sched->hw_rq_count);

>>>>>>>>>> +    }

>>>>>>>>>>        }

>>>>>>>>>> -EXPORT_SYMBOL(drm_sched_job_recovery);

>>>>>>>>>> +EXPORT_SYMBOL(drm_sched_resubmit_jobs);

>>>>>>>>>>          /**

>>>>>>>>>>         * drm_sched_job_init - init a scheduler job

>>>>>>>>>> @@ -634,26 +704,14 @@ static int drm_sched_main(void *param)

>>>>>>>>>>                        DRM_ERROR("fence add callback failed 

>>>>>>>>>> (%d)\n",

>>>>>>>>>>                              r);

>>>>>>>>>>                    dma_fence_put(fence);

>>>>>>>>>> -        } else {

>>>>>>>>>> -            if (s_fence->finished.error < 0)

>>>>>>>>>> - drm_sched_expel_job_unlocked(sched_job);

>>>>>>>>>> +        } else

>>>>>>>>>>                    drm_sched_process_job(NULL, &s_fence->cb);

>>>>>>>>>> -        }

>>>>>>>>>> wake_up(&sched->job_scheduled);

>>>>>>>>>>            }

>>>>>>>>>>            return 0;

>>>>>>>>>>        }

>>>>>>>>>>        -static void drm_sched_expel_job_unlocked(struct

>>>>>>>>>> drm_sched_job *s_job)

>>>>>>>>>> -{

>>>>>>>>>> -    struct drm_gpu_scheduler *sched = s_job->sched;

>>>>>>>>>> -

>>>>>>>>>> -    spin_lock(&sched->job_list_lock);

>>>>>>>>>> -    list_del_init(&s_job->node);

>>>>>>>>>> -    spin_unlock(&sched->job_list_lock);

>>>>>>>>>> -}

>>>>>>>>>> -

>>>>>>>>>>        /**

>>>>>>>>>>         * drm_sched_init - Init a gpu scheduler instance

>>>>>>>>>>         *

>>>>>>>>>> diff --git a/drivers/gpu/drm/v3d/v3d_sched.c

>>>>>>>>>> b/drivers/gpu/drm/v3d/v3d_sched.c

>>>>>>>>>> index 445b2ef..f76d9ed 100644

>>>>>>>>>> --- a/drivers/gpu/drm/v3d/v3d_sched.c

>>>>>>>>>> +++ b/drivers/gpu/drm/v3d/v3d_sched.c

>>>>>>>>>> @@ -178,18 +178,22 @@ v3d_job_timedout(struct drm_sched_job

>>>>>>>>>> *sched_job)

>>>>>>>>>>            for (q = 0; q < V3D_MAX_QUEUES; q++) {

>>>>>>>>>>                struct drm_gpu_scheduler *sched =

>>>>>>>>>> &v3d->queue[q].sched;

>>>>>>>>>>        -        kthread_park(sched->thread);

>>>>>>>>>> -        drm_sched_hw_job_reset(sched, (sched_job->sched == 

>>>>>>>>>> sched ?

>>>>>>>>>> +        drm_sched_stop(sched, (sched_job->sched == sched ?

>>>>>>>>>>                                   sched_job : NULL));

>>>>>>>>>> +

>>>>>>>>>> +        if(sched_job)

>>>>>>>>>> + drm_sched_increase_karma(sched_job);

>>>>>>>>>>            }

>>>>>>>>>>              /* get the GPU back into the init state */

>>>>>>>>>>            v3d_reset(v3d);

>>>>>>>>>>        +    for (q = 0; q < V3D_MAX_QUEUES; q++)

>>>>>>>>>> + drm_sched_resubmit_jobs(sched_job->sched);

>>>>>>>>>> +

>>>>>>>>>>            /* Unblock schedulers and restart their jobs. */

>>>>>>>>>>            for (q = 0; q < V3D_MAX_QUEUES; q++) {

>>>>>>>>>> - drm_sched_job_recovery(&v3d->queue[q].sched);

>>>>>>>>>> - kthread_unpark(v3d->queue[q].sched.thread);

>>>>>>>>>> + drm_sched_start(&v3d->queue[q].sched, true);

>>>>>>>>>>            }

>>>>>>>>>> mutex_unlock(&v3d->reset_lock);

>>>>>>>>>> diff --git a/include/drm/gpu_scheduler.h

>>>>>>>>>> b/include/drm/gpu_scheduler.h

>>>>>>>>>> index 47e1979..5ab2d97 100644

>>>>>>>>>> --- a/include/drm/gpu_scheduler.h

>>>>>>>>>> +++ b/include/drm/gpu_scheduler.h

>>>>>>>>>> @@ -175,6 +175,7 @@ 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.

>>>>>>>>>> + * @finish_node: used in a list to wait on before resetting the

>>>>>>>>>> scheduler

>>>>>>>>>>         * @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

>>>>>>>>>> @@ -193,6 +194,7 @@ struct drm_sched_job {

>>>>>>>>>>            struct dma_fence_cb        finish_cb;

>>>>>>>>>>            struct work_struct finish_work;

>>>>>>>>>>            struct list_head        node;

>>>>>>>>>> +    struct list_head        finish_node;

>>>>>>>>>>            uint64_t            id;

>>>>>>>>>>            atomic_t            karma;

>>>>>>>>>>            enum drm_sched_priority s_priority;

>>>>>>>>>> @@ -298,9 +300,11 @@ 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_hw_job_reset(struct drm_gpu_scheduler *sched,

>>>>>>>>>> -                struct drm_sched_job *job);

>>>>>>>>>> -void drm_sched_job_recovery(struct drm_gpu_scheduler *sched);

>>>>>>>>>> +void drm_sched_stop(struct drm_gpu_scheduler *sched,

>>>>>>>>>> +            struct drm_sched_job *job);

>>>>>>>>>> +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);

>>>>>>>>>>        bool drm_sched_dependency_optimized(struct dma_fence* 

>>>>>>>>>> fence,

>>>>>>>>>>                            struct drm_sched_entity *entity);

>>>>>>>>>>        void drm_sched_fault(struct drm_gpu_scheduler *sched);

>>>>>>>>> _______________________________________________

>>>>>>>>> amd-gfx mailing list

>>>>>>>>> amd-gfx@lists.freedesktop.org

>>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

>>>>>>> _______________________________________________

>>>>>>> amd-gfx mailing list

>>>>>>> amd-gfx@lists.freedesktop.org

>>>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

>>>>>> _______________________________________________

>>>>>> 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

>>> _______________________________________________

>>> amd-gfx mailing list

>>> amd-gfx@lists.freedesktop.org

>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

>>

>
Koenig, Christian Jan. 11, 2019, 9:42 a.m.
Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey:
> [SNIP]

>>>> But we will not be adding the cb back in drm_sched_stop anymore, now we

>>>> are only going to add back the cb in drm_sched_startr after rerunning

>>>> those jobs in drm_sched_resubmit_jobs and assign them a new parent

>>>> there

>>>> anyway.

>>> Yeah, but when we find that we don't need to reset anything anymore

>>> then adding the callbacks again won't be possible any more.

>>>

>>> Christian.

>> I am not sure I understand it, can u point me to example of how this

>> will happen ? I am attaching my latest patches with waiting only for

>> the last job's fence here just so we are on same page regarding the code.


Well the whole idea is to prepare all schedulers, then check once more 
if the offending job hasn't completed in the meantime.

If the job completed we need to be able to rollback everything and 
continue as if nothing had happened.

Christian.

>>

>> Andrey

>>
Grodzovsky, Andrey Jan. 11, 2019, 3:37 p.m.
On 01/11/2019 04:42 AM, Koenig, Christian wrote:
> Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey:

>> [SNIP]

>>>>> But we will not be adding the cb back in drm_sched_stop anymore, now we

>>>>> are only going to add back the cb in drm_sched_startr after rerunning

>>>>> those jobs in drm_sched_resubmit_jobs and assign them a new parent

>>>>> there

>>>>> anyway.

>>>> Yeah, but when we find that we don't need to reset anything anymore

>>>> then adding the callbacks again won't be possible any more.

>>>>

>>>> Christian.

>>> I am not sure I understand it, can u point me to example of how this

>>> will happen ? I am attaching my latest patches with waiting only for

>>> the last job's fence here just so we are on same page regarding the code.

> Well the whole idea is to prepare all schedulers, then check once more

> if the offending job hasn't completed in the meantime.

>

> If the job completed we need to be able to rollback everything and

> continue as if nothing had happened.

>

> Christian.


Oh, but this piece of functionality - skipping HW ASIC reset in case the 
guilty job done is totally missing form this patch series and so needs 
to be added. So what you say actually is that for the case were we skip 
HW asic reset because the guilty job did complete we also need to skip  
resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the 
old parent fence pointer for reuse ? If so I would like to add all this 
functionality as a third patch since the first 2 patches are more about 
resolving race condition with jobs in flight while doing reset - what do 
you think ?

Andrey
>

>>> Andrey

>>>

> _______________________________________________

> amd-gfx mailing list

> amd-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Koenig, Christian Jan. 11, 2019, 7:11 p.m.
Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey:
>

> On 01/11/2019 04:42 AM, Koenig, Christian wrote:

>> Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey:

>>> [SNIP]

>>>>>> But we will not be adding the cb back in drm_sched_stop anymore, now we

>>>>>> are only going to add back the cb in drm_sched_startr after rerunning

>>>>>> those jobs in drm_sched_resubmit_jobs and assign them a new parent

>>>>>> there

>>>>>> anyway.

>>>>> Yeah, but when we find that we don't need to reset anything anymore

>>>>> then adding the callbacks again won't be possible any more.

>>>>>

>>>>> Christian.

>>>> I am not sure I understand it, can u point me to example of how this

>>>> will happen ? I am attaching my latest patches with waiting only for

>>>> the last job's fence here just so we are on same page regarding the code.

>> Well the whole idea is to prepare all schedulers, then check once more

>> if the offending job hasn't completed in the meantime.

>>

>> If the job completed we need to be able to rollback everything and

>> continue as if nothing had happened.

>>

>> Christian.

> Oh, but this piece of functionality - skipping HW ASIC reset in case the

> guilty job done is totally missing form this patch series and so needs

> to be added. So what you say actually is that for the case were we skip

> HW asic reset because the guilty job did complete we also need to skip

> resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the

> old parent fence pointer for reuse ? If so I would like to add all this

> functionality as a third patch since the first 2 patches are more about

> resolving race condition with jobs in flight while doing reset - what do

> you think ?


Yeah, sounds perfectly fine to me.

Christian.

>

> Andrey

>>>> Andrey

>>>>

>> _______________________________________________

>> amd-gfx mailing list

>> amd-gfx@lists.freedesktop.org

>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Grodzovsky, Andrey Jan. 11, 2019, 10:03 p.m.
On 01/11/2019 02:11 PM, Koenig, Christian wrote:
> Am 11.01.19 um 16:37 schrieb Grodzovsky, Andrey:

>> On 01/11/2019 04:42 AM, Koenig, Christian wrote:

>>> Am 10.01.19 um 16:56 schrieb Grodzovsky, Andrey:

>>>> [SNIP]

>>>>>>> But we will not be adding the cb back in drm_sched_stop anymore, now we

>>>>>>> are only going to add back the cb in drm_sched_startr after rerunning

>>>>>>> those jobs in drm_sched_resubmit_jobs and assign them a new parent

>>>>>>> there

>>>>>>> anyway.

>>>>>> Yeah, but when we find that we don't need to reset anything anymore

>>>>>> then adding the callbacks again won't be possible any more.

>>>>>>

>>>>>> Christian.

>>>>> I am not sure I understand it, can u point me to example of how this

>>>>> will happen ? I am attaching my latest patches with waiting only for

>>>>> the last job's fence here just so we are on same page regarding the code.

>>> Well the whole idea is to prepare all schedulers, then check once more

>>> if the offending job hasn't completed in the meantime.

>>>

>>> If the job completed we need to be able to rollback everything and

>>> continue as if nothing had happened.

>>>

>>> Christian.

>> Oh, but this piece of functionality - skipping HW ASIC reset in case the

>> guilty job done is totally missing form this patch series and so needs

>> to be added. So what you say actually is that for the case were we skip

>> HW asic reset because the guilty job did complete we also need to skip

>> resubmitting the jobs in drm_sched_resubmit_jobs and hence preserve the

>> old parent fence pointer for reuse ? If so I would like to add all this

>> functionality as a third patch since the first 2 patches are more about

>> resolving race condition with jobs in flight while doing reset - what do

>> you think ?

> Yeah, sounds perfectly fine to me.

>

> Christian.


I realized there is another complication now for XGMI hive use case, we 
currently skip gpu recover for adev in case another gpu recover for 
different adev in same hive is running, under the assumption that we are 
going to reset all devices in hive anyway because that should cover our 
own dev too. But if we chose to skip now HW asic reset if our guilty job 
did finish we will aslo not HW reset any other devices in the hive even 
if one of them might actually had a bad job, wanted to do gpu recover 
but skipped it because our recover was in progress in that time.
My general idea on that is to keep a list of guilty jobs per hive, when 
you start gpu recover you first add you guilty job to the hive and 
trylock hive->reset_lock. Any owner of hive->reset_lock (gpu recovery in 
progress) once he finished his recovery and released hive->reset_lock 
should go over hive->bad_jobs_list and if at least one of them is still 
not signaled (not done) trigger another gpu recovery and so on. If you 
do manage to trylock you also go over the list, clean it and perform 
recovery. This list probably needs to be protected with per hive lock.
I also think we can for now at least finish reviewing the first 2 
patches and submit them since as I said before they are not dealing with 
this topic and fixing existing race conditions. If you are OK with that 
I can send for review the last iteration of the first 2 patches where I 
wait for the last fence in ring mirror list.

Andrey

>

>> Andrey

>>>>> Andrey

>>>>>

>>> _______________________________________________

>>> amd-gfx mailing list

>>> amd-gfx@lists.freedesktop.org

>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

> _______________________________________________

> amd-gfx mailing list

> amd-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/amd-gfx