[igt-dev] [PATCH i-g-t] tests/kms_properties: Create dynamic subtests

[Thasleem, Mohammed]

Hi Ville,

Thanks for the reviews.
Addressed your comments at below series.
https://patchwork.freedesktop.org/series/109555/ 
Could you please provide inputs?

Thanks,
Mohammed Thasleem
-----Original Message-----
From: Ville Syrjälä <ville.syrjala at linux.intel.com> 
Sent: Friday, October 7, 2022 1:27 AM
To: Thasleem, Mohammed <mohammed.thasleem at intel.com>
Cc: igt-dev at lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_properties: Create dynamic subtests

On Fri, Oct 07, 2022 at 01:03:19AM +0530, Mohammed Thasleem wrote:
> Modified kms_properties to include dynamic test cases.
> 
> v2: Fixed compilation issue.
> v3: Removed redundant code.
> v4: Moved get_prop_sanity calls to igt_subtest_group.
> v5: Replace for loop with for_each_disconnected_output.
> 
> Signed-off-by: Mohammed Thasleem <mohammed.thasleem at intel.com>
> ---
>  tests/kms_properties.c | 176 
> ++++++++++++++++++-----------------------
>  1 file changed, 77 insertions(+), 99 deletions(-)
> 
> diff --git a/tests/kms_properties.c b/tests/kms_properties.c index 
> 2958efac..76ef64ab 100644
> --- a/tests/kms_properties.c
> +++ b/tests/kms_properties.c
> @@ -43,6 +43,7 @@ static void prepare_pipe(igt_display_t *display, enum pipe pipe, igt_output_t *o
>  	igt_create_pattern_fb(display->drm_fd, mode->hdisplay, mode->vdisplay,
>  			      DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR, fb);
>  
> +	igt_display_reset(display);
>  	igt_output_set_pipe(output, pipe);
>  
>  	igt_plane_set_fb(igt_output_get_plane_type(output, 
> DRM_PLANE_TYPE_PRIMARY), fb); @@ -230,77 +231,52 @@ static void 
> run_connector_property_tests(igt_display_t *display, enum pipe pipe,
>  
>  static void plane_properties(igt_display_t *display, bool atomic)  {
> -	bool found_any = false, found;
>  	igt_output_t *output;
>  	enum pipe pipe;
>  
> -	if (atomic)
> -		igt_skip_on(!display->is_atomic);
> -
> -	for_each_pipe(display, pipe) {
> -		found = false;
> -
> -		for_each_valid_output_on_pipe(display, pipe, output) {
> -			found_any = found = true;
> -
> +	for_each_pipe_with_single_output(display, pipe, output) {
> +		igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(pipe),
> +			      igt_output_name(output)) {
>  			run_plane_property_tests(display, pipe, output, atomic);
> -			break;
>  		}
>  	}
> -
> -	igt_skip_on(!found_any);
>  }
>  
>  static void crtc_properties(igt_display_t *display, bool atomic)  {
> -	bool found_any_valid_pipe = false, found;
>  	enum pipe pipe;
>  	igt_output_t *output;
>  
> -	if (atomic)
> -		igt_skip_on(!display->is_atomic);
> -
> -	for_each_pipe(display, pipe) {
> -		found = false;
> -
> -		for_each_valid_output_on_pipe(display, pipe, output) {
> -			found_any_valid_pipe = found = true;
> -
> +	for_each_pipe_with_single_output(display, pipe, output) {
> +		igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(pipe),
> +			      igt_output_name(output)) {
>  			run_crtc_property_tests(display, pipe, output, atomic);
> -			break;
>  		}
>  	}
> -
> -	igt_skip_on(!found_any_valid_pipe);
>  }
>  
>  static void connector_properties(igt_display_t *display, bool atomic)  
> {
> -	int i;
>  	enum pipe pipe;
>  	igt_output_t *output;
>  
> -	if (atomic)
> -		igt_skip_on(!display->is_atomic);
> -
>  	for_each_connected_output(display, output) {
> -		bool found = false;
>  
>  		for_each_pipe(display, pipe) {
>  			if (!igt_pipe_connector_valid(pipe, output))
>  				continue;
>  
> -			found = true;
> -			run_connector_property_tests(display, pipe, output, atomic);
> -			break;
> +			igt_dynamic_f("pipe-%s-%s", kmstest_pipe_name(pipe),
> +				      igt_output_name(output)) {
> +				run_connector_property_tests(display, pipe, output, atomic);
> +			}
>  		}
> +		break;
>  
> -		igt_assert_f(found, "Connected output should have at least 1 valid crtc\n");
>  	}
>  
> -	for (i = 0; i < display->n_outputs; i++)
> -		if (!igt_output_is_connected(&display->outputs[i]))
> -			run_connector_property_tests(display, PIPE_NONE, &display->outputs[i], atomic);
> +	for_each_disconnected_output(display, output)

Nothing to do with the subject of this patch.

> +		run_connector_property_tests(display, PIPE_NONE, output, atomic);
>  }
>  
>  static void test_invalid_properties(int fd, @@ -368,7 +344,6 @@ 
> static void test_object_invalid_properties(igt_display_t *display,
>  	igt_output_t *output;
>  	igt_plane_t *plane;
>  	enum pipe pipe;
> -	int i;
>  
>  	for_each_pipe(display, pipe)
>  		test_invalid_properties(display->drm_fd, id, type, 
> display->pipes[pipe].crtc_id, DRM_MODE_OBJECT_CRTC, atomic); @@ -377,7 +352,7 @@ static void test_object_invalid_properties(igt_display_t *display,
>  		for_each_plane_on_pipe(display, pipe, plane)
>  			test_invalid_properties(display->drm_fd, id, type, 
> plane->drm_plane->plane_id, DRM_MODE_OBJECT_PLANE, atomic);
>  
> -	for (i = 0, output = &display->outputs[0]; i < display->n_outputs; output = &display->outputs[++i])
> +	for_each_connected_output(display, output)

Those two things are not at all the same.

There are clearly multiple patches hiding inside this single mega patch of yours. Please split it up properly.

--
Ville Syrjälä
Intel