Re: [PATCH i-g-t][V3] tests/kms_hdr: Add visual HDR verification subtests

[Alex Hung <alex.hung@amd.com>]

On 5/12/26 13:18, Sharma, Swati2 wrote:
> Hi Alex,
> 
> On 12-05-2026 06:06 am, Alex Hung wrote:
>> Hi,
>>
>> Does anyone have any concerns if I merge this patch?
>>
>> On 5/7/26 15:13, Alex Hung wrote:
>>> From: Wayne Lin <wayne.lin@amd.com>
>>>
>>> The new subtests display an HDR test pattern under a given metadata
>>> mode and pause for user confirmation, enabling manual inspection of
>>> HDR output quality on a connected HDR panel.
>>>
>>> New subtests:
>>> kms_hdr --run-subtest static-swap-smpte2084 --interactive- 
>>> debug=smpte2084
>>> kms_hdr --run-subtest static-swap-traditional-sdr --interactive- 
>>> debug=traditional-sdr
>>>
>>> Co-developed-by: Alex Hung <alex.hung@amd.com>
>>> Signed-off-by: Alex Hung <alex.hung@amd.com>
> Is Co-developed redundant?

Thanks for catching this.

>>> Signed-off-by: Wayne Lin <wayne.lin@amd.com>
>>> Assisted-by: Copilot:Claude-Sonnet-4.6
>>> ---
>>>   tests/kms_hdr.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 89 insertions(+)
>>>
>>> diff --git a/tests/kms_hdr.c b/tests/kms_hdr.c
>>> index 876c1c03a..c13e985e1 100644
>>> --- a/tests/kms_hdr.c
>>> +++ b/tests/kms_hdr.c
>>> @@ -67,6 +67,14 @@
>>>    *
>>>    * @swap:                    swapping static HDR metadata
>>>    * @toggle:                  entering and exiting HDR mode
>>> + *
>>> + * SUBTEST: static-swap-smpte2084
>>> + * Description: Show a visual HDR pattern with SMPTE ST 2084 
>>> metadata and
>>> + * require user confirmation.
>>> + *
>>> + * SUBTEST: static-swap-traditional-sdr
>>> + * Description: Show a visual HDR pattern with traditional SDR gamma 
>>> metadata
>>> + * and require user confirmation.
>>>    */
>>>
>>>   IGT_TEST_DESCRIPTION("Test HDR metadata interfaces and bpc switch");
>>> @@ -647,6 +655,79 @@ static void test_hdr(data_t *data, uint32_t flags)
>>>       }
>>>   }
>>>
>>> +static void test_hdr_visual(data_t *data,
>>> +                 void (*fill_metadata)(struct hdr_output_metadata *),
>>> +                 const char *mode_name) +{
>>> +    igt_display_t *display = &data->display;
>>> +    igt_output_t *output;
>>> +    igt_fb_t afb;
>>> +    int afb_id;
>>> +    bool found = false;
>>> +    struct hdr_output_metadata hdr;
>>> +
>>> +    igt_display_reset(display);
>>> +
>>> +    for_each_connected_output(display, output) {
>>> +        igt_crtc_t *crtc;
>>> +
>>> +        if (!has_max_bpc(output) || !igt_output_supports_hdr(output)) {
>>> +            igt_info("%s: Doesn't support IGT_CONNECTOR_MAX_BPC or 
>>> IGT_CONNECTOR_HDR_OUTPUT_METADATA.\n",
>>> +                 igt_output_name(output));
>>> +            continue;
>>> +        }
>>> +
>>> +        if (!igt_is_panel_hdr(data->fd, output)) {
>>> +            igt_info("Panel attached via %s connector is non-HDR\n", 
>>> igt_output_name(output));
>>> +            continue;
>>> +        }
>>> +
>>> +        if (igt_get_output_max_bpc(output) < 10) {
>>> +            igt_info("%s: Doesn't support 10 bpc.\n", 
>>> igt_output_name(output));
>>> +            continue;
>>> +        }
>>> +
>>> +        for_each_crtc(display, crtc) {
>>> +            igt_output_set_crtc(output, crtc);
>>> +            if (!igt_crtc_connector_valid(crtc, output))
>>> +                continue;
>>> +
>>> +            prepare_test(data, output, crtc);
>>> +
>>> +            afb_id = igt_create_fb(data->fd, 512, 512,
>>> +                           DRM_FORMAT_XRGB2101010,
>>> +                           DRM_FORMAT_MOD_LINEAR, &afb);
>>> +            igt_assert(afb_id);
>>> +
>>> +            draw_hdr_pattern(&afb);
>>> +
>>> +            igt_plane_set_fb(data->primary, &afb);
>>> +            igt_plane_set_size(data->primary, data->w, data->h);
>>> +            fill_metadata(&hdr);
>>> +            igt_hdr_set_metadata(data->output, &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("Displaying %s HDR pattern. Press a key to 
>>> continue.\n", mode_name);
>>> +            igt_debug_wait_for_keypress(mode_name);
>>> +
>>> +            igt_hdr_set_metadata(data->output, 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);
>>> +
>>> +            found = true;
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    igt_require_f(found, "No HDR-capable connector found.\n");
>>> +}
>>> +
>>>   int igt_main()
>>>   {
>>>       data_t data = {};
>>> @@ -698,6 +779,14 @@ int igt_main()
>>>       igt_subtest_with_dynamic("invalid-hdr")
>>>           test_hdr(&data, TEST_INVALID_HDR);
>>>
>>> +    igt_describe("Show HDR pattern with SMPTE ST 2084 metadata and 
>>> require user confirmation");
>>> +    igt_subtest("static-swap-smpte2084")
>>> +        test_hdr_visual(&data, igt_hdr_fill_st2084, "smpte2084");
>>> +
>>> +    igt_describe("Show HDR pattern with traditional SDR gamma 
>>> metadata and require user confirmation");
>>> +    igt_subtest("static-swap-traditional-sdr")
>>> +        test_hdr_visual(&data, igt_hdr_fill_sdr, "traditional-sdr");
> igt_debug_wait_for_keypress() is a no-op in CI — these subtests just 
> duplicate static-swap with no added validation.
> Instead, add igt_debug_interactive_mode_check() calls inside the 
> existing test_static_swap() after each metadata commit. This is CI-safe 
> (no-op without terminal) and provides Y/n visual verdict when run with 
> --interactive-debug=all. See tests/intel/kms_dsc.c:90-92,216 for 
> reference. No new subtests or functions needed.

Thanks. Let me change it and then send V4.

>>> +
>>>       igt_fixture() {
>>>           igt_display_fini(&data.display);
>>>           drm_close_driver(data.fd);
>>> -- 
>>> 2.43.0
>>>
>>