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: 5463 bytes --]

On Thu, 4 Apr 2024 13:56:02 +0300
Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:

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

Good, that's better than my first impression.

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

Yeah, a screenshot or CRC test would be best.

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

This seems just enough, so please have my acked-by for this patch as
is, too.


Thanks,
pq

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