Re: [PATCH 2/3] tools: Add amd_hdr_visual for manual HDR verification

vitaly prosyak
2026-03-18T20:13:54Z

On 2026-03-18 05:52, Kamil Konieczny wrote:
> Hi Alex,
> On 2026-03-17 at 11:51:31 -0600, Alex Hung wrote:
>> From: Wayne Lin <wayne.lin@amd.com>
>>
>> Add a visual verification tool for AMD HDR display output. This tool
>> displays HDR test patterns with different metadata types and waits for
>> user confirmation, enabling manual inspection of HDR output quality.
>>
>> Subtests:
>> - static-swap-smpte2084: Display with SMPTE ST2084 (PQ) HDR metadata
>> - static-swap-traditional-sdr: Display with traditional SDR gamma metadata
>>
>> Co-developed-by: Alex Hung <alex.hung@amd.com>
>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>> Signed-off-by: Wayne Lin <wayne.lin@amd.com>
>> ---
>> tools/amd_hdr_visual.c | 372 +++++++++++++++++++++++++++++++++++++++++
>> tools/meson.build | 5 +
>> 2 files changed, 377 insertions(+)
>> create mode 100644 tools/amd_hdr_visual.c
>>
>> diff --git a/tools/amd_hdr_visual.c b/tools/amd_hdr_visual.c
>> new file mode 100644
>> index 000000000..58decb356
>> --- /dev/null
>> +++ b/tools/amd_hdr_visual.c
>> @@ -0,0 +1,372 @@
>> +/*
> Add SPDX here, look into other C files and use Linux script
> checkpatch.pl for verifing.
>
>> + * Copyright 2026 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person obtaining a
> Remove this text, it is replaced by SPDX.
>
>> + * copy of this software and associated documentation files (the "Software"),
>> + * to deal in the Software without restriction, including without limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#include "igt.h"
> Why igt.h here? It should be after system headers but also
> try to decouple it from big test libs. Tools should at least
> try to be tools, not tests.
>
>> +#include <fcntl.h>
>> +#include <termios.h>
>> +#include <unistd.h>
> Add newline here.
>
>> +#include "igt_edid.h"
>> +#include "igt_hdr.h"
>> +
>> +IGT_TEST_DESCRIPTION("Test HDR metadata interfaces and bpc switch");
> Not needed in tools, remove it. Or did you mean a test, not a tool?
> +cc Vitaly
> Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
>
> Regards,
> Kamil
>
>> +
>> +/* Test flags. */
>> +enum {
>> + TEST_NONE = 1 << 0,
>> + TEST_DPMS = 1 << 1,
>> + TEST_SUSPEND = 1 << 2,
>> + TEST_SWAP = 1 << 3,
>> +};
>> +
>> +/* BPC connector state. */
>> +typedef struct output_bpc {
>> + unsigned int current;
>> + unsigned int maximum;
>> +} output_bpc_t;
>> +
>> +/* Common test data. */
>> +typedef struct data {
>> + igt_display_t display;
>> + igt_plane_t *primary;
>> + igt_output_t *output;
>> + igt_crtc_t *pipe;
>> + igt_pipe_crc_t *pipe_crc;
>> + drmModeModeInfo *mode;
>> + enum pipe pipe_id;
>> + int fd;
>> + int w;
>> + int h;
>> +} data_t;
>> +
>> +/* Common test cleanup. */
>> +static void test_fini(data_t *data)
>> +{
>> + igt_pipe_crc_free(data->pipe_crc);
>> + igt_display_reset(&data->display);
>> +}
>> +
>> +static void test_cycle_flags(data_t *data, uint32_t test_flags)
>> +{
>> + if (test_flags & TEST_DPMS) {
>> + kmstest_set_connector_dpms(data->fd,
>> + data->output->config.connector,
>> + DRM_MODE_DPMS_OFF);
>> + kmstest_set_connector_dpms(data->fd,
>> + data->output->config.connector,
>> + DRM_MODE_DPMS_ON);
>> + }
>> +
>> + if (test_flags & TEST_SUSPEND)
>> + igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
>> + SUSPEND_TEST_NONE);
>> +}
>> +
>> +/* Returns the current and maximum bpc from the connector debugfs. */
>> +static output_bpc_t get_output_bpc(data_t *data)
>> +{
>> + char buf[256];
>> + char *start_loc;
>> + int fd, res;
>> + output_bpc_t info;
>> +
>> + fd = igt_debugfs_connector_dir(data->fd, data->output->name, O_RDONLY);
>> + igt_assert(fd >= 0);
>> +
>> + res = igt_debugfs_simple_read(fd, "output_bpc", buf, sizeof(buf));
>> +
>> + igt_require(res > 0);
>> +
>> + close(fd);
>> +
>> + igt_assert(start_loc = strstr(buf, "Current: "));
>> + igt_assert_eq(sscanf(start_loc, "Current: %u", &info.current), 1);
>> +
>> + igt_assert(start_loc = strstr(buf, "Maximum: "));
>> + igt_assert_eq(sscanf(start_loc, "Maximum: %u", &info.maximum), 1);
>> +
>> + return info;
>> +}
>> +
>> +/* Verifies that connector has the correct output bpc. */
>> +static void assert_output_bpc(data_t *data, unsigned int bpc)
>> +{
>> + output_bpc_t info = get_output_bpc(data);
>> +
>> + igt_require_f(info.maximum >= bpc,
>> + "Monitor doesn't support %u bpc, max is %u\n", bpc,
>> + info.maximum);
>> +
>> + igt_assert_eq(info.current, bpc);
>> +}
>> +
>> +/* Fills the FB with a test HDR pattern. */
>> +static void draw_hdr_pattern(igt_fb_t *fb)
>> +{
>> + cairo_t *cr = igt_get_cairo_ctx(fb->fd, fb);
>> +
>> + igt_paint_color(cr, 0, 0, fb->width, fb->height, 1.0, 1.0, 1.0);
>> + igt_paint_test_pattern(cr, fb->width, fb->height);
>> +
>> + igt_put_cairo_ctx(cr);
>> +}
>> +
>> +/* Prepare test data. */
>> +static void prepare_test(data_t *data, igt_output_t *output, enum pipe pipe)
>> +{
>> + igt_display_t *display = &data->display;
>> +
>> + data->pipe_id = pipe;
>> + data->pipe = &data->display.crtcs[data->pipe_id];
>> + igt_assert(data->pipe);
>> +
>> + igt_display_reset(display);
>> +
>> + data->output = output;
>> + igt_assert(data->output);
>> +
>> + data->mode = igt_output_get_mode(data->output);
>> + igt_assert(data->mode);
>> +
>> + data->primary =
>> + igt_crtc_get_plane_type(data->pipe, DRM_PLANE_TYPE_PRIMARY);
>> +
>> + data->pipe_crc = igt_pipe_crc_new(data->fd, data->pipe_id,
>> + IGT_PIPE_CRC_SOURCE_AUTO);
>> +
>> + igt_output_set_crtc(data->output, igt_crtc_for_pipe(display, data->pipe_id));
>> +
>> + data->w = data->mode->hdisplay;
>> + data->h = data->mode->vdisplay;
>> +}
>> +
>> +static bool igt_crtc_is_free(igt_display_t *display, igt_crtc_t *crtc)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < display->n_outputs; i++)
>> + if (display->outputs[i].pending_crtc == crtc)
>> + return false;
>> +
>> + return true;
>> +}
>> +
>> +/* Returns true if an output supports max bpc property. */
>> +static bool has_max_bpc(igt_output_t *output)
>> +{
>> + return igt_output_has_prop(output, IGT_CONNECTOR_MAX_BPC) &&
>> + igt_output_get_prop(output, IGT_CONNECTOR_MAX_BPC);
>> +}
>> +
>> +
>> +
>> +/* Sets the HDR output metadata prop. */
>> +static void set_hdr_output_metadata(data_t *data,
>> + struct hdr_output_metadata const *meta)
>> +{
>> + igt_output_replace_prop_blob(data->output,
>> + IGT_CONNECTOR_HDR_OUTPUT_METADATA, meta,
>> + meta ? sizeof(*meta) : 0);
>> +}
>> +
>> +
>> +
>> +static void test_static_toggle(data_t *data, igt_output_t *output,
>> + uint32_t flags)
>> +{
>> + igt_display_t *display = &data->display;
>> + struct hdr_output_metadata hdr;
>> + igt_crc_t ref_crc, new_crc;
>> + igt_crtc_t *crtc;
>> + igt_fb_t afb;
>> + int afb_id;
>> +
>> + for_each_crtc(display, crtc) {
>> + if (!igt_crtc_connector_valid(crtc, output))
>> + continue;
>> +
>> + if (!igt_crtc_is_free(display, crtc))
>> + continue;
>> +
>> + prepare_test(data, output, crtc->pipe);
>> +
>> + /* 10-bit formats are slow, so limit the size. */
>> + afb_id = igt_create_fb(data->fd, 512, 512, DRM_FORMAT_XRGB2101010, 0, &afb);
>> + igt_assert(afb_id);
>> +
>> + draw_hdr_pattern(&afb);
>> +
>> + igt_hdr_fill_output_metadata_st2084(&hdr);
>> +
>> + /* Start with no metadata. */
>> + igt_plane_set_fb(data->primary, &afb);
>> + igt_plane_set_size(data->primary, data->w, data->h);
>> + 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);
>> +
>> + /* Apply HDR metadata and 10bpc. We expect a modeset for entering. */
>> + 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);
>> + if (is_amdgpu_device(data->fd))
>> + assert_output_bpc(data, 10);
>> +
>> + /* Verify that the CRC are equal after DPMS or suspend. */
>> + igt_pipe_crc_collect_crc(data->pipe_crc, &ref_crc);
>> + test_cycle_flags(data, flags);
>> + igt_pipe_crc_collect_crc(data->pipe_crc, &new_crc);
>> +
>> + /* Disable HDR metadata and drop back to 8bpc. We expect a modeset for exiting. */
>> + 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);
>> +
>> + igt_assert_crc_equal(&ref_crc, &new_crc);
>> +
>> + test_fini(data);
>> + igt_remove_fb(data->fd, &afb);
>> +
>> + break;
>> + }
>> +}
>> +
>> +static void test_static_swap(data_t *data, igt_output_t *output,
>> + void (*fill_metadata)(struct hdr_output_metadata *),
>> + const char *mode_name)
>> +{
>> + igt_display_t *display = &data->display;
>> + igt_crtc_t *crtc;
>> + igt_fb_t afb;
>> + int afb_id;
>> + struct hdr_output_metadata hdr;
>> +
>> + for_each_crtc(display, crtc) {
>> + if (!igt_crtc_connector_valid(crtc, output))
>> + continue;
>> +
>> + if (!igt_crtc_is_free(display, crtc))
>> + continue;
>> +
>> + prepare_test(data, output, crtc->pipe);
>> +
>> + /* 10-bit formats are slow, so limit the size. */
>> + afb_id = igt_create_fb(data->fd, 512, 512, DRM_FORMAT_XRGB2101010, 0, &afb);
>> + igt_assert(afb_id);
>> +
>> + draw_hdr_pattern(&afb);
>> +
>> + /* Start in the specified HDR mode. */
>> + igt_plane_set_fb(data->primary, &afb);
>> + igt_plane_set_size(data->primary, data->w, data->h);
>> + fill_metadata(&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_info("wait %s!\n", mode_name);
>> + igt_debug_wait_for_keypress(mode_name);
>> +
>> + /* Exit HDR mode 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);
>> +
>> + 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)
>> +{
>> + return igt_output_has_prop(output, IGT_CONNECTOR_HDR_OUTPUT_METADATA);
>> +}
>> +
>> +static void test_hdr(data_t *data, const char *test_name, uint32_t flags,
>> + void (*fill_metadata)(struct hdr_output_metadata *),
>> + const char *mode_name)
>> +{
>> + igt_output_t *output;
>> + int valid_tests = 0;
>> +
>> + for_each_connected_output(&data->display, output) {
>> + /* To test HDR, 10 bpc is required, so we need to
>> + * set MAX_BPC property to 10bpc prior to setting
>> + * HDR metadata property. Therefore, checking.
>> + */
>> + if (!has_max_bpc(output) || !has_hdr(output)) {
>> + igt_info("%s connector not found with HDR metadata/max_bpc connector property\n", output->name);
>> + continue;
>> + }
>> +
>> + if (!igt_hdr_is_panel_hdr(data->fd, output->id)) {
>> + igt_info("Panel attached via %s connector is non-HDR\n", output->name);
>> + continue;
>> + }
>> +
>> + 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_SWAP)
>> + test_static_swap(data, output, fill_metadata, mode_name);
>> +
>> + valid_tests++;
>> + }
>> +
>> + igt_require_f(valid_tests, "No connector found with HDR metadata/max_bpc connector property (or) panel is non-HDR\n");
>> +}
>> +
The file uses igt_main(), igt_fixture(), igt_subtest(), IGT_TEST_DESCRIPTION(),
and igt_assert*() -- this is a test, not a tool. IGT tools (e.g., intel_gpu_top,
amd_hdmi_compliance) typically have a regular main() with argument parsing. If
this is intentionally a test that requires manual visual confirmation, it should
go in tests/ (perhaps tests/amdgpu/), not tools/. If it must be a tool, decouple
it from the test framework.

Alternatively, if the intent is to keep the igt_main() pattern (as
amd_hdmi_compliance.c does), then IGT_TEST_DESCRIPTION() is acceptable but the
patch description should clarify this is a manual-verification test binary placed
in tools/ because it requires human interaction and shouldn't run in CI.
>> +int igt_main()
>> +{
>> + data_t data = { 0 };
>> +
>> + igt_fixture() {
>> + data.fd = drm_open_driver_master(DRIVER_AMDGPU);
>> +
>> + kmstest_set_vt_graphics_mode();
>> +
>> + igt_display_require(&data.display, data.fd);
>> + igt_require(data.display.is_atomic);
>> +
>> + igt_display_require_output(&data.display);
>> + }
>> +
>> + igt_describe("Tests swapping to SMPTE ST2084 HDR metadata");
>> + igt_subtest("static-swap-smpte2084")
>> + test_hdr(&data, "static-swap-smpte2084", TEST_SWAP,
>> + igt_hdr_fill_output_metadata_st2084, "smpte2084");
>> +
>> + igt_describe("Tests swapping to traditional SDR gamma HDR metadata");
>> + igt_subtest("static-swap-traditional-sdr")
>> + test_hdr(&data, "static-swap-traditional-sdr", TEST_SWAP,
>> + igt_hdr_fill_output_metadata_sdr, "traditional-sdr");
>> +
>> + igt_fixture() {
>> + igt_display_fini(&data.display);
>> + }
>> +}
>> diff --git a/tools/meson.build b/tools/meson.build
>> index 8185ba160..9612acd83 100644
>> --- a/tools/meson.build
>> +++ b/tools/meson.build
>> @@ -104,6 +104,11 @@ executable('amd_hdmi_compliance', 'amd_hdmi_compliance.c',
>> install_rpath : bindir_rpathdir,
>> install : true)
>>
>> +executable('amd_hdr_visual', 'amd_hdr_visual.c',
>> + dependencies : [tool_deps],
>> + install_rpath : bindir_rpathdir,
>> + install : true)
>> +
>> if libudev.found()
>> msm_dp_compliance_src = [
>> 'msm_dp_compliance.c',
>> --
>> 2.43.0
>>