From: Karthik B S <karthik.b.s@intel.com>
To: Pranay Samala <pranay.samala@intel.com>, <igt-dev@lists.freedesktop.org>
Cc: <swati2.sharma@intel.com>, <sameer.lattannavar@intel.com>
Subject: Re: [PATCH i-g-t v6 1/3] Revert "tests/kms_hdr: Move capability checks inside dynamic subtests"
Date: Thu, 18 Jun 2026 13:34:07 +0530 [thread overview]
Message-ID: <a63d0993-e834-4f9e-91fe-a0a2d4ec4694@intel.com> (raw)
In-Reply-To: <20260618063347.2940031-2-pranay.samala@intel.com>
On 6/18/2026 12:03 PM, Pranay Samala wrote:
> This reverts commit d9667a6fd6468adcdb5542d741ac423339a20556.
>
> The previous change moved output capability checks inside dynamic
> subtests to avoid losing valid results when a later output failed a
> check. However, the real issue is in the helper behavior rather
> than the kms_hdr test flow itself.
>
> Move the checks back outside the dynamic subtests and restore the
> previous behavior. The issue will instead be handled at the
> helper/API level by updating igt_get_output_max_bpc() to return
> status, allowing callers to handle read/parse failures explicitly
> without changing the existing kms_hdr control flow.
>
> v2:
> - Update commit message (Karthik)
>
> v3:
> - Add the missing changes merged recently (Swati)
>
> v5:
> - Ensure remaining formats tested that might succeed (Swati)
> - Update if condition for clarity (Swati)
>
> v6:
> - Make v5 changes as separate patch (Karthik)
>
> Signed-off-by: Pranay Samala <pranay.samala@intel.com>
> Reviewed-by: Swati Sharma <swati2.sharma@intel.com>
Reviewed-by: Karthik B S <karthik.b.s@intel.com>
> ---
> tests/kms_hdr.c | 175 +++++++++++++++++++++++++++---------------------
> 1 file changed, 99 insertions(+), 76 deletions(-)
>
> diff --git a/tests/kms_hdr.c b/tests/kms_hdr.c
> index affd38887..7127a56b3 100644
> --- a/tests/kms_hdr.c
> +++ b/tests/kms_hdr.c
> @@ -255,6 +255,17 @@ static void test_bpc_switch(data_t *data, uint32_t flags)
> for_each_connected_output(display, output) {
> igt_crtc_t *crtc;
>
> + if (!has_max_bpc(output)) {
> + igt_info("%s: Doesn't support IGT_CONNECTOR_MAX_BPC.\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);
> @@ -266,29 +277,24 @@ static void test_bpc_switch(data_t *data, uint32_t flags)
> for (int i = 0; i < ARRAY_SIZE(hdr_test_formats); i++) {
> prepare_test(data, output, crtc);
>
> + if (is_intel_device(data->fd) &&
> + !igt_max_bpc_constraint(display, crtc, output, 10)) {
> + igt_info("%s: No suitable mode found to use 10 bpc.\n",
> + igt_output_name(output));
> +
> + test_fini(data);
> + break;
> + }
> +
> data->mode = igt_output_get_mode(output);
> data->w = data->mode->hdisplay;
> data->h = data->mode->vdisplay;
>
> igt_dynamic_f("pipe-%s-%s-%s",
> igt_crtc_name(crtc), output->name,
> - igt_format_str(hdr_test_formats[i])) {
> - igt_require_f(has_max_bpc(output),
> - "%s: Doesn't support IGT_CONNECTOR_MAX_BPC.\n",
> - igt_output_name(output));
> -
> - igt_require_f(igt_get_output_max_bpc(output) >= 10,
> - "%s: Doesn't support 10 bpc.\n",
> - igt_output_name(output));
> -
> - igt_require_f(!is_intel_device(data->fd) ||
> - igt_max_bpc_constraint(display, crtc, output, 10),
> - "%s: No suitable mode found to use 10 bpc.\n",
> - igt_output_name(output));
> -
> + igt_format_str(hdr_test_formats[i]))
> test_bpc_switch_on_output(data, crtc, output,
> hdr_test_formats[i], flags);
> - }
>
> test_fini(data);
> }
> @@ -530,6 +536,40 @@ static void test_hdr(data_t *data, uint32_t flags)
> for_each_connected_output(display, output) {
> igt_crtc_t *crtc;
>
> + /* 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) || !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;
> + }
> +
> + /* For negative test, panel should be non-hdr. */
> + if ((flags & TEST_INVALID_HDR) && igt_is_panel_hdr(data->fd, output)) {
> + igt_info("%s: Can't run negative test on HDR panel.\n",
> + igt_output_name(output));
> + continue;
> + }
> +
> + if ((flags & ~TEST_INVALID_HDR) && !igt_is_panel_hdr(data->fd, output)) {
> + igt_info("%s: Can't run HDR tests on non-HDR panel.\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;
> + }
> +
> + if ((flags & TEST_BRIGHTNESS) && !output_is_internal_panel(output)) {
> + igt_info("%s: Can't run brightness test on non-internal panel.\n",
> + igt_output_name(output));
> + continue;
> + }
> +
> for_each_crtc(display, crtc) {
> igt_output_set_crtc(output, crtc);
> if (!intel_pipe_output_combo_valid(display)) {
> @@ -545,73 +585,56 @@ static void test_hdr(data_t *data, uint32_t flags)
> data->w = data->mode->hdisplay;
> data->h = data->mode->vdisplay;
>
> - igt_dynamic_f("pipe-%s-%s-%s", igt_crtc_name(crtc), output->name,
> - igt_format_str(hdr_test_formats[i])) {
> - igt_require_f(has_max_bpc(output) &&
> - igt_output_supports_hdr(output),
> - "%s: Doesn't support IGT_CONNECTOR_MAX_BPC "
> - "or IGT_CONNECTOR_HDR_OUTPUT_METADATA.\n",
> - igt_output_name(output));
> -
> - if (flags & TEST_INVALID_HDR)
> - igt_require_f(!igt_is_panel_hdr(data->fd, output),
> - "%s: Can't run negative test on "
> - "HDR panel.\n",
> - igt_output_name(output));
> -
> - if (!(flags & TEST_INVALID_HDR))
> - igt_require_f(igt_is_panel_hdr(data->fd, output),
> - "%s: Can't run HDR tests on "
> - "non-HDR panel.\n",
> - igt_output_name(output));
> -
> - igt_require_f(igt_get_output_max_bpc(output) >= 10,
> - "%s: Doesn't support 10 bpc.\n",
> - igt_output_name(output));
> -
> - if (flags & TEST_BRIGHTNESS)
> - igt_require_f(output_is_internal_panel(output),
> - "%s: Can't run brightness test on "
> - "non-internal panel.\n",
> - igt_output_name(output));
> -
> - /* Signal HDR requirement via metadata.
> - * A framebuffer must be present for the driver to accept
> - * a metadata commit. Use TEST_ONLY so hardware state is
> - * unchanged.
> - */
> - igt_create_fb(data->fd,
> - data->w, data->h,
> - hdr_test_formats[i],
> - DRM_FORMAT_MOD_LINEAR,
> - &data->afb);
> - igt_plane_set_fb(data->primary, &data->afb);
> - igt_plane_set_size(data->primary, data->w, data->h);
> - igt_output_set_prop_value(data->output, IGT_CONNECTOR_MAX_BPC, 10);
> - igt_hdr_fill_st2084(&hdr);
> - igt_hdr_set_metadata(data->output, &hdr);
> - igt_require_f(!igt_display_try_commit_atomic(display,
> - DRM_MODE_ATOMIC_TEST_ONLY |
> - DRM_MODE_ATOMIC_ALLOW_MODESET,
> - NULL),
> - "%s: Couldn't set HDR metadata\n",
> - igt_output_name(output));
> -
> - /* Reset IGT display state; hardware was not changed. */
> + /* Signal HDR requirement via metadata.
> + * A framebuffer must be present for the driver to accept
> + * a metadata commit. Use TEST_ONLY so hardware state is
> + * unchanged.
> + */
> + igt_create_fb(data->fd,
> + data->w, data->h,
> + hdr_test_formats[i],
> + DRM_FORMAT_MOD_LINEAR,
> + &data->afb);
> + igt_plane_set_fb(data->primary, &data->afb);
> + igt_plane_set_size(data->primary, data->w, data->h);
> + igt_output_set_prop_value(data->output, IGT_CONNECTOR_MAX_BPC, 10);
> + igt_hdr_fill_st2084(&hdr);
> + igt_hdr_set_metadata(data->output, &hdr);
> + if (igt_display_try_commit_atomic(display,
> + DRM_MODE_ATOMIC_TEST_ONLY |
> + DRM_MODE_ATOMIC_ALLOW_MODESET,
> + NULL)) {
> + igt_info("%s: Couldn't set HDR metadata\n",
> + igt_output_name(output));
> igt_hdr_set_metadata(data->output, NULL);
> igt_plane_set_fb(data->primary, NULL);
> igt_remove_fb(data->fd, &data->afb);
> + test_fini(data);
> + break;
> + }
> +
> + /* Reset IGT display state; hardware was not changed. */
> + igt_hdr_set_metadata(data->output, NULL);
> + igt_plane_set_fb(data->primary, NULL);
> + igt_remove_fb(data->fd, &data->afb);
>
> - igt_require_f(!is_intel_device(data->fd) ||
> - igt_max_bpc_constraint(display, crtc, output, 10),
> - "%s: No suitable mode found to use 10 bpc.\n",
> - igt_output_name(output));
> + if (is_intel_device(data->fd) &&
> + !igt_max_bpc_constraint(display, crtc, output, 10)) {
> + igt_info("%s: No suitable mode found to use 10 bpc.\n",
> + igt_output_name(output));
> +
> + test_fini(data);
> + break;
> + }
>
> - if (igt_is_dsc_enabled(data->fd, output->name))
> - flags |= TEST_NEEDS_DSC;
> - else
> - flags &= ~TEST_NEEDS_DSC;
> + if (igt_is_dsc_enabled(data->fd, output->name))
> + flags |= TEST_NEEDS_DSC;
> + else
> + flags &= ~TEST_NEEDS_DSC;
>
> + igt_dynamic_f("pipe-%s-%s-%s",
> + igt_crtc_name(crtc), output->name,
> + igt_format_str(hdr_test_formats[i])) {
> if (flags & (TEST_NONE | TEST_DPMS | TEST_SUSPEND |
> TEST_INVALID_HDR | TEST_BRIGHTNESS))
> test_static_toggle(data,
next prev parent reply other threads:[~2026-06-18 8:04 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-06-18 6:33 [PATCH i-g-t v6 0/3] Improve max-bpc query error handling Pranay Samala
2026-06-18 6:33 ` [PATCH i-g-t v6 1/3] Revert "tests/kms_hdr: Move capability checks inside dynamic subtests" Pranay Samala
2026-06-18 8:04 ` Karthik B S [this message]
2026-06-18 6:33 ` [PATCH i-g-t v6 2/3] tests/kms_hdr: Simplify flag check and continue with next format on failure Pranay Samala
2026-06-18 8:31 ` Karthik B S
2026-06-18 6:33 ` [PATCH i-g-t v6 3/3] lib/tests: make igt_get_output_max_bpc() return status and handle failures in callers Pranay Samala
2026-06-18 7:56 ` ✓ Xe.CI.BAT: success for Improve max-bpc query error handling Patchwork
2026-06-18 8:02 ` ✗ i915.CI.BAT: failure " Patchwork
2026-06-18 15:37 ` ✗ Xe.CI.FULL: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a63d0993-e834-4f9e-91fe-a0a2d4ec4694@intel.com \
--to=karthik.b.s@intel.com \
--cc=igt-dev@lists.freedesktop.org \
--cc=pranay.samala@intel.com \
--cc=sameer.lattannavar@intel.com \
--cc=swati2.sharma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox