Re: [RFC v1 7/9] drm/xe/exec: Switch hw engine group execution mode upon job submission

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

On Wed, Jul 17, 2024 at 10:57:06PM +0000, Matthew Brost wrote:
> On Wed, Jul 17, 2024 at 03:07:28PM +0200, Francois Dugast wrote:
> > Update the current execution mode of the hw engine group which will be
> > used to run the job that is about to be submitted. This triggers the
> > required operations to ensure mutual exclusion of executions modes in
> > this hw engine group.
> > 
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_exec.c      | 14 +++++++++++++-
> >  drivers/gpu/drm/xe/xe_hw_engine.c | 13 +++++++++++++
> >  drivers/gpu/drm/xe/xe_hw_engine.h |  2 ++
> >  3 files changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/xe_exec.c
> > index 2d72cdec3a0b..35418b5d1f5c 100644
> > --- a/drivers/gpu/drm/xe/xe_exec.c
> > +++ b/drivers/gpu/drm/xe/xe_exec.c
> > @@ -14,6 +14,7 @@
> >  #include "xe_bo.h"
> >  #include "xe_device.h"
> >  #include "xe_exec_queue.h"
> > +#include "xe_hw_engine.h"
> >  #include "xe_macros.h"
> >  #include "xe_ring_ops_types.h"
> >  #include "xe_sched_job.h"
> > @@ -124,6 +125,8 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >  	bool write_locked, skip_retry = false;
> >  	ktime_t end = 0;
> >  	int err = 0;
> > +	struct xe_hw_engine_group *group;
> > +	enum xe_hw_engine_group_execution_mode mode;
> >  
> >  	if (XE_IOCTL_DBG(xe, args->extensions) ||
> >  	    XE_IOCTL_DBG(xe, args->pad[0] || args->pad[1] || args->pad[2]) ||
> > @@ -182,6 +185,13 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >  		}
> >  	}
> >  
> > +	group = q->hwe->hw_engine_group;
> > +	mode = xe_hw_engine_group_find_exec_mode(q);
> > +
> 
> So you only need to call xe_hw_engine_group_get_mode if in dma-fence
> mode as LR submissions are allowed in the suspended state. They are just
> held in GuC until submission is enabled.
> 
>

You will have to kick the worker though to resume if in fault mode
though. Kinda a trade off I guess but I'd lean towards just kicking the
worker.

> > +	err = xe_hw_engine_group_get_mode(group, mode);
> > +	if (err)
> > +		goto err_syncs;
> > +
> >  retry:
> >  	if (!xe_vm_in_lr_mode(vm) && xe_vm_userptr_check_repin(vm)) {
> >  		err = down_write_killable(&vm->lock);
> > @@ -199,7 +209,7 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >  		downgrade_write(&vm->lock);
> >  		write_locked = false;
> >  		if (err)
> > -			goto err_unlock_list;
> > +			goto err_hw_exec_mode;
> >  	}
> >  
> >  	if (!args->num_batch_buffer) {
> > @@ -324,6 +334,8 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >  	up_read(&vm->lock);
> >  	if (err == -EAGAIN && !skip_retry)
> >  		goto retry;
> > +err_hw_exec_mode:
> > +	xe_hw_engine_group_put(group);
> >  err_syncs:
> >  	for (i = 0; i < num_syncs; i++)
> >  		xe_sync_entry_cleanup(&syncs[i]);
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c
> > index 4f539711357a..e6c755a04fd8 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > @@ -1345,3 +1345,16 @@ void xe_hw_engine_group_put(struct xe_hw_engine_group *group)
> >  {
> >  	up_read(&group->mode_sem);
> >  }
> > +
> > +/**
> > + * xe_hw_engine_group_find_exec_mode() - Find the execution mode for this exec queue
> > + * @q: The exec_queue
> > + */
> > +enum xe_hw_engine_group_execution_mode
> > +xe_hw_engine_group_find_exec_mode(struct xe_exec_queue *q)
> > +{
> > +	if (xe_vm_in_lr_mode(q->vm))
> > +		return EXEC_MODE_LR;
> > +	else
> > +		return EXEC_MODE_DMA_FENCE;
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> > index fce0adf6a7c4..6dfebb18cbb7 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> > @@ -76,5 +76,7 @@ int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group *group, struct x
> >  int xe_hw_engine_group_get_mode(struct xe_hw_engine_group *group,
> >  				enum xe_hw_engine_group_execution_mode mode);
> >  void xe_hw_engine_group_put(struct xe_hw_engine_group *group);
> > +enum xe_hw_engine_group_execution_mode
> > +xe_hw_engine_group_find_exec_mode(struct xe_exec_queue *q);
> 
> Same comment as previous patch, consider a standalone file for
> xe_hw_engine_group.
> 
> Matt
> 
> >  
> >  #endif
> > -- 
> > 2.43.0
> >