[Intel-xe] [PATCH v15 05/10] drm/xe/mmio: grab mem_access in xe_mmio_ioctl

[Rodrigo Vivi]

On Fri, Jul 28, 2023 at 02:05:24PM -0700, Matt Roper wrote:
> On Wed, Jul 19, 2023 at 09:38:07AM +0100, Matthew Auld wrote:
> > Any kind of device memory access should first ensure the device is not
> > suspended, mmio included.
> 
> I know things already went off track quite a while back, but it sounds
> like this is taking us even farther down the wrong path.
> xe_device_mem_access_get was intended as a place to hook in things like
> the ugly workarounds that we used to have on early PVC steppings where
> we need special care when the CPU might access VRAM.

Not actually. mem_access was created with that PVC case in mind, but that
was not the only case.

When dealing with that case on PVC, we couldn't easily answer the question
of how to ensure that we take the action A on *any* kind of memory access?

Then the request that came from Stuart was to design the driver from the
beginning where this type of question could be easily answered. And that
we could trap any kind of memory access.

Runtime PM is another place where we need to track any kind of memory
access and ensure that we are resumed before any memory access happen.

> Although we need
> to be out of D3 to access VRAM, we shouldn't be using
> xe_device_mem_access for general D3 management that's unrelated to VRAM
> access; we should be using actual runtime PM for that (and then
> xe_device_mem_access_get should either have an assertion to ensure that
> runtime PM was already grabbed at a higher level, or it should grab its
> own wakeref).

Well, it is true. We can do it like i915 and spread the with_runtime_pm
wrapper everywhere along with the mem_access and force_wakes...

> 
> As far as I know, we don't actually have anything that needs
> xe_device_mem_access right now (since we're not supporting old
> pre-production workarounds for PVC).

I was the one that removed that workaround and decided to keep RC6
entirely disabled on PVC, but recently after discussing the possibility
of keeping PVC ready on Xe to remove the force_probe protection I entirely
changed my mind and I'm planing to restore the workaround and to enable
the RC6 on PVC.

But besides that, if we end up removing it, then later we cannot answer
the original question like we couldn't answer on i915 for PVC.

> In theory we should be able to
> just rip it out of the driver completely with no impact as long we're
> handling device-level runtime PM and GT-level forcewake properly
> everywhere.

We do have a big impact in the runtime if we entirely rip that out.
One of the most difficult part of the memory management is that any
eviction/restoration coming from inside the runtime_pm cannot grab
the runtime_pm reference. This work is one of  the main reasons why
d3cold is not supported in i915.

The mem_access infra is allowing this in a very easy and neat way,
and taking care of all the locking, thanks to the awesome work that
Matt Auld has done here so far.

> 
> If we're too far down this path to fix things now and want to just
> repurpose xe_device_mem_access as an alternate reference counting layer
> over top of runtime PM, then we should probably rename it to something
> that more accurately reflects what we're using it for now.

Yeap, okay, this is one of the things I am considrering to be honest.
I have a todo item on my plate that is to spin of the mem_access from
xe_device, either to a specific component or embedded into the runtime_pm.
And then add documentation around it.

One thing I like about the 'mem_access' naming is that the purpose is
more obvious. If we are cooking a patch that we know it is working with
some sort of memory access we might take a look to the mem_access
component/calls.

In i915 there were many many merged regression on many cases because we
were always forgetting that we needed to run some memory access
'with_runtime_pm'.

>  And then we
> can re-add an actual memory access layer independently if/when it
> becomes necessary to deal with future hardware ugliness.

As long as we are able to easily answer that original question easily
I don't mind the name, or location of this infrastructure. But we
definitely need it.

And we might just go with PVC now anyway.

Thanks for the comments here and let me know if you have more
thoughts with this perspective in mind so I can adjust whenever
I get there to change the mem_access layer.

Rodrigo.

> 
> 
> Matt
> 
> > 
> > Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Reviewed-by: Matthew Brost <matthew.brost at intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_mmio.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> > index 448b874c7a3c..8d0f07261bfd 100644
> > --- a/drivers/gpu/drm/xe/xe_mmio.c
> > +++ b/drivers/gpu/drm/xe/xe_mmio.c
> > @@ -483,6 +483,7 @@ int xe_mmio_ioctl(struct drm_device *dev, void *data,
> >  	 */
> >  	reg = XE_REG(args->addr);
> >  
> > +	xe_device_mem_access_get(xe);
> >  	xe_force_wake_get(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> >  
> >  	if (args->flags & DRM_XE_MMIO_WRITE) {
> > @@ -526,6 +527,7 @@ int xe_mmio_ioctl(struct drm_device *dev, void *data,
> >  
> >  exit:
> >  	xe_force_wake_put(gt_to_fw(gt), XE_FORCEWAKE_ALL);
> > +	xe_device_mem_access_put(xe);
> >  
> >  	return ret;
> >  }
> > -- 
> > 2.41.0
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation