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

[Raag Jadav <raag.jadav@intel.com>]

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.

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