[PATCH i-g-t] tests/i915/i915_pm_backlight: Add new subtest to validate dual panel backlight
Hogander, Jouni
jouni.hogander at intel.com
Wed Oct 19 07:32:23 UTC 2022
Hello Nidhi,
see my inline comments below.
Also consider changing your strategy a bit:
struct context {
int max;
char path[PATH_MAX];
};
#define MAX_EDP_OUTPUTS 2;
struct context contexts [MAX_EDP_OUTPUTS];
In fixture fill that array (maybe in test_setup) and iterate it over
for each test case.
On Thu, 2022-10-13 at 17:59 +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 at intel.com>
> ---
> tests/i915/i915_pm_backlight.c | 193 ++++++++++++++++++++++++++++++-
> ----------
> 1 file changed, 140 insertions(+), 53 deletions(-)
>
> diff --git a/tests/i915/i915_pm_backlight.c
> b/tests/i915/i915_pm_backlight.c
> index cafae7f..127c540 100644
> --- a/tests/i915/i915_pm_backlight.c
> +++ b/tests/i915/i915_pm_backlight.c
> @@ -34,28 +34,31 @@
> #include <errno.h>
> #include <unistd.h>
> #include <time.h>
> +#include "igt_device.h"
> +#include "igt_device_scan.h"
>
> struct context {
> int max;
> + 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 */
>
> 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);for_each_pipe_with_valid_output
> + igt_assert(snprintf(full, PATH_MAX, "%s/%s/%s",
> BACKLIGHT_PATH, context->path, fname) < PATH_MAX);
>
> fd = open(full, O_RDONLY);
> if (fd == -1)
> @@ -73,14 +76,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);
> fd = open(full, O_WRONLY);
> if (fd == -1)
> return -errno;
> @@ -100,12 +103,12 @@ static void test_and_verify(struct context
> *context, int val)
> const int tolerance = val * TOLERANCE / 100;
> int result;for_each_pipe_with_valid_output
>
> - 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 +127,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");for_each_pipe_with_valid_output
> + 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,for_each_pipe_with_vali
> d_output "for_each_pipe_with_valid_outputbrightness", context), 0);
> + backlight_read(&val, "brightness", context);
> igt_assert_eq(val, context->max / 2);
> }
>
> @@ -179,21 +182,56 @@ test_fade_with_suspend(struct context *context,
> igt_output_t *output)
> test_fade(context);
> }
>
> +static void test_setup(igt_output_t *output, igt_display_t display)
Output should be local variable.
> +{
> + igt_plane_t *primary;
> + drmModeModeInfo *mode;
> + struct igt_fb fb;
> + igt_output_t *first_output = NULL;
> + enum pipe pipe, first = PIPE_NONE;
> +
> + igt_display_reset(&display);
> +
> + for_each_pipe_with_valid_output(&display, pipe, output) {
> + if (first == pipe || output == first_output) {
> + continue;
> + } else {
No need for else here. This is setting up also other outputs e.g. DP.
> + first = pipe;
> + first_output = output;
> + igt_output_set_pipe(output, pipe);
> + }
Here you could parse the backlight control interface and do "continue"
if such doesn't exist. Then also store backlight interface somewhere.
That way you could remove the second loop from the fixture.
> +
> + 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();
> + }
> +}
> +
> igt_main
> {
> struct context context = {0};
> - int old;
> + int old, fd;
> + int count = 0;
> igt_display_t display;
> igt_output_t *output;
> struct igt_fb fb;
> + char buffer_n[PATH_MAX];
> + char const *path_n[2];
Maybe add define like MAX_EDP_OUTPUTS?
> + char file_path_n[PATH_MAX] =
> "";for_each_pipe_with_valid_output
>
> 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,53 +240,102 @@ 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);
> -
> - /* should be ../../cardX-$output */
> - igt_assert_lt(12, readlink(BACKLIGHT_PATH "/device",
> full_name, sizeof(full_name) - 1));
> - name = basename(full_name);
> -
> - for_each_pipe_with_valid_output(&display, pipe,
> output) {
> + for (int i = 0; i < 2; i++) {
> + if (i == 0)
> + path_n[i] = "intel_backlight";
> + else {
> + for_each_pipe_with_valid_output(&disp
> lay, pipe, output) {
> + snprintf(buffer_n, PATH_MAX,
> "card%i-%s-backlight", igt_device_get_card_index(display.drm_fd),
> igt_output_name(output));
> + snprintf(file_path_n,
> PATH_MAX, "%s/%s/brightness", BACKLIGHT_PATH, buffer_n);
> + fd = open(file_path_n,
> O_RDONLY);
> + if (fd == -1)
> + continue;
> + close(fd);
> + path_n[i] = buffer_n;
You will end up having same pointer to buffer_n in all items in path_n.
Maybe you could do:
char path_n[2][PATH_MAX];
> + break;
Also, I think you should do this in same loop where you are setting
update ouput. I.e. in test_setup. There you could also take care that
outputs that do not have backlight control are not set up.
> + }
> + }
> +
> + /* should be ../../cardX-$output */
> + snprintf(file_path_n, PATH_MAX,
> "%s/%s/device", BACKLIGHT_PATH, path_n[i]);
> + igt_assert_lt(16, readlink(file_path_n,
> full_name, sizeof(full_name) - 1));
> + name = basename(full_name);
> +
> + for_each_pipe_with_valid_output(&display,
> pipe, output) {
> if (strcmp(name + 6, output->name))
> continue;
> found = true;
I'm not sure if this check makes sense at all.
> break;
> - }
> + }
>
> - igt_require_f(found,
> - "Could not map backlight for \"%s\" to
> connected output\n",
> - name);
> + igt_require_f(found,
> + "Could not map backlight for
> \"%s\" to connected output\n"
> + name);
> + count++;
> + }
> + test_setup(output,
> display);for_each_pipe_with_valid_output
> + }
>
> - igt_output_set_pipe(output, pipe);
> - mode = igt_output_get_mode(output);
> + igt_subtest_with_dynamic("basic-brightness") {
> + for (int j = 0; j < count; j++) {
> + igt_dynamic_f("connector-%i", j) {
> + strcpy(context.path, path_n[j]);
> + igt_skip_on(backlight_read(&old,
> "brightness", &context));
> + igt_assert(backlight_read(&context.ma
> x, "max_brightness", &context) > -1);
> + test_brightness(&context);
> + }
> + }
> + }
> + igt_subtest_with_dynamic("bad-brightness") {
> + for (int j = 0; j < count; j++) {
> + igt_dynamic_f("connector-%i", j) {
> + strcpy(context.path, path_n[j]);
> + igt_skip_on(backlight_read(&old,
> "brightness", &context));
> + igt_assert(backlight_read(&context.ma
> x, "max_brightness", &context) > -1);
> + test_bad_brightness(&context);
> + }
> + }
> + }
>
> - 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_with_dynamic("fade") {
> + for (int j = 0; j < count; j++) {
> + igt_dynamic_f("connector-%i", j) {
> + strcpy(context.path, path_n[j]);
> + igt_skip_on(backlight_read(&old,
> "brightness", &context));
> + igt_assert(backlight_read(&context.ma
> x, "max_brightness", &context) > -1);
> + test_fade(&context);
> + }
> + }
> + }
>
> - igt_display_commit2(&display, display.is_atomic ?
> COMMIT_ATOMIC : COMMIT_LEGACY);
> - igt_pm_enable_sata_link_power_management();
> + igt_subtest_with_dynamic("fade_with_dpms") {
> + for (int j = 0; j < count; j++) {
> + igt_dynamic_f("connector-%i", j) {
> + strcpy(context.path, path_n[j]);
> + igt_skip_on(backlight_read(&old,
> "brightness", &context));
> + igt_assert(backlight_read(&context.ma
> x, "max_brightness", &context) > -1);
> + test_fade_with_dpms(&context,
> 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_subtest_with_dynamic("fade_with_suspend") {
> + for (int j = 0; j < count; j++) {
> + igt_dynamic_f("connector-%i", j) {
> + strcpy(context.path, path_n[j]);
> + igt_skip_on(backlight_read(&old,
> "brightness", &context));
> + igt_assert(backlight_read(&context.ma
> x, "max_brightness", &context) > -1);
> + test_fade_with_suspend(&context,
> output);
> + }
> + }
> + }
>
> igt_fixture {
> /* Restore old brightness */
> - backlight_write(old, "brightness");
> + for (int j = 0; j < count; j++) {
> + strcpy(context.path, path_n[j]);
> + backlight_write(old, "brightness", &context);
> + }
>
> igt_display_fini(&display);
> igt_remove_fb(display.drm_fd, &fb);
More information about the Intel-gfx-trybot
mailing list