lpsp platform agnostic support

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

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

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

lib/igt_pm.c +2 -0

index 9d441e1b..7a6cab7c 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,29 @@ bool igt_pm_pc8_plus_residencies_enabled(int msr_fd)
 
 	return true;
 }
+

lib/igt_pm.c +15 -0

+{
+	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_info", 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..076d8c27 100644

Quoted 32 lines · Anshuman Gupta

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

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

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

lib/igt_pm.c +8 -0

index 9d441e1b..7a6cab7c 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,29 @@ bool igt_pm_pc8_plus_residencies_enabled(int msr_fd)
 
 	return true;
 }
+
+/**
+ * igt_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.
+ *

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

Quoted 4 lines

lib/igt_pm.c +4 -0

+ * Returns:
+ * True if given output is lpsp capable otherwise false.
+ */
+bool igt_output_is_lpsp_capable(int drm_fd, igt_output_t *output)

Do we have a file for i915-specific features? If not, maybe prefixing the function with i915 would help readers understand that this is an i915-specific function.

Quoted 26 lines

lib/igt_pm.c +15 -0

+{
+	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_info", buf, sizeof(buf));
+
+	if (len < 0)
+		igt_assert_eq(len, -ENODEV);
+
+	close(fd);
+
+	return strstr(buf, "LPSP capable");
+}

lib/igt_pm.h +1 -0

index 5e438452..076d8c27 100644
@@ -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 igt_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
 

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

Quoted 2 lines

lib/igt_pm.h +0 -0

 #endif /* IGT_PM_H */

pub 2048R/33A53379 2019-12-02 Martin Peres <[email protected]> sub 2048R/C5E7DE5F 2019-12-02 [expires: 2020-12-01]

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 debugfs attribute i915_lpsp_info, which exposes whether an output is capable of driving lpsp and whether lpsp is enabled.

v2:
- CI failures fixup.

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

---
 tests/i915/i915_pm_lpsp.c | 303 +++++++++++++++++++++++---------------
 1 file changed, 186 insertions(+), 117 deletions(-)

tests/i915/i915_pm_lpsp.c +138 -59

index 42938e10..e400c92e 100644
@@ -25,49 +25,125 @@
  */
 
 #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	5000
 
-static bool supports_lpsp(uint32_t devid)
+#define PWR_DOMAIN_INFO "i915_power_domain_info"
+
+const char *snd_module[10] = {"snd_hda_codec_hdmi", "snd_hda_intel", "snd_hda_codec", "snd_hda_core", NULL};
+
+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 void debugfs_read(int fd, const char *param, char *buf, int len)
 {
-	return IS_HASWELL(devid) || IS_BROADWELL(devid);
+	len = igt_debugfs_simple_read(fd, param, buf, len);
+	if (len < 0)
+		igt_assert_eq(len, -ENODEV);
 }
 
-static bool lpsp_is_enabled(int drm_fd)
+static bool lpsp_is_enabled(data_t *data)
 {
-	uint32_t val;
+	char buf[MAX_SINK_LPSP_INFO_BUF_LEN];
+	int fd;
+
+	fd = igt_debugfs_connector_dir(data->drm_fd, data->output->name,
+				       O_RDONLY);
+	igt_require(fd >= 0);
+
+	debugfs_read(fd, "i915_lpsp_info", buf, sizeof(buf));
+	close(fd);
 
-	val = INREG(HSW_PWR_WELL_CTL2);
-	return !(val & HSW_PWR_WELL_STATE_ENABLED);
+	return strstr(buf, "LPSP enabled");
 }
 
-/* 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)
+/*
+ * 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(data_t *data)
 {
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-	igt_assert(lpsp_is_enabled(drm_fd));
+	igt_output_t *output;
+	enum pipe pipe;
+	igt_plane_t *primary;
+
+	for_each_pipe_with_single_output(&data->display, pipe, output) {
+		data->output = output;
+		igt_output_set_pipe(data->output, pipe);
+		primary = igt_output_get_plane_type(data->output,
+						    DRM_PLANE_TYPE_PRIMARY);
+		igt_plane_set_fb(primary, NULL);
+		igt_display_commit(&data->display);
+	}
+
+	igt_assert(lpsp_is_enabled(data));
 }
 
-static uint32_t create_fb(int drm_fd, int width, int height)
+static void check_output_lpsp(data_t *data)
 {
-	struct igt_fb fb;
+	if (igt_output_is_lpsp_capable(data->drm_fd, data->output))
+		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));
+	else
+		igt_assert(!lpsp_is_enabled(data));
+}
 
-	return igt_create_pattern_fb(drm_fd, width, height, DRM_FORMAT_XRGB8888,
-				     LOCAL_DRM_FORMAT_MOD_NONE, &fb);
+static void setup_lpsp_output(data_t *data)
+{
+	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 cleanup_lpsp_output(data_t *data)
+{
+	igt_plane_t *primary;
+
+	if (!data->output)
+		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;
 }
 
-static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			drmModeConnectorPtr *drm_connectors, uint32_t devid,
-			bool use_panel_fitter)
+static void edp_subtest(data_t *data, bool use_panel_fitter)
 {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
+	igt_display_t *display = &data->display;
+	igt_output_t *output;
+	int valid_output;
+
 	drmModeModeInfo std_1024_mode = {
 		.clock = 65000,
 		.hdisplay = 1024,

@@ -86,23 +162,17 @@ static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
 		.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];
+	for_each_connected_output(display, output) {
+		drmModeConnectorPtr c = output->config.connector;
 
 		if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
 			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
+		if (!igt_pipe_connector_valid(PIPE_A, output))
 			continue;
 
-		if (!use_panel_fitter && c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
+		data->output = output;
+
 		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

@@ -113,124 +183,123 @@ static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
 				   c->modes[0].hdisplay > 1024);
 			igt_assert(c->count_modes &&
 				   c->modes[0].vdisplay > 768);
-			mode = &std_1024_mode;
-			break;
+			data->mode = &std_1024_mode;
+			igt_output_override_mode(output, data->mode);
+		} else {
+			data->mode = igt_output_get_mode(output);
 		}
-	}
-	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);
+		setup_lpsp_output(data);
+
+		if (use_panel_fitter && IS_HASWELL(data->devid))
+			igt_assert(!lpsp_is_enabled(data));
+		else
+			check_output_lpsp(data);
 
-	igt_assert(buffer_id);
-	igt_assert(connector);
-	igt_assert(mode);
+		cleanup_lpsp_output(data);
+		valid_output++;
+	}
 
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
+	igt_require_f(valid_output, "No edp connector found\n");
+}
 
-	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));
+static bool unload_snd_hda_core_module(void)
+{
+	int i;
+
+	/* unbind snd_hda_intel */
+	kick_snd_hda_intel();
+
+	for (i = 0; snd_module[i]; i++) {
+		if (igt_kmod_is_loaded(snd_module[i]) &&
+		    igt_kmod_unload(snd_module[i], KMOD_REMOVE_FORCE)) {
+			igt_warn("Could not unload %s\n", snd_module[i]);
+			igt_kmod_list_loaded();
+			igt_lsof("/dev/snd");
+			return false;
+		}
 	}
+
+	return true;
 }
 
-static void non_edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			    drmModeConnectorPtr *drm_connectors)
+static void load_snd_hda_core_module(void)
 {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
+	igt_kmod_load("snd_hda_core", NULL);
+}

tests/i915/i915_pm_lpsp.c +36 -54

-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
+	for_each_connected_output(display, output) {
+		drmModeConnectorPtr c = output->config.connector;
 
 		if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
 			continue;
 		if (c->connection != DRM_MODE_CONNECTED)
 			continue;
+		if (!igt_pipe_connector_valid(PIPE_A, output))
+			continue;
 
-		if (c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
+		data->output = output;
+		data->mode = igt_output_get_mode(output);
+		setup_lpsp_output(data);
+		check_output_lpsp(data);
+		cleanup_lpsp_output(data);
+		load_snd_hda_core_module();
+		valid_output++;
 	}
-	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_require_f(valid_output, "No non-edp connector found\n");
 }
 
-#define MAX_CONNECTORS 32
-
-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);
+	data_t data = {};
 
-		drm_res = drmModeGetResources(drm_fd);
-		igt_require(drm_res);
-		igt_assert(drm_res->count_connectors <= MAX_CONNECTORS);
-
-		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);
+		screens_disabled_subtest(&data);
+	igt_describe("This test validates lpsp on eDP panel");
 	igt_subtest("edp-native")
-		edp_subtest(drm_fd, drm_res, drm_connectors, devid, false);
+		edp_subtest(&data, false);
+	igt_fixture
+		cleanup_lpsp_output(&data);
+	igt_describe("This test validates lpsp on eDP panel while forcing panel_fitter");
 	igt_subtest("edp-panel-fitter")
-		edp_subtest(drm_fd, drm_res, drm_connectors, devid, true);
+		edp_subtest(&data, true);
+	igt_fixture
+		cleanup_lpsp_output(&data);
+	igt_describe("This test validates lpsp on DP/HDMI/DSI panels");
 	igt_subtest("non-edp")
-		non_edp_subtest(drm_fd, drm_res, drm_connectors);
+		non_edp_subtest(&data);
+	igt_fixture
+		cleanup_lpsp_output(&data);
 
 	igt_fixture {
-		int i;
-
-		intel_register_access_fini(&mmio_data);
-		for (i = 0; i < drm_res->count_connectors; i++)

Quoted 275 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 debugfs attribute i915_lpsp_info, which exposes whether an output is capable of driving lpsp and whether lpsp is enabled.

v2:
- CI failures fixup.

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

---
 tests/i915/i915_pm_lpsp.c | 303 +++++++++++++++++++++++---------------
 1 file changed, 186 insertions(+), 117 deletions(-)

tests/i915/i915_pm_lpsp.c +138 -59

index 42938e10..e400c92e 100644
@@ -25,49 +25,125 @@
  */
 
 #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	5000
 
-static bool supports_lpsp(uint32_t devid)
+#define PWR_DOMAIN_INFO "i915_power_domain_info"
+
+const char *snd_module[10] = {"snd_hda_codec_hdmi", "snd_hda_intel", "snd_hda_codec", "snd_hda_core", NULL};
+
+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 void debugfs_read(int fd, const char *param, char *buf, int len)
 {
-	return IS_HASWELL(devid) || IS_BROADWELL(devid);
+	len = igt_debugfs_simple_read(fd, param, buf, len);
+	if (len < 0)
+		igt_assert_eq(len, -ENODEV);
 }
 
-static bool lpsp_is_enabled(int drm_fd)
+static bool lpsp_is_enabled(data_t *data)
 {
-	uint32_t val;
+	char buf[MAX_SINK_LPSP_INFO_BUF_LEN];
+	int fd;
+
+	fd = igt_debugfs_connector_dir(data->drm_fd, data->output->name,
+				       O_RDONLY);
+	igt_require(fd >= 0);
+
+	debugfs_read(fd, "i915_lpsp_info", buf, sizeof(buf));
+	close(fd);
 
-	val = INREG(HSW_PWR_WELL_CTL2);
-	return !(val & HSW_PWR_WELL_STATE_ENABLED);
+	return strstr(buf, "LPSP enabled");
 }
 
-/* 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)
+/*
+ * 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(data_t *data)
 {
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-	igt_assert(lpsp_is_enabled(drm_fd));
+	igt_output_t *output;
+	enum pipe pipe;
+	igt_plane_t *primary;
+
+	for_each_pipe_with_single_output(&data->display, pipe, output) {
+		data->output = output;
+		igt_output_set_pipe(data->output, pipe);
+		primary = igt_output_get_plane_type(data->output,
+						    DRM_PLANE_TYPE_PRIMARY);
+		igt_plane_set_fb(primary, NULL);
+		igt_display_commit(&data->display);
+	}
+
+	igt_assert(lpsp_is_enabled(data));
 }
 
-static uint32_t create_fb(int drm_fd, int width, int height)
+static void check_output_lpsp(data_t *data)
 {
-	struct igt_fb fb;
+	if (igt_output_is_lpsp_capable(data->drm_fd, data->output))
+		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));
+	else
+		igt_assert(!lpsp_is_enabled(data));
+}
 
-	return igt_create_pattern_fb(drm_fd, width, height, DRM_FORMAT_XRGB8888,
-				     LOCAL_DRM_FORMAT_MOD_NONE, &fb);
+static void setup_lpsp_output(data_t *data)
+{
+	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 cleanup_lpsp_output(data_t *data)
+{
+	igt_plane_t *primary;
+
+	if (!data->output)
+		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;
 }
 
-static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			drmModeConnectorPtr *drm_connectors, uint32_t devid,
-			bool use_panel_fitter)
+static void edp_subtest(data_t *data, bool use_panel_fitter)
 {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
+	igt_display_t *display = &data->display;
+	igt_output_t *output;
+	int valid_output;
+
 	drmModeModeInfo std_1024_mode = {
 		.clock = 65000,
 		.hdisplay = 1024,

@@ -86,23 +162,17 @@ static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
 		.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];
+	for_each_connected_output(display, output) {
+		drmModeConnectorPtr c = output->config.connector;
 
 		if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
 			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
+		if (!igt_pipe_connector_valid(PIPE_A, output))
 			continue;
 
-		if (!use_panel_fitter && c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
+		data->output = output;
+
 		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

@@ -113,124 +183,123 @@ static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
 				   c->modes[0].hdisplay > 1024);
 			igt_assert(c->count_modes &&
 				   c->modes[0].vdisplay > 768);
-			mode = &std_1024_mode;
-			break;
+			data->mode = &std_1024_mode;
+			igt_output_override_mode(output, data->mode);
+		} else {
+			data->mode = igt_output_get_mode(output);
 		}
-	}
-	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);
+		setup_lpsp_output(data);
+
+		if (use_panel_fitter && IS_HASWELL(data->devid))
+			igt_assert(!lpsp_is_enabled(data));
+		else
+			check_output_lpsp(data);
 
-	igt_assert(buffer_id);
-	igt_assert(connector);
-	igt_assert(mode);
+		cleanup_lpsp_output(data);
+		valid_output++;
+	}
 
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
+	igt_require_f(valid_output, "No edp connector found\n");
+}
 
-	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));
+static bool unload_snd_hda_core_module(void)
+{
+	int i;
+
+	/* unbind snd_hda_intel */
+	kick_snd_hda_intel();
+
+	for (i = 0; snd_module[i]; i++) {
+		if (igt_kmod_is_loaded(snd_module[i]) &&
+		    igt_kmod_unload(snd_module[i], KMOD_REMOVE_FORCE)) {
+			igt_warn("Could not unload %s\n", snd_module[i]);
+			igt_kmod_list_loaded();
+			igt_lsof("/dev/snd");
+			return false;
+		}
 	}
+
+	return true;
 }
 
-static void non_edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			    drmModeConnectorPtr *drm_connectors)
+static void load_snd_hda_core_module(void)
 {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
+	igt_kmod_load("snd_hda_core", NULL);
+}

tests/i915/i915_pm_lpsp.c static void edp_subtest(int drm_fd, drmModeResPtr drm_res, · line 238

Quoted 10 lines

tests/i915/i915_pm_lpsp.c +8 -1

+
+static void non_edp_subtest(data_t *data)
+{
+	igt_display_t *display = &data->display;
+	igt_output_t *output;
+	int valid_output;
 
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
+	/* DP/HDMI panel requires to drive lpsp without audio */
+	igt_require(unload_snd_hda_core_module());

I'm not super happy with having to unload modules that it would be very easy to forget to re-add and would make audio tests fail :s

IMO, if the DUT is not playing sound, we should be able to enter LPSP. If no, then I would consider this a driver bug for wasting energy sending 0s to the screen.

Thoughts on this?

Martin

Quoted 121 lines

tests/i915/i915_pm_lpsp.c +39 -57

-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
+	for_each_connected_output(display, output) {
+		drmModeConnectorPtr c = output->config.connector;
 
 		if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
 			continue;
 		if (c->connection != DRM_MODE_CONNECTED)
 			continue;
+		if (!igt_pipe_connector_valid(PIPE_A, output))
+			continue;
 
-		if (c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
+		data->output = output;
+		data->mode = igt_output_get_mode(output);
+		setup_lpsp_output(data);
+		check_output_lpsp(data);
+		cleanup_lpsp_output(data);
+		load_snd_hda_core_module();
+		valid_output++;
 	}
-	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_require_f(valid_output, "No non-edp connector found\n");
 }
 
-#define MAX_CONNECTORS 32
-
-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);
+	data_t data = {};
 
-		drm_res = drmModeGetResources(drm_fd);
-		igt_require(drm_res);
-		igt_assert(drm_res->count_connectors <= MAX_CONNECTORS);
-
-		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);
+		screens_disabled_subtest(&data);
+	igt_describe("This test validates lpsp on eDP panel");
 	igt_subtest("edp-native")
-		edp_subtest(drm_fd, drm_res, drm_connectors, devid, false);
+		edp_subtest(&data, false);
+	igt_fixture
+		cleanup_lpsp_output(&data);
+	igt_describe("This test validates lpsp on eDP panel while forcing panel_fitter");
 	igt_subtest("edp-panel-fitter")
-		edp_subtest(drm_fd, drm_res, drm_connectors, devid, true);
+		edp_subtest(&data, true);
+	igt_fixture
+		cleanup_lpsp_output(&data);
+	igt_describe("This test validates lpsp on DP/HDMI/DSI panels");
 	igt_subtest("non-edp")
-		non_edp_subtest(drm_fd, drm_res, drm_connectors);
+		non_edp_subtest(&data);
+	igt_fixture
+		cleanup_lpsp_output(&data);
 
 	igt_fixture {
-		int i;
-
-		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);
+		free(data.pwr_dmn_info);
+		load_snd_hda_core_module();
+		close(data.drm_fd);

tests/i915/i915_pm_lpsp.c static void edp_subtest(int drm_fd, drmModeResPtr drm_res,

Quoted 4 lines

tests/i915/i915_pm_lpsp.c +1 -0

+		igt_display_fini(&data.display);
 	}
 }
 

pub 2048R/33A53379 2019-12-02 Martin Peres <[email protected]> sub 2048R/C5E7DE5F 2019-12-02 [expires: 2020-12-01]

Quoted 290 lines · Martin

Quoted 275 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 debugfs attribute i915_lpsp_info, which exposes whether an output is capable of driving lpsp and whether lpsp is enabled.

v2:
- CI failures fixup.

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

---
 tests/i915/i915_pm_lpsp.c | 303 +++++++++++++++++++++++---------------
 1 file changed, 186 insertions(+), 117 deletions(-)

tests/i915/i915_pm_lpsp.c +138 -59

index 42938e10..e400c92e 100644
@@ -25,49 +25,125 @@
  */
 
 #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	5000
 
-static bool supports_lpsp(uint32_t devid)
+#define PWR_DOMAIN_INFO "i915_power_domain_info"
+
+const char *snd_module[10] = {"snd_hda_codec_hdmi", "snd_hda_intel", "snd_hda_codec", "snd_hda_core", NULL};
+
+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 void debugfs_read(int fd, const char *param, char *buf, int len)
 {
-	return IS_HASWELL(devid) || IS_BROADWELL(devid);
+	len = igt_debugfs_simple_read(fd, param, buf, len);
+	if (len < 0)
+		igt_assert_eq(len, -ENODEV);
 }
 
-static bool lpsp_is_enabled(int drm_fd)
+static bool lpsp_is_enabled(data_t *data)
 {
-	uint32_t val;
+	char buf[MAX_SINK_LPSP_INFO_BUF_LEN];
+	int fd;
+
+	fd = igt_debugfs_connector_dir(data->drm_fd, data->output->name,
+				       O_RDONLY);
+	igt_require(fd >= 0);
+
+	debugfs_read(fd, "i915_lpsp_info", buf, sizeof(buf));
+	close(fd);
 
-	val = INREG(HSW_PWR_WELL_CTL2);
-	return !(val & HSW_PWR_WELL_STATE_ENABLED);
+	return strstr(buf, "LPSP enabled");
 }
 
-/* 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)
+/*
+ * 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(data_t *data)
 {
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
-	igt_assert(lpsp_is_enabled(drm_fd));
+	igt_output_t *output;
+	enum pipe pipe;
+	igt_plane_t *primary;
+
+	for_each_pipe_with_single_output(&data->display, pipe, output) {
+		data->output = output;
+		igt_output_set_pipe(data->output, pipe);
+		primary = igt_output_get_plane_type(data->output,
+						    DRM_PLANE_TYPE_PRIMARY);
+		igt_plane_set_fb(primary, NULL);
+		igt_display_commit(&data->display);
+	}
+
+	igt_assert(lpsp_is_enabled(data));
 }
 
-static uint32_t create_fb(int drm_fd, int width, int height)
+static void check_output_lpsp(data_t *data)
 {
-	struct igt_fb fb;
+	if (igt_output_is_lpsp_capable(data->drm_fd, data->output))
+		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));
+	else
+		igt_assert(!lpsp_is_enabled(data));
+}
 
-	return igt_create_pattern_fb(drm_fd, width, height, DRM_FORMAT_XRGB8888,
-				     LOCAL_DRM_FORMAT_MOD_NONE, &fb);
+static void setup_lpsp_output(data_t *data)
+{
+	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 cleanup_lpsp_output(data_t *data)
+{
+	igt_plane_t *primary;
+
+	if (!data->output)
+		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;
 }
 
-static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			drmModeConnectorPtr *drm_connectors, uint32_t devid,
-			bool use_panel_fitter)
+static void edp_subtest(data_t *data, bool use_panel_fitter)
 {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
+	igt_display_t *display = &data->display;
+	igt_output_t *output;
+	int valid_output;
+
 	drmModeModeInfo std_1024_mode = {
 		.clock = 65000,
 		.hdisplay = 1024,

@@ -86,23 +162,17 @@ static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
 		.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];
+	for_each_connected_output(display, output) {
+		drmModeConnectorPtr c = output->config.connector;
 
 		if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
 			continue;
-		if (c->connection != DRM_MODE_CONNECTED)
+		if (!igt_pipe_connector_valid(PIPE_A, output))
 			continue;
 
-		if (!use_panel_fitter && c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
+		data->output = output;
+
 		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

@@ -113,124 +183,123 @@ static void edp_subtest(int drm_fd, drmModeResPtr drm_res,
 				   c->modes[0].hdisplay > 1024);
 			igt_assert(c->count_modes &&
 				   c->modes[0].vdisplay > 768);
-			mode = &std_1024_mode;
-			break;
+			data->mode = &std_1024_mode;
+			igt_output_override_mode(output, data->mode);
+		} else {
+			data->mode = igt_output_get_mode(output);
 		}
-	}
-	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);
+		setup_lpsp_output(data);
+
+		if (use_panel_fitter && IS_HASWELL(data->devid))
+			igt_assert(!lpsp_is_enabled(data));
+		else
+			check_output_lpsp(data);
 
-	igt_assert(buffer_id);
-	igt_assert(connector);
-	igt_assert(mode);
+		cleanup_lpsp_output(data);
+		valid_output++;
+	}
 
-	rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0,
-			    &connector->connector_id, 1, mode);
-	igt_assert_eq(rc, 0);
+	igt_require_f(valid_output, "No edp connector found\n");
+}
 
-	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));
+static bool unload_snd_hda_core_module(void)
+{
+	int i;
+
+	/* unbind snd_hda_intel */
+	kick_snd_hda_intel();
+
+	for (i = 0; snd_module[i]; i++) {
+		if (igt_kmod_is_loaded(snd_module[i]) &&
+		    igt_kmod_unload(snd_module[i], KMOD_REMOVE_FORCE)) {
+			igt_warn("Could not unload %s\n", snd_module[i]);
+			igt_kmod_list_loaded();
+			igt_lsof("/dev/snd");
+			return false;
+		}
 	}
+
+	return true;
 }
 
-static void non_edp_subtest(int drm_fd, drmModeResPtr drm_res,
-			    drmModeConnectorPtr *drm_connectors)
+static void load_snd_hda_core_module(void)
 {
-	int i, rc;
-	uint32_t crtc_id = 0, buffer_id = 0;
-	drmModeConnectorPtr connector = NULL;
-	drmModeModeInfoPtr mode = NULL;
+	igt_kmod_load("snd_hda_core", NULL);
+}

tests/i915/i915_pm_lpsp.c static void edp_subtest(int drm_fd, drmModeResPtr drm_res, · line 238

Quoted 10 lines

tests/i915/i915_pm_lpsp.c +8 -1

+
+static void non_edp_subtest(data_t *data)
+{
+	igt_display_t *display = &data->display;
+	igt_output_t *output;
+	int valid_output;
 
-	kmstest_unset_all_crtcs(drm_fd, drm_res);
+	/* DP/HDMI panel requires to drive lpsp without audio */
+	igt_require(unload_snd_hda_core_module());

I'm not super happy with having to unload modules that it would be very easy to forget to re-add and would make audio tests fail :s

Quoted 3 lines

IMO, if the DUT is not playing sound, we should be able to enter LPSP. If no, then I would consider this a driver bug for wasting energy sending 0s to the screen.

i915 contorls AUDIO power domain on request of i915_audio_component_ops get_power/put_power, which is an external dependency.
Despite unloading the module, it fails to enter lpsp for HDMI panels, audio driver didn't invoke the put_power to release the i915 power resources. It seems bug with audio driver interface.
My plan was to raise a gitlab bug based upon lpsp failure due to AUDIO_POWER_DOMAIN non-zero ref count despite there was no audio module.

Quoted 2 lines

Thoughts on this?

I agree with you, if there is no audio is being played from DP/hdmi we should be able to enter lpsp, but it seems audio codec requires power at the time of codec probe itself.
@Kai Vehmanen may be the best one to confirm hdmi-codec behavior?

IMO this should not block our igt validation, therefore i think in order to validate i915 lpsp use cases it would be a better approach to unload the snd module. These igt are pending since time of haswell/broadwell. Please suggest how to proceed this?

Thanks,
Anshuman Gupta.

Quoted 132 lines

Martin

Quoted 115 lines

tests/i915/i915_pm_lpsp.c +36 -54

-	for (i = 0; i < drm_res->count_connectors; i++) {
-		drmModeConnectorPtr c = drm_connectors[i];
+	for_each_connected_output(display, output) {
+		drmModeConnectorPtr c = output->config.connector;
 
 		if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
 			continue;
 		if (c->connection != DRM_MODE_CONNECTED)
 			continue;
+		if (!igt_pipe_connector_valid(PIPE_A, output))
+			continue;
 
-		if (c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
+		data->output = output;
+		data->mode = igt_output_get_mode(output);
+		setup_lpsp_output(data);
+		check_output_lpsp(data);
+		cleanup_lpsp_output(data);
+		load_snd_hda_core_module();
+		valid_output++;
 	}
-	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_require_f(valid_output, "No non-edp connector found\n");
 }
 
-#define MAX_CONNECTORS 32
-
-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);
+	data_t data = {};
 
-		drm_res = drmModeGetResources(drm_fd);
-		igt_require(drm_res);
-		igt_assert(drm_res->count_connectors <= MAX_CONNECTORS);
-
-		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);
+		screens_disabled_subtest(&data);
+	igt_describe("This test validates lpsp on eDP panel");
 	igt_subtest("edp-native")
-		edp_subtest(drm_fd, drm_res, drm_connectors, devid, false);
+		edp_subtest(&data, false);
+	igt_fixture
+		cleanup_lpsp_output(&data);
+	igt_describe("This test validates lpsp on eDP panel while forcing panel_fitter");
 	igt_subtest("edp-panel-fitter")
-		edp_subtest(drm_fd, drm_res, drm_connectors, devid, true);
+		edp_subtest(&data, true);
+	igt_fixture
+		cleanup_lpsp_output(&data);
+	igt_describe("This test validates lpsp on DP/HDMI/DSI panels");
 	igt_subtest("non-edp")
-		non_edp_subtest(drm_fd, drm_res, drm_connectors);
+		non_edp_subtest(&data);
+	igt_fixture
+		cleanup_lpsp_output(&data);
 
 	igt_fixture {
-		int i;
-
-		intel_register_access_fini(&mmio_data);
-		for (i = 0; i < drm_res->count_connectors; i++)

tests/i915/i915_pm_lpsp.c static void edp_subtest(int drm_fd, drmModeResPtr drm_res,

Quoted 10 lines

tests/i915/i915_pm_lpsp.c +4 -3

-			drmModeFreeConnector(drm_connectors[i]);
-		drmModeFreeResources(drm_res);
-		close(drm_fd);
+		free(data.pwr_dmn_info);
+		load_snd_hda_core_module();
+		close(data.drm_fd);
+		igt_display_fini(&data.display);
 	}
 }
 

pub 2048R/33A53379 2019-12-02 Martin Peres <[email protected]> sub 2048R/C5E7DE5F 2019-12-02 [expires: 2020-12-01]

Quoted 6 lines · Martin

tests/i915/i915_pm_lpsp.c static void edp_subtest(int drm_fd, drmModeResPtr drm_res, · line 238

Quoted 2 lines · Anshuman Gupta

tests/i915/i915_pm_lpsp.c +2 -0

+	/* DP/HDMI panel requires to drive lpsp without audio */
+	igt_require(unload_snd_hda_core_module());

I'm not super happy with having to unload modules that it would be very easy to forget to re-add and would make audio tests fail :s

Quoted 3 lines

IMO, if the DUT is not playing sound, we should be able to enter LPSP. If no, then I would consider this a driver bug for wasting energy sending 0s to the screen.

i915 contorls AUDIO power domain on request of i915_audio_component_ops get_power/put_power, which is an external dependency.
Despite unloading the module, it fails to enter lpsp for HDMI panels, audio driver didn't invoke the put_power to release the i915 power resources. It seems bug with audio driver interface.

My plan was to raise a gitlab bug based upon lpsp failure due to AUDIO_POWER_DOMAIN non-zero ref count despite there was no audio module.

Quoted 2 lines

Thoughts on this?

I agree with you, if there is no audio is being played from DP/hdmi we should be able to enter lpsp, but it seems audio codec requires power at the time of codec probe itself.
@Kai Vehmanen may be the best one to confirm hdmi-codec behavior?

Hey,

Quoted 7 lines · Anshuman Gupta

Quoted 6 lines · Martin

tests/i915/i915_pm_lpsp.c static void edp_subtest(int drm_fd, drmModeResPtr drm_res, · line 238

Quoted 2 lines · Anshuman Gupta

tests/i915/i915_pm_lpsp.c +2 -0

+	/* DP/HDMI panel requires to drive lpsp without audio */
+	igt_require(unload_snd_hda_core_module());

I'm not super happy with having to unload modules that it would be very easy to forget to re-add and would make audio tests fail :s

at least this may be painful to maintain. We have three audio drivers in active use (HDA snd-hda-intel, SST DSP and SOF DSP -> drivers is selected at runtime dependsing on hw generation, enabled features and the available DSP FW), and the modules you need to unload are different for all three, and may change over time. So not so nice to maintain if you need to support a large variety of hardware targets.

Quoted 3 lines

Quoted 3 lines

IMO, if the DUT is not playing sound, we should be able to enter LPSP. If no, then I would consider this a driver bug for wasting energy sending 0s to the screen.

Quoted 5 lines

i915 contorls AUDIO power domain on request of i915_audio_component_ops get_power/put_power, which is an external dependency.
Despite unloading the module, it fails to enter lpsp for HDMI panels, audio driver didn't invoke the put_power to release the i915 power resources. It seems bug with audio driver interface.

Yes, this sounds like a plain bug. Audio should never keep powers on unless it needs it for some actual use-case.

Quoted 10 lines

My plan was to raise a gitlab bug based upon lpsp failure due to AUDIO_POWER_DOMAIN non-zero ref count despite there was no audio module.

Quoted 2 lines

Thoughts on this?

I agree with you, if there is no audio is being played from DP/hdmi we should be able to enter lpsp, but it seems audio codec requires power at the time of codec probe itself.
@Kai Vehmanen may be the best one to confirm hdmi-codec behavior?

Yes, audio codec asks for i915 power when it is probed, and also whenever it uses the HDA bus. But when it is idle, it will release i915 power via put_power. One possible problem could be that the audio driver is uilt without runtime pm support. Then it will indeed keep a ref to i915 power all the time. Can you provide a bit of data about the case (which hardware, kernel config used, possible dmesg log of error)?

Br, Kai (Sound Open Firmware (SOF) team)

It makes sense to skip the test if edp-panel native resoultion is limted to 1024x768 rather than failing the test.

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

---
 tests/i915/i915_pm_lpsp.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

tests/i915/i915_pm_lpsp.c +0 -1

index e400c92e..0ce694e6 100644
@@ -179,10 +179,10 @@ static void edp_subtest(data_t *data, bool use_panel_fitter)
 			 * support LPSP are too new for panels with native
 			 * 1024x768 resolution, so this should force the panel
 			 * fitter. */
-			igt_assert(c->count_modes &&

Some times delayed audio codec disabling causes failure of test, using igt_wait to check lpsp after disabling all outputs.

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

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

tests/i915/i915_pm_lpsp.c +0 -0

index 0ce694e6..5f15c034 100644

Quoted 2 lines · Anshuman Gupta

Some times delayed audio codec disabling causes failure of test, using igt_wait to check lpsp after disabling all outputs.

Seems like this answers my question on patch 2/5: We are sending audio to the screen even when we do not have anything to send...

I know there are some screens that take ages to re-enable sound after not getting any for a while, but waiting 10 or even 30s without sound before disabling it does not sound like a terrible idea. My TV automatically disables sound after way less than that anyway.

So, to me, this is a driver bug.

That being said, the patch is fine even if your reason for it is not acceptable.

Martin

Quoted 18 lines

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

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

tests/i915/i915_pm_lpsp.c +1 -1

index 0ce694e6..5f15c034 100644
@@ -91,7 +91,7 @@ static void screens_disabled_subtest(data_t *data)
 		igt_display_commit(&data->display);
 	}
 
-	igt_assert(lpsp_is_enabled(data));
+	igt_assert(igt_wait(lpsp_is_enabled(data), 1000, 100));
 }
 

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

Quoted 2 lines

tests/i915/i915_pm_lpsp.c +0 -0

 static void check_output_lpsp(data_t *data)

pub 2048R/33A53379 2019-12-02 Martin Peres <[email protected]> sub 2048R/C5E7DE5F 2019-12-02 [expires: 2020-12-01]

Initialize the mode set params for lpsp/non-lpsp screen based upon their output lpsp capability instead of edp/non-edp output type.

v2:
- CI failures fixup.

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

---
 tests/i915/i915_pm_rpm.c | 53 +++++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 23 deletions(-)

tests/i915/i915_pm_rpm.c +28 -21

index 4f8124dc..e2fb732c 100644
@@ -94,6 +94,7 @@ struct mode_set_data {
 	drmModeResPtr res;
 	drmModeConnectorPtr connectors[MAX_CONNECTORS];
 	drmModePropertyBlobPtr edids[MAX_CONNECTORS];
+	igt_display_t display;
 
 	uint32_t devid;
 };

@@ -254,29 +255,29 @@ static bool init_modeset_params_for_type(struct mode_set_data *data,
 {
 	drmModeConnectorPtr connector = NULL;
 	drmModeModeInfoPtr mode = NULL;
+	igt_output_t *output = NULL;
+	igt_display_t *display = &data->display;
 
-	if (!data->res)
+	if (!data->res || !display)
 		return false;
 
-	for (int i = 0; i < data->res->count_connectors; i++) {
-		drmModeConnectorPtr c = data->connectors[i];
+	for_each_connected_output(display, output) {
+		drmModeConnectorPtr c = output->config.connector;
 
 		if (type == SCREEN_TYPE_LPSP &&
-		    c->connector_type != DRM_MODE_CONNECTOR_eDP)
+		     !igt_output_is_lpsp_capable(drm_fd, output))
 			continue;
 
 		if (type == SCREEN_TYPE_NON_LPSP &&
-		    c->connector_type == DRM_MODE_CONNECTOR_eDP)
+		    igt_output_is_lpsp_capable(drm_fd, output))
 			continue;
 
-		if (c->connection == DRM_MODE_CONNECTED && c->count_modes) {
-			connector = c;
-			mode = &c->modes[0];
-			break;
-		}
+		connector = c;
+		mode = igt_output_get_mode(output);
+		break;
 	}
 
-	if (!connector)
+	if (!connector || !mode)
 		return false;
 
 	igt_create_pattern_fb(drm_fd, mode->hdisplay, mode->vdisplay,

@@ -397,6 +398,7 @@ static void init_mode_set_data(struct mode_set_data *data)
 		kmstest_set_vt_graphics_mode();
 	}
 
+	igt_display_require(&data->display, drm_fd);
 	data->devid = intel_get_drm_devid(drm_fd);
 	init_modeset_cached_params(&ms_data);
 }

@@ -410,6 +412,8 @@ static void fini_mode_set_data(struct mode_set_data *data)
 		}
 		drmModeFreeResources(data->res);
 	}
+
+	igt_display_fini(&data->display);
 }
 
 static void get_drm_info(struct compare_data *data)

@@ -760,7 +764,7 @@ static void dump_file(int dir, const char *filename)
 	free(contents);
 }
 
-static bool setup_environment(void)
+static bool setup_environment(bool display_disabled)
 {
 	if (has_runtime_pm)
 		goto out;

@@ -772,7 +776,8 @@ static bool setup_environment(void)
 	debugfs = igt_debugfs_dir(drm_fd);
 	igt_require(debugfs != -1);
 
-	init_mode_set_data(&ms_data);
+	if (!display_disabled)
+		init_mode_set_data(&ms_data);
 
 	igt_pm_enable_sata_link_power_management();
 

@@ -785,13 +790,14 @@ static bool setup_environment(void)
 	igt_require(igt_pm_dmc_loaded(debugfs));
 
 out:
-	disable_all_screens(&ms_data);
+	if (!display_disabled)
+		disable_all_screens(&ms_data);
 	dump_file(debugfs, "i915_runtime_pm_status");
 
 	return wait_for_suspended();
 }
 
-static void teardown_environment(void)
+static void teardown_environment(bool display_disabled)
 {
 	close(msr_fd);
 	if (has_pc8)

@@ -801,7 +807,8 @@ static void teardown_environment(void)
 
 	igt_pm_restore_sata_link_power_management();
 
-	fini_mode_set_data(&ms_data);
+	if (!display_disabled)
+		fini_mode_set_data(&ms_data);
 
 	close(debugfs);
 	close(drm_fd);

@@ -2008,7 +2015,7 @@ static struct option long_options[] = {
 igt_main_args("", long_options, help_str, opt_handler, NULL)
 {
 	igt_subtest("basic-rte") {
-		igt_assert(setup_environment());
+		igt_assert(setup_environment(false));
 		basic_subtest();
 	}
 

@@ -2016,7 +2023,7 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
 	 * PC8+. We don't want bug reports from cases where the machine is just
 	 * not properly configured. */
 	igt_fixture
-		igt_require(setup_environment());
+		igt_require(setup_environment(false));
 
 	if (stay)
 		igt_subtest("stay")

@@ -2146,7 +2153,7 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
 	}
 
 	igt_fixture
-		teardown_environment();
+		teardown_environment(false);
 
 	igt_subtest("module-reload") {
 		igt_debug("Reload w/o display\n");

@@ -2155,9 +2162,9 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
 		igt_kmsg(KMSG_INFO "Reloading i915 w/o display\n");
 		igt_assert_eq(igt_i915_driver_load("disable_display=1 mmio_debug=-1"), 0);
 
-		igt_assert(setup_environment());
+		igt_assert(setup_environment(true));
 		igt_assert(igt_wait(device_in_pci_d3(), 2000, 100));
-		teardown_environment();
+		teardown_environment(true);
 
 		igt_debug("Reload as normal\n");
 		igt_i915_driver_unload();

@@ -2165,11 +2172,11 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
 		igt_kmsg(KMSG_INFO "Reloading i915 as normal\n");
 		igt_assert_eq(igt_i915_driver_load("mmio_debug=-1"), 0);