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

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Thu, 2022-09-29 at 14:58 +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>
> ---
>  tests/i915/i915_pm_backlight.c | 110 ++++++++++++++++++++++++-------
> --
>  1 file changed, 80 insertions(+), 30 deletions(-)
> 
> diff --git a/tests/i915/i915_pm_backlight.c
> b/tests/i915/i915_pm_backlight.c
> index cafae7f7..bf67b50c 100644
> --- a/tests/i915/i915_pm_backlight.c
> +++ b/tests/i915/i915_pm_backlight.c
> @@ -34,9 +34,12 @@
>  #include <errno.h>
>  #include <unistd.h>
>  #include <time.h>
> +#include "igt_device.h"
> +#include "igt_device_scan.h"
>  
>  struct context {
>         int max;
> +       const char *path;
>  };
>  
>  
> @@ -48,14 +51,14 @@ struct context {
>  
>  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)
> @@ -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;
>  
> -       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");
> +       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);
>  }
>  
> @@ -186,6 +189,8 @@ igt_main
>         igt_display_t display;
>         igt_output_t *output;
>         struct igt_fb fb;
> +       const char *paths[2][50];
> +       char path1[PATH_MAX];
>  
>         igt_fixture {
>                 enum pipe pipe;
> @@ -202,10 +207,6 @@ 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);
> @@ -233,22 +234,71 @@ igt_main
>  
>                 igt_display_commit2(&display, display.is_atomic ?
> COMMIT_ATOMIC : COMMIT_LEGACY);
>                 igt_pm_enable_sata_link_power_management();
> +
> +               memcpy(paths[0], "intel_backlight", 8);
> +               for (int i = 0; (i = display.n_outputs); i++) {
> +                       output = &display.outputs[i];
> +                       igt_output_set_pipe(output, PIPE_ANY);

What igt_output_set_pipe(output, PIPE_ANY) is doing?

> +                       snprintf(path1, 100 , "card%i-%s-backlight",
> igt_device_get_card_index(display.drm_fd), igt_output_name(output));

I'm not sure if card* can be something else than card0? Definetely it
has nothing to do with n_outputs.


> +                       int fd = open(path1, O_WRONLY);

You can't expect cardx-xxx-backlight to succeed. There has to be some
path (e.g. /sys/dev/char/226\:0/cardx-xxx-backlight). Maybe
igt_sysfs_path could be used here?

> +                       if (fd == -1)
> +                               continue;

close(fd);

> +                       memcpy(paths[1], name, 30);

You have paths[0] = intel_backlight and paths[1] = card0-eDP1? You
wanted to copy path1?

> +                       break;
> +               }

This loop generally looks strange. You are checking path1 copying
variable "name" into paths[1] ?

> 
> +               /* Get the max value and skip the whole test if sysfs
> interface not available */
> +               for (size_t i = 0; i < sizeof(paths) /
> sizeof(paths[0]); i++) {
> +                       memcpy(&context.path, paths[i], 8);

is 8 enough? I think you should use pointer here instead of copying it


> +                       igt_skip_on(backlight_read(&old,
> "brightness", &context));
> +                       igt_assert(backlight_read(&context.max,
> "max_brightness", &context) > -1);
> +               }
> +       }
> +       
> +       igt_subtest_with_dynamic("basic-brightness") {
> +               for (size_t j = 0; j < sizeof(paths) /
> sizeof(paths[0]); j++) {
> +                       igt_dynamic_f("connector-%zu", j) {
> +                               memcpy(&context.path, paths[j], 8);
> +                               test_brightness(&context);
> +                       }
> +               }
> +       }
> +       igt_subtest_with_dynamic("bad-brightness") {
> +               for (size_t j = 0; j < sizeof(paths) /
> sizeof(paths[0]); j++) {
> +                       igt_dynamic_f("connector-%zu", j) {
> +                               memcpy(&context.path, paths[j], 8);
> +                               test_bad_brightness(&context);
> +                       }
> +               }
> +       }
> +
> +       igt_subtest_with_dynamic("fade") {
> +               for (size_t j = 0; j < sizeof(paths) /
> sizeof(paths[0]); j++) {
> +                       igt_dynamic_f("connector-%zu", j) {
> +                               memcpy(&context.path, paths[j], 8);
> +                               test_fade(&context);
> +                       }
> +               }
>         }
>  
> -       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_dpms") {
> +               for (size_t j = 0; j < sizeof(paths) /
> sizeof(paths[0]); j++) {
> +                       igt_dynamic_f("connector-%zu", j) {
> +                               memcpy(&context.path, paths[j], 8);
> +                               test_fade_with_dpms(&context,
> output);
> +                       }
> +               }
> +       }
> +
> +       igt_subtest_with_dynamic("fade_with_suspend") {
> +               for (size_t j = 0; j < sizeof(paths) /
> sizeof(paths[0]); j++) {
> +                       igt_dynamic_f("connector-%zu", j) {
> +                               memcpy(&context.path, paths[j], 8);
> +                               test_fade_with_suspend(&context,
> output);
> +                       }
> +               }
> +       }
>  
>         igt_fixture {
> -               /* Restore old brightness */
> -               backlight_write(old, "brightness");
>  
>                 igt_display_fini(&display);
>                 igt_remove_fb(display.drm_fd, &fb);