[PATCH i-g-t] tests/km_pm_brightness.c: check for flickers while adjusting the brightness.

[Kamil Konieczny]

Hi Santhosh,
On 2024-06-27 at 08:32:11 +0530, Santhosh Reddy Guddati wrote:

First concers is subject, please do not use '.c' from filename,
also drop dot from end of sentence:

[PATCH i-g-t] tests/km_pm_brightness: check for flickers while adjusting the brightness

Please improve description:

> The test is added to check for flickers on the panel while
> changing brightness.Though changing the brightness doesn't have impact

s/brightness.Though/brightness. Though/

> on CRC,but if there is a flicker observed , the crc will change

s/CRC,but/CRC but/
s/, the/ then/

> and this test validates the crc.

s/and this test validates the crc././

Third concern is this comment:

/* Some rounding may happen depending on hw */

Are you sure exact checks are ok?
Also I do not see any pm (power management) in the test?
But maybe I am missing something here.

> ---
>  tests/intel/kms_pm_backlight.c | 44 ++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/tests/intel/kms_pm_backlight.c b/tests/intel/kms_pm_backlight.c
> index 8672afa7a..e29fd7320 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-crc-change
> + * Description: Test the brightness change with CRC.
> + * Functionality: backlight, CRC
>   */
>  
>  struct context {
> @@ -255,6 +259,44 @@ static void test_setup(igt_display_t display, igt_output_t *output)
>  	}
>  }
>  
> +static void test_brightness_crc(struct context *context)
> +{
> +	int min = 0;
> +	int max = context->max;
> +	int step = (max - min) / 10;
> +	int brightness;
> +	igt_crc_t out_crc_before, out_crc_after;
> +	igt_display_t *display = context->output->display;
> +	char *crc_str;
> +	igt_pipe_crc_t *ref_crc;
> +
> +	// Get the CRC before changing brightness
> +	ref_crc = igt_pipe_crc_new(display->drm_fd, context->output->config.pipe, IGT_PIPE_CRC_SOURCE_AUTO);
> +	igt_assert(ref_crc);
> +
> +	for (brightness = min; brightness <= max; brightness += step) {
> +		igt_info("Brightness: %d\n", brightness);
> +
> +		igt_pipe_crc_collect_crc(ref_crc, &out_crc_before);
> +		crc_str = igt_crc_to_string(&out_crc_before);
> +		igt_debug("CRC before write: %s\n", crc_str);
> +
> +		igt_assert_eq(backlight_write(brightness, "brightness", context), 0);
> +		igt_debug("Brightness after write: %d\n", brightness);
> +
> +		igt_pipe_crc_collect_crc(ref_crc, &out_crc_after);
> +		crc_str = igt_crc_to_string(&out_crc_after);
> +		igt_debug("CRC after write: %s\n", crc_str);
> +

Print igt_info (or better - igt_debug) here.

> +		// Compare the CRC values
> +		if (igt_check_crc_equal(&out_crc_before, &out_crc_after)) {
> +			igt_info("CRC values are same for brightness: %d\n", brightness);
> +		} else {
> +			igt_info("CRC values are different for brightness: %d\n", brightness);

Why igt_info and not igt_assert_f here? Or drop 'if ... else ...'
and use something like:

		crc_ok = igt_check_crc_equal(&out_crc_before, &out_crc_after);
        igt_debug("brightness: %d flicker: %s\n", brightness, crc_ok ? "none" : "detected");
        igt_assert_f(crc_ok, "Flicker observed for brightness %d\n", brightness);

Regards,
Kamil

> +		}
> +	}
> +}
> +
>  igt_main
>  {
>  	int fd;
> @@ -275,6 +317,7 @@ igt_main
>  		{ "fade", "test basic fade.", test_fade, TEST_NONE },
>  		{ "fade-with-dpms", "test the fade with DPMS.", test_fade, TEST_DPMS },
>  		{ "fade-with-suspend", "test the fade with suspend.", test_fade, TEST_SUSPEND },
> +		{ "brightness-crc-change", "test the brightness change with CRC.", test_brightness_crc, TEST_NONE },
>  	};
>  
>  	igt_fixture {
> @@ -288,6 +331,7 @@ igt_main
>  		 */
>  		kmstest_set_vt_graphics_mode();
>  		igt_display_require(&display, drm_open_driver(DRIVER_INTEL | DRIVER_XE));
> +		igt_require_pipe_crc(display.drm_fd);
>  
>  		for_each_connected_output(&display, output) {
>  			if (output->config.connector->connector_type != DRM_MODE_CONNECTOR_eDP)
> -- 
> 2.34.1
>