Re: [PATCH i-g-t 1/7] lib/gt: Fix IGT_NO_FORCEWAKE handling

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

On Wed, Sep 24, 2025 at 08:22:22AM -0500, Lucas De Marchi wrote:
> On Mon, Sep 22, 2025 at 07:09:43PM +0300, Ville Syrjälä wrote:
> >From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> >igt_open_forcewake_handle_for_pcidev() forgets to check for
> >the IGT_NO_FORCEWAKE environment variable, which is a real
> >problem when one wants to frob the registers without forcing
> >power state changes in the hardware (eg. to observe display
> >low power watermark residencies/etc.)
> >
> >Add the missing IGT_NO_FORCEWAKE check.
> >
> >Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> >Fixes: 114b9ff31d19 ("lib/igt_gt: Add igt_open_forcewake_handle_for_pcidev()")
> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> We should also probably update this comment in lib/intel_mmio.c:
> 
>         /* Find where the forcewake lock is. Forcewake doesn't exist
>          * gen < 6, but the debugfs should do the right things for us.
>          */
>         ret = igt_open_forcewake_handle_for_pcidev(pci_dev);
>         if (ret < 0)
>                 mmio_data->key = FAKEKEY;
> 
> because it's not only about gen < 6, but rather that it can also be
> bypassed.
> 
> Also odd that there's no way to differentiate a real error from a fake
> forcewake.

Are there real errors? I suppose in some cases you might consider the
lack of a driver to be an error, but for me it basically never is.

I suppose one way we could deal with this is to have the caller of
intel_register_access_init() specify whether it wants forcewake or
not. For display stuff we'd never want it, wheras gt stuff (is
presume there is some?) would perhaps always want it.

intel_reg is a much harder problem because sometimes you want
forcewake and sometimes you don't. That could be the case even
for registers that need forcewake because you may want to see
how the hw behaves when you don't have the required forcewake.

And I typically use both intel_reg and other display specific
tools simultaneously so the env variable takes care of everything
for me. Even if we changed the display tools to not take forcewake
I'd still need to set the env variable for intel_reg.

-- 
Ville Syrjälä
Intel