Re: [PATCH v1] drm/xe/pm: Handle GT resume failure

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

On Fri, Dec 19, 2025 at 07:00:02PM +0100, Raag Jadav wrote:
> On Fri, Dec 19, 2025 at 11:08:25AM -0500, Rodrigo Vivi wrote:
> > On Fri, Dec 19, 2025 at 06:04:42AM +0100, Raag Jadav wrote:
> > > On Thu, Dec 18, 2025 at 10:46:10AM -0800, Matt Roper wrote:
> > > > On Thu, Dec 18, 2025 at 12:12:59PM +0100, Raag Jadav wrote:
> > > > > On Wed, Dec 17, 2025 at 09:38:34AM -0800, Matt Roper wrote:
> > > > > > On Wed, Dec 17, 2025 at 12:25:32PM -0500, Rodrigo Vivi wrote:
> > > > > > > On Wed, Dec 17, 2025 at 06:49:09PM +0530, Raag Jadav wrote:
> > > > > > > > We've been historically ignoring GT resume failure. Since the function
> > > > > > > > can return error, handle it properly.
> > > > > > > 
> > > > > > > I probably had a reason for it, but since I didn't document and
> > > > > > > cannot remember it, let's go forward and make the clean flow.
> > > > > > > 
> > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > > > > 
> > > > > > > > 
> > > > > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/gpu/drm/xe/xe_pm.c | 14 ++++++++++----
> > > > > > > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
> > > > > > > > index 4390ba69610d..a8b50091d62e 100644
> > > > > > > > --- a/drivers/gpu/drm/xe/xe_pm.c
> > > > > > > > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > > > > > > > @@ -260,8 +260,11 @@ int xe_pm_resume(struct xe_device *xe)
> > > > > > > >  
> > > > > > > >  	xe_irq_resume(xe);
> > > > > > > >  
> > > > > > > > -	for_each_gt(gt, xe, id)
> > > > > > > > -		xe_gt_resume(gt);
> > > > > > > > +	for_each_gt(gt, xe, id) {
> > > > > > > > +		err = xe_gt_resume(gt);
> > > > > > > > +		if (err)
> > > > > > > > +			goto err;
> > > > > > 
> > > > > > When we propagate these errors upward, what's the end result / where
> > > > > > does it eventually get handled?  If the device is still [partially]
> > > > > > usable after an error, wouldn't it be better to not bail out of the loop
> > > > > > immediately, but rather at least try to resume the other GTs, the
> > > > > > display, etc. before returning the error at the end to indicate
> > > > > > something failed?  Then you might still have a partially functioning
> > > > > > device and have a better chance of at least having your screen turn back
> > > > > > on to show the relevant error messages?
> > > > > 
> > > > > I had a similar question when I came across xe_device_probe(), but as
> > > > > Lucas mentioned[1] that the expectation here is pretty much "all or
> > > > > nothing". Again, not my call but I think we should be consistent.
> > > > 
> > > > I think device probe is a bit different --- if you can't bring up the
> > > > hardware successfully at the very beginning then something is pretty
> > > > wrong and it's best to just not enable and start using the device at
> > > > all.  But the resume paths are different --- the device is already bound
> > > > and in use, and was working properly previously.  If we intentionally
> > > > don't even try to power up other parts of the device that might still
> > > > work (display, other GTs, etc.), then we're making the situation worse
> > > > and that could be the difference between the user having a functional UI
> > > > that gives them a chance to save their work and shutdown/recover
> > > > gracefully vs having to just power off the machine because their monitor
> > > > is black and they don't have any idea what's going on.  Powering up
> > > > other units like display also makes it more likely that we can get
> > > > useful debugging information out of the machine to figure out what
> > > > actually went wrong.
> > > 
> > > Fair, but this also means the existing error handing in resume path is
> > > redundant and should be removed.
> > 
> > Not necessarily. Otherwise resume itself wouldn't have the failure path.
> > I believe that Matt is suggesting is that we need to scrutinize them all
> > and handle with care without a one-rule-fits-all.
> > 
> > If there's a chance of getting display back even without some engines
> > for instance, perhaps we should try it.
> > 
> > Imagine that media gt failed to come back, but you still have everything
> > else. But user will try to open the lid on their laptops and will just
> > get a blank screen. We need to avoid this scenario and work for a more
> > reliable platform with more granular and contained error handling.
> 
> Or perhaps we can reconsider the ordering to make sure we have essential
> parts up and running before we move on to non-essentials?

indeed, but careful with chicken-egg cases...

> 
> I'm not a fan of hiding errors but upto you.

Nobody is in favor of hiding errors. Just that this patch, as is, might
impose a worse user experience. So, someone needs to take the ball here
and do some study and design the flow for a more reliable experience
and error handling.

> 
> Raag
> 
> > > > > [1] https://lore.kernel.org/intel-xe/lliho4ci6gi5spxxelttgqntbh7rxr4utg4dgfevlrdy54phrh@2k4mjuofaqye/
> > > > > 
> > > > > > > > +	}
> > > > > > > >  
> > > > > > > >  	xe_display_pm_resume(xe);
> > > > > > > >  
> > > > > > > > @@ -656,8 +659,11 @@ int xe_pm_runtime_resume(struct xe_device *xe)
> > > > > > > >  
> > > > > > > >  	xe_irq_resume(xe);
> > > > > > > >  
> > > > > > > > -	for_each_gt(gt, xe, id)
> > > > > > > > -		xe->d3cold.allowed ? xe_gt_resume(gt) : xe_gt_runtime_resume(gt);
> > > > > > > > +	for_each_gt(gt, xe, id) {
> > > > > > > > +		err = xe->d3cold.allowed ? xe_gt_resume(gt) : xe_gt_runtime_resume(gt);
> > > > > > > > +		if (err)
> > > > > > > > +			goto out;
> > > > > > > > +	}
> > > > > > > >  
> > > > > > > >  	xe_display_pm_runtime_resume(xe);
> > > > > > > >  
> > > > > > > > -- 
> > > > > > > > 2.43.0
> > > > > > > >