Re: [igt-dev] [PATCH i-g-t 6/7] tests/kms_hdr: Add subtest to validate HDR mode using luminance sensor

Shankar, Uma
2020-01-14T15:03:56Z

>-----Original Message-----
>From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of Petri Latvala
>Sent: Wednesday, January 8, 2020 2:59 PM
>To: Sharma, Swati2 <swati2.sharma@intel.com>
>Cc: igt-dev@lists.freedesktop.org
>Subject: Re: [igt-dev] [PATCH i-g-t 6/7] tests/kms_hdr: Add subtest to validate HDR
>mode using luminance sensor
>
>On Tue, Dec 31, 2019 at 06:51:57PM +0530, Swati Sharma wrote:
>> From: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>
>> Add subtest to validate HDR mode by setting the static metadata and
>> this involves display level verification for infoframes using a
>> luminance sensor. Therefore, luminance sensor is required for this
>> subtest, if sensor is not available test will skip.
>>
>> On intel driver, haven't validated this subtest but included this for
>> amd driver.
>>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>> ---
>> tests/kms_hdr.c | 271
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 271 insertions(+)
>>
>> diff --git a/tests/kms_hdr.c b/tests/kms_hdr.c index
>> dfd377b5..a63021b1 100644
>> --- a/tests/kms_hdr.c
>> +++ b/tests/kms_hdr.c
>> @@ -44,6 +44,7 @@ enum {
>> TEST_NONE = 0,
>> TEST_DPMS = 1 << 0,
>> TEST_SUSPEND = 1 << 1,
>> + TEST_OUTPUT = 1 << 2,
>> };
>>
>> /* BPC connector state. */
>> @@ -61,6 +62,7 @@ typedef struct data {
>> igt_pipe_crc_t *pipe_crc;
>> drmModeModeInfo *mode;
>> enum pipe pipe_id;
>> + int sensor_fd;
>> int fd;
>> int w;
>> int h;
>> @@ -165,6 +167,7 @@ static void prepare_test(data_t *data,
>> igt_output_t *output, enum pipe pipe)
>>
>> data->w = data->mode->hdisplay;
>> data->h = data->mode->vdisplay;
>> + data->sensor_fd = -1;
>> }
>>
>> static bool igt_pipe_is_free(igt_display_t *display, enum pipe pipe)
>> @@ -393,6 +396,269 @@ static void test_static_toggle(data_t *data, igt_output_t
>*output,
>> }
>> }
>>
>> +/*
>> + * Loads the sensor if unloaded. The sensor is a serial to USB
>> +interface that
>> + * prints the current measured luminance (nits) as a float, separated
>> +by a
>> + * newline. Uses baudrate 9600.
>> + */
>> +static void open_sensor(data_t *data) {
>> + struct termios toptions;
>> + int res;
>> +
>> + /* Return if already loaded. */
>> + if (data->sensor_fd >= 0)
>> + return;
>> +
>> + data->sensor_fd = open("/dev/ttyUSB0", O_RDWR | O_NOCTTY);
>
>What is the sensor device in question? How can you even know this device, if it exists,
>is the sensor?
>
>Connecting a usb-serial cable for a debug terminal is common enough to warrant not
>using ttyUSB0 blindly.

I agree, this is a hard coding and need to have better detection mechanism to identify
if a luminance sensor is actually present.

>
>> + if (data->sensor_fd < 0) {
>> + igt_info("Luminance sensor not found.\n");
>> + return;
>> + }
>> +
>> + res = tcgetattr(data->sensor_fd, &toptions);
>> + igt_assert_lte(0, res);
>> +
>> + cfsetispeed(&toptions, B9600);
>> + cfsetospeed(&toptions, B9600);
>> +
>> + toptions.c_cflag &= ~(PARENB | CSTOPB | CSIZE);
>> + toptions.c_cflag |= (CS8 | CREAD | CLOCAL);
>> + toptions.c_lflag |= ICANON;
>> +
>> + cfmakeraw(&toptions);
>> +
>> + res = tcsetattr(data->sensor_fd, TCSANOW, &toptions);
>> + igt_assert_lte(0, res);
>> +
>> + res = tcsetattr(data->sensor_fd, TCSAFLUSH, &toptions);
>> + igt_assert_lte(0, res);
>> +}
>> +
>> +/* Reads a string from the sensor. */ static void
>> +get_sensor_str(data_t *data, char *dst, int dst_bytes) {
>> + char c = 0;
>> + int i = 0;
>> +
>> + igt_require(data->sensor_fd >= 0);
>> + igt_set_timeout(3, "Waiting for sensor read\n");
>> +
>> + dst_bytes -= 1;
>> +
>> + while (c != '\n' && i < dst_bytes) {
>> + int n = read(data->sensor_fd, &c, 1);
>> + igt_assert_lte(0, n);
>> +
>> + dst[i++] = c;
>> + }
>> +
>> + igt_reset_timeout();
>> +
>> + if (dst_bytes > 0) {
>> + dst[i] = 0;
>> + }
>
>
>The buffer can be left non-nul-terminated if the device gives too much text.
>
>
>--
>Petri Latvala
>
>
>
>
>> +}
>> +
>> +/* Asserts that two given values in nits are equal within a given threshold. */
>> +static void assert_nits_equal(double n0, double n1, double threshold)
>> +{
>> + double diff = fabs(n0 - n1);
>> +
>> + igt_assert_f(diff <= threshold,
>> + "Nits not in threshold: | %.3f - %.3f | > %.3f\n",
>> + n0, n1, threshold);
>> +}
>> +
>> +/* Returns the current luminance reading from the sensor in cd/m^2. */
>> +static float get_sensor_nits(data_t *data)
>> +{
>> + float nits = -1.0f;
>> + char buf[32];
>> +
>> + /* Sensor opening is deferred until we actually need it - here. */
>> + open_sensor(data);
>> +
>> + /* Flush old data from the buffer in case it's been a while. */
>> + igt_require(data->sensor_fd >= 0);
>> + tcflush(data->sensor_fd, TCIOFLUSH);
>> +
>> + /* Read twice so we get a clean line. */
>> + get_sensor_str(data, buf, ARRAY_SIZE(buf));
>> + get_sensor_str(data, buf, ARRAY_SIZE(buf));
>> +
>> + sscanf(buf, "%f", &nits);
>> +
>> + return nits;
>> +}
>> +
>> +/* Logs the cursor sensor nits. */
>> +static void log_sensor_nits(double nits)
>> +{
>> + igt_info("Sensor: %.3f nits\n", nits);
>> +}
>> +
>> +/*
>> + * Waits for the monitor to light-up to a given threshold, useful for
>> + * post-modeset measurement.
>> + */
>> +static void wait_for_threshold(data_t *data, double threshold)
>> +{
>> + /*
>> + * If we read too quick the sensor might still be lit up.
>> + * Easy hack: just wait a second.
>> + */
>> + sleep(1);
>> +
>> + /* Poll sensor until lightup. */
>> + igt_set_timeout(5, "Waiting for sensor read\n");
>> +
>> + for (;;) {
>> + double nits = get_sensor_nits(data);
>> +
>> + if (nits >= threshold)
>> + break;
>> + }
>> +
>> + igt_reset_timeout();
>> +}
>> +
>> +/* PQ Inverse, L normalized luminance to signal value V. */
>> +static double calc_pq_inverse(double l)
>> +{
>> + double c1 = 0.8359375;
>> + double c2 = 18.8515625;
>> + double c3 = 18.6875;
>> + double m1 = 0.1593017578125;
>> + double m2 = 78.84375;
>> + double lm = pow(l, m1);
>> +
>> + return pow((c1 + c2 * lm) / (1.0 + c3 * lm), m2);
>> +}
>> +
>> +/* Fills the FB with a solid color given mapping to a light value in nits. */
>> +static void draw_light(igt_fb_t *fb, double nits)
>> +{
>> + cairo_t *cr;
>> + double l, v;
>> + int x, y, w, h;
>> +
>> + cr = igt_get_cairo_ctx(fb->fd, fb);
>> +
>> + l = nits / 10000.0;
>> + v = calc_pq_inverse(l);
>> +
>> + /*
>> + * Draw a white rect in the bottom center of the screen for the sensor.
>> + * It's only 10% of the width and height of the screen since not every
>> + * monitor is capable of full HDR brightness for the whole screen.
>> + */
>> + w = fb->width * 0.10;
>> + h = fb->height * 0.10;
>> + x = (fb->width - w) / 2;
>> + y = (fb->height - h);
>> +
>> + igt_paint_color(cr, 0, 0, fb->width, fb->height, 0.0, 0.0, 0.0);
>> + igt_paint_color(cr, x, y, w, h, v, v, v);
>> +
>> + igt_put_cairo_ctx(fb->fd, fb, cr);
>> +}
>> +
>> +/*
>> + * Note: This test is display specific in the sense that it requries
>> + * a display that is capable of going above SDR brightness levels.
>> + * Most HDR400 or higher certified displays should be capable of this.
>> + *
>> + * Some displays may require first limiting the output brightness
>> + * in the OSD for this to work.
>> + *
>> + * Requires the luminance sensor to be attached to the test machine,
>> + * if sensor isn't attached to the test machine, test will skip.
>> + */
>> +static void test_static_output(data_t *data, igt_output_t *output)
>> +{
>> + igt_display_t *display = &data->display;
>> + struct hdr_output_metadata hdr;
>> + enum pipe pipe;
>> + igt_fb_t afb;
>> + int afb_id;
>> + double lightup = 100.0;
>> + double threshold = 15.0;
>> + double nits_sdr0, nits_sdr1, nits_sdr2, nits_hdr;
>> +
>> + for_each_pipe(display, pipe) {
>> + if (!igt_pipe_connector_valid(pipe, output))
>> + continue;
>> +
>> + if (!igt_pipe_is_free(display, pipe))
>> + continue;
>> +
>> + prepare_test(data, output, pipe);
>> +
>> + afb_id = igt_create_fb(data->fd, data->w, data->h,
>DRM_FORMAT_XRGB2101010, 0, &afb);
>> + igt_assert(afb_id);
>> +
>> + draw_light(&afb, 10000.0);
>> +
>> + igt_info("Test SDR, 8bpc\n");
>> + 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);
>> + if (is_amdgpu_device(data->fd))
>> + assert_output_bpc(data, 8);
>> +
>> + wait_for_threshold(data, lightup);
>> + nits_sdr0 = get_sensor_nits(data);
>> + log_sensor_nits(nits_sdr0);
>> +
>> + igt_info("Test SDR, 10bpc\n");
>> + igt_output_set_prop_value(data->output,
>IGT_CONNECTOR_MAX_BPC, 10);
>> + igt_display_commit_atomic(display,
>DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
>> + if (is_amdgpu_device(data->fd))
>> + assert_output_bpc(data, 10);
>> +
>> + wait_for_threshold(data, lightup);
>> + nits_sdr1 = get_sensor_nits(data);
>> + log_sensor_nits(nits_sdr1);
>> +
>> + igt_info("Test HDR, 10bpc\n");
>> + 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);
>> +
>> + wait_for_threshold(data, lightup);
>> + nits_hdr = get_sensor_nits(data);
>> + log_sensor_nits(nits_hdr);
>> +
>> + igt_info("Exit HDR into SDR, 8bpc\n");
>> + 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);
>> + if (is_amdgpu_device(data->fd))
>> + assert_output_bpc(data, 8);
>> +
>> + wait_for_threshold(data, lightup);
>> + nits_sdr2 = get_sensor_nits(data);
>> + log_sensor_nits(nits_sdr2);
>> +
>> + /* Verify that all nit values were as expected. */
>> + assert_nits_equal(nits_sdr0, nits_sdr1, threshold);
>> + assert_nits_equal(nits_sdr0, nits_sdr2, threshold);
>> +
>> + igt_assert_f(nits_hdr - nits_sdr0 > threshold * 2.0,
>> + "No measurable difference between SDR and HDR luminance: "
>> + "threshold=%.1f actual=%.1f\n",
>> + threshold * 2.0, nits_hdr - nits_sdr0);
>> +
>> + test_fini(data);
>> + igt_remove_fb(data->fd, &afb);
>> +
>> + break;
>> + }
>> +}
>> +
>> /* Returns true if an output supports hdr metadata property */
>> static bool has_hdr(igt_output_t *output)
>> {
>> @@ -418,6 +684,8 @@ static void test_hdr(data_t *data, const char *test_name,
>uint32_t flags)
>> igt_info("HDR %s test execution on %s\n", test_name, output-
>>name);
>> if (flags & TEST_NONE || flags & TEST_DPMS || flags &
>TEST_SUSPEND)
>> test_static_toggle(data, output, flags);
>> + if (flags & TEST_OUTPUT)
>> + test_static_output(data, output);
>> valid_tests++;
>> }
>>
>> @@ -453,6 +721,9 @@ igt_main
>> igt_describe("Tests static toggle with suspend");
>> igt_subtest("static-toggle-suspend") test_hdr(&data, "static-toggle-suspend",
>TEST_SUSPEND);
>>
>> + igt_describe("Tests HDR mode by setting the static metadata");
>> + igt_subtest("static-output") test_hdr(&data, "static-output", TEST_OUTPUT);
>> +
>> igt_fixture {
>> igt_display_fini(&data.display);
>> }
>> --
>> 2.24.1
>>
>> _______________________________________________
>> igt-dev mailing list
>> igt-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/igt-dev
>_______________________________________________
>igt-dev mailing list
>igt-dev@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/igt-dev