[igt-dev] [PATCH i-g-t 1/2] tests/kms_invalid_mode.c: Convert tests to dynamic

[Gupta, Nidhi1]

Hi Petri,

Thanks for the review, 

-----Original Message-----
From: Latvala, Petri <petri.latvala at intel.com> 
Sent: Wednesday, June 22, 2022 3:11 PM
To: Gupta, Nidhi1 <nidhi1.gupta at intel.com>
Cc: igt-dev at lists.freedesktop.org; Modem, Bhanuprakash <bhanuprakash.modem at intel.com>
Subject: Re: [PATCH i-g-t 1/2] tests/kms_invalid_mode.c: Convert tests to dynamic

On Mon, Jun 20, 2022 at 02:40:50PM +0530, Nidhi Gupta wrote:
> Convert the existing subtests to dynamic subtests at pipe level.
> 
> Signed-off-by: Nidhi Gupta <nidhi1.gupta at intel.com>
> ---
>  tests/kms_invalid_mode.c | 53 
> +++++++++++++++++-----------------------
>  1 file changed, 23 insertions(+), 30 deletions(-)
> 
> diff --git a/tests/kms_invalid_mode.c b/tests/kms_invalid_mode.c index 
> 630798d8..3d583202 100644
> --- a/tests/kms_invalid_mode.c
> +++ b/tests/kms_invalid_mode.c
> @@ -32,6 +32,7 @@ typedef struct _data data_t;
>  
>  struct _data {
>  	int drm_fd;
> +	enum pipe pipe;
>  	igt_display_t display;
>  	igt_output_t *output;
>  	drmModeResPtr res;
> @@ -177,21 +178,21 @@ adjust_mode_bad_vtotal(data_t *data, drmModeModeInfoPtr mode)
>  	return true;
>  }
>  
> -static int
> +static void
>  test_output(data_t *data)
>  {
>  	igt_output_t *output = data->output;
>  	drmModeModeInfo mode;
>  	struct igt_fb fb;
> -	int i;
> +	int ret;
> +	uint32_t crtc_id;
>  
>  	/*
>  	 * FIXME test every mode we have to be more
>  	 * sure everything is really getting rejected?
>  	 */
>  	mode = *igt_output_get_mode(output);
> -	if (!data->adjust_mode(data, &mode))
> -		return 0;
> +	igt_skip_on(!data->adjust_mode(data, &mode));

This may be a nitpick but prefer fail/skip constructs that don't need negation. Here that would be

igt_require(data->adjust_mode(data, &mode));


>  
>  	igt_create_fb(data->drm_fd,
>  		      max_t(uint16_t, mode.hdisplay, 64),

Previously this fb allocation was done once for all pipes, now it's done once per pipe. What's the effect on total runtime?

Without this patch time taken was 0.001s and with this patch time taken is 0.009s.
Full subtest execution will take 0.009s.
Overall it's a very small test.



> @@ -202,32 +203,14 @@ test_output(data_t *data)
>  
>  	kmstest_unset_all_crtcs(data->drm_fd, data->res);
>  
> -	for (i = 0; i < data->res->count_crtcs; i++) {
> -		int ret;
> -
> -		igt_info("Checking pipe %c connector %s with mode %s\n",
> -			 'A'+i, output->name, mode.name);
> +	crtc_id = data->display.pipes[data->pipe].crtc_id;
>  
> -		ret = drmModeSetCrtc(data->drm_fd, data->res->crtcs[i],
> -				     fb.fb_id, 0, 0,
> -				     &output->id, 1, &mode);
> -		igt_assert_lt(ret, 0);
> -	}
> +	ret = drmModeSetCrtc(data->drm_fd, crtc_id,
> +			     fb.fb_id, 0, 0,
> +			     &output->id, 1, &mode);
> +	igt_assert_lt(ret, 0);
>  
>  	igt_remove_fb(data->drm_fd, &fb);
> -
> -	return 1;
> -}
> -
> -static void test(data_t *data)
> -{
> -	int valid_connectors = 0;
> -
> -	for_each_connected_output(&data->display, data->output) {
> -		valid_connectors += test_output(data);
> -	}
> -
> -	igt_require_f(valid_connectors, "No suitable connectors found\n");
>  }
>  
>  static int i915_max_dotclock(data_t *data) @@ -297,6 +280,10 @@ 
> static data_t data;
>  
>  igt_main
>  {
> +
> +	enum pipe pipe;
> +        igt_output_t *output;
> +

Something's off with this indentation.


--
Petri Latvala


>  	igt_fixture {
>  		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
>  
> @@ -311,9 +298,15 @@ igt_main
>  	}
>  
>  	for (int i = 0; i < ARRAY_SIZE(subtests); i++) {
> -		igt_subtest(subtests[i].name) {
> -			data.adjust_mode = subtests[i].adjust_mode;
> -			test(&data);
> +		igt_subtest_with_dynamic(subtests[i].name) {
> +			for_each_pipe_with_valid_output(&data.display, pipe, output) {
> +				igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe)) {
> +					data.output = output;
> +					data.pipe = pipe;
> +					data.adjust_mode = subtests[i].adjust_mode;
> +					test_output(&data);
> +				}
> +			}
>  		}
>  	}
>  
> -- 
> 2.26.2
>