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

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

On Fri, 07 Jun 2024, Mohammed Thasleem <mohammed.thasleem@intel.com> wrote:
> Set brightness by deviding max brightness, store and read it back.
> The actual brightness should be same during DPMS on and off cycle.

Not strictly related to the patch at hand, but please find some some
observations about the whole test inline.

>
> v2: Update test path and testplan documentation.
>
> Signed-off-by: Mohammed Thasleem <mohammed.thasleem@intel.com>
> ---
>  tests/intel/kms_pm_backlight.c | 41 ++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/tests/intel/kms_pm_backlight.c b/tests/intel/kms_pm_backlight.c
> index 2e691dab0..107b7d668 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_SUSPENDED));
> +
> +	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_ACTIVE));
> +
> +	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);
> @@ -354,6 +384,17 @@ igt_main
>  		}
>  	}
>  
> +	igt_describe("test brightness with dpms on and off cycle");
> +	igt_subtest_with_dynamic("brightness-with-dpms") {
> +		for (int j = 0; j < (dual_edp ? 2 : 1); j++) {

The "dual_edp ? 2 : 1" is what caught my eyes here.

First I thought, why don't you have "num_edp" or something here.

Next I thought, backlight is not limited to eDP. Why aren't we testing
backlight with LVDS or DSI? "num_edp" would be wrong too.

Then I realized the whole dual_edp thing and backlight <-> output
mapping is a bit fragile, and depends on things that aren't necessarily
guaranteed to be stable. It assumes the "first" eDP connector is named
intel_backlight and the rest card%i-%s-backlight. It probably works in
the limited environment and systems we have, but something to keep in
mind.


BR,
Jani.


> +			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++)

-- 
Jani Nikula, Intel