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

["Zhou, David(ChunMing)" <David1.Zhou@amd.com>]

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

-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