Re: [PATCH i-g-t] tests/intel/km_pm_brightness: Check for flickers with changes in brightness

["Sharma, Swati2" <swati2.sharma@intel.com>]

Hi Santosh,

Please find review comments below

On 06-Sep-24 8:24 AM, Santhosh Reddy Guddati wrote:
>      The test is added to check for flickers on the panel while
This can be rephrased as "New test is added..."

>      changing brightness.
> 
>      If there is any flicker observed on the panel while changing
>      the brightness then crc will change.This patch set is trying
					   ^^
Add space after full stop.

>      to address the flickers observed using crc when brightness
>      is being changed.
> 
>      v3: Addressed review feedback - Kamil , thasleem

Also, what got changed from previous patch is mentioned in commit

There is extra tab in commit message.

Apart from this, fixes are still required in subject. There is no file 
named kms_pm_brightness :/

/s/km_pm_brightness/kms_pm_backlight

Also, subject can be made more meaningful something like
"Add test to detect flickering with brightness changes"

> 
> Signed-off-by: Santhosh Reddy Guddati <santhosh.reddy.guddati@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 140a95bcc..ae0ef8462 100644
> --- a/tests/intel/kms_pm_backlight.c
> +++ b/tests/intel/kms_pm_backlight.c
> @@ -66,6 +66,10 @@
>    * SUBTEST: fade-with-suspend
>    * Description: Test the fade with suspend.
>    * Functionality: backlight, suspend
> + *
> + * SUBTEST: brightness-with-crc
> + * Description: Test the brightness change with CRC.
> + * Functionality: backlight, CRC


This subtest can be added after "SUBTEST: basic-brightness" to club all 
brightness test together and it can be renamed as "basic-crc-brightness".
Functionality as defined in other IGTs is crc. So, /s/CRC/crc.

>    */
>   
>   struct context {
> @@ -257,6 +261,41 @@ 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;
> +	bool crc_ok;
> +
> +	/* create the pipe_crc object for this pipe */

This comment is not required

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

IMO, all all igt_debug prints are not required.

> +
> +		/* compare the crc values before and after changing brightness */
> +		crc_ok = igt_check_crc_equal(&out_crc_before, &out_crc_after);

In igt_check_crc_equal(), we already have igt_debug to print mismatched 
CRC values.

> +		igt_debug("brightness: %d flicker: %s\n", brightness, crc_ok ? "none" : "detected");
> +		igt_assert_f(crc_ok, "Flicker observed for brightness %d\n", brightness);
> +	}
> +}
> +
>   igt_main
>   {
>   	int fd;
> @@ -277,6 +316,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-with-crc", "test the brightness change with CRC.", test_brightness_crc, TEST_NONE },

Move it up. Add after brightness tests.

>   	};
>   
>   	igt_fixture {
> @@ -290,6 +330,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)