Re: [PATCH v3 2/2] drm/sched: Rework HW fence processing.

["Grodzovsky, Andrey" <Andrey.Grodzovsky-5C7GfCeVMHo@public.gmane.org>]

BTW, the problem I pointed out with drm_sched_entity_kill_jobs_cb is not 
an issue with this patch set since it removes the cb from 
s_fence->finished in general so we only free the job once - directly 
from drm_sched_entity_kill_jobs_cb.

Andrey


On 12/11/2018 11:20 AM, Christian König wrote:
> Yeah, completely correct explained.
>
> I was unfortunately really busy today, but going to give that a look 
> as soon as I have time.
>
> Christian.
>
> Am 11.12.18 um 17:01 schrieb Grodzovsky, Andrey:
>> A I understand you say that by the time the fence callback runs the job
>> might have already been released,
>>
>> but how so if the job gets released from drm_sched_job_finish work
>> handler in the normal flow - so, after the HW
>>
>> fence (s_fence->parent) cb is executed. Other 2 flows are error use
>> cases where amdgpu_job_free is called directly in which
>>
>> cases I assume the job wasn't submitted to HW. Last flow I see is
>> drm_sched_entity_kill_jobs_cb and here I actually see a problem
>>
>> with the code as it's today - drm_sched_fence_finished is called which
>> will trigger s_fence->finished callback to run which today
>>
>> schedules drm_sched_job_finish which releases the job, but we don't even
>> wait for that and call free_job cb directly from
>>
>> after that which seems wrong to me.
>>
>> Andrey
>>
>>
>> On 12/10/2018 09:45 PM, Zhou, David(ChunMing) wrote:
>>> I don't think adding cb to sched job would work as soon as their 
>>> lifetime is different with fence.
>>> Unless you make the sched job reference, otherwise we will get 
>>> trouble sooner or later.
>>>
>>> -David
>>>
>>>> -----Original Message-----
>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>>> Andrey Grodzovsky
>>>> Sent: Tuesday, December 11, 2018 5:44 AM
>>>> To: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org;
>>>> ckoenig.leichtzumerken@gmail.com; eric@anholt.net;
>>>> etnaviv@lists.freedesktop.org
>>>> Cc: Zhou, David(ChunMing) <David1.Zhou@amd.com>; Liu, Monk
>>>> <Monk.Liu@amd.com>; Grodzovsky, Andrey
>>>> <Andrey.Grodzovsky@amd.com>
>>>> Subject: [PATCH v3 2/2] 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
>>>>
>>>> Suggested-by: Christian Koenig <Christian.Koenig@amd.com>
>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/scheduler/sched_main.c | 58 
>>>> ++++++++++++++++----------
>>>> --------
>>>>    include/drm/gpu_scheduler.h            |  6 ++--
>>>>    2 files changed, 30 insertions(+), 34 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>> index cdf95e2..f0c1f32 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);
>>>> @@ -359,12 +346,11 @@ 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);
>>>> -        }
>>>> -        else {
>>>> +        } else {
>>>>                /* TODO Is it get/put neccessey here ? */
>>>> dma_fence_get(&s_job->s_fence->finished);
>>>>                list_add(&s_job->finish_node, &wait_list); @@ -
>>>> 417,31 +403,34 @@ EXPORT_SYMBOL(drm_sched_stop);  void
>>>> drm_sched_start(struct drm_gpu_scheduler *sched, bool unpark_only)  {
>>>>        struct drm_sched_job *s_job, *tmp;
>>>> -    unsigned long flags;
>>>>        int r;
>>>>
>>>>        if (unpark_only)
>>>>            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);
>>>> @@ -590,18 +579,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);
>>>>    }
>>>>
>>>>    /**
>>>> @@ -664,16 +662,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 c94b592..f29aa1c 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.
>>>> @@ -182,6 +178,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
>>>> @@ -199,6 +196,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
>>>>
>>>> _______________________________________________
>>>> 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