[PATCH] drm/xe: Drop EXEC_QUEUE_FLAG_BANNED

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Matthew Brost
Sent: Tuesday, June 4, 2024 11:47 AM
To: intel-xe at lists.freedesktop.org
Cc: Brost, Matthew <matthew.brost at intel.com>
Subject: [PATCH] drm/xe: Drop EXEC_QUEUE_FLAG_BANNED
> 
> Clean up laying violation of setting q->flags EXEC_QUEUE_FLAG_BANNED bit
> in GuC backend. Move banned to GuC owned bit and report banned status to
> upper layers via reset_status vfunc. This is a slight change in behavior
> as reset_status returns true if wedged or killed bits set too, but in
> all of these cases submission to queue is no longer allowed.
> 
> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_exec.c             |  2 +-
>  drivers/gpu/drm/xe/xe_exec_queue.c       |  2 +-
>  drivers/gpu/drm/xe/xe_exec_queue_types.h | 12 +++++-------
>  drivers/gpu/drm/xe/xe_guc_submit.c       | 10 ++++++----
>  4 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/xe_exec.c
> index 97eeb973e897..4cf6c6ab4866 100644
> --- a/drivers/gpu/drm/xe/xe_exec.c
> +++ b/drivers/gpu/drm/xe/xe_exec.c
> @@ -141,7 +141,7 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  			 q->width != args->num_batch_buffer))
>  		return -EINVAL;
>  
> -	if (XE_IOCTL_DBG(xe, q->flags & EXEC_QUEUE_FLAG_BANNED)) {
> +	if (XE_IOCTL_DBG(xe, q->ops->reset_status(q))) {
>  		err = -ECANCELED;
>  		goto err_exec_queue;
>  	}
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> index 27215075c799..cf45df0328da 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> @@ -677,7 +677,7 @@ int xe_exec_queue_get_property_ioctl(struct drm_device *dev, void *data,
>  
>  	switch (args->property) {
>  	case DRM_XE_EXEC_QUEUE_GET_PROPERTY_BAN:
> -		args->value = !!(q->flags & EXEC_QUEUE_FLAG_BANNED);
> +		args->value = q->ops->reset_status(q);

LGTM.

Maybe migrating over to using q->ops->reset_status could be done later, and
instead we could just check the EXEC_QUEUE_STATE_BANNED flag directly for
now, saving the change to reset_status for a separate patch.  That way, we'd
have more room to justify this change in the commit message separately from
the one made to the EXEC_QUEUE_FLAG_BANNED.  But it's not strictly
necessary, IMO.

Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt

>  		ret = 0;
>  		break;
>  	default:
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> index 18d8b2a60928..f0c5f82ce7e3 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
> +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> @@ -70,18 +70,16 @@ struct xe_exec_queue {
>  	 */
>  	struct dma_fence *last_fence;
>  
> -/* queue no longer allowed to submit */
> -#define EXEC_QUEUE_FLAG_BANNED			BIT(0)
>  /* queue used for kernel submission only */
> -#define EXEC_QUEUE_FLAG_KERNEL			BIT(1)
> +#define EXEC_QUEUE_FLAG_KERNEL			BIT(0)
>  /* kernel engine only destroyed at driver unload */
> -#define EXEC_QUEUE_FLAG_PERMANENT		BIT(2)
> +#define EXEC_QUEUE_FLAG_PERMANENT		BIT(1)
>  /* for VM jobs. Caller needs to hold rpm ref when creating queue with this flag */
> -#define EXEC_QUEUE_FLAG_VM			BIT(3)
> +#define EXEC_QUEUE_FLAG_VM			BIT(2)
>  /* child of VM queue for multi-tile VM jobs */
> -#define EXEC_QUEUE_FLAG_BIND_ENGINE_CHILD	BIT(4)
> +#define EXEC_QUEUE_FLAG_BIND_ENGINE_CHILD	BIT(3)
>  /* kernel exec_queue only, set priority to highest level */
> -#define EXEC_QUEUE_FLAG_HIGH_PRIORITY		BIT(5)
> +#define EXEC_QUEUE_FLAG_HIGH_PRIORITY		BIT(4)
>  
>  	/**
>  	 * @flags: flags for this exec queue, should statically setup aside from ban
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 47aab04cf34f..4464ba337d12 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -61,6 +61,7 @@ exec_queue_to_guc(struct xe_exec_queue *q)
>  #define EXEC_QUEUE_STATE_RESET			(1 << 6)
>  #define EXEC_QUEUE_STATE_KILLED			(1 << 7)
>  #define EXEC_QUEUE_STATE_WEDGED			(1 << 8)
> +#define EXEC_QUEUE_STATE_BANNED			(1 << 9)
>  
>  static bool exec_queue_registered(struct xe_exec_queue *q)
>  {
> @@ -134,12 +135,12 @@ static void set_exec_queue_destroyed(struct xe_exec_queue *q)
>  
>  static bool exec_queue_banned(struct xe_exec_queue *q)
>  {
> -	return (q->flags & EXEC_QUEUE_FLAG_BANNED);
> +	return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_BANNED;
>  }
>  
>  static void set_exec_queue_banned(struct xe_exec_queue *q)
>  {
> -	q->flags |= EXEC_QUEUE_FLAG_BANNED;
> +	atomic_or(EXEC_QUEUE_STATE_BANNED, &q->guc->state);
>  }
>  
>  static bool exec_queue_suspended(struct xe_exec_queue *q)
> @@ -189,8 +190,9 @@ static void set_exec_queue_wedged(struct xe_exec_queue *q)
>  
>  static bool exec_queue_killed_or_banned_or_wedged(struct xe_exec_queue *q)
>  {
> -	return exec_queue_banned(q) || (atomic_read(&q->guc->state) &
> -		(EXEC_QUEUE_STATE_WEDGED | EXEC_QUEUE_STATE_KILLED));
> +	return (atomic_read(&q->guc->state) &
> +		(EXEC_QUEUE_STATE_WEDGED | EXEC_QUEUE_STATE_KILLED |
> +		 EXEC_QUEUE_STATE_BANNED));
>  }
>  
>  #ifdef CONFIG_PROVE_LOCKING
> -- 
> 2.34.1
> 
>