[Intel-xe] [PATCH v2 4/9] drm/sched: Split free_job into own work item

Christian König christian.koenig at amd.com
Fri Aug 25 08:02:32 UTC 2023


Am 25.08.23 um 04:58 schrieb Matthew Brost:
> On Fri, Aug 25, 2023 at 01:04:10AM +0200, Danilo Krummrich wrote:
>> On Thu, Aug 10, 2023 at 07:31:32PM -0700, Matthew Brost wrote:
>>> Rather than call free_job and run_job in same work item have a dedicated
>>> work item for each. This aligns with the design and intended use of work
>>> queues.
>>>
>>> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_main.c | 137 ++++++++++++++++++-------
>>>   include/drm/gpu_scheduler.h            |   8 +-
>>>   2 files changed, 106 insertions(+), 39 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>>> index cede47afc800..b67469eac179 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -213,11 +213,12 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
>>>    * drm_sched_rq_select_entity_rr - Select an entity which could provide a job to run
>>>    *
>>>    * @rq: scheduler run queue to check.
>>> + * @dequeue: dequeue selected entity
>>>    *
>>>    * Try to find a ready entity, returns NULL if none found.
>>>    */
>>>   static struct drm_sched_entity *
>>> -drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
>>> +drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq, bool dequeue)
>>>   {
>>>   	struct drm_sched_entity *entity;
>>>   
>>> @@ -227,8 +228,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
>>>   	if (entity) {
>>>   		list_for_each_entry_continue(entity, &rq->entities, list) {
>>>   			if (drm_sched_entity_is_ready(entity)) {
>>> -				rq->current_entity = entity;
>>> -				reinit_completion(&entity->entity_idle);
>>> +				if (dequeue) {
>>> +					rq->current_entity = entity;
>>> +					reinit_completion(&entity->entity_idle);
>>> +				}
>>>   				spin_unlock(&rq->lock);
>>>   				return entity;
>>>   			}
>>> @@ -238,8 +241,10 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
>>>   	list_for_each_entry(entity, &rq->entities, list) {
>>>   
>>>   		if (drm_sched_entity_is_ready(entity)) {
>>> -			rq->current_entity = entity;
>>> -			reinit_completion(&entity->entity_idle);
>>> +			if (dequeue) {
>>> +				rq->current_entity = entity;
>>> +				reinit_completion(&entity->entity_idle);
>>> +			}
>>>   			spin_unlock(&rq->lock);
>>>   			return entity;
>>>   		}
>>> @@ -257,11 +262,12 @@ drm_sched_rq_select_entity_rr(struct drm_sched_rq *rq)
>>>    * drm_sched_rq_select_entity_fifo - Select an entity which provides a job to run
>>>    *
>>>    * @rq: scheduler run queue to check.
>>> + * @dequeue: dequeue selected entity
>>>    *
>>>    * Find oldest waiting ready entity, returns NULL if none found.
>>>    */
>>>   static struct drm_sched_entity *
>>> -drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>>> +drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq, bool dequeue)
>>>   {
>>>   	struct rb_node *rb;
>>>   
>>> @@ -271,8 +277,10 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>>>   
>>>   		entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
>>>   		if (drm_sched_entity_is_ready(entity)) {
>>> -			rq->current_entity = entity;
>>> -			reinit_completion(&entity->entity_idle);
>>> +			if (dequeue) {
>>> +				rq->current_entity = entity;
>>> +				reinit_completion(&entity->entity_idle);
>>> +			}
>>>   			break;
>>>   		}
>>>   	}
>>> @@ -282,13 +290,54 @@ drm_sched_rq_select_entity_fifo(struct drm_sched_rq *rq)
>>>   }
>>>   
>>>   /**
>>> - * drm_sched_submit_queue - scheduler queue submission
>>> + * drm_sched_run_job_queue - queue job submission
>>>    * @sched: scheduler instance
>>>    */
>>> -static void drm_sched_submit_queue(struct drm_gpu_scheduler *sched)
>>> +static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
>>>   {
>>>   	if (!READ_ONCE(sched->pause_submit))
>>> -		queue_work(sched->submit_wq, &sched->work_submit);
>>> +		queue_work(sched->submit_wq, &sched->work_run_job);
>>> +}
>>> +
>>> +static struct drm_sched_entity *
>>> +drm_sched_select_entity(struct drm_gpu_scheduler *sched, bool dequeue);
>>> +
>>> +/**
>>> + * drm_sched_run_job_queue_if_ready - queue job submission if ready
>>> + * @sched: scheduler instance
>>> + */
>>> +static void drm_sched_run_job_queue_if_ready(struct drm_gpu_scheduler *sched)
>>> +{
>>> +	if (drm_sched_select_entity(sched, false))
>>> +		drm_sched_run_job_queue(sched);
>>> +}
>>> +
>>> +/**
>>> + * drm_sched_free_job_queue - queue free job
>>> + *
>>> + * @sched: scheduler instance to queue free job
>>> + */
>>> +static void drm_sched_free_job_queue(struct drm_gpu_scheduler *sched)
>>> +{
>>> +	if (!READ_ONCE(sched->pause_submit))
>>> +		queue_work(sched->submit_wq, &sched->work_free_job);
>>> +}
>>> +
>>> +/**
>>> + * drm_sched_free_job_queue_if_ready - queue free job if ready
>>> + *
>>> + * @sched: scheduler instance to queue free job
>>> + */
>>> +static void drm_sched_free_job_queue_if_ready(struct drm_gpu_scheduler *sched)
>>> +{
>>> +	struct drm_sched_job *job;
>>> +
>>> +	spin_lock(&sched->job_list_lock);
>>> +	job = list_first_entry_or_null(&sched->pending_list,
>>> +				       struct drm_sched_job, list);
>>> +	if (job && dma_fence_is_signaled(&job->s_fence->finished))
>>> +		drm_sched_free_job_queue(sched);
>>> +	spin_unlock(&sched->job_list_lock);
>>>   }
>>>   
>>>   /**
>>> @@ -310,7 +359,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job, int result)
>>>   	dma_fence_get(&s_fence->finished);
>>>   	drm_sched_fence_finished(s_fence, result);
>>>   	dma_fence_put(&s_fence->finished);
>>> -	drm_sched_submit_queue(sched);
>>> +	drm_sched_free_job_queue(sched);
>>>   }
>>>   
>>>   /**
>>> @@ -906,18 +955,19 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched)
>>>   void drm_sched_wakeup_if_can_queue(struct drm_gpu_scheduler *sched)
>>>   {
>>>   	if (drm_sched_can_queue(sched))
>>> -		drm_sched_submit_queue(sched);
>>> +		drm_sched_run_job_queue(sched);
>>>   }
>>>   
>>>   /**
>>>    * drm_sched_select_entity - Select next entity to process
>>>    *
>>>    * @sched: scheduler instance
>>> + * @dequeue: dequeue selected entity
>>>    *
>>>    * Returns the entity to process or NULL if none are found.
>>>    */
>>>   static struct drm_sched_entity *
>>> -drm_sched_select_entity(struct drm_gpu_scheduler *sched)
>>> +drm_sched_select_entity(struct drm_gpu_scheduler *sched, bool dequeue)
>>>   {
>>>   	struct drm_sched_entity *entity;
>>>   	int i;
>>> @@ -935,8 +985,10 @@ drm_sched_select_entity(struct drm_gpu_scheduler *sched)
>>>   	/* Kernel run queue has higher priority than normal run queue*/
>>>   	for (i = DRM_SCHED_PRIORITY_COUNT - 1; i >= DRM_SCHED_PRIORITY_MIN; i--) {
>>>   		entity = sched->sched_policy == DRM_SCHED_POLICY_FIFO ?
>>> -			drm_sched_rq_select_entity_fifo(&sched->sched_rq[i]) :
>>> -			drm_sched_rq_select_entity_rr(&sched->sched_rq[i]);
>>> +			drm_sched_rq_select_entity_fifo(&sched->sched_rq[i],
>>> +							dequeue) :
>>> +			drm_sched_rq_select_entity_rr(&sched->sched_rq[i],
>>> +						      dequeue);
>>>   		if (entity)
>>>   			break;
>>>   	}
>>> @@ -1024,30 +1076,44 @@ drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
>>>   EXPORT_SYMBOL(drm_sched_pick_best);
>>>   
>>>   /**
>>> - * drm_sched_main - main scheduler thread
>>> + * drm_sched_free_job_work - worker to call free_job
>>>    *
>>> - * @param: scheduler instance
>>> + * @w: free job work
>>>    */
>>> -static void drm_sched_main(struct work_struct *w)
>>> +static void drm_sched_free_job_work(struct work_struct *w)
>>>   {
>>>   	struct drm_gpu_scheduler *sched =
>>> -		container_of(w, struct drm_gpu_scheduler, work_submit);
>>> -	struct drm_sched_entity *entity;
>>> +		container_of(w, struct drm_gpu_scheduler, work_free_job);
>>>   	struct drm_sched_job *cleanup_job;
>>> -	int r;
>>>   
>>>   	if (READ_ONCE(sched->pause_submit))
>>>   		return;
>>>   
>>>   	cleanup_job = drm_sched_get_cleanup_job(sched);
>> I tried this patch with Nouveau and found a race condition:
>>
>> In drm_sched_run_job_work() the job is added to the pending_list via
>> drm_sched_job_begin(), then the run_job() callback is called and the scheduled
>> fence is signaled.
>>
>> However, in parallel drm_sched_get_cleanup_job() might be called from
>> drm_sched_free_job_work(), which picks the first job from the pending_list and
>> for the next job on the pending_list sets the scheduled fence' timestamp field.

Well why can this happen in parallel? Either the work items are 
scheduled to a single threaded work queue or you have protected the 
pending list with some locks.

Just moving the free_job into a separate work item without such 
precautions won't work because of quite a bunch of other reasons as well.

>>
>> The job can be on the pending_list, but the scheduled fence might not yet be
>> signaled. The call to actually signal the fence will subsequently fault because
>> it will try to dereference the timestamp.
>>
>> I'm not sure what's the best way to fix this, maybe it's enough to re-order
>> signalling the scheduled fence and adding the job to the pending_list. Not sure
>> if this has other implications though.
>>
> We really want the job on the pending list before calling run_job.
>
> I'm thinking we just delete the updating of the timestamp, not sure why
> this is useful.

This is used for calculating how long each job has spend on the hw, so 
big NAK to deleting this.

Regards,
Christian.

>
> Or we could do something like this where we try to update the timestamp,
> if we can't update the timestamp run_job worker will do it anyways.
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 67e0fb6e7d18..54bd3e88f139 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -1074,8 +1074,10 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
>                                                  typeof(*next), list);
>
>                  if (next) {
> -                       next->s_fence->scheduled.timestamp =
> -                               job->s_fence->finished.timestamp;
> +                       if (test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT,
> +                                    &next->s_fence->scheduled.flags))
> +                               next->s_fence->scheduled.timestamp =
> +                                       job->s_fence->finished.timestamp;
>                          /* start TO timer for next job */
>                          drm_sched_start_timeout(sched);
>                  }
>
> I guess I'm leaning towards the latter option.
>
> Matt
>
>> - Danilo
>>
>>> -	entity = drm_sched_select_entity(sched);
>>> +	if (cleanup_job) {
>>> +		sched->ops->free_job(cleanup_job);
>>> +
>>> +		drm_sched_free_job_queue_if_ready(sched);
>>> +		drm_sched_run_job_queue_if_ready(sched);
>>> +	}
>>> +}
>>>   
>>> -	if (!entity && !cleanup_job)
>>> -		return;	/* No more work */
>>> +/**
>>> + * drm_sched_run_job_work - worker to call run_job
>>> + *
>>> + * @w: run job work
>>> + */
>>> +static void drm_sched_run_job_work(struct work_struct *w)
>>> +{
>>> +	struct drm_gpu_scheduler *sched =
>>> +		container_of(w, struct drm_gpu_scheduler, work_run_job);
>>> +	struct drm_sched_entity *entity;
>>> +	int r;
>>>   
>>> -	if (cleanup_job)
>>> -		sched->ops->free_job(cleanup_job);
>>> +	if (READ_ONCE(sched->pause_submit))
>>> +		return;
>>>   
>>> +	entity = drm_sched_select_entity(sched, true);
>>>   	if (entity) {
>>>   		struct dma_fence *fence;
>>>   		struct drm_sched_fence *s_fence;
>>> @@ -1056,9 +1122,7 @@ static void drm_sched_main(struct work_struct *w)
>>>   		sched_job = drm_sched_entity_pop_job(entity);
>>>   		if (!sched_job) {
>>>   			complete_all(&entity->entity_idle);
>>> -			if (!cleanup_job)
>>> -				return;	/* No more work */
>>> -			goto again;
>>> +			return;	/* No more work */
>>>   		}
>>>   
>>>   		s_fence = sched_job->s_fence;
>>> @@ -1088,10 +1152,8 @@ static void drm_sched_main(struct work_struct *w)
>>>   		}
>>>   
>>>   		wake_up(&sched->job_scheduled);
>>> +		drm_sched_run_job_queue_if_ready(sched);
>>>   	}
>>> -
>>> -again:
>>> -	drm_sched_submit_queue(sched);
>>>   }
>>>   
>>>   /**
>>> @@ -1150,7 +1212,8 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>>>   	spin_lock_init(&sched->job_list_lock);
>>>   	atomic_set(&sched->hw_rq_count, 0);
>>>   	INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout);
>>> -	INIT_WORK(&sched->work_submit, drm_sched_main);
>>> +	INIT_WORK(&sched->work_run_job, drm_sched_run_job_work);
>>> +	INIT_WORK(&sched->work_free_job, drm_sched_free_job_work);
>>>   	atomic_set(&sched->_score, 0);
>>>   	atomic64_set(&sched->job_id_count, 0);
>>>   	sched->pause_submit = false;
>>> @@ -1275,7 +1338,8 @@ EXPORT_SYMBOL(drm_sched_submit_ready);
>>>   void drm_sched_submit_stop(struct drm_gpu_scheduler *sched)
>>>   {
>>>   	WRITE_ONCE(sched->pause_submit, true);
>>> -	cancel_work_sync(&sched->work_submit);
>>> +	cancel_work_sync(&sched->work_run_job);
>>> +	cancel_work_sync(&sched->work_free_job);
>>>   }
>>>   EXPORT_SYMBOL(drm_sched_submit_stop);
>>>   
>>> @@ -1287,6 +1351,7 @@ EXPORT_SYMBOL(drm_sched_submit_stop);
>>>   void drm_sched_submit_start(struct drm_gpu_scheduler *sched)
>>>   {
>>>   	WRITE_ONCE(sched->pause_submit, false);
>>> -	queue_work(sched->submit_wq, &sched->work_submit);
>>> +	queue_work(sched->submit_wq, &sched->work_run_job);
>>> +	queue_work(sched->submit_wq, &sched->work_free_job);
>>>   }
>>>   EXPORT_SYMBOL(drm_sched_submit_start);
>>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>>> index 04eec2d7635f..fbc083a92757 100644
>>> --- a/include/drm/gpu_scheduler.h
>>> +++ b/include/drm/gpu_scheduler.h
>>> @@ -487,9 +487,10 @@ struct drm_sched_backend_ops {
>>>    *                 finished.
>>>    * @hw_rq_count: the number of jobs currently in the hardware queue.
>>>    * @job_id_count: used to assign unique id to the each job.
>>> - * @submit_wq: workqueue used to queue @work_submit
>>> + * @submit_wq: workqueue used to queue @work_run_job and @work_free_job
>>>    * @timeout_wq: workqueue used to queue @work_tdr
>>> - * @work_submit: schedules jobs and cleans up entities
>>> + * @work_run_job: schedules jobs
>>> + * @work_free_job: cleans up jobs
>>>    * @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the
>>>    *            timeout interval is over.
>>>    * @pending_list: the list of jobs which are currently in the job queue.
>>> @@ -518,7 +519,8 @@ struct drm_gpu_scheduler {
>>>   	atomic64_t			job_id_count;
>>>   	struct workqueue_struct		*submit_wq;
>>>   	struct workqueue_struct		*timeout_wq;
>>> -	struct work_struct		work_submit;
>>> +	struct work_struct		work_run_job;
>>> +	struct work_struct		work_free_job;
>>>   	struct delayed_work		work_tdr;
>>>   	struct list_head		pending_list;
>>>   	spinlock_t			job_list_lock;
>>> -- 
>>> 2.34.1
>>>



More information about the Intel-xe mailing list