[igt-dev] [PATCH i-g-t] tests/i915/i915_pm_backlight: Add new subtest to validate dual panel backlight
Modem, Bhanuprakash
bhanuprakash.modem at intel.com
Fri Nov 4 02:34:45 UTC 2022
On Wed-02-11-2022 10:38 pm, 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 at intel.com>
> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> ---
> tests/i915/i915_pm_backlight.c | 208 ++++++++++++++++++++++++++++-------------
> 1 file changed, 144 insertions(+), 64 deletions(-)
>
> diff --git a/tests/i915/i915_pm_backlight.c b/tests/i915/i915_pm_backlight.c
> index cafae7f..54e51b7 100644
> --- a/tests/i915/i915_pm_backlight.c
> +++ b/tests/i915/i915_pm_backlight.c
> @@ -34,29 +34,39 @@
> #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];
> };
>
> +enum {
> + TEST_NONE = 0,
> + TEST_DPMS,
> + TEST_SUSPEND,
> +};
>
> #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];
> + char dst[512];
> 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);
As we are using dynamic subtests, don't assert here. Instead return some
error.
> fd = open(full, O_RDONLY);
> if (fd == -1)
> return -errno;
> @@ -73,14 +83,14 @@ 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);
Please check above comment.
> fd = open(full, O_WRONLY);
> if (fd == -1)
> return -errno;
> @@ -100,12 +110,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 +134,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);
> }
>
> @@ -154,7 +164,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));
Please check above comments.
>
> @@ -167,33 +177,77 @@ 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));
Please check above comments.
> -
> - 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)
> +{
> + 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;
> + }
There is a possibility that all pipes are invalid to use the selected
output.
> }
>
> 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];
> + struct {
> + const char *name;
> + const char *desc;
> + void (*test_t) (struct context *);
> + int flags;
> + } tests[] = {
> + { "basic-brightness", "desc", test_brightness, TEST_NONE },
> + { "bad-brightness", "desc", test_bad_brightness, TEST_NONE },
> + { "fade", "desc", test_fade, TEST_NONE },
> + { "fade-with-dpms", "desc", test_fade, TEST_DPMS },
> + { "fade-with-suspend", "desc", test_fade, TEST_SUSPEND },
> + };
>
> 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 +256,82 @@ 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);
It is perfectly fine, if display->outputs[] exposes list of connectors
as eDP-1 followed by eDP-2. In case the order got changed, this code
does't work.
>
> - 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;
> + }
> + 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;
> + }
Once we find the duel edp, there is no point in checking for other
connectors.
> }
> + igt_require_f(found, "No valid output found.\n");
> + }
>
> - igt_require_f(found,
> - "Could not map backlight for \"%s\" to connected output\n",
> - name);
> + for (i = 0; i < ARRAY_SIZE(tests); i++) {
> + igt_describe(tests[i].desc);
> + igt_subtest_with_dynamic(tests[i].name) {
Probably, we should have a separate patch for dynamic subtests.
> + for (int j = 0; j < (dual_edp ? 2 : 1); j++) {
> + test_setup(display, &contexts->output[j]);
>
> - igt_output_set_pipe(output, pipe);
> - mode = igt_output_get_mode(output);
> + if (backlight_read(&old, "brightness", &contexts[j]))
> + continue;
>
> - 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_assert(backlight_read(&contexts[j].max, "max_brightness", &contexts[j]) > -1);
Please drop this. This is redundant & also igt_assert is not allowed
inside the igt_subtest_with_dynamic() before execting the igt_dynamic()
block.
>
> - igt_display_commit2(&display, display.is_atomic ? COMMIT_ATOMIC : COMMIT_LEGACY);
> - igt_pm_enable_sata_link_power_management();
> - }
> + if (tests[i].flags == TEST_DPMS)
> + check_dpms(contexts[j].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);
> + if (tests[i].flags == TEST_SUSPEND)
> + check_suspend(contexts[j].output);
> +
> + 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);
> + }
> + /* TODO: Add tests for dual eDP. */
> + }
> + }
>
> igt_fixture {
> /* Restore old brightness */
> - backlight_write(old, "brightness");
> + for (int j = 0; j < (dual_edp ? 2 : 1); j++) {
> + backlight_write(old, "brightness", &contexts[j]);
Same "old" content for both the eDPs?
- Bhanu
> + }
>
> igt_display_fini(&display);
> - igt_remove_fb(display.drm_fd, &fb);
> igt_pm_restore_sata_link_power_management();
> close(display.drm_fd);
> }
More information about the igt-dev
mailing list