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

[Matt Roper]

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


Matt

> 
> 
> Matt
> 
> >  }
> >  
> >  /* XXX: add support for register ranges, maybe REGISTER..REGISTER */
> > -- 
> > 2.46.1
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation