[Intel-gfx] [PATCH v6 i-g-t 0/4] tests/slpc: Add basic IGT test

[Intel-gfx] [PATCH v6 i-g-t 0/4] tests/slpc: Add basic IGT test

[Vinay Belgaumkar]

Borrow some subtests from xe_guc_pc. Also add per GT debugfs helpers.

v3: Review comments and add HAX patch
v4: Modify the condition for skipping the test
v5: Update the SLPC helper to per GT
v6: Review comments (Ashutosh)

Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

Vinay Belgaumkar (4):
  lib/debugfs: Add per GT debugfs helpers
  lib: Make SLPC helper function per GT
  i915_pm_freq_api: Add some basic SLPC igt tests
  HAX: tests/i915: Try out the SLPC IGT tests

 lib/igt_debugfs.c                     |  60 ++++++++++
 lib/igt_debugfs.h                     |   4 +
 lib/igt_pm.c                          |  36 ++++--
 lib/igt_pm.h                          |   3 +-
 tests/i915/i915_pm_freq_api.c         | 152 ++++++++++++++++++++++++++
 tests/intel-ci/fast-feedback.testlist |   2 +
 tests/meson.build                     |   1 +
 7 files changed, 247 insertions(+), 11 deletions(-)
 create mode 100644 tests/i915/i915_pm_freq_api.c

-- 
2.38.1

[Intel-gfx] [PATCH i-g-t 1/4] lib/debugfs: Add per GT debugfs helpers

[Vinay Belgaumkar]

These can be used to open per-gt debugfs files.

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Vinay Belgaumkar <viay.belgaumkar@intel.com>
---
 lib/igt_debugfs.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_debugfs.h |  4 ++++
 2 files changed, 64 insertions(+)

diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
index 05889bbe..afde2da6 100644
--- a/lib/igt_debugfs.c
+++ b/lib/igt_debugfs.c
@@ -217,6 +217,37 @@ int igt_debugfs_dir(int device)
 	return open(path, O_RDONLY);
 }
 
+/**
+ * igt_debugfs_gt_dir:
+ * @device: fd of the device
+ * @gt: GT instance number
+ *
+ * This opens the debugfs directory corresponding to device for use
+ * with igt_sysfs_get() and related functions.
+ *
+ * Returns:
+ * The directory fd, or -1 on failure.
+ */
+int igt_debugfs_gt_dir(int device, unsigned int gt)
+{
+	int debugfs_gt_dir_fd;
+	char path[PATH_MAX];
+	char gtpath[16];
+	int ret;
+
+	if (!igt_debugfs_path(device, path, sizeof(path)))
+		return -1;
+
+	ret = snprintf(gtpath, sizeof(gtpath), "/gt%u", gt);
+	igt_assert(ret < sizeof(gtpath));
+	strncat(path, gtpath, sizeof(path) - 1);
+
+	debugfs_gt_dir_fd = open(path, O_RDONLY);
+	igt_debug_on_f(debugfs_gt_dir_fd < 0, "path: %s\n", path);
+
+	return debugfs_gt_dir_fd;
+}
+
 /**
  * igt_debugfs_connector_dir:
  * @device: fd of the device
@@ -313,6 +344,35 @@ bool igt_debugfs_exists(int device, const char *filename, int mode)
 	return false;
 }
 
+/**
+ * igt_debugfs_gt_open:
+ * @device: open i915 drm fd
+ * @gt: gt instance number
+ * @filename: name of the debugfs node to open
+ * @mode: mode bits as used by open()
+ *
+ * This opens a debugfs file as a Unix file descriptor. The filename should be
+ * relative to the drm device's root, i.e. without "drm/$minor".
+ *
+ * Returns:
+ * The Unix file descriptor for the debugfs file or -1 if that didn't work out.
+ */
+int
+igt_debugfs_gt_open(int device, unsigned int gt, const char *filename, int mode)
+{
+	int dir, ret;
+
+	dir = igt_debugfs_gt_dir(device, gt);
+	if (dir < 0)
+		return dir;
+
+	ret = openat(dir, filename, mode);
+
+	close(dir);
+
+	return ret;
+}
+
 /**
  * igt_debugfs_simple_read:
  * @dir: fd of the debugfs directory
diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h
index 4824344a..3e6194ad 100644
--- a/lib/igt_debugfs.h
+++ b/lib/igt_debugfs.h
@@ -45,6 +45,10 @@ void __igt_debugfs_write(int fd, const char *filename, const char *buf, int size
 int igt_debugfs_simple_read(int dir, const char *filename, char *buf, int size);
 bool igt_debugfs_search(int fd, const char *filename, const char *substring);
 
+int igt_debugfs_gt_dir(int device, unsigned int gt);
+int igt_debugfs_gt_open(int device, unsigned int gt, const char *filename,
+			int mode);
+
 /**
  * igt_debugfs_read:
  * @filename: name of the debugfs file
-- 
2.38.1

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

[Vinay Belgaumkar]

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@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] = {};
 
-	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;
+	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
+ */
+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

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

[Dixit, Ashutosh]

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@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.

> +	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@intel.com>

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
>

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

[Belgaumkar, 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@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?
ok.
>
>> -	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.
yup, makes sense.
>
>> +	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.
Didn't want to introduce another aggregation here. If SLPC is enabled on 
GT0, it is obviously enabled on all other tiles on that device. There is 
no per tile SLPC/GuC control.
>
> 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".
ok.
>
> With the above comments addressed this is:
>
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> Also, why is igt@i915_pm_rps@basic-api still skipping on DG2/ATSM in
> pre-merge CI even after this series?

basic-api is supposed to skip on GuC platforms. It wasn't due to the 
test incorrectly reading the SLPC enabled status from debugfs (which is 
being fixed here).

Thanks for the review,

Vinay.

>
> 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
>>

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

[Belgaumkar, 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@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, 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.

Thanks,

Vinay.

>
>> +	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@intel.com>
>
> 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
>>

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

[Dixit, Ashutosh]

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@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.

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@intel.com>
> >
> > 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
> >>

Re: [Intel-gfx] [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@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@intel.com>
> > >
> > > 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
> > >>

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

[Dixit, Ashutosh]

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@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

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@intel.com>
> > > >
> > > > 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
> > > >>

[Intel-gfx] [PATCH i-g-t 3/4] i915_pm_freq_api: Add some basic SLPC igt tests

[Vinay Belgaumkar]

Validate basic api for GT freq control. Also test
interaction with GT reset. We skip rps tests with
SLPC enabled, this will re-introduce some coverage.
SLPC selftests are already covering some other workload
related scenarios.

v2: Rename test (Rodrigo)
v3: Review comments (Ashutosh)
v4: Skip when SLPC is disabled. Check for enable_guc is
not sufficient as kernel config may have it but the
platform doesn't actually support it.
v5: Use the updated SLPC helper

Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
---
 tests/i915/i915_pm_freq_api.c | 152 ++++++++++++++++++++++++++++++++++
 tests/meson.build             |   1 +
 2 files changed, 153 insertions(+)
 create mode 100644 tests/i915/i915_pm_freq_api.c

diff --git a/tests/i915/i915_pm_freq_api.c b/tests/i915/i915_pm_freq_api.c
new file mode 100644
index 00000000..f0f4e3f9
--- /dev/null
+++ b/tests/i915/i915_pm_freq_api.c
@@ -0,0 +1,152 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+
+#include <dirent.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <inttypes.h>
+#include <stdlib.h>
+#include <sys/stat.h>
+#include <sys/syscall.h>
+#include <sys/types.h>
+#include <unistd.h>
+
+#include "drmtest.h"
+#include "i915/gem.h"
+#include "igt_sysfs.h"
+#include "igt.h"
+
+IGT_TEST_DESCRIPTION("Test SLPC freq API");
+/*
+ * Too many intermediate components and steps before freq is adjusted
+ * Specially if workload is under execution, so let's wait 100 ms.
+ */
+#define ACT_FREQ_LATENCY_US 100000
+
+static uint32_t get_freq(int dirfd, uint8_t id)
+{
+	uint32_t val;
+
+	igt_assert(igt_sysfs_rps_scanf(dirfd, id, "%u", &val) == 1);
+
+	return val;
+}
+
+static int set_freq(int dirfd, uint8_t id, uint32_t val)
+{
+	return igt_sysfs_rps_printf(dirfd, id, "%u", val);
+}
+
+static void test_freq_basic_api(int dirfd, int gt)
+{
+	uint32_t rpn, rp0, rpe;
+
+	/* Save frequencies */
+	rpn = get_freq(dirfd, RPS_RPn_FREQ_MHZ);
+	rp0 = get_freq(dirfd, RPS_RP0_FREQ_MHZ);
+	rpe = get_freq(dirfd, RPS_RP1_FREQ_MHZ);
+	igt_info("System min freq: %dMHz; max freq: %dMHz\n", rpn, rp0);
+
+	/*
+	 * Negative bound tests
+	 * RPn is the floor
+	 * RP0 is the ceiling
+	 */
+	igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rpn - 1) < 0);
+	igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rp0 + 1) < 0);
+	igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, rpn - 1) < 0);
+	igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, rp0 + 1) < 0);
+
+	/* Assert min requests are respected from rp0 to rpn */
+	igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rp0) > 0);
+	igt_assert(get_freq(dirfd, RPS_MIN_FREQ_MHZ) == rp0);
+	igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rpe) > 0);
+	igt_assert(get_freq(dirfd, RPS_MIN_FREQ_MHZ) == rpe);
+	igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rpn) > 0);
+	igt_assert(get_freq(dirfd, RPS_MIN_FREQ_MHZ) == rpn);
+
+	/* Assert max requests are respected from rpn to rp0 */
+	igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, rpn) > 0);
+	igt_assert(get_freq(dirfd, RPS_MAX_FREQ_MHZ) == rpn);
+	igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, rpe) > 0);
+	igt_assert(get_freq(dirfd, RPS_MAX_FREQ_MHZ) == rpe);
+	igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, rp0) > 0);
+	igt_assert(get_freq(dirfd, RPS_MAX_FREQ_MHZ) == rp0);
+
+}
+
+static void test_reset(int i915, int dirfd, int gt)
+{
+	uint32_t rpn = get_freq(dirfd, RPS_RPn_FREQ_MHZ);
+	int fd;
+
+	igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, rpn) > 0);
+	igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, rpn) > 0);
+	usleep(ACT_FREQ_LATENCY_US);
+	igt_assert(get_freq(dirfd, RPS_MIN_FREQ_MHZ) == rpn);
+
+	/* Manually trigger a GT reset */
+	fd = igt_debugfs_gt_open(i915, gt, "reset", O_WRONLY);
+	igt_require(fd >= 0);
+	igt_ignore_warn(write(fd, "1\n", 2));
+	close(fd);
+
+	igt_assert(get_freq(dirfd, RPS_MIN_FREQ_MHZ) == rpn);
+	igt_assert(get_freq(dirfd, RPS_MAX_FREQ_MHZ) == rpn);
+}
+
+igt_main
+{
+	int i915 = -1;
+	uint32_t *stash_min, *stash_max;
+
+	igt_fixture {
+		int num_gts, dirfd, gt;
+
+		i915 = drm_open_driver(DRIVER_INTEL);
+		igt_require_gem(i915);
+		/* i915_pm_rps already covers execlist path */
+		igt_skip_on_f(!i915_is_slpc_enabled(i915),
+			      "This test is supported only with SLPC enabled\n");
+
+		num_gts = igt_sysfs_get_num_gt(i915);
+		stash_min = (uint32_t*)malloc(sizeof(uint32_t) * num_gts);
+		stash_max = (uint32_t*)malloc(sizeof(uint32_t) * num_gts);
+
+		/* Save curr min and max across GTs */
+		for_each_sysfs_gt_dirfd(i915, dirfd, gt) {
+			stash_min[gt] = get_freq(dirfd, RPS_MIN_FREQ_MHZ);
+			stash_max[gt] = get_freq(dirfd, RPS_MAX_FREQ_MHZ);
+		}
+	}
+
+	igt_describe("Test basic API for controlling min/max GT frequency");
+	igt_subtest_with_dynamic_f("freq-basic-api") {
+		int dirfd, gt;
+
+		for_each_sysfs_gt_dirfd(i915, dirfd, gt)
+			igt_dynamic_f("gt%u", gt)
+				test_freq_basic_api(dirfd, gt);
+	}
+
+	igt_describe("Test basic freq API works after a reset");
+	igt_subtest_with_dynamic_f("freq-reset") {
+		int dirfd, gt;
+
+		for_each_sysfs_gt_dirfd(i915, dirfd, gt)
+			igt_dynamic_f("gt%u", gt)
+				test_reset(i915, dirfd, gt);
+	}
+
+	igt_fixture {
+		int dirfd, gt;
+		/* Restore frequencies */
+		for_each_sysfs_gt_dirfd(i915, dirfd, gt) {
+			igt_assert(set_freq(dirfd, RPS_MAX_FREQ_MHZ, stash_max[gt]) > 0);
+			igt_assert(set_freq(dirfd, RPS_MIN_FREQ_MHZ, stash_min[gt]) > 0);
+		}
+		close(i915);
+	}
+}
diff --git a/tests/meson.build b/tests/meson.build
index da31e782..46109f10 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -202,6 +202,7 @@ i915_progs = [
 	'gem_workarounds',
 	'i915_fb_tiling',
 	'i915_getparams_basic',
+	'i915_pm_freq_api',
 	'i915_hangman',
 	'i915_hwmon',
 	'i915_module_load',
-- 
2.38.1

[Intel-gfx] [PATCH i-g-t 4/4] HAX: tests/i915: Try out the SLPC IGT tests

[Vinay Belgaumkar]

Trying out for CI. Do not review.

Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
---
 tests/intel-ci/fast-feedback.testlist | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/intel-ci/fast-feedback.testlist b/tests/intel-ci/fast-feedback.testlist
index d9fcb62d..653668dd 100644
--- a/tests/intel-ci/fast-feedback.testlist
+++ b/tests/intel-ci/fast-feedback.testlist
@@ -139,6 +139,8 @@ igt@prime_self_import@basic-with_fd_dup
 igt@prime_self_import@basic-with_one_bo
 igt@prime_self_import@basic-with_one_bo_two_files
 igt@prime_self_import@basic-with_two_bos
+igt@i915_pm_freq_api@freq-basic-api
+igt@i915_pm_freq_api@freq-reset
 igt@prime_vgem@basic-fence-flip
 igt@prime_vgem@basic-fence-mmap
 igt@prime_vgem@basic-fence-read
-- 
2.38.1

[Intel-gfx] [PATCH v5 i-g-t 0/4] tests/slpc: Add basic IGT test

[Vinay Belgaumkar]

Borrow some subtests from xe_guc_pc. Also add per GT debugfs helpers.

v3: Review comments and add HAX patch
v4: Modify the condition for skipping the test
v5: Update the SLPC helper to per GT

Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

Vinay Belgaumkar (4):
  lib/debugfs: Add per GT debugfs helpers
  lib: Make SLPC helper function per GT
  i915_pm_freq_api: Add some basic SLPC igt tests
  HAX: tests/i915: Try out the SLPC IGT tests

 lib/igt_debugfs.c                     |  60 ++++++++++
 lib/igt_debugfs.h                     |   4 +
 lib/igt_pm.c                          |  20 ++--
 lib/igt_pm.h                          |   2 +-
 tests/i915/i915_pm_freq_api.c         | 152 ++++++++++++++++++++++++++
 tests/i915/i915_pm_rps.c              |   6 +-
 tests/intel-ci/fast-feedback.testlist |   2 +
 tests/meson.build                     |   1 +
 8 files changed, 233 insertions(+), 14 deletions(-)
 create mode 100644 tests/i915/i915_pm_freq_api.c

-- 
2.38.1

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

[Vinay Belgaumkar]

Use default of 0 where GT id is not being used.

Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
---
 lib/igt_pm.c             | 20 ++++++++++----------
 lib/igt_pm.h             |  2 +-
 tests/i915/i915_pm_rps.c |  6 +++---
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/lib/igt_pm.c b/lib/igt_pm.c
index 704acf7d..8ca7c181 100644
--- a/lib/igt_pm.c
+++ b/lib/igt_pm.c
@@ -1329,21 +1329,21 @@ void igt_pm_print_pci_card_runtime_status(void)
 	}
 }
 
-bool i915_is_slpc_enabled(int fd)
+bool i915_is_slpc_enabled(int drm_fd, int gt)
 {
-	int debugfs_fd = igt_debugfs_dir(fd);
-	char buf[4096] = {};
-	int len;
+	int debugfs_fd;
+	char buf[256] = {};
+
+	debugfs_fd = igt_debugfs_gt_open(drm_fd, gt, "uc/guc_slpc_info", O_RDONLY);
 
-	igt_require(debugfs_fd != -1);
+	/* if guc_slpc_info not present then return false */
+	if (debugfs_fd < 0)
+		return false;
+	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");
 }
 
 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..1b054dce 100644
--- a/lib/igt_pm.h
+++ b/lib/igt_pm.h
@@ -84,7 +84,7 @@ 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(int fd, int gt);
 int igt_pm_get_runtime_suspended_time(struct pci_device *pci_dev);
 int igt_pm_get_runtime_usage(struct pci_device *pci_dev);
 
diff --git a/tests/i915/i915_pm_rps.c b/tests/i915/i915_pm_rps.c
index d4ee2d58..85dae449 100644
--- a/tests/i915/i915_pm_rps.c
+++ b/tests/i915/i915_pm_rps.c
@@ -916,21 +916,21 @@ igt_main
 	}
 
 	igt_subtest("basic-api") {
-		igt_skip_on_f(i915_is_slpc_enabled(drm_fd),
+		igt_skip_on_f(i915_is_slpc_enabled(drm_fd, 0),
 			      "This subtest is not supported when SLPC is enabled\n");
 		min_max_config(basic_check, false);
 	}
 
 	/* Verify the constraints, check if we can reach idle */
 	igt_subtest("min-max-config-idle") {
-		igt_skip_on_f(i915_is_slpc_enabled(drm_fd),
+		igt_skip_on_f(i915_is_slpc_enabled(drm_fd, 0),
 			      "This subtest is not supported when SLPC is enabled\n");
 		min_max_config(idle_check, true);
 	}
 
 	/* Verify the constraints with high load, check if we can reach max */
 	igt_subtest("min-max-config-loaded") {
-		igt_skip_on_f(i915_is_slpc_enabled(drm_fd),
+		igt_skip_on_f(i915_is_slpc_enabled(drm_fd, 0),
 			      "This subtest is not supported when SLPC is enabled\n");
 		load_helper_run(HIGH);
 		min_max_config(loaded_check, false);
-- 
2.38.1

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

[Dixit, Ashutosh]

On Thu, 13 Apr 2023 15:44:12 -0700, Vinay Belgaumkar wrote:
>
> Use default of 0 where GT id is not being used.
>
> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> ---
>  lib/igt_pm.c             | 20 ++++++++++----------
>  lib/igt_pm.h             |  2 +-
>  tests/i915/i915_pm_rps.c |  6 +++---
>  3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 704acf7d..8ca7c181 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -1329,21 +1329,21 @@ void igt_pm_print_pci_card_runtime_status(void)
>	}
>  }
>
> -bool i915_is_slpc_enabled(int fd)
> +bool i915_is_slpc_enabled(int drm_fd, int gt)

OK, we understand that the debugfs dir path is per gt, but I am wondering
if we need to expose this as a function argument? Since, in all instances,
we are always passing gt as 0.

Maybe the caller is only interested in knowing if slpc is enabled. Can SLPC
be enabled for gt 0 and disabled for gt 1? In the case the caller should
really call something like:

	for_each_gt()
		i915_is_slpc_enabled(fd, gt)

and return false if slpc is disabled for any gt.

I think what we should do is write two functions:

1. Rename the function above with the gt argument to something like:

	i915_is_slpc_enabled_gt()

2. Have another function without the gt argument:

	i915_is_slpc_enabled() which will do:

	for_each_gt()
		i915_is_slpc_enabled_gt(fd, gt)

and return false if slpc is disabled for any gt.

And then have the tests call this second function without the gt argument.

I think this will be cleaner than passing 0 as the gt from the tests.

Thanks.
--
Ashutosh


>  {
> -	int debugfs_fd = igt_debugfs_dir(fd);
> -	char buf[4096] = {};
> -	int len;
> +	int debugfs_fd;
> +	char buf[256] = {};
> +
> +	debugfs_fd = igt_debugfs_gt_open(drm_fd, gt, "uc/guc_slpc_info", O_RDONLY);
>
> -	igt_require(debugfs_fd != -1);
> +	/* if guc_slpc_info not present then return false */
> +	if (debugfs_fd < 0)
> +		return false;
> +	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");
>  }
>
>  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..1b054dce 100644
> --- a/lib/igt_pm.h
> +++ b/lib/igt_pm.h
> @@ -84,7 +84,7 @@ 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(int fd, int gt);
>  int igt_pm_get_runtime_suspended_time(struct pci_device *pci_dev);
>  int igt_pm_get_runtime_usage(struct pci_device *pci_dev);
>
> diff --git a/tests/i915/i915_pm_rps.c b/tests/i915/i915_pm_rps.c
> index d4ee2d58..85dae449 100644
> --- a/tests/i915/i915_pm_rps.c
> +++ b/tests/i915/i915_pm_rps.c
> @@ -916,21 +916,21 @@ igt_main
>	}
>
>	igt_subtest("basic-api") {
> -		igt_skip_on_f(i915_is_slpc_enabled(drm_fd),
> +		igt_skip_on_f(i915_is_slpc_enabled(drm_fd, 0),
>			      "This subtest is not supported when SLPC is enabled\n");
>		min_max_config(basic_check, false);
>	}
>
>	/* Verify the constraints, check if we can reach idle */
>	igt_subtest("min-max-config-idle") {
> -		igt_skip_on_f(i915_is_slpc_enabled(drm_fd),
> +		igt_skip_on_f(i915_is_slpc_enabled(drm_fd, 0),
>			      "This subtest is not supported when SLPC is enabled\n");
>		min_max_config(idle_check, true);
>	}
>
>	/* Verify the constraints with high load, check if we can reach max */
>	igt_subtest("min-max-config-loaded") {
> -		igt_skip_on_f(i915_is_slpc_enabled(drm_fd),
> +		igt_skip_on_f(i915_is_slpc_enabled(drm_fd, 0),
>			      "This subtest is not supported when SLPC is enabled\n");
>		load_helper_run(HIGH);
>		min_max_config(loaded_check, false);
> --
> 2.38.1
>

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

[Belgaumkar, Vinay]

On 4/14/2023 11:10 AM, Dixit, Ashutosh wrote:
> On Thu, 13 Apr 2023 15:44:12 -0700, Vinay Belgaumkar wrote:
>> Use default of 0 where GT id is not being used.
>>
>> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
>> ---
>>   lib/igt_pm.c             | 20 ++++++++++----------
>>   lib/igt_pm.h             |  2 +-
>>   tests/i915/i915_pm_rps.c |  6 +++---
>>   3 files changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
>> index 704acf7d..8ca7c181 100644
>> --- a/lib/igt_pm.c
>> +++ b/lib/igt_pm.c
>> @@ -1329,21 +1329,21 @@ void igt_pm_print_pci_card_runtime_status(void)
>> 	}
>>   }
>>
>> -bool i915_is_slpc_enabled(int fd)
>> +bool i915_is_slpc_enabled(int drm_fd, int gt)
> OK, we understand that the debugfs dir path is per gt, but I am wondering
> if we need to expose this as a function argument? Since, in all instances,
> we are always passing gt as 0.
>
> Maybe the caller is only interested in knowing if slpc is enabled. Can SLPC
> be enabled for gt 0 and disabled for gt 1? In the case the caller should
> really call something like:
>
> 	for_each_gt()
> 		i915_is_slpc_enabled(fd, gt)
>
> and return false if slpc is disabled for any gt.
>
> I think what we should do is write two functions:
>
> 1. Rename the function above with the gt argument to something like:
>
> 	i915_is_slpc_enabled_gt()
>
> 2. Have another function without the gt argument:
>
> 	i915_is_slpc_enabled() which will do:
>
> 	for_each_gt()
> 		i915_is_slpc_enabled_gt(fd, gt)
>
> and return false if slpc is disabled for any gt.
>
> And then have the tests call this second function without the gt argument.
>
> I think this will be cleaner than passing 0 as the gt from the tests.

ok, created a helper for the helper :) This will hard code GT 0 instead 
of the tests doing it, when necessary.

Thanks,

Vinay.

>
> Thanks.
> --
> Ashutosh
>
>
>>   {
>> -	int debugfs_fd = igt_debugfs_dir(fd);
>> -	char buf[4096] = {};
>> -	int len;
>> +	int debugfs_fd;
>> +	char buf[256] = {};
>> +
>> +	debugfs_fd = igt_debugfs_gt_open(drm_fd, gt, "uc/guc_slpc_info", O_RDONLY);
>>
>> -	igt_require(debugfs_fd != -1);
>> +	/* if guc_slpc_info not present then return false */
>> +	if (debugfs_fd < 0)
>> +		return false;
>> +	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");
>>   }
>>
>>   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..1b054dce 100644
>> --- a/lib/igt_pm.h
>> +++ b/lib/igt_pm.h
>> @@ -84,7 +84,7 @@ 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(int fd, int gt);
>>   int igt_pm_get_runtime_suspended_time(struct pci_device *pci_dev);
>>   int igt_pm_get_runtime_usage(struct pci_device *pci_dev);
>>
>> diff --git a/tests/i915/i915_pm_rps.c b/tests/i915/i915_pm_rps.c
>> index d4ee2d58..85dae449 100644
>> --- a/tests/i915/i915_pm_rps.c
>> +++ b/tests/i915/i915_pm_rps.c
>> @@ -916,21 +916,21 @@ igt_main
>> 	}
>>
>> 	igt_subtest("basic-api") {
>> -		igt_skip_on_f(i915_is_slpc_enabled(drm_fd),
>> +		igt_skip_on_f(i915_is_slpc_enabled(drm_fd, 0),
>> 			      "This subtest is not supported when SLPC is enabled\n");
>> 		min_max_config(basic_check, false);
>> 	}
>>
>> 	/* Verify the constraints, check if we can reach idle */
>> 	igt_subtest("min-max-config-idle") {
>> -		igt_skip_on_f(i915_is_slpc_enabled(drm_fd),
>> +		igt_skip_on_f(i915_is_slpc_enabled(drm_fd, 0),
>> 			      "This subtest is not supported when SLPC is enabled\n");
>> 		min_max_config(idle_check, true);
>> 	}
>>
>> 	/* Verify the constraints with high load, check if we can reach max */
>> 	igt_subtest("min-max-config-loaded") {
>> -		igt_skip_on_f(i915_is_slpc_enabled(drm_fd),
>> +		igt_skip_on_f(i915_is_slpc_enabled(drm_fd, 0),
>> 			      "This subtest is not supported when SLPC is enabled\n");
>> 		load_helper_run(HIGH);
>> 		min_max_config(loaded_check, false);
>> --
>> 2.38.1
>>