[PATCH] drm/xe: Skip TLB invalidation time out log if ct is disabled

[Lin, Shuicheng]

Thanks all for the review. Will check it again after the 2 series are merged.

Best Regards
Shuicheng

> -----Original Message-----
> From: Brost, Matthew <matthew.brost at intel.com>
> Sent: Wednesday, February 21, 2024 2:35 AM
> To: Auld, Matthew <matthew.auld at intel.com>
> Cc: Lin, Shuicheng <shuicheng.lin at intel.com>; intel-xe at lists.freedesktop.org
> Subject: Re: [PATCH] drm/xe: Skip TLB invalidation time out log if ct is disabled
> 
> On Tue, Feb 20, 2024 at 06:07:00PM +0000, Matthew Auld wrote:
> > On 20/02/2024 15:05, Matthew Brost wrote:
> > > On Tue, Feb 20, 2024 at 02:13:56AM +0000, Shuicheng Lin wrote:
> > > > Suspend may cause the TLB invalidation time out as below log.
> > > > Skip the log print if ct is disabled to make log clean.
> > > > "
> > > > [  228.812266] xe_gt_tlb_invalidation_wait enter [  228.812311]
> > > > xe_gt_suspend enter [  228.812782] xe 0000:03:00.0: [drm] GT0:
> > > > suspended [  228.812786] xe_gt_suspend enter [  228.813508] xe
> > > > 0000:03:00.0: [drm] GT1: suspended … [  229.067007] xe
> > > > 0000:03:00.0: [drm] *ERROR* TILE0 [GTT] GT0: TLB invalidation
> > > > time'd out, seqno=321, recv=319 [  229.067099] xe 0000:03:00.0:
> > > > [drm] *ERROR* GT0: CT disabled "
> > > >
> > >
> > > This doesn't look right for a few reasons.
> > > - The timeout still can race suspend and then a resume
> > > - The xe_guc_ct_enabled check also supresses the -ETIME return
> > > - I think this message it actually valid
> > >
> > > What should probably be done is signal all pending TLB invalidations
> > > on suspend. I think we are doing a bit of rework in [1] in this area too.
> > > I'd say let's get [1] to land and if this is still an issue fixup
> > > the suspend path to signal all TLB invalidation waiters. Signaling
> > > all waiters on suspend shoud avoid having this message be printed.
> >
> > I think [1] will only help with rpm, also currently all callers of
> > xe_gt_tlb_invalidation_wait() will always have an rpm ref anyway, AFAICT.
> > There is the forced suspend path which is quite a different beast
> > though, so likely it is there where we need to be more solid?
> >
> 
> Yes it likely not the rpm path here but still think it worth getting [1] in and
> perhaps a version [2] in first though.
> 
> Matt
> 
> [1] https://patchwork.freedesktop.org/series/129217/
> [2] https://patchwork.freedesktop.org/series/122772/
> 
> > >
> > > Matt
> > >
> > > [1] https://patchwork.freedesktop.org/series/129217/
> > >
> > > > Signed-off-by: Shuicheng Lin <shuicheng.lin at intel.com>
> > > > ---
> > > >   drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c | 17 ++++++++++++-----
> > > >   1 file changed, 12 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
> > > > b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
> > > > index 7b3a54748b49..8aac12efea84 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
> > > > +++ b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
> > > > @@ -330,11 +330,18 @@ int xe_gt_tlb_invalidation_wait(struct xe_gt
> *gt, int seqno)
> > > >   	if (!ret) {
> > > >   		struct drm_printer p = xe_gt_err_printer(gt);
> > > > -		xe_tile_report_driver_error(gt_to_tile(gt),
> XE_TILE_DRV_ERR_GTT,
> > > > -					    "GT%u: TLB invalidation time'd out,
> seqno=%d, recv=%d",
> > > > -					    gt->info.id, seqno, gt-
> >tlb_invalidation.seqno_recv);
> > > > -		xe_guc_ct_print(&guc->ct, &p, true);
> > > > -		return -ETIME;
> > > > +		/*
> > > > +		 * guc ct may be disabled during the waiting period and lead to
> the timeout.
> > > > +		 * Such as power suspend just after this tlb invalidation wait.
> > > > +		 * Skip the error log print if ct is disabled.
> > > > +		 */
> > > > +		if (xe_guc_ct_enabled(&guc->ct)) {
> > > > +			xe_tile_report_driver_error(gt_to_tile(gt),
> XE_TILE_DRV_ERR_GTT,
> > > > +						    "GT%u: TLB invalidation
> time'd out, seqno=%d, recv=%d",
> > > > +						    gt->info.id, seqno, gt-
> >tlb_invalidation.seqno_recv);
> > > > +			xe_guc_ct_print(&guc->ct, &p, true);
> > > > +			return -ETIME;
> > > > +		}
> > > >   	}
> > > >   	return 0;
> > > > --
> > > > 2.25.1
> > > >