<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr"> <div id="divtagdefaultwrapper" dir="ltr" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;"> Patch is Reviewed-by: David Francis <david.francis@amd.com> </div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="divRplyFwdMsg" dir="ltr">From: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> Sent: April 16, 2019 1:45:12 PM To: igt-dev@lists.freedesktop.org Cc: Kazlauskas, Nicholas; Francis, David; Li, Sun peng (Leo) Subject: [PATCH i-g-t] amdgpu/amd_abm: Fix getting and setting abm level property <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">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 </div> </div> </div> </body> </html>