Re: [igt-dev] [PATCH V6 i-g-t 2/6] kms_writeback: Add initial writeback tests

From: "Ser, Simon" <simon.ser@intel.com>
To: "Liviu.Dudau@arm.com" <Liviu.Dudau@arm.com>,
	"rodrigosiqueiramelo@gmail.com" <rodrigosiqueiramelo@gmail.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"igt-dev@lists.freedesktop.org" <igt-dev@lists.freedesktop.org>,
	"nd@arm.com" <nd@arm.com>
Subject: Re: [igt-dev] [PATCH V6 i-g-t 2/6] kms_writeback: Add initial writeback tests
Date: Wed, 17 Jul 2019 11:46:39 +0000	[thread overview]
Message-ID: <aae987e0b45629a632f87ad02cf256d30045c8aa.camel@intel.com> (raw)
In-Reply-To: <20190716152207.GC13094@e110455-lin.cambridge.arm.com>

Thanks for the clarification!

On Tue, 2019-07-16 at 16:22 +0100, Liviu.Dudau@arm.com wrote:
> > > > +static void invalid_out_fence(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb)
> > > > +{
> > > > +	int i, ret;
> > > > +	int32_t out_fence;
> > > > +	struct {
> > > > +		uint32_t fb_id;
> > > > +		bool ptr_valid;
> > > > +		int32_t *out_fence_ptr;
> > > > +	} invalid_tests[] = {
> > > > +		{
> > > > +			/* No output buffer, but the WRITEBACK_OUT_FENCE_PTR set. */
> > > > +			.fb_id = 0,
> > > > +			.ptr_valid = true,
> > > > +			.out_fence_ptr = &out_fence,
> > > > +		},
> > > > +		{
> > > > +			/* Invalid output buffer. */
> > > > +			.fb_id = invalid_fb->fb_id,
> > > > +			.ptr_valid = true,
> > > > +			.out_fence_ptr = &out_fence,
> > > > +		},
> > > 
> > > This doesn't belong in this function (invalid_out_fence), since this
> > > checks an invalid framebuffer, not an invalid fence. We should probably
> > > move it to writeback_fb_id (and rename that function to test_fb?).
> 
> It tries to test that you can't trick the driver to do any work on a fence if
> the framebuffer is invalid, so the set of tests tries: no fb with valid fence,
> invalid fb with valid fence, valid fb but invalid fence and assumes that no
> fb with invalid fence is a NOP anyway.

Yeah, that makes sense, it's just confusing to put it in a function
named invalid_out_fence. Here the out fence is valid, but the output
buffer isn't, so it should probably be moved away (or this function
should be renamed).

> > > > +		{
> > > > +			/* Invalid WRITEBACK_OUT_FENCE_PTR. */
> > > > +			.fb_id = valid_fb->fb_id,
> > > > +			.ptr_valid = false,
> > > > +			.out_fence_ptr = (int32_t *)0x8,
> > > > +		},
> > > > +	};
> > > > +
> > > > +	for (i = 0; i < ARRAY_SIZE(invalid_tests); i++) {
> > > > +		ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > +					invalid_tests[i].fb_id,
> > > > +					invalid_tests[i].out_fence_ptr,
> > > > +					invalid_tests[i].ptr_valid);
> > > > +		igt_assert(ret != 0);
> > > 
> > > Maybe we can check for -ret == EINVAL?
> > > 
> > > > +	}
> > > > +}
> > > > +
> > > > +static void writeback_fb_id(igt_output_t *output, igt_fb_t *valid_fb, igt_fb_t *invalid_fb)
> > > 
> > > invalid_fb doesn't seem to be used here. valid_fb seems to be set to
> > > the input framebuffer. It's probably not a good idea to use the same FB
> > > for input and writeback output.
> > > 
> > > > +{
> > > > +
> > > > +	int ret;
> > > > +
> > > > +	/* Valid output buffer */
> > > > +	ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > +				valid_fb->fb_id, NULL, false);
> > > > +	igt_assert(ret == 0);
> > > > +
> > > > +	/* Invalid object for WRITEBACK_FB_ID */
> > > > +	ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > +				output->id, NULL, false);
> > > > +	igt_assert(ret == -EINVAL);
> > > > +
> > > > +	/* Zero WRITEBACK_FB_ID */
> > > > +	ret = do_writeback_test(output, DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > +				0, NULL, false);
> > > > +	igt_assert(ret == 0);
> > > > +}
> > > > +
> > > > +igt_main
> > > > +{
> > > > +	igt_display_t display;
> > > > +	igt_output_t *output;
> > > > +	igt_plane_t *plane;
> > > > +	igt_fb_t input_fb;
> > > > +	drmModeModeInfo mode;
> > > > +	int ret;
> > > > +
> > > > +	memset(&display, 0, sizeof(display));
> > > > +
> > > > +	igt_fixture {
> > > > +		display.drm_fd = drm_open_driver_master(DRIVER_ANY);
> > > > +		igt_display_require(&display, display.drm_fd);
> > > > +
> > > > +		kmstest_set_vt_graphics_mode();
> > > > +
> > > > +		igt_display_require(&display, display.drm_fd);
> > > > +
> > > > +		igt_require(display.is_atomic);
> > > > +
> > > > +		output = kms_writeback_get_output(&display);
> > > > +		igt_require(output);
> > > > +
> > > > +		if (output->use_override_mode)
> > > > +			memcpy(&mode, &output->override_mode, sizeof(mode));
> > > > +		else
> > > > +			memcpy(&mode, &output->config.default_mode, sizeof(mode));
> > > > +
> > > > +		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> > > > +		igt_require(plane);
> > > 
> > > Maybe we can assert on this?
> > > 
> > > > +		ret = igt_create_fb(display.drm_fd, mode.hdisplay,
> > > > +				    mode.vdisplay,
> > > > +				    DRM_FORMAT_XRGB8888,
> > > > +				    igt_fb_mod_to_tiling(0),
> > > 
> > > This is supposed to take a modifier. DRM_FORMAT_MOD_LINEAR would be
> > > better to make this clear.
> 
> Agree. The patchset pre-dates the modifiers support (or has the same age, I
> forgot)
> 
> > > (Applies to other lines of this patch)
> > > 
> > > > +				    &input_fb);
> > > > +		igt_assert(ret >= 0);
> > > > +		igt_plane_set_fb(plane, &input_fb);
> > > > +	}
> > > > +
> > > > +	igt_subtest("writeback-pixel-formats") {
> > > > +		drmModePropertyBlobRes *formats_blob = get_writeback_formats_blob(output);
> > > 
> > > Need to drmModeFreePropertyBlob this.
> > > 
> > > > +		const char *valid_chars = "0123456 ABCGNRUVXY";
> > > > +		unsigned int i;
> > > > +		char *c;
> > > > +
> > > > +		/*
> > > > +		 * We don't have a comprehensive list of formats, so just check
> > > > +		 * that the blob length is sensible and that it doesn't contain
> > > > +		 * any outlandish characters
> > > > +		 */
> > > > +		igt_assert(!(formats_blob->length % 4));
> > > > +		c = formats_blob->data;
> > > > +		for (i = 0; i < formats_blob->length; i++)
> > > > +			igt_assert_f(strchr(valid_chars, c[i]),
> > > > +				     "Unexpected character %c\n", c[i]);
> > > 
> > > Honestly, I'm not a fan of this check. There's no guarantee that fourcc
> > > codes will be made from ASCII characters, in fact some formats already
> > > have non-printable chars in them. I don't want to have to update this
> > > test when a new fourcc format is added.
> 
> Like the comments says, we don't have a full list of formats to check against.
> Suggestions on how to improve are welcome, but I don't think we should delay
> (any longer) the patchset due to this.

Agreed.

> Best regards,
> Liviu
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx