Re: [PATCH] drm/xe/xe_gt_debugfs: Add synchronous gt reset debugfs

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Jun 06, 2024 at 12:35:42PM -0500, Lucas De Marchi wrote:
> On Thu, Jun 06, 2024 at 04:38:49PM GMT, Cavitt, Jonathan wrote:
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Thursday, June 6, 2024 9:26 AM
> > To: Cavitt, Jonathan <jonathan.cavitt@intel.com>
> > Cc: intel-xe@lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta@intel.com>; Brost, Matthew <matthew.brost@intel.com>; Harrison, John C <john.c.harrison@intel.com>; Summers, Stuart <stuart.summers@intel.com>
> > Subject: Re: [PATCH] drm/xe/xe_gt_debugfs: Add synchronous gt reset debugfs
> > > 
> > > On Thu, Jun 06, 2024 at 02:11:02PM +0000, Cavitt, Jonathan wrote:
> > > > -----Original Message-----
> > > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > Sent: Wednesday, June 5, 2024 3:10 PM
> > > > To: Cavitt, Jonathan <jonathan.cavitt@intel.com>
> > > > Cc: intel-xe@lists.freedesktop.org; Gupta, saurabhg <saurabhg.gupta@intel.com>; Brost, Matthew <matthew.brost@intel.com>; Harrison, John C <john.c.harrison@intel.com>; Summers, Stuart <stuart.summers@intel.com>
> > > > Subject: Re: [PATCH] drm/xe/xe_gt_debugfs: Add synchronous gt reset debugfs
> > > > >
> > > > > On Wed, Jun 05, 2024 at 08:08:28AM -0700, Jonathan Cavitt wrote:
> > > > > > We currently have debugfs support that allows the userspace to initiate
> > > > > > an asynchronous gt reset on command.  However, userspace may also wish
> > > > > > to wait for the completion of the gt reset before performing any
> > > > > > additional work.  To that end, add a version of the force_reset gt
> > > > > > debugfs function that operates synchronously.
> > > > > >
> > > > > > Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1068
> > > > >
> > > > > do we already have the igt changes that really closes this?
> > > >
> > > > The IGT change that closes this issue has not yet landed.  It relies on
> > > > using the new debugfs, so if it landed first, we'd be attempting to call
> > > > a nonexistent debugfs function.
> > > 
> > > Please provide the IGT and the kernel patch at the same time.
> > > you can copy patchwork or lore links in the one or the other cover letter
> > > to explain and justify each other.
> > 
> > Here's the related IGT change:
> > https://patchwork.freedesktop.org/series/134509/
> 
> great. Let's remove the Closes from this patch then.
> We can do that while applying, but...
> 

I think Rodrigo just pushed these...

> > > > > > @@ -116,6 +116,17 @@ static int force_reset(struct xe_gt *gt, struct drm_printer *p)
> > > > > >  	return 0;
> > > > > >  }
> > > > > >
> > > > > > +static int force_reset_sync(struct xe_gt *gt, struct drm_printer *p)
> > > > > > +{
> > > > > > +	xe_pm_runtime_get(gt_to_xe(gt));
> > > > > > +	xe_gt_reset_async(gt);
> > > > > > +	xe_pm_runtime_put(gt_to_xe(gt));
> > > > > > +
> 
> why aren't we just calling force_reset(gt, p); here?
> 

Opps, I missed that in my review and agree what you suggest is better.
Given that Rodrigo just pushed this, let's do a quick follow up to fix?

Matt 

> Lucas De Marchi
> 
> > > > > > +	flush_work(&gt->reset.worker);
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  static int sa_info(struct xe_gt *gt, struct drm_printer *p)
> > > > > >  {
> > > > > >  	struct xe_tile *tile = gt_to_tile(gt);
> > > > > > @@ -261,6 +272,7 @@ static int vecs_default_lrc(struct xe_gt *gt, struct drm_printer *p)
> > > > > >  static const struct drm_info_list debugfs_list[] = {
> > > > > >  	{"hw_engines", .show = xe_gt_debugfs_simple_show, .data = hw_engines},
> > > > > >  	{"force_reset", .show = xe_gt_debugfs_simple_show, .data = force_reset},
> > > > > > +	{"force_reset_sync", .show = xe_gt_debugfs_simple_show, .data = force_reset_sync},
> > > > > >  	{"sa_info", .show = xe_gt_debugfs_simple_show, .data = sa_info},
> > > > > >  	{"topology", .show = xe_gt_debugfs_simple_show, .data = topology},
> > > > > >  	{"steering", .show = xe_gt_debugfs_simple_show, .data = steering},
> > > > > > --
> > > > > > 2.25.1
> > > > > >
> > > > >
> > >