Re: [PATCH i-g-t] tests/intel/kms_pm_backlight: Brightness test during DPMS on and off

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 18 Sep 2024, "Hogander, Jouni" <jouni.hogander@intel.com> wrote:
> On Thu, 2024-07-04 at 17:01 +0530, Mohammed Thasleem wrote:
>> Set brightness by deviding max brightness, store and read it back.
>> The actual brightness should be same during DPMS on and off cycle.
>> 
>> v2: Update test path and testplan documentation.
>> v3: Check backlight for all internal panels. (Jani)
>> 
>> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com>
>> ---
>>  tests/intel/kms_pm_backlight.c | 51 ++++++++++++++++++++++++++++++--
>> --
>>  1 file changed, 46 insertions(+), 5 deletions(-)
>> 
>> diff --git a/tests/intel/kms_pm_backlight.c
>> b/tests/intel/kms_pm_backlight.c
>> index 8672afa7a..bb85c6e29 100644
>> --- a/tests/intel/kms_pm_backlight.c
>> +++ b/tests/intel/kms_pm_backlight.c
>> @@ -64,6 +64,10 @@
>>   * SUBTEST: fade-with-suspend
>>   * Description: Test the fade with suspend.
>>   * Functionality: backlight, suspend
>> + *
>> + * SUBTEST: brightness-with-dpms
>> + * Description: test brightness with dpms on and off cycle.
>> + * Functionality: backlight, backlight
>>   */
>>  
>>  struct context {
>> @@ -217,6 +221,32 @@ check_suspend(igt_output_t *output)
>>         igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
>> SUSPEND_TEST_NONE);
>>  }
>>  
>> +static void check_dpms_cycle(igt_display_t *display, igt_output_t
>> *output, struct context *context)
>> +{
>> +       int max, val_1, val_2;
>> +
>> +       backlight_read(&max, "max_brightness", context);
>> +       igt_assert(max);
>> +
>> +       backlight_write(max / 2, "brightness", context);
>> +       backlight_read(&val_1, "actual_brightness", context);
>> +
>> +       igt_require(igt_setup_runtime_pm(output->display->drm_fd));
>> +
>> +       kmstest_set_connector_dpms(output->display->drm_fd,
>> +                                  output->config.connector,
>> +                                  DRM_MODE_DPMS_OFF);
>> +       igt_require(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_SUSP
>> ENDED));
>> +
>> +       kmstest_set_connector_dpms(output->display->drm_fd,
>> +                                  output->config.connector,
>> +                                  DRM_MODE_DPMS_ON);
>> +       igt_assert(igt_wait_for_pm_status(IGT_RUNTIME_PM_STATUS_ACTIV
>> E));
>> +
>> +       backlight_read(&val_2, "actual_brightness", context);
>> +       igt_assert_eq(val_1, val_2);
>> +}
>> +
>>  static void test_cleanup(igt_display_t *display, igt_output_t
>> *output)
>>  {
>>         igt_output_set_pipe(output, PIPE_NONE);
>> @@ -262,7 +292,7 @@ igt_main
>>         igt_display_t display;
>>         igt_output_t *output;
>>         char file_path_n[PATH_MAX] = "";
>> -       bool dual_edp = false;
>> +       bool num_panel = false;
>
> I think this is unrelated change and doesn't improve readability:
> num_panel is somehow stating it's number of panels, but it is actually
> a boolean.

It's a misreading of my earlier review I think [1].

BR,
Jani.


[1] https://lore.kernel.org/r/87zfrxknqt.fsf@intel.com



>
> BR,
>
> Jouni Högander
>
>>         struct context contexts[NUM_EDP_OUTPUTS];
>>         struct {
>>                 const char *name;
>> @@ -290,7 +320,7 @@ igt_main
>>                 igt_display_require(&display,
>> drm_open_driver(DRIVER_INTEL | DRIVER_XE));
>>  
>>                 for_each_connected_output(&display, output) {
>> -                       if (output->config.connector->connector_type
>> != DRM_MODE_CONNECTOR_eDP)
>> +                       if (!output_is_internal_panel(output))
>>                                 continue;
>>  
>>                         if (found)
>> @@ -327,7 +357,7 @@ igt_main
>>                                 contexts[i++].output = output;
>>  
>>                                 if (found)
>> -                                       dual_edp = true;
>> +                                       num_panel = true;
>>                                 else
>>                                         found = true;
>>                         }
>> @@ -338,7 +368,7 @@ igt_main
>>         for (i = 0; i < ARRAY_SIZE(tests); i++) {
>>                 igt_describe(tests[i].desc);
>>                 igt_subtest_with_dynamic(tests[i].name) {
>> -                       for (int j = 0; j < (dual_edp ? 2 : 1); j++)
>> {
>> +                       for (int j = 0; j < (num_panel ? 2 : 1); j++)
>> {
>>                                 test_setup(display, &contexts-
>> >output[j]);
>>  
>>                                 if (tests[i].flags == TEST_DPMS)
>> @@ -355,9 +385,20 @@ igt_main
>>                 }
>>         }
>>  
>> +       igt_describe("test brightness with dpms on and off cycle");
>> +       igt_subtest_with_dynamic("brightness-with-dpms") {
>> +               for (int j = 0; j < (num_panel ? 2 : 1); j++) {
>> +                       test_setup(display, &contexts->output[j]);
>> +                       igt_dynamic_f("%s",
>> igt_output_name(contexts[j].output)) {
>> +                               check_dpms_cycle(&display, &contexts-
>> >output[j], &contexts[j]);
>> +                               test_cleanup(&display, &contexts-
>> >output[j]);
>> +                       }
>> +               }
>> +       }
>> +
>>         igt_fixture {
>>                 /* Restore old brightness */
>> -               for (i = 0; i < (dual_edp ? 2 : 1); i++)
>> +               for (i = 0; i < (num_panel ? 2 : 1); i++)
>>                         backlight_write(contexts[i].old,
>> "brightness", &contexts[i]);
>>  
>>                 igt_display_fini(&display);
>

-- 
Jani Nikula, Intel