[igt-dev] [PATCH 0/3] Add new subtest and test cleanup

[igt-dev] [PATCH 0/3] Add new subtest and test cleanup

[Nidhi Gupta]

Add new subtest to validate dual panel backlight,
create structure for subtests and convert exixting
subtests to dynamic subtests for better reporting.

Nidhi Gupta (3):
  tests/i915/i915_pm_backlight: Add new subtest to validate dual panel
    backlight
  tests/i915/i915_pm_backlight: Add a common structure for all subtests
  tests/i915/i915_pm_backlight: Create dynamic subtests

 tests/i915/i915_pm_backlight.c | 226 ++++++++++++++++++++++++++++-------------
 1 file changed, 157 insertions(+), 69 deletions(-)

-- 
1.9.1

[igt-dev] [PATCH 1/3] tests/i915/i915_pm_backlight: Add new subtest to validate dual panel backlight

[Nidhi Gupta]

Since driver can now support multiple eDPs and Debugfs structure for
backlight changed per connector the test should then iterate through
all eDP connectors.

Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
---
 tests/i915/i915_pm_backlight.c | 210 +++++++++++++++++++++++++++++------------
 1 file changed, 150 insertions(+), 60 deletions(-)

diff --git a/tests/i915/i915_pm_backlight.c b/tests/i915/i915_pm_backlight.c
index cafae7f..cf58f8b 100644
--- a/tests/i915/i915_pm_backlight.c
+++ b/tests/i915/i915_pm_backlight.c
@@ -34,29 +34,34 @@
 #include <errno.h>
 #include <unistd.h>
 #include <time.h>
+#include "igt_device.h"
+#include "igt_device_scan.h"
 
 struct context {
 	int max;
+	igt_output_t *output;
+	char path[PATH_MAX];
 };
 
-
 #define TOLERANCE 5 /* percent */
-#define BACKLIGHT_PATH "/sys/class/backlight/intel_backlight"
+#define BACKLIGHT_PATH "/sys/class/backlight"
 
 #define FADESTEPS 10
 #define FADESPEED 100 /* milliseconds between steps */
 
+#define NUM_EDP_OUTPUTS 2
+
 IGT_TEST_DESCRIPTION("Basic backlight sysfs test");
 
-static int backlight_read(int *result, const char *fname)
+static int backlight_read(int *result, const char *fname, struct context *context)
 {
 	int fd;
 	char full[PATH_MAX];
 	char dst[64];
 	int r, e;
 
-	igt_assert(snprintf(full, PATH_MAX, "%s/%s", BACKLIGHT_PATH, fname) < PATH_MAX);
-
+	igt_assert(snprintf(full, PATH_MAX, "%s/%s/%s", BACKLIGHT_PATH, context->path, fname)
+			    < PATH_MAX);
 	fd = open(full, O_RDONLY);
 	if (fd == -1)
 		return -errno;
@@ -73,14 +78,15 @@ static int backlight_read(int *result, const char *fname)
 	return errno;
 }
 
-static int backlight_write(int value, const char *fname)
+static int backlight_write(int value, const char *fname, struct context *context)
 {
 	int fd;
 	char full[PATH_MAX];
 	char src[64];
 	int len;
 
-	igt_assert(snprintf(full, PATH_MAX, "%s/%s", BACKLIGHT_PATH, fname) < PATH_MAX);
+	igt_assert(snprintf(full, PATH_MAX, "%s/%s/%s", BACKLIGHT_PATH, context->path, fname)
+		   < PATH_MAX);
 	fd = open(full, O_WRONLY);
 	if (fd == -1)
 		return -errno;
@@ -100,12 +106,12 @@ static void test_and_verify(struct context *context, int val)
 	const int tolerance = val * TOLERANCE / 100;
 	int result;
 
-	igt_assert_eq(backlight_write(val, "brightness"), 0);
-	igt_assert_eq(backlight_read(&result, "brightness"), 0);
+	igt_assert_eq(backlight_write(val, "brightness", context), 0);
+	igt_assert_eq(backlight_read(&result, "brightness", context), 0);
 	/* Check that the exact value sticks */
 	igt_assert_eq(result, val);
 
-	igt_assert_eq(backlight_read(&result, "actual_brightness"), 0);
+	igt_assert_eq(backlight_read(&result, "actual_brightness", context), 0);
 	/* Some rounding may happen depending on hw */
 	igt_assert_f(result >= max(0, val - tolerance) &&
 		     result <= min(context->max, val + tolerance),
@@ -124,16 +130,16 @@ static void test_bad_brightness(struct context *context)
 {
 	int val;
 	/* First write some sane value */
-	backlight_write(context->max / 2, "brightness");
+	backlight_write(context->max / 2, "brightness", context);
 	/* Writing invalid values should fail and not change the value */
-	igt_assert_lt(backlight_write(-1, "brightness"), 0);
-	backlight_read(&val, "brightness");
+	igt_assert_lt(backlight_write(-1, "brightness", context), 0);
+	backlight_read(&val, "brightness", context);
 	igt_assert_eq(val, context->max / 2);
-	igt_assert_lt(backlight_write(context->max + 1, "brightness"), 0);
-	backlight_read(&val, "brightness");
+	igt_assert_lt(backlight_write(context->max + 1, "brightness", context), 0);
+	backlight_read(&val, "brightness", context);
 	igt_assert_eq(val, context->max / 2);
-	igt_assert_lt(backlight_write(INT_MAX, "brightness"), 0);
-	backlight_read(&val, "brightness");
+	igt_assert_lt(backlight_write(INT_MAX, "brightness", context), 0);
+	backlight_read(&val, "brightness", context);
 	igt_assert_eq(val, context->max / 2);
 }
 
@@ -179,21 +185,57 @@ test_fade_with_suspend(struct context *context, igt_output_t *output)
 	test_fade(context);
 }
 
+static void test_cleanup(igt_display_t *display, igt_output_t *output)
+{
+	igt_output_set_pipe(output, PIPE_NONE);
+	igt_display_commit2(display, display->is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+	igt_pm_restore_sata_link_power_management();
+}
+
+static void test_setup(igt_display_t display, igt_output_t *output)
+{
+	igt_plane_t *primary;
+	drmModeModeInfo *mode;
+	struct igt_fb fb;
+	enum pipe pipe;
+
+	igt_display_reset(&display);
+
+	for_each_pipe(&display, pipe) {
+		if (!igt_pipe_connector_valid(pipe, output))
+			continue;
+
+		igt_output_set_pipe(output, pipe);
+		mode = igt_output_get_mode(output);
+
+		igt_create_pattern_fb(display.drm_fd,
+				      mode->hdisplay, mode->vdisplay,
+				      DRM_FORMAT_XRGB8888,
+				      DRM_FORMAT_MOD_LINEAR, &fb);
+		primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
+		igt_plane_set_fb(primary, &fb);
+
+		igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
+		igt_pm_enable_sata_link_power_management();
+
+		break;
+	}
+}
+
 igt_main
 {
-	struct context context = {0};
-	int old;
+	int old, fd;
+	int i = 0;
 	igt_display_t display;
 	igt_output_t *output;
-	struct igt_fb fb;
+	char file_path_n[PATH_MAX] = "";
+	bool dual_edp = false;
+	struct context contexts[NUM_EDP_OUTPUTS];
 
 	igt_fixture {
-		enum pipe pipe;
 		bool found = false;
 		char full_name[32] = {};
 		char *name;
-		drmModeModeInfo *mode;
-		igt_plane_t *primary;
 
 		/*
 		 * Backlight tests requires the output to be enabled,
@@ -202,56 +244,104 @@ igt_main
 		kmstest_set_vt_graphics_mode();
 		igt_display_require(&display, drm_open_driver(DRIVER_INTEL));
 
-		/* Get the max value and skip the whole test if sysfs interface not available */
-		igt_skip_on(backlight_read(&old, "brightness"));
-		igt_assert(backlight_read(&context.max, "max_brightness") > -1);
+		for_each_connected_output(&display, output) {
+			if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_eDP)
+				continue;
 
-		/* should be ../../cardX-$output */
-		igt_assert_lt(12, readlink(BACKLIGHT_PATH "/device", full_name, sizeof(full_name) - 1));
-		name = basename(full_name);
+			if (found)
+				snprintf(file_path_n, PATH_MAX, "%s/card%i-%s-backlight/brightness",
+					 BACKLIGHT_PATH, igt_device_get_card_index(display.drm_fd),
+					 igt_output_name(output));
+			else
+				snprintf(file_path_n, PATH_MAX, "%s/intel_backlight/brightness",
+					 BACKLIGHT_PATH);
 
-		for_each_pipe_with_valid_output(&display, pipe, output) {
-			if (strcmp(name + 6, output->name))
+			fd = open(file_path_n, O_RDONLY);
+			if (fd == -1)
 				continue;
-			found = true;
-			break;
-		}
 
-		igt_require_f(found,
-			      "Could not map backlight for \"%s\" to connected output\n",
-			      name);
-
-		igt_output_set_pipe(output, pipe);
-		mode = igt_output_get_mode(output);
+			if (found)
+				snprintf(contexts[i].path, PATH_MAX, "card%i-%s-backlight",
+					 igt_device_get_card_index(display.drm_fd),
+					 igt_output_name(output));
+			else
+				snprintf(contexts[i].path, PATH_MAX, "intel_backlight");
+
+			close(fd);
+
+			/* should be ../../cardX-$output */
+			snprintf(file_path_n, PATH_MAX, "%s/%s/device", BACKLIGHT_PATH,
+				 contexts[i].path);
+			igt_assert_lt(16, readlink(file_path_n, full_name, sizeof(full_name) - 1));
+			name = basename(full_name);
+
+			if (!strcmp(name + 6, output->name)) {
+				contexts[i++].output = output;
+
+				if (found)
+					dual_edp = true;
+				else
+					found = true;
+			}
+		}
+		igt_require_f(found, "No valid output found.\n");
+	}
 
-		igt_create_pattern_fb(display.drm_fd,
-				      mode->hdisplay, mode->vdisplay,
-				      DRM_FORMAT_XRGB8888,
-				      DRM_FORMAT_MOD_LINEAR, &fb);
-		primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
-		igt_plane_set_fb(primary, &fb);
+	igt_subtest("basic-brightness") {
+		for (i = 0; i < (dual_edp ? 2 : 1); i++) {
+			test_setup(display, &contexts->output[i]);
+			igt_assert(backlight_read(&contexts[i].max,
+						"max_brightness", &contexts[i]) > -1);
+			test_brightness(&contexts[i]);
+			test_cleanup(&display, output);
+		}
+	}
 
-		igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
-		igt_pm_enable_sata_link_power_management();
+	igt_subtest("bad-brightness") {
+		for (i = 0; i < (dual_edp ? 2 : 1); i++) {
+			test_setup(display, &contexts->output[i]);
+			igt_assert(backlight_read(&contexts[i].max,
+						"max_brightness", &contexts[i]) > -1);
+			test_bad_brightness(&contexts[i]);
+			test_cleanup(&display, output);
+		}
 	}
+	igt_subtest("fade") {
+		for (i = 0; i < (dual_edp ? 2 : 1); i++) {
+			test_setup(display, &contexts->output[i]);
+			igt_assert(backlight_read(&contexts[i].max,
+						"max_brightness", &contexts[i]) > -1);
+			test_fade(&contexts[i]);
+			test_cleanup(&display, output);
+		}
+	}
+	igt_subtest("fade_with_dpms") {
+		for (i = 0; i < (dual_edp ? 2 : 1); i++) {
+			test_setup(display, &contexts->output[i]);
+			igt_assert(backlight_read(&contexts[i].max,
+						"max_brightness", &contexts[i]) > -1);
+			test_fade_with_dpms(&contexts[i], output);
+			test_cleanup(&display, output);
+		}
+	}
+	igt_subtest("fade_with_suspend") {
+		for (i = 0; i < (dual_edp ? 2 : 1); i++) {
+			test_setup(display, &contexts->output[i]);
+			igt_assert(backlight_read(&contexts[i].max,
+						"max_brightness", &contexts[i]) > -1);
+			test_fade_with_suspend(&contexts[i], output);
+			test_cleanup(&display, output);
+		}
+	}
+
 
-	igt_subtest("basic-brightness")
-		test_brightness(&context);
-	igt_subtest("bad-brightness")
-		test_bad_brightness(&context);
-	igt_subtest("fade")
-		test_fade(&context);
-	igt_subtest("fade_with_dpms")
-		test_fade_with_dpms(&context, output);
-	igt_subtest("fade_with_suspend")
-		test_fade_with_suspend(&context, output);
 
 	igt_fixture {
 		/* Restore old brightness */
-		backlight_write(old, "brightness");
+		for (i = 0; i < (dual_edp ? 2 : 1); i++)
+			backlight_write(old, "brightness", &contexts[i]);
 
 		igt_display_fini(&display);
-		igt_remove_fb(display.drm_fd, &fb);
 		igt_pm_restore_sata_link_power_management();
 		close(display.drm_fd);
 	}
-- 
1.9.1

[igt-dev] [PATCH 2/3] tests/i915/i915_pm_backlight: Add a common structure for all subtests

[Nidhi Gupta]

Create a common structure for all the subtests which
includes the test name, test description and flag
indicating whether the test is suspend test or dpms test.
And then create the array of the above structure and
iterate over it.

Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
---
 tests/i915/i915_pm_backlight.c | 94 ++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 53 deletions(-)

diff --git a/tests/i915/i915_pm_backlight.c b/tests/i915/i915_pm_backlight.c
index cf58f8b..41ec215 100644
--- a/tests/i915/i915_pm_backlight.c
+++ b/tests/i915/i915_pm_backlight.c
@@ -43,6 +43,12 @@ struct context {
 	char path[PATH_MAX];
 };
 
+enum {
+	TEST_NONE = 0,
+	TEST_DPMS,
+	TEST_SUSPEND,
+};
+
 #define TOLERANCE 5 /* percent */
 #define BACKLIGHT_PATH "/sys/class/backlight"
 
@@ -160,7 +166,7 @@ static void test_fade(struct context *context)
 }
 
 static void
-test_fade_with_dpms(struct context *context, igt_output_t *output)
+check_dpms(igt_output_t *output)
 {
 	igt_require(igt_setup_runtime_pm(output->display->drm_fd));
 
@@ -173,16 +179,12 @@ test_fade_with_dpms(struct context *context, igt_output_t *output)
 				   output->config.connector,
 				   DRM_MODE_DPMS_ON);
 	igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_ACTIVE));
-
-	test_fade(context);
 }
 
 static void
-test_fade_with_suspend(struct context *context, igt_output_t *output)
+check_suspend(igt_output_t *output)
 {
 	igt_system_suspend_autoresume(SUSPEND_STATE_MEM, SUSPEND_TEST_NONE);
-
-	test_fade(context);
 }
 
 static void test_cleanup(igt_display_t *display, igt_output_t *output)
@@ -224,13 +226,25 @@ static void test_setup(igt_display_t display, igt_output_t *output)
 
 igt_main
 {
-	int old, fd;
+	int old[NUM_EDP_OUTPUTS], fd;
 	int i = 0;
 	igt_display_t display;
 	igt_output_t *output;
 	char file_path_n[PATH_MAX] = "";
 	bool dual_edp = false;
 	struct context contexts[NUM_EDP_OUTPUTS];
+	struct {
+		const char *name;
+		const char *desc;
+		void (*test_t) (struct context *);
+		int flags;
+	} tests[] = {
+		{ "basic-brightness", "test the basic brightness.", test_brightness, TEST_NONE },
+		{ "bad-brightness", "test the bad brightness.", test_bad_brightness, TEST_NONE },
+		{ "fade", "test basic fade.", test_fade, TEST_NONE },
+		{ "fade-with-dpms", "test the fade with DPMS.", test_fade, TEST_DPMS },
+		{ "fade-with-suspend", "test the fade with suspend.", test_fade, TEST_SUSPEND },
+	};
 
 	igt_fixture {
 		bool found = false;
@@ -287,59 +301,33 @@ igt_main
 		igt_require_f(found, "No valid output found.\n");
 	}
 
-	igt_subtest("basic-brightness") {
-		for (i = 0; i < (dual_edp ? 2 : 1); i++) {
-			test_setup(display, &contexts->output[i]);
-			igt_assert(backlight_read(&contexts[i].max,
-						"max_brightness", &contexts[i]) > -1);
-			test_brightness(&contexts[i]);
-			test_cleanup(&display, output);
-		}
-	}
+	for (i = 0; i < ARRAY_SIZE(tests); i++) {
+		igt_describe(tests[i].desc);
+		igt_subtest(tests[i].name) {
+			for (int j = 0; j < (dual_edp ? 2 : 1); j++) {
+				test_setup(display, &contexts->output[j]);
+
+				if (backlight_read(&old[j], "brightness", &contexts[j]))
+					continue;
+
+				if (tests[i].flags == TEST_DPMS)
+					check_dpms(contexts[j].output);
+
+				if (tests[i].flags == TEST_SUSPEND)
+					check_suspend(contexts[j].output);
+
+				igt_assert(backlight_read(&contexts[j].max, "max_brightness", &contexts[j]) > -1);
+				tests[i].test_t(&contexts[j]);
+			}
 
-	igt_subtest("bad-brightness") {
-		for (i = 0; i < (dual_edp ? 2 : 1); i++) {
-			test_setup(display, &contexts->output[i]);
-			igt_assert(backlight_read(&contexts[i].max,
-						"max_brightness", &contexts[i]) > -1);
-			test_bad_brightness(&contexts[i]);
-			test_cleanup(&display, output);
-		}
-	}
-	igt_subtest("fade") {
-		for (i = 0; i < (dual_edp ? 2 : 1); i++) {
-			test_setup(display, &contexts->output[i]);
-			igt_assert(backlight_read(&contexts[i].max,
-						"max_brightness", &contexts[i]) > -1);
-			test_fade(&contexts[i]);
-			test_cleanup(&display, output);
-		}
-	}
-	igt_subtest("fade_with_dpms") {
-		for (i = 0; i < (dual_edp ? 2 : 1); i++) {
-			test_setup(display, &contexts->output[i]);
-			igt_assert(backlight_read(&contexts[i].max,
-						"max_brightness", &contexts[i]) > -1);
-			test_fade_with_dpms(&contexts[i], output);
-			test_cleanup(&display, output);
-		}
-	}
-	igt_subtest("fade_with_suspend") {
-		for (i = 0; i < (dual_edp ? 2 : 1); i++) {
-			test_setup(display, &contexts->output[i]);
-			igt_assert(backlight_read(&contexts[i].max,
-						"max_brightness", &contexts[i]) > -1);
-			test_fade_with_suspend(&contexts[i], output);
 			test_cleanup(&display, output);
 		}
 	}
 
-
-
 	igt_fixture {
 		/* Restore old brightness */
-		for (i = 0; i < (dual_edp ? 2 : 1); i++)
-			backlight_write(old, "brightness", &contexts[i]);
+		for (int j = 0; j < (dual_edp ? 2 : 1); j++)
+			backlight_write(old[j], "brightness", &contexts[j]);
 
 		igt_display_fini(&display);
 		igt_pm_restore_sata_link_power_management();
-- 
1.9.1

[igt-dev] [PATCH 3/3] tests/i915/i915_pm_backlight: Create dynamic subtests

[Nidhi Gupta]

-Modified the test to include dynamic subtests.
-Replaced all asserts and igt_require inside
 dynamic subtests, as skip is not allowed in
 dynamic structure.

Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
---
 tests/i915/i915_pm_backlight.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/tests/i915/i915_pm_backlight.c b/tests/i915/i915_pm_backlight.c
index 41ec215..ab2787c 100644
--- a/tests/i915/i915_pm_backlight.c
+++ b/tests/i915/i915_pm_backlight.c
@@ -165,20 +165,25 @@ static void test_fade(struct context *context)
 	}
 }
 
-static void
+static int
 check_dpms(igt_output_t *output)
 {
-	igt_require(igt_setup_runtime_pm(output->display->drm_fd));
+	if ((igt_setup_runtime_pm(output->display->drm_fd)) == 0)
+		return -errno;
 
 	kmstest_set_connector_dpms(output->display->drm_fd,
 				   output->config.connector,
 				   DRM_MODE_DPMS_OFF);
-	igt_require(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSPENDED));
+	if ((igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSPENDED)) == 0)
+		return -errno;
 
 	kmstest_set_connector_dpms(output->display->drm_fd,
 				   output->config.connector,
 				   DRM_MODE_DPMS_ON);
-	igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_ACTIVE));
+	if ((igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_ACTIVE)) == 0)
+		return -errno;
+
+	return 1;
 }
 
 static void
@@ -227,7 +232,7 @@ static void test_setup(igt_display_t display, igt_output_t *output)
 igt_main
 {
 	int old[NUM_EDP_OUTPUTS], fd;
-	int i = 0;
+	int i = 0, ret = 0;
 	igt_display_t display;
 	igt_output_t *output;
 	char file_path_n[PATH_MAX] = "";
@@ -303,24 +308,29 @@ igt_main
 
 	for (i = 0; i < ARRAY_SIZE(tests); i++) {
 		igt_describe(tests[i].desc);
-		igt_subtest(tests[i].name) {
+		igt_subtest_with_dynamic(tests[i].name) {
 			for (int j = 0; j < (dual_edp ? 2 : 1); j++) {
 				test_setup(display, &contexts->output[j]);
 
 				if (backlight_read(&old[j], "brightness", &contexts[j]))
 					continue;
 
-				if (tests[i].flags == TEST_DPMS)
-					check_dpms(contexts[j].output);
+				if (tests[i].flags == TEST_DPMS) {
+					ret = check_dpms(contexts[j].output);
+					if (ret == 0)
+						continue;
+				}
 
 				if (tests[i].flags == TEST_SUSPEND)
 					check_suspend(contexts[j].output);
 
-				igt_assert(backlight_read(&contexts[j].max, "max_brightness", &contexts[j]) > -1);
-				tests[i].test_t(&contexts[j]);
+				igt_dynamic_f("%s", igt_output_name(contexts[j].output)) {
+					igt_assert(backlight_read(&contexts[j].max, "max_brightness", &contexts[j]) > -1);
+					tests[i].test_t(&contexts[j]);
+				}
+				test_cleanup(&display, output);
 			}
-
-			test_cleanup(&display, output);
+			/* TODO: Add tests for dual eDP. */
 		}
 	}
 
-- 
1.9.1

[igt-dev] ✗ Fi.CI.BAT: failure for Add new subtest and test cleanup (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 14640 bytes --]

== Series Details ==

Series: Add new subtest and test cleanup (rev2)
URL   : https://patchwork.freedesktop.org/series/110845/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12379 -> IGTPW_8100
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_8100 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_8100, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/index.html

Participating hosts (40 -> 40)
------------------------------

  Additional (2): fi-rkl-11600 fi-tgl-dsi 
  Missing    (2): bat-atsm-1 fi-bdw-samus 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_8100:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_pm_backlight@basic-brightness:
    - fi-rkl-11600:       NOTRUN -> [SKIP][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-11600/igt@i915_pm_backlight@basic-brightness.html

  
#### Warnings ####

  * igt@i915_pm_backlight@basic-brightness:
    - fi-rkl-guc:         [SKIP][2] ([i915#3012]) -> [SKIP][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/fi-rkl-guc/igt@i915_pm_backlight@basic-brightness.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-guc/igt@i915_pm_backlight@basic-brightness.html
    - bat-dg1-6:          [SKIP][4] ([i915#1155]) -> [SKIP][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/bat-dg1-6/igt@i915_pm_backlight@basic-brightness.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/bat-dg1-6/igt@i915_pm_backlight@basic-brightness.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_pm_backlight@basic-brightness:
    - {bat-rpls-2}:       [SKIP][6] ([i915#1155]) -> [SKIP][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/bat-rpls-2/igt@i915_pm_backlight@basic-brightness.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/bat-rpls-2/igt@i915_pm_backlight@basic-brightness.html
    - {bat-dg2-11}:       [SKIP][8] ([i915#1155]) -> [SKIP][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/bat-dg2-11/igt@i915_pm_backlight@basic-brightness.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/bat-dg2-11/igt@i915_pm_backlight@basic-brightness.html
    - {bat-dg2-8}:        [SKIP][10] ([i915#1155]) -> [SKIP][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/bat-dg2-8/igt@i915_pm_backlight@basic-brightness.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/bat-dg2-8/igt@i915_pm_backlight@basic-brightness.html
    - {bat-adlm-1}:       [SKIP][12] ([i915#1155]) -> [SKIP][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/bat-adlm-1/igt@i915_pm_backlight@basic-brightness.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/bat-adlm-1/igt@i915_pm_backlight@basic-brightness.html
    - {bat-rpls-1}:       [SKIP][14] ([i915#1155]) -> [SKIP][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/bat-rpls-1/igt@i915_pm_backlight@basic-brightness.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/bat-rpls-1/igt@i915_pm_backlight@basic-brightness.html
    - {fi-tgl-dsi}:       NOTRUN -> [SKIP][16]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-tgl-dsi/igt@i915_pm_backlight@basic-brightness.html
    - {bat-dg2-9}:        [SKIP][17] ([i915#1155]) -> [SKIP][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/bat-dg2-9/igt@i915_pm_backlight@basic-brightness.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/bat-dg2-9/igt@i915_pm_backlight@basic-brightness.html

  
New tests
---------

  New tests have been introduced between CI_DRM_12379 and IGTPW_8100:

### New IGT tests (1) ###

  * igt@i915_pm_backlight@basic-brightness@edp-1:
    - Statuses : 10 pass(s)
    - Exec time: [0.0] s

  

Known issues
------------

  Here are the changes found in IGTPW_8100 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_gttfill@basic:
    - fi-pnv-d510:        [PASS][19] -> [FAIL][20] ([i915#7229])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/fi-pnv-d510/igt@gem_exec_gttfill@basic.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-pnv-d510/igt@gem_exec_gttfill@basic.html

  * igt@gem_huc_copy@huc-copy:
    - fi-rkl-11600:       NOTRUN -> [SKIP][21] ([i915#2190])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-11600/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-rkl-11600:       NOTRUN -> [SKIP][22] ([i915#4613]) +3 similar issues
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-11600/igt@gem_lmem_swapping@basic.html

  * igt@gem_tiled_pread_basic:
    - fi-rkl-11600:       NOTRUN -> [SKIP][23] ([i915#3282])
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-11600/igt@gem_tiled_pread_basic.html

  * igt@i915_pm_backlight@basic-brightness:
    - fi-snb-2520m:       [PASS][24] -> [SKIP][25] ([fdo#109271])
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/fi-snb-2520m/igt@i915_pm_backlight@basic-brightness.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-snb-2520m/igt@i915_pm_backlight@basic-brightness.html
    - fi-bxt-dsi:         [PASS][26] -> [SKIP][27] ([fdo#109271])
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/fi-bxt-dsi/igt@i915_pm_backlight@basic-brightness.html
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-bxt-dsi/igt@i915_pm_backlight@basic-brightness.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-bxt-dsi:         [PASS][28] -> [DMESG-FAIL][29] ([i915#5334])
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/fi-bxt-dsi/igt@i915_selftest@live@gt_heartbeat.html
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-bxt-dsi/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@hangcheck:
    - fi-hsw-4770:        [PASS][30] -> [INCOMPLETE][31] ([i915#4785])
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-rkl-11600:       NOTRUN -> [INCOMPLETE][32] ([i915#4817])
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-11600/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_chamelium@hdmi-edid-read:
    - fi-rkl-11600:       NOTRUN -> [SKIP][33] ([fdo#111827]) +7 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-11600/igt@kms_chamelium@hdmi-edid-read.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor:
    - fi-rkl-11600:       NOTRUN -> [SKIP][34] ([i915#4103])
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-11600/igt@kms_cursor_legacy@basic-busy-flip-before-cursor.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-rkl-11600:       NOTRUN -> [SKIP][35] ([fdo#109285] / [i915#4098])
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-11600/igt@kms_force_connector_basic@force-load-detect.html

  * igt@kms_psr@primary_page_flip:
    - fi-rkl-11600:       NOTRUN -> [SKIP][36] ([i915#1072]) +3 similar issues
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-11600/igt@kms_psr@primary_page_flip.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - fi-rkl-11600:       NOTRUN -> [SKIP][37] ([i915#3555] / [i915#4098])
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-11600/igt@kms_setmode@basic-clone-single-crtc.html

  * igt@prime_vgem@basic-read:
    - fi-rkl-11600:       NOTRUN -> [SKIP][38] ([fdo#109295] / [i915#3291] / [i915#3708]) +2 similar issues
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-11600/igt@prime_vgem@basic-read.html

  * igt@prime_vgem@basic-userptr:
    - fi-rkl-11600:       NOTRUN -> [SKIP][39] ([fdo#109295] / [i915#3301] / [i915#3708])
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-rkl-11600/igt@prime_vgem@basic-userptr.html

  * igt@runner@aborted:
    - fi-hsw-4770:        NOTRUN -> [FAIL][40] ([fdo#109271] / [i915#4312] / [i915#5594])
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-hsw-4770/igt@runner@aborted.html

  
#### Possible fixes ####

  * igt@fbdev@read:
    - {bat-rpls-2}:       [SKIP][41] ([i915#2582]) -> [PASS][42] +4 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/bat-rpls-2/igt@fbdev@read.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/bat-rpls-2/igt@fbdev@read.html

  * igt@gem_huc_copy@huc-copy:
    - {bat-dg2-8}:        [FAIL][43] ([i915#7029]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/bat-dg2-8/igt@gem_huc_copy@huc-copy.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/bat-dg2-8/igt@gem_huc_copy@huc-copy.html

  * igt@i915_selftest@live@hugepages:
    - {bat-rpls-2}:       [DMESG-WARN][45] ([i915#5278]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/bat-rpls-2/igt@i915_selftest@live@hugepages.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/bat-rpls-2/igt@i915_selftest@live@hugepages.html

  * igt@i915_selftest@live@migrate:
    - {bat-adlp-6}:       [INCOMPLETE][47] ([i915#7348]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/bat-adlp-6/igt@i915_selftest@live@migrate.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/bat-adlp-6/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@requests:
    - {bat-rpls-1}:       [INCOMPLETE][49] ([i915#6257]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/bat-rpls-1/igt@i915_selftest@live@requests.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/bat-rpls-1/igt@i915_selftest@live@requests.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions:
    - fi-bsw-kefka:       [FAIL][51] ([i915#6298]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor@atomic-transitions.html

  
#### Warnings ####

  * igt@i915_pm_backlight@basic-brightness:
    - fi-hsw-4770:        [SKIP][53] ([fdo#109271] / [i915#3012]) -> [SKIP][54] ([fdo#109271])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12379/fi-hsw-4770/igt@i915_pm_backlight@basic-brightness.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/fi-hsw-4770/igt@i915_pm_backlight@basic-brightness.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1155]: https://gitlab.freedesktop.org/drm/intel/issues/1155
  [i915#1759]: https://gitlab.freedesktop.org/drm/intel/issues/1759
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#3012]: https://gitlab.freedesktop.org/drm/intel/issues/3012
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3301]: https://gitlab.freedesktop.org/drm/intel/issues/3301
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4785]: https://gitlab.freedesktop.org/drm/intel/issues/4785
  [i915#4817]: https://gitlab.freedesktop.org/drm/intel/issues/4817
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5278]: https://gitlab.freedesktop.org/drm/intel/issues/5278
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#5594]: https://gitlab.freedesktop.org/drm/intel/issues/5594
  [i915#6257]: https://gitlab.freedesktop.org/drm/intel/issues/6257
  [i915#6298]: https://gitlab.freedesktop.org/drm/intel/issues/6298
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6434]: https://gitlab.freedesktop.org/drm/intel/issues/6434
  [i915#7029]: https://gitlab.freedesktop.org/drm/intel/issues/7029
  [i915#7229]: https://gitlab.freedesktop.org/drm/intel/issues/7229
  [i915#7346]: https://gitlab.freedesktop.org/drm/intel/issues/7346
  [i915#7348]: https://gitlab.freedesktop.org/drm/intel/issues/7348
  [i915#7456]: https://gitlab.freedesktop.org/drm/intel/issues/7456


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7056 -> IGTPW_8100

  CI-20190529: 20190529
  CI_DRM_12379: fbd41f5bba3fa2af510a37dadb4ef872e2c54701 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8100: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/index.html
  IGT_7056: 05096a6754048f4b5a2de798ab4bea32012b556a @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git


Testlist changes
----------------

+igt@i915_pm_backlight@fade-with-dpms
+igt@i915_pm_backlight@fade-with-suspend
-igt@i915_pm_backlight@fade_with_dpms
-igt@i915_pm_backlight@fade_with_suspend

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8100/index.html

[-- Attachment #2: Type: text/html, Size: 16328 bytes --]

Re: [igt-dev] [PATCH 2/3] tests/i915/i915_pm_backlight: Add a common structure for all subtests

[Hogander, Jouni]

One inline comment below.

On Tue, 2022-11-15 at 11:46 +0530, Nidhi Gupta wrote:
> Create a common structure for all the subtests which
> includes the test name, test description and flag
> indicating whether the test is suspend test or dpms test.
> And then create the array of the above structure and
> iterate over it.
> 
> Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
> ---
>  tests/i915/i915_pm_backlight.c | 94 ++++++++++++++++++--------------
> ----------
>  1 file changed, 41 insertions(+), 53 deletions(-)
> 
> diff --git a/tests/i915/i915_pm_backlight.c
> b/tests/i915/i915_pm_backlight.c
> index cf58f8b..41ec215 100644
> --- a/tests/i915/i915_pm_backlight.c
> +++ b/tests/i915/i915_pm_backlight.c
> @@ -43,6 +43,12 @@ struct context {
>         char path[PATH_MAX];
>  };
>  
> +enum {
> +       TEST_NONE = 0,
> +       TEST_DPMS,
> +       TEST_SUSPEND,
> +};
> +
>  #define TOLERANCE 5 /* percent */
>  #define BACKLIGHT_PATH "/sys/class/backlight"
>  
> @@ -160,7 +166,7 @@ static void test_fade(struct context *context)
>  }
>  
>  static void
> -test_fade_with_dpms(struct context *context, igt_output_t *output)
> +check_dpms(igt_output_t *output)
>  {
>         igt_require(igt_setup_runtime_pm(output->display->drm_fd));
>  
> @@ -173,16 +179,12 @@ test_fade_with_dpms(struct context *context,
> igt_output_t *output)
>                                    output->config.connector,
>                                    DRM_MODE_DPMS_ON);
>         igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_ACTIV
> E));
> -
> -       test_fade(context);
>  }
>  
>  static void
> -test_fade_with_suspend(struct context *context, igt_output_t
> *output)
> +check_suspend(igt_output_t *output)
>  {
>         igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
> SUSPEND_TEST_NONE);
> -
> -       test_fade(context);
>  }
>  
>  static void test_cleanup(igt_display_t *display, igt_output_t
> *output)
> @@ -224,13 +226,25 @@ static void test_setup(igt_display_t display,
> igt_output_t *output)
>  
>  igt_main
>  {
> -       int old, fd;
> +       int old[NUM_EDP_OUTPUTS], fd;
>         int i = 0;
>         igt_display_t display;
>         igt_output_t *output;
>         char file_path_n[PATH_MAX] = "";
>         bool dual_edp = false;
>         struct context contexts[NUM_EDP_OUTPUTS];
> +       struct {
> +               const char *name;
> +               const char *desc;
> +               void (*test_t) (struct context *);
> +               int flags;
> +       } tests[] = {
> +               { "basic-brightness", "test the basic brightness.",
> test_brightness, TEST_NONE },
> +               { "bad-brightness", "test the bad brightness.",
> test_bad_brightness, TEST_NONE },
> +               { "fade", "test basic fade.", test_fade, TEST_NONE },
> +               { "fade-with-dpms", "test the fade with DPMS.",
> test_fade, TEST_DPMS },
> +               { "fade-with-suspend", "test the fade with suspend.",
> test_fade, TEST_SUSPEND },
> +       };
>  
>         igt_fixture {
>                 bool found = false;
> @@ -287,59 +301,33 @@ igt_main
>                 igt_require_f(found, "No valid output found.\n");
>         }
>  
> -       igt_subtest("basic-brightness") {
> -               for (i = 0; i < (dual_edp ? 2 : 1); i++) {
> -                       test_setup(display, &contexts->output[i]);
> -                       igt_assert(backlight_read(&contexts[i].max,
> -                                               "max_brightness",
> &contexts[i]) > -1);
> -                       test_brightness(&contexts[i]);
> -                       test_cleanup(&display, output);
> -               }
> -       }
> +       for (i = 0; i < ARRAY_SIZE(tests); i++) {
> +               igt_describe(tests[i].desc);
> +               igt_subtest(tests[i].name) {
> +                       for (int j = 0; j < (dual_edp ? 2 : 1); j++)
> {
> +                               test_setup(display, &contexts-
> >output[j]);
> +
> +                               if (backlight_read(&old[j],
> "brightness", &contexts[j]))
> +                                       continue;
> +
> +                               if (tests[i].flags == TEST_DPMS)
> +                                       check_dpms(contexts[j].output
> );
> +
> +                               if (tests[i].flags == TEST_SUSPEND)
> +                                       check_suspend(contexts[j].out
> put);
> +
> +                               igt_assert(backlight_read(&contexts[j
> ].max, "max_brightness", &contexts[j]) > -1);
> +                               tests[i].test_t(&contexts[j]);
> +                       }
>  
> -       igt_subtest("bad-brightness") {
> -               for (i = 0; i < (dual_edp ? 2 : 1); i++) {
> -                       test_setup(display, &contexts->output[i]);
> -                       igt_assert(backlight_read(&contexts[i].max,
> -                                               "max_brightness",
> &contexts[i]) > -1);
> -                       test_bad_brightness(&contexts[i]);
> -                       test_cleanup(&display, output);
> -               }
> -       }
> -       igt_subtest("fade") {
> -               for (i = 0; i < (dual_edp ? 2 : 1); i++) {
> -                       test_setup(display, &contexts->output[i]);
> -                       igt_assert(backlight_read(&contexts[i].max,
> -                                               "max_brightness",
> &contexts[i]) > -1);
> -                       test_fade(&contexts[i]);
> -                       test_cleanup(&display, output);
> -               }
> -       }
> -       igt_subtest("fade_with_dpms") {
> -               for (i = 0; i < (dual_edp ? 2 : 1); i++) {
> -                       test_setup(display, &contexts->output[i]);
> -                       igt_assert(backlight_read(&contexts[i].max,
> -                                               "max_brightness",
> &contexts[i]) > -1);
> -                       test_fade_with_dpms(&contexts[i], output);
> -                       test_cleanup(&display, output);
> -               }
> -       }
> -       igt_subtest("fade_with_suspend") {
> -               for (i = 0; i < (dual_edp ? 2 : 1); i++) {
> -                       test_setup(display, &contexts->output[i]);
> -                       igt_assert(backlight_read(&contexts[i].max,
> -                                               "max_brightness",
> &contexts[i]) > -1);
> -                       test_fade_with_suspend(&contexts[i], output);
>                         test_cleanup(&display, output);
>                 }
>         }
>  
> -
> -
>         igt_fixture {
>                 /* Restore old brightness */
> -               for (i = 0; i < (dual_edp ? 2 : 1); i++)
> -                       backlight_write(old, "brightness",
> &contexts[i]);
> +               for (int j = 0; j < (dual_edp ? 2 : 1); j++)
> +                       backlight_write(old[j], "brightness",
> &contexts[j]);

I this change belongs to previous patch.

>  
>                 igt_display_fini(&display);
>                 igt_pm_restore_sata_link_power_management();

Re: [igt-dev] [PATCH 3/3] tests/i915/i915_pm_backlight: Create dynamic subtests

[Hogander, Jouni]

On Tue, 2022-11-15 at 11:46 +0530, Nidhi Gupta wrote:
> -Modified the test to include dynamic subtests.
> -Replaced all asserts and igt_require inside
>  dynamic subtests, as skip is not allowed in
>  dynamic structure.
> 
> Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
> ---
>  tests/i915/i915_pm_backlight.c | 34 ++++++++++++++++++++++----------
> --
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/tests/i915/i915_pm_backlight.c
> b/tests/i915/i915_pm_backlight.c
> index 41ec215..ab2787c 100644
> --- a/tests/i915/i915_pm_backlight.c
> +++ b/tests/i915/i915_pm_backlight.c
> @@ -165,20 +165,25 @@ static void test_fade(struct context *context)
>         }
>  }
>  
> -static void
> +static int
>  check_dpms(igt_output_t *output)
>  {
> -       igt_require(igt_setup_runtime_pm(output->display->drm_fd));
> +       if ((igt_setup_runtime_pm(output->display->drm_fd)) == 0)
> +               return -errno;
>  
>         kmstest_set_connector_dpms(output->display->drm_fd,
>                                    output->config.connector,
>                                    DRM_MODE_DPMS_OFF);
> -
>        igt_require(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSPEN
> DED));
> +       if ((igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSPENDED))
> == 0)
> +               return -errno;

Are you sure that errno is set for all possible failures in
igt_setup_runtime_pm, igt_wait_for_pm_status and
igt_wait_for_pm_status? 
>  
>         kmstest_set_connector_dpms(output->display->drm_fd,
>                                    output->config.connector,
>                                    DRM_MODE_DPMS_ON);
> -
>        igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_ACTIVE)
> );
> +       if ((igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_ACTIVE)) ==
> 0)
> +               return -errno;
> +
> +       return 1;

Good practice is to use 0 to indicate success.

>  }
>  
>  static void
> @@ -227,7 +232,7 @@ static void test_setup(igt_display_t display,
> igt_output_t *output)
>  igt_main
>  {
>         int old[NUM_EDP_OUTPUTS], fd;
> -       int i = 0;
> +       int i = 0, ret = 0;
>         igt_display_t display;
>         igt_output_t *output;
>         char file_path_n[PATH_MAX] = "";
> @@ -303,24 +308,29 @@ igt_main
>  
>         for (i = 0; i < ARRAY_SIZE(tests); i++) {
>                 igt_describe(tests[i].desc);
> -               igt_subtest(tests[i].name) {
> +               igt_subtest_with_dynamic(tests[i].name) {
>                         for (int j = 0; j < (dual_edp ? 2 : 1); j++)
> {
>                                 test_setup(display, &contexts-
> >output[j]);
>  
>                                 if (backlight_read(&old[j],
> "brightness", &contexts[j]))
>                                         continue;
>  
> -                               if (tests[i].flags == TEST_DPMS)
> -
>                                        check_dpms(contexts[j].output);
> +                               if (tests[i].flags == TEST_DPMS) {
> +                                       ret =
> check_dpms(contexts[j].output);
> +                                       if (ret == 0)
> +                                               continue;
> +                               }

So is 0 failure or success? Earlier there were those asserts inside
check_dpms, but now the test is just quietly not run if check_dpms
fails? What is the problem with dynamic subtest and those asserts?

>  
>                                 if (tests[i].flags == TEST_SUSPEND)
>                                         check_suspend(contexts[j].out
> put);
>  
> -
>                                igt_assert(backlight_read(&contexts[j].
> max, "max_brightness", &contexts[j]) > -1);
> -                               tests[i].test_t(&contexts[j]);
> +                               igt_dynamic_f("%s",
> igt_output_name(contexts[j].output)) {
> +                                       igt_assert(backlight_read(&co
> ntexts[j].max, "max_brightness", &contexts[j]) > -1);
> +                                       tests[i].test_t(&contexts[j])
> ;
> +                               }
> +                               test_cleanup(&display, output);
>                         }
> -
> -                       test_cleanup(&display, output);
> +                       /* TODO: Add tests for dual eDP. */
>                 }
>         }
>  

Re: [igt-dev] [PATCH 1/3] tests/i915/i915_pm_backlight: Add new subtest to validate dual panel backlight

[Hogander, Jouni]

One inline comment below.

On Tue, 2022-11-15 at 11:46 +0530, Nidhi Gupta wrote:
> Since driver can now support multiple eDPs and Debugfs structure for
> backlight changed per connector the test should then iterate through
> all eDP connectors.
> 
> Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> ---
>  tests/i915/i915_pm_backlight.c | 210 +++++++++++++++++++++++++++++--
> ----------
>  1 file changed, 150 insertions(+), 60 deletions(-)
> 
> diff --git a/tests/i915/i915_pm_backlight.c
> b/tests/i915/i915_pm_backlight.c
> index cafae7f..cf58f8b 100644
> --- a/tests/i915/i915_pm_backlight.c
> +++ b/tests/i915/i915_pm_backlight.c
> @@ -34,29 +34,34 @@
>  #include <errno.h>
>  #include <unistd.h>
>  #include <time.h>
> +#include "igt_device.h"
> +#include "igt_device_scan.h"
>  
>  struct context {
>         int max;
> +       igt_output_t *output;
> +       char path[PATH_MAX];
>  };
>  
> -
>  #define TOLERANCE 5 /* percent */
> -#define BACKLIGHT_PATH "/sys/class/backlight/intel_backlight"
> +#define BACKLIGHT_PATH "/sys/class/backlight"
>  
>  #define FADESTEPS 10
>  #define FADESPEED 100 /* milliseconds between steps */
>  
> +#define NUM_EDP_OUTPUTS 2
> +
>  IGT_TEST_DESCRIPTION("Basic backlight sysfs test");
>  
> -static int backlight_read(int *result, const char *fname)
> +static int backlight_read(int *result, const char *fname, struct
> context *context)
>  {
>         int fd;
>         char full[PATH_MAX];
>         char dst[64];
>         int r, e;
>  
> -       igt_assert(snprintf(full, PATH_MAX, "%s/%s", BACKLIGHT_PATH,
> fname) < PATH_MAX);
> -
> +       igt_assert(snprintf(full, PATH_MAX, "%s/%s/%s",
> BACKLIGHT_PATH, context->path, fname)
> +                           < PATH_MAX);
>         fd = open(full, O_RDONLY);
>         if (fd == -1)
>                 return -errno;
> @@ -73,14 +78,15 @@ static int backlight_read(int *result, const char
> *fname)
>         return errno;
>  }
>  
> -static int backlight_write(int value, const char *fname)
> +static int backlight_write(int value, const char *fname, struct
> context *context)
>  {
>         int fd;
>         char full[PATH_MAX];
>         char src[64];
>         int len;
>  
> -       igt_assert(snprintf(full, PATH_MAX, "%s/%s", BACKLIGHT_PATH,
> fname) < PATH_MAX);
> +       igt_assert(snprintf(full, PATH_MAX, "%s/%s/%s",
> BACKLIGHT_PATH, context->path, fname)
> +                  < PATH_MAX);
>         fd = open(full, O_WRONLY);
>         if (fd == -1)
>                 return -errno;
> @@ -100,12 +106,12 @@ static void test_and_verify(struct context
> *context, int val)
>         const int tolerance = val * TOLERANCE / 100;
>         int result;
>  
> -       igt_assert_eq(backlight_write(val, "brightness"), 0);
> -       igt_assert_eq(backlight_read(&result, "brightness"), 0);
> +       igt_assert_eq(backlight_write(val, "brightness", context),
> 0);
> +       igt_assert_eq(backlight_read(&result, "brightness", context),
> 0);
>         /* Check that the exact value sticks */
>         igt_assert_eq(result, val);
>  
> -       igt_assert_eq(backlight_read(&result, "actual_brightness"),
> 0);
> +       igt_assert_eq(backlight_read(&result, "actual_brightness",
> context), 0);
>         /* Some rounding may happen depending on hw */
>         igt_assert_f(result >= max(0, val - tolerance) &&
>                      result <= min(context->max, val + tolerance),
> @@ -124,16 +130,16 @@ static void test_bad_brightness(struct context
> *context)
>  {
>         int val;
>         /* First write some sane value */
> -       backlight_write(context->max / 2, "brightness");
> +       backlight_write(context->max / 2, "brightness", context);
>         /* Writing invalid values should fail and not change the
> value */
> -       igt_assert_lt(backlight_write(-1, "brightness"), 0);
> -       backlight_read(&val, "brightness");
> +       igt_assert_lt(backlight_write(-1, "brightness", context), 0);
> +       backlight_read(&val, "brightness", context);
>         igt_assert_eq(val, context->max / 2);
> -       igt_assert_lt(backlight_write(context->max + 1,
> "brightness"), 0);
> -       backlight_read(&val, "brightness");
> +       igt_assert_lt(backlight_write(context->max + 1, "brightness",
> context), 0);
> +       backlight_read(&val, "brightness", context);
>         igt_assert_eq(val, context->max / 2);
> -       igt_assert_lt(backlight_write(INT_MAX, "brightness"), 0);
> -       backlight_read(&val, "brightness");
> +       igt_assert_lt(backlight_write(INT_MAX, "brightness",
> context), 0);
> +       backlight_read(&val, "brightness", context);
>         igt_assert_eq(val, context->max / 2);
>  }
>  
> @@ -179,21 +185,57 @@ test_fade_with_suspend(struct context *context,
> igt_output_t *output)
>         test_fade(context);
>  }
>  
> +static void test_cleanup(igt_display_t *display, igt_output_t
> *output)
> +{
> +       igt_output_set_pipe(output, PIPE_NONE);
> +       igt_display_commit2(display, display->is_atomic ?
> COMMIT_ATOMIC : COMMIT_LEGACY);
> +       igt_pm_restore_sata_link_power_management();
> +}
> +
> +static void test_setup(igt_display_t display, igt_output_t *output)
> +{
> +       igt_plane_t *primary;
> +       drmModeModeInfo *mode;
> +       struct igt_fb fb;
> +       enum pipe pipe;
> +
> +       igt_display_reset(&display);
> +
> +       for_each_pipe(&display, pipe) {
> +               if (!igt_pipe_connector_valid(pipe, output))
> +                       continue;
> +
> +               igt_output_set_pipe(output, pipe);
> +               mode = igt_output_get_mode(output);
> +
> +               igt_create_pattern_fb(display.drm_fd,
> +                                     mode->hdisplay, mode->vdisplay,
> +                                     DRM_FORMAT_XRGB8888,
> +                                     DRM_FORMAT_MOD_LINEAR, &fb);
> +               primary = igt_output_get_plane_type(output,
> DRM_PLANE_TYPE_PRIMARY);
> +               igt_plane_set_fb(primary, &fb);
> +
> +               igt_display_commit2(&display, display.is_atomic ?
> COMMIT_ATOMIC : COMMIT_LEGACY);
> +               igt_pm_enable_sata_link_power_management();
> +
> +               break;
> +       }
> +}
> +
>  igt_main
>  {
> -       struct context context = {0};
> -       int old;
> +       int old, fd;
> +       int i = 0;
>         igt_display_t display;
>         igt_output_t *output;
> -       struct igt_fb fb;
> +       char file_path_n[PATH_MAX] = "";
> +       bool dual_edp = false;
> +       struct context contexts[NUM_EDP_OUTPUTS];
>  
>         igt_fixture {
> -               enum pipe pipe;
>                 bool found = false;
>                 char full_name[32] = {};
>                 char *name;
> -               drmModeModeInfo *mode;
> -               igt_plane_t *primary;
>  
>                 /*
>                  * Backlight tests requires the output to be enabled,
> @@ -202,56 +244,104 @@ igt_main
>                 kmstest_set_vt_graphics_mode();
>                 igt_display_require(&display,
> drm_open_driver(DRIVER_INTEL));
>  
> -               /* Get the max value and skip the whole test if sysfs
> interface not available */
> -               igt_skip_on(backlight_read(&old, "brightness"));
> -               igt_assert(backlight_read(&context.max,
> "max_brightness") > -1);
> +               for_each_connected_output(&display, output) {
> +                       if (output->config.connector->connector_type
> != DRM_MODE_CONNECTOR_eDP)
> +                               continue;
>  
> -               /* should be ../../cardX-$output */
> -               igt_assert_lt(12, readlink(BACKLIGHT_PATH "/device",
> full_name, sizeof(full_name) - 1));
> -               name = basename(full_name);
> +                       if (found)
> +                               snprintf(file_path_n, PATH_MAX,
> "%s/card%i-%s-backlight/brightness",
> +                                        BACKLIGHT_PATH,
> igt_device_get_card_index(display.drm_fd),
> +                                        igt_output_name(output));
> +                       else
> +                               snprintf(file_path_n, PATH_MAX,
> "%s/intel_backlight/brightness",
> +                                        BACKLIGHT_PATH);
>  
> -               for_each_pipe_with_valid_output(&display, pipe,
> output) {
> -                       if (strcmp(name + 6, output->name))
> +                       fd = open(file_path_n, O_RDONLY);
> +                       if (fd == -1)
>                                 continue;
> -                       found = true;
> -                       break;
> -               }
>  
> -               igt_require_f(found,
> -                             "Could not map backlight for \"%s\" to
> connected output\n",
> -                             name);
> -
> -               igt_output_set_pipe(output, pipe);
> -               mode = igt_output_get_mode(output);
> +                       if (found)
> +                               snprintf(contexts[i].path, PATH_MAX,
> "card%i-%s-backlight",
> +                                       
> igt_device_get_card_index(display.drm_fd),
> +                                        igt_output_name(output));
> +                       else
> +                               snprintf(contexts[i].path, PATH_MAX,
> "intel_backlight");
> +
> +                       close(fd);
> +
> +                       /* should be ../../cardX-$output */
> +                       snprintf(file_path_n, PATH_MAX,
> "%s/%s/device", BACKLIGHT_PATH,
> +                                contexts[i].path);
> +                       igt_assert_lt(16, readlink(file_path_n,
> full_name, sizeof(full_name) - 1));
> +                       name = basename(full_name);
> +
> +                       if (!strcmp(name + 6, output->name)) {
> +                               contexts[i++].output = output;
> +
> +                               if (found)
> +                                       dual_edp = true;
> +                               else
> +                                       found = true;
> +                       }
> +               }
> +               igt_require_f(found, "No valid output found.\n");
> +       }
>  
> -               igt_create_pattern_fb(display.drm_fd,
> -                                     mode->hdisplay, mode->vdisplay,
> -                                     DRM_FORMAT_XRGB8888,
> -                                     DRM_FORMAT_MOD_LINEAR, &fb);
> -               primary = igt_output_get_plane_type(output,
> DRM_PLANE_TYPE_PRIMARY);
> -               igt_plane_set_fb(primary, &fb);
> +       igt_subtest("basic-brightness") {
> +               for (i = 0; i < (dual_edp ? 2 : 1); i++) {
> +                       test_setup(display, &contexts->output[i]);
> +                       igt_assert(backlight_read(&contexts[i].max,
> +                                               "max_brightness",
> &contexts[i]) > -1);

Instead of reading this many times for each test and for each backlight
you could do it once in the loop where contexts array is filled.

> +                       test_brightness(&contexts[i]);
> +                       test_cleanup(&display, output);
> +               }
> +       }
>  
> -               igt_display_commit2(&display, display.is_atomic ?
> COMMIT_ATOMIC : COMMIT_LEGACY);
> -               igt_pm_enable_sata_link_power_management();
> +       igt_subtest("bad-brightness") {
> +               for (i = 0; i < (dual_edp ? 2 : 1); i++) {
> +                       test_setup(display, &contexts->output[i]);
> +                       igt_assert(backlight_read(&contexts[i].max,
> +                                               "max_brightness",
> &contexts[i]) > -1);
> +                       test_bad_brightness(&contexts[i]);
> +                       test_cleanup(&display, output);
> +               }
>         }
> +       igt_subtest("fade") {
> +               for (i = 0; i < (dual_edp ? 2 : 1); i++) {
> +                       test_setup(display, &contexts->output[i]);
> +                       igt_assert(backlight_read(&contexts[i].max,
> +                                               "max_brightness",
> &contexts[i]) > -1);
> +                       test_fade(&contexts[i]);
> +                       test_cleanup(&display, output);
> +               }
> +       }
> +       igt_subtest("fade_with_dpms") {
> +               for (i = 0; i < (dual_edp ? 2 : 1); i++) {
> +                       test_setup(display, &contexts->output[i]);
> +                       igt_assert(backlight_read(&contexts[i].max,
> +                                               "max_brightness",
> &contexts[i]) > -1);
> +                       test_fade_with_dpms(&contexts[i], output);
> +                       test_cleanup(&display, output);
> +               }
> +       }
> +       igt_subtest("fade_with_suspend") {
> +               for (i = 0; i < (dual_edp ? 2 : 1); i++) {
> +                       test_setup(display, &contexts->output[i]);
> +                       igt_assert(backlight_read(&contexts[i].max,
> +                                               "max_brightness",
> &contexts[i]) > -1);
> +                       test_fade_with_suspend(&contexts[i], output);
> +                       test_cleanup(&display, output);
> +               }
> +       }
> +
>  
> -       igt_subtest("basic-brightness")
> -               test_brightness(&context);
> -       igt_subtest("bad-brightness")
> -               test_bad_brightness(&context);
> -       igt_subtest("fade")
> -               test_fade(&context);
> -       igt_subtest("fade_with_dpms")
> -               test_fade_with_dpms(&context, output);
> -       igt_subtest("fade_with_suspend")
> -               test_fade_with_suspend(&context, output);
>  
>         igt_fixture {
>                 /* Restore old brightness */
> -               backlight_write(old, "brightness");
> +               for (i = 0; i < (dual_edp ? 2 : 1); i++)
> +                       backlight_write(old, "brightness",
> &contexts[i]);
>  
>                 igt_display_fini(&display);
> -               igt_remove_fb(display.drm_fd, &fb);
>                 igt_pm_restore_sata_link_power_management();
>                 close(display.drm_fd);
>         }

Re: [igt-dev] [PATCH 3/3] tests/i915/i915_pm_backlight: Create dynamic subtests

[Hogander, Jouni]

Hello,

One comment inline below.

On Tue, 2022-11-15 at 18:30 +0530, Nidhi Gupta wrote:
> Modified the test to include dynamic subtests.
> 
> Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
> ---
>  tests/i915/i915_pm_backlight.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/i915/i915_pm_backlight.c
> b/tests/i915/i915_pm_backlight.c
> index e94214a..cb8efcb 100644
> --- a/tests/i915/i915_pm_backlight.c
> +++ b/tests/i915/i915_pm_backlight.c
> @@ -305,7 +305,7 @@ igt_main
>  
>         for (i = 0; i < ARRAY_SIZE(tests); i++) {
>                 igt_describe(tests[i].desc);
> -               igt_subtest(tests[i].name) {
> +               igt_subtest_with_dynamic(tests[i].name) {
>                         for (int j = 0; j < (dual_edp ? 2 : 1); j++)
> {
>                                 test_setup(display, &contexts-
> >output[j]);
>  
> @@ -318,11 +318,13 @@ igt_main
>                                 if (tests[i].flags == TEST_SUSPEND)
>                                         check_suspend(contexts[j].out
> put);
>  
> -
>                                igt_assert(backlight_read(&contexts[j].
> max, "max_brightness", &contexts[j]) > -1);
> -                               tests[i].test_t(&contexts[j]);
> +                               igt_dynamic_f("%s",
> igt_output_name(contexts[j].output)) {
> +                                       igt_assert(backlight_read(&co
> ntexts[j].max, "max_brightness", &contexts[j]) > -1);

You are already reading max brightness.

> +                                       tests[i].test_t(&contexts[j])
> ;
> +                               }
> +                               test_cleanup(&display, output);
>                         }
> -
> -                       test_cleanup(&display, output);
> +                       /* TODO: Add tests for dual eDP. */
>                 }
>         }
>  

[igt-dev] [PATCH 3/3] tests/i915/i915_pm_backlight: Create dynamic subtests

[Nidhi Gupta]

Modified the test to include dynamic subtests.

Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
---
 tests/i915/i915_pm_backlight.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tests/i915/i915_pm_backlight.c b/tests/i915/i915_pm_backlight.c
index e94214a..cb8efcb 100644
--- a/tests/i915/i915_pm_backlight.c
+++ b/tests/i915/i915_pm_backlight.c
@@ -305,7 +305,7 @@ igt_main
 
 	for (i = 0; i < ARRAY_SIZE(tests); i++) {
 		igt_describe(tests[i].desc);
-		igt_subtest(tests[i].name) {
+		igt_subtest_with_dynamic(tests[i].name) {
 			for (int j = 0; j < (dual_edp ? 2 : 1); j++) {
 				test_setup(display, &contexts->output[j]);
 
@@ -318,11 +318,13 @@ igt_main
 				if (tests[i].flags == TEST_SUSPEND)
 					check_suspend(contexts[j].output);
 
-				igt_assert(backlight_read(&contexts[j].max, "max_brightness", &contexts[j]) > -1);
-				tests[i].test_t(&contexts[j]);
+				igt_dynamic_f("%s", igt_output_name(contexts[j].output)) {
+					igt_assert(backlight_read(&contexts[j].max, "max_brightness", &contexts[j]) > -1);
+					tests[i].test_t(&contexts[j]);
+				}
+				test_cleanup(&display, output);
 			}
-
-			test_cleanup(&display, output);
+			/* TODO: Add tests for dual eDP. */
 		}
 	}
 
-- 
1.9.1