[PATCH i-g-t v2 1/2] tests/kms_hdr: Add brightness tests for hdr
Kandpal, Suraj
suraj.kandpal at intel.com
Mon Sep 30 05:54:53 UTC 2024
>
> On 27-09-2024 15:50, Santhosh Reddy Guddati wrote:
> > Add subtests for Aux based backlight control on hdr enabled edp panels
> > by changing brightness via sysfs.
Correction what your test is doing here is checking if brightness can be manipulated during HDR
And not checking if its taking Aux based path since if the edp doesn’t support it, it will take the usual
Path. Tbh I don’t see a way to check which path its taking so maybe update the commit message to
Say this is a test to see if we can manipulate brightness while in HDR
> >
> > v2: Add function to adjust brightness in range 0 to max, refactor code
> > and store and restore brightness before and after test. (Pranay)
> >
> > Signed-off-by: Santhosh Reddy Guddati
> > <santhosh.reddy.guddati at intel.com>
> > ---
> > tests/kms_hdr.c | 144
> ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 144 insertions(+)
> >
> > diff --git a/tests/kms_hdr.c b/tests/kms_hdr.c index
> > f123c6b36..115ed6ab5 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 @@ -81,6 +93,9 @@
> > IGT_TEST_DESCRIPTION("Test HDR metadata interfaces and bpc switch");
> > #define HDR_STATIC_METADATA_BLOCK 0x06
> > #define USE_EXTENDED_TAG 0x07
> >
> > +/* Intel Backlight Path. */
The comment here is not required the macro name is self explanatory
> > +#define INTEL_BACKLIGHT_PATH "/sys/class/backlight/intel_backlight"
> > +
> > /* DRM HDR definitions. Not in the UAPI header, unfortunately. */
> > enum hdmi_metadata_type {
> > HDMI_STATIC_METADATA_TYPE1 = 0,
> > @@ -100,6 +115,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,
> > };
Why 3 extra tests I don’t thinking testing the brightness with dpms and suspend is any value add you will
Essentially only be testing hdr which is already being done
> >
> > /* BPC connector state. */
> > @@ -623,6 +641,112 @@ static bool has_hdr(igt_output_t *output)
> > return igt_output_has_prop(output,
> IGT_CONNECTOR_HDR_OUTPUT_METADATA);
> > }
> >
> > +/* Move adjust_brightness to lib/igt_sysfs to reuse in other tests.
> > +*/ static void adjust_brightness(data_t *data, uint32_t flags) {
> > + char brightness_path[1024];
> > + int brightness_fd;
> > + char brightness_value[64];
> > +
> > + snprintf(brightness_path, sizeof(brightness_path),
> > + "%s/%s", INTEL_BACKLIGHT_PATH, "brightness");
Nit: Please align this with start bracket looks cleaner like I show below:
(XXXX ...,
YYYY);
Some other places I feel this needs to be addressed marked them with Ditto.
> > +
> > + /* Get current brightness */
> > + brightness_fd = open(brightness_path, O_RDONLY);
> > + igt_assert_f(brightness_fd >= 0, "Failed to open %s\n",
> > +brightness_path);
> > +
> > + igt_assert(read(brightness_fd, brightness_value,
> > +sizeof(brightness_value)) > 0);
> > +
> > + int current_brightness = atoi(brightness_value);
No no never declare variables in the middle makes code very messy please declare
Them at the start of the function.
> > +
> > + close(brightness_fd);
> > +
> > + /* Get max brightness */
> > + snprintf(brightness_path, sizeof(brightness_path),
> > + "%s/%s", INTEL_BACKLIGHT_PATH, "max_brightness");
Ditto
> > +
> > + brightness_fd = open(brightness_path, O_RDONLY);
> > + igt_assert_f(brightness_fd >= 0, "Failed to open %s\n",
> > +brightness_path);
> > +
> > + igt_assert(read(brightness_fd, brightness_value,
> > +sizeof(brightness_value)) > 0);
> > +
> > + int max_brightness = atoi(brightness_value);
Again no variables to be declare in middle.
> > +
> > + close(brightness_fd);
> > +
> > + for (int write_brightness = 0; write_brightness <= max_brightness;
> write_brightness += 50) {
Keep the naming simple just j or something and declare it at the start
> > + brightness_fd = open(brightness_path, O_WRONLY);
> > + igt_assert_f(brightness_fd >= 0, "Failed to open %s\n",
> > +brightness_path);
> > +
> > + snprintf(brightness_value, sizeof(brightness_value), "%d",
> write_brightness);
> > + igt_assert(write(brightness_fd, brightness_value,
> > +strlen(brightness_value)) > 0);
> > +
> > + close(brightness_fd);
> > +
> > + /* 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);
> > + }
> > +
> > + brightness_fd = open(brightness_path, O_WRONLY);
> > + igt_assert_f(brightness_fd >= 0, "Failed to open %s\n",
> > +brightness_path);
> > +
> > + snprintf(brightness_value, sizeof(brightness_value), "%d",
> current_brightness);
> > + igt_assert(write(brightness_fd, brightness_value,
> > +strlen(brightness_value)) > 0); }
> > +
> > +static void 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);
Ditto
> > + 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);
Idt you need to start if as hdr just make sure you set your metadata here as 0 check static toggle test
> > + 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);
> > +
Whats the point of lall this output_bpc_equal this makes it very similar to static_toggle_test
If that’s the case why not modify that function to adjust brightness and test that based on the flag inside that function should significantly
Reduce the size of this patch.
> > + adjust_brightness(data, flags);
> > +
> > + /* 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);
> > +
> > +}
> > +
> > static void test_hdr(data_t *data, uint32_t flags)
> > {
> > igt_display_t *display = &data->display; @@ -691,6 +815,14 @@
> > 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)) {
> > + igt_require(output->config.connector-
> >connector_type ==
> > +
> DRM_MODE_CONNECTOR_eDP);
> > + igt_require_f(is_intel_device(data->fd),
> > + "Only
> supported on Intel devices\n");
> > + test_brightness_output(data, pipe,
> output, flags);
> > + }
> > }
> >
> > /* One pipe is enough */
> > @@ -734,6 +866,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);
More information about the igt-dev
mailing list