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

[vitaly prosyak <vprosyak@amd.com>]

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
>>