[PATCH i-g-t 2/2] tests/kms_vrr: Add new test to validate LOBF
Modem, Bhanuprakash
bhanuprakash.modem at intel.com
Tue Jul 30 12:44:06 UTC 2024
Hi Jeevan,
On 30-07-2024 02:01 pm, Jeevan B wrote:
> Validate refresh rate changes that appear to be stable but actually
> change slightly in the VRR using the fixed refresh rate framework
> for non-PSR scenarios.
>
> v2: Add PR check.
> v3: Update commit message and fix code structure.
>
> Signed-off-by: Jeevan B <jeevan.b at intel.com>
> ---
> tests/kms_vrr.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c
> index 7e8885f16..88855b01d 100644
> --- a/tests/kms_vrr.c
> +++ b/tests/kms_vrr.c
> @@ -31,6 +31,7 @@
> */
>
> #include "igt.h"
> +#include "igt_psr.h"
> #include "i915/intel_drrs.h"
> #include "sw_sync.h"
> #include <fcntl.h>
> @@ -76,6 +77,11 @@
> * without a full modeset.
> * Functionality: LRR
> *
> + * SUBTEST: lobf
> + * Description: Test to validate link-off between active frames in non-psr
> + * operation
> + * Functionality: LOBF
> + *
> * SUBTEST: max-min
> * Description: Oscillates between highest and lowest refresh each frame for
> * manual flicker profiling
> @@ -106,7 +112,8 @@ enum {
> TEST_FASTSET = 1 << 7,
> TEST_MAXMIN = 1 << 8,
> TEST_CMRR = 1 << 9,
> - TEST_NEGATIVE = 1 << 10,
> + TEST_LINK_OFF = 1 << 10,
> + TEST_NEGATIVE = 1 << 11,
> };
>
> enum {
> @@ -129,6 +136,7 @@ typedef struct vtest_ns {
> typedef struct data {
> igt_display_t display;
> int drm_fd;
> + int debugfs_fd;
> igt_plane_t *primary;
> igt_fb_t fb[2];
> range_t range;
> @@ -784,6 +792,45 @@ test_seamless_virtual_rr_basic(data_t *data, enum pipe pipe, igt_output_t *outpu
> }
> }
>
> +static void
> +test_lobf(data_t *data, enum pipe pipe, igt_output_t *output, uint32_t flags)
> +{
> + bool change_mode = false, flip = true;
> + time_t startTime = time(NULL);
> + time_t secs = 4, change_time = 3;
----------------------^----------------^
What is the funda behind these magic numbers?
> + igt_plane_t *primary;
> + drmModeModeInfo mode = *igt_output_get_mode(output);
> +
> + data->debugfs_fd = igt_debugfs_dir(data->drm_fd);
Please move this initialization to output_constraint().
> +
> + igt_info("LOBF test execution on %s, PIPE %s with VRR range: (%u-%u) Hz\n",
> + output->name, kmstest_pipe_name(pipe), data->range.min, data->range.max);
> +
> + igt_require(!igt_get_i915_edp_lobf_status(data->drm_fd, output->name));
I would recommend to move this check to output_constraint();
Ex:
if ((flags & TEST_LINK_OFF) &&
igt_get_i915_edp_lobf_status()) {
igt_info("Please add your skip message");
return false;
}
> + primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> + igt_create_color_fb(data->drm_fd, mode.hdisplay, mode.vdisplay,
> + DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> + 1, 1, 1, &data->fb[0]);
> + igt_create_color_fb(data->drm_fd, mode.hdisplay, mode.vdisplay,
> + DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> + 1, 1, 0, &data->fb[1]);
Please re-use prepare_test() to create framebuffers.
> +
> + igt_output_override_mode(output, &data->switch_modes[HIGH_RR_MODE]);
> +
> + while (time(NULL) - startTime < secs) {
> + flip = !flip;
> + igt_plane_set_fb(primary, flip ? &data->fb[0] : &data->fb[1]);
Instead of this loop, can't we use flip_and_measure(3 sec)?
Ex:
prepare_test();
flip_and_measure(3 sec);
igt_output_override_mode(low_rr_mode);
flip_and_measure(1 sec);
igt_assert_f(igt_get_i915_edp_lobf_status("LOBF not enabled"));
> + igt_display_commit(&data->display);
> + if (!change_mode && time(NULL) - startTime >= change_time) {
> + igt_output_override_mode(output, &data->switch_modes[LOW_RR_MODE]);
What is the expectation if there is no LOW_RR_MODE available? I can see,
there is an VRR panel available in CI which is having single mode [2].
Is this test not applicable on this panel?
[2]:
https://intel-gfx-ci.01.org/tree/intel-xe/xe-1689-7d7c6494ae7dcfb15998b64387ba66b49da16286/bat-lnl-1/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-xr24.html
> + change_mode = true;
> + }
> + }
> +
> + igt_assert_f(igt_get_i915_edp_lobf_status(data->drm_fd, output->name),
> + "LOBF not enabled\n");
> +}
> +
> static void
> test_cmrr(data_t *data, enum pipe pipe, igt_output_t *output, uint32_t flags)
> {
> @@ -851,6 +898,15 @@ static bool output_constraint(data_t *data, igt_output_t *output, uint32_t flags
> !intel_output_has_drrs(data->drm_fd, output)) {
> igt_info("Selected panel won't support DRRS.\n");
> return false;
> +
> + if ((flags & TEST_LINK_OFF) &&
> + output->config.connector->connector_type != DRM_MODE_CONNECTOR_eDP)
Instead of writing a new condition, why can't we extend the existing
logic [1].
[1]:
https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/kms_vrr.c#L846
> + return false;
> +
> + if ((flags & TEST_LINK_OFF) &&
> + (psr_sink_support(data->drm_fd, data->debugfs_fd, PSR_MODE_1, NULL) ||
> + psr_sink_support(data->drm_fd, data->debugfs_fd, PR_MODE, NULL)))
data->debugfs is not initialized, hence this check will never pass.
> + psr_disable(data->drm_fd, data->debugfs_fd, NULL);
> }
>
> /* Reset output */
> @@ -1030,13 +1086,20 @@ igt_main_args("drs:", long_opts, help_str, opt_handler, &data)
> igt_subtest_with_dynamic("seamless-rr-switch-virtual")
> run_vrr_test(&data, test_seamless_virtual_rr_basic, TEST_SEAMLESS_VIRTUAL_RR);
>
> - igt_describe("Test to validate the the content rate exactly match with the "
> + igt_describe("Test to validate the content rate exactly match with the "
This change is not related to this patch.
> "requested rate without any frame drops.");
> igt_subtest_with_dynamic("cmrr") {
> igt_require(intel_display_ver(intel_get_drm_devid(data.drm_fd)) >= 20);
>
> run_vrr_test(&data, test_cmrr, TEST_CMRR);
> }
Please add one new line here.
- Bhanu
> + igt_describe("Test to validate the link-off between active frames in "
> + "non-PSR operation.");
> + igt_subtest_with_dynamic("lobf") {
> + igt_require(intel_display_ver(intel_get_drm_devid(data.drm_fd)) >= 20);
> +
> + run_vrr_test(&data, test_lobf, TEST_LINK_OFF);
> + }
> }
>
> igt_fixture {
More information about the igt-dev
mailing list