Re: [igt-dev] [i-g-t 3/5] tests/kms_vrr: Add Negative tests to validate VRR

["Navare, Manasi" <manasi.d.navare@intel.com>]

On Fri, Mar 04, 2022 at 09:32:29AM +0530, Modem, Bhanuprakash wrote:
> On Fri-04-03-2022 06:50 am, Navare, Manasi wrote:
> >On Thu, Feb 24, 2022 at 10:46:46AM +0530, Bhanuprakash Modem wrote:
> >>This patch will try to enable VRR on Non-VRR panel. VRR should
> >>not be enabled on the Non-VRR panel. Hence Kernel should reject
> >>the commit.
> >>
> >>Cc: Manasi Navare <manasi.d.navare@intel.com>
> >>Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> >>---
> >>  tests/kms_vrr.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 67 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c
> >>index 8976d4a6c3..7deab77311 100644
> >>--- a/tests/kms_vrr.c
> >>+++ b/tests/kms_vrr.c
> >>@@ -45,6 +45,7 @@ enum {
> >>  	TEST_DPMS = 1 << 0,
> >>  	TEST_SUSPEND = 1 << 1,
> >>  	TEST_FLIPLINE = 1 << 2,
> >>+	TEST_NEGATIVE = 1 << 3,
> >>  };
> >>  typedef struct range {
> >>@@ -244,6 +245,34 @@ static void prepare_test(data_t *data, igt_output_t *output, enum pipe pipe)
> >>  	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> >>  }
> >>+static void prepare_negative_test(data_t *data, igt_output_t *output, enum pipe pipe)
> >>+{
> >>+	drmModeModeInfo *mode;
> >>+
> >>+	/* Reset output */
> >>+	igt_display_reset(&data->display);
> >>+	igt_output_set_pipe(output, pipe);
> >>+
> >>+	/* Capture VRR range */
> >>+	data->range = get_vrr_range(data, output);
> >>+
> >>+	/* Prepare resources */
> >>+	mode = igt_output_get_mode(output);
> >>+	igt_assert(igt_create_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> >>+				 DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> >>+				 &data->fb0));
> >>+
> >>+	/* Take care of any required modesetting before the test begins. */
> >>+	data->primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> >>+	igt_plane_set_fb(data->primary, &data->fb0);
> >>+
> >>+	/* Clear vrr_enabled state before enabling it, because
> >>+	 * it might be left enabled if the previous test fails.
> >>+	 */
> >>+	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED, 0);
> >>+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
> >>+}
> >>+
> >>  /* Performs an atomic non-blocking page-flip on a pipe. */
> >>  static void
> >>  do_flip(data_t *data, igt_fb_t *fb)
> >>@@ -430,6 +459,32 @@ test_basic(data_t *data, enum pipe pipe, igt_output_t *output, uint32_t flags)
> >>  	igt_remove_fb(data->drm_fd, &data->fb0);
> >>  }
> >>+/* VRR on Non-VRR panel: VRR should not be enabled on the Non-VRR panel.
> >>+ * Kernel should reject the commit.
> >>+ */
> >>+static void
> >>+test_negative_basic(data_t *data, enum pipe pipe,
> >>+		    igt_output_t *output, uint32_t flags)
> >>+{
> >>+	int ret;
> >>+
> >>+	igt_info("VRR Negative Test execution on %s, PIPE_%s.\n",
> >>+			output->name, kmstest_pipe_name(pipe));
> >>+
> >>+	prepare_negative_test(data, output, pipe);
> >>+	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED, 1);
> >>+
> >>+	ret = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
> >>+	igt_assert_f(ret == 0, "VRR shouln't be enabled on Non-VRR panel.\n");
> >
> >No this is not correct since in the kernel, if its a non VRR panel then it will just
> >do the modeset without enabling VRR so you will just see the visual artifacts but not reject it.
> 
> Hmm, intel_vrr_compute_config() will just return & even don't care about
> "vrr_enabled" if the panel is Non-VRR.
> 
> In general, if we try to commit with invalid config, Kernel must reject
> (with -22 invalid arg) right? Here, we requested to enable VRR, but Kernel
> returned success without enabling it.
> 
> So the flow would be:
> * Set "vrr_enabled" on selected pipe & do modeset
> * Readback the "vrr_enabled" prop to make sure that Kernel enabled the VRR
> on selected pipe.
> 
> Am I missing anything?

Yes so in case of VRR, its an enhancement to improve the visual quality.
But if userspace requests VRR but the panel doesnt support it, the kernel will
not reject the mode since having some screen output without VRR is better than no output with a black screen.

So it will succeed with the modeset.
However like the VRR enable register will not be set for example, so if we absolutely want to add a negative test
we can probe the VRR enable register through debugfs that reads the VRR enable i915 register which in this case
will just be 0.

Manasi

> 
> - Bhanu
> 
> >
> >Manasi
> >
> >>+
> >>+	/* Clean-up */
> >>+	igt_plane_set_fb(data->primary, NULL);
> >>+	igt_output_set_pipe(output, PIPE_NONE);
> >>+	set_vrr_on_pipe(data, pipe, false);
> >>+
> >>+	igt_remove_fb(data->drm_fd, &data->fb0);
> >>+}
> >>+
> >>  /* Runs tests on outputs that are VRR capable. */
> >>  static void
> >>  run_vrr_test(data_t *data, test_t test, uint32_t flags)
> >>@@ -439,8 +494,14 @@ run_vrr_test(data_t *data, test_t test, uint32_t flags)
> >>  	for_each_connected_output(&data->display, output) {
> >>  		enum pipe pipe;
> >>-		if (!has_vrr(output))
> >>-			continue;
> >>+		/* For Negative tests, panel should be non-vrr. */
> >>+		if (flags & TEST_NEGATIVE) {
> >>+			if (has_vrr(output))
> >>+				continue;
> >>+		} else {
> >>+			if (!has_vrr(output))
> >>+				continue;
> >>+		}
> >>  		for_each_pipe(&data->display, pipe) {
> >>  			if (igt_pipe_connector_valid(pipe, output)) {
> >>@@ -486,6 +547,10 @@ igt_main
> >>  	igt_subtest_with_dynamic("flipline")
> >>  		run_vrr_test(&data, test_basic, TEST_FLIPLINE);
> >>+	igt_describe("Make sure that VRR should not be enabled on the Non-VRR panel.");
> >>+	igt_subtest_with_dynamic("negative-basic")
> >>+		run_vrr_test(&data, test_negative_basic, TEST_NEGATIVE);
> >>+
> >>  	igt_fixture {
> >>  		igt_display_fini(&data.display);
> >>  	}
> >>-- 
> >>2.35.0
> >>
>