[RFC v1 3/9] drm/xe/hw_engine_group: Register hw engine group's exec queues
Francois Dugast
francois.dugast at intel.com
Mon Jul 22 08:17:06 UTC 2024
On Wed, Jul 17, 2024 at 08:09:28PM +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 at 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
>
> > + 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.
A hw engine group's list of exec queue is initialized at the time the group
is setup. This is done in the first patch in the series:
"drm/xe/hw_engine_group: Introduce xe_hw_engine_group"
Francois
>
> 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
> >
More information about the Intel-xe
mailing list