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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 04, 2024 at 11:51:05AM +0300, Pekka Paalanen wrote:
> 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?

It does a TEST_ONLY commit using data->fb and the specified size.
So just confirms that the kernel accepts it, not that it actually
looks correct on screen.

> 
> 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?

I would expect them to actually care about testing their driver
properly. But perhaps my expectations are a bit high.

> 
> This is the only question I have here, everything else is
> 
> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>

Ta.

> 
> 
> 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;
> 



-- 
Ville Syrjälä
Intel