[PATCH i-g-t 2/2] tests/kms_vrr: Add new test to validate LOBF

[Modem, Bhanuprakash]

Hi Jeevan,

On 31-07-2024 05:32 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.
> v4: update logic using existing code.
> 
> Signed-off-by: Jeevan B <jeevan.b at intel.com>
> ---
>   tests/kms_vrr.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c
> index 7e8885f16..851b8bbee 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,27 @@ 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)
> +{
> +	uint64_t rate[] = {0};
> +
> +	rate[0] = rate_from_refresh(data->switch_modes[HIGH_RR_MODE].vrefresh);
> +	prepare_test(data, output, pipe);
> +	data->debugfs_fd = igt_debugfs_dir(data->drm_fd);

Please drop this redundant initialization as we already initialized in 
output_constgraint().

> +
> +	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_output_override_mode(output, &data->switch_modes[HIGH_RR_MODE]);
> +	flip_and_measure(data, output, pipe, rate, 1, TEST_DURATION_NS);
> +	igt_output_override_mode(output, &data->switch_modes[LOW_RR_MODE]);
> +	rate[0] = rate_from_refresh(data->switch_modes[LOW_RR_MODE].vrefresh);
> +	flip_and_measure(data, output, pipe, rate, 1, NSECS_PER_SEC);
> +	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)
>   {
> @@ -843,6 +872,8 @@ static void test_cleanup(data_t *data, enum pipe pipe, igt_output_t *output)
>   
>   static bool output_constraint(data_t *data, igt_output_t *output, uint32_t flags)
>   {
> +	data->debugfs_fd = igt_debugfs_dir(data->drm_fd);

Don't we need to close this fd on cleanup?

> +
>   	if ((flags & (TEST_SEAMLESS_VRR | TEST_SEAMLESS_DRRS | TEST_CMRR)) &&

As mentioned in previous revisions, please extend this check by oring 
with TEST_LINK_OFF.

>   	    output->config.connector->connector_type != DRM_MODE_CONNECTOR_eDP)
>   		return false;
> @@ -852,6 +883,20 @@ static bool output_constraint(data_t *data, igt_output_t *output, uint32_t flags
>   		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)
> +		return false;

Please read above comment.

> +
> +	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)))
> +		psr_disable(data->drm_fd, data->debugfs_fd, NULL);
> +
> +	if ((flags & TEST_LINK_OFF) &&
> +		igt_get_i915_edp_lobf_status(data->drm_fd, output->name)) {
> +		igt_info("LOBF not supported. \n");
> +		return false;
> +	}

Please combine these two conditions.

Ex:
	if (flags & TEST_LINK_OFF) {
		if (psr check)
			psr_disable();

		if (lobf status)
			return false;
	}

>   
>   	/* Reset output */
>   	igt_display_reset(&data->display);
> @@ -1030,13 +1075,21 @@ 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 "

As mentioned in previous revisions, this change is not related to this 
patch. Please drop this.

- Bhanu

>   			     "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);
>   		}
> +
> +		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 {