Re: [PATCH 4/7] drm/xe: Relax runtime pm protection around VM

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

Hi, Rodrigo.

On Fri, 2024-05-03 at 15:13 -0400, Rodrigo Vivi wrote:
> In the regular use case scenario, user space will create a
> VM, and keep it alive for the entire duration of its workload.
> 
> For the regular desktop cases, it means that the VM
> is alive even on idle scenarios where display goes off. This
> is unacceptable since this would entirely block runtime PM
> indefinitely, blocking deeper Package-C state. This would be
> a waste drainage of power.
> 
> So, let's limit the protection only for the long running workloads,
> which memory might be mapped and accessed during this entire
> workload.
> 
> This indeed opens up a risk of use case without display, and
> without long-running workload, where memory might be mapped
> and accessed with direct read and write operations without
> any gpu execution involved. Because of this, we are also
> adding here, the extra protection for the special vm_op access
> callback.

A couple of ignorant questions:

Why aren't the runtime_pm get / put in xe_sched_job_create() /
destroy() sufficient also for LR vms? If not, could the vm deactivation
/ reactivation be used for this (see xe_vm_reactivate_rebind)

> 
> In the ideal case of the mmapped scenario of vm_ops, we would
> also get references in the 'open' and 'mmap' callbacks, and
> put it back on the 'close' callback, for a balanced case.
> However, this would also block the regular desktop case, so
> we are not doing this.

I'm not completely following here. We have xe_runtime_pm_get() in the
fault handler + some form of delayed xe_runtime_pm_put(). Does this say
we ideally should replace that with open + mmap / close? 

Thanks,
Thomas

> 
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Francois Dugast <francois.dugast@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_bo.c | 17 ++++++++++++++++-
>  drivers/gpu/drm/xe/xe_vm.c |  6 +++---
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 52a16cb4e736..48eca9f2651a 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -1157,11 +1157,26 @@ static vm_fault_t xe_gem_fault(struct
> vm_fault *vmf)
>  	return ret;
>  }
>  
> +static int xe_vm_access(struct vm_area_struct *vma, unsigned long
> addr,
> +			void *buf, int len, int write)
> +{
> +	struct ttm_buffer_object *tbo = vma->vm_private_data;
> +	struct drm_device *ddev = tbo->base.dev;
> +	struct xe_device *xe = to_xe_device(ddev);
> +	int ret;
> +
> +	xe_pm_runtime_get(xe);
> +	ret = ttm_bo_vm_access(vma, addr, buf, len, write);
> +	xe_pm_runtime_put(xe);
> +
> +	return ret;
> +}
> +
>  static const struct vm_operations_struct xe_gem_vm_ops = {
>  	.fault = xe_gem_fault,
>  	.open = ttm_bo_vm_open,
>  	.close = ttm_bo_vm_close,
> -	.access = ttm_bo_vm_access
> +	.access = xe_vm_access
>  };
>  
>  static const struct drm_gem_object_funcs xe_gem_object_funcs = {
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index dfd31b346021..aa298b768620 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1347,7 +1347,7 @@ struct xe_vm *xe_vm_create(struct xe_device
> *xe, u32 flags)
>  
>  	vm->pt_ops = &xelp_pt_ops;
>  
> -	if (!(flags & XE_VM_FLAG_MIGRATION))
> +	if (flags & XE_VM_FLAG_LR_MODE)
>  		xe_pm_runtime_get_noresume(xe);
>  
>  	vm_resv_obj = drm_gpuvm_resv_object_alloc(&xe->drm);
> @@ -1457,7 +1457,7 @@ struct xe_vm *xe_vm_create(struct xe_device
> *xe, u32 flags)
>  	for_each_tile(tile, xe, id)
>  		xe_range_fence_tree_fini(&vm->rftree[id]);
>  	kfree(vm);
> -	if (!(flags & XE_VM_FLAG_MIGRATION))
> +	if (flags & XE_VM_FLAG_LR_MODE)
>  		xe_pm_runtime_put(xe);
>  	return ERR_PTR(err);
>  }
> @@ -1592,7 +1592,7 @@ static void vm_destroy_work_func(struct
> work_struct *w)
>  
>  	mutex_destroy(&vm->snap_mutex);
>  
> -	if (!(vm->flags & XE_VM_FLAG_MIGRATION))
> +	if (vm->flags & XE_VM_FLAG_LR_MODE)
>  		xe_pm_runtime_put(xe);
>  
>  	for_each_tile(tile, xe, id)