[Intel-xe] [PATCH 1/2] drm/xe: Deprecate XE_EXEC_QUEUE_SET_PROPERTY_COMPUTE_MODE implementation

[Niranjana Vishwanathapura]

On Wed, Sep 13, 2023 at 10:07:24PM +0000, Matthew Brost wrote:
>On Wed, Sep 13, 2023 at 02:34:04PM -0700, Niranjana Vishwanathapura wrote:
>> On Tue, Sep 12, 2023 at 04:51:58PM -0700, Matthew Brost wrote:
>> > We are going to remove XE_EXEC_QUEUE_SET_PROPERTY_COMPUTE_MODE from the
>> > uAPI, deprecate the implementation first by making
>> > XE_EXEC_QUEUE_SET_PROPERTY_COMPUTE_MODE a NOP.
>> >
>>
>> So, only make it creation time parameter, right?
>> May be mention that in the commit text here?
>>
>
>What? This was a creation time parameter that had to had match the VM
>setting, now it is a NOP.

I meant setting it based on what mode VM is in.
Basically, I think the commit text should mention why this change
is being done.

>
>> > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
>> > ---
>> > drivers/gpu/drm/xe/xe_exec_queue.c       | 57 ++++++------------------
>> > drivers/gpu/drm/xe/xe_exec_queue_types.h |  6 +--
>> > 2 files changed, 16 insertions(+), 47 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
>> > index 6725157d8c1d..ac392bcf7bc8 100644
>> > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
>> > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
>> > @@ -323,39 +323,6 @@ static int exec_queue_set_preemption_timeout(struct xe_device *xe,
>> > static int exec_queue_set_compute_mode(struct xe_device *xe, struct xe_exec_queue *q,
>> > 				       u64 value, bool create)
>> > {
>> > -	if (XE_IOCTL_DBG(xe, !create))
>> > -		return -EINVAL;
>> > -
>> > -	if (XE_IOCTL_DBG(xe, q->flags & EXEC_QUEUE_FLAG_COMPUTE_MODE))
>> > -		return -EINVAL;
>> > -
>> > -	if (XE_IOCTL_DBG(xe, q->flags & EXEC_QUEUE_FLAG_VM))
>> > -		return -EINVAL;
>> > -
>> > -	if (value) {
>> > -		struct xe_vm *vm = q->vm;
>> > -		int err;
>> > -
>> > -		if (XE_IOCTL_DBG(xe, xe_vm_in_fault_mode(vm)))
>> > -			return -EOPNOTSUPP;
>> > -
>> > -		if (XE_IOCTL_DBG(xe, !xe_vm_in_compute_mode(vm)))
>> > -			return -EOPNOTSUPP;
>> > -
>> > -		if (XE_IOCTL_DBG(xe, q->width != 1))
>> > -			return -EINVAL;
>> > -
>> > -		q->compute.context = dma_fence_context_alloc(1);
>> > -		spin_lock_init(&q->compute.lock);
>> > -
>> > -		err = xe_vm_add_compute_exec_queue(vm, q);
>> > -		if (XE_IOCTL_DBG(xe, err))
>> > -			return err;
>> > -
>> > -		q->flags |= EXEC_QUEUE_FLAG_COMPUTE_MODE;
>> > -		q->flags &= ~EXEC_QUEUE_FLAG_PERSISTENT;
>> > -	}
>> > -
>> > 	return 0;
>> > }
>> >
>> > @@ -365,7 +332,7 @@ static int exec_queue_set_persistence(struct xe_device *xe, struct xe_exec_queue
>> > 	if (XE_IOCTL_DBG(xe, !create))
>> > 		return -EINVAL;
>> >
>> > -	if (XE_IOCTL_DBG(xe, q->flags & EXEC_QUEUE_FLAG_COMPUTE_MODE))
>> > +	if (XE_IOCTL_DBG(xe, xe_vm_in_compute_mode(q->vm)))
>> > 		return -EINVAL;
>> >
>> > 	if (value)
>> > @@ -742,18 +709,21 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
>> > 		xe_vm_put(vm);
>> > 		if (IS_ERR(q))
>> > 			return PTR_ERR(q);
>> > +
>> > +		if (xe_vm_in_compute_mode(vm)) {
>> > +			q->compute.context = dma_fence_context_alloc(1);
>> > +			spin_lock_init(&q->compute.lock);
>> > +
>> > +			err = xe_vm_add_compute_exec_queue(vm, q);
>> > +			if (XE_IOCTL_DBG(xe, err))
>> > +				goto put_exec_queue;
>>
>> Don't we need the below checks here?
>>
>> if (XE_IOCTL_DBG(xe, xe_vm_in_fault_mode(vm)))
>>        return -EOPNOTSUPP;
>>
>
>No. If xe_vm_in_compute_mode is true, xe_vm_in_fault_mode must be false.
>

Yah, makes sense.

>> if (XE_IOCTL_DBG(xe, q->width != 1))
>>        return -EINVAL;
>>
>
>This check was stale, parallel engines should in compute mode now.
>

Looks like we have an IGT gap then.
Should we keep it until we fill validation gap?

>> > +		}
>> > 	}
>> >
>> > 	if (args->extensions) {
>> > 		err = exec_queue_user_extensions(xe, q, args->extensions, 0, true);
>> > 		if (XE_IOCTL_DBG(xe, err))
>> > -			goto put_exec_queue;
>> > -	}
>> > -
>> > -	if (XE_IOCTL_DBG(xe, q->vm && xe_vm_in_compute_mode(q->vm) !=
>> > -			 !!(q->flags & EXEC_QUEUE_FLAG_COMPUTE_MODE))) {
>> > -		err = -EOPNOTSUPP;
>> > -		goto put_exec_queue;
>> > +			goto kill_exec_queue;
>> > 	}
>> >
>> > 	q->persistent.xef = xef;
>> > @@ -762,14 +732,15 @@ int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
>> > 	err = xa_alloc(&xef->exec_queue.xa, &id, q, xa_limit_32b, GFP_KERNEL);
>> > 	mutex_unlock(&xef->exec_queue.lock);
>> > 	if (err)
>> > -		goto put_exec_queue;
>> > +		goto kill_exec_queue;
>> >
>> > 	args->exec_queue_id = id;
>> >
>> > 	return 0;
>> >
>> > -put_exec_queue:
>> > +kill_exec_queue:
>> > 	xe_exec_queue_kill(q);
>> > +put_exec_queue:
>> > 	xe_exec_queue_put(q);
>>
>> Not directly related to this patch, but now that I am looking at it, probably
>> the function exec_queue_kill_compute() should be renamed to
>> xe_vm_remove_compute_exec_queue() and move it to xe_vm.c?
>>
>
>Yea exec_queue_kill_compute should be cleaned up. Can do in a follow up.
>

Ok.

Niranjana

>Matt
>
>> Niranjana
>>
>> > 	return err;
>> > }
>> > diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
>> > index 347d28442701..f73b9e80b25a 100644
>> > --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
>> > +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
>> > @@ -60,12 +60,10 @@ struct xe_exec_queue {
>> > #define EXEC_QUEUE_FLAG_PERMANENT		BIT(2)
>> > /* queue keeps running pending jobs after destroy ioctl */
>> > #define EXEC_QUEUE_FLAG_PERSISTENT		BIT(3)
>> > -/* queue for use with compute VMs */
>> > -#define EXEC_QUEUE_FLAG_COMPUTE_MODE		BIT(4)
>> > /* for VM jobs. Caller needs to hold rpm ref when creating queue with this flag */
>> > -#define EXEC_QUEUE_FLAG_VM			BIT(5)
>> > +#define EXEC_QUEUE_FLAG_VM			BIT(4)
>> > /* child of VM queue for multi-tile VM jobs */
>> > -#define EXEC_QUEUE_FLAG_BIND_ENGINE_CHILD	BIT(6)
>> > +#define EXEC_QUEUE_FLAG_BIND_ENGINE_CHILD	BIT(5)
>> >
>> > 	/**
>> > 	 * @flags: flags for this exec queue, should statically setup aside from ban
>> > --
>> > 2.34.1
>> >