Re: [RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Wed, 2024-07-17 at 20:09 +0000, Matthew Brost wrote:
> On Wed, Jul 17, 2024 at 03:07:24PM +0200, Francois Dugast wrote:
> > Add helpers to safely add and delete the exec queues attached to a
> > hw
> > engine group, and make use them at the time of creation and
> > destruction
> > of the exec queues. Keeping track of them is required to control
> > the
> > execution mode of the hw engine group.
> > 
> 
> Missed a few more things...
> 
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_exec_queue.c |  7 +++++
> >  drivers/gpu/drm/xe/xe_hw_engine.c  | 45
> > ++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_hw_engine.h  |  4 +++
> >  3 files changed, 56 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c
> > b/drivers/gpu/drm/xe/xe_exec_queue.c
> > index 0ba37835849b..645423a63434 100644
> > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > @@ -192,6 +192,9 @@ void xe_exec_queue_destroy(struct kref *ref)
> >  			xe_exec_queue_put(eq);
> >  	}
> >  
> > +	if (q->vm)
> 
> I think this code path can be called GSC exec queues which don't have
> a
> q->hwe->hw_engine_group. So I think:
> 
> if (q->vm && q->hwe->hw_engine_group)
> 	xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group,
> q);
> 
> > +		xe_hw_engine_group_del_exec_queue(q->hwe-
> > >hw_engine_group, q);
> > +
> >  	q->ops->fini(q);
> >  }
> >  
> > @@ -640,6 +643,10 @@ int xe_exec_queue_create_ioctl(struct
> > drm_device *dev, void *data,
> >  			if (XE_IOCTL_DBG(xe, err))
> >  				goto put_exec_queue;
> >  		}
> > +
> > +		err = xe_hw_engine_group_add_exec_queue(q->hwe-
> > >hw_engine_group, q);
> 
> So this only called on non-VM bind exec queues. I think techincally
> this
> wrong as VM bind exec queues can use dma-fences plus the copy engine.
> I
> plan dropping the copy engine for these [1] and I don't think it
> worth
> fixing VM bind path with mode switching if we only are going to drop
> this requirement soon. Let me just respin [1] and hopefully we can
> prioritize though reviews so these two series land at roughly the
> same
> time. 
> 
> [1]
> https://patchwork.freedesktop.org/patch/582003/?series=125608&rev=5


Matt, Francois

I have expressed concerns a couple of times about ripping out the
capability of gpu binding, mainly for two reasons.

1) I think a pretty common use-case for a bo is
a) clearing/readback
b) binding

If we can have those executed by the same exec_queue then the binding's
dependencies are implicitly resolved and no latency incurred. If
executed by the cpu, we have to add the latency of signalling the
clearing fence and waking up the executing thread.

2) Considering using page-table bos out of non-mappable VRAM.

It's entirely possible these concerns were considered non-valid at some
point but in case they weren't, just a reminder.

How difficult would it be to perform the switching also on these VM
binds?

/Thomas

> 
> > +		if (err)
> > +			goto put_exec_queue;
> >  	}
> >  
> >  	mutex_lock(&xef->exec_queue.lock);
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c
> > b/drivers/gpu/drm/xe/xe_hw_engine.c
> > index f8df85d25617..4dcc885a55c8 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> > @@ -1178,3 +1178,48 @@ u64 xe_hw_engine_read_timestamp(struct
> > xe_hw_engine *hwe)
> >  {
> >  	return xe_mmio_read64_2x32(hwe->gt, RING_TIMESTAMP(hwe-
> > >mmio_base));
> >  }
> > +
> > +/**
> > + * xe_hw_engine_group_add_exec_queue() - Add an exec queue to a hw
> > engine group
> > + * @group: The hw engine group
> > + * @q: The exec_queue
> > + *
> > + * Return: 0 on success,
> > + *	    -EINTR if the lock could not be acquired
> > + */
> > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group
> > *group, struct xe_exec_queue *q)
> > +{
> > +	int err;
> > +
> 
> Also assert here this is not a VM bind queue (e.g. EXEC_QUEUE_FLAG_VM
> is
> clear).
> 
> > +	err = down_write_killable(&group->mode_sem);
> > +	if (err)
> > +		return err;
> > +
> > +	list_add(&q->hw_engine_group_link, &group-
> > >exec_queue_list);
> 
> I don't see where INIT_LIST_HEAD is called on group->exec_queue_list.
> I
> think that should unconditionally be done in __xe_exec_queue_alloc.
> 
> Matt
> 
> > +	up_write(&group->mode_sem);
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * xe_hw_engine_group_del_exec_queue() - Delete an exec queue from
> > a hw engine group
> > + * @group: The hw engine group
> > + * @q: The exec_queue
> > + *
> > + * Return: 0 on success,
> > + *	    -EINTR if the lock could not be acquired
> > + */
> > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group
> > *group, struct xe_exec_queue *q)
> > +{
> > +	int err;
> > +
> > +	err = down_write_killable(&group->mode_sem);
> > +	if (err)
> > +		return err;
> > +
> > +	if (!list_empty(&group->exec_queue_list))
> > +		list_del(&q->hw_engine_group_link);
> > +	up_write(&group->mode_sem);
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h
> > b/drivers/gpu/drm/xe/xe_hw_engine.h
> > index 7f2d27c0ba1a..ce59d83a75ad 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> > @@ -9,6 +9,7 @@
> >  #include "xe_hw_engine_types.h"
> >  
> >  struct drm_printer;
> > +struct xe_exec_queue;
> >  
> >  #ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> >  #define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> > @@ -70,4 +71,7 @@ static inline bool xe_hw_engine_is_valid(struct
> > xe_hw_engine *hwe)
> >  const char *xe_hw_engine_class_to_str(enum xe_engine_class class);
> >  u64 xe_hw_engine_read_timestamp(struct xe_hw_engine *hwe);
> >  
> > +int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group
> > *group, struct xe_exec_queue *q);
> > +int xe_hw_engine_group_del_exec_queue(struct xe_hw_engine_group
> > *group, struct xe_exec_queue *q);
> > +
> >  #endif
> > -- 
> > 2.43.0
> >