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@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).
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.
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
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
