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

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

On Tue, Jul 25, 2023 at 04:11:41PM +0100, Matthew Auld wrote:
> 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.

What I'm not so sure is about the extra case that that would bring:

at xe_vm_create()
if (!(flags & XE_VM_FLAG_MIGRATION))

> 
> > 
> > > 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.

hmm indeed! And I think we had already discussed that in the past...
sorry for the noise

> 
> > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Signed-off-by: Matthew Auld <matthew.auld@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
> > >