Re: [igt-dev] [i-g-t, v3] tests/kms_cursor_crc: skip pipe on invalid connector in cursor size test

["Modem, Bhanuprakash" <bhanuprakash.modem@intel.com>]

On Mon-23-01-2023 09:00 pm, Dnyaneshwar Bhadane wrote:
> Only the valid pipe connector combination reach to the igt commit.
> Cursor max-size test will not affect existing flow as only skip
> for invalid connector.
> For cursor-dpms and cursor-suspend not require to check require_cursor_size
> becuase the cursor height and width used from drm capablities.
> 
> --v3
> - Added for_each_pipe_valid_connector() to check for valid connection.
> - initialize pipe with default 0/PIPE_A suggestion from Petri.
> 
> Signed-off-by: Dnyaneshwar Bhadane <dnyaneshwar.bhadane@intel.com>
> 
> ---
>   lib/igt_kms.h          | 15 +++++++++++++++
>   tests/kms_cursor_crc.c | 18 ++++--------------
>   2 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index be5482e0..5daa751a 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -534,6 +534,21 @@ static inline bool igt_output_is_connected(igt_output_t *output)
>   
>   	return false;
>   }
> +/**
> + * for_each_pipe_valid_connector:
> + * @display: a pointer to an #igt_display_t structure
> + * @pipe: from pipe to iterate.
> + * @output: #igt_output_t to check.
> + *
> + * It loop over the all the pipe and check for the given
> + * @output/@pipe can be used together.
> + *
> + */
> +#define for_each_pipe_valid_connector(display, pipe, output) \
> +	for_each_pipe_static(pipe) \
> +		for_each_if ((((display)->pipes[(pipe)].enabled) && \
> +				igt_pipe_connector_valid((pipe), (output))))
> +
>   
>   /**
>    * igt_pipe_connector_valid:
> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
> index d5a4b30b..0394768b 100644
> --- a/tests/kms_cursor_crc.c
> +++ b/tests/kms_cursor_crc.c
> @@ -749,7 +749,7 @@ static void run_size_tests(data_t *data, int w, int h)
>   				}
>   			}
>   
> -			for_each_pipe(&data->display, pipe) {
> +			for_each_pipe_valid_connector(&data->display, pipe, data->output) {
>   				data->pipe = pipe;
>   
>   				if (require_cursor_size(data, w, h)) {
> @@ -772,7 +772,7 @@ static void run_tests_on_pipe(data_t *data)
>   {
>   	enum pipe pipe;
>   	int cursor_size;
> -
> +	pipe = PIPE_A;

This looks ugly to me.

>   	igt_fixture {
>   		data->output = igt_get_single_output_for_pipe(&data->display, pipe);

Probably, this might be the bug in existing code. Perhaps, you can drop 
data->output from igt_fixture and update each subtest to check for the 
valid output. Please check my engg patch for BJ [1].

s/for_each_pipe/for_each_pipe_with_single_output/

[1]: https://patchwork.freedesktop.org/patch/519058/

- Bhanu

>   		igt_require(data->output);
> @@ -850,15 +850,10 @@ static void run_tests_on_pipe(data_t *data)
>   
>   	igt_describe("Check random placement of a cursor with DPMS.");
>   	igt_subtest_with_dynamic("cursor-dpms") {
> -		for_each_pipe(&data->display, pipe) {
> +		for_each_pipe_valid_connector(&data->display, pipe, data->output) {
>   			data->pipe = pipe;
>   			data->flags = TEST_DPMS;
>   
> -			if (require_cursor_size(data, data->cursor_max_w, data->cursor_max_h)) {
> -				igt_debug("Cursor size %dx%d not supported by driver\n",
> -					  data->cursor_max_w, data->cursor_max_h);
> -				continue;
> -			}
>   
>   			igt_dynamic_f("pipe-%s-%s",
>   				      kmstest_pipe_name(pipe),
> @@ -871,15 +866,10 @@ static void run_tests_on_pipe(data_t *data)
>   
>   	igt_describe("Check random placement of a cursor with suspend.");
>   	igt_subtest_with_dynamic("cursor-suspend") {
> -		for_each_pipe(&data->display, pipe) {
> +		for_each_pipe_valid_connector(&data->display, pipe, data->output) {
>   			data->pipe = pipe;
>   			data->flags = TEST_SUSPEND;
>   
> -			if (require_cursor_size(data, data->cursor_max_w, data->cursor_max_h)) {
> -				igt_debug("Cursor size %dx%d not supported by driver\n",
> -					  data->cursor_max_w, data->cursor_max_h);
> -				continue;
> -			}
>   
>   			igt_dynamic_f("pipe-%s-%s",
>   				      kmstest_pipe_name(pipe),