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

["Christian König" <ckoenig.leichtzumerken-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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