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

["Ser, Simon" <simon.ser@intel.com>]

On Thu, 2019-07-18 at 12:15 +0100, Liviu.Dudau@arm.com wrote:
> On Thu, Jul 18, 2019 at 09:56:39AM +0000, Ser, Simon wrote:
> > On Thu, 2019-07-18 at 10:49 +0100, Liviu.Dudau@arm.com wrote:
> > > On Wed, Jul 17, 2019 at 11:46:39AM +0000, Ser, Simon wrote:
> > > > 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).
> > > 
> > > Don't want to offend or anything, but this does sound like bikeshedding. You
> > > have a couple of parameters that you want to have a test for because they are
> > > linked together (output framebuffer and fence) and you go through the
> > > combination of possible bad options in the test. Not sure what name we can use
> > > for the function, other than maybe 'test_invalid_parameters'? Given that 2/3
> > > tests an invalid out fence, the name was thought to be relevant.
> > > 
> > > Having invalid out buffer test separate into its own test brings no benefits, IMHO.
> > 
> > Well, the issue is that I've been confused when reviewing the patch
> > series. I had trouble understanding what the test does and why. I also
> > had trouble to identify that do_writeback_test never submits a
> > writeback operation (see other e-mail).
> > 
> > A name that is relevant "all the time, most of the time", is not
> > relevant at all in my opinion. It just tricks the reader into thinking
> > the test does one thing, while it also does something else.
> > 
> > If it would be obvious, I wouldn't mind. But here IMHO it hurts
> > readability. So I'd prefer to rename the function.
> 
> I take your comments as a valid point.
> 
> Does "test_invalid_parameters" sound like a good name for the function? Is so,
> Rodrigo, can you please use that name in the next revision of the patch?

Yes, this name sounds perfectly fine :)

> > If you think it's obvious, then maybe it's just me. I'd love to hear
> > from others if they have a different opinion.
> > 
> > (As a side note, I agree I have a tendency to bikeshed, I try to mark
> > my bikesheddings behind "nit:" flags.)
> 
> I've only said "it sounded like" :)

Eheh :P

> Best regards,
> Liviu
> 
> > > Best regards,
> > > Liviu
> > > 
> > > > > > > > +		{
> > > > > > > > +			/* 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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev