[igt-dev] [PATCH i-g-t 2/4] lib: Make SLPC helper function per GT

[Dixit, Ashutosh]

On Mon, 24 Apr 2023 09:55:14 -0700, Dixit, Ashutosh wrote:
>
> On Sun, 23 Apr 2023 13:16:44 -0700, Belgaumkar, Vinay wrote:
> >
>
> Hi Vinay,
>
> > On 4/14/2023 1:25 PM, Dixit, Ashutosh wrote:
> > > On Fri, 14 Apr 2023 12:16:37 -0700, Vinay Belgaumkar wrote:
> > > Hi Vinay,
> > >
> > >> Use default of 0 where GT id is not being used.
> > >>
> > >> v2: Add a helper for GT 0 (Ashutosh)
> > >>
> > >> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
> > >> ---
> > >>   lib/igt_pm.c | 36 ++++++++++++++++++++++++++----------
> > >>   lib/igt_pm.h |  3 ++-
> > >>   2 files changed, 28 insertions(+), 11 deletions(-)
> > >>
> > >> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> > >> index 704acf7d..8a30bb3b 100644
> > >> --- a/lib/igt_pm.c
> > >> +++ b/lib/igt_pm.c
> > >> @@ -1329,21 +1329,37 @@ void igt_pm_print_pci_card_runtime_status(void)
> > >>	}
> > >>   }
> > >>
> > >> -bool i915_is_slpc_enabled(int fd)
> > >> +/**
> > >> + * i915_is_slpc_enabled_gt:
> > >> + * @drm_fd: DRM file descriptor
> > >> + * @gt: GT id
> > >> + * Check if SLPC is enabled on a GT
> > >> + */
> > >> +bool i915_is_slpc_enabled_gt(int drm_fd, int gt)
> > >>   {
> > >> -	int debugfs_fd = igt_debugfs_dir(fd);
> > >> -	char buf[4096] = {};
> > >> -	int len;
> > >> +	int debugfs_fd;
> > >> +	char buf[256] = {};
> > > Shouldn't this be 4096 as before?
> > >
> > >> -	igt_require(debugfs_fd != -1);
> > >> +	debugfs_fd = igt_debugfs_gt_open(drm_fd, gt, "uc/guc_slpc_info", O_RDONLY);
> > >> +
> > >> +	/* if guc_slpc_info not present then return false */
> > >> +	if (debugfs_fd < 0)
> > >> +		return false;
> > > I think this should just be:
> > >
> > >	igt_require_fd(debugfs_fd);
> > >
> > > Basically we cannot determine if SLPC is enabled or not if say debugfs is
> > > not mounted, so it's not correct return false from here.
> >
> > Actually, rethinking on this, we should keep it to return false. This is
> > making tests skip on platforms where it shouldn't. Debugfs will not be
> > mounted only when driver load fails,
>
> Debugfs not being mounted has nothing to do with driver load, it is just
> that this command has not been run before running the tests (the system
> would typically be configured to run this after boot):
>
>	mount -t debugfs none /sys/kernel/debug/
>
> Ah, igt_debugfs_path() will mount debugfs if not mounted and assert if
> mount fails. So IGT itself is mounting debugfs if not mounted.
>
> > which would cause the test to fail
> > when we try to create the drm fd before this. Case in point -
> > https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8839/fi-tgl-1115g4/igt@i915_pm_rps@basic-api.html
> > - here, the test should have run (guc disabled platform) but it skipped.
>
> OK, sorry yes because it is checking for guc_slpc_info, which would
> indicate whether or not slpc is enabled.
>
> But the issue is still there, whether or not we solve it. Say SLPC is
> enabled but debugfs was not mounted. In the code above we will conclude
> that slpc is not enabled. Because mulitple conditions have been combined
> into one and there is no way to check for them separately (debugfs being
> mounted and guc_slpc_info being present).
>
> The original code above has this check:
>
>	igt_require(debugfs_fd != -1);
>
> Which is checking for whether or not debugfs is mounted. Where does this
> check move in this series?
>
> Anyway maybe for now just change the code to return false.

I think the correct way to do it would be remove igt_debugfs_gt_open from
Patch 1 and call the sequence in igt_debugfs_gt_open directly from
i915_is_slpc_enabled_gt, something like:

	dir = igt_debugfs_gt_dir(device, gt);
	igt_require(dir);

	debugfs_fd = openat(dir, "uc/guc_slpc_info", O_RDONLY);
	if (debugfs_fd < 0)
		return false;

>
> Thanks.
> --
> Ashutosh
>
> > >> +	read(debugfs_fd, buf, sizeof(buf)-1);
> > >>
> > >> -	len = igt_debugfs_simple_read(debugfs_fd, "gt/uc/guc_slpc_info", buf, sizeof(buf));
> > >>	close(debugfs_fd);
> > >>
> > >> -	if (len < 0)
> > >> -		return false;
> > >> -	else
> > >> -		return strstr(buf, "SLPC state: running");
> > >> +	return strstr(buf, "SLPC state: running");
> > >> +}
> > >> +
> > >> +/**
> > >> + * i915_is_slpc_enabled:
> > >> + * @drm_fd: DRM file descriptor
> > >> + * Check if SLPC is enabled on GT 0
> > > Hmm, not sure why we are not using the i915_for_each_gt() loop here since
> > > that is the correct way of doing it.
> > >
> > > At the min let's remove the GT 0 in the comment above. This function
> > > doesn't check for GT0, it checks if "slpc is enabled for the device". We
> > > can check only on GT0 if we are certain that checking on GT0 is sufficient,
> > > that is if SLPC is disabled on GT0 it's disabled for the device. But then
> > > someone can ask the question in that case why are we exposing slpc_enabled
> > > for each gt from the kernel rather than at the device level.
> > >
> > > In any case for now let's change the above comment to:
> > >
> > > "Check if SLPC is enabled" or ""Check if SLPC is enabled for the i915
> > > device".
> > >
> > > With the above comments addressed this is:
> > >
> > > Reviewed-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > >
> > > Also, why is igt at i915_pm_rps@basic-api still skipping on DG2/ATSM in
> > > pre-merge CI even after this series?
> > >
> > > Thanks.
> > > --
> > > Ashutosh
> > >
> > >
> > >> + */
> > >> +bool i915_is_slpc_enabled(int drm_fd)
> > >> +{
> > >> +	return i915_is_slpc_enabled_gt(drm_fd, 0);
> > >>   }
> > >>   int igt_pm_get_runtime_suspended_time(struct pci_device *pci_dev)
> > >> diff --git a/lib/igt_pm.h b/lib/igt_pm.h
> > >> index d0d6d673..448cf42d 100644
> > >> --- a/lib/igt_pm.h
> > >> +++ b/lib/igt_pm.h
> > >> @@ -84,7 +84,8 @@ void igt_pm_set_d3cold_allowed(struct igt_device_card *card, const char *val);
> > >>   void igt_pm_setup_pci_card_runtime_pm(struct pci_device *pci_dev);
> > >>   void igt_pm_restore_pci_card_runtime_pm(void);
> > >>   void igt_pm_print_pci_card_runtime_status(void);
> > >> -bool i915_is_slpc_enabled(int fd);
> > >> +bool i915_is_slpc_enabled_gt(int drm_fd, int gt);
> > >> +bool i915_is_slpc_enabled(int drm_fd);
> > >>   int igt_pm_get_runtime_suspended_time(struct pci_device *pci_dev);
> > >>   int igt_pm_get_runtime_usage(struct pci_device *pci_dev);
> > >>
> > >> --
> > >> 2.38.1
> > >>