Re: [PATCH i-g-t 4/5] tests/kms_cursor_crc: Test the SIZE_HINTS property

[Pekka Paalanen <pekka.paalanen@collabora.com>]

[-- Attachment #1: Type: text/plain, Size: 4219 bytes --]

On Fri, 15 Mar 2024 21:15:04 +0200
Ville Syrjala <ville.syrjala@linux.intel.com> wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Make sure the driver accepts all the sizes declared in the
> plane's SIZE_HINTS property.
> 
> TODO: Actually test that each reported size works fully?

What does this mean? What does cursor_size_supported() do?

I don't know that, so it looks like you just create an FB and destroy
it, never putting it on the plane where the SIZE_HINTS were found.

>       Would need a bunch of restructuring of the test,
>       and for the moment i915 is the only driver that
>       supports SIZE_HINTS and it's already >100% covered
>       by the existing tests. So I'll leave this for the
>       next guy whose driver has different needs...

How would they know to fix this test? They just see the test pass and
are happy, right?

This is the only question I have here, everything else is

Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>


Thanks,
pq


> 
> Cc: Simon Ser <contact@emersion.fr>
> Cc: Jonas Ådahl <jadahl@redhat.com>
> Cc: Daniel Stone <daniel@fooishbar.org>
> Cc: Sameer Lattannavar <sameer.lattannavar@intel.com>
> Cc: Sebastian Wick <sebastian.wick@redhat.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Pekka Paalanen <pekka.paalanen@collabora.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  tests/kms_cursor_crc.c | 63 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c
> index 38b354972afd..6543e09d3ebb 100644
> --- a/tests/kms_cursor_crc.c
> +++ b/tests/kms_cursor_crc.c
> @@ -55,6 +55,10 @@
>   * Description: Check random placement of a cursor with suspend.
>   * Functionality: cursor, suspend
>   *
> + * SUBTEST: cursor-size-hints
> + * Description: Check that sizes declared in SIZE_HINTS are accepted.
> + * Functionality: cursor
> + *
>   * SUBTEST: cursor-%s
>   * Description: %arg[1]
>   *
> @@ -817,6 +821,46 @@ static bool execution_constraint(enum pipe pipe)
>  	return false;
>  }
>  
> +static void test_size_hints(data_t *data)
> +{
> +	const struct drm_plane_size_hint *hints;
> +	drmModePropertyBlobPtr blob;
> +	uint64_t blob_id;
> +	int count;
> +
> +	igt_require(igt_plane_has_prop(data->cursor, IGT_PLANE_SIZE_HINTS));
> +
> +	blob_id = igt_plane_get_prop(data->cursor, IGT_PLANE_SIZE_HINTS);
> +	/*
> +	 * blob_id==0 is reserved for potential future use, but the
> +	 * meaning has not yet been defined for fail outright if we see it.
> +	 */
> +	igt_assert(blob_id);
> +
> +	blob = drmModeGetPropertyBlob(data->drm_fd, blob_id);
> +	igt_assert(blob);
> +
> +	hints = blob->data;
> +	count = blob->length / sizeof(hints[0]);
> +	igt_assert_lt(0, count);
> +
> +	for (int i = 0; i < count; i++) {
> +		int w = hints[i].width;
> +		int h = hints[i].height;
> +
> +		igt_create_fb(data->drm_fd, w, h,
> +			      DRM_FORMAT_ARGB8888,
> +			      DRM_FORMAT_MOD_LINEAR,
> +			      &data->fb);
> +
> +		igt_assert(cursor_size_supported(data, w, h));
> +
> +		igt_remove_fb(data->drm_fd, &data->fb);
> +	}
> +
> +	drmModeFreePropertyBlob(blob);
> +}
> +
>  static void run_size_tests(data_t *data, int w, int h)
>  {
>  	enum pipe pipe;
> @@ -1011,6 +1055,25 @@ static void run_tests_on_pipe(data_t *data)
>  	igt_fixture
>  		igt_remove_fb(data->drm_fd, &data->fb);
>  
> +	igt_describe("Check that sizes declared in SIZE_HINTS are accepted.");
> +	igt_subtest_with_dynamic("cursor-size-hints") {
> +		for_each_pipe_with_single_output(&data->display, pipe, data->output) {
> +			if (execution_constraint(pipe))
> +				continue;
> +
> +			data->pipe = pipe;
> +
> +			if (!valid_pipe_output_combo(data))
> +				continue;
> +
> +			igt_dynamic_f("pipe-%s-%s",
> +				      kmstest_pipe_name(pipe),
> +				      data->output->name)
> +				run_test(data, test_size_hints,
> +					 data->cursor_max_w, data->cursor_max_h);
> +		}
> +	}
> +
>  	for (cursor_size = 32; cursor_size <= 512; cursor_size *= 2) {
>  		int w = cursor_size;
>  		int h = cursor_size;


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]