[PATCH v1] drm/xe/pm: Wait for lmem ready in resume

[Rodrigo Vivi]

On Thu, May 08, 2025 at 10:47:59AM -0500, Lucas De Marchi wrote:
> On Thu, May 08, 2025 at 11:28:29AM +0530, Poosa, Karthik wrote:
> > 
> > On 22-04-2025 21:30, Lucas De Marchi wrote:
> > > On Thu, Apr 10, 2025 at 04:30:57PM +0530, Karthik Poosa wrote:
> > > > Add wait for LMEM ready during system and runtime resume.
> > > > This wait is there in probe and is missing during resume.
> > > > 
> > > > Signed-off-by: Karthik Poosa <karthik.poosa at intel.com>
> > > > ---
> > > > drivers/gpu/drm/xe/xe_device.c | 2 +-
> > > > drivers/gpu/drm/xe/xe_device.h | 1 +
> > > > drivers/gpu/drm/xe/xe_pm.c     | 8 ++++++++
> > > > 3 files changed, 10 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_device.c
> > > > b/drivers/gpu/drm/xe/xe_device.c
> > > > index 75e753e0a682..4c0d9eb51d1f 100644
> > > > --- a/drivers/gpu/drm/xe/xe_device.c
> > > > +++ b/drivers/gpu/drm/xe/xe_device.c
> > > > @@ -630,7 +630,7 @@ static bool verify_lmem_ready(struct xe_device *xe)
> > > >     return !!val;
> > > > }
> > > > 
> > > > -static int wait_for_lmem_ready(struct xe_device *xe)
> > > > +int wait_for_lmem_ready(struct xe_device *xe)
> > > > {
> > > >     unsigned long timeout, start;
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_device.h
> > > > b/drivers/gpu/drm/xe/xe_device.h
> > > > index 0bc3bc8e6803..60bc92f9ab22 100644
> > > > --- a/drivers/gpu/drm/xe/xe_device.h
> > > > +++ b/drivers/gpu/drm/xe/xe_device.h
> > > > @@ -47,6 +47,7 @@ int xe_device_probe_early(struct xe_device *xe);
> > > > int xe_device_probe(struct xe_device *xe);
> > > > void xe_device_remove(struct xe_device *xe);
> > > > void xe_device_shutdown(struct xe_device *xe);
> > > > +int wait_for_lmem_ready(struct xe_device *xe);
> > > 
> > > if you are exporting the function, please follow the naming:
> > > xe_device_*.
> > > 
> > > However, I'm not sure this is really needed, particularly on pm runtime
> > > resume. At the very least it needs better explanation in the commit
> > > message. Do we have any known failures this is supposedly to fix?
> > > 
> > > Lucas De Marchi
> > > 
> > While there are no current issues without this check, it's a prudent
> > measure to prevent any future problems.
> 
> no, not without proper justification. I see no reason to wait for it in
> pm runtime resume. We shouldn't sprinkle sleeps in the driver to be
> prudent.

We need this on D3Cold -> D0. If power went off, during the wake-up, memory
links needs to be retrained and this is an indication that everything went
well on memory bring up and that we are able to access it from the driver.

That said, I believe the code should be changed here to only have this if
d3cold was allowed to start with, and not in every runtime resume.

> 
> Lucas De Marchi
> 
> > > > 
> > > > void xe_device_wmb(struct xe_device *xe);
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> > > > index 4e112fbacada..2e59670660c1 100644
> > > > --- a/drivers/gpu/drm/xe/xe_pm.c
> > > > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > > > @@ -182,6 +182,10 @@ int xe_pm_resume(struct xe_device *xe)
> > > >     if (err)
> > > >         return err;
> > > > 
> > > > +    err = wait_for_lmem_ready(xe);
> > > > +    if (err)
> > > > +        goto err;
> > > > +
> > > >     xe_display_pm_resume_early(xe);
> > > > 
> > > >     /*
> > > > @@ -478,6 +482,10 @@ int xe_pm_runtime_resume(struct xe_device *xe)
> > > >         if (err)
> > > >             goto out;
> > > > 
> > > > +        err = wait_for_lmem_ready(xe);
> > > > +        if (err)
> > > > +            goto out;
> > > > +
> > > >         xe_display_pm_resume_early(xe);
> > > > 
> > > >         /*
> > > > -- 
> > > > 2.25.1
> > > >