Re: [RFC PATCH 2/6] drm/sched: Teach scheduler about DMA_RESV_USAGE_PREEMPT

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Nov 12, 2024 at 10:06:21AM +0100, Philipp Stanner wrote:
> Hi Matt,
> 
> On Sat, 2024-11-09 at 09:29 -0800, Matthew Brost wrote:
> > Follow the semantics of DMA_RESV_USAGE_PREEMPT in the DRM scheduler
> > by
> > storing preemptive fences in a dedicated xarray, which is waited on
> > after all other fences are signaled. In addition to following these
> > semantics, pipeline preemptive fences by enabling signaling on all
> > preemptive fences before waiting on any of them.
> 
> the commit message lacks the *motivation*. Why is the current state a
> problem, why is that feature needed etc.
> 

Yes, I do this in the cover letter but this is missing here. Will add in
next rev.

> > 
> > Cc: Philipp Stanner <pstanner@redhat.com>
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Cc: Luben Tuikov <ltuikov89@gmail.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Simona Vetter <simona.vetter@ffwll.ch>
> > Cc: Christian Koenig <christian.koenig@amd.com>
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/scheduler/sched_entity.c | 29 ++++++++++++--
> >  drivers/gpu/drm/scheduler/sched_main.c   | 48 ++++++++++++++++------
> > --
> >  include/drm/gpu_scheduler.h              | 15 ++++++++
> >  3 files changed, 73 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
> > b/drivers/gpu/drm/scheduler/sched_entity.c
> > index 69bcf0e99d57..c6c4978aa65a 100644
> > --- a/drivers/gpu/drm/scheduler/sched_entity.c
> > +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> > @@ -201,11 +201,13 @@ static void
> > drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
> >  	struct drm_sched_job *job = container_of(cb, struct
> > drm_sched_job,
> >  						 finish_cb);
> >  	unsigned long index;
> > +	struct xarray *dependencies = &job->dependencies;
> >  
> >  	dma_fence_put(f);
> >  
> > +again:
> >  	/* Wait for all dependencies to avoid data corruptions */
> > -	xa_for_each(&job->dependencies, index, f) {
> > +	xa_for_each(dependencies, index, f) {
> >  		struct drm_sched_fence *s_fence =
> > to_drm_sched_fence(f);
> >  
> >  		if (s_fence && f == &s_fence->scheduled) {
> > @@ -223,7 +225,7 @@ static void drm_sched_entity_kill_jobs_cb(struct
> > dma_fence *f,
> >  			dma_fence_put(&s_fence->scheduled);
> >  		}
> >  
> > -		xa_erase(&job->dependencies, index);
> > +		xa_erase(dependencies, index);
> >  		if (f && !dma_fence_add_callback(f, &job->finish_cb,
> >  						
> > drm_sched_entity_kill_jobs_cb))
> >  			return;
> > @@ -231,6 +233,11 @@ static void drm_sched_entity_kill_jobs_cb(struct
> > dma_fence *f,
> >  		dma_fence_put(f);
> >  	}
> >  
> > +	if (dependencies != &job->preempt_dependencies) {
> > +		dependencies = &job->preempt_dependencies;
> > +		goto again;
> > +	}
> > +
> 
> I think this should have a comment. It can only trigger once, right? So
> I guess that's why it doesn't make sense considering making it a loop
> instead of goto upwards?
> 

Yes, can only trigger once. I personally don't mind goto while others
find them offensive.

> >  	INIT_WORK(&job->work, drm_sched_entity_kill_jobs_work);
> >  	schedule_work(&job->work);
> >  }
> > @@ -456,17 +463,33 @@ drm_sched_job_dependency(struct drm_sched_job
> > *job,
> >  			 struct drm_sched_entity *entity)
> >  {
> >  	struct dma_fence *f;
> > +	struct xarray *dependencies;
> > +
> > +again:
> > +	dependencies = job->resolve_preempt_dependencies ?
> > +		&job->preempt_dependencies : &job->dependencies;
> 
> I don't think it's good to use the ternary operator for such long
> statements.
> 
> if-else is more readable.
>

Sure.
 
> >  
> >  	/* We keep the fence around, so we can iterate over all
> > dependencies
> >  	 * in drm_sched_entity_kill_jobs_cb() to ensure all deps are
> > signaled
> >  	 * before killing the job.
> >  	 */
> > -	f = xa_load(&job->dependencies, job->last_dependency);
> > +	f = xa_load(dependencies, job->last_dependency);
> >  	if (f) {
> >  		job->last_dependency++;
> >  		return dma_fence_get(f);
> >  	}
> >  
> > +	/* Switch resolving preempt dependencies pipelining
> > signaling */
> 
> I don't understand this comment. I guess you want to say that this section resolves preemption dependencies for the (fence) pipeline signaling?
> 

'Switch to resolving preempt dependencies. Enabling signaling on all
preempt dependencies to pipeline the hardware preemption'

Is that better more / clear?

> > +	if (!job->resolve_preempt_dependencies) {
> > +		unsigned long index;
> > +
> > +		xa_for_each(&job->preempt_dependencies, index, f)
> > +			dma_fence_enable_sw_signaling(f);
> > +
> > +		job->resolve_preempt_dependencies = true;
> 
> Hm, is this set to false ever again? It seems it doesn't need to? So
> the goto again is only ever triggered once?
> 

resolve_preempt_dependencies can only go from 0 - > 1 exactly one time.

> > +		goto again;
> > +	}
> > +
> >  	if (job->sched->ops->prepare_job)
> >  		return job->sched->ops->prepare_job(job, entity);
> >  
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > b/drivers/gpu/drm/scheduler/sched_main.c
> > index 7ce25281c74c..eceb9b8c6f5f 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -829,6 +829,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
> >  	INIT_LIST_HEAD(&job->list);
> >  
> >  	xa_init_flags(&job->dependencies, XA_FLAGS_ALLOC);
> > +	xa_init_flags(&job->preempt_dependencies, XA_FLAGS_ALLOC);
> >  
> >  	return 0;
> >  }
> > @@ -864,21 +865,14 @@ void drm_sched_job_arm(struct drm_sched_job
> > *job)
> >  }
> >  EXPORT_SYMBOL(drm_sched_job_arm);
> >  
> > -/**
> > - * drm_sched_job_add_dependency - adds the fence as a job dependency
> > - * @job: scheduler job to add the dependencies to
> > - * @fence: the dma_fence to add to the list of dependencies.
> > - *
> > - * Note that @fence is consumed in both the success and error cases.
> > - *
> > - * Returns:
> > - * 0 on success, or an error on failing to expand the array.
> > - */
> > -int drm_sched_job_add_dependency(struct drm_sched_job *job,
> > -				 struct dma_fence *fence)
> > +static int __drm_sched_job_add_dependency(struct drm_sched_job *job,
> > +					  struct dma_fence *fence,
> > +					  bool is_preempt)
> >  {
> >  	struct dma_fence *entry;
> >  	unsigned long index;
> > +	struct xarray *dependencies = is_preempt ? &job-
> > >preempt_dependencies :
> > +		&job->dependencies;
> 
> Same – is better as an if-else below
> 

Sure.

> >  	u32 id = 0;
> >  	int ret;
> >  
> > @@ -889,25 +883,41 @@ int drm_sched_job_add_dependency(struct
> > drm_sched_job *job,
> >  	 * This lets the size of the array of deps scale with the
> > number of
> >  	 * engines involved, rather than the number of BOs.
> >  	 */
> > -	xa_for_each(&job->dependencies, index, entry) {
> > +	xa_for_each(dependencies, index, entry) {
> >  		if (entry->context != fence->context)
> >  			continue;
> >  
> >  		if (dma_fence_is_later(fence, entry)) {
> >  			dma_fence_put(entry);
> > -			xa_store(&job->dependencies, index, fence,
> > GFP_KERNEL);
> > +			xa_store(dependencies, index, fence,
> > GFP_KERNEL);
> >  		} else {
> >  			dma_fence_put(fence);
> >  		}
> >  		return 0;
> >  	}
> >  
> > -	ret = xa_alloc(&job->dependencies, &id, fence, xa_limit_32b,
> > GFP_KERNEL);
> > +	ret = xa_alloc(dependencies, &id, fence, xa_limit_32b,
> > GFP_KERNEL);
> >  	if (ret != 0)
> >  		dma_fence_put(fence);
> >  
> >  	return ret;
> >  }
> > +
> > +/**
> > + * drm_sched_job_add_dependency - adds the fence as a job dependency
> > + * @job: scheduler job to add the dependencies to
> > + * @fence: the dma_fence to add to the list of dependencies.
> > + *
> > + * Note that @fence is consumed in both the success and error cases.
> > + *
> > + * Returns:
> > + * 0 on success, or an error on failing to expand the array.
> > + */
> > +int drm_sched_job_add_dependency(struct drm_sched_job *job,
> > +				 struct dma_fence *fence)
> > +{
> > +	return __drm_sched_job_add_dependency(job, fence, false);
> > +}
> >  EXPORT_SYMBOL(drm_sched_job_add_dependency);
> >  
> >  /**
> > @@ -963,7 +973,9 @@ int drm_sched_job_add_resv_dependencies(struct
> > drm_sched_job *job,
> >  	dma_resv_for_each_fence(&cursor, resv, usage, fence) {
> >  		/* Make sure to grab an additional ref on the added
> > fence */
> >  		dma_fence_get(fence);
> > -		ret = drm_sched_job_add_dependency(job, fence);
> > +		ret = __drm_sched_job_add_dependency(job, fence,
> > +						    
> > cursor.fence_usage ==
> > +						    
> > DMA_RESV_USAGE_PREEMPT);
> >  		if (ret) {
> >  			dma_fence_put(fence);
> >  			return ret;
> > @@ -1030,6 +1042,10 @@ void drm_sched_job_cleanup(struct
> > drm_sched_job *job)
> >  	}
> >  	xa_destroy(&job->dependencies);
> >  
> > +	xa_for_each(&job->preempt_dependencies, index, fence) {
> > +		dma_fence_put(fence);
> > +	}
> > +	xa_destroy(&job->preempt_dependencies);
> >  }
> >  EXPORT_SYMBOL(drm_sched_job_cleanup);
> >  
> > diff --git a/include/drm/gpu_scheduler.h
> > b/include/drm/gpu_scheduler.h
> > index 95e17504e46a..de16cf6b1869 100644
> > --- a/include/drm/gpu_scheduler.h
> > +++ b/include/drm/gpu_scheduler.h
> > @@ -353,6 +353,13 @@ struct drm_sched_job {
> >  
> >  	u32				credits;
> >  
> > +	/**
> > +	 * @resolve_preempt_dependencies:
> > +	 *
> > +	 * Job is currently resolving preempt dependencies.
> > +	 */
> > +	bool				resolve_preempt_dependencies
> > ;
> 
> I think this should be called "resolving_preempt_dependencies". Just 2
> letters more and it emphasizes that this is happening "currently".
> 

That is more clear. Will rename.

Matt

> 
> P.
> 
> > +
> >  	/*
> >  	 * work is used only after finish_cb has been used and will
> > not be
> >  	 * accessed anymore.
> > @@ -376,6 +383,14 @@ struct drm_sched_job {
> >  	 */
> >  	struct xarray			dependencies;
> >  
> > +	/**
> > +	 * @preempt_dependencies:
> > +	 *
> > +	 * Contains the dependencies as struct dma_fence for this
> > job which are
> > +	 * preempt fences.
> > +	 */
> > +	struct xarray			preempt_dependencies;
> > +
> >  	/** @last_dependency: tracks @dependencies as they signal */
> >  	unsigned long			last_dependency;
> >  
>