[Intel-xe] [PATCH] drm/xe/engine: add missing rpm for bind engines

[Matthew Auld]

On 25/07/2023 15:07, Rodrigo Vivi wrote:
> On Tue, Jul 25, 2023 at 12:01:17PM +0100, Matthew Auld wrote:
>> Bind engines need to use the migration vm, however we don't have any rpm
>> for such a vm, otherwise the kernel would prevent rpm suspend-resume.
>> There are two issues here, first is the actual engine create which needs
>> to touch the lrc, but since that is in VRAM we trigger loads of missing
>> mem_access asserts.
> 
> with this in mind, should we really create the new ENGINE_FLAG_HOLD_RPM,
> or should we simply use the existent ENGINE_FLAG_VM?

Sure, I can do that instead.

> 
>> The second issue is when destroying the actual
>> engine, which requires GuC CT to deregister the context.
>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/499
> 
> I wonder if we should get the mem_acceess from inside the ct path
> to be sure?

Do you mean calling xe_device_mem_access_get() in ct_send()? I think 
that will give various locking issues since any locks the caller is 
holding you then can't ever grab in the suspend/resume callbacks without 
deadlocking. And the callers of ct_send() are all over the place, and 
some are quite deep down. Also some callers expect some future CT 
response so holding in ct_send() is only one side, and would need to be 
kept held over to the wait side.

> 
>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_engine.c       | 20 ++++++++++++++++++++
>>   drivers/gpu/drm/xe/xe_engine_types.h |  1 +
>>   2 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_engine.c b/drivers/gpu/drm/xe/xe_engine.c
>> index 59e0a9e085ba..dba71f53e53e 100644
>> --- a/drivers/gpu/drm/xe/xe_engine.c
>> +++ b/drivers/gpu/drm/xe/xe_engine.c
>> @@ -76,6 +76,17 @@ static struct xe_engine *__xe_engine_create(struct xe_device *xe,
>>   	if (err)
>>   		goto err_lrc;
>>   
>> +	/*
>> +	 * Normally the user vm holds an rpm ref to keep the device awake, and
>> +	 * the context holds a ref for the vm, however for some engines we use
>> +	 * the kernels migrate vm underneath which offers no such rpm ref. Make
>> +	 * sure we keep a ref here, so we can perform GuC CT actions when
>> +	 * needed. Caller is expected to have already grabbed the rpm ref
>> +	 * outside any sensitive locks.
>> +	 */
>> +	if (e->flags & ENGINE_FLAG_HOLD_RPM)
>> +		drm_WARN_ON(&xe->drm, !xe_device_mem_access_get_if_ongoing(xe));
>> +
>>   	return e;
>>   
>>   err_lrc:
>> @@ -152,6 +163,8 @@ void xe_engine_fini(struct xe_engine *e)
>>   		xe_lrc_finish(e->lrc + i);
>>   	if (e->vm)
>>   		xe_vm_put(e->vm);
>> +	if (e->flags & ENGINE_FLAG_HOLD_RPM)
>> +		xe_device_mem_access_put(gt_to_xe(e->gt));
>>   
>>   	kfree(e);
>>   }
>> @@ -560,14 +573,21 @@ int xe_engine_create_ioctl(struct drm_device *dev, void *data,
>>   			if (XE_IOCTL_DBG(xe, !hwe))
>>   				return -EINVAL;
>>   
>> +			/* The migration vm doesn't hold rpm ref */
>> +			xe_device_mem_access_get(xe);
>> +
>>   			migrate_vm = xe_migrate_get_vm(gt_to_tile(gt)->migrate);
>>   			new = xe_engine_create(xe, migrate_vm, logical_mask,
>>   					       args->width, hwe,
>> +					       ENGINE_FLAG_HOLD_RPM |
>>   					       ENGINE_FLAG_PERSISTENT |
>>   					       ENGINE_FLAG_VM |
>>   					       (id ?
>>   					       ENGINE_FLAG_BIND_ENGINE_CHILD :
>>   					       0));
>> +
>> +			xe_device_mem_access_put(xe); /* now held by engine */
>> +
>>   			xe_vm_put(migrate_vm);
>>   			if (IS_ERR(new)) {
>>   				err = PTR_ERR(new);
>> diff --git a/drivers/gpu/drm/xe/xe_engine_types.h b/drivers/gpu/drm/xe/xe_engine_types.h
>> index 36bfaeec23f4..a3867e4db0bb 100644
>> --- a/drivers/gpu/drm/xe/xe_engine_types.h
>> +++ b/drivers/gpu/drm/xe/xe_engine_types.h
>> @@ -59,6 +59,7 @@ struct xe_engine {
>>   #define ENGINE_FLAG_VM			BIT(4)
>>   #define ENGINE_FLAG_BIND_ENGINE_CHILD	BIT(5)
>>   #define ENGINE_FLAG_WA			BIT(6)
>> +#define ENGINE_FLAG_HOLD_RPM		BIT(7)
>>   
>>   	/**
>>   	 * @flags: flags for this engine, should statically setup aside from ban
>> -- 
>> 2.41.0
>>