[igt-dev] [PATCH V6 i-g-t 2/6] kms_writeback: Add initial writeback tests
Ser, Simon
simon.ser at intel.com
Thu Jul 18 09:56:39 UTC 2019
On Thu, 2019-07-18 at 10:49 +0100, Liviu.Dudau at 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 at 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.
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.)
> 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
More information about the igt-dev
mailing list