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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, May 09, 2024 at 07:48:09AM -0400, Gupta, Anshuman wrote:
> 
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Saturday, May 4, 2024 12:43 AM
> > To: intel-xe@lists.freedesktop.org
> > Cc: De Marchi, Lucas <lucas.demarchi@intel.com>; Brost, Matthew
> > <matthew.brost@intel.com>; Dugast, Francois <francois.dugast@intel.com>;
> > thomas.hellstrom@linux.intel.com; Auld, Matthew
> > <matthew.auld@intel.com>; Gupta, Anshuman
> > <anshuman.gupta@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Subject: [PATCH 4/7] drm/xe: Relax runtime pm protection around VM
> > 
> > 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.
> AFAIU for this particular case we are already having protection, as we are invalidating the pte upon entry to runtime suspend.
> So a pagefault should wake the device. Therefore we don't need this extra precaution.

Indee and removed from the new version just sent.

I was just not confident because my tests were failing...
but now igt is fixed as well:
https://lore.kernel.org/all/20240509191636.504200-3-rodrigo.vivi@intel.com/

> Thanks,
> Anshuman
> > 
> > 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.
> > 
> > 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)
> > --
> > 2.44.0
>