[PATCH v3] drm/xe: Handle unreliable MMIO reads during forcewake

[Lin, Shuicheng]

On Thu, Oct 17, 2024 10:51 AM Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com> wrote:
> On 17-10-2024 22:00, Shuicheng Lin wrote:
> > In some cases, when the driver attempts to read an MMIO register, the
> > hardware may return 0xFFFFFFFF. The current force wake path code
> > treats this as a valid response, as it only checks the BIT.
> > However, 0xFFFFFFFF should be considered an invalid value, indicating
> > a potential issue. To address this, we should add a log entry to
> > highlight this condition and return failure.
> > The force wake failure log level is changed from notice to err to
> > match the failure return value.
> >
> > v2 (Matt Brost):
> >    - set ret value (-EIO) to kick the error to upper layers
> > v3 (Rodrigo):
> >    - add commit message for the log level promotion from notice to err
> 
> Feel free to retain RB on this version.
> 
> As a general practice, creating a new version solely for commit message or
> changelog updates is avoided, as this triggers a full CI run. Making changes
> before pushing and running checkpatch should be sufficient.
> 
> BR
> Himal

Himal, Thanks very much for your experience sharing. It is really good for me as I am still new with the process.
Yes. Maintainer could help update the commit message during push stage, but it will cost extra effort for the maintainer.
Unless maintainer explicitly offer it, I still prefer to correct the commit message by myself.
For the RB, I forgot to update it when I update this patch. Let me add it and send a new one. :(
I will try my best to minimize the patch version number, and it is still a long way to reach it.

Shuicheng
> 
> >
> > Suggested-by: Alex Zuo <alex.zuo at intel.com>
> > Signed-off-by: Shuicheng Lin <shuicheng.lin at intel.com>
> > Cc: Matthew Brost <matthew.brost at intel.com>
> > Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> > Cc: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_force_wake.c | 12 +++++++++---
> >   1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_force_wake.c
> > b/drivers/gpu/drm/xe/xe_force_wake.c
> > index a64c14757c84..08621717b14b 100644
> > --- a/drivers/gpu/drm/xe/xe_force_wake.c
> > +++ b/drivers/gpu/drm/xe/xe_force_wake.c
> > @@ -115,9 +115,15 @@ static int __domain_wait(struct xe_gt *gt, struct
> xe_force_wake_domain *domain,
> >   			     XE_FORCE_WAKE_ACK_TIMEOUT_MS *
> USEC_PER_MSEC,
> >   			     &value, true);
> >   	if (ret)
> > -		xe_gt_notice(gt, "Force wake domain %d failed to ack %s
> (%pe) reg[%#x] = %#x\n",
> > -			     domain->id, str_wake_sleep(wake), ERR_PTR(ret),
> > -			     domain->reg_ack.addr, value);
> > +		xe_gt_err(gt, "Force wake domain %d failed to ack %s (%pe)
> reg[%#x] = %#x\n",
> > +			  domain->id, str_wake_sleep(wake), ERR_PTR(ret),
> > +			  domain->reg_ack.addr, value);
> > +	if (value == ~0) {
> > +		xe_gt_err(gt,
> > +			  "Force wake domain %d: %s. MMIO unreliable
> (forcewake register returns 0xFFFFFFFF)!\n",
> > +			  domain->id, str_wake_sleep(wake));
> > +		ret = -EIO;
> > +	}
> >
> >   	return ret;
> >   }