[PATCH i-g-t v1 1/2] tests/kms_hdr: Add brightness tests for hdr

[Samala, Pranay]

Hi Santosh,

> -----Original Message-----
> From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of Santhosh
> Reddy Guddati
> Sent: Tuesday, September 24, 2024 5:26 PM
> To: igt-dev at lists.freedesktop.org
> Cc: Reddy Guddati, Santhosh <santhosh.reddy.guddati at intel.com>
> Subject: [PATCH i-g-t v1 1/2] tests/kms_hdr: Add brightness tests for hdr
> 
> Add subtests for Aux based backlight control on hdr enabled edp panels by
> changing brightness via sysfs.
> 
> Signed-off-by: Santhosh Reddy Guddati <santhosh.reddy.guddati at intel.com>
> ---
>  tests/kms_hdr.c | 106
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 106 insertions(+)
> 
> diff --git a/tests/kms_hdr.c b/tests/kms_hdr.c index f123c6b36..4e6ff87b4
> 100644
> --- a/tests/kms_hdr.c
> +++ b/tests/kms_hdr.c
> @@ -64,6 +64,18 @@
>   * Description: Tests static toggle with suspend
>   * Functionality: colorspace, static_hdr, suspend
>   *
> + * SUBTEST: brightness-with-hdr
> + * Description: Tests brightness with HDR
> + * Functionality: colorspace, static_hdr
> + *
> + * SUBTEST: brightness-with-dpms
> + * Description: Tests brightness with dpms
> + * Functionality: colorspace, static_hdr, dpms
> + *
> + * SUBTEST: brightness-with-suspend
> + * Description: Tests brightness with suspend
> + * Functionality: colorspace, static_hdr, suspend
> + *
>   * SUBTEST: static-%s
>   * Description: Tests %arg[1].
>   * Functionality: colorspace, static_hdr @@ -100,6 +112,9 @@ enum {
>  	TEST_SWAP = 1 << 3,
>  	TEST_INVALID_METADATA_SIZES = 1 << 4,
>  	TEST_INVALID_HDR = 1 << 5,
> +	TEST_BRIGHTNESS = 1 << 6,
> +	TEST_BRIGHTNESS_DPMS = 1 << 7,
> +	TEST_BRIGHTNESS_SUSPEND = 1 << 8,
>  };
> 
>  /* BPC connector state. */
> @@ -623,6 +638,80 @@ static bool has_hdr(igt_output_t *output)
>  	return igt_output_has_prop(output,
> IGT_CONNECTOR_HDR_OUTPUT_METADATA);
>  }
> 
> +static int test_brightness_output(data_t *data, enum pipe pipe,
> +igt_output_t *output, uint32_t flags) {
> +	igt_display_t *display = &data->display;
> +	igt_fb_t afb;
> +	int afb_id;
> +	struct hdr_output_metadata hdr;
> +
> +	/* 10-bit formats are slow, so limit the size. */
> +	afb_id = igt_create_fb(data->fd, 512, 512,
> +				   DRM_FORMAT_XRGB2101010,
> DRM_FORMAT_MOD_LINEAR, &afb);

Alignment should match open parenthesis

> +	igt_assert(afb_id);
> +
> +	draw_hdr_pattern(&afb);
> +
> +	/* Start in SDR. */
> +	igt_plane_set_fb(data->primary, &afb);
> +	igt_plane_set_size(data->primary, data->w, data->h);
> +	igt_output_set_prop_value(data->output, IGT_CONNECTOR_MAX_BPC,
> 8);
> +	igt_display_commit_atomic(display,
> DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> +	igt_assert_output_bpc_equal(data->fd, pipe, output->name, 8);
> +
> +	/* Enter HDR, a modeset is allowed here. */
> +	fill_hdr_output_metadata_st2048(&hdr);
> +	set_hdr_output_metadata(data, &hdr);
> +	igt_output_set_prop_value(data->output, IGT_CONNECTOR_MAX_BPC,
> 10);
> +	igt_display_commit_atomic(display,
> DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> +	igt_assert_output_bpc_equal(data->fd, pipe, output->name, 10);

Why are we not verifying the crc values here, as it is doing for other subtests?

> +
> +	/* Adjust brightness */
> +	char brightness_path[1024];
> +	int brightness_fd;
> +	char brightness_value[4];
> +
> +	snprintf(brightness_path, sizeof(brightness_path),
> +			 "/sys/class/backlight/intel_backlight/brightness");
> +
> +	brightness_fd = open(brightness_path, O_WRONLY);
> +	igt_assert_f(brightness_fd >= 0, "Failed to open %s\n",
> +brightness_path);
> +
> +	int write_brightness = 50;

Rather than going for a random value, we can check if this brightness value 
is in the range of 0 to max_brightness or not.

Or write a function which will read the max_brightness value and reduce it 
by the value which you will send it as an argument.

Leave a blank line after declarations.

> +	snprintf(brightness_value, sizeof(brightness_value), "%d",
> write_brightness);
> +	igt_assert(write(brightness_fd, brightness_value,
> +strlen(brightness_value)) > 0);
> +
> +	close(brightness_fd);

Instead of reading/writing the brightness value in the test via sysfs, 
you can write a function in lib/igt_sysfs.

Those functions can be used to read max_brightness value/
edit brightness value/compare it also.

> +
> +	if (flags & TEST_BRIGHTNESS_DPMS)
> +		test_cycle_flags(data, TEST_DPMS);
> +	if (flags & TEST_BRIGHTNESS_SUSPEND)
> +		test_cycle_flags(data, TEST_SUSPEND);
> +
> +	/* Verify brightness adjustment */
> +	brightness_fd = open(brightness_path, O_RDONLY);
> +	igt_assert(brightness_fd >= 0);
> +
> +	igt_assert(read(brightness_fd, brightness_value,
> +sizeof(brightness_value)) > 0);
> +
> +	int read_brightness = atoi(brightness_value);
> +
> +	igt_assert_eq(read_brightness, write_brightness);
> +
> +	close(brightness_fd);

Similarly, we can also have a function in lib/igt_sysfs to compare the
 brightness values.

Use exit handler to set the brightness value to default value before 
exiting the test.

> +
> +	/* Exit HDR and enter 8bpc, cleanup. */
> +	set_hdr_output_metadata(data, NULL);
> +	igt_output_set_prop_value(data->output, IGT_CONNECTOR_MAX_BPC,
> 8);
> +	igt_display_commit_atomic(display,
> DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> +	igt_assert_output_bpc_equal(data->fd, pipe, output->name, 8);
> +
> +	test_fini(data);
> +	igt_remove_fb(data->fd, &afb);
> +
> +	return 0;
> +}
> +
>  static void test_hdr(data_t *data, uint32_t flags)  {
>  	igt_display_t *display = &data->display; @@ -691,6 +780,11 @@ static
> void test_hdr(data_t *data, uint32_t flags)
>  					test_static_swap(data, pipe, output);
>  				if (flags & TEST_INVALID_METADATA_SIZES)
>  					test_invalid_metadata_sizes(data,
> output);
> +				if (flags & (TEST_BRIGHTNESS |
> TEST_BRIGHTNESS_DPMS | TEST_BRIGHTNESS_SUSPEND)) {

It exceeds 100-character limitation in a line.

> +					igt_require(output->config.connector-
> >connector_type == DRM_MODE_CONNECTOR_eDP);

It exceeds 100-character limitation in a line.

> +					igt_require_f(is_intel_device(data->fd),
> "Only supported on Intel devices\n");

It exceeds 100-character limitation in a line.

Regards,
Pranay Samala
> +					test_brightness_output(data, pipe,
> output, flags);
> +				}
>  			}
> 
>  			/* One pipe is enough */
> @@ -734,6 +828,18 @@ igt_main
>  	igt_subtest_with_dynamic("static-toggle-suspend")
>  		test_hdr(&data, TEST_SUSPEND);
> 
> +	igt_describe("Tests brightness while entering and exiting HDR mode");
> +	igt_subtest_with_dynamic("brightness-with-hdr")
> +		test_hdr(&data, TEST_BRIGHTNESS);
> +
> +	igt_describe("Tests brightness with dpms");
> +	igt_subtest_with_dynamic("brightness-with-dpms")
> +		test_hdr(&data, TEST_BRIGHTNESS_DPMS);
> +
> +	igt_describe("Tests brightness with suspend");
> +	igt_subtest_with_dynamic("brightness-with-suspend")
> +		test_hdr(&data, TEST_BRIGHTNESS_SUSPEND);
> +
>  	igt_describe("Tests swapping static HDR metadata");
>  	igt_subtest_with_dynamic("static-swap")
>  		test_hdr(&data, TEST_SWAP);
> --
> 2.34.1