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@xxxxxxxxx>
> > >> ---
> > >> 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@xxxxxxxxxxxxxx
> > - 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@xxxxxxxxx>
> > >
> > > Also, why is igt@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
> > >>
