[PATCH i-g-t 2/2] tests/kms_vrr: Add new test to validate LOBF
B, Jeevan
jeevan.b at intel.com
Wed Jul 31 09:22:22 UTC 2024
> -----Original Message-----
> From: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>
> Sent: Tuesday, July 30, 2024 6:14 PM
> To: B, Jeevan <jeevan.b at intel.com>; igt-dev at lists.freedesktop.org
> Cc: Joshi, Kunal1 <kunal1.joshi at intel.com>; Manna, Animesh
> <animesh.manna at intel.com>
> Subject: Re: [PATCH i-g-t 2/2] tests/kms_vrr: Add new test to validate LOBF
>
> 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?
Well we want to run the test for 4 secs and change the modeset at 3 so that in remaining half second we can expect the LOBF active status.
>
> > + 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().
Got it.
>
> > +
> > + 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;
> }
>
Sure will do this.
> > + 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.
Ok
>
> > +
> > + 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)?
>
Will see if can adopt that.
> 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 at 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].
>
ok
> [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