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

["Koenig, Christian" <Christian.Koenig@amd.com>]

Am 07.12.18 um 16:22 schrieb Grodzovsky, Andrey:
>
> On 12/07/2018 03:19 AM, Christian König wrote:
>> Am 07.12.18 um 04:18 schrieb Zhou, David(ChunMing):
>>>> -----Original Message-----
>>>> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
>>>> Andrey Grodzovsky
>>>> Sent: Friday, December 07, 2018 1:41 AM
>>>> To: dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org;
>>>> ckoenig.leichtzumerken@gmail.com; eric@anholt.net;
>>>> etnaviv@lists.freedesktop.org
>>>> Cc: Liu, Monk <Monk.Liu@amd.com>
>>>> Subject: [PATCH 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.
>>>>
>>>> Suggested-by: Christian Koenig <Christian.Koenig@amd.com>
>>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/scheduler/sched_fence.c |  4 +++-
>>>> drivers/gpu/drm/scheduler/sched_main.c  | 39 ++++++++++++++++----------
>>>> -------
>>>>    include/drm/gpu_scheduler.h             | 10 +++++++--
>>>>    3 files changed, 30 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_fence.c
>>>> b/drivers/gpu/drm/scheduler/sched_fence.c
>>>> index d8d2dff..e62c239 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_fence.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_fence.c
>>>> @@ -151,7 +151,8 @@ struct drm_sched_fence
>>>> *to_drm_sched_fence(struct dma_fence *f)
>>>> EXPORT_SYMBOL(to_drm_sched_fence);
>>>>
>>>>    struct drm_sched_fence *drm_sched_fence_create(struct
>>>> drm_sched_entity *entity,
>>>> -                           void *owner)
>>>> +                           void *owner,
>>>> +                           struct drm_sched_job *s_job)
>>>>    {
>>>>        struct drm_sched_fence *fence = NULL;
>>>>        unsigned seq;
>>>> @@ -163,6 +164,7 @@ struct drm_sched_fence
>>>> *drm_sched_fence_create(struct drm_sched_entity *entity,
>>>>        fence->owner = owner;
>>>>        fence->sched = entity->rq->sched;
>>>>        spin_lock_init(&fence->lock);
>>>> +    fence->s_job = s_job;
>>>>
>>>>        seq = atomic_inc_return(&entity->fence_seq);
>>>>        dma_fence_init(&fence->scheduled,
>>>> &drm_sched_fence_ops_scheduled, diff --git
>>>> a/drivers/gpu/drm/scheduler/sched_main.c
>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>> index 8fb7f86..2860037 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>> @@ -284,31 +284,17 @@ 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);
>>>>
>>>>        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);
>>>> @@ -418,13 +404,17 @@ void drm_sched_start(struct drm_gpu_scheduler
>>>> *sched, bool unpark_only)  {
>>>>        struct drm_sched_job *s_job, *tmp;
>>>>        bool found_guilty = false;
>>>> -    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 any in flight jobs who didn't signal yet. 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;
>>>> @@ -453,7 +443,6 @@ void drm_sched_start(struct drm_gpu_scheduler
>>>> *sched, bool unpark_only)
>>>>        }
>>>>
>>>>        drm_sched_start_timeout(sched);
>>>> -    spin_unlock_irqrestore(&sched->job_list_lock, flags);
>>>>
>>>>    unpark:
>>>>        kthread_unpark(sched->thread);
>>>> @@ -505,7 +494,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
>>>>        job->sched = sched;
>>>>        job->entity = entity;
>>>>        job->s_priority = entity->rq - sched->sched_rq;
>>>> -    job->s_fence = drm_sched_fence_create(entity, owner);
>>>> +    job->s_fence = drm_sched_fence_create(entity, owner, job);
>>>>        if (!job->s_fence)
>>>>            return -ENOMEM;
>>>>        job->id = atomic64_inc_return(&sched->job_id_count);
>>>> @@ -593,15 +582,25 @@ 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_gpu_scheduler *sched = s_fence->sched;
>>>> +    struct drm_sched_job *s_job = s_fence->s_job;
>>> Seems we are back to original, Christian argued s_fence and s_job are
>>> with different lifetime , Do their lifetime become same now?
>> No, that still doesn't work. The scheduler fence lives much longer
>> than the job, so we would have a dangling pointer here.
>>
>> The real question is why we actually need that? I mean we just need to
>> move the callback structure into the job, don't we?
>>
>> Christian.
> So let's say I move the .cb from drm_sched_fence to drm_sched_job, and
> there it's ok to reference
> job->s_fence because s_fence life as at least as the job, right ?

Exactly. Well, we could actually make the live of s_fence a bit shorter 
and drop it as soon as it is signaled.

Christian.

>
> Andrey
>
>>> -David
>>>
>>>> +    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);
>>>>    }
>>>>
>>>>    /**
>>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>>> index c94b592..23855c6 100644
>>>> --- a/include/drm/gpu_scheduler.h
>>>> +++ b/include/drm/gpu_scheduler.h
>>>> @@ -115,6 +115,8 @@ struct drm_sched_rq {
>>>>        struct drm_sched_entity        *current_entity;
>>>>    };
>>>>
>>>> +struct drm_sched_job;
>>>> +
>>>>    /**
>>>>     * struct drm_sched_fence - fences corresponding to the scheduling
>>>> of a job.
>>>>     */
>>>> @@ -160,6 +162,9 @@ struct drm_sched_fence {
>>>>             * @owner: job owner for debugging
>>>>             */
>>>>        void                *owner;
>>>> +
>>>> +    /* Back pointer to owning job */
>>>> +    struct drm_sched_job         *s_job;
>>>>    };
>>>>
>>>>    struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f); @@
>>>> -330,8 +335,9 @@ void drm_sched_entity_set_priority(struct
>>>> drm_sched_entity *entity,
>>>>                       enum drm_sched_priority priority);  bool
>>>> drm_sched_entity_is_ready(struct drm_sched_entity *entity);
>>>>
>>>> -struct drm_sched_fence *drm_sched_fence_create(
>>>> -    struct drm_sched_entity *s_entity, void *owner);
>>>> +struct drm_sched_fence *drm_sched_fence_create(struct
>>>> drm_sched_entity *s_entity,
>>>> +                           void *owner,
>>>> +                           struct drm_sched_job *s_job);
>>>>    void drm_sched_fence_scheduled(struct drm_sched_fence *fence);  void
>>>> drm_sched_fence_finished(struct drm_sched_fence *fence);
>>>>
>>>> -- 
>>>> 2.7.4
>>>>
>>>> _______________________________________________
>>>> 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