[igt-dev] [PATCH i-g-t] amdgpu/amd_abm: Fix getting and setting abm level property

* [igt-dev] [PATCH i-g-t] amdgpu/amd_abm: Fix getting and setting abm level property
@ 2019-04-16 17:45 Nicholas Kazlauskas
  2019-04-16 18:34 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Nicholas Kazlauskas @ 2019-04-16 17:45 UTC (permalink / raw)
  To: igt-dev

This patch addresses a few problems:

1. Inner loop that iterates over the properties uses the same iterator
as the outer loop over connectors. If the eDP panel isn't on the first
output, then it won't be checked and the tests will skip.

2. We can get null pointer deferences if any of the DRM calls return
NULL.

3. The proplist isn't freed after being acquired.

These can be fixed by using the kmstest_get_property helper to
get the prop_id. The prop_id and KMS connector ID are then stored
for use later in the test when we need to set the ABM level.

All the necessary cached state has been placed into a data structure and
many callsites have been updated to make use of this.

Cc: David Francis <david.francis@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 tests/amdgpu/amd_abm.c | 147 ++++++++++++++++++-----------------------
 1 file changed, 63 insertions(+), 84 deletions(-)

diff --git a/tests/amdgpu/amd_abm.c b/tests/amdgpu/amd_abm.c
index 363b1e90..74156462 100644
--- a/tests/amdgpu/amd_abm.c
+++ b/tests/amdgpu/amd_abm.c
@@ -34,6 +34,14 @@
 
 #define BACKLIGHT_PATH "/sys/class/backlight/amdgpu_bl0"
 
+typedef struct data {
+	igt_display_t display;
+	int drm_fd;
+	int debugfs;
+	uint32_t output_id;
+	uint32_t abm_prop_id;
+} data_t;
+
 static int read_current_backlight_pwm(int debugfs_dir)
 {
 	char buf[20];
@@ -76,35 +84,14 @@ static int backlight_write_brightness(int value)
 	return 0;
 }
 
-static void set_abm_level(igt_display_t *display, int level)
+static void set_abm_level(data_t *data, int level)
 {
-	int i, ret;
-	int output_id;
-	drmModeObjectPropertiesPtr props;
-	uint32_t prop_id;
-	drmModePropertyPtr prop;
 	uint32_t type = DRM_MODE_OBJECT_CONNECTOR;
+	int ret;
 
-	for (i = 0; i < display->n_outputs; i++) {
-		output_id = display->outputs[i].id;
-		props = drmModeObjectGetProperties(display->drm_fd, output_id, type);
-
-		for (i = 0; i < props->count_props; i++) {
-			prop_id = props->props[i];
-			prop = drmModeGetProperty(display->drm_fd, prop_id);
-
-			igt_assert(prop);
-
-			if (strcmp(prop->name, "abm level") == 0) {
-				ret = drmModeObjectSetProperty(display->drm_fd, output_id, type, prop_id, level);
-
-				igt_assert_eq(ret, 0);
-			}
-
-			drmModeFreeProperty(prop);
-		}
-	}
-
+	ret = drmModeObjectSetProperty(data->drm_fd, data->output_id, type,
+				       data->abm_prop_id, level);
+	igt_assert_eq(ret, 0);
 }
 
 static int backlight_read_max_brightness(int *result)
@@ -132,24 +119,21 @@ static int backlight_read_max_brightness(int *result)
 	return errno;
 }
 
-static void skip_if_incompatible(igt_display_t *display, int debugfs_dir)
+static void test_init(data_t *data)
 {
+	igt_display_t *display = &data->display;
 	int ret, i;
 	char buf[20];
 	bool abm_prop_exists;
-	int output_id;
-	drmModeObjectPropertiesPtr props;
 	uint32_t type = DRM_MODE_OBJECT_CONNECTOR;
-	uint32_t prop_id;
-	drmModePropertyPtr prop;
 
-	ret = igt_debugfs_simple_read(debugfs_dir, "amdgpu_current_backlight_pwm",
+	ret = igt_debugfs_simple_read(data->debugfs, "amdgpu_current_backlight_pwm",
 			 buf, sizeof(buf));
 
 	if (ret < 0)
 		igt_skip("No current backlight debugfs entry.\n");
 
-	ret = igt_debugfs_simple_read(debugfs_dir, "amdgpu_target_backlight_pwm",
+	ret = igt_debugfs_simple_read(data->debugfs, "amdgpu_target_backlight_pwm",
 			 buf, sizeof(buf));
 
 	if (ret < 0)
@@ -158,18 +142,14 @@ static void skip_if_incompatible(igt_display_t *display, int debugfs_dir)
 	abm_prop_exists = false;
 
 	for (i = 0; i < display->n_outputs; i++) {
-		output_id = display->outputs[i].id;
-		props = drmModeObjectGetProperties(display->drm_fd, output_id, type);
+		data->output_id = display->outputs[i].id;
 
-		for (i = 0; i < props->count_props; i++) {
-			prop_id = props->props[i];
-			prop = drmModeGetProperty(display->drm_fd, prop_id);
+		abm_prop_exists = kmstest_get_property(
+			data->drm_fd, data->output_id, type, "abm level",
+			&data->abm_prop_id, NULL, NULL);
 
-			if (strcmp(prop->name, "abm level") == 0)
-				abm_prop_exists = true;
-
-			drmModeFreeProperty(prop);
-		}
+		if (abm_prop_exists)
+			break;
 	}
 
 	if (!abm_prop_exists)
@@ -177,7 +157,7 @@ static void skip_if_incompatible(igt_display_t *display, int debugfs_dir)
 }
 
 
-static void backlight_dpms_cycle(igt_display_t *display, int debugfs, igt_output_t *output)
+static void backlight_dpms_cycle(data_t *data, igt_output_t *output)
 {
 	int ret;
 	int max_brightness;
@@ -186,19 +166,19 @@ static void backlight_dpms_cycle(igt_display_t *display, int debugfs, igt_output
 	ret = backlight_read_max_brightness(&max_brightness);
 	igt_assert_eq(ret, 0);
 
-	set_abm_level(display, 0);
+	set_abm_level(data, 0);
 	backlight_write_brightness(max_brightness / 2);
 	usleep(100000);
-	pwm_1 = read_target_backlight_pwm(debugfs);
+	pwm_1 = read_target_backlight_pwm(data->debugfs);
 
-	kmstest_set_connector_dpms(display->drm_fd, output->config.connector, DRM_MODE_DPMS_OFF);
-	kmstest_set_connector_dpms(display->drm_fd, output->config.connector, DRM_MODE_DPMS_ON);
+	kmstest_set_connector_dpms(data->drm_fd, output->config.connector, DRM_MODE_DPMS_OFF);
+	kmstest_set_connector_dpms(data->drm_fd, output->config.connector, DRM_MODE_DPMS_ON);
 	usleep(100000);
-	pwm_2 = read_target_backlight_pwm(debugfs);
+	pwm_2 = read_target_backlight_pwm(data->debugfs);
 	igt_assert_eq(pwm_1, pwm_2);
 }
 
-static void backlight_monotonic_basic(igt_display_t *display, int debugfs)
+static void backlight_monotonic_basic(data_t *data)
 {
 	int ret;
 	int max_brightness;
@@ -211,23 +191,23 @@ static void backlight_monotonic_basic(igt_display_t *display, int debugfs)
 
 	brightness_step = max_brightness / 10;
 
-	set_abm_level(display, 0);
+	set_abm_level(data, 0);
 	backlight_write_brightness(max_brightness);
 	usleep(100000);
-	prev_pwm = read_target_backlight_pwm(debugfs);
+	prev_pwm = read_target_backlight_pwm(data->debugfs);
 	for (brightness = max_brightness - brightness_step;
 	     brightness > 0;
 	     brightness -= brightness_step) {
 		backlight_write_brightness(brightness);
 		usleep(100000);
-		pwm = read_target_backlight_pwm(debugfs);
+		pwm = read_target_backlight_pwm(data->debugfs);
 		igt_assert(pwm < prev_pwm);
 		prev_pwm = pwm;
 	}
 
 }
 
-static void backlight_monotonic_abm(igt_display_t *display, int debugfs)
+static void backlight_monotonic_abm(data_t *data)
 {
 	int ret, i;
 	int max_brightness;
@@ -240,23 +220,23 @@ static void backlight_monotonic_abm(igt_display_t *display, int debugfs)
 
 	brightness_step = max_brightness / 10;
 	for (i = 1; i < 5; i++) {
-		set_abm_level(display, i);
+		set_abm_level(data, i);
 		backlight_write_brightness(max_brightness);
 		usleep(100000);
-		prev_pwm = read_target_backlight_pwm(debugfs);
+		prev_pwm = read_target_backlight_pwm(data->debugfs);
 		for (brightness = max_brightness - brightness_step;
 		     brightness > 0;
 		     brightness -= brightness_step) {
 			backlight_write_brightness(brightness);
 			usleep(100000);
-			pwm = read_target_backlight_pwm(debugfs);
+			pwm = read_target_backlight_pwm(data->debugfs);
 			igt_assert(pwm < prev_pwm);
 			prev_pwm = pwm;
 		}
 	}
 }
 
-static void abm_enabled(igt_display_t *display, int debugfs)
+static void abm_enabled(data_t *data)
 {
 	int ret, i;
 	int max_brightness;
@@ -265,16 +245,16 @@ static void abm_enabled(igt_display_t *display, int debugfs)
 	ret = backlight_read_max_brightness(&max_brightness);
 	igt_assert_eq(ret, 0);
 
-	set_abm_level(display, 0);
+	set_abm_level(data, 0);
 	backlight_write_brightness(max_brightness);
 	usleep(100000);
-	prev_pwm = read_target_backlight_pwm(debugfs);
+	prev_pwm = read_target_backlight_pwm(data->debugfs);
 	pwm_without_abm = prev_pwm;
 
 	for (i = 1; i < 5; i++) {
-		set_abm_level(display, i);
+		set_abm_level(data, i);
 		usleep(100000);
-		pwm = read_target_backlight_pwm(debugfs);
+		pwm = read_target_backlight_pwm(data->debugfs);
 		igt_assert(pwm <= prev_pwm);
 		igt_assert(pwm < pwm_without_abm);
 		prev_pwm = pwm;
@@ -282,7 +262,7 @@ static void abm_enabled(igt_display_t *display, int debugfs)
 
 }
 
-static void abm_gradual(igt_display_t *display, int debugfs)
+static void abm_gradual(data_t *data)
 {
 	int ret, i;
 	int convergence_delay = 15;
@@ -293,71 +273,70 @@ static void abm_gradual(igt_display_t *display, int debugfs)
 
 	igt_assert_eq(ret, 0);
 
-	set_abm_level(display, 0);
+	set_abm_level(data, 0);
 	backlight_write_brightness(max_brightness);
 
 	sleep(convergence_delay);
-	prev_pwm = read_target_backlight_pwm(debugfs);
-	curr = read_current_backlight_pwm(debugfs);
+	prev_pwm = read_target_backlight_pwm(data->debugfs);
+	curr = read_current_backlight_pwm(data->debugfs);
 
 	igt_assert_eq(prev_pwm, curr);
-	set_abm_level(display, 4);
+	set_abm_level(data, 4);
 	for (i = 0; i < 10; i++) {
 		usleep(100000);
-		pwm = read_current_backlight_pwm(debugfs);
+		pwm = read_current_backlight_pwm(data->debugfs);
 		igt_assert(pwm < prev_pwm);
 		prev_pwm = pwm;
 	}
 
 	sleep(convergence_delay - 1);
 
-	prev_pwm = read_target_backlight_pwm(debugfs);
-	curr = read_current_backlight_pwm(debugfs);
+	prev_pwm = read_target_backlight_pwm(data->debugfs);
+	curr = read_current_backlight_pwm(data->debugfs);
 
 	igt_assert_eq(prev_pwm, curr);
 }
 
 igt_main
 {
-	igt_display_t display;
-	int debugfs;
+	data_t data = { 0 };
 	enum pipe pipe;
 	igt_output_t *output;
 
 	igt_skip_on_simulation();
 
 	igt_fixture {
-		display.drm_fd = drm_open_driver_master(DRIVER_AMDGPU);
+		data.drm_fd = drm_open_driver_master(DRIVER_AMDGPU);
 
-		if (display.drm_fd == -1)
+		if (data.drm_fd == -1)
 			igt_skip("Not an amdgpu driver.\n");
 
-		debugfs = igt_debugfs_dir(display.drm_fd);
+		data.debugfs = igt_debugfs_dir(data.drm_fd);
 
 		kmstest_set_vt_graphics_mode();
 
-		igt_display_require(&display, display.drm_fd);
+		igt_display_require(&data.display, data.drm_fd);
 
-		skip_if_incompatible(&display, debugfs);
+		test_init(&data);
 
-		for_each_pipe_with_valid_output(&display, pipe, output) {
+		for_each_pipe_with_valid_output(&data.display, pipe, output) {
 			if (output->config.connector->connector_type == DRM_MODE_CONNECTOR_eDP)
 				break;
 		}
 	}
 
 	igt_subtest("dpms_cycle")
-		backlight_dpms_cycle(&display, debugfs, output);
+		backlight_dpms_cycle(&data, output);
 	igt_subtest("backlight_monotonic_basic")
-		backlight_monotonic_basic(&display, debugfs);
+		backlight_monotonic_basic(&data);
 	igt_subtest("backlight_monotonic_abm")
-		backlight_monotonic_abm(&display, debugfs);
+		backlight_monotonic_abm(&data);
 	igt_subtest("abm_enabled")
-		abm_enabled(&display, debugfs);
+		abm_enabled(&data);
 	igt_subtest("abm_gradual")
-		abm_gradual(&display, debugfs);
+		abm_gradual(&data);
 
 	igt_fixture {
-		igt_display_fini(&display);
+		igt_display_fini(&data.display);
 	}
 }
-- 
2.17.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply related	[flat|nested] 5+ messages in thread