[PATCH IGT v2 0/6] IGT PSR Fix-ups

[PATCH IGT v2 0/6] IGT PSR Fix-ups

[Jim Bride]

These patches, along with the kernel series at
https://patchwork.freedesktop.org/series/24049/ allow our PSR
IGT tests to run more predictably on HSW, BDW, and SKL.  These
patches depend on the kernel series in order to run properly.  On
the systems I have available the following sets of tests run and pass.
I still see some very sporadic (every few hundred tests executions or so)
failures to read the sink CRC on SKL.

HSW:
	* kms_psr_sink_crc (all)
	* kms_frontbuffer_tracking (subtests psr-1p*, my system doesn't
	  support FBC)
	* kms_fbcon_fbt (subtests psr*)

BDW and SKL:
    	* kms_psr_sink_crc (all)
	* kms_frontbuffer_tracking (subtests psr-1p* and fbcpsr-1p*)
	* kms_fbcon_fbt (all)

Jim Bride (6):
  tests/kms_psr_sink_crc: Change assert_or_manual() to a macro
  lib: Add PSR utility functions to igt library.
  tests/kms_psr_sink_crc: Refactor to use new PSR library primitives
  tests/kms_frontbuffer_tracking: Refactor to use IGT PSR library
    functions
  tests/kms_frontbuffer_tracking: Fix multidraw subtest
  tests/kms_fbcon_fbt: Refactor to use IGT PSR library functions

 lib/Makefile.sources             |   2 +
 lib/igt.h                        |   1 +
 lib/igt_psr.c                    | 235 +++++++++++++++++++++++++++++++++++++++
 lib/igt_psr.h                    |  43 +++++++
 tests/kms_fbcon_fbt.c            |  54 +++------
 tests/kms_frontbuffer_tracking.c | 122 ++++----------------
 tests/kms_psr_sink_crc.c         | 144 ++++++++++++------------
 7 files changed, 391 insertions(+), 210 deletions(-)
 create mode 100644 lib/igt_psr.c
 create mode 100644 lib/igt_psr.h

-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH IGT v2 1/6] tests/kms_psr_sink_crc: Change assert_or_manual() to a macro

[Jim Bride]

Make assert_or_manual() a macro so that we get accurate line number
information when this assertion fails.

v2: Rebase

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 tests/kms_psr_sink_crc.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
index bd3fa5e..1a03719 100644
--- a/tests/kms_psr_sink_crc.c
+++ b/tests/kms_psr_sink_crc.c
@@ -278,11 +278,11 @@ static bool is_green(char *crc)
 		(bh & mask) == 0);
 }
 
-static void assert_or_manual(bool condition, const char *expected)
-{
-	igt_debug_manual_check("no-crc", expected);
-	igt_assert(igt_interactive_debug || condition);
-}
+#define assert_or_manual(condition, expected)             \
+do {                                                      \
+	igt_debug_manual_check("no-crc", expected);       \
+	igt_assert(igt_interactive_debug || condition);   \
+} while (0)
 
 static void run_test(data_t *data)
 {
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH IGT v2 2/6] lib: Add PSR utility functions to igt library.

[Jim Bride]

Factor out some code that was replicated in three test utilities into
some new IGT library functions so that we are checking PSR status in
a consistent fashion across all of our PSR tests.

v2: * Add sink CRC helper function
    * Misc. bug fixes
    * Rebase

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 lib/Makefile.sources |   2 +
 lib/igt.h            |   1 +
 lib/igt_psr.c        | 235 +++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_psr.h        |  43 ++++++++++
 4 files changed, 281 insertions(+)
 create mode 100644 lib/igt_psr.c
 create mode 100644 lib/igt_psr.h

diff --git a/lib/Makefile.sources b/lib/Makefile.sources
index 53fdb54..6a73c8c 100644
--- a/lib/Makefile.sources
+++ b/lib/Makefile.sources
@@ -83,6 +83,8 @@ lib_source_list =	 	\
 	uwildmat/uwildmat.c	\
 	igt_kmod.c		\
 	igt_kmod.h		\
+	igt_psr.c		\
+	igt_psr.h		\
 	$(NULL)
 
 .PHONY: version.h.tmp
diff --git a/lib/igt.h b/lib/igt.h
index a069deb..0b8e3a8 100644
--- a/lib/igt.h
+++ b/lib/igt.h
@@ -37,6 +37,7 @@
 #include "igt_gt.h"
 #include "igt_kms.h"
 #include "igt_pm.h"
+#include "igt_psr.h"
 #include "igt_stats.h"
 #ifdef HAVE_CHAMELIUM
 #include "igt_chamelium.h"
diff --git a/lib/igt_psr.c b/lib/igt_psr.c
new file mode 100644
index 0000000..a2823bf
--- /dev/null
+++ b/lib/igt_psr.c
@@ -0,0 +1,235 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "igt.h"
+#include <stdio.h>
+#include <stdbool.h>
+#include <sys/types.h>
+#include <string.h>
+#include <fcntl.h>
+
+/**
+ * SECTION:igt_psr
+ * @short_description: Panel Self Refresh helpers
+ * @title: Panel Self Refresh
+ * @include: igt.h
+ *
+ * This library provides various helpers to enable Panel Self Refresh,
+ * as well as to check the state of PSR on the system (enabled vs.
+ * disabled, active vs. inactive) or to wait for PSR to be active
+ * or inactive.
+ */
+
+/**
+ * igt_psr_source_support:
+ *
+ * Returns true if the source supports PSR.
+ */
+bool igt_psr_source_support(int fd)
+{
+	char buf[512];
+
+	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
+
+	return strstr(buf, "Source_OK: yes\n");
+}
+
+
+/**
+ * igt_psr_sink_support:
+ *
+ * Returns true if the current eDP panel supports PSR.
+ */
+bool igt_psr_sink_support(int fd)
+{
+	char buf[256];
+
+	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
+	return strstr(buf, "Sink_Support: yes\n");
+}
+
+/**
+ * igt_psr_possible:
+ *
+ * Returns true if both the source and sink support PSR.
+ */
+bool igt_psr_possible(int fd)
+{
+	char buf[512];
+
+	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
+
+	return igt_psr_source_support(fd) && igt_psr_sink_support(fd);
+}
+
+/**
+ * igt_psr_active:
+ *
+ * Returns true if PSR is active on the panel.
+ */
+bool igt_psr_active(int fd)
+{
+	char buf[512];
+	bool actret = false;
+	bool hwactret = false;
+
+	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
+	hwactret = (strstr(buf, "HW Enabled & Active bit: yes\n") != NULL);
+	actret = (strstr(buf, "Active: yes\n") != NULL);
+	igt_debug("hwactret: %s actret: %s\n", hwactret ? "true" : "false",
+		 actret ? "true" : "false");
+	return hwactret && actret;
+}
+
+/**
+ * igt_psr_await_status:
+ * @active: A boolean that causes the function to wait for PSR to activate
+ *          if set to true, or to wait for PSR to deactivate if false.
+ *
+ * Returns true if the requested condition is met.
+ */
+bool igt_psr_await_status(int fd, bool active)
+{
+	const int timeout = 10;
+	int count = 0;
+	while (count < timeout) {
+		if (igt_psr_active(fd) == active) {
+			igt_debug("PSR %s after %d seconds.\n",
+				  active ? "Active" : "Inactive", count);
+			return true;
+		}
+		count++;
+		sleep(1);
+	}
+	return false;
+}
+
+/**
+ * igt_psr_enabled:
+ *
+ * Returns true if the PSR feature is enabled.
+ */
+bool igt_psr_enabled(int fd)
+{
+	char buf[256];
+
+	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
+	return strstr(buf, "Enabled: yes\n");
+}
+
+/**
+ * igt_psr_print_status:
+ *
+ * Dumps the contents of i915_edp_psr_status from debugfs.
+ */
+void igt_psr_print_status(int fd)
+{
+        char buf[256];
+
+        igt_debugfs_read(fd, "i915_edp_psr_status", buf);
+        igt_info("PSR status:\n%s\n", buf);
+}
+
+/**
+ * igt_psr_valid_connector:
+ * @connector: a drmModeConnector pointer to check
+ *
+ * Returns true if connector is an eDP connector.
+ */
+bool igt_psr_valid_connector(drmModeConnectorPtr connector)
+{
+	return (connector->connector_type == DRM_MODE_CONNECTOR_eDP);
+}
+
+/**
+ * igt_psr_find_good_mode
+ * @connector: a drmModeConnector pointer to find the mode on
+ * @mode: a drmModeModePtr pointer that is set to the matching mode
+ *
+ * Returns true (and populates *mode with the match) if a valid
+ * PSR mdoe is found, and false otherwise.
+ */
+bool igt_psr_find_good_mode(drmModeConnectorPtr connector,
+			    drmModeModeInfoPtr *mode)
+{
+	int i;
+
+	if (!connector->count_modes) {
+		igt_warn("no modes for connector %d.\n",
+			 connector->connector_id);
+		return false;
+	} else {
+		igt_debug("Connector has %d modes.\n", connector->count_modes);
+	}
+
+	for (i = 0; i < connector->count_modes; i++) {
+		if ((connector->modes[i].vtotal -
+		     connector->modes[i].vdisplay) >= 36) {
+			igt_debug("Mode %d good for PSR.\n", i);
+			*mode = &(connector->modes[i]);
+			return true;
+		} else {
+			igt_debug("Throwing out mode %d\n", i);
+		}
+	}
+	return false;
+}
+
+int igt_psr_open_sink_crc(int drm_fd)
+{
+	int crc_fd;
+
+	crc_fd = openat(drm_fd, "i915_sink_crc_eDP1", O_RDONLY);
+	igt_assert_lte(0, crc_fd);
+	return crc_fd;
+}
+
+
+int igt_psr_get_sink_crc(int fd, char *data)
+{
+	int rc, errno_;
+	char buf[13];
+
+	memset(buf, 0, 13);
+	lseek(fd, 0, SEEK_SET);
+
+	rc = read(fd, buf, 12);
+	errno_ = errno;
+
+	if (rc == -1) {
+		if (errno_ == ENOTTY)
+			igt_info("Sink CRC not supported: panel doesn't support it\n");
+		else if (errno_ != ETIMEDOUT)
+			igt_assert_f(rc != -1, "Unexpected error: %d\n",
+				     errno_);
+		return errno_;
+	} else {
+		int i;
+		unsigned long long val = strtoull(buf, NULL, 16);
+
+		for (i = 5; i >= 0; i--, val >>= 8) {
+			data[i] = (unsigned char) (val & 0xff);
+		}
+		return 0;
+	}
+}
diff --git a/lib/igt_psr.h b/lib/igt_psr.h
new file mode 100644
index 0000000..d94cdfa
--- /dev/null
+++ b/lib/igt_psr.h
@@ -0,0 +1,43 @@
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef IGT_PSR_H
+#define IGT_PSR_H
+
+#define igt_psr_enable() igt_set_module_param_int("enable_psr", 1)
+#define igt_psr_disable() igt_set_module_param_int("enable_psr", 0)
+
+bool igt_psr_sink_support(int fd);
+bool igt_psr_source_support(int fd);
+bool igt_psr_possible(int fd);
+bool igt_psr_active(int fd);
+bool igt_psr_await_status(int fd, bool active);
+bool igt_psr_enabled(int fd);
+void igt_psr_print_status(int fd);
+bool igt_psr_valid_connector(drmModeConnectorPtr connector);
+bool igt_psr_find_good_mode(drmModeConnectorPtr connector,
+			    drmModeModeInfoPtr *mode);
+int igt_psr_open_sink_crc(int drm_fd);
+int igt_psr_get_sink_crc(int fd, char *data);
+
+#endif /* IGT_PSR_H */
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH IGT v2 3/6] tests/kms_psr_sink_crc: Refactor to use new PSR library primitives

[Jim Bride]

v2: * Minor functional tweaks & bug fixes
    * Rebase

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 tests/kms_psr_sink_crc.c | 134 +++++++++++++++++++++++------------------------
 1 file changed, 66 insertions(+), 68 deletions(-)

diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
index 1a03719..50d002d 100644
--- a/tests/kms_psr_sink_crc.c
+++ b/tests/kms_psr_sink_crc.c
@@ -33,8 +33,6 @@
 
 bool running_with_psr_disabled;
 
-#define CRC_BLACK "000000000000"
-
 enum operations {
 	PAGE_FLIP,
 	MMAP_GTT,
@@ -64,13 +62,15 @@ static const char *op_str(enum operations op)
 
 typedef struct {
 	int drm_fd;
+	int debugfs_fd;
+	int crc_fd;
 	int test_plane;
 	enum operations op;
 	uint32_t devid;
 	uint32_t crtc_id;
 	igt_display_t display;
 	drm_intel_bufmgr *bufmgr;
-	struct igt_fb fb_green, fb_white;
+	struct igt_fb fb_green, fb_white, fb_blue;
 	igt_plane_t *primary, *sprite, *cursor;
 	int mod_size;
 	int mod_stride;
@@ -98,6 +98,7 @@ static void setup_output(data_t *data)
 {
 	igt_display_t *display = &data->display;
 	igt_output_t *output;
+	drmModeModeInfoPtr mode = NULL;
 	enum pipe pipe;
 
 	for_each_pipe_with_valid_output(display, pipe, output) {
@@ -106,10 +107,15 @@ static void setup_output(data_t *data)
 		if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
 			continue;
 
+		if (!igt_psr_find_good_mode(c, &mode))
+			continue;
+
+		igt_assert(mode != NULL);
+		igt_output_override_mode(output, mode);
 		igt_output_set_pipe(output, pipe);
 		data->crtc_id = output->config.crtc->crtc_id;
 		data->output = output;
-		data->mode = igt_output_get_mode(output);
+		data->mode = &output->override_mode;
 
 		return;
 	}
@@ -119,10 +125,33 @@ static void display_init(data_t *data)
 {
 	igt_display_init(&data->display, data->drm_fd);
 	setup_output(data);
+
+	/* We need to be able to do a modeset before we enable PSR to
+	 * ensure that we are running at a mode such that PSR setup can
+	 * complete within a single vblank interval.
+	 */
+	igt_debug("Selected mode:\n");
+	kmstest_dump_mode(data->mode);
+	igt_create_color_fb(data->drm_fd,
+			    data->mode->hdisplay, data->mode->vdisplay,
+			    DRM_FORMAT_XRGB8888,
+			    LOCAL_I915_FORMAT_MOD_X_TILED,
+			    0.0, .0, 1.0,
+			    &data->fb_blue);
+
+	data->primary = igt_output_get_plane_type(data->output,
+						  DRM_PLANE_TYPE_PRIMARY);
+	igt_plane_set_fb(data->primary, &data->fb_blue);
+	igt_display_commit(&data->display);
+	igt_set_module_param_int("enable_psr", running_with_psr_disabled ?
+				 0 : 1);
 }
 
 static void display_fini(data_t *data)
 {
+	close(data->crc_fd);
+	close(data->debugfs_fd);
+	igt_output_override_mode(data->output, NULL);
 	igt_display_fini(&data->display);
 }
 
@@ -192,90 +221,59 @@ static void fill_render(data_t *data, uint32_t handle, unsigned char color)
 	gem_bo_busy(data->drm_fd, handle);
 }
 
-static bool psr_possible(data_t *data)
+static inline bool psr_possible(data_t *data)
 {
-	char buf[512];
-
-	igt_debugfs_read(data->drm_fd, "i915_edp_psr_status", buf);
-
 	return running_with_psr_disabled ||
-		strstr(buf, "Sink_Support: yes\n");
+		igt_psr_sink_support(data->drm_fd);
 }
 
-static bool psr_active(data_t *data)
+static inline bool psr_active(data_t *data)
 {
-	char buf[512];
-
-	igt_debugfs_read(data->drm_fd, "i915_edp_psr_status", buf);
-
 	return running_with_psr_disabled ||
-		strstr(buf, "HW Enabled & Active bit: yes\n");
+		igt_psr_active(data->drm_fd);
 }
 
-static bool wait_psr_entry(data_t *data)
+static inline bool wait_psr_entry(data_t *data)
 {
-	int timeout = 5;
-	while (timeout--) {
-		if (psr_active(data))
-			return true;
-		sleep(1);
-	}
-	return false;
+	return running_with_psr_disabled ||
+		igt_psr_await_status(data->drm_fd, true);
 }
 
 static void get_sink_crc(data_t *data, char *crc) {
-	int dir;
+	int rc, tries = 10;
 
+	memset(crc, 0, 6);
 	if (igt_interactive_debug)
 		return;
 
-	dir = igt_debugfs_dir(data->drm_fd);
-	igt_require_f(igt_sysfs_scanf(dir, "i915_sink_crc_eDP1", "%s\n", crc),
-		      "Sink CRC is unreliable on this machine. Try manual debug with --interactive-debug=no-crc\n");
-	close(dir);
-
-	igt_debug("%s\n", crc);
+	do {
+		rc = igt_psr_get_sink_crc(data->crc_fd, crc);
+		if (rc)
+			usleep(50000);
+	} while (rc && --tries);
+
+	if (rc)
+		igt_skip("Could not get sink CRC after ten tries.\n");
+	
+	igt_debug("CRC: %hhx %hhx %hhx %hhx %hhx %hhx\n",
+		  crc[0], crc[1], crc[2], crc[3], crc[4], crc[5]);
 	igt_debug_wait_for_keypress("crc");
 
 	/* The important value was already taken.
 	 * Now give a time for human eyes
 	 */
 	usleep(300000);
-
-	/* Black screen is always invalid */
-	igt_assert(strcmp(crc, CRC_BLACK) != 0);
 }
 
 static bool is_green(char *crc)
 {
-	char color_mask[5] = "FFFF\0";
-	char rs[5], gs[5], bs[5];
-	unsigned int rh, gh, bh, mask;
-	int ret;
+	unsigned int rh, gh, bh;
 
-	if (igt_interactive_debug)
-		return false;
+	rh = (crc[1] << 8) | crc[0];
+	gh = (crc[3] << 8) | crc[2];
+	bh = (crc[5] << 8) | crc[4];
 
-	sscanf(color_mask, "%4x", &mask);
-
-	memcpy(rs, &crc[0], 4);
-	rs[4] = '\0';
-	ret = sscanf(rs, "%4x", &rh);
-	igt_require(ret > 0);
-
-	memcpy(gs, &crc[4], 4);
-	gs[4] = '\0';
-	ret = sscanf(gs, "%4x", &gh);
-	igt_require(ret > 0);
-
-	memcpy(bs, &crc[8], 4);
-	bs[4] = '\0';
-	ret = sscanf(bs, "%4x", &bh);
-	igt_require(ret > 0);
-
-	return ((rh & mask) == 0 &&
-		(gh & mask) != 0 &&
-		(bh & mask) == 0);
+	return ((rh == 0) && (gh != 0) && (bh == 0));
 }
 
 #define assert_or_manual(condition, expected)             \
@@ -289,8 +287,8 @@ static void run_test(data_t *data)
 	uint32_t handle = data->fb_white.gem_handle;
 	igt_plane_t *test_plane;
 	void *ptr;
-	char ref_crc[12];
-	char crc[12];
+	char ref_crc[6];
+	char crc[6];
 	const char *expected = "";
 
 	/* Confirm that screen became Green */
@@ -347,9 +345,9 @@ static void run_test(data_t *data)
 		memset(ptr, 0xff, data->mod_size);
 		get_sink_crc(data, crc);
 		if (data->test_plane == DRM_PLANE_TYPE_PRIMARY)
-			assert_or_manual(strcmp(ref_crc, crc) == 0, "screen WHITE");
+			assert_or_manual(memcmp(ref_crc, crc, sizeof(crc)) == 0, "screen WHITE");
 		else
-			assert_or_manual(strcmp(ref_crc, crc) == 0,
+			assert_or_manual(memcmp(ref_crc, crc, sizeof(crc)) == 0,
 			       "GREEN background with WHITE box");
 
 		igt_info("Waiting 10s...\n");
@@ -392,7 +390,7 @@ static void run_test(data_t *data)
 		break;
 	}
 	get_sink_crc(data, crc);
-	assert_or_manual(strcmp(ref_crc, crc) != 0, expected);
+	assert_or_manual(memcmp(ref_crc, crc, sizeof(crc)) != 0, expected);
 }
 
 static void test_cleanup(data_t *data) {
@@ -504,12 +502,11 @@ int main(int argc, char *argv[])
 
 	igt_fixture {
 		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);
+		data.debugfs_fd = igt_debugfs_dir(data.drm_fd);
+		data.crc_fd = igt_psr_open_sink_crc(data.debugfs_fd);
 		kmstest_set_vt_graphics_mode();
 		data.devid = intel_get_drm_devid(data.drm_fd);
 
-		igt_set_module_param_int("enable_psr", running_with_psr_disabled ?
-					 0 : 1);
-
 		igt_skip_on(!psr_possible(&data));
 
 		data.bufmgr = drm_intel_bufmgr_gem_init(data.drm_fd, 4096);
@@ -517,6 +514,7 @@ int main(int argc, char *argv[])
 		drm_intel_bufmgr_gem_enable_reuse(data.bufmgr);
 
 		display_init(&data);
+		igt_skip_on(!psr_possible(&data));
 	}
 
 	igt_subtest("psr_basic") {
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH IGT v2 4/6] tests/kms_frontbuffer_tracking: Refactor to use IGT PSR library functions

[Jim Bride]

v2: * Minor functional tweaks and bug fixes
    * Rebase

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 tests/kms_frontbuffer_tracking.c | 119 +++++++--------------------------------
 1 file changed, 19 insertions(+), 100 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index c24e4a8..3a8b754 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -183,7 +183,7 @@ struct {
 };
 
 
-#define SINK_CRC_SIZE 12
+#define SINK_CRC_SIZE 6
 typedef struct {
 	char data[SINK_CRC_SIZE];
 } sink_crc_t;
@@ -327,28 +327,6 @@ drmModeModeInfo std_1024_mode = {
 	.name = "Custom 1024x768",
 };
 
-static drmModeModeInfoPtr get_connector_smallest_mode(drmModeConnectorPtr c)
-{
-	int i;
-	drmModeModeInfoPtr smallest = NULL;
-
-	for (i = 0; i < c->count_modes; i++) {
-		drmModeModeInfoPtr mode = &c->modes[i];
-
-		if (!smallest)
-			smallest = mode;
-
-		if (mode->hdisplay * mode->vdisplay <
-		    smallest->hdisplay * smallest->vdisplay)
-			smallest = mode;
-	}
-
-	if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
-		smallest = &std_1024_mode;
-
-	return smallest;
-}
-
 static drmModeConnectorPtr get_connector(uint32_t id)
 {
 	int i;
@@ -421,30 +399,6 @@ static void init_mode_params(struct modeset_params *params, uint32_t crtc_id,
 	params->sprite.h = 64;
 }
 
-static bool connector_get_mode(drmModeConnectorPtr c, drmModeModeInfoPtr *mode)
-{
-	*mode = NULL;
-
-	if (c->connection != DRM_MODE_CONNECTED || !c->count_modes)
-		return false;
-
-	if (c->connector_type == DRM_MODE_CONNECTOR_eDP && opt.no_edp)
-		return false;
-
-	if (opt.small_modes)
-		*mode = get_connector_smallest_mode(c);
-	else
-		*mode = &c->modes[0];
-
-	 /* On HSW the CRC WA is so awful that it makes you think everything is
-	  * bugged. */
-	if (IS_HASWELL(intel_get_drm_devid(drm.fd)) &&
-	    c->connector_type == DRM_MODE_CONNECTOR_eDP)
-		*mode = &std_1024_mode;
-
-	return true;
-}
-
 static bool connector_supports_pipe_a(drmModeConnectorPtr connector)
 {
 	int i;
@@ -473,7 +427,7 @@ static bool find_connector(bool edp_only, bool pipe_a, uint32_t forbidden_id,
 			continue;
 		if (c->connector_id == forbidden_id)
 			continue;
-		if (!connector_get_mode(c, &mode))
+		if (!igt_psr_find_good_mode(c, &mode))
 			continue;
 
 		*ret_connector = c;
@@ -804,23 +758,6 @@ static void fbc_print_status(void)
 	igt_info("FBC status:\n%s\n", buf);
 }
 
-static bool psr_is_enabled(void)
-{
-	char buf[256];
-
-	debugfs_read("i915_edp_psr_status", buf);
-	return strstr(buf, "\nActive: yes\n") &&
-	       strstr(buf, "\nHW Enabled & Active bit: yes\n");
-}
-
-static void psr_print_status(void)
-{
-	char buf[256];
-
-	debugfs_read("i915_edp_psr_status", buf);
-	igt_info("PSR status:\n%s\n", buf);
-}
-
 static struct timespec fbc_get_last_action(void)
 {
 	struct timespec ret = { 0, 0 };
@@ -926,44 +863,31 @@ static bool fbc_wait_until_enabled(void)
 	return igt_wait(fbc_is_enabled(), 2000, 1);
 }
 
-static bool psr_wait_until_enabled(void)
-{
-	return igt_wait(psr_is_enabled(), 5000, 1);
-}
-
 #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
 #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
-#define psr_enable() igt_set_module_param_int("enable_psr", 1)
-#define psr_disable() igt_set_module_param_int("enable_psr", 0)
 
 static void get_sink_crc(sink_crc_t *crc, bool mandatory)
 {
-	int rc, errno_;
+	int rc;
 
 	if (!sink_crc.supported) {
 		memcpy(crc, "unsupported!", SINK_CRC_SIZE);
 		return;
 	}
 
-	lseek(sink_crc.fd, 0, SEEK_SET);
-
-	rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE);
-	errno_ = errno;
-
-	if (rc == -1 && errno_ == ENOTTY) {
+	rc = igt_psr_get_sink_crc(sink_crc.fd, crc->data);
+	if (rc == ENOTTY) {
 		igt_info("Sink CRC not supported: panel doesn't support it\n");
 		sink_crc.supported = false;
-	} else if (rc == -1 && errno_ == ETIMEDOUT) {
-		if (sink_crc.reliable) {
-			igt_info("Sink CRC is unreliable on this machine.\n");
+	} else if (rc == ETIMEDOUT) {
+		if (sink_crc.reliable) 
 			sink_crc.reliable = false;
-		}
+		
 
 		if (mandatory)
 			igt_skip("Sink CRC is unreliable on this machine.\n");
 	} else {
-		igt_assert_f(rc != -1, "Unexpected error: %d\n", errno_);
-		igt_assert(rc == SINK_CRC_SIZE);
+		igt_assert_f(rc == 0, "Unexpected error: %d\n", rc);
 	}
 }
 
@@ -1180,7 +1104,7 @@ static void disable_features(const struct test_mode *t)
 		return;
 
 	fbc_disable();
-	psr_disable();
+	igt_psr_disable();
 }
 
 static void *busy_thread_func(void *data)
@@ -1432,7 +1356,7 @@ static void setup_sink_crc(void)
 	fill_fb_region(&prim_mode_params.fb, COLOR_PRIM_BG);
 	set_mode_for_params(&prim_mode_params);
 
-	sink_crc.fd = openat(drm.debugfs, "i915_sink_crc_eDP1", O_RDONLY);
+	sink_crc.fd = igt_psr_open_sink_crc(drm.debugfs);
 	igt_assert_lte(0, sink_crc.fd);
 
 	/* Do a first read to try to detect if it's supported. */
@@ -1547,14 +1471,6 @@ static void teardown_fbc(void)
 {
 }
 
-static bool psr_sink_has_support(void)
-{
-	char buf[256];
-
-	debugfs_read("i915_edp_psr_status", buf);
-	return strstr(buf, "Sink_Support: yes\n");
-}
-
 static void setup_psr(void)
 {
 	if (get_connector(prim_mode_params.connector_id)->connector_type !=
@@ -1563,7 +1479,7 @@ static void setup_psr(void)
 		return;
 	}
 
-	if (!psr_sink_has_support()) {
+	if (!igt_psr_sink_support(drm.fd)) {
 		igt_info("Can't test PSR: not supported by sink.\n");
 		return;
 	}
@@ -1717,12 +1633,15 @@ static int adjust_assertion_flags(const struct test_mode *t, int flags)
 	}								\
 									\
 	if (flags_ & ASSERT_PSR_ENABLED) {				\
-		if (!psr_wait_until_enabled()) {			\
-			psr_print_status();				\
+		if (!igt_psr_await_status(drm.fd, true)) {		\
+			igt_psr_print_status(drm.fd);			\
 			igt_assert_f(false, "PSR disabled\n");		\
 		}							\
 	} else if (flags_ & ASSERT_PSR_DISABLED) {			\
-		igt_assert(!psr_wait_until_enabled());			\
+		if (!igt_psr_await_status(drm.fd, false)) {		\
+			igt_psr_print_status(drm.fd);                   \
+			igt_assert_f(false, "PSR enabled\n");		\
+		}							\
 	}								\
 } while (0)
 
@@ -1822,7 +1741,7 @@ static void enable_features_for_test(const struct test_mode *t)
 	if (t->feature & FEATURE_FBC)
 		fbc_enable();
 	if (t->feature & FEATURE_PSR)
-		psr_enable();
+		igt_psr_enable();
 }
 
 static void check_test_requirements(const struct test_mode *t)
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH IGT v2 5/6] tests/kms_frontbuffer_tracking: Fix multidraw subtest

[Jim Bride]

The multidraw subtest was not taking whether or not the GEM buffer had
ever been in write-combining mode when checking for PSR state, so fix
that.

Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 tests/kms_frontbuffer_tracking.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 3a8b754..c52d7a0 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -2059,7 +2059,8 @@ static void multidraw_subtest(const struct test_mode *t)
 				assertions = used_method != IGT_DRAW_MMAP_GTT ?
 					     ASSERT_LAST_ACTION_CHANGED :
 					     ASSERT_NO_ACTION_CHANGE;
-				if (op_disables_psr(t, used_method))
+				if (op_disables_psr(t, used_method) &&
+				    !wc_used)
 					assertions |= ASSERT_PSR_DISABLED;
 
 				do_assertions(assertions);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH IGT v2 6/6] tests/kms_fbcon_fbt: Refactor to use IGT PSR library functions

[Jim Bride]

v2: * Minor functional tweaks and bug fixes
    * Rebase

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
---
 tests/kms_fbcon_fbt.c | 54 +++++++++++++++++----------------------------------
 1 file changed, 18 insertions(+), 36 deletions(-)

diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
index d009091..593adb9 100644
--- a/tests/kms_fbcon_fbt.c
+++ b/tests/kms_fbcon_fbt.c
@@ -103,8 +103,9 @@ static bool fbc_is_enabled(int fd)
 	return strstr(buf, "FBC enabled\n");
 }
 
-static bool fbc_wait_until_enabled(int fd)
+static bool fbc_wait_until_enabled(int fd, bool enabled)
 {
+	enabled = enabled;
 	return igt_wait(fbc_is_enabled(fd), 5000, 1);
 }
 
@@ -124,6 +125,13 @@ static void set_mode_for_one_screen(struct drm_info *drm, struct igt_fb *fb,
 
 		if (c->connection == DRM_MODE_CONNECTED && c->count_modes &&
 		    connector_possible(c)) {
+			if (c->connector_type == DRM_MODE_CONNECTOR_eDP) {
+				bool bret;
+
+				bret = igt_psr_find_good_mode(c, &mode);
+				if (bret)
+					break;
+			}
 			mode = &c->modes[0];
 			break;
 		}
@@ -147,35 +155,9 @@ static void set_mode_for_one_screen(struct drm_info *drm, struct igt_fb *fb,
 	igt_assert_eq(rc, 0);
 }
 
-static bool psr_supported_on_chipset(int fd)
-{
-	char buf[256];
-
-	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
-	return strstr(buf, "Sink_Support: yes\n");
-}
-
-static bool connector_can_psr(drmModeConnectorPtr connector)
-{
-	return (connector->connector_type == DRM_MODE_CONNECTOR_eDP);
-}
-
-static bool psr_is_enabled(int fd)
-{
-	char buf[256];
-
-	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
-	return strstr(buf, "\nActive: yes\n");
-}
-
-static bool psr_wait_until_enabled(int fd)
-{
-	return igt_wait(psr_is_enabled(fd), 5000, 1);
-}
-
 struct feature {
 	bool (*supported_on_chipset)(int fd);
-	bool (*wait_until_enabled)(int fd);
+	bool (*wait_until_enabled)(int fd, bool status);
 	bool (*connector_possible_fn)(drmModeConnectorPtr connector);
 	const char *param_name;
 } fbc = {
@@ -184,9 +166,9 @@ struct feature {
 	.connector_possible_fn = connector_can_fbc,
 	.param_name = "enable_fbc",
 }, psr = {
-	.supported_on_chipset = psr_supported_on_chipset,
-	.wait_until_enabled = psr_wait_until_enabled,
-	.connector_possible_fn = connector_can_psr,
+	.supported_on_chipset = igt_psr_sink_support,
+	.wait_until_enabled = igt_psr_await_status,
+	.connector_possible_fn = igt_psr_valid_connector,
 	.param_name = "enable_psr",
 };
 
@@ -210,17 +192,17 @@ static void subtest(struct feature *feature, bool suspend)
 
 	kmstest_unset_all_crtcs(drm.fd, drm.res);
 	wait_user("Modes unset.");
-	igt_assert(!feature->wait_until_enabled(drm.fd));
+	igt_assert(!feature->wait_until_enabled(drm.fd, true));
 
 	set_mode_for_one_screen(&drm, &fb, feature->connector_possible_fn);
 	wait_user("Screen set.");
-	igt_assert(feature->wait_until_enabled(drm.fd));
+	igt_assert(feature->wait_until_enabled(drm.fd, true));
 
 	if (suspend) {
 		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
 					      SUSPEND_TEST_NONE);
 		sleep(5);
-		igt_assert(feature->wait_until_enabled(drm.fd));
+		igt_assert(feature->wait_until_enabled(drm.fd, true));
 	}
 
 	igt_remove_fb(drm.fd, &fb);
@@ -230,13 +212,13 @@ static void subtest(struct feature *feature, bool suspend)
 	sleep(3);
 
 	wait_user("Back to fbcon.");
-	igt_assert(!feature->wait_until_enabled(drm.fd));
+	igt_assert(!feature->wait_until_enabled(drm.fd, true));
 
 	if (suspend) {
 		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
 					      SUSPEND_TEST_NONE);
 		sleep(5);
-		igt_assert(!feature->wait_until_enabled(drm.fd));
+		igt_assert(!feature->wait_until_enabled(drm.fd, true));
 	}
 }
 
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH IGT v2 1/6] tests/kms_psr_sink_crc: Change assert_or_manual() to a macro

[Rodrigo Vivi]

oh! at least one good reason why igt is full of macros! :(

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Fri, Jun 30, 2017 at 12:12 PM, Jim Bride <jim.bride@linux.intel.com> wrote:
> Make assert_or_manual() a macro so that we get accurate line number
> information when this assertion fails.
>
> v2: Rebase
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  tests/kms_psr_sink_crc.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
> index bd3fa5e..1a03719 100644
> --- a/tests/kms_psr_sink_crc.c
> +++ b/tests/kms_psr_sink_crc.c
> @@ -278,11 +278,11 @@ static bool is_green(char *crc)
>                 (bh & mask) == 0);
>  }
>
> -static void assert_or_manual(bool condition, const char *expected)
> -{
> -       igt_debug_manual_check("no-crc", expected);
> -       igt_assert(igt_interactive_debug || condition);
> -}
> +#define assert_or_manual(condition, expected)             \
> +do {                                                      \
> +       igt_debug_manual_check("no-crc", expected);       \
> +       igt_assert(igt_interactive_debug || condition);   \
> +} while (0)
>
>  static void run_test(data_t *data)
>  {
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH IGT v2 2/6] lib: Add PSR utility functions to igt library.

[Rodrigo Vivi]

On Fri, Jun 30, 2017 at 12:12 PM, Jim Bride <jim.bride@linux.intel.com> wrote:
> Factor out some code that was replicated in three test utilities into
> some new IGT library functions so that we are checking PSR status in
> a consistent fashion across all of our PSR tests.
>
> v2: * Add sink CRC helper function
>     * Misc. bug fixes
>     * Rebase
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  lib/Makefile.sources |   2 +
>  lib/igt.h            |   1 +
>  lib/igt_psr.c        | 235 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_psr.h        |  43 ++++++++++
>  4 files changed, 281 insertions(+)
>  create mode 100644 lib/igt_psr.c
>  create mode 100644 lib/igt_psr.h
>
> diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> index 53fdb54..6a73c8c 100644
> --- a/lib/Makefile.sources
> +++ b/lib/Makefile.sources
> @@ -83,6 +83,8 @@ lib_source_list =             \
>         uwildmat/uwildmat.c     \
>         igt_kmod.c              \
>         igt_kmod.h              \
> +       igt_psr.c               \
> +       igt_psr.h               \
>         $(NULL)
>
>  .PHONY: version.h.tmp
> diff --git a/lib/igt.h b/lib/igt.h
> index a069deb..0b8e3a8 100644
> --- a/lib/igt.h
> +++ b/lib/igt.h
> @@ -37,6 +37,7 @@
>  #include "igt_gt.h"
>  #include "igt_kms.h"
>  #include "igt_pm.h"
> +#include "igt_psr.h"
>  #include "igt_stats.h"
>  #ifdef HAVE_CHAMELIUM
>  #include "igt_chamelium.h"
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> new file mode 100644
> index 0000000..a2823bf
> --- /dev/null
> +++ b/lib/igt_psr.c
> @@ -0,0 +1,235 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "igt.h"
> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <sys/types.h>
> +#include <string.h>
> +#include <fcntl.h>
> +
> +/**
> + * SECTION:igt_psr
> + * @short_description: Panel Self Refresh helpers
> + * @title: Panel Self Refresh
> + * @include: igt.h
> + *
> + * This library provides various helpers to enable Panel Self Refresh,
> + * as well as to check the state of PSR on the system (enabled vs.
> + * disabled, active vs. inactive) or to wait for PSR to be active
> + * or inactive.
> + */
> +
> +/**
> + * igt_psr_source_support:
> + *
> + * Returns true if the source supports PSR.
> + */
> +bool igt_psr_source_support(int fd)
> +{
> +       char buf[512];
> +
> +       igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> +
> +       return strstr(buf, "Source_OK: yes\n");
> +}
> +
> +
> +/**
> + * igt_psr_sink_support:
> + *
> + * Returns true if the current eDP panel supports PSR.
> + */
> +bool igt_psr_sink_support(int fd)
> +{
> +       char buf[256];
> +
> +       igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> +       return strstr(buf, "Sink_Support: yes\n");
> +}
> +
> +/**
> + * igt_psr_possible:
> + *
> + * Returns true if both the source and sink support PSR.
> + */
> +bool igt_psr_possible(int fd)
> +{
> +       char buf[512];
> +
> +       igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> +
> +       return igt_psr_source_support(fd) && igt_psr_sink_support(fd);
> +}
> +
> +/**
> + * igt_psr_active:
> + *
> + * Returns true if PSR is active on the panel.
> + */
> +bool igt_psr_active(int fd)
> +{
> +       char buf[512];
> +       bool actret = false;
> +       bool hwactret = false;
> +
> +       igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> +       hwactret = (strstr(buf, "HW Enabled & Active bit: yes\n") != NULL);
> +       actret = (strstr(buf, "Active: yes\n") != NULL);
> +       igt_debug("hwactret: %s actret: %s\n", hwactret ? "true" : "false",
> +                actret ? "true" : "false");
> +       return hwactret && actret;
> +}
> +
> +/**
> + * igt_psr_await_status:
> + * @active: A boolean that causes the function to wait for PSR to activate
> + *          if set to true, or to wait for PSR to deactivate if false.
> + *
> + * Returns true if the requested condition is met.
> + */
> +bool igt_psr_await_status(int fd, bool active)
> +{
> +       const int timeout = 10;
> +       int count = 0;
> +       while (count < timeout) {
> +               if (igt_psr_active(fd) == active) {
> +                       igt_debug("PSR %s after %d seconds.\n",
> +                                 active ? "Active" : "Inactive", count);
> +                       return true;
> +               }
> +               count++;
> +               sleep(1);
> +       }
> +       return false;
> +}
> +
> +/**
> + * igt_psr_enabled:
> + *
> + * Returns true if the PSR feature is enabled.
> + */
> +bool igt_psr_enabled(int fd)
> +{
> +       char buf[256];
> +
> +       igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> +       return strstr(buf, "Enabled: yes\n");
> +}
> +
> +/**
> + * igt_psr_print_status:
> + *
> + * Dumps the contents of i915_edp_psr_status from debugfs.
> + */
> +void igt_psr_print_status(int fd)
> +{
> +        char buf[256];
> +
> +        igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> +        igt_info("PSR status:\n%s\n", buf);
> +}
> +
> +/**
> + * igt_psr_valid_connector:
> + * @connector: a drmModeConnector pointer to check
> + *
> + * Returns true if connector is an eDP connector.
> + */
> +bool igt_psr_valid_connector(drmModeConnectorPtr connector)
> +{
> +       return (connector->connector_type == DRM_MODE_CONNECTOR_eDP);
> +}
> +
> +/**
> + * igt_psr_find_good_mode
> + * @connector: a drmModeConnector pointer to find the mode on
> + * @mode: a drmModeModePtr pointer that is set to the matching mode
> + *
> + * Returns true (and populates *mode with the match) if a valid
> + * PSR mdoe is found, and false otherwise.
> + */
> +bool igt_psr_find_good_mode(drmModeConnectorPtr connector,
> +                           drmModeModeInfoPtr *mode)
> +{
> +       int i;
> +
> +       if (!connector->count_modes) {
> +               igt_warn("no modes for connector %d.\n",
> +                        connector->connector_id);
> +               return false;
> +       } else {
> +               igt_debug("Connector has %d modes.\n", connector->count_modes);
> +       }
> +
> +       for (i = 0; i < connector->count_modes; i++) {
> +               if ((connector->modes[i].vtotal -
> +                    connector->modes[i].vdisplay) >= 36) {

why 36?

doesn't this depend on the panel setup time and on mode resolution?

> +                       igt_debug("Mode %d good for PSR.\n", i);
> +                       *mode = &(connector->modes[i]);
> +                       return true;
> +               } else {
> +                       igt_debug("Throwing out mode %d\n", i);
> +               }
> +       }
> +       return false;
> +}
> +
> +int igt_psr_open_sink_crc(int drm_fd)
> +{
> +       int crc_fd;
> +
> +       crc_fd = openat(drm_fd, "i915_sink_crc_eDP1", O_RDONLY);
> +       igt_assert_lte(0, crc_fd);
> +       return crc_fd;
> +}
> +
> +
> +int igt_psr_get_sink_crc(int fd, char *data)
> +{
> +       int rc, errno_;
> +       char buf[13];
> +
> +       memset(buf, 0, 13);
> +       lseek(fd, 0, SEEK_SET);
> +
> +       rc = read(fd, buf, 12);
> +       errno_ = errno;
> +
> +       if (rc == -1) {
> +               if (errno_ == ENOTTY)
> +                       igt_info("Sink CRC not supported: panel doesn't support it\n");
> +               else if (errno_ != ETIMEDOUT)
> +                       igt_assert_f(rc != -1, "Unexpected error: %d\n",
> +                                    errno_);
> +               return errno_;
> +       } else {
> +               int i;
> +               unsigned long long val = strtoull(buf, NULL, 16);
> +
> +               for (i = 5; i >= 0; i--, val >>= 8) {
> +                       data[i] = (unsigned char) (val & 0xff);
> +               }
> +               return 0;
> +       }
> +}
> diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> new file mode 100644
> index 0000000..d94cdfa
> --- /dev/null
> +++ b/lib/igt_psr.h
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#ifndef IGT_PSR_H
> +#define IGT_PSR_H
> +
> +#define igt_psr_enable() igt_set_module_param_int("enable_psr", 1)
> +#define igt_psr_disable() igt_set_module_param_int("enable_psr", 0)
> +
> +bool igt_psr_sink_support(int fd);
> +bool igt_psr_source_support(int fd);
> +bool igt_psr_possible(int fd);
> +bool igt_psr_active(int fd);
> +bool igt_psr_await_status(int fd, bool active);
> +bool igt_psr_enabled(int fd);
> +void igt_psr_print_status(int fd);
> +bool igt_psr_valid_connector(drmModeConnectorPtr connector);
> +bool igt_psr_find_good_mode(drmModeConnectorPtr connector,
> +                           drmModeModeInfoPtr *mode);
> +int igt_psr_open_sink_crc(int drm_fd);
> +int igt_psr_get_sink_crc(int fd, char *data);
> +
> +#endif /* IGT_PSR_H */
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH IGT v2 4/6] tests/kms_frontbuffer_tracking: Refactor to use IGT PSR library functions

[Rodrigo Vivi]

I believe it would be better to create the psr lib with only one
function at time and on every patch that adds the new function already
removes that from here and from frontbuffer tracking test as well...

easier to review and to make sure that we are not changing the behaviour.

also...

On Fri, Jun 30, 2017 at 12:12 PM, Jim Bride <jim.bride@linux.intel.com> wrote:
> v2: * Minor functional tweaks and bug fixes
>     * Rebase
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 119 +++++++--------------------------------
>  1 file changed, 19 insertions(+), 100 deletions(-)
>
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index c24e4a8..3a8b754 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -183,7 +183,7 @@ struct {
>  };
>
>
> -#define SINK_CRC_SIZE 12
> +#define SINK_CRC_SIZE 6

I believe this deserves a separated patch...

>  typedef struct {
>         char data[SINK_CRC_SIZE];
>  } sink_crc_t;
> @@ -327,28 +327,6 @@ drmModeModeInfo std_1024_mode = {
>         .name = "Custom 1024x768",
>  };
>
> -static drmModeModeInfoPtr get_connector_smallest_mode(drmModeConnectorPtr c)
> -{
> -       int i;
> -       drmModeModeInfoPtr smallest = NULL;
> -
> -       for (i = 0; i < c->count_modes; i++) {
> -               drmModeModeInfoPtr mode = &c->modes[i];
> -
> -               if (!smallest)
> -                       smallest = mode;
> -
> -               if (mode->hdisplay * mode->vdisplay <
> -                   smallest->hdisplay * smallest->vdisplay)
> -                       smallest = mode;
> -       }
> -
> -       if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
> -               smallest = &std_1024_mode;
> -
> -       return smallest;
> -}
> -
>  static drmModeConnectorPtr get_connector(uint32_t id)
>  {
>         int i;
> @@ -421,30 +399,6 @@ static void init_mode_params(struct modeset_params *params, uint32_t crtc_id,
>         params->sprite.h = 64;
>  }
>
> -static bool connector_get_mode(drmModeConnectorPtr c, drmModeModeInfoPtr *mode)
> -{
> -       *mode = NULL;
> -
> -       if (c->connection != DRM_MODE_CONNECTED || !c->count_modes)
> -               return false;
> -
> -       if (c->connector_type == DRM_MODE_CONNECTOR_eDP && opt.no_edp)
> -               return false;
> -
> -       if (opt.small_modes)
> -               *mode = get_connector_smallest_mode(c);
> -       else
> -               *mode = &c->modes[0];
> -
> -        /* On HSW the CRC WA is so awful that it makes you think everything is
> -         * bugged. */
> -       if (IS_HASWELL(intel_get_drm_devid(drm.fd)) &&
> -           c->connector_type == DRM_MODE_CONNECTOR_eDP)
> -               *mode = &std_1024_mode;
> -
> -       return true;
> -}
> -
>  static bool connector_supports_pipe_a(drmModeConnectorPtr connector)
>  {
>         int i;
> @@ -473,7 +427,7 @@ static bool find_connector(bool edp_only, bool pipe_a, uint32_t forbidden_id,
>                         continue;
>                 if (c->connector_id == forbidden_id)
>                         continue;
> -               if (!connector_get_mode(c, &mode))
> +               if (!igt_psr_find_good_mode(c, &mode))
>                         continue;
>
>                 *ret_connector = c;
> @@ -804,23 +758,6 @@ static void fbc_print_status(void)
>         igt_info("FBC status:\n%s\n", buf);
>  }
>
> -static bool psr_is_enabled(void)
> -{
> -       char buf[256];
> -
> -       debugfs_read("i915_edp_psr_status", buf);
> -       return strstr(buf, "\nActive: yes\n") &&
> -              strstr(buf, "\nHW Enabled & Active bit: yes\n");
> -}
> -
> -static void psr_print_status(void)
> -{
> -       char buf[256];
> -
> -       debugfs_read("i915_edp_psr_status", buf);
> -       igt_info("PSR status:\n%s\n", buf);
> -}
> -
>  static struct timespec fbc_get_last_action(void)
>  {
>         struct timespec ret = { 0, 0 };
> @@ -926,44 +863,31 @@ static bool fbc_wait_until_enabled(void)
>         return igt_wait(fbc_is_enabled(), 2000, 1);
>  }
>
> -static bool psr_wait_until_enabled(void)
> -{
> -       return igt_wait(psr_is_enabled(), 5000, 1);
> -}
> -
>  #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
>  #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
> -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
> -#define psr_disable() igt_set_module_param_int("enable_psr", 0)
>
>  static void get_sink_crc(sink_crc_t *crc, bool mandatory)
>  {
> -       int rc, errno_;
> +       int rc;
>
>         if (!sink_crc.supported) {
>                 memcpy(crc, "unsupported!", SINK_CRC_SIZE);

this doesn't fit anymore

>                 return;
>         }
>
> -       lseek(sink_crc.fd, 0, SEEK_SET);
> -
> -       rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE);
> -       errno_ = errno;
> -
> -       if (rc == -1 && errno_ == ENOTTY) {
> +       rc = igt_psr_get_sink_crc(sink_crc.fd, crc->data);
> +       if (rc == ENOTTY) {
>                 igt_info("Sink CRC not supported: panel doesn't support it\n");
>                 sink_crc.supported = false;
> -       } else if (rc == -1 && errno_ == ETIMEDOUT) {
> -               if (sink_crc.reliable) {
> -                       igt_info("Sink CRC is unreliable on this machine.\n");
> +       } else if (rc == ETIMEDOUT) {
> +               if (sink_crc.reliable)
>                         sink_crc.reliable = false;
> -               }
> +
>
>                 if (mandatory)
>                         igt_skip("Sink CRC is unreliable on this machine.\n");
>         } else {
> -               igt_assert_f(rc != -1, "Unexpected error: %d\n", errno_);
> -               igt_assert(rc == SINK_CRC_SIZE);
> +               igt_assert_f(rc == 0, "Unexpected error: %d\n", rc);
>         }
>  }
>
> @@ -1180,7 +1104,7 @@ static void disable_features(const struct test_mode *t)
>                 return;
>
>         fbc_disable();
> -       psr_disable();
> +       igt_psr_disable();
>  }
>
>  static void *busy_thread_func(void *data)
> @@ -1432,7 +1356,7 @@ static void setup_sink_crc(void)
>         fill_fb_region(&prim_mode_params.fb, COLOR_PRIM_BG);
>         set_mode_for_params(&prim_mode_params);
>
> -       sink_crc.fd = openat(drm.debugfs, "i915_sink_crc_eDP1", O_RDONLY);
> +       sink_crc.fd = igt_psr_open_sink_crc(drm.debugfs);
>         igt_assert_lte(0, sink_crc.fd);
>
>         /* Do a first read to try to detect if it's supported. */
> @@ -1547,14 +1471,6 @@ static void teardown_fbc(void)
>  {
>  }
>
> -static bool psr_sink_has_support(void)
> -{
> -       char buf[256];
> -
> -       debugfs_read("i915_edp_psr_status", buf);
> -       return strstr(buf, "Sink_Support: yes\n");
> -}
> -
>  static void setup_psr(void)
>  {
>         if (get_connector(prim_mode_params.connector_id)->connector_type !=
> @@ -1563,7 +1479,7 @@ static void setup_psr(void)
>                 return;
>         }
>
> -       if (!psr_sink_has_support()) {
> +       if (!igt_psr_sink_support(drm.fd)) {
>                 igt_info("Can't test PSR: not supported by sink.\n");
>                 return;
>         }
> @@ -1717,12 +1633,15 @@ static int adjust_assertion_flags(const struct test_mode *t, int flags)
>         }                                                               \
>                                                                         \
>         if (flags_ & ASSERT_PSR_ENABLED) {                              \
> -               if (!psr_wait_until_enabled()) {                        \
> -                       psr_print_status();                             \
> +               if (!igt_psr_await_status(drm.fd, true)) {              \
> +                       igt_psr_print_status(drm.fd);                   \
>                         igt_assert_f(false, "PSR disabled\n");          \
>                 }                                                       \
>         } else if (flags_ & ASSERT_PSR_DISABLED) {                      \
> -               igt_assert(!psr_wait_until_enabled());                  \
> +               if (!igt_psr_await_status(drm.fd, false)) {             \
> +                       igt_psr_print_status(drm.fd);                   \
> +                       igt_assert_f(false, "PSR enabled\n");           \
> +               }                                                       \
>         }                                                               \
>  } while (0)
>
> @@ -1822,7 +1741,7 @@ static void enable_features_for_test(const struct test_mode *t)
>         if (t->feature & FEATURE_FBC)
>                 fbc_enable();
>         if (t->feature & FEATURE_PSR)
> -               psr_enable();
> +               igt_psr_enable();
>  }
>
>  static void check_test_requirements(const struct test_mode *t)
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH IGT v2 5/6] tests/kms_frontbuffer_tracking: Fix multidraw subtest

[Rodrigo Vivi]

cc: Paulo Zanoni...

On Fri, Jun 30, 2017 at 12:12 PM, Jim Bride <jim.bride@linux.intel.com> wrote:
> The multidraw subtest was not taking whether or not the GEM buffer had
> ever been in write-combining mode when checking for PSR state, so fix
> that.
>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index 3a8b754..c52d7a0 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -2059,7 +2059,8 @@ static void multidraw_subtest(const struct test_mode *t)
>                                 assertions = used_method != IGT_DRAW_MMAP_GTT ?
>                                              ASSERT_LAST_ACTION_CHANGED :
>                                              ASSERT_NO_ACTION_CHANGE;
> -                               if (op_disables_psr(t, used_method))
> +                               if (op_disables_psr(t, used_method) &&
> +                                   !wc_used)
>                                         assertions |= ASSERT_PSR_DISABLED;
>
>                                 do_assertions(assertions);
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH IGT v2 4/6] tests/kms_frontbuffer_tracking: Refactor to use IGT PSR library functions

[Paulo Zanoni]

Em Sex, 2017-06-30 às 12:12 -0700, Jim Bride escreveu:
> v2: * Minor functional tweaks and bug fixes
>     * Rebase
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>

This patch is not just a refactor. It changes a how the code behaves.
Please split it into many patches: one patch replaces code with
equivalent code from the PSR library, and the other patches should each
do one of the actual code changes that this patch does.

> ---
>  tests/kms_frontbuffer_tracking.c | 119 +++++++--------------------
> ------------
>  1 file changed, 19 insertions(+), 100 deletions(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index c24e4a8..3a8b754 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -183,7 +183,7 @@ struct {
>  };
>  
>  
> -#define SINK_CRC_SIZE 12
> +#define SINK_CRC_SIZE 6

As Rodrigo mentioned, this is going to break.


>  typedef struct {
>  	char data[SINK_CRC_SIZE];
>  } sink_crc_t;
> @@ -327,28 +327,6 @@ drmModeModeInfo std_1024_mode = {
>  	.name = "Custom 1024x768",
>  };
>  
> -static drmModeModeInfoPtr
> get_connector_smallest_mode(drmModeConnectorPtr c)
> -{
> -	int i;
> -	drmModeModeInfoPtr smallest = NULL;
> -
> -	for (i = 0; i < c->count_modes; i++) {
> -		drmModeModeInfoPtr mode = &c->modes[i];
> -
> -		if (!smallest)
> -			smallest = mode;
> -
> -		if (mode->hdisplay * mode->vdisplay <
> -		    smallest->hdisplay * smallest->vdisplay)
> -			smallest = mode;
> -	}
> -
> -	if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
> -		smallest = &std_1024_mode;
> -
> -	return smallest;
> -}
> -
>  static drmModeConnectorPtr get_connector(uint32_t id)
>  {
>  	int i;
> @@ -421,30 +399,6 @@ static void init_mode_params(struct
> modeset_params *params, uint32_t crtc_id,
>  	params->sprite.h = 64;
>  }
>  
> -static bool connector_get_mode(drmModeConnectorPtr c,
> drmModeModeInfoPtr *mode)
> -{
> -	*mode = NULL;
> -
> -	if (c->connection != DRM_MODE_CONNECTED || !c->count_modes)
> -		return false;
> -
> -	if (c->connector_type == DRM_MODE_CONNECTOR_eDP &&
> opt.no_edp)
> -		return false;
> -
> -	if (opt.small_modes)
> -		*mode = get_connector_smallest_mode(c);
> -	else
> -		*mode = &c->modes[0];
> -
> -	 /* On HSW the CRC WA is so awful that it makes you think
> everything is
> -	  * bugged. */
> -	if (IS_HASWELL(intel_get_drm_devid(drm.fd)) &&
> -	    c->connector_type == DRM_MODE_CONNECTOR_eDP)
> -		*mode = &std_1024_mode;
> -
> -	return true;
> -}
> -
>  static bool connector_supports_pipe_a(drmModeConnectorPtr connector)
>  {
>  	int i;
> @@ -473,7 +427,7 @@ static bool find_connector(bool edp_only, bool
> pipe_a, uint32_t forbidden_id,
>  			continue;
>  		if (c->connector_id == forbidden_id)
>  			continue;
> -		if (!connector_get_mode(c, &mode))
> +		if (!igt_psr_find_good_mode(c, &mode))

igt_psr_find_good_mode() doesn't seem to do what connector_get_mode()
does, we can't use it. This is going to introduce bugs.


>  			continue;
>  
>  		*ret_connector = c;
> @@ -804,23 +758,6 @@ static void fbc_print_status(void)
>  	igt_info("FBC status:\n%s\n", buf);
>  }
>  
> -static bool psr_is_enabled(void)
> -{
> -	char buf[256];
> -
> -	debugfs_read("i915_edp_psr_status", buf);
> -	return strstr(buf, "\nActive: yes\n") &&
> -	       strstr(buf, "\nHW Enabled & Active bit: yes\n");
> -}
> -
> -static void psr_print_status(void)
> -{
> -	char buf[256];
> -
> -	debugfs_read("i915_edp_psr_status", buf);
> -	igt_info("PSR status:\n%s\n", buf);
> -}
> -
>  static struct timespec fbc_get_last_action(void)
>  {
>  	struct timespec ret = { 0, 0 };
> @@ -926,44 +863,31 @@ static bool fbc_wait_until_enabled(void)
>  	return igt_wait(fbc_is_enabled(), 2000, 1);
>  }
>  
> -static bool psr_wait_until_enabled(void)
> -{
> -	return igt_wait(psr_is_enabled(), 5000, 1);
> -}
> -
>  #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
>  #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
> -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
> -#define psr_disable() igt_set_module_param_int("enable_psr", 0)
>  
>  static void get_sink_crc(sink_crc_t *crc, bool mandatory)
>  {
> -	int rc, errno_;
> +	int rc;
>  
>  	if (!sink_crc.supported) {
>  		memcpy(crc, "unsupported!", SINK_CRC_SIZE);
>  		return;
>  	}
>  
> -	lseek(sink_crc.fd, 0, SEEK_SET);
> -
> -	rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE);
> -	errno_ = errno;
> -
> -	if (rc == -1 && errno_ == ENOTTY) {
> +	rc = igt_psr_get_sink_crc(sink_crc.fd, crc->data);
> +	if (rc == ENOTTY) {
>  		igt_info("Sink CRC not supported: panel doesn't
> support it\n");

We'll print this twice now.


>  		sink_crc.supported = false;
> -	} else if (rc == -1 && errno_ == ETIMEDOUT) {
> -		if (sink_crc.reliable) {
> -			igt_info("Sink CRC is unreliable on this
> machine.\n");
> +	} else if (rc == ETIMEDOUT) {
> +		if (sink_crc.reliable) 
>  			sink_crc.reliable = false;
> -		}
> +		
>  

There are some problems with white spaces in the lines above: a double
blank line and trailing spaces at the end of lines.


>  		if (mandatory)
>  			igt_skip("Sink CRC is unreliable on this
> machine.\n");
>  	} else {
> -		igt_assert_f(rc != -1, "Unexpected error: %d\n",
> errno_);
> -		igt_assert(rc == SINK_CRC_SIZE);
> +		igt_assert_f(rc == 0, "Unexpected error: %d\n", rc);
>  	}
>  }
>  
> @@ -1180,7 +1104,7 @@ static void disable_features(const struct
> test_mode *t)
>  		return;
>  
>  	fbc_disable();
> -	psr_disable();
> +	igt_psr_disable();
>  }
>  
>  static void *busy_thread_func(void *data)
> @@ -1432,7 +1356,7 @@ static void setup_sink_crc(void)
>  	fill_fb_region(&prim_mode_params.fb, COLOR_PRIM_BG);
>  	set_mode_for_params(&prim_mode_params);
>  
> -	sink_crc.fd = openat(drm.debugfs, "i915_sink_crc_eDP1",
> O_RDONLY);
> +	sink_crc.fd = igt_psr_open_sink_crc(drm.debugfs);
>  	igt_assert_lte(0, sink_crc.fd);
>  
>  	/* Do a first read to try to detect if it's supported. */
> @@ -1547,14 +1471,6 @@ static void teardown_fbc(void)
>  {
>  }
>  
> -static bool psr_sink_has_support(void)
> -{
> -	char buf[256];
> -
> -	debugfs_read("i915_edp_psr_status", buf);
> -	return strstr(buf, "Sink_Support: yes\n");
> -}
> -
>  static void setup_psr(void)
>  {
>  	if (get_connector(prim_mode_params.connector_id)-
> >connector_type !=
> @@ -1563,7 +1479,7 @@ static void setup_psr(void)
>  		return;
>  	}
>  
> -	if (!psr_sink_has_support()) {
> +	if (!igt_psr_sink_support(drm.fd)) {
>  		igt_info("Can't test PSR: not supported by
> sink.\n");
>  		return;
>  	}
> @@ -1717,12 +1633,15 @@ static int adjust_assertion_flags(const
> struct test_mode *t, int flags)
>  	}								
> \
>  									
> \
>  	if (flags_ & ASSERT_PSR_ENABLED) {				
> \
> -		if (!psr_wait_until_enabled()) {			
> \
> -			psr_print_status();				
> \
> +		if (!igt_psr_await_status(drm.fd, true)) {		
> \
> +			igt_psr_print_status(drm.fd);		
> 	\
>  			igt_assert_f(false, "PSR disabled\n");	
> 	\
>  		}							
> \
>  	} else if (flags_ & ASSERT_PSR_DISABLED) {			
> \
> -		igt_assert(!psr_wait_until_enabled());		
> 	\
> +		if (!igt_psr_await_status(drm.fd, false)) {		
> \
> +			igt_psr_print_status(drm.fd);               
>     \
> +			igt_assert_f(false, "PSR enabled\n");	
> 	\
> +		}							
> \
>  	}								
> \
>  } while (0)
>  
> @@ -1822,7 +1741,7 @@ static void enable_features_for_test(const
> struct test_mode *t)
>  	if (t->feature & FEATURE_FBC)
>  		fbc_enable();
>  	if (t->feature & FEATURE_PSR)
> -		psr_enable();
> +		igt_psr_enable();
>  }
>  
>  static void check_test_requirements(const struct test_mode *t)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH IGT v2 2/6] lib: Add PSR utility functions to igt library.

[Paulo Zanoni]

Em Sex, 2017-06-30 às 12:12 -0700, Jim Bride escreveu:
> Factor out some code that was replicated in three test utilities into
> some new IGT library functions so that we are checking PSR status in
> a consistent fashion across all of our PSR tests.
> 
> v2: * Add sink CRC helper function
>     * Misc. bug fixes
>     * Rebase
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  lib/Makefile.sources |   2 +
>  lib/igt.h            |   1 +
>  lib/igt_psr.c        | 235
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_psr.h        |  43 ++++++++++
>  4 files changed, 281 insertions(+)
>  create mode 100644 lib/igt_psr.c
>  create mode 100644 lib/igt_psr.h
> 
> diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> index 53fdb54..6a73c8c 100644
> --- a/lib/Makefile.sources
> +++ b/lib/Makefile.sources
> @@ -83,6 +83,8 @@ lib_source_list =	 	\
>  	uwildmat/uwildmat.c	\
>  	igt_kmod.c		\
>  	igt_kmod.h		\
> +	igt_psr.c		\
> +	igt_psr.h		\
>  	$(NULL)
>  
>  .PHONY: version.h.tmp
> diff --git a/lib/igt.h b/lib/igt.h
> index a069deb..0b8e3a8 100644
> --- a/lib/igt.h
> +++ b/lib/igt.h
> @@ -37,6 +37,7 @@
>  #include "igt_gt.h"
>  #include "igt_kms.h"
>  #include "igt_pm.h"
> +#include "igt_psr.h"
>  #include "igt_stats.h"
>  #ifdef HAVE_CHAMELIUM
>  #include "igt_chamelium.h"
> diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> new file mode 100644
> index 0000000..a2823bf
> --- /dev/null
> +++ b/lib/igt_psr.c
> @@ -0,0 +1,235 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person
> obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> the
> + * Software is furnished to do so, subject to the following
> conditions:
> + *
> + * The above copyright notice and this permission notice (including
> the next
> + * paragraph) shall be included in all copies or substantial
> portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "igt.h"
> +#include <stdio.h>
> +#include <stdbool.h>
> +#include <sys/types.h>
> +#include <string.h>
> +#include <fcntl.h>
> +
> +/**
> + * SECTION:igt_psr
> + * @short_description: Panel Self Refresh helpers
> + * @title: Panel Self Refresh
> + * @include: igt.h
> + *
> + * This library provides various helpers to enable Panel Self
> Refresh,
> + * as well as to check the state of PSR on the system (enabled vs.
> + * disabled, active vs. inactive) or to wait for PSR to be active
> + * or inactive.
> + */
> +
> +/**
> + * igt_psr_source_support:
> + *
> + * Returns true if the source supports PSR.
> + */
> +bool igt_psr_source_support(int fd)
> +{
> +	char buf[512];
> +
> +	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> +
> +	return strstr(buf, "Source_OK: yes\n");
> +}
> +
> +
> +/**
> + * igt_psr_sink_support:
> + *
> + * Returns true if the current eDP panel supports PSR.
> + */
> +bool igt_psr_sink_support(int fd)
> +{
> +	char buf[256];

Why are some buffers 512 while the others are 256? They're both for the
same file.

> +
> +	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> +	return strstr(buf, "Sink_Support: yes\n");
> +}
> +
> +/**
> + * igt_psr_possible:
> + *
> + * Returns true if both the source and sink support PSR.
> + */
> +bool igt_psr_possible(int fd)
> +{
> +	char buf[512];
> +
> +	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> +
> +	return igt_psr_source_support(fd) &&
> igt_psr_sink_support(fd);
> +}
> +
> +/**
> + * igt_psr_active:
> + *
> + * Returns true if PSR is active on the panel.
> + */
> +bool igt_psr_active(int fd)
> +{
> +	char buf[512];
> +	bool actret = false;
> +	bool hwactret = false;
> +
> +	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> +	hwactret = (strstr(buf, "HW Enabled & Active bit: yes\n") !=
> NULL);
> +	actret = (strstr(buf, "Active: yes\n") != NULL);
> +	igt_debug("hwactret: %s actret: %s\n", hwactret ? "true" :
> "false",
> +		 actret ? "true" : "false");
> +	return hwactret && actret;
> +}
> +
> +/**
> + * igt_psr_await_status:
> + * @active: A boolean that causes the function to wait for PSR to
> activate
> + *          if set to true, or to wait for PSR to deactivate if
> false.
> + *
> + * Returns true if the requested condition is met.
> + */
> +bool igt_psr_await_status(int fd, bool active)
> +{
> +	const int timeout = 10;
> +	int count = 0;
> +	while (count < timeout) {
> +		if (igt_psr_active(fd) == active) {
> +			igt_debug("PSR %s after %d seconds.\n",
> +				  active ? "Active" : "Inactive",
> count);
> +			return true;
> +		}
> +		count++;
> +		sleep(1);
> +	}
> +	return false;

Since this is in a lib now, can you please make use of igt_wait or just
make it work as expected when the signal helper is active?


> +}
> +
> +/**
> + * igt_psr_enabled:
> + *
> + * Returns true if the PSR feature is enabled.
> + */
> +bool igt_psr_enabled(int fd)
> +{
> +	char buf[256];
> +
> +	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> +	return strstr(buf, "Enabled: yes\n");
> +}
> +
> +/**
> + * igt_psr_print_status:
> + *
> + * Dumps the contents of i915_edp_psr_status from debugfs.
> + */
> +void igt_psr_print_status(int fd)
> +{
> +        char buf[256];
> +
> +        igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> +        igt_info("PSR status:\n%s\n", buf);
> +}
> +
> +/**
> + * igt_psr_valid_connector:
> + * @connector: a drmModeConnector pointer to check
> + *
> + * Returns true if connector is an eDP connector.
> + */
> +bool igt_psr_valid_connector(drmModeConnectorPtr connector)
> +{
> +	return (connector->connector_type ==
> DRM_MODE_CONNECTOR_eDP);
> +}
> +
> +/**
> + * igt_psr_find_good_mode
> + * @connector: a drmModeConnector pointer to find the mode on
> + * @mode: a drmModeModePtr pointer that is set to the matching mode
> + *
> + * Returns true (and populates *mode with the match) if a valid
> + * PSR mdoe is found, and false otherwise.
> + */
> +bool igt_psr_find_good_mode(drmModeConnectorPtr connector,
> +			    drmModeModeInfoPtr *mode)
> +{
> +	int i;
> +
> +	if (!connector->count_modes) {
> +		igt_warn("no modes for connector %d.\n",
> +			 connector->connector_id);
> +		return false;
> +	} else {
> +		igt_debug("Connector has %d modes.\n", connector-
> >count_modes);
> +	}
> +
> +	for (i = 0; i < connector->count_modes; i++) {
> +		if ((connector->modes[i].vtotal -
> +		     connector->modes[i].vdisplay) >= 36) {
> +			igt_debug("Mode %d good for PSR.\n", i);
> +			*mode = &(connector->modes[i]);
> +			return true;

Since the callers may be trying to find modes that fit other
restrictions (e.g., kms_frontbuffer_tracking) I'd suggest you to
extract igt_psr_mode_is_compatible(mode) with the vtotal/vdisplay
check.


> +		} else {
> +			igt_debug("Throwing out mode %d\n", i);
> +		}
> +	}
> +	return false;
> +}
> +
> +int igt_psr_open_sink_crc(int drm_fd)
> +{
> +	int crc_fd;
> +
> +	crc_fd = openat(drm_fd, "i915_sink_crc_eDP1", O_RDONLY);
> +	igt_assert_lte(0, crc_fd);
> +	return crc_fd;
> +}
> +
> +

Please add documentation for this one, especially around the
expectation for "data".

> +int igt_psr_get_sink_crc(int fd, char *data)
> +{
> +	int rc, errno_;
> +	char buf[13];
> +
> +	memset(buf, 0, 13);
> +	lseek(fd, 0, SEEK_SET);
> +
> +	rc = read(fd, buf, 12);
> +	errno_ = errno;
> +
> +	if (rc == -1) {
> +		if (errno_ == ENOTTY)
> +			igt_info("Sink CRC not supported: panel
> doesn't support it\n");
> +		else if (errno_ != ETIMEDOUT)
> +			igt_assert_f(rc != -1, "Unexpected error:
> %d\n",
> +				     errno_);
> +		return errno_;
> +	} else {
> +		int i;
> +		unsigned long long val = strtoull(buf, NULL, 16);
> +
> +		for (i = 5; i >= 0; i--, val >>= 8) {
> +			data[i] = (unsigned char) (val & 0xff);
> +		}

This is a very non-trivial piece of code, a comment here would be good.
 Why are we doing this? I'm not sure what's the value in converting CRC
data formats when the only thing we do is check for equality.

> +		return 0;
> +	}
> +}
> diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> new file mode 100644
> index 0000000..d94cdfa
> --- /dev/null
> +++ b/lib/igt_psr.h
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright © 2017 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person
> obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom
> the
> + * Software is furnished to do so, subject to the following
> conditions:
> + *
> + * The above copyright notice and this permission notice (including
> the next
> + * paragraph) shall be included in all copies or substantial
> portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#ifndef IGT_PSR_H
> +#define IGT_PSR_H
> +
> +#define igt_psr_enable() igt_set_module_param_int("enable_psr", 1)
> +#define igt_psr_disable() igt_set_module_param_int("enable_psr", 0)
> +
> +bool igt_psr_sink_support(int fd);
> +bool igt_psr_source_support(int fd);
> +bool igt_psr_possible(int fd);
> +bool igt_psr_active(int fd);
> +bool igt_psr_await_status(int fd, bool active);
> +bool igt_psr_enabled(int fd);
> +void igt_psr_print_status(int fd);
> +bool igt_psr_valid_connector(drmModeConnectorPtr connector);
> +bool igt_psr_find_good_mode(drmModeConnectorPtr connector,
> +			    drmModeModeInfoPtr *mode);
> +int igt_psr_open_sink_crc(int drm_fd);
> +int igt_psr_get_sink_crc(int fd, char *data);
> +
> +#endif /* IGT_PSR_H */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH IGT v2 4/6] tests/kms_frontbuffer_tracking: Refactor to use IGT PSR library functions

[Paulo Zanoni]

Em Sex, 2017-06-30 às 12:12 -0700, Jim Bride escreveu:
> v2: * Minor functional tweaks and bug fixes
>     * Rebase
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 119 +++++++--------------------
> ------------
>  1 file changed, 19 insertions(+), 100 deletions(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index c24e4a8..3a8b754 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -183,7 +183,7 @@ struct {
>  };
>  
>  
> -#define SINK_CRC_SIZE 12
> +#define SINK_CRC_SIZE 6
>  typedef struct {
>  	char data[SINK_CRC_SIZE];
>  } sink_crc_t;
> @@ -327,28 +327,6 @@ drmModeModeInfo std_1024_mode = {
>  	.name = "Custom 1024x768",
>  };
>  
> -static drmModeModeInfoPtr
> get_connector_smallest_mode(drmModeConnectorPtr c)
> -{
> -	int i;
> -	drmModeModeInfoPtr smallest = NULL;
> -
> -	for (i = 0; i < c->count_modes; i++) {
> -		drmModeModeInfoPtr mode = &c->modes[i];
> -
> -		if (!smallest)
> -			smallest = mode;
> -
> -		if (mode->hdisplay * mode->vdisplay <
> -		    smallest->hdisplay * smallest->vdisplay)
> -			smallest = mode;
> -	}
> -
> -	if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
> -		smallest = &std_1024_mode;
> -
> -	return smallest;
> -}
> -
>  static drmModeConnectorPtr get_connector(uint32_t id)
>  {
>  	int i;
> @@ -421,30 +399,6 @@ static void init_mode_params(struct
> modeset_params *params, uint32_t crtc_id,
>  	params->sprite.h = 64;
>  }
>  
> -static bool connector_get_mode(drmModeConnectorPtr c,
> drmModeModeInfoPtr *mode)
> -{
> -	*mode = NULL;
> -
> -	if (c->connection != DRM_MODE_CONNECTED || !c->count_modes)
> -		return false;
> -
> -	if (c->connector_type == DRM_MODE_CONNECTOR_eDP &&
> opt.no_edp)
> -		return false;
> -
> -	if (opt.small_modes)
> -		*mode = get_connector_smallest_mode(c);
> -	else
> -		*mode = &c->modes[0];
> -
> -	 /* On HSW the CRC WA is so awful that it makes you think
> everything is
> -	  * bugged. */
> -	if (IS_HASWELL(intel_get_drm_devid(drm.fd)) &&
> -	    c->connector_type == DRM_MODE_CONNECTOR_eDP)
> -		*mode = &std_1024_mode;
> -
> -	return true;
> -}
> -
>  static bool connector_supports_pipe_a(drmModeConnectorPtr connector)
>  {
>  	int i;
> @@ -473,7 +427,7 @@ static bool find_connector(bool edp_only, bool
> pipe_a, uint32_t forbidden_id,
>  			continue;
>  		if (c->connector_id == forbidden_id)
>  			continue;
> -		if (!connector_get_mode(c, &mode))
> +		if (!igt_psr_find_good_mode(c, &mode))
>  			continue;
>  
>  		*ret_connector = c;
> @@ -804,23 +758,6 @@ static void fbc_print_status(void)
>  	igt_info("FBC status:\n%s\n", buf);
>  }
>  
> -static bool psr_is_enabled(void)
> -{
> -	char buf[256];
> -
> -	debugfs_read("i915_edp_psr_status", buf);
> -	return strstr(buf, "\nActive: yes\n") &&
> -	       strstr(buf, "\nHW Enabled & Active bit: yes\n");
> -}
> -
> -static void psr_print_status(void)
> -{
> -	char buf[256];
> -
> -	debugfs_read("i915_edp_psr_status", buf);
> -	igt_info("PSR status:\n%s\n", buf);
> -}
> -
>  static struct timespec fbc_get_last_action(void)
>  {
>  	struct timespec ret = { 0, 0 };
> @@ -926,44 +863,31 @@ static bool fbc_wait_until_enabled(void)
>  	return igt_wait(fbc_is_enabled(), 2000, 1);
>  }
>  
> -static bool psr_wait_until_enabled(void)
> -{
> -	return igt_wait(psr_is_enabled(), 5000, 1);

Changing the timeout from 5s to 10s is going to significantly increase
the runtime for kms_frontbuffer_tracking.

> -}
> -
>  #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
>  #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
> -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
> -#define psr_disable() igt_set_module_param_int("enable_psr", 0)
>  
>  static void get_sink_crc(sink_crc_t *crc, bool mandatory)
>  {
> -	int rc, errno_;
> +	int rc;
>  
>  	if (!sink_crc.supported) {
>  		memcpy(crc, "unsupported!", SINK_CRC_SIZE);
>  		return;
>  	}
>  
> -	lseek(sink_crc.fd, 0, SEEK_SET);
> -
> -	rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE);
> -	errno_ = errno;
> -
> -	if (rc == -1 && errno_ == ENOTTY) {
> +	rc = igt_psr_get_sink_crc(sink_crc.fd, crc->data);
> +	if (rc == ENOTTY) {
>  		igt_info("Sink CRC not supported: panel doesn't
> support it\n");
>  		sink_crc.supported = false;
> -	} else if (rc == -1 && errno_ == ETIMEDOUT) {
> -		if (sink_crc.reliable) {
> -			igt_info("Sink CRC is unreliable on this
> machine.\n");
> +	} else if (rc == ETIMEDOUT) {
> +		if (sink_crc.reliable) 
>  			sink_crc.reliable = false;
> -		}
> +		
>  
>  		if (mandatory)
>  			igt_skip("Sink CRC is unreliable on this
> machine.\n");
>  	} else {
> -		igt_assert_f(rc != -1, "Unexpected error: %d\n",
> errno_);
> -		igt_assert(rc == SINK_CRC_SIZE);
> +		igt_assert_f(rc == 0, "Unexpected error: %d\n", rc);
>  	}
>  }
>  
> @@ -1180,7 +1104,7 @@ static void disable_features(const struct
> test_mode *t)
>  		return;
>  
>  	fbc_disable();
> -	psr_disable();
> +	igt_psr_disable();
>  }
>  
>  static void *busy_thread_func(void *data)
> @@ -1432,7 +1356,7 @@ static void setup_sink_crc(void)
>  	fill_fb_region(&prim_mode_params.fb, COLOR_PRIM_BG);
>  	set_mode_for_params(&prim_mode_params);
>  
> -	sink_crc.fd = openat(drm.debugfs, "i915_sink_crc_eDP1",
> O_RDONLY);
> +	sink_crc.fd = igt_psr_open_sink_crc(drm.debugfs);
>  	igt_assert_lte(0, sink_crc.fd);
>  
>  	/* Do a first read to try to detect if it's supported. */
> @@ -1547,14 +1471,6 @@ static void teardown_fbc(void)
>  {
>  }
>  
> -static bool psr_sink_has_support(void)
> -{
> -	char buf[256];
> -
> -	debugfs_read("i915_edp_psr_status", buf);
> -	return strstr(buf, "Sink_Support: yes\n");
> -}
> -
>  static void setup_psr(void)
>  {
>  	if (get_connector(prim_mode_params.connector_id)-
> >connector_type !=
> @@ -1563,7 +1479,7 @@ static void setup_psr(void)
>  		return;
>  	}
>  
> -	if (!psr_sink_has_support()) {
> +	if (!igt_psr_sink_support(drm.fd)) {
>  		igt_info("Can't test PSR: not supported by
> sink.\n");
>  		return;
>  	}
> @@ -1717,12 +1633,15 @@ static int adjust_assertion_flags(const
> struct test_mode *t, int flags)
>  	}								
> \
>  									
> \
>  	if (flags_ & ASSERT_PSR_ENABLED) {				
> \
> -		if (!psr_wait_until_enabled()) {			
> \
> -			psr_print_status();				
> \
> +		if (!igt_psr_await_status(drm.fd, true)) {	

> 	\
> +			igt_psr_print_status(drm.fd);		
> 	\
>  			igt_assert_f(false, "PSR disabled\n");	
> 	\
>  		}							
> \
>  	} else if (flags_ & ASSERT_PSR_DISABLED) {			
> \
> -		igt_assert(!psr_wait_until_enabled());		
> 	\
> +		if (!igt_psr_await_status(drm.fd, false)) {	

There's a difference between "wait until disabled" and "wait until
enabled". If we're expecting PSR to be disabled and we call
wait_until_enabled(), we guarantee that PSR is going to be disabled
during the whole timeout. And that's the point: guarantee that PSR
stays disabled during the entire time, i.e., nothing enables it at any
point in the next $timeout seconds. It could be the case that PSR got
disabled and then re-enabled due to some delayed work function, and we
want to prevent that by staying the whole timeout re-checking its
status.

So here, if we just assert that PSR got disabled at some point during
the timeout, we're changing the behavior of the test and we're losing
the original purpose of the assertion.

> 	\
> +			igt_psr_print_status(drm.fd);               
>     \
> +			igt_assert_f(false, "PSR enabled\n");	
> 	\
> +		}							
> \
>  	}								
> \
>  } while (0)
>  
> @@ -1822,7 +1741,7 @@ static void enable_features_for_test(const
> struct test_mode *t)
>  	if (t->feature & FEATURE_FBC)
>  		fbc_enable();
>  	if (t->feature & FEATURE_PSR)
> -		psr_enable();
> +		igt_psr_enable();
>  }
>  
>  static void check_test_requirements(const struct test_mode *t)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH IGT v2 6/6] tests/kms_fbcon_fbt: Refactor to use IGT PSR library functions

[Paulo Zanoni]

Em Sex, 2017-06-30 às 12:12 -0700, Jim Bride escreveu:
> v2: * Minor functional tweaks and bug fixes
>     * Rebase
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  tests/kms_fbcon_fbt.c | 54 +++++++++++++++++----------------------
> ------------
>  1 file changed, 18 insertions(+), 36 deletions(-)
> 
> diff --git a/tests/kms_fbcon_fbt.c b/tests/kms_fbcon_fbt.c
> index d009091..593adb9 100644
> --- a/tests/kms_fbcon_fbt.c
> +++ b/tests/kms_fbcon_fbt.c
> @@ -103,8 +103,9 @@ static bool fbc_is_enabled(int fd)
>  	return strstr(buf, "FBC enabled\n");
>  }
>  
> -static bool fbc_wait_until_enabled(int fd)
> +static bool fbc_wait_until_enabled(int fd, bool enabled)
>  {
> +	enabled = enabled;

If GCC is complaining, please use the appropriate annotations instead
of doing this.


>  	return igt_wait(fbc_is_enabled(fd), 5000, 1);
>  }
>  
> @@ -124,6 +125,13 @@ static void set_mode_for_one_screen(struct
> drm_info *drm, struct igt_fb *fb,
>  
>  		if (c->connection == DRM_MODE_CONNECTED && c-
> >count_modes &&
>  		    connector_possible(c)) {
> +			if (c->connector_type ==
> DRM_MODE_CONNECTOR_eDP) {
> +				bool bret;
> +
> +				bret = igt_psr_find_good_mode(c,
> &mode);
> +				if (bret)
> +					break;
> +			}
>  			mode = &c->modes[0];
>  			break;
>  		}
> @@ -147,35 +155,9 @@ static void set_mode_for_one_screen(struct
> drm_info *drm, struct igt_fb *fb,
>  	igt_assert_eq(rc, 0);
>  }
>  
> -static bool psr_supported_on_chipset(int fd)
> -{
> -	char buf[256];
> -
> -	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> -	return strstr(buf, "Sink_Support: yes\n");
> -}
> -
> -static bool connector_can_psr(drmModeConnectorPtr connector)
> -{
> -	return (connector->connector_type ==
> DRM_MODE_CONNECTOR_eDP);
> -}
> -
> -static bool psr_is_enabled(int fd)
> -{
> -	char buf[256];
> -
> -	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> -	return strstr(buf, "\nActive: yes\n");
> -}
> -
> -static bool psr_wait_until_enabled(int fd)
> -{
> -	return igt_wait(psr_is_enabled(fd), 5000, 1);

Same problem here as we had in patch 4: changing the timeout from 5 to
10 is going to impact runtime significantly.


> -}
> -
>  struct feature {
>  	bool (*supported_on_chipset)(int fd);
> -	bool (*wait_until_enabled)(int fd);
> +	bool (*wait_until_enabled)(int fd, bool status);
>  	bool (*connector_possible_fn)(drmModeConnectorPtr
> connector);
>  	const char *param_name;
>  } fbc = {
> @@ -184,9 +166,9 @@ struct feature {
>  	.connector_possible_fn = connector_can_fbc,
>  	.param_name = "enable_fbc",
>  }, psr = {
> -	.supported_on_chipset = psr_supported_on_chipset,
> -	.wait_until_enabled = psr_wait_until_enabled,
> -	.connector_possible_fn = connector_can_psr,
> +	.supported_on_chipset = igt_psr_sink_support,
> +	.wait_until_enabled = igt_psr_await_status,
> +	.connector_possible_fn = igt_psr_valid_connector,
>  	.param_name = "enable_psr",
>  };
>  
> @@ -210,17 +192,17 @@ static void subtest(struct feature *feature,
> bool suspend)
>  
>  	kmstest_unset_all_crtcs(drm.fd, drm.res);
>  	wait_user("Modes unset.");
> -	igt_assert(!feature->wait_until_enabled(drm.fd));
> +	igt_assert(!feature->wait_until_enabled(drm.fd, true));
>  
>  	set_mode_for_one_screen(&drm, &fb, feature-
> >connector_possible_fn);
>  	wait_user("Screen set.");
> -	igt_assert(feature->wait_until_enabled(drm.fd));
> +	igt_assert(feature->wait_until_enabled(drm.fd, true));
>  
>  	if (suspend) {
>  		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
>  					      SUSPEND_TEST_NONE);
>  		sleep(5);
> -		igt_assert(feature->wait_until_enabled(drm.fd));
> +		igt_assert(feature->wait_until_enabled(drm.fd,
> true));
>  	}
>  
>  	igt_remove_fb(drm.fd, &fb);
> @@ -230,13 +212,13 @@ static void subtest(struct feature *feature,
> bool suspend)
>  	sleep(3);
>  
>  	wait_user("Back to fbcon.");
> -	igt_assert(!feature->wait_until_enabled(drm.fd));
> +	igt_assert(!feature->wait_until_enabled(drm.fd, true));
>  
>  	if (suspend) {
>  		igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
>  					      SUSPEND_TEST_NONE);
>  		sleep(5);
> -		igt_assert(!feature->wait_until_enabled(drm.fd));
> +		igt_assert(!feature->wait_until_enabled(drm.fd,
> true));
>  	}
>  }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH IGT v2 2/6] lib: Add PSR utility functions to igt library.

[Jim Bride]

On Fri, Jun 30, 2017 at 01:11:52PM -0700, Rodrigo Vivi wrote:
> On Fri, Jun 30, 2017 at 12:12 PM, Jim Bride <jim.bride@linux.intel.com> wrote:
> > Factor out some code that was replicated in three test utilities into
> > some new IGT library functions so that we are checking PSR status in
> > a consistent fashion across all of our PSR tests.
> >
> > v2: * Add sink CRC helper function
> >     * Misc. bug fixes
> >     * Rebase
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > ---
> >  lib/Makefile.sources |   2 +
> >  lib/igt.h            |   1 +
> >  lib/igt_psr.c        | 235 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/igt_psr.h        |  43 ++++++++++
> >  4 files changed, 281 insertions(+)
> >  create mode 100644 lib/igt_psr.c
> >  create mode 100644 lib/igt_psr.h
> >
> > diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> > index 53fdb54..6a73c8c 100644
> > --- a/lib/Makefile.sources
> > +++ b/lib/Makefile.sources
> > @@ -83,6 +83,8 @@ lib_source_list =             \
> >         uwildmat/uwildmat.c     \
> >         igt_kmod.c              \
> >         igt_kmod.h              \
> > +       igt_psr.c               \
> > +       igt_psr.h               \
> >         $(NULL)
> >
> >  .PHONY: version.h.tmp
> > diff --git a/lib/igt.h b/lib/igt.h
> > index a069deb..0b8e3a8 100644
> > --- a/lib/igt.h
> > +++ b/lib/igt.h
> > @@ -37,6 +37,7 @@
> >  #include "igt_gt.h"
> >  #include "igt_kms.h"
> >  #include "igt_pm.h"
> > +#include "igt_psr.h"
> >  #include "igt_stats.h"
> >  #ifdef HAVE_CHAMELIUM
> >  #include "igt_chamelium.h"
> > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > new file mode 100644
> > index 0000000..a2823bf
> > --- /dev/null
> > +++ b/lib/igt_psr.c
> > @@ -0,0 +1,235 @@
> > +/*
> > + * Copyright © 2017 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#include "igt.h"
> > +#include <stdio.h>
> > +#include <stdbool.h>
> > +#include <sys/types.h>
> > +#include <string.h>
> > +#include <fcntl.h>
> > +
> > +/**
> > + * SECTION:igt_psr
> > + * @short_description: Panel Self Refresh helpers
> > + * @title: Panel Self Refresh
> > + * @include: igt.h
> > + *
> > + * This library provides various helpers to enable Panel Self Refresh,
> > + * as well as to check the state of PSR on the system (enabled vs.
> > + * disabled, active vs. inactive) or to wait for PSR to be active
> > + * or inactive.
> > + */
> > +
> > +/**
> > + * igt_psr_source_support:
> > + *
> > + * Returns true if the source supports PSR.
> > + */
> > +bool igt_psr_source_support(int fd)
> > +{
> > +       char buf[512];
> > +
> > +       igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +
> > +       return strstr(buf, "Source_OK: yes\n");
> > +}
> > +
> > +
> > +/**
> > + * igt_psr_sink_support:
> > + *
> > + * Returns true if the current eDP panel supports PSR.
> > + */
> > +bool igt_psr_sink_support(int fd)
> > +{
> > +       char buf[256];
> > +
> > +       igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +       return strstr(buf, "Sink_Support: yes\n");
> > +}
> > +
> > +/**
> > + * igt_psr_possible:
> > + *
> > + * Returns true if both the source and sink support PSR.
> > + */
> > +bool igt_psr_possible(int fd)
> > +{
> > +       char buf[512];
> > +
> > +       igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +
> > +       return igt_psr_source_support(fd) && igt_psr_sink_support(fd);
> > +}
> > +
> > +/**
> > + * igt_psr_active:
> > + *
> > + * Returns true if PSR is active on the panel.
> > + */
> > +bool igt_psr_active(int fd)
> > +{
> > +       char buf[512];
> > +       bool actret = false;
> > +       bool hwactret = false;
> > +
> > +       igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +       hwactret = (strstr(buf, "HW Enabled & Active bit: yes\n") != NULL);
> > +       actret = (strstr(buf, "Active: yes\n") != NULL);
> > +       igt_debug("hwactret: %s actret: %s\n", hwactret ? "true" : "false",
> > +                actret ? "true" : "false");
> > +       return hwactret && actret;
> > +}
> > +
> > +/**
> > + * igt_psr_await_status:
> > + * @active: A boolean that causes the function to wait for PSR to activate
> > + *          if set to true, or to wait for PSR to deactivate if false.
> > + *
> > + * Returns true if the requested condition is met.
> > + */
> > +bool igt_psr_await_status(int fd, bool active)
> > +{
> > +       const int timeout = 10;
> > +       int count = 0;
> > +       while (count < timeout) {
> > +               if (igt_psr_active(fd) == active) {
> > +                       igt_debug("PSR %s after %d seconds.\n",
> > +                                 active ? "Active" : "Inactive", count);
> > +                       return true;
> > +               }
> > +               count++;
> > +               sleep(1);
> > +       }
> > +       return false;
> > +}
> > +
> > +/**
> > + * igt_psr_enabled:
> > + *
> > + * Returns true if the PSR feature is enabled.
> > + */
> > +bool igt_psr_enabled(int fd)
> > +{
> > +       char buf[256];
> > +
> > +       igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +       return strstr(buf, "Enabled: yes\n");
> > +}
> > +
> > +/**
> > + * igt_psr_print_status:
> > + *
> > + * Dumps the contents of i915_edp_psr_status from debugfs.
> > + */
> > +void igt_psr_print_status(int fd)
> > +{
> > +        char buf[256];
> > +
> > +        igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +        igt_info("PSR status:\n%s\n", buf);
> > +}
> > +
> > +/**
> > + * igt_psr_valid_connector:
> > + * @connector: a drmModeConnector pointer to check
> > + *
> > + * Returns true if connector is an eDP connector.
> > + */
> > +bool igt_psr_valid_connector(drmModeConnectorPtr connector)
> > +{
> > +       return (connector->connector_type == DRM_MODE_CONNECTOR_eDP);
> > +}
> > +
> > +/**
> > + * igt_psr_find_good_mode
> > + * @connector: a drmModeConnector pointer to find the mode on
> > + * @mode: a drmModeModePtr pointer that is set to the matching mode
> > + *
> > + * Returns true (and populates *mode with the match) if a valid
> > + * PSR mdoe is found, and false otherwise.
> > + */
> > +bool igt_psr_find_good_mode(drmModeConnectorPtr connector,
> > +                           drmModeModeInfoPtr *mode)
> > +{
> > +       int i;
> > +
> > +       if (!connector->count_modes) {
> > +               igt_warn("no modes for connector %d.\n",
> > +                        connector->connector_id);
> > +               return false;
> > +       } else {
> > +               igt_debug("Connector has %d modes.\n", connector->count_modes);
> > +       }
> > +
> > +       for (i = 0; i < connector->count_modes; i++) {
> > +               if ((connector->modes[i].vtotal -
> > +                    connector->modes[i].vdisplay) >= 36) {
> 
> why 36?
> 
> doesn't this depend on the panel setup time and on mode resolution?

We assume the worst case that the spec allows for this calculation as far
as panel setup time goes.  I should probably consider a #define here but
basically 36 lines is what I came up with as a rough calculation as to
how many lines of vertical blank are needed to accommodate the worst-case
panel setup time.

> > +                       igt_debug("Mode %d good for PSR.\n", i);
> > +                       *mode = &(connector->modes[i]);
> > +                       return true;
> > +               } else {
> > +                       igt_debug("Throwing out mode %d\n", i);
> > +               }
> > +       }
> > +       return false;
> > +}
> > +
> > +int igt_psr_open_sink_crc(int drm_fd)
> > +{
> > +       int crc_fd;
> > +
> > +       crc_fd = openat(drm_fd, "i915_sink_crc_eDP1", O_RDONLY);
> > +       igt_assert_lte(0, crc_fd);
> > +       return crc_fd;
> > +}
> > +
> > +
> > +int igt_psr_get_sink_crc(int fd, char *data)
> > +{
> > +       int rc, errno_;
> > +       char buf[13];
> > +
> > +       memset(buf, 0, 13);
> > +       lseek(fd, 0, SEEK_SET);
> > +
> > +       rc = read(fd, buf, 12);
> > +       errno_ = errno;
> > +
> > +       if (rc == -1) {
> > +               if (errno_ == ENOTTY)
> > +                       igt_info("Sink CRC not supported: panel doesn't support it\n");
> > +               else if (errno_ != ETIMEDOUT)
> > +                       igt_assert_f(rc != -1, "Unexpected error: %d\n",
> > +                                    errno_);
> > +               return errno_;
> > +       } else {
> > +               int i;
> > +               unsigned long long val = strtoull(buf, NULL, 16);
> > +
> > +               for (i = 5; i >= 0; i--, val >>= 8) {
> > +                       data[i] = (unsigned char) (val & 0xff);
> > +               }
> > +               return 0;
> > +       }
> > +}
> > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > new file mode 100644
> > index 0000000..d94cdfa
> > --- /dev/null
> > +++ b/lib/igt_psr.h
> > @@ -0,0 +1,43 @@
> > +/*
> > + * Copyright © 2017 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the next
> > + * paragraph) shall be included in all copies or substantial portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef IGT_PSR_H
> > +#define IGT_PSR_H
> > +
> > +#define igt_psr_enable() igt_set_module_param_int("enable_psr", 1)
> > +#define igt_psr_disable() igt_set_module_param_int("enable_psr", 0)
> > +
> > +bool igt_psr_sink_support(int fd);
> > +bool igt_psr_source_support(int fd);
> > +bool igt_psr_possible(int fd);
> > +bool igt_psr_active(int fd);
> > +bool igt_psr_await_status(int fd, bool active);
> > +bool igt_psr_enabled(int fd);
> > +void igt_psr_print_status(int fd);
> > +bool igt_psr_valid_connector(drmModeConnectorPtr connector);
> > +bool igt_psr_find_good_mode(drmModeConnectorPtr connector,
> > +                           drmModeModeInfoPtr *mode);
> > +int igt_psr_open_sink_crc(int drm_fd);
> > +int igt_psr_get_sink_crc(int fd, char *data);
> > +
> > +#endif /* IGT_PSR_H */
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH IGT v2 2/6] lib: Add PSR utility functions to igt library.

[Jim Bride]

On Fri, Jun 30, 2017 at 05:54:32PM -0300, Paulo Zanoni wrote:
> Em Sex, 2017-06-30 às 12:12 -0700, Jim Bride escreveu:
> > Factor out some code that was replicated in three test utilities into
> > some new IGT library functions so that we are checking PSR status in
> > a consistent fashion across all of our PSR tests.
> > 
> > v2: * Add sink CRC helper function
> >     * Misc. bug fixes
> >     * Rebase
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > ---
> >  lib/Makefile.sources |   2 +
> >  lib/igt.h            |   1 +
> >  lib/igt_psr.c        | 235
> > +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/igt_psr.h        |  43 ++++++++++
> >  4 files changed, 281 insertions(+)
> >  create mode 100644 lib/igt_psr.c
> >  create mode 100644 lib/igt_psr.h
> > 
> > diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> > index 53fdb54..6a73c8c 100644
> > --- a/lib/Makefile.sources
> > +++ b/lib/Makefile.sources
> > @@ -83,6 +83,8 @@ lib_source_list =	 	\
> >  	uwildmat/uwildmat.c	\
> >  	igt_kmod.c		\
> >  	igt_kmod.h		\
> > +	igt_psr.c		\
> > +	igt_psr.h		\
> >  	$(NULL)
> >  
> >  .PHONY: version.h.tmp
> > diff --git a/lib/igt.h b/lib/igt.h
> > index a069deb..0b8e3a8 100644
> > --- a/lib/igt.h
> > +++ b/lib/igt.h
> > @@ -37,6 +37,7 @@
> >  #include "igt_gt.h"
> >  #include "igt_kms.h"
> >  #include "igt_pm.h"
> > +#include "igt_psr.h"
> >  #include "igt_stats.h"
> >  #ifdef HAVE_CHAMELIUM
> >  #include "igt_chamelium.h"
> > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > new file mode 100644
> > index 0000000..a2823bf
> > --- /dev/null
> > +++ b/lib/igt_psr.c
> > @@ -0,0 +1,235 @@
> > +/*
> > + * Copyright © 2017 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom
> > the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice (including
> > the next
> > + * paragraph) shall be included in all copies or substantial
> > portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> > OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#include "igt.h"
> > +#include <stdio.h>
> > +#include <stdbool.h>
> > +#include <sys/types.h>
> > +#include <string.h>
> > +#include <fcntl.h>
> > +
> > +/**
> > + * SECTION:igt_psr
> > + * @short_description: Panel Self Refresh helpers
> > + * @title: Panel Self Refresh
> > + * @include: igt.h
> > + *
> > + * This library provides various helpers to enable Panel Self
> > Refresh,
> > + * as well as to check the state of PSR on the system (enabled vs.
> > + * disabled, active vs. inactive) or to wait for PSR to be active
> > + * or inactive.
> > + */
> > +
> > +/**
> > + * igt_psr_source_support:
> > + *
> > + * Returns true if the source supports PSR.
> > + */
> > +bool igt_psr_source_support(int fd)
> > +{
> > +	char buf[512];
> > +
> > +	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +
> > +	return strstr(buf, "Source_OK: yes\n");
> > +}
> > +
> > +
> > +/**
> > + * igt_psr_sink_support:
> > + *
> > + * Returns true if the current eDP panel supports PSR.
> > + */
> > +bool igt_psr_sink_support(int fd)
> > +{
> > +	char buf[256];
> 
> Why are some buffers 512 while the others are 256? They're both for the
> same file.

It's arbitrary.  I'll make things consistent when I revisit this patch.

> > +
> > +	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +	return strstr(buf, "Sink_Support: yes\n");
> > +}
> > +
> > +/**
> > + * igt_psr_possible:
> > + *
> > + * Returns true if both the source and sink support PSR.
> > + */
> > +bool igt_psr_possible(int fd)
> > +{
> > +	char buf[512];
> > +
> > +	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +
> > +	return igt_psr_source_support(fd) &&
> > igt_psr_sink_support(fd);
> > +}
> > +
> > +/**
> > + * igt_psr_active:
> > + *
> > + * Returns true if PSR is active on the panel.
> > + */
> > +bool igt_psr_active(int fd)
> > +{
> > +	char buf[512];
> > +	bool actret = false;
> > +	bool hwactret = false;
> > +
> > +	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +	hwactret = (strstr(buf, "HW Enabled & Active bit: yes\n") !=
> > NULL);
> > +	actret = (strstr(buf, "Active: yes\n") != NULL);
> > +	igt_debug("hwactret: %s actret: %s\n", hwactret ? "true" :
> > "false",
> > +		 actret ? "true" : "false");
> > +	return hwactret && actret;
> > +}
> > +
> > +/**
> > + * igt_psr_await_status:
> > + * @active: A boolean that causes the function to wait for PSR to
> > activate
> > + *          if set to true, or to wait for PSR to deactivate if
> > false.
> > + *
> > + * Returns true if the requested condition is met.
> > + */
> > +bool igt_psr_await_status(int fd, bool active)
> > +{
> > +	const int timeout = 10;
> > +	int count = 0;
> > +	while (count < timeout) {
> > +		if (igt_psr_active(fd) == active) {
> > +			igt_debug("PSR %s after %d seconds.\n",
> > +				  active ? "Active" : "Inactive",
> > count);
> > +			return true;
> > +		}
> > +		count++;
> > +		sleep(1);
> > +	}
> > +	return false;
> 
> Since this is in a lib now, can you please make use of igt_wait or just
> make it work as expected when the signal helper is active?

Sure.

> 
> > +}
> > +
> > +/**
> > + * igt_psr_enabled:
> > + *
> > + * Returns true if the PSR feature is enabled.
> > + */
> > +bool igt_psr_enabled(int fd)
> > +{
> > +	char buf[256];
> > +
> > +	igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +	return strstr(buf, "Enabled: yes\n");
> > +}
> > +
> > +/**
> > + * igt_psr_print_status:
> > + *
> > + * Dumps the contents of i915_edp_psr_status from debugfs.
> > + */
> > +void igt_psr_print_status(int fd)
> > +{
> > +        char buf[256];
> > +
> > +        igt_debugfs_read(fd, "i915_edp_psr_status", buf);
> > +        igt_info("PSR status:\n%s\n", buf);
> > +}
> > +
> > +/**
> > + * igt_psr_valid_connector:
> > + * @connector: a drmModeConnector pointer to check
> > + *
> > + * Returns true if connector is an eDP connector.
> > + */
> > +bool igt_psr_valid_connector(drmModeConnectorPtr connector)
> > +{
> > +	return (connector->connector_type ==
> > DRM_MODE_CONNECTOR_eDP);
> > +}
> > +
> > +/**
> > + * igt_psr_find_good_mode
> > + * @connector: a drmModeConnector pointer to find the mode on
> > + * @mode: a drmModeModePtr pointer that is set to the matching mode
> > + *
> > + * Returns true (and populates *mode with the match) if a valid
> > + * PSR mdoe is found, and false otherwise.
> > + */
> > +bool igt_psr_find_good_mode(drmModeConnectorPtr connector,
> > +			    drmModeModeInfoPtr *mode)
> > +{
> > +	int i;
> > +
> > +	if (!connector->count_modes) {
> > +		igt_warn("no modes for connector %d.\n",
> > +			 connector->connector_id);
> > +		return false;
> > +	} else {
> > +		igt_debug("Connector has %d modes.\n", connector-
> > >count_modes);
> > +	}
> > +
> > +	for (i = 0; i < connector->count_modes; i++) {
> > +		if ((connector->modes[i].vtotal -
> > +		     connector->modes[i].vdisplay) >= 36) {
> > +			igt_debug("Mode %d good for PSR.\n", i);
> > +			*mode = &(connector->modes[i]);
> > +			return true;
> 
> Since the callers may be trying to find modes that fit other
> restrictions (e.g., kms_frontbuffer_tracking) I'd suggest you to
> extract igt_psr_mode_is_compatible(mode) with the vtotal/vdisplay
> check.

Can you explain what you mean by extract here?  Do you mean have it live
somewhere other than with the other PSR functions?  If so, what would
you suggest?

> 
> > +		} else {
> > +			igt_debug("Throwing out mode %d\n", i);
> > +		}
> > +	}
> > +	return false;
> > +}
> > +
> > +int igt_psr_open_sink_crc(int drm_fd)
> > +{
> > +	int crc_fd;
> > +
> > +	crc_fd = openat(drm_fd, "i915_sink_crc_eDP1", O_RDONLY);
> > +	igt_assert_lte(0, crc_fd);
> > +	return crc_fd;
> > +}
> > +
> > +
> 
> Please add documentation for this one, especially around the
> expectation for "data".

Good call.  
> > +int igt_psr_get_sink_crc(int fd, char *data)
> > +{
> > +	int rc, errno_;
> > +	char buf[13];
> > +
> > +	memset(buf, 0, 13);
> > +	lseek(fd, 0, SEEK_SET);
> > +
> > +	rc = read(fd, buf, 12);
> > +	errno_ = errno;
> > +
> > +	if (rc == -1) {
> > +		if (errno_ == ENOTTY)
> > +			igt_info("Sink CRC not supported: panel
> > doesn't support it\n");
> > +		else if (errno_ != ETIMEDOUT)
> > +			igt_assert_f(rc != -1, "Unexpected error:
> > %d\n",
> > +				     errno_);
> > +		return errno_;
> > +	} else {
> > +		int i;
> > +		unsigned long long val = strtoull(buf, NULL, 16);
> > +
> > +		for (i = 5; i >= 0; i--, val >>= 8) {
> > +			data[i] = (unsigned char) (val & 0xff);
> > +		}
> 
> This is a very non-trivial piece of code, a comment here would be good.
>  Why are we doing this? I'm not sure what's the value in converting CRC
> data formats when the only thing we do is check for equality.

We were actually dealing with the CRC values differently across tests.  In
some cases the values were kept as strings and compared with string funcs,
and in others they were converted to binary values and then compared with
memcmp.  I chose to have the binary values be what is used.  I'll add a
comment.

> > +		return 0;
> > +	}
> > +}
> > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > new file mode 100644
> > index 0000000..d94cdfa
> > --- /dev/null
> > +++ b/lib/igt_psr.h
> > @@ -0,0 +1,43 @@
> > +/*
> > + * Copyright © 2017 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute,
> > sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom
> > the
> > + * Software is furnished to do so, subject to the following
> > conditions:
> > + *
> > + * The above copyright notice and this permission notice (including
> > the next
> > + * paragraph) shall be included in all copies or substantial
> > portions of the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
> > EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
> > OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > OTHER DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef IGT_PSR_H
> > +#define IGT_PSR_H
> > +
> > +#define igt_psr_enable() igt_set_module_param_int("enable_psr", 1)
> > +#define igt_psr_disable() igt_set_module_param_int("enable_psr", 0)
> > +
> > +bool igt_psr_sink_support(int fd);
> > +bool igt_psr_source_support(int fd);
> > +bool igt_psr_possible(int fd);
> > +bool igt_psr_active(int fd);
> > +bool igt_psr_await_status(int fd, bool active);
> > +bool igt_psr_enabled(int fd);
> > +void igt_psr_print_status(int fd);
> > +bool igt_psr_valid_connector(drmModeConnectorPtr connector);
> > +bool igt_psr_find_good_mode(drmModeConnectorPtr connector,
> > +			    drmModeModeInfoPtr *mode);
> > +int igt_psr_open_sink_crc(int drm_fd);
> > +int igt_psr_get_sink_crc(int fd, char *data);
> > +
> > +#endif /* IGT_PSR_H */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH IGT v2 5/6] tests/kms_frontbuffer_tracking: Fix multidraw subtest

[Paulo Zanoni]

Em Sex, 2017-06-30 às 12:12 -0700, Jim Bride escreveu:
> The multidraw subtest was not taking whether or not the GEM buffer
> had
> ever been in write-combining mode when checking for PSR state, so fix
> that.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index 3a8b754..c52d7a0 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -2059,7 +2059,8 @@ static void multidraw_subtest(const struct
> test_mode *t)
>  				assertions = used_method !=
> IGT_DRAW_MMAP_GTT ?
>  					     ASSERT_LAST_ACTION_CHAN
> GED :
>  					     ASSERT_NO_ACTION_CHANGE
> ;
> -				if (op_disables_psr(t, used_method))
> +				if (op_disables_psr(t, used_method)
> &&
> +				    !wc_used)
>  					assertions |=
> ASSERT_PSR_DISABLED;
>  
>  				do_assertions(assertions);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH IGT v2 4/6] tests/kms_frontbuffer_tracking: Refactor to use IGT PSR library functions

[Jim Bride]

On Fri, Jun 30, 2017 at 01:19:57PM -0700, Rodrigo Vivi wrote:
> I believe it would be better to create the psr lib with only one
> function at time and on every patch that adds the new function already
> removes that from here and from frontbuffer tracking test as well...
> 
> easier to review and to make sure that we are not changing the behaviour.

I'm testing a new series with the requested structural changes and review
feedback to-date.  I hope to send them out on Monday (testing takes a while.)

Jim

> also...
> 
> On Fri, Jun 30, 2017 at 12:12 PM, Jim Bride <jim.bride@linux.intel.com> wrote:
> > v2: * Minor functional tweaks and bug fixes
> >     * Rebase
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> > ---
> >  tests/kms_frontbuffer_tracking.c | 119 +++++++--------------------------------
> >  1 file changed, 19 insertions(+), 100 deletions(-)
> >
> > diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> > index c24e4a8..3a8b754 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -183,7 +183,7 @@ struct {
> >  };
> >
> >
> > -#define SINK_CRC_SIZE 12
> > +#define SINK_CRC_SIZE 6
> 
> I believe this deserves a separated patch...
> 
> >  typedef struct {
> >         char data[SINK_CRC_SIZE];
> >  } sink_crc_t;
> > @@ -327,28 +327,6 @@ drmModeModeInfo std_1024_mode = {
> >         .name = "Custom 1024x768",
> >  };
> >
> > -static drmModeModeInfoPtr get_connector_smallest_mode(drmModeConnectorPtr c)
> > -{
> > -       int i;
> > -       drmModeModeInfoPtr smallest = NULL;
> > -
> > -       for (i = 0; i < c->count_modes; i++) {
> > -               drmModeModeInfoPtr mode = &c->modes[i];
> > -
> > -               if (!smallest)
> > -                       smallest = mode;
> > -
> > -               if (mode->hdisplay * mode->vdisplay <
> > -                   smallest->hdisplay * smallest->vdisplay)
> > -                       smallest = mode;
> > -       }
> > -
> > -       if (c->connector_type == DRM_MODE_CONNECTOR_eDP)
> > -               smallest = &std_1024_mode;
> > -
> > -       return smallest;
> > -}
> > -
> >  static drmModeConnectorPtr get_connector(uint32_t id)
> >  {
> >         int i;
> > @@ -421,30 +399,6 @@ static void init_mode_params(struct modeset_params *params, uint32_t crtc_id,
> >         params->sprite.h = 64;
> >  }
> >
> > -static bool connector_get_mode(drmModeConnectorPtr c, drmModeModeInfoPtr *mode)
> > -{
> > -       *mode = NULL;
> > -
> > -       if (c->connection != DRM_MODE_CONNECTED || !c->count_modes)
> > -               return false;
> > -
> > -       if (c->connector_type == DRM_MODE_CONNECTOR_eDP && opt.no_edp)
> > -               return false;
> > -
> > -       if (opt.small_modes)
> > -               *mode = get_connector_smallest_mode(c);
> > -       else
> > -               *mode = &c->modes[0];
> > -
> > -        /* On HSW the CRC WA is so awful that it makes you think everything is
> > -         * bugged. */
> > -       if (IS_HASWELL(intel_get_drm_devid(drm.fd)) &&
> > -           c->connector_type == DRM_MODE_CONNECTOR_eDP)
> > -               *mode = &std_1024_mode;
> > -
> > -       return true;
> > -}
> > -
> >  static bool connector_supports_pipe_a(drmModeConnectorPtr connector)
> >  {
> >         int i;
> > @@ -473,7 +427,7 @@ static bool find_connector(bool edp_only, bool pipe_a, uint32_t forbidden_id,
> >                         continue;
> >                 if (c->connector_id == forbidden_id)
> >                         continue;
> > -               if (!connector_get_mode(c, &mode))
> > +               if (!igt_psr_find_good_mode(c, &mode))
> >                         continue;
> >
> >                 *ret_connector = c;
> > @@ -804,23 +758,6 @@ static void fbc_print_status(void)
> >         igt_info("FBC status:\n%s\n", buf);
> >  }
> >
> > -static bool psr_is_enabled(void)
> > -{
> > -       char buf[256];
> > -
> > -       debugfs_read("i915_edp_psr_status", buf);
> > -       return strstr(buf, "\nActive: yes\n") &&
> > -              strstr(buf, "\nHW Enabled & Active bit: yes\n");
> > -}
> > -
> > -static void psr_print_status(void)
> > -{
> > -       char buf[256];
> > -
> > -       debugfs_read("i915_edp_psr_status", buf);
> > -       igt_info("PSR status:\n%s\n", buf);
> > -}
> > -
> >  static struct timespec fbc_get_last_action(void)
> >  {
> >         struct timespec ret = { 0, 0 };
> > @@ -926,44 +863,31 @@ static bool fbc_wait_until_enabled(void)
> >         return igt_wait(fbc_is_enabled(), 2000, 1);
> >  }
> >
> > -static bool psr_wait_until_enabled(void)
> > -{
> > -       return igt_wait(psr_is_enabled(), 5000, 1);
> > -}
> > -
> >  #define fbc_enable() igt_set_module_param_int("enable_fbc", 1)
> >  #define fbc_disable() igt_set_module_param_int("enable_fbc", 0)
> > -#define psr_enable() igt_set_module_param_int("enable_psr", 1)
> > -#define psr_disable() igt_set_module_param_int("enable_psr", 0)
> >
> >  static void get_sink_crc(sink_crc_t *crc, bool mandatory)
> >  {
> > -       int rc, errno_;
> > +       int rc;
> >
> >         if (!sink_crc.supported) {
> >                 memcpy(crc, "unsupported!", SINK_CRC_SIZE);
> 
> this doesn't fit anymore
> 
> >                 return;
> >         }
> >
> > -       lseek(sink_crc.fd, 0, SEEK_SET);
> > -
> > -       rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE);
> > -       errno_ = errno;
> > -
> > -       if (rc == -1 && errno_ == ENOTTY) {
> > +       rc = igt_psr_get_sink_crc(sink_crc.fd, crc->data);
> > +       if (rc == ENOTTY) {
> >                 igt_info("Sink CRC not supported: panel doesn't support it\n");
> >                 sink_crc.supported = false;
> > -       } else if (rc == -1 && errno_ == ETIMEDOUT) {
> > -               if (sink_crc.reliable) {
> > -                       igt_info("Sink CRC is unreliable on this machine.\n");
> > +       } else if (rc == ETIMEDOUT) {
> > +               if (sink_crc.reliable)
> >                         sink_crc.reliable = false;
> > -               }
> > +
> >
> >                 if (mandatory)
> >                         igt_skip("Sink CRC is unreliable on this machine.\n");
> >         } else {
> > -               igt_assert_f(rc != -1, "Unexpected error: %d\n", errno_);
> > -               igt_assert(rc == SINK_CRC_SIZE);
> > +               igt_assert_f(rc == 0, "Unexpected error: %d\n", rc);
> >         }
> >  }
> >
> > @@ -1180,7 +1104,7 @@ static void disable_features(const struct test_mode *t)
> >                 return;
> >
> >         fbc_disable();
> > -       psr_disable();
> > +       igt_psr_disable();
> >  }
> >
> >  static void *busy_thread_func(void *data)
> > @@ -1432,7 +1356,7 @@ static void setup_sink_crc(void)
> >         fill_fb_region(&prim_mode_params.fb, COLOR_PRIM_BG);
> >         set_mode_for_params(&prim_mode_params);
> >
> > -       sink_crc.fd = openat(drm.debugfs, "i915_sink_crc_eDP1", O_RDONLY);
> > +       sink_crc.fd = igt_psr_open_sink_crc(drm.debugfs);
> >         igt_assert_lte(0, sink_crc.fd);
> >
> >         /* Do a first read to try to detect if it's supported. */
> > @@ -1547,14 +1471,6 @@ static void teardown_fbc(void)
> >  {
> >  }
> >
> > -static bool psr_sink_has_support(void)
> > -{
> > -       char buf[256];
> > -
> > -       debugfs_read("i915_edp_psr_status", buf);
> > -       return strstr(buf, "Sink_Support: yes\n");
> > -}
> > -
> >  static void setup_psr(void)
> >  {
> >         if (get_connector(prim_mode_params.connector_id)->connector_type !=
> > @@ -1563,7 +1479,7 @@ static void setup_psr(void)
> >                 return;
> >         }
> >
> > -       if (!psr_sink_has_support()) {
> > +       if (!igt_psr_sink_support(drm.fd)) {
> >                 igt_info("Can't test PSR: not supported by sink.\n");
> >                 return;
> >         }
> > @@ -1717,12 +1633,15 @@ static int adjust_assertion_flags(const struct test_mode *t, int flags)
> >         }                                                               \
> >                                                                         \
> >         if (flags_ & ASSERT_PSR_ENABLED) {                              \
> > -               if (!psr_wait_until_enabled()) {                        \
> > -                       psr_print_status();                             \
> > +               if (!igt_psr_await_status(drm.fd, true)) {              \
> > +                       igt_psr_print_status(drm.fd);                   \
> >                         igt_assert_f(false, "PSR disabled\n");          \
> >                 }                                                       \
> >         } else if (flags_ & ASSERT_PSR_DISABLED) {                      \
> > -               igt_assert(!psr_wait_until_enabled());                  \
> > +               if (!igt_psr_await_status(drm.fd, false)) {             \
> > +                       igt_psr_print_status(drm.fd);                   \
> > +                       igt_assert_f(false, "PSR enabled\n");           \
> > +               }                                                       \
> >         }                                                               \
> >  } while (0)
> >
> > @@ -1822,7 +1741,7 @@ static void enable_features_for_test(const struct test_mode *t)
> >         if (t->feature & FEATURE_FBC)
> >                 fbc_enable();
> >         if (t->feature & FEATURE_PSR)
> > -               psr_enable();
> > +               igt_psr_enable();
> >  }
> >
> >  static void check_test_requirements(const struct test_mode *t)
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx