Re: [PATCH i-g-t 6/6] tools/intel_reg: Add forcewake support to xe

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Sep 20, 2024 at 11:04:20AM -0700, Matt Roper wrote:
> On Fri, Sep 20, 2024 at 11:01:37AM -0700, Matt Roper wrote:
> > On Wed, Sep 18, 2024 at 09:36:29AM -0700, Lucas De Marchi wrote:
> > > Now that the igt lib is prepared, start passing the open driver fd so
> > > the correct forcewake is taken.
> > > 
> > > Before:
> > > $ sudo ./build/tools/intel_reg read 0x2358
> > >                                     (0x00002358): 0xffffffff
> > 
> > Hmm, this example makes me wonder if we actually should have called this
> > top-level "forcewake_all" debugfs node something else.  This example
> > shows that we're not relying on this interface solely for forcewake
> > purposes, but as a wider "wake everything up and make sure it's
> > accessible" interface.  At the hardware level, forcewake deals only with
> > the GT-specific ability to exit+suppress RC6; if we read GT registers
> > while in RC6, we'll get back 0x0 as a result (which isn't what we see
> > here).
> > 
> > The failures we had here were because the device itself had gone into
> > runtime D3 (so register reads come back as ~0); using forcewake
> > indirectly suppresses runtime D3 as a side effect.  But if you'd wanted
> > to read a non-GT register (sgunit, display, etc.), those register reads
> > also would have failed before this change due to D3, even though the
> > registers themselves don't need forcewake.  So we're in a situation
> > where we also rely on the "forcewake" interface for reading/writing
> > non-GT registers that don't care about forcewake.
> > 
> > If a future platform adds, for example, a new sleep + wakelock system to
> > part of the sgunit, do we want to make this debugfs automatically wake
> > the sgunit too in case we want to read its registers?  For that matter,
> > should this device-level "wake up everything so we can read registers"
> > interface also be grabbing all of our display power wells by default?
> > 
> > Anyway, I'm probably overthinking this; we don't need to worry about it
> > for this patch series.
> > 
> > > 
> > > After:
> > > $ sudo ./build/tools/intel_reg read 0x2358
> > > Opened device: /dev/dri/card0
> > >                                     (0x00002358): 0x91e59eeb
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > >  tools/intel_reg.c | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/intel_reg.c b/tools/intel_reg.c
> > > index 906ae9b84..b650c3697 100644
> > > --- a/tools/intel_reg.c
> > > +++ b/tools/intel_reg.c
> > > @@ -802,7 +802,18 @@ static int parse_reg(struct config *config, struct reg *reg, const char *s)
> > >  
> > >  static int register_access_init(struct config *config)
> > >  {
> > > -	return intel_register_access_init(&config->mmio_data, config->pci_dev, 0, -1);
> > > +	int drm_fd = __drm_open_driver(DRIVER_INTEL | DRIVER_XE);
> > > +	int ret;
> > > +
> > > +	if (drm_fd < 0) {
> > > +		fprintf(stderr, "could not open i915 or xe device\n");
> > > +		return EXIT_FAILURE;
> > > +	}
> > > +
> > > +	ret = intel_register_access_init(&config->mmio_data, config->pci_dev, 0, drm_fd);
> > > +	close(drm_fd);
> > > +
> > > +	return ret;
> > 
> > As noted on the previous patch, if we get here, it's always a 'return 0'
> > since intel_register_access_init() can't fail (and should probably have
> > just been a void function).  But not a blocker.
> > 
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Actually I just saw Ville's reply and it's a good point that this tool
> did used to be able to run before/without the driver loaded.  So we
> should just skip the "wake stuff up" part and proceed without failing
> for that case.

I've also been mildly tempted to make IGT_NO_FORCEWAKE=1
the default because it's annoying when I sometimes forget
to set it and then I might no longer be doing just the
register access I though I was doing. But that might
be a bit surprising to folks wanting to read GT
registers/etc. I suppose a sensible middle ground would
be to disable the forcewake stuff by defaut only in
the display specific tools, leaving it on for the likes
of intel_reg and anything gt specific.

-- 
Ville Syrjälä
Intel