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

[Lucas De Marchi]

On Fri, Sep 20, 2024 at 09:30:28PM GMT, Ville Syrjälä wrote:
>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 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.

humn, but the current state is the worst possible one: it works
differently with and without. I was thinking the case of working without
a driver would be with a different backend.

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

maybe off by default and add --forcewake that would require the driver
to be loaded?

Lucas De Marchi

>
>-- 
>Ville Syrjälä
>Intel