[igt-dev] [PATCH v4 1/2] tests/kms_lease: Create dynamic subtests

[Modem, Bhanuprakash]

On Wed-03-08-2022 03:59 pm, Mohammed Thasleem wrote:
> Modified tests/kms_lease to include dynamic test cases.
> 
> v2: Updated dynamic test cases to all test cases.
> v3: Removed dynamic test for which it's not required.
> v4: Moved content of for loop in to igt_dynamic.
> 
> Signed-off-by: Mohammed Thasleem <mohammed.thasleem at intel.com>
> ---
>   tests/kms_lease.c | 68 +++++++++++++++++++++--------------------------
>   1 file changed, 30 insertions(+), 38 deletions(-)
> 
> diff --git a/tests/kms_lease.c b/tests/kms_lease.c
> index 0bf102a6..d734143c 100644
> --- a/tests/kms_lease.c
> +++ b/tests/kms_lease.c
> @@ -809,39 +809,20 @@ static void lease_invalid_plane(data_t *data)
>   }
>   
>   
> -static void run_test(data_t *data, void (*testfunc)(data_t *))
> +static void run_test(data_t *data, void (*testfunc)(data_t *), enum pipe p, igt_output_t *output)
>   {
>   	lease_t *master = &data->master;
>   	igt_display_t *display = &master->display;
> -	igt_output_t *output;
> -	enum pipe p;
> -	unsigned int valid_tests = 0;
> -
> -	for_each_pipe_with_valid_output(display, p, output) {
> -		igt_info("Beginning %s on pipe %s, connector %s\n",
> -			 igt_subtest_name(),
> -			 kmstest_pipe_name(p),
> -			 igt_output_name(output));
> -
> -		data->pipe = p;
> -		data->crtc_id = pipe_to_crtc_id(display, p);
> -		data->connector_id = output->id;
> -		data->plane_id =
> -			igt_pipe_get_plane_type(&data->master.display.pipes[data->pipe],
> -						DRM_PLANE_TYPE_PRIMARY)->drm_plane->plane_id;
>   
> -		testfunc(data);
> +	data->pipe = p;
> +	data->crtc_id = pipe_to_crtc_id(display, p);
> +	data->connector_id = output->id;

We can do these assignments at caller, right?

> +	data->plane_id =
> +		igt_pipe_get_plane_type(&data->master.display.pipes[data->pipe],
> +					DRM_PLANE_TYPE_PRIMARY)->drm_plane->plane_id;
>   
> -		igt_info("\n%s on pipe %s, connector %s: PASSED\n\n",
> -			 igt_subtest_name(),
> -			 kmstest_pipe_name(p),
> -			 igt_output_name(output));
> -
> -		valid_tests++;
> -	}
> +	testfunc(data);
>   
> -	igt_require_f(valid_tests,
> -		      "no valid crtc/connector combinations found\n");
>   }
>   
>   #define assert_double_id_err(ret) \
> @@ -1020,19 +1001,23 @@ static void possible_crtcs_filtering(data_t *data)
>   	mcl.flags = 0;
>   
>   	for (i = 0; i < resources->count_crtcs; i++) {
> -		int lease_fd;
>   
> -		object_ids[mcl.object_count - 1] =
> -			resources->crtcs[i];
> +		igt_dynamic_f("pipe-%s", kmstest_pipe_name(i)) {
> +			int lease_fd;
>   
> -		igt_assert_eq(create_lease(master_fd, &mcl), 0);
> -		lease_fd = mcl.fd;
> +			object_ids[mcl.object_count - 1] =
> +				resources->crtcs[i];
>   
> -		drmSetClientCap(lease_fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
> +			igt_assert_eq(create_lease(master_fd, &mcl), 0);
> +			lease_fd = mcl.fd;
>   
> -		check_crtc_masks(master_fd, lease_fd, 1 << i);
> +			drmSetClientCap(lease_fd,
> +					DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
>   
> -		close(lease_fd);
> +			check_crtc_masks(master_fd, lease_fd, 1 << i);
> +
> +			close(lease_fd);
> +		}
>   	}
>   
>   	free(object_ids);
> @@ -1218,6 +1203,8 @@ static void lease_uevent(data_t *data)
>   igt_main
>   {
>   	data_t data;
> +	igt_output_t *output;
> +	enum pipe p;
>   	const struct {
>   		const char *name;
>   		void (*func)(data_t *);
> @@ -1255,8 +1242,13 @@ igt_main
>   	for (f = funcs; f->name; f++) {
>   
>   		igt_describe(f->desc);
> -		igt_subtest_f("%s", f->name) {
> -			run_test(&data, f->func);
> +		igt_subtest_with_dynamic_f("%s", f->name) {
> +			for_each_pipe_with_valid_output(&data.master.display, p, output) {

Cont.. to the previous comment, we can use as below.
for_each_pipe_with_valid_output(display, data->pipe, data->output)

Apart from these minor comments, this patch looks good to me.

- Bhanu

> +				igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(p),
> +					      igt_output_name(output)) {
> +					run_test(&data, f->func, p, output);
> +				}
> +			}
>   		}
>   	}
>   
> @@ -1267,7 +1259,7 @@ igt_main
>   
>   	igt_describe("Tests that  possible_crtcs logically match between master and "
>   		     "lease, and that the values are correctly renumbered on the lease side.");
> -	igt_subtest("possible-crtcs-filtering")
> +	igt_subtest_with_dynamic("possible-crtcs-filtering")
>   		possible_crtcs_filtering(&data);
>   
>   	igt_describe("Tests the drop/set_master interactions.");