Am 07.12.18 um 04:18 schrieb Zhou, David(ChunMing):

-----Original Message-----
From: dri-devel <dri-devel-boun...@lists.freedesktop.org> On Behalf Of
Andrey Grodzovsky
Sent: Friday, December 07, 2018 1:41 AM
To: dri-de...@lists.freedesktop.org; amd-gfx@lists.freedesktop.org;
ckoenig.leichtzumer...@gmail.com; e...@anholt.net;
etna...@lists.freedesktop.org
Cc: Liu, Monk <monk....@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.koe...@amd.com>
Signed-off-by: Andrey Grodzovsky <andrey.grodzov...@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.


-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-de...@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

Reply via email to