On Mon, 24 Apr 2023 10:07:26 -0700, Dixit, Ashutosh wrote:
>
> 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
Or retain the function but don't use it.
> 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
> > > >>
