Re: [PATCH v2] drm/xe/ufence: Kick ufence immediately when possible

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

On Thu, Oct 17, 2024 at 11:53:39PM +0000, Matthew Brost wrote:
> On Thu, Oct 17, 2024 at 11:42:37AM +0200, Nirmoy Das wrote:
> > If the backing fence is signaled then signal ufence soon with
> > system_wq. This should reduce load from the xe ordered_wq and also
> > won't block signaling a ufence which doesn't require any serialization.
> > 
> > v2: fix system_wq typo
> > 
> > Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1630
> > Cc: Matthew Auld <matthew.auld@intel.com>
> > gc: Matthew Brost <matthew.brost@intel.com>
> > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_sync.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
> > index c6cf227ead40..1a7907293088 100644
> > --- a/drivers/gpu/drm/xe/xe_sync.c
> > +++ b/drivers/gpu/drm/xe/xe_sync.c
> > @@ -89,10 +89,14 @@ static void user_fence_worker(struct work_struct *w)
> >  	user_fence_put(ufence);
> >  }
> >  
> > -static void kick_ufence(struct xe_user_fence *ufence, struct dma_fence *fence)
> > +static void kick_ufence(struct xe_user_fence *ufence, struct dma_fence *fence,
> > +			bool signaled)
> 
> Predantic but maybe replace 'bool signaled' which a flag. We really want
> to avoid bools if possible.
> 
> e.g.
> 
> #define UFENCE_FLAG_SIGNALED	BIT(0)
> 

Sorry double reply, 'signaled' not actually relavent here as in both
calling cases the fence is signaled. Rather we want to pick between a
worker (needed if in a IRQ context) and doing it directly. Maybe
rename this to something more relavent indicating that.

Matt

> static void kick_ufence(struct xe_user_fence *ufence, struct dma_fence *fence,
> 			unsigned int flags)
> {
> 	...
> 	if (flags & UFENCE_FLAG_SIGNALED)
> 		...
> 	else
> 		...
> 	...	
> }
> 
> >  {
> >  	INIT_WORK(&ufence->worker, user_fence_worker);
> > -	queue_work(ufence->xe->ordered_wq, &ufence->worker);
> > +	if (signaled)
> > +		queue_work(system_wq, &ufence->worker);
> 
> I don't think you need the work queue here, just call the work func directly.
> 
> s/queue_work(system_wq, &ufence->worker)/user_fence_worker(&ufence->worker)
> 
> Matt
> 
> > +	else
> > +		queue_work(ufence->xe->ordered_wq, &ufence->worker);
> >  	dma_fence_put(fence);
> >  }
> >  
> > @@ -100,7 +104,7 @@ static void user_fence_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
> >  {
> >  	struct xe_user_fence *ufence = container_of(cb, struct xe_user_fence, cb);
> >  
> > -	kick_ufence(ufence, fence);
> > +	kick_ufence(ufence, fence, false);
> >  }
> >  
> >  int xe_sync_entry_parse(struct xe_device *xe, struct xe_file *xef,
> > @@ -236,7 +240,7 @@ void xe_sync_entry_signal(struct xe_sync_entry *sync, struct dma_fence *fence)
> >  		err = dma_fence_add_callback(fence, &sync->ufence->cb,
> >  					     user_fence_cb);
> >  		if (err == -ENOENT) {
> > -			kick_ufence(sync->ufence, fence);
> > +			kick_ufence(sync->ufence, fence, true);
> >  		} else if (err) {
> >  			XE_WARN_ON("failed to add user fence");
> >  			user_fence_put(sync->ufence);
> > -- 
> > 2.46.0
> >