Thanks for the clarification!
On Tue, 2019-07-16 at 16:22 +0100, Liviu.Dudau@xxxxxxx 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
