lpsp platform agnostic support

This lib function evaluate the lpsp capability from the connector specific debugfs attribute i915_lpsp_info.

v2:
- changed the lib function prefix igt_output_is_lpsp_capable to i915_output_is_lpsp_capable. [Martin]
v3:
- early return for the connector which doesn't support lpsp on any platform.
v4:
- debugfs entry changed from i915_lpsp_info to i915_lpsp_capability.

Signed-off-by Anshuman Gupta <[email protected]>

---
  lib/igt_pm.c | 37 +++++++++++++++++++++++++++++++++++++
  lib/igt_pm.h |  1 +
  2 files changed, 38 insertions(+)

lib/igt_pm.c +14 -0

index 9d441e1b..3418b69a 100644
@@ -37,6 +37,7 @@
  #include <dirent.h>
 
  #include "drmtest.h"
+#include "igt_kms.h"
  #include "igt_pm.h"
  #include "igt_aux.h"
  #include "igt_sysfs.h"

@@ -827,3 +828,39 @@ bool igt_pm_pc8_plus_residencies_enabled(int msr_fd)
 
  	return true;
  }
+
+/**
+ * i915_output_is_lpsp_capable:
+ * @drm_fd: fd to drm device
+ * @output: igt output for which lpsp capability need to be evaluated
+ * Check lpsp capability for a given output.
+ *
+ * Returns:
+ * True if given output is lpsp capable otherwise false.
+ */
+bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output)
+{
+	drmModeConnectorPtr c = output->config.connector;

lib/igt_pm.c +13 -0

+
+	fd = igt_debugfs_connector_dir(drm_fd, output->name, O_RDONLY);
+	igt_require(fd >= 0);
+	len = igt_debugfs_simple_read(fd, "i915_lpsp_capability",
+				      buf, sizeof(buf));
+
+	if (len < 0)
+		igt_assert_eq(len, -ENODEV);
+
+	close(fd);
+
+	return strstr(buf, "LPSP: capable");
+}

lib/igt_pm.h +0 -0

index 5e438452..162d3ca3 100644

Quoted 49 lines · Anshuman Gupta

This lib function evaluate the lpsp capability from the connector specific debugfs attribute i915_lpsp_info.

v2:
- changed the lib function prefix igt_output_is_lpsp_capable to i915_output_is_lpsp_capable. [Martin]
v3:
- early return for the connector which doesn't support lpsp on any platform.
v4:
- debugfs entry changed from i915_lpsp_info to i915_lpsp_capability.

Signed-off-by Anshuman Gupta <[email protected]>

---
 lib/igt_pm.c | 37 +++++++++++++++++++++++++++++++++++++
 lib/igt_pm.h |  1 +
 2 files changed, 38 insertions(+)

lib/igt_pm.c +18 -0

index 9d441e1b..3418b69a 100644
@@ -37,6 +37,7 @@
 #include <dirent.h>
 #include "drmtest.h"
+#include "igt_kms.h"
 #include "igt_pm.h"
 #include "igt_aux.h"
 #include "igt_sysfs.h"

@@ -827,3 +828,39 @@ bool igt_pm_pc8_plus_residencies_enabled(int msr_fd)
 	return true;
 }
+
+/**
+ * i915_output_is_lpsp_capable:
+ * @drm_fd: fd to drm device
+ * @output: igt output for which lpsp capability need to be evaluated
+ * Check lpsp capability for a given output.
+ *
+ * Returns:
+ * True if given output is lpsp capable otherwise false.
+ */
+bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output)
+{
+	drmModeConnectorPtr c = output->config.connector;
+	char buf[256];
+	int fd, len;
+
+	/* only eDP/DP/DSI/HDMI can support LPSP */

lib/igt_pm.c bool igt_pm_pc8_plus_residencies_enabled(int msr_fd) · line 853

Quoted 6 lines

lib/igt_pm.c +6 -0

+	if (c->connector_type != DRM_MODE_CONNECTOR_eDP &&
+	    c->connector_type != DRM_MODE_CONNECTOR_DSI &&
+	    c->connector_type != DRM_MODE_CONNECTOR_DisplayPort &&
+	    c->connector_type != DRM_MODE_CONNECTOR_HDMIA &&
+	    c->connector_type != DRM_MODE_CONNECTOR_HDMIB)
+		return false;

We can avoid check for Connector type as already taken care in kernel... rt?

Regards,
Animesh

Quoted 15 lines

lib/igt_pm.c +13 -0

+
+	fd = igt_debugfs_connector_dir(drm_fd, output->name, O_RDONLY);
+	igt_require(fd >= 0);
+	len = igt_debugfs_simple_read(fd, "i915_lpsp_capability",
+				      buf, sizeof(buf));
+
+	if (len < 0)
+		igt_assert_eq(len, -ENODEV);
+
+	close(fd);
+
+	return strstr(buf, "LPSP: capable");
+}

lib/igt_pm.h +0 -0

index 5e438452..162d3ca3 100644

lib/igt_pm.h enum igt_runtime_pm_status igt_get_runtime_pm_status(void); · line 57

Quoted 8 lines

lib/igt_pm.h +1 -0

@@ -53,5 +53,6 @@ enum igt_runtime_pm_status igt_get_runtime_pm_status(void);
 bool igt_wait_for_pm_status(enum igt_runtime_pm_status status);
 bool igt_pm_dmc_loaded(int debugfs);
 bool igt_pm_pc8_plus_residencies_enabled(int msr_fd);
+bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
 #endif /* IGT_PM_H */

This lib function evaluate the lpsp capability from the connector specific debugfs attribute i915_lpsp_info.

v2:
- changed the lib function prefix igt_output_is_lpsp_capable to i915_output_is_lpsp_capable. [Martin]
v3:
- early return for the connector which doesn't support lpsp on any platform.
v4:
- debugfs entry changed from i915_lpsp_info to i915_lpsp_capability. v5:
- return lpsp capability false if i915_lpsp_capability debugfs is not present. [Animesh]

Signed-off-by Anshuman Gupta <[email protected]>

---
  lib/igt_pm.c | 29 +++++++++++++++++++++++++++++
  lib/igt_pm.h |  1 +
  2 files changed, 30 insertions(+)

lib/igt_pm.c +29 -0

index 9d441e1b..b409ec46 100644
@@ -37,6 +37,7 @@
  #include <dirent.h>
 
  #include "drmtest.h"
+#include "igt_kms.h"
  #include "igt_pm.h"
  #include "igt_aux.h"
  #include "igt_sysfs.h"

@@ -827,3 +828,31 @@ bool igt_pm_pc8_plus_residencies_enabled(int msr_fd)
 
  	return true;
  }
+
+/**
+ * i915_output_is_lpsp_capable:
+ * @drm_fd: fd to drm device
+ * @output: igt output for which lpsp capability need to be evaluated
+ * Check lpsp capability for a given output.
+ *
+ * Returns:
+ * True if given output is lpsp capable otherwise false.
+ */
+bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output)
+{
+	char buf[256];
+	int fd, len;
+
+	fd = igt_debugfs_connector_dir(drm_fd, output->name, O_RDONLY);
+	igt_require(fd >= 0);
+	len = igt_debugfs_simple_read(fd, "i915_lpsp_capability",
+				      buf, sizeof(buf));
+
+	/* if i915_lpsp_capability not present return the capability as false */
+	if (len < 0)
+		return false;
+
+	close(fd);
+
+	return strstr(buf, "LPSP: capable");
+}

lib/igt_pm.h +0 -0

index 5e438452..162d3ca3 100644

Current implementation of lpsp igt test, assumed that every non-edp panel isn't a lpsp panel but it is not true on TGL anymore, any HDMI/DP/DSI panel connected on pipe A and connected to PORT_{A,B,C} can drive LPSP.
Even on older Gen9 platform a DP panel can drive lpsp on Port A. This requires complete design change in current lpsp igt for a platform agnostic support.

The new igt approach is relies on connector specific i915_lpsp_capability and i915_lpsp_status debugfs attributes, these debugfs exposes whether an output is capable of driving lpsp and lpsp is enabled.

Nuking edp-native and non-edp test, introducing a new dynamic igt subtest kms-lpsp, which validates lpsp on each connected output and skip the subtest if output is not lpsp capable.

v2:
- CI failures fixup.
v3:
- removed unloading of snd_modules. [Martin]
v4:
- Don't test non-lpsp(if lpsp disabled), no ROI to test that.
- nuke panel-fitter test.
v5:
- Added dynamic subtest and igt changes according to kernel debugfs i915_lpsp_status changes.

Signed-off-by Anshuman Gupta <[email protected]>

---
  tests/i915/i915_pm_lpsp.c | 285 +++++++++++++++-----------------------
  1 file changed, 114 insertions(+), 171 deletions(-)

tests/i915/i915_pm_lpsp.c +23 -13

index 08f82e7c..7be23e6c 100644
@@ -25,210 +25,153 @@
   */
 
  #include "igt.h"
+#include "igt_kmod.h"
+#include "igt_pm.h"
+#include "igt_sysfs.h"
  #include <sys/types.h>
  #include <sys/stat.h>
  #include <fcntl.h>
  #include <unistd.h>
 
+#define MAX_SINK_LPSP_INFO_BUF_LEN	4096
 
-static bool supports_lpsp(uint32_t devid)
-{
-	return IS_HASWELL(devid) || IS_BROADWELL(devid);
-}
+#define PWR_DOMAIN_INFO "i915_power_domain_info"
 
-static bool lpsp_is_enabled(int drm_fd)
+typedef struct {
+	int drm_fd;
+	int debugfs_fd;
+	uint32_t devid;
+	char *pwr_dmn_info;
+	igt_display_t display;
+	struct igt_fb fb;
+	drmModeModeInfo *mode;
+	igt_output_t *output;
+} data_t;
+
+static bool lpsp_is_enabled(data_t *data)
  {
-	uint32_t val;
+	char buf[MAX_SINK_LPSP_INFO_BUF_LEN];
+	int len;
 
-	val = INREG(HSW_PWR_WELL_CTL2);
-	return !(val & HSW_PWR_WELL_STATE_ENABLED);
-}
+	len = igt_debugfs_simple_read(data->debugfs_fd, "i915_lpsp_status",
+				      buf, sizeof(buf));
+	if (len < 0)
+		igt_assert_eq(len, -ENODEV);
 
-/* The LPSP mode is all about an enabled pipe, but we expect to also be in the
- * low power mode when no pipes are enabled, so do this check anyway. */
-static void screens_disabled_subtest(int drm_fd, drmModeResPtr drm_res)
-{

tests/i915/i915_pm_lpsp.c +70 -154

+		igt_display_commit(&data->display);
+		valid_output++;
+	}
 
-	return igt_create_pattern_fb(drm_fd, width, height, DRM_FORMAT_XRGB8888,
-				     LOCAL_DRM_FORMAT_MOD_NONE, &fb);
+	igt_require_f(valid_output, "No connected output found\n");
+	igt_assert_f(lpsp_is_enabled(data), "lpsp is not enabled\n%s:\n%s\n",
+		     PWR_DOMAIN_INFO, data->pwr_dmn_info =
+		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
  }
 
-static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			drmModeConnectorPtr *drm_connectors, uint32_t devid,
-			bool use_panel_fitter)
+static void setup_lpsp_output(data_t *data)
  {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
-	drmModeModeInfo std_1024_mode = {
-		.clock = 65000,
-		.hdisplay = 1024,
-		.hsync_start = 1048,
-		.hsync_end = 1184,
-		.htotal = 1344,
-		.hskew = 0,
-		.vdisplay = 768,
-		.vsync_start = 771,
-		.vsync_end = 777,
-		.vtotal = 806,
-		.vscan = 0,
-		.vrefresh = 60,
-		.flags = 0xA,
-		.type = 0x40,
-		.name = "Custom 1024x768",
-	};
-
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-
-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
-
-		if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
-			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
-			continue;
-
-		if (!use_panel_fitter && c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
-		if (use_panel_fitter) {
-			connector = c;
-
-			/* This is one of the modes Xorg creates for panels, so
-			 * it should work just fine. Notice that Gens that
-			 * support LPSP are too new for panels with native
-			 * 1024x768 resolution, so this should force the panel
-			 * fitter. */
-			igt_assert(c->count_modes &&
-				   c->modes[0].hdisplay > 1024);
-			igt_assert(c->count_modes &&
-				   c->modes[0].vdisplay > 768);
-			mode = &std_1024_mode;
-			break;
-		}
-	}
-	igt_require(connector);
-
-	crtc_id = kmstest_find_crtc_for_connector(drm_fd, drm_res, connector,
-						  0);
-	buffer_id = create_fb(drm_fd, mode->hdisplay, mode->vdisplay);
-
-	igt_assert(buffer_id);
-	igt_assert(connector);
-	igt_assert(mode);
-
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
-
-	if (use_panel_fitter) {
-		if (IS_HASWELL(devid))
-			igt_assert(!lpsp_is_enabled(drm_fd));
-		else
-			igt_assert(lpsp_is_enabled(drm_fd));
-	} else {
-		igt_assert(lpsp_is_enabled(drm_fd));
-	}
+	igt_plane_t *primary;
+
+	/* set output pipe = PIPE_A for LPSP */
+	igt_output_set_pipe(data->output, PIPE_A);
+	primary = igt_output_get_plane_type(data->output,
+					    DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+	igt_create_pattern_fb(data->drm_fd,
+			      data->mode->hdisplay, data->mode->vdisplay,
+			      DRM_FORMAT_XRGB8888,
+			      LOCAL_DRM_FORMAT_MOD_NONE,
+			      &data->fb);
+	igt_plane_set_fb(primary, &data->fb);
+	igt_display_commit(&data->display);
  }
 
-static void non_edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			    drmModeConnectorPtr *drm_connectors)
+static void test_cleanup(data_t *data)
  {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
-
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-
-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
-
-		if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
-			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
-			continue;
-
-		if (c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
-	}
-	igt_require(connector);
-
-	crtc_id = kmstest_find_crtc_for_connector(drm_fd, drm_res, connector,
-						  0);
-	buffer_id = create_fb(drm_fd, mode->hdisplay, mode->vdisplay);
-
-	igt_assert(buffer_id);
-	igt_assert(mode);
-
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
-
-	igt_assert(!lpsp_is_enabled(drm_fd));
+	igt_plane_t *primary;
+
+	if (!data->output || data->output->pending_pipe == PIPE_NONE)
+		return;
+
+	primary = igt_output_get_plane_type(data->output,
+					    DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+	igt_output_set_pipe(data->output, PIPE_NONE);
+	igt_display_commit(&data->display);
+	igt_remove_fb(data->drm_fd, &data->fb);
+	data->output = NULL;
  }
 
-#define MAX_CONNECTORS 32
+static void test_lpsp(data_t *data)
+{
+	/* LPSP is low power single pipe usages i.e. PIPE_A */
+	igt_require(igt_pipe_connector_valid(PIPE_A, data->output));
+	igt_require_f(i915_output_is_lpsp_capable(data->drm_fd, data->output),
+		      "output is not lpsp capable\n");
+
+	data->mode = igt_output_get_mode(data->output);
+	setup_lpsp_output(data);
+	igt_assert_f(lpsp_is_enabled(data), "%s: lpsp is not enabled\n%s:\n%s\n",
+		     data->output->name, PWR_DOMAIN_INFO, data->pwr_dmn_info =
+		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
+}
 
-int drm_fd;
-uint32_t devid;
-drmModeResPtr drm_res;
-drmModeConnectorPtr drm_connectors[MAX_CONNECTORS];
-struct intel_mmio_data mmio_data;
+IGT_TEST_DESCRIPTION("These tests validates display Low Power Single Pipe configurations");
  igt_main
  {
-	igt_fixture {
-		int i;
-
-		drm_fd = drm_open_driver_master(DRIVER_INTEL);
-		igt_require(drm_fd >= 0);
-
-		devid = intel_get_drm_devid(drm_fd);
-
-		drm_res = drmModeGetResources(drm_fd);
-		igt_require(drm_res);
-		igt_assert(drm_res->count_connectors <= MAX_CONNECTORS);
+	data_t data = {};
 
-		for (i = 0; i < drm_res->count_connectors; i++)
-			drm_connectors[i] = drmModeGetConnectorCurrent(drm_fd,
-							drm_res->connectors[i]);
+	igt_fixture {
 
+		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
+		igt_require(data.drm_fd >= 0);
+		data.debugfs_fd = igt_debugfs_dir(data.drm_fd);
+		igt_require(data.debugfs_fd >= 0);
  		igt_pm_enable_audio_runtime_pm();
-
-		igt_require(supports_lpsp(devid));
-
-		intel_register_access_init(&mmio_data, intel_get_pci_device(), 0, drm_fd);
-
  		kmstest_set_vt_graphics_mode();
+		data.devid = intel_get_drm_devid(data.drm_fd);
+		igt_display_require(&data.display, data.drm_fd);
+		igt_require(igt_pm_dmc_loaded(data.debugfs_fd));
  	}
 
+	igt_describe("This test validates lpsp while all crtc are disabled");
  	igt_subtest("screens-disabled")
-		screens_disabled_subtest(drm_fd, drm_res);
-	igt_subtest("edp-native")
-		edp_subtest(drm_fd, drm_res, drm_connectors, devid, false);
-	igt_subtest("non-edp")
-		non_edp_subtest(drm_fd, drm_res, drm_connectors);
+		screens_disabled_subtest(&data);
 
-	igt_fixture {
-		int i;
+	igt_describe("This test validates lpsp on all connected outputs on low power PIPE_A");
+	igt_subtest_with_dynamic_f("kms-lpsp") {
+		igt_display_t *display = &data.display;
+		igt_output_t *output;
 
-		intel_register_access_fini(&mmio_data);
-		for (i = 0; i < drm_res->count_connectors; i++)
-			drmModeFreeConnector(drm_connectors[i]);
-		drmModeFreeResources(drm_res);
-		close(drm_fd);
+		for_each_connected_output(display, output) {
+			igt_dynamic_f("kms-lpsp-%s", output->name) {
+				data.output = output;
+				test_lpsp(&data);
+			}
+

Quoted 85 lines · Anshuman Gupta

Current implementation of lpsp igt test, assumed that every non-edp panel isn't a lpsp panel but it is not true on TGL anymore, any HDMI/DP/DSI panel connected on pipe A and connected to PORT_{A,B,C} can drive LPSP.
Even on older Gen9 platform a DP panel can drive lpsp on Port A. This requires complete design change in current lpsp igt for a platform agnostic support.

The new igt approach is relies on connector specific i915_lpsp_capability and i915_lpsp_status debugfs attributes, these debugfs exposes whether an output is capable of driving lpsp and lpsp is enabled.

Nuking edp-native and non-edp test, introducing a new dynamic igt subtest kms-lpsp, which validates lpsp on each connected output and skip the subtest if output is not lpsp capable.

v2:
- CI failures fixup.
v3:
- removed unloading of snd_modules. [Martin]
v4:
- Don't test non-lpsp(if lpsp disabled), no ROI to test that.
- nuke panel-fitter test.
v5:
- Added dynamic subtest and igt changes according to kernel debugfs i915_lpsp_status changes.

Signed-off-by Anshuman Gupta <[email protected]>

---
 tests/i915/i915_pm_lpsp.c | 285 +++++++++++++++-----------------------
 1 file changed, 114 insertions(+), 171 deletions(-)

tests/i915/i915_pm_lpsp.c +24 -15

index 08f82e7c..7be23e6c 100644
@@ -25,210 +25,153 @@
  */
 #include "igt.h"
+#include "igt_kmod.h"
+#include "igt_pm.h"
+#include "igt_sysfs.h"
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <unistd.h>
+#define MAX_SINK_LPSP_INFO_BUF_LEN	4096
-static bool supports_lpsp(uint32_t devid)
-{
-	return IS_HASWELL(devid) || IS_BROADWELL(devid);
-}
+#define PWR_DOMAIN_INFO "i915_power_domain_info"
-static bool lpsp_is_enabled(int drm_fd)
+typedef struct {
+	int drm_fd;
+	int debugfs_fd;
+	uint32_t devid;
+	char *pwr_dmn_info;
+	igt_display_t display;
+	struct igt_fb fb;
+	drmModeModeInfo *mode;
+	igt_output_t *output;
+} data_t;
+
+static bool lpsp_is_enabled(data_t *data)
 {
-	uint32_t val;
+	char buf[MAX_SINK_LPSP_INFO_BUF_LEN];
+	int len;
-	val = INREG(HSW_PWR_WELL_CTL2);
-	return !(val & HSW_PWR_WELL_STATE_ENABLED);
-}
+	len = igt_debugfs_simple_read(data->debugfs_fd, "i915_lpsp_status",
+				      buf, sizeof(buf));
+	if (len < 0)
+		igt_assert_eq(len, -ENODEV);
-/* The LPSP mode is all about an enabled pipe, but we expect to also be in the
- * low power mode when no pipes are enabled, so do this check anyway. */
-static void screens_disabled_subtest(int drm_fd, drmModeResPtr drm_res)
-{
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-	igt_assert(lpsp_is_enabled(drm_fd));
+	return strstr(buf, "LPSP: enabled");
 }

tests/i915/i915_pm_lpsp.c line 67

Quoted 5 lines

tests/i915/i915_pm_lpsp.c +4 -1

-static uint32_t create_fb(int drm_fd, int width, int height)
+/*
+ * The LPSP mode is all about an enabled pipe, but we expect to also be in the
+ * low power mode when no pipes are enabled, so do this check anyway.
+ */

All pipe disabled in not same as lpsp. I feel the idea behind lpsp is single pipe usage with least power consumption ... there maybe no display connected.

Quoted 5 lines

tests/i915/i915_pm_lpsp.c +3 -1

+static void screens_disabled_subtest(data_t *data)
 {
-	struct igt_fb fb;
+	igt_output_t *output;
+	int valid_output = 0;

tests/i915/i915_pm_lpsp.c line 76

Quoted 5 lines

tests/i915/i915_pm_lpsp.c +5 -0

+	enum pipe pipe;
+
+	for_each_pipe_with_single_output(&data->display, pipe, output) {
+		data->output = output;
+		igt_output_set_pipe(data->output, PIPE_NONE);

Passing PIPE_NONE and testing lpsp maybe not the right thing here. We may not have connector but need to enable a single pipe to test lpsp...  rt?

Regards,

Animesh

Quoted 237 lines

tests/i915/i915_pm_lpsp.c +72 -154

+		igt_display_commit(&data->display);
+		valid_output++;
+	}
-	return igt_create_pattern_fb(drm_fd, width, height, DRM_FORMAT_XRGB8888,
-				     LOCAL_DRM_FORMAT_MOD_NONE, &fb);
+	igt_require_f(valid_output, "No connected output found\n");
+	igt_assert_f(lpsp_is_enabled(data), "lpsp is not enabled\n%s:\n%s\n",
+		     PWR_DOMAIN_INFO, data->pwr_dmn_info =
+		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
 }
-static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			drmModeConnectorPtr *drm_connectors, uint32_t devid,
-			bool use_panel_fitter)
+static void setup_lpsp_output(data_t *data)
 {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
-	drmModeModeInfo std_1024_mode = {
-		.clock = 65000,
-		.hdisplay = 1024,
-		.hsync_start = 1048,
-		.hsync_end = 1184,
-		.htotal = 1344,
-		.hskew = 0,
-		.vdisplay = 768,
-		.vsync_start = 771,
-		.vsync_end = 777,
-		.vtotal = 806,
-		.vscan = 0,
-		.vrefresh = 60,
-		.flags = 0xA,
-		.type = 0x40,
-		.name = "Custom 1024x768",
-	};
-
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-
-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
-
-		if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
-			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
-			continue;
-
-		if (!use_panel_fitter && c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
-		if (use_panel_fitter) {
-			connector = c;
-
-			/* This is one of the modes Xorg creates for panels, so
-			 * it should work just fine. Notice that Gens that
-			 * support LPSP are too new for panels with native
-			 * 1024x768 resolution, so this should force the panel
-			 * fitter. */
-			igt_assert(c->count_modes &&
-				   c->modes[0].hdisplay > 1024);
-			igt_assert(c->count_modes &&
-				   c->modes[0].vdisplay > 768);
-			mode = &std_1024_mode;
-			break;
-		}
-	}
-	igt_require(connector);
-
-	crtc_id = kmstest_find_crtc_for_connector(drm_fd, drm_res, connector,
-						  0);
-	buffer_id = create_fb(drm_fd, mode->hdisplay, mode->vdisplay);
-
-	igt_assert(buffer_id);
-	igt_assert(connector);
-	igt_assert(mode);
-
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
-
-	if (use_panel_fitter) {
-		if (IS_HASWELL(devid))
-			igt_assert(!lpsp_is_enabled(drm_fd));
-		else
-			igt_assert(lpsp_is_enabled(drm_fd));
-	} else {
-		igt_assert(lpsp_is_enabled(drm_fd));
-	}
+	igt_plane_t *primary;
+
+	/* set output pipe = PIPE_A for LPSP */
+	igt_output_set_pipe(data->output, PIPE_A);
+	primary = igt_output_get_plane_type(data->output,
+					    DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+	igt_create_pattern_fb(data->drm_fd,
+			      data->mode->hdisplay, data->mode->vdisplay,
+			      DRM_FORMAT_XRGB8888,
+			      LOCAL_DRM_FORMAT_MOD_NONE,
+			      &data->fb);
+	igt_plane_set_fb(primary, &data->fb);
+	igt_display_commit(&data->display);
 }
-static void non_edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			    drmModeConnectorPtr *drm_connectors)
+static void test_cleanup(data_t *data)
 {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
-
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-
-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
-
-		if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
-			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
-			continue;
-
-		if (c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
-	}
-	igt_require(connector);
-
-	crtc_id = kmstest_find_crtc_for_connector(drm_fd, drm_res, connector,
-						  0);
-	buffer_id = create_fb(drm_fd, mode->hdisplay, mode->vdisplay);
-
-	igt_assert(buffer_id);
-	igt_assert(mode);
-
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
-
-	igt_assert(!lpsp_is_enabled(drm_fd));
+	igt_plane_t *primary;
+
+	if (!data->output || data->output->pending_pipe == PIPE_NONE)
+		return;
+
+	primary = igt_output_get_plane_type(data->output,
+					    DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+	igt_output_set_pipe(data->output, PIPE_NONE);
+	igt_display_commit(&data->display);
+	igt_remove_fb(data->drm_fd, &data->fb);
+	data->output = NULL;
 }
-#define MAX_CONNECTORS 32
+static void test_lpsp(data_t *data)
+{
+	/* LPSP is low power single pipe usages i.e. PIPE_A */
+	igt_require(igt_pipe_connector_valid(PIPE_A, data->output));
+	igt_require_f(i915_output_is_lpsp_capable(data->drm_fd, data->output),
+		      "output is not lpsp capable\n");
+
+	data->mode = igt_output_get_mode(data->output);
+	setup_lpsp_output(data);
+	igt_assert_f(lpsp_is_enabled(data), "%s: lpsp is not enabled\n%s:\n%s\n",
+		     data->output->name, PWR_DOMAIN_INFO, data->pwr_dmn_info =
+		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
+}
-int drm_fd;
-uint32_t devid;
-drmModeResPtr drm_res;
-drmModeConnectorPtr drm_connectors[MAX_CONNECTORS];
-struct intel_mmio_data mmio_data;
+IGT_TEST_DESCRIPTION("These tests validates display Low Power Single Pipe configurations");
 igt_main
 {
-	igt_fixture {
-		int i;
-
-		drm_fd = drm_open_driver_master(DRIVER_INTEL);
-		igt_require(drm_fd >= 0);
-
-		devid = intel_get_drm_devid(drm_fd);
-
-		drm_res = drmModeGetResources(drm_fd);
-		igt_require(drm_res);
-		igt_assert(drm_res->count_connectors <= MAX_CONNECTORS);
+	data_t data = {};
-		for (i = 0; i < drm_res->count_connectors; i++)
-			drm_connectors[i] = drmModeGetConnectorCurrent(drm_fd,
-							drm_res->connectors[i]);
+	igt_fixture {
+		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
+		igt_require(data.drm_fd >= 0);
+		data.debugfs_fd = igt_debugfs_dir(data.drm_fd);
+		igt_require(data.debugfs_fd >= 0);
 		igt_pm_enable_audio_runtime_pm();
-
-		igt_require(supports_lpsp(devid));
-
-		intel_register_access_init(&mmio_data, intel_get_pci_device(), 0, drm_fd);
-
 		kmstest_set_vt_graphics_mode();
+		data.devid = intel_get_drm_devid(data.drm_fd);
+		igt_display_require(&data.display, data.drm_fd);
+		igt_require(igt_pm_dmc_loaded(data.debugfs_fd));
 	}
+	igt_describe("This test validates lpsp while all crtc are disabled");
 	igt_subtest("screens-disabled")
-		screens_disabled_subtest(drm_fd, drm_res);
-	igt_subtest("edp-native")
-		edp_subtest(drm_fd, drm_res, drm_connectors, devid, false);
-	igt_subtest("non-edp")
-		non_edp_subtest(drm_fd, drm_res, drm_connectors);
+		screens_disabled_subtest(&data);
-	igt_fixture {
-		int i;
+	igt_describe("This test validates lpsp on all connected outputs on low power PIPE_A");
+	igt_subtest_with_dynamic_f("kms-lpsp") {
+		igt_display_t *display = &data.display;
+		igt_output_t *output;
-		intel_register_access_fini(&mmio_data);
-		for (i = 0; i < drm_res->count_connectors; i++)
-			drmModeFreeConnector(drm_connectors[i]);
-		drmModeFreeResources(drm_res);
-		close(drm_fd);
+		for_each_connected_output(display, output) {
+			igt_dynamic_f("kms-lpsp-%s", output->name) {
+				data.output = output;
+				test_lpsp(&data);
+			}
+
+			test_cleanup(&data);
+		}

tests/i915/i915_pm_lpsp.c line 177

Quoted 8 lines

tests/i915/i915_pm_lpsp.c +6 -0

+	}
+
+	igt_fixture {
+		free(data.pwr_dmn_info);
+		close(data.drm_fd);
+		igt_display_fini(&data.display);
 	}
 }

Quoted 86 lines · Animesh

Quoted 85 lines · Anshuman Gupta

Current implementation of lpsp igt test, assumed that every non-edp panel isn't a lpsp panel but it is not true on TGL anymore, any HDMI/DP/DSI panel connected on pipe A and connected to PORT_{A,B,C} can drive LPSP.
Even on older Gen9 platform a DP panel can drive lpsp on Port A. This requires complete design change in current lpsp igt for a platform agnostic support.

The new igt approach is relies on connector specific i915_lpsp_capability and i915_lpsp_status debugfs attributes, these debugfs exposes whether an output is capable of driving lpsp and lpsp is enabled.

Nuking edp-native and non-edp test, introducing a new dynamic igt subtest kms-lpsp, which validates lpsp on each connected output and skip the subtest if output is not lpsp capable.

v2:
- CI failures fixup.
v3:
- removed unloading of snd_modules. [Martin]
v4:
- Don't test non-lpsp(if lpsp disabled), no ROI to test that.
- nuke panel-fitter test.
v5:
- Added dynamic subtest and igt changes according to kernel debugfs i915_lpsp_status changes.

Signed-off-by Anshuman Gupta <[email protected]>

---
  tests/i915/i915_pm_lpsp.c | 285 +++++++++++++++-----------------------
  1 file changed, 114 insertions(+), 171 deletions(-)

tests/i915/i915_pm_lpsp.c +24 -15

index 08f82e7c..7be23e6c 100644
@@ -25,210 +25,153 @@
   */
  #include "igt.h"
+#include "igt_kmod.h"
+#include "igt_pm.h"
+#include "igt_sysfs.h"
  #include <sys/types.h>
  #include <sys/stat.h>
  #include <fcntl.h>
  #include <unistd.h>
+#define MAX_SINK_LPSP_INFO_BUF_LEN	4096
-static bool supports_lpsp(uint32_t devid)
-{
-	return IS_HASWELL(devid) || IS_BROADWELL(devid);
-}
+#define PWR_DOMAIN_INFO "i915_power_domain_info"
-static bool lpsp_is_enabled(int drm_fd)
+typedef struct {
+	int drm_fd;
+	int debugfs_fd;
+	uint32_t devid;
+	char *pwr_dmn_info;
+	igt_display_t display;
+	struct igt_fb fb;
+	drmModeModeInfo *mode;
+	igt_output_t *output;
+} data_t;
+
+static bool lpsp_is_enabled(data_t *data)
  {
-	uint32_t val;
+	char buf[MAX_SINK_LPSP_INFO_BUF_LEN];
+	int len;
-	val = INREG(HSW_PWR_WELL_CTL2);
-	return !(val & HSW_PWR_WELL_STATE_ENABLED);
-}
+	len = igt_debugfs_simple_read(data->debugfs_fd, "i915_lpsp_status",
+				      buf, sizeof(buf));
+	if (len < 0)
+		igt_assert_eq(len, -ENODEV);
-/* The LPSP mode is all about an enabled pipe, but we expect to also be in the
- * low power mode when no pipes are enabled, so do this check anyway. */
-static void screens_disabled_subtest(int drm_fd, drmModeResPtr drm_res)
-{
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-	igt_assert(lpsp_is_enabled(drm_fd));
+	return strstr(buf, "LPSP: enabled");
  }

Quoted 8 lines

tests/i915/i915_pm_lpsp.c line 67

Quoted 5 lines

tests/i915/i915_pm_lpsp.c +4 -1

-static uint32_t create_fb(int drm_fd, int width, int height)
+/*
+ * The LPSP mode is all about an enabled pipe, but we expect to also be in the
+ * low power mode when no pipes are enabled, so do this check anyway.
+ */

All pipe disabled in not same as lpsp. I feel the idea behind lpsp is single pipe usage with least power consumption ... there maybe no display connected.

Yes you are correct that is the same above comment has mentioned that "LPSP mode is all about an enabled pipe", but here test method checks for lpsp when all screens are disbaled i.e. all crtc are disabled, that is almost same as there is no display connected with respect to power consumption.

Quoted 4 lines

Quoted 4 lines

tests/i915/i915_pm_lpsp.c +2 -1

+static void screens_disabled_subtest(data_t *data)
  {
-	struct igt_fb fb;
+	igt_output_t *output;

Quoted 8 lines

tests/i915/i915_pm_lpsp.c line 76

Quoted 6 lines

tests/i915/i915_pm_lpsp.c +6 -0

+	int valid_output = 0;
+	enum pipe pipe;
+
+	for_each_pipe_with_single_output(&data->display, pipe, output) {
+		data->output = output;
+		igt_output_set_pipe(data->output, PIPE_NONE);

Passing PIPE_NONE and testing lpsp maybe not the right thing here. We may not have connector but need to enable a single pipe to test lpsp...  rt?

PIPE_NONE commit will disbale a crtc, when all crtc gets disbaled, all power well except PG1 will be disbaled, PG1 is controlled by DMC.

So testing lpsp is here is one of the test case, i.e we want to test power well associated to PIPE_B should be turned off. There may be a bug in driver that when all crtc are disbaled PIPE_B associated power is not turned off, that is what this test validated. Enabling a pipe for lpsp will get covered in kms-lpsp test.
Thanks,
Anshuman Gupta.

Quoted 241 lines

Regards,

Animesh

Quoted 237 lines

tests/i915/i915_pm_lpsp.c +72 -154

+		igt_display_commit(&data->display);
+		valid_output++;
+	}
-	return igt_create_pattern_fb(drm_fd, width, height, DRM_FORMAT_XRGB8888,
-				     LOCAL_DRM_FORMAT_MOD_NONE, &fb);
+	igt_require_f(valid_output, "No connected output found\n");
+	igt_assert_f(lpsp_is_enabled(data), "lpsp is not enabled\n%s:\n%s\n",
+		     PWR_DOMAIN_INFO, data->pwr_dmn_info =
+		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
  }
-static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			drmModeConnectorPtr *drm_connectors, uint32_t devid,
-			bool use_panel_fitter)
+static void setup_lpsp_output(data_t *data)
  {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
-	drmModeModeInfo std_1024_mode = {
-		.clock = 65000,
-		.hdisplay = 1024,
-		.hsync_start = 1048,
-		.hsync_end = 1184,
-		.htotal = 1344,
-		.hskew = 0,
-		.vdisplay = 768,
-		.vsync_start = 771,
-		.vsync_end = 777,
-		.vtotal = 806,
-		.vscan = 0,
-		.vrefresh = 60,
-		.flags = 0xA,
-		.type = 0x40,
-		.name = "Custom 1024x768",
-	};
-
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-
-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
-
-		if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
-			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
-			continue;
-
-		if (!use_panel_fitter && c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
-		if (use_panel_fitter) {
-			connector = c;
-
-			/* This is one of the modes Xorg creates for panels, so
-			 * it should work just fine. Notice that Gens that
-			 * support LPSP are too new for panels with native
-			 * 1024x768 resolution, so this should force the panel
-			 * fitter. */
-			igt_assert(c->count_modes &&
-				   c->modes[0].hdisplay > 1024);
-			igt_assert(c->count_modes &&
-				   c->modes[0].vdisplay > 768);
-			mode = &std_1024_mode;
-			break;
-		}
-	}
-	igt_require(connector);
-
-	crtc_id = kmstest_find_crtc_for_connector(drm_fd, drm_res, connector,
-						  0);
-	buffer_id = create_fb(drm_fd, mode->hdisplay, mode->vdisplay);
-
-	igt_assert(buffer_id);
-	igt_assert(connector);
-	igt_assert(mode);
-
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
-
-	if (use_panel_fitter) {
-		if (IS_HASWELL(devid))
-			igt_assert(!lpsp_is_enabled(drm_fd));
-		else
-			igt_assert(lpsp_is_enabled(drm_fd));
-	} else {
-		igt_assert(lpsp_is_enabled(drm_fd));
-	}
+	igt_plane_t *primary;
+
+	/* set output pipe = PIPE_A for LPSP */
+	igt_output_set_pipe(data->output, PIPE_A);
+	primary = igt_output_get_plane_type(data->output,
+					    DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+	igt_create_pattern_fb(data->drm_fd,
+			      data->mode->hdisplay, data->mode->vdisplay,
+			      DRM_FORMAT_XRGB8888,
+			      LOCAL_DRM_FORMAT_MOD_NONE,
+			      &data->fb);
+	igt_plane_set_fb(primary, &data->fb);
+	igt_display_commit(&data->display);
  }
-static void non_edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			    drmModeConnectorPtr *drm_connectors)
+static void test_cleanup(data_t *data)
  {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
-
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-
-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
-
-		if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
-			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
-			continue;
-
-		if (c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
-	}
-	igt_require(connector);
-
-	crtc_id = kmstest_find_crtc_for_connector(drm_fd, drm_res, connector,
-						  0);
-	buffer_id = create_fb(drm_fd, mode->hdisplay, mode->vdisplay);
-
-	igt_assert(buffer_id);
-	igt_assert(mode);
-
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
-
-	igt_assert(!lpsp_is_enabled(drm_fd));
+	igt_plane_t *primary;
+
+	if (!data->output || data->output->pending_pipe == PIPE_NONE)
+		return;
+
+	primary = igt_output_get_plane_type(data->output,
+					    DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+	igt_output_set_pipe(data->output, PIPE_NONE);
+	igt_display_commit(&data->display);
+	igt_remove_fb(data->drm_fd, &data->fb);
+	data->output = NULL;
  }
-#define MAX_CONNECTORS 32
+static void test_lpsp(data_t *data)
+{
+	/* LPSP is low power single pipe usages i.e. PIPE_A */
+	igt_require(igt_pipe_connector_valid(PIPE_A, data->output));
+	igt_require_f(i915_output_is_lpsp_capable(data->drm_fd, data->output),
+		      "output is not lpsp capable\n");
+
+	data->mode = igt_output_get_mode(data->output);
+	setup_lpsp_output(data);
+	igt_assert_f(lpsp_is_enabled(data), "%s: lpsp is not enabled\n%s:\n%s\n",
+		     data->output->name, PWR_DOMAIN_INFO, data->pwr_dmn_info =
+		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
+}
-int drm_fd;
-uint32_t devid;
-drmModeResPtr drm_res;
-drmModeConnectorPtr drm_connectors[MAX_CONNECTORS];
-struct intel_mmio_data mmio_data;
+IGT_TEST_DESCRIPTION("These tests validates display Low Power Single Pipe configurations");
  igt_main
  {
-	igt_fixture {
-		int i;
-
-		drm_fd = drm_open_driver_master(DRIVER_INTEL);
-		igt_require(drm_fd >= 0);
-
-		devid = intel_get_drm_devid(drm_fd);
-
-		drm_res = drmModeGetResources(drm_fd);
-		igt_require(drm_res);
-		igt_assert(drm_res->count_connectors <= MAX_CONNECTORS);
+	data_t data = {};
-		for (i = 0; i < drm_res->count_connectors; i++)
-			drm_connectors[i] = drmModeGetConnectorCurrent(drm_fd,
-							drm_res->connectors[i]);
+	igt_fixture {
+		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
+		igt_require(data.drm_fd >= 0);
+		data.debugfs_fd = igt_debugfs_dir(data.drm_fd);
+		igt_require(data.debugfs_fd >= 0);
  		igt_pm_enable_audio_runtime_pm();
-
-		igt_require(supports_lpsp(devid));
-
-		intel_register_access_init(&mmio_data, intel_get_pci_device(), 0, drm_fd);
-
  		kmstest_set_vt_graphics_mode();
+		data.devid = intel_get_drm_devid(data.drm_fd);
+		igt_display_require(&data.display, data.drm_fd);
+		igt_require(igt_pm_dmc_loaded(data.debugfs_fd));
  	}
+	igt_describe("This test validates lpsp while all crtc are disabled");
  	igt_subtest("screens-disabled")
-		screens_disabled_subtest(drm_fd, drm_res);
-	igt_subtest("edp-native")
-		edp_subtest(drm_fd, drm_res, drm_connectors, devid, false);
-	igt_subtest("non-edp")
-		non_edp_subtest(drm_fd, drm_res, drm_connectors);
+		screens_disabled_subtest(&data);
-	igt_fixture {
-		int i;
+	igt_describe("This test validates lpsp on all connected outputs on low power PIPE_A");
+	igt_subtest_with_dynamic_f("kms-lpsp") {
+		igt_display_t *display = &data.display;
+		igt_output_t *output;
-		intel_register_access_fini(&mmio_data);
-		for (i = 0; i < drm_res->count_connectors; i++)
-			drmModeFreeConnector(drm_connectors[i]);
-		drmModeFreeResources(drm_res);
-		close(drm_fd);
+		for_each_connected_output(display, output) {
+			igt_dynamic_f("kms-lpsp-%s", output->name) {
+				data.output = output;
+				test_lpsp(&data);
+			}
+
+			test_cleanup(&data);
+		}

Quoted 8 lines

tests/i915/i915_pm_lpsp.c line 177

Quoted 8 lines

tests/i915/i915_pm_lpsp.c +6 -0

+	}
+
+	igt_fixture {
+		free(data.pwr_dmn_info);
+		close(data.drm_fd);
+		igt_display_fini(&data.display);
  	}
  }

Quoted 111 lines · Anshuman Gupta

Quoted 86 lines · Animesh

Quoted 85 lines · Anshuman Gupta

Current implementation of lpsp igt test, assumed that every non-edp panel isn't a lpsp panel but it is not true on TGL anymore, any HDMI/DP/DSI panel connected on pipe A and connected to PORT_{A,B,C} can drive LPSP.
Even on older Gen9 platform a DP panel can drive lpsp on Port A. This requires complete design change in current lpsp igt for a platform agnostic support.

The new igt approach is relies on connector specific i915_lpsp_capability and i915_lpsp_status debugfs attributes, these debugfs exposes whether an output is capable of driving lpsp and lpsp is enabled.

Nuking edp-native and non-edp test, introducing a new dynamic igt subtest kms-lpsp, which validates lpsp on each connected output and skip the subtest if output is not lpsp capable.

v2:
- CI failures fixup.
v3:
- removed unloading of snd_modules. [Martin]
v4:
- Don't test non-lpsp(if lpsp disabled), no ROI to test that.
- nuke panel-fitter test.
v5:
- Added dynamic subtest and igt changes according to kernel debugfs i915_lpsp_status changes.

Signed-off-by Anshuman Gupta <[email protected]>

---
 tests/i915/i915_pm_lpsp.c | 285 +++++++++++++++-----------------------
 1 file changed, 114 insertions(+), 171 deletions(-)

tests/i915/i915_pm_lpsp.c +24 -15

index 08f82e7c..7be23e6c 100644
@@ -25,210 +25,153 @@
  */
 #include "igt.h"
+#include "igt_kmod.h"
+#include "igt_pm.h"
+#include "igt_sysfs.h"
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <unistd.h>
+#define MAX_SINK_LPSP_INFO_BUF_LEN	4096
-static bool supports_lpsp(uint32_t devid)
-{
-	return IS_HASWELL(devid) || IS_BROADWELL(devid);
-}
+#define PWR_DOMAIN_INFO "i915_power_domain_info"
-static bool lpsp_is_enabled(int drm_fd)
+typedef struct {
+	int drm_fd;
+	int debugfs_fd;
+	uint32_t devid;
+	char *pwr_dmn_info;
+	igt_display_t display;
+	struct igt_fb fb;
+	drmModeModeInfo *mode;
+	igt_output_t *output;
+} data_t;
+
+static bool lpsp_is_enabled(data_t *data)
 {
-	uint32_t val;
+	char buf[MAX_SINK_LPSP_INFO_BUF_LEN];
+	int len;
-	val = INREG(HSW_PWR_WELL_CTL2);
-	return !(val & HSW_PWR_WELL_STATE_ENABLED);
-}
+	len = igt_debugfs_simple_read(data->debugfs_fd, "i915_lpsp_status",
+				      buf, sizeof(buf));
+	if (len < 0)
+		igt_assert_eq(len, -ENODEV);
-/* The LPSP mode is all about an enabled pipe, but we expect to also be in the
- * low power mode when no pipes are enabled, so do this check anyway. */
-static void screens_disabled_subtest(int drm_fd, drmModeResPtr drm_res)
-{
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-	igt_assert(lpsp_is_enabled(drm_fd));
+	return strstr(buf, "LPSP: enabled");
 }

Quoted 8 lines

tests/i915/i915_pm_lpsp.c line 67

Quoted 5 lines

tests/i915/i915_pm_lpsp.c +4 -1

-static uint32_t create_fb(int drm_fd, int width, int height)
+/*
+ * The LPSP mode is all about an enabled pipe, but we expect to also be in the
+ * low power mode when no pipes are enabled, so do this check anyway.
+ */

All pipe disabled in not same as lpsp. I feel the idea behind lpsp is single pipe usage with least power consumption ... there maybe no display connected.

Yes you are correct that is the same above comment has mentioned that "LPSP mode is all about an enabled pipe", but here test method checks for lpsp when all screens are disbaled i.e. all crtc are disabled, that is almost same as there is no display connected with respect to power consumption.

Quoted 12 lines

tests/i915/i915_pm_lpsp.c line 76

Quoted 10 lines

tests/i915/i915_pm_lpsp.c +8 -1

+static void screens_disabled_subtest(data_t *data)
 {
-	struct igt_fb fb;
+	igt_output_t *output;
+	int valid_output = 0;
+	enum pipe pipe;
+
+	for_each_pipe_with_single_output(&data->display, pipe, output) {
+		data->output = output;
+		igt_output_set_pipe(data->output, PIPE_NONE);

Passing PIPE_NONE and testing lpsp maybe not the right thing here. We may not have connector but need to enable a single pipe to test lpsp...  rt?

Quoted 2 lines

PIPE_NONE commit will disbale a crtc, when all crtc gets disbaled, all power well except PG1 will be disbaled, PG1 is controlled by DMC.

I do not think DC9 is controlled by DMC, during all crtc disable driver will disable all power-well and enter into DC9, no role of DMC here ... am I missing anything?

Regards,
Animesh

Quoted 247 lines

So testing lpsp is here is one of the test case, i.e we want to test power well associated to PIPE_B should be turned off. There may be a bug in driver that when all crtc are disbaled PIPE_B associated power is not turned off, that is what this test validated. Enabling a pipe for lpsp will get covered in kms-lpsp test.
Thanks,
Anshuman Gupta.

Quoted 241 lines

Regards,

Animesh

Quoted 237 lines

tests/i915/i915_pm_lpsp.c +72 -154

+		igt_display_commit(&data->display);
+		valid_output++;
+	}
-	return igt_create_pattern_fb(drm_fd, width, height, DRM_FORMAT_XRGB8888,
-				     LOCAL_DRM_FORMAT_MOD_NONE, &fb);
+	igt_require_f(valid_output, "No connected output found\n");
+	igt_assert_f(lpsp_is_enabled(data), "lpsp is not enabled\n%s:\n%s\n",
+		     PWR_DOMAIN_INFO, data->pwr_dmn_info =
+		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
 }
-static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			drmModeConnectorPtr *drm_connectors, uint32_t devid,
-			bool use_panel_fitter)
+static void setup_lpsp_output(data_t *data)
 {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
-	drmModeModeInfo std_1024_mode = {
-		.clock = 65000,
-		.hdisplay = 1024,
-		.hsync_start = 1048,
-		.hsync_end = 1184,
-		.htotal = 1344,
-		.hskew = 0,
-		.vdisplay = 768,
-		.vsync_start = 771,
-		.vsync_end = 777,
-		.vtotal = 806,
-		.vscan = 0,
-		.vrefresh = 60,
-		.flags = 0xA,
-		.type = 0x40,
-		.name = "Custom 1024x768",
-	};
-
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-
-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
-
-		if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
-			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
-			continue;
-
-		if (!use_panel_fitter && c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
-		if (use_panel_fitter) {
-			connector = c;
-
-			/* This is one of the modes Xorg creates for panels, so
-			 * it should work just fine. Notice that Gens that
-			 * support LPSP are too new for panels with native
-			 * 1024x768 resolution, so this should force the panel
-			 * fitter. */
-			igt_assert(c->count_modes &&
-				   c->modes[0].hdisplay > 1024);
-			igt_assert(c->count_modes &&
-				   c->modes[0].vdisplay > 768);
-			mode = &std_1024_mode;
-			break;
-		}
-	}
-	igt_require(connector);
-
-	crtc_id = kmstest_find_crtc_for_connector(drm_fd, drm_res, connector,
-						  0);
-	buffer_id = create_fb(drm_fd, mode->hdisplay, mode->vdisplay);
-
-	igt_assert(buffer_id);
-	igt_assert(connector);
-	igt_assert(mode);
-
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
-
-	if (use_panel_fitter) {
-		if (IS_HASWELL(devid))
-			igt_assert(!lpsp_is_enabled(drm_fd));
-		else
-			igt_assert(lpsp_is_enabled(drm_fd));
-	} else {
-		igt_assert(lpsp_is_enabled(drm_fd));
-	}
+	igt_plane_t *primary;
+
+	/* set output pipe = PIPE_A for LPSP */
+	igt_output_set_pipe(data->output, PIPE_A);
+	primary = igt_output_get_plane_type(data->output,
+					    DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+	igt_create_pattern_fb(data->drm_fd,
+			      data->mode->hdisplay, data->mode->vdisplay,
+			      DRM_FORMAT_XRGB8888,
+			      LOCAL_DRM_FORMAT_MOD_NONE,
+			      &data->fb);
+	igt_plane_set_fb(primary, &data->fb);
+	igt_display_commit(&data->display);
 }
-static void non_edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			    drmModeConnectorPtr *drm_connectors)
+static void test_cleanup(data_t *data)
 {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
-
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-
-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
-
-		if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
-			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
-			continue;
-
-		if (c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
-	}
-	igt_require(connector);
-
-	crtc_id = kmstest_find_crtc_for_connector(drm_fd, drm_res, connector,
-						  0);
-	buffer_id = create_fb(drm_fd, mode->hdisplay, mode->vdisplay);
-
-	igt_assert(buffer_id);
-	igt_assert(mode);
-
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
-
-	igt_assert(!lpsp_is_enabled(drm_fd));
+	igt_plane_t *primary;
+
+	if (!data->output || data->output->pending_pipe == PIPE_NONE)
+		return;
+
+	primary = igt_output_get_plane_type(data->output,
+					    DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+	igt_output_set_pipe(data->output, PIPE_NONE);
+	igt_display_commit(&data->display);
+	igt_remove_fb(data->drm_fd, &data->fb);
+	data->output = NULL;
 }
-#define MAX_CONNECTORS 32
+static void test_lpsp(data_t *data)
+{
+	/* LPSP is low power single pipe usages i.e. PIPE_A */
+	igt_require(igt_pipe_connector_valid(PIPE_A, data->output));
+	igt_require_f(i915_output_is_lpsp_capable(data->drm_fd, data->output),
+		      "output is not lpsp capable\n");
+
+	data->mode = igt_output_get_mode(data->output);
+	setup_lpsp_output(data);
+	igt_assert_f(lpsp_is_enabled(data), "%s: lpsp is not enabled\n%s:\n%s\n",
+		     data->output->name, PWR_DOMAIN_INFO, data->pwr_dmn_info =
+		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
+}
-int drm_fd;
-uint32_t devid;
-drmModeResPtr drm_res;
-drmModeConnectorPtr drm_connectors[MAX_CONNECTORS];
-struct intel_mmio_data mmio_data;
+IGT_TEST_DESCRIPTION("These tests validates display Low Power Single Pipe configurations");
 igt_main
 {
-	igt_fixture {
-		int i;
-
-		drm_fd = drm_open_driver_master(DRIVER_INTEL);
-		igt_require(drm_fd >= 0);
-
-		devid = intel_get_drm_devid(drm_fd);
-
-		drm_res = drmModeGetResources(drm_fd);
-		igt_require(drm_res);
-		igt_assert(drm_res->count_connectors <= MAX_CONNECTORS);
+	data_t data = {};
-		for (i = 0; i < drm_res->count_connectors; i++)
-			drm_connectors[i] = drmModeGetConnectorCurrent(drm_fd,
-							drm_res->connectors[i]);
+	igt_fixture {
+		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
+		igt_require(data.drm_fd >= 0);
+		data.debugfs_fd = igt_debugfs_dir(data.drm_fd);
+		igt_require(data.debugfs_fd >= 0);
 		igt_pm_enable_audio_runtime_pm();
-
-		igt_require(supports_lpsp(devid));
-
-		intel_register_access_init(&mmio_data, intel_get_pci_device(), 0, drm_fd);
-
 		kmstest_set_vt_graphics_mode();
+		data.devid = intel_get_drm_devid(data.drm_fd);
+		igt_display_require(&data.display, data.drm_fd);
+		igt_require(igt_pm_dmc_loaded(data.debugfs_fd));
 	}
+	igt_describe("This test validates lpsp while all crtc are disabled");
 	igt_subtest("screens-disabled")
-		screens_disabled_subtest(drm_fd, drm_res);
-	igt_subtest("edp-native")
-		edp_subtest(drm_fd, drm_res, drm_connectors, devid, false);
-	igt_subtest("non-edp")
-		non_edp_subtest(drm_fd, drm_res, drm_connectors);
+		screens_disabled_subtest(&data);
-	igt_fixture {
-		int i;
+	igt_describe("This test validates lpsp on all connected outputs on low power PIPE_A");
+	igt_subtest_with_dynamic_f("kms-lpsp") {
+		igt_display_t *display = &data.display;
+		igt_output_t *output;
-		intel_register_access_fini(&mmio_data);
-		for (i = 0; i < drm_res->count_connectors; i++)
-			drmModeFreeConnector(drm_connectors[i]);
-		drmModeFreeResources(drm_res);
-		close(drm_fd);
+		for_each_connected_output(display, output) {
+			igt_dynamic_f("kms-lpsp-%s", output->name) {
+				data.output = output;
+				test_lpsp(&data);
+			}
+
+			test_cleanup(&data);
+		}

Quoted 8 lines

Quoted 8 lines

tests/i915/i915_pm_lpsp.c line 177

Quoted 8 lines

tests/i915/i915_pm_lpsp.c +6 -0

+	}
+
+	igt_fixture {
+		free(data.pwr_dmn_info);
+		close(data.drm_fd);
+		igt_display_fini(&data.display);
 	}
 }

Quoted 112 lines · Animesh

Quoted 111 lines · Anshuman Gupta

Quoted 86 lines · Animesh

Quoted 85 lines · Anshuman Gupta

Current implementation of lpsp igt test, assumed that every non-edp panel isn't a lpsp panel but it is not true on TGL anymore, any HDMI/DP/DSI panel connected on pipe A and connected to PORT_{A,B,C} can drive LPSP.
Even on older Gen9 platform a DP panel can drive lpsp on Port A. This requires complete design change in current lpsp igt for a platform agnostic support.

The new igt approach is relies on connector specific i915_lpsp_capability and i915_lpsp_status debugfs attributes, these debugfs exposes whether an output is capable of driving lpsp and lpsp is enabled.

Nuking edp-native and non-edp test, introducing a new dynamic igt subtest kms-lpsp, which validates lpsp on each connected output and skip the subtest if output is not lpsp capable.

v2:
- CI failures fixup.
v3:
- removed unloading of snd_modules. [Martin]
v4:
- Don't test non-lpsp(if lpsp disabled), no ROI to test that.
- nuke panel-fitter test.
v5:
- Added dynamic subtest and igt changes according to kernel debugfs i915_lpsp_status changes.

Signed-off-by Anshuman Gupta <[email protected]>

---
  tests/i915/i915_pm_lpsp.c | 285 +++++++++++++++-----------------------
  1 file changed, 114 insertions(+), 171 deletions(-)

tests/i915/i915_pm_lpsp.c +24 -15

index 08f82e7c..7be23e6c 100644
@@ -25,210 +25,153 @@
   */
  #include "igt.h"
+#include "igt_kmod.h"
+#include "igt_pm.h"
+#include "igt_sysfs.h"
  #include <sys/types.h>
  #include <sys/stat.h>
  #include <fcntl.h>
  #include <unistd.h>
+#define MAX_SINK_LPSP_INFO_BUF_LEN	4096
-static bool supports_lpsp(uint32_t devid)
-{
-	return IS_HASWELL(devid) || IS_BROADWELL(devid);
-}
+#define PWR_DOMAIN_INFO "i915_power_domain_info"
-static bool lpsp_is_enabled(int drm_fd)
+typedef struct {
+	int drm_fd;
+	int debugfs_fd;
+	uint32_t devid;
+	char *pwr_dmn_info;
+	igt_display_t display;
+	struct igt_fb fb;
+	drmModeModeInfo *mode;
+	igt_output_t *output;
+} data_t;
+
+static bool lpsp_is_enabled(data_t *data)
  {
-	uint32_t val;
+	char buf[MAX_SINK_LPSP_INFO_BUF_LEN];
+	int len;
-	val = INREG(HSW_PWR_WELL_CTL2);
-	return !(val & HSW_PWR_WELL_STATE_ENABLED);
-}
+	len = igt_debugfs_simple_read(data->debugfs_fd, "i915_lpsp_status",
+				      buf, sizeof(buf));
+	if (len < 0)
+		igt_assert_eq(len, -ENODEV);
-/* The LPSP mode is all about an enabled pipe, but we expect to also be in the
- * low power mode when no pipes are enabled, so do this check anyway. */
-static void screens_disabled_subtest(int drm_fd, drmModeResPtr drm_res)
-{
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-	igt_assert(lpsp_is_enabled(drm_fd));
+	return strstr(buf, "LPSP: enabled");
  }

Quoted 8 lines

tests/i915/i915_pm_lpsp.c line 67

Quoted 5 lines

tests/i915/i915_pm_lpsp.c +4 -1

-static uint32_t create_fb(int drm_fd, int width, int height)
+/*
+ * The LPSP mode is all about an enabled pipe, but we expect to also be in the
+ * low power mode when no pipes are enabled, so do this check anyway.
+ */

All pipe disabled in not same as lpsp. I feel the idea behind lpsp is single pipe usage with least power consumption ... there maybe no display connected.

Yes you are correct that is the same above comment has mentioned that "LPSP mode is all about an enabled pipe", but here test method checks for lpsp when all screens are disbaled i.e. all crtc are disabled, that is almost same as there is no display connected with respect to power consumption.

Quoted 12 lines

tests/i915/i915_pm_lpsp.c line 76

Quoted 10 lines

tests/i915/i915_pm_lpsp.c +8 -1

+static void screens_disabled_subtest(data_t *data)
  {
-	struct igt_fb fb;
+	igt_output_t *output;
+	int valid_output = 0;
+	enum pipe pipe;
+
+	for_each_pipe_with_single_output(&data->display, pipe, output) {
+		data->output = output;
+		igt_output_set_pipe(data->output, PIPE_NONE);

Passing PIPE_NONE and testing lpsp maybe not the right thing here. We may not have connector but need to enable a single pipe to test lpsp...  rt?

Quoted 3 lines

Quoted 2 lines

PIPE_NONE commit will disbale a crtc, when all crtc gets disbaled, all power well except PG1 will be disbaled, PG1 is controlled by DMC.

I do not think DC9 is controlled by DMC, during all crtc disable driver will disable all power-well and enter into DC9, no role of DMC here ... am I missing anything?

yes DC9 is not controlled by DMC, and i915 program DC9 in case of runtime suspend or system wide suspend, so DC9 a deeper state depends on i915 suspend, let's think about a use case when all CRTC are disabled but gem has valid run time usages count or let's say i915 device is always on i.e (/sys/bus/pci/devices/0000\:00\:02.0/power/control attribute in on), in those cases runtime status will be always active, so i think test method to validate PIPE_B associated power well is valid here when all crtc are disbaled i.e. all display are off.

We want validate a intermediate power state here, which is a valid use case. Please correct me if my thought process about test method is wrong. Thanks ,
Anshuman Gupta.

Quoted 250 lines

Regards,
Animesh

Quoted 247 lines

So testing lpsp is here is one of the test case, i.e we want to test power well associated to PIPE_B should be turned off. There may be a bug in driver that when all crtc are disbaled PIPE_B associated power is not turned off, that is what this test validated. Enabling a pipe for lpsp will get covered in kms-lpsp test.
Thanks,
Anshuman Gupta.

Quoted 241 lines

Regards,

Animesh

Quoted 237 lines

tests/i915/i915_pm_lpsp.c +72 -154

+		igt_display_commit(&data->display);
+		valid_output++;
+	}
-	return igt_create_pattern_fb(drm_fd, width, height, DRM_FORMAT_XRGB8888,
-				     LOCAL_DRM_FORMAT_MOD_NONE, &fb);
+	igt_require_f(valid_output, "No connected output found\n");
+	igt_assert_f(lpsp_is_enabled(data), "lpsp is not enabled\n%s:\n%s\n",
+		     PWR_DOMAIN_INFO, data->pwr_dmn_info =
+		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
  }
-static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			drmModeConnectorPtr *drm_connectors, uint32_t devid,
-			bool use_panel_fitter)
+static void setup_lpsp_output(data_t *data)
  {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
-	drmModeModeInfo std_1024_mode = {
-		.clock = 65000,
-		.hdisplay = 1024,
-		.hsync_start = 1048,
-		.hsync_end = 1184,
-		.htotal = 1344,
-		.hskew = 0,
-		.vdisplay = 768,
-		.vsync_start = 771,
-		.vsync_end = 777,
-		.vtotal = 806,
-		.vscan = 0,
-		.vrefresh = 60,
-		.flags = 0xA,
-		.type = 0x40,
-		.name = "Custom 1024x768",
-	};
-
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-
-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
-
-		if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
-			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
-			continue;
-
-		if (!use_panel_fitter && c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
-		if (use_panel_fitter) {
-			connector = c;
-
-			/* This is one of the modes Xorg creates for panels, so
-			 * it should work just fine. Notice that Gens that
-			 * support LPSP are too new for panels with native
-			 * 1024x768 resolution, so this should force the panel
-			 * fitter. */
-			igt_assert(c->count_modes &&
-				   c->modes[0].hdisplay > 1024);
-			igt_assert(c->count_modes &&
-				   c->modes[0].vdisplay > 768);
-			mode = &std_1024_mode;
-			break;
-		}
-	}
-	igt_require(connector);
-
-	crtc_id = kmstest_find_crtc_for_connector(drm_fd, drm_res, connector,
-						  0);
-	buffer_id = create_fb(drm_fd, mode->hdisplay, mode->vdisplay);
-
-	igt_assert(buffer_id);
-	igt_assert(connector);
-	igt_assert(mode);
-
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
-
-	if (use_panel_fitter) {
-		if (IS_HASWELL(devid))
-			igt_assert(!lpsp_is_enabled(drm_fd));
-		else
-			igt_assert(lpsp_is_enabled(drm_fd));
-	} else {
-		igt_assert(lpsp_is_enabled(drm_fd));
-	}
+	igt_plane_t *primary;
+
+	/* set output pipe = PIPE_A for LPSP */
+	igt_output_set_pipe(data->output, PIPE_A);
+	primary = igt_output_get_plane_type(data->output,
+					    DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+	igt_create_pattern_fb(data->drm_fd,
+			      data->mode->hdisplay, data->mode->vdisplay,
+			      DRM_FORMAT_XRGB8888,
+			      LOCAL_DRM_FORMAT_MOD_NONE,
+			      &data->fb);
+	igt_plane_set_fb(primary, &data->fb);
+	igt_display_commit(&data->display);
  }
-static void non_edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			    drmModeConnectorPtr *drm_connectors)
+static void test_cleanup(data_t *data)
  {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
-
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-
-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
-
-		if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
-			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
-			continue;
-
-		if (c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
-	}
-	igt_require(connector);
-
-	crtc_id = kmstest_find_crtc_for_connector(drm_fd, drm_res, connector,
-						  0);
-	buffer_id = create_fb(drm_fd, mode->hdisplay, mode->vdisplay);
-
-	igt_assert(buffer_id);
-	igt_assert(mode);
-
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
-
-	igt_assert(!lpsp_is_enabled(drm_fd));
+	igt_plane_t *primary;
+
+	if (!data->output || data->output->pending_pipe == PIPE_NONE)
+		return;
+
+	primary = igt_output_get_plane_type(data->output,
+					    DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+	igt_output_set_pipe(data->output, PIPE_NONE);
+	igt_display_commit(&data->display);
+	igt_remove_fb(data->drm_fd, &data->fb);
+	data->output = NULL;
  }
-#define MAX_CONNECTORS 32
+static void test_lpsp(data_t *data)
+{
+	/* LPSP is low power single pipe usages i.e. PIPE_A */
+	igt_require(igt_pipe_connector_valid(PIPE_A, data->output));
+	igt_require_f(i915_output_is_lpsp_capable(data->drm_fd, data->output),
+		      "output is not lpsp capable\n");
+
+	data->mode = igt_output_get_mode(data->output);
+	setup_lpsp_output(data);
+	igt_assert_f(lpsp_is_enabled(data), "%s: lpsp is not enabled\n%s:\n%s\n",
+		     data->output->name, PWR_DOMAIN_INFO, data->pwr_dmn_info =
+		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
+}
-int drm_fd;
-uint32_t devid;
-drmModeResPtr drm_res;
-drmModeConnectorPtr drm_connectors[MAX_CONNECTORS];
-struct intel_mmio_data mmio_data;
+IGT_TEST_DESCRIPTION("These tests validates display Low Power Single Pipe configurations");
  igt_main
  {
-	igt_fixture {
-		int i;
-
-		drm_fd = drm_open_driver_master(DRIVER_INTEL);
-		igt_require(drm_fd >= 0);
-
-		devid = intel_get_drm_devid(drm_fd);
-
-		drm_res = drmModeGetResources(drm_fd);
-		igt_require(drm_res);
-		igt_assert(drm_res->count_connectors <= MAX_CONNECTORS);
+	data_t data = {};
-		for (i = 0; i < drm_res->count_connectors; i++)
-			drm_connectors[i] = drmModeGetConnectorCurrent(drm_fd,
-							drm_res->connectors[i]);
+	igt_fixture {
+		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
+		igt_require(data.drm_fd >= 0);
+		data.debugfs_fd = igt_debugfs_dir(data.drm_fd);
+		igt_require(data.debugfs_fd >= 0);
  		igt_pm_enable_audio_runtime_pm();
-
-		igt_require(supports_lpsp(devid));
-
-		intel_register_access_init(&mmio_data, intel_get_pci_device(), 0, drm_fd);
-
  		kmstest_set_vt_graphics_mode();
+		data.devid = intel_get_drm_devid(data.drm_fd);
+		igt_display_require(&data.display, data.drm_fd);
+		igt_require(igt_pm_dmc_loaded(data.debugfs_fd));
  	}
+	igt_describe("This test validates lpsp while all crtc are disabled");
  	igt_subtest("screens-disabled")
-		screens_disabled_subtest(drm_fd, drm_res);
-	igt_subtest("edp-native")
-		edp_subtest(drm_fd, drm_res, drm_connectors, devid, false);
-	igt_subtest("non-edp")
-		non_edp_subtest(drm_fd, drm_res, drm_connectors);
+		screens_disabled_subtest(&data);
-	igt_fixture {
-		int i;
+	igt_describe("This test validates lpsp on all connected outputs on low power PIPE_A");
+	igt_subtest_with_dynamic_f("kms-lpsp") {
+		igt_display_t *display = &data.display;
+		igt_output_t *output;
-		intel_register_access_fini(&mmio_data);
-		for (i = 0; i < drm_res->count_connectors; i++)
-			drmModeFreeConnector(drm_connectors[i]);
-		drmModeFreeResources(drm_res);
-		close(drm_fd);
+		for_each_connected_output(display, output) {
+			igt_dynamic_f("kms-lpsp-%s", output->name) {
+				data.output = output;
+				test_lpsp(&data);
+			}
+
+			test_cleanup(&data);
+		}

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

tests/i915/i915_pm_lpsp.c line 177

Quoted 8 lines

tests/i915/i915_pm_lpsp.c +6 -0

+	}
+
+	igt_fixture {
+		free(data.pwr_dmn_info);
+		close(data.drm_fd);
+		igt_display_fini(&data.display);
  	}
  }

Quoted 113 lines · Anshuman Gupta

Quoted 112 lines · Animesh

Quoted 111 lines · Anshuman Gupta

Quoted 86 lines · Animesh

Quoted 85 lines · Anshuman Gupta

Current implementation of lpsp igt test, assumed that every non-edp panel isn't a lpsp panel but it is not true on TGL anymore, any HDMI/DP/DSI panel connected on pipe A and connected to PORT_{A,B,C} can drive LPSP.
Even on older Gen9 platform a DP panel can drive lpsp on Port A. This requires complete design change in current lpsp igt for a platform agnostic support.

The new igt approach is relies on connector specific i915_lpsp_capability and i915_lpsp_status debugfs attributes, these debugfs exposes whether an output is capable of driving lpsp and lpsp is enabled.

Nuking edp-native and non-edp test, introducing a new dynamic igt subtest kms-lpsp, which validates lpsp on each connected output and skip the subtest if output is not lpsp capable.

v2:
- CI failures fixup.
v3:
- removed unloading of snd_modules. [Martin]
v4:
- Don't test non-lpsp(if lpsp disabled), no ROI to test that.
- nuke panel-fitter test.
v5:
- Added dynamic subtest and igt changes according to kernel debugfs i915_lpsp_status changes.

Signed-off-by Anshuman Gupta <[email protected]>

---
 tests/i915/i915_pm_lpsp.c | 285 +++++++++++++++-----------------------
 1 file changed, 114 insertions(+), 171 deletions(-)

tests/i915/i915_pm_lpsp.c +24 -15

index 08f82e7c..7be23e6c 100644
@@ -25,210 +25,153 @@
  */
 #include "igt.h"
+#include "igt_kmod.h"
+#include "igt_pm.h"
+#include "igt_sysfs.h"
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <unistd.h>
+#define MAX_SINK_LPSP_INFO_BUF_LEN	4096
-static bool supports_lpsp(uint32_t devid)
-{
-	return IS_HASWELL(devid) || IS_BROADWELL(devid);
-}
+#define PWR_DOMAIN_INFO "i915_power_domain_info"
-static bool lpsp_is_enabled(int drm_fd)
+typedef struct {
+	int drm_fd;
+	int debugfs_fd;
+	uint32_t devid;
+	char *pwr_dmn_info;
+	igt_display_t display;
+	struct igt_fb fb;
+	drmModeModeInfo *mode;
+	igt_output_t *output;
+} data_t;
+
+static bool lpsp_is_enabled(data_t *data)
 {
-	uint32_t val;
+	char buf[MAX_SINK_LPSP_INFO_BUF_LEN];
+	int len;
-	val = INREG(HSW_PWR_WELL_CTL2);
-	return !(val & HSW_PWR_WELL_STATE_ENABLED);
-}
+	len = igt_debugfs_simple_read(data->debugfs_fd, "i915_lpsp_status",
+				      buf, sizeof(buf));
+	if (len < 0)
+		igt_assert_eq(len, -ENODEV);
-/* The LPSP mode is all about an enabled pipe, but we expect to also be in the
- * low power mode when no pipes are enabled, so do this check anyway. */
-static void screens_disabled_subtest(int drm_fd, drmModeResPtr drm_res)
-{
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-	igt_assert(lpsp_is_enabled(drm_fd));
+	return strstr(buf, "LPSP: enabled");
 }

Quoted 8 lines

tests/i915/i915_pm_lpsp.c line 67

Quoted 5 lines

tests/i915/i915_pm_lpsp.c +4 -1

-static uint32_t create_fb(int drm_fd, int width, int height)
+/*
+ * The LPSP mode is all about an enabled pipe, but we expect to also be in the
+ * low power mode when no pipes are enabled, so do this check anyway.
+ */

All pipe disabled in not same as lpsp. I feel the idea behind lpsp is single pipe usage with least power consumption ... there maybe no display connected.

Yes you are correct that is the same above comment has mentioned that "LPSP mode is all about an enabled pipe", but here test method checks for lpsp when all screens are disbaled i.e. all crtc are disabled, that is almost same as there is no display connected with respect to power consumption.

Quoted 12 lines

tests/i915/i915_pm_lpsp.c line 76

Quoted 10 lines

tests/i915/i915_pm_lpsp.c +8 -1

+static void screens_disabled_subtest(data_t *data)
 {
-	struct igt_fb fb;
+	igt_output_t *output;
+	int valid_output = 0;
+	enum pipe pipe;
+
+	for_each_pipe_with_single_output(&data->display, pipe, output) {
+		data->output = output;
+		igt_output_set_pipe(data->output, PIPE_NONE);

Passing PIPE_NONE and testing lpsp maybe not the right thing here. We may not have connector but need to enable a single pipe to test lpsp...  rt?

Quoted 4 lines

Quoted 3 lines

Quoted 2 lines

PIPE_NONE commit will disbale a crtc, when all crtc gets disbaled, all power well except PG1 will be disbaled, PG1 is controlled by DMC.

I do not think DC9 is controlled by DMC, during all crtc disable driver will disable all power-well and enter into DC9, no role of DMC here ... am I missing anything?

yes DC9 is not controlled by DMC, and i915 program DC9 in case of runtime suspend or system wide suspend, so DC9 a deeper state depends on i915 suspend, let's think about a use case when all CRTC are disabled but gem has valid run time usages count or let's say i915 device is always on i.e (/sys/bus/pci/devices/0000\:00\:02.0/power/control attribute in on), in those cases runtime status will be always active, so i think test method to validate PIPE_B associated power well is valid here when all crtc are disbaled i.e. all display are off.

As per the above example though DC9 is not triggered but all power-well will be disabled by driver as it is display engine specific. This is not lpsp.

Here valid testcase would be to check PG1 is enabled or not.

Regards,
Animesh

Quoted 254 lines

We want validate a intermediate power state here, which is a valid use case. Please correct me if my thought process about test method is wrong. Thanks ,
Anshuman Gupta.

Quoted 250 lines

Regards,
Animesh

Quoted 247 lines

So testing lpsp is here is one of the test case, i.e we want to test power well associated to PIPE_B should be turned off. There may be a bug in driver that when all crtc are disbaled PIPE_B associated power is not turned off, that is what this test validated. Enabling a pipe for lpsp will get covered in kms-lpsp test.
Thanks,
Anshuman Gupta.

Quoted 241 lines

Regards,

Animesh

Quoted 237 lines

tests/i915/i915_pm_lpsp.c +72 -154

+		igt_display_commit(&data->display);
+		valid_output++;
+	}
-	return igt_create_pattern_fb(drm_fd, width, height, DRM_FORMAT_XRGB8888,
-				     LOCAL_DRM_FORMAT_MOD_NONE, &fb);
+	igt_require_f(valid_output, "No connected output found\n");
+	igt_assert_f(lpsp_is_enabled(data), "lpsp is not enabled\n%s:\n%s\n",
+		     PWR_DOMAIN_INFO, data->pwr_dmn_info =
+		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
 }
-static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			drmModeConnectorPtr *drm_connectors, uint32_t devid,
-			bool use_panel_fitter)
+static void setup_lpsp_output(data_t *data)
 {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
-	drmModeModeInfo std_1024_mode = {
-		.clock = 65000,
-		.hdisplay = 1024,
-		.hsync_start = 1048,
-		.hsync_end = 1184,
-		.htotal = 1344,
-		.hskew = 0,
-		.vdisplay = 768,
-		.vsync_start = 771,
-		.vsync_end = 777,
-		.vtotal = 806,
-		.vscan = 0,
-		.vrefresh = 60,
-		.flags = 0xA,
-		.type = 0x40,
-		.name = "Custom 1024x768",
-	};
-
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-
-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
-
-		if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
-			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
-			continue;
-
-		if (!use_panel_fitter && c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
-		if (use_panel_fitter) {
-			connector = c;
-
-			/* This is one of the modes Xorg creates for panels, so
-			 * it should work just fine. Notice that Gens that
-			 * support LPSP are too new for panels with native
-			 * 1024x768 resolution, so this should force the panel
-			 * fitter. */
-			igt_assert(c->count_modes &&
-				   c->modes[0].hdisplay > 1024);
-			igt_assert(c->count_modes &&
-				   c->modes[0].vdisplay > 768);
-			mode = &std_1024_mode;
-			break;
-		}
-	}
-	igt_require(connector);
-
-	crtc_id = kmstest_find_crtc_for_connector(drm_fd, drm_res, connector,
-						  0);
-	buffer_id = create_fb(drm_fd, mode->hdisplay, mode->vdisplay);
-
-	igt_assert(buffer_id);
-	igt_assert(connector);
-	igt_assert(mode);
-
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
-
-	if (use_panel_fitter) {
-		if (IS_HASWELL(devid))
-			igt_assert(!lpsp_is_enabled(drm_fd));
-		else
-			igt_assert(lpsp_is_enabled(drm_fd));
-	} else {
-		igt_assert(lpsp_is_enabled(drm_fd));
-	}
+	igt_plane_t *primary;
+
+	/* set output pipe = PIPE_A for LPSP */
+	igt_output_set_pipe(data->output, PIPE_A);
+	primary = igt_output_get_plane_type(data->output,
+					    DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+	igt_create_pattern_fb(data->drm_fd,
+			      data->mode->hdisplay, data->mode->vdisplay,
+			      DRM_FORMAT_XRGB8888,
+			      LOCAL_DRM_FORMAT_MOD_NONE,
+			      &data->fb);
+	igt_plane_set_fb(primary, &data->fb);
+	igt_display_commit(&data->display);
 }
-static void non_edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			    drmModeConnectorPtr *drm_connectors)
+static void test_cleanup(data_t *data)
 {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
-
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-
-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
-
-		if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
-			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
-			continue;
-
-		if (c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
-	}
-	igt_require(connector);
-
-	crtc_id = kmstest_find_crtc_for_connector(drm_fd, drm_res, connector,
-						  0);
-	buffer_id = create_fb(drm_fd, mode->hdisplay, mode->vdisplay);
-
-	igt_assert(buffer_id);
-	igt_assert(mode);
-
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
-
-	igt_assert(!lpsp_is_enabled(drm_fd));
+	igt_plane_t *primary;
+
+	if (!data->output || data->output->pending_pipe == PIPE_NONE)
+		return;
+
+	primary = igt_output_get_plane_type(data->output,
+					    DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(primary, NULL);
+	igt_output_set_pipe(data->output, PIPE_NONE);
+	igt_display_commit(&data->display);
+	igt_remove_fb(data->drm_fd, &data->fb);
+	data->output = NULL;
 }
-#define MAX_CONNECTORS 32
+static void test_lpsp(data_t *data)
+{
+	/* LPSP is low power single pipe usages i.e. PIPE_A */
+	igt_require(igt_pipe_connector_valid(PIPE_A, data->output));
+	igt_require_f(i915_output_is_lpsp_capable(data->drm_fd, data->output),
+		      "output is not lpsp capable\n");
+
+	data->mode = igt_output_get_mode(data->output);
+	setup_lpsp_output(data);
+	igt_assert_f(lpsp_is_enabled(data), "%s: lpsp is not enabled\n%s:\n%s\n",
+		     data->output->name, PWR_DOMAIN_INFO, data->pwr_dmn_info =
+		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
+}
-int drm_fd;
-uint32_t devid;
-drmModeResPtr drm_res;
-drmModeConnectorPtr drm_connectors[MAX_CONNECTORS];
-struct intel_mmio_data mmio_data;
+IGT_TEST_DESCRIPTION("These tests validates display Low Power Single Pipe configurations");
 igt_main
 {
-	igt_fixture {
-		int i;
-
-		drm_fd = drm_open_driver_master(DRIVER_INTEL);
-		igt_require(drm_fd >= 0);
-
-		devid = intel_get_drm_devid(drm_fd);
-
-		drm_res = drmModeGetResources(drm_fd);
-		igt_require(drm_res);
-		igt_assert(drm_res->count_connectors <= MAX_CONNECTORS);
+	data_t data = {};
-		for (i = 0; i < drm_res->count_connectors; i++)
-			drm_connectors[i] = drmModeGetConnectorCurrent(drm_fd,
-							drm_res->connectors[i]);
+	igt_fixture {
+		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
+		igt_require(data.drm_fd >= 0);
+		data.debugfs_fd = igt_debugfs_dir(data.drm_fd);
+		igt_require(data.debugfs_fd >= 0);
 		igt_pm_enable_audio_runtime_pm();
-
-		igt_require(supports_lpsp(devid));
-
-		intel_register_access_init(&mmio_data, intel_get_pci_device(), 0, drm_fd);
-
 		kmstest_set_vt_graphics_mode();
+		data.devid = intel_get_drm_devid(data.drm_fd);
+		igt_display_require(&data.display, data.drm_fd);
+		igt_require(igt_pm_dmc_loaded(data.debugfs_fd));
 	}
+	igt_describe("This test validates lpsp while all crtc are disabled");
 	igt_subtest("screens-disabled")
-		screens_disabled_subtest(drm_fd, drm_res);
-	igt_subtest("edp-native")
-		edp_subtest(drm_fd, drm_res, drm_connectors, devid, false);
-	igt_subtest("non-edp")
-		non_edp_subtest(drm_fd, drm_res, drm_connectors);
+		screens_disabled_subtest(&data);
-	igt_fixture {
-		int i;
+	igt_describe("This test validates lpsp on all connected outputs on low power PIPE_A");
+	igt_subtest_with_dynamic_f("kms-lpsp") {
+		igt_display_t *display = &data.display;
+		igt_output_t *output;
-		intel_register_access_fini(&mmio_data);
-		for (i = 0; i < drm_res->count_connectors; i++)
-			drmModeFreeConnector(drm_connectors[i]);
-		drmModeFreeResources(drm_res);
-		close(drm_fd);
+		for_each_connected_output(display, output) {
+			igt_dynamic_f("kms-lpsp-%s", output->name) {
+				data.output = output;
+				test_lpsp(&data);
+			}
+
+			test_cleanup(&data);
+		}

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

tests/i915/i915_pm_lpsp.c line 177

Quoted 8 lines

tests/i915/i915_pm_lpsp.c +6 -0

+	}
+
+	igt_fixture {
+		free(data.pwr_dmn_info);
+		close(data.drm_fd);
+		igt_display_fini(&data.display);
 	}
 }

When all screens are disabled, it takes some time to release eDP/DP AUX IO power domain ref count. Also, delayed audio codec disabling takes time to release AUDIO power domain ref-count, using igt_wait to check lpsp after disabling all screens.

Signed-off-by Anshuman Gupta <[email protected]>

---
  tests/i915/i915_pm_lpsp.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

tests/i915/i915_pm_lpsp.c +0 -0

index 7be23e6c..e05d3791 100644

Quoted 10 lines · Anshuman Gupta

When all screens are disabled, it takes some time to release eDP/DP AUX IO power domain ref count. Also, delayed audio codec disabling takes time to release AUDIO power domain ref-count, using igt_wait to check lpsp after disabling all screens.

Signed-off-by Anshuman Gupta <[email protected]>

---
 tests/i915/i915_pm_lpsp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

tests/i915/i915_pm_lpsp.c static void screens_disabled_subtest(data_t *data) · line 81

Quoted 9 lines

tests/i915/i915_pm_lpsp.c +1 -1

index 7be23e6c..e05d3791 100644
@@ -79,7 +79,8 @@ static void screens_disabled_subtest(data_t *data)
 	}
 	igt_require_f(valid_output, "No connected output found\n");
-	igt_assert_f(lpsp_is_enabled(data), "lpsp is not enabled\n%s:\n%s\n",
+	igt_assert_f(igt_wait(lpsp_is_enabled(data), 1000, 100),

Better to add code-comment here how the above time is calculated. Is it based on observation or captured anywhere?

Regards,
Animesh

tests/i915/i915_pm_lpsp.c static void screens_disabled_subtest(data_t *data) · line 86

Quoted 4 lines

tests/i915/i915_pm_lpsp.c +1 -0

+		     "lpsp is not enabled\n%s:\n%s\n",
 		     PWR_DOMAIN_INFO, data->pwr_dmn_info =
 		     igt_sysfs_get(data->debugfs_fd, PWR_DOMAIN_INFO));
 }