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

[Matt Roper <matthew.d.roper@intel.com>]

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>


Matt

>  }
>  
>  /* XXX: add support for register ranges, maybe REGISTER..REGISTER */
> -- 
> 2.46.1
> 

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