Em Sex, 2017-04-28 às 20:07 +0530, Praveen Paneri escreveu: > Now that we have support for Y-tiled buffers, add another > iteration of tests for Y-tiled buffers.
Have you tested this on platforms that don't support Y-tiled buffers? I don't see a check for that, so I wonder if we'll just fail some assertion or correctly hit some igt_skip() call I couldn't find. More below. > > Signed-off-by: Praveen Paneri <[email protected]> > --- > tests/kms_fbc_crc.c | 71 +++++++++++++++++++++++++++++------------ > ------------ > 1 file changed, 39 insertions(+), 32 deletions(-) > > diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c > index 7964e05..cdf04c1 100644 > --- a/tests/kms_fbc_crc.c > +++ b/tests/kms_fbc_crc.c > @@ -318,12 +318,10 @@ static void prepare_crtc(data_t *data) > igt_output_set_pipe(output, data->pipe); > } > > -static void create_fbs(data_t *data, bool tiled, struct igt_fb *fbs) > +static void create_fbs(data_t *data, uint64_t tiling, struct igt_fb > *fbs) > { > int rc; > drmModeModeInfo *mode = igt_output_get_mode(data->output); > - uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED : > - LOCAL_DRM_FORMAT_MOD_NONE; > > rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode- > >vdisplay, > DRM_FORMAT_XRGB8888, tiling, > @@ -344,8 +342,8 @@ static void get_ref_crcs(data_t *data) > struct igt_fb fbs[4]; > int i; > > - create_fbs(data, false, &fbs[0]); > - create_fbs(data, false, &fbs[2]); > + create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[0]); > + create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[2]); > > fill_mmap_gtt(data, fbs[2].gem_handle, 0xff); > fill_mmap_gtt(data, fbs[3].gem_handle, 0xff); > @@ -366,7 +364,7 @@ static void get_ref_crcs(data_t *data) > igt_remove_fb(data->drm_fd, &fbs[i]); > } > > -static bool prepare_test(data_t *data, enum test_mode test_mode) > +static bool prepare_test(data_t *data, enum test_mode test_mode, > uint64_t tiling) > { > igt_display_t *display = &data->display; > igt_output_t *output = data->output; > @@ -374,7 +372,7 @@ static bool prepare_test(data_t *data, enum > test_mode test_mode) > > data->primary = igt_output_get_plane_type(data->output, > DRM_PLANE_TYPE_PRIMARY); > > - create_fbs(data, true, data->fb); > + create_fbs(data, tiling, data->fb); > > igt_pipe_crc_free(data->pipe_crc); > data->pipe_crc = NULL; > @@ -484,32 +482,41 @@ static void run_test(data_t *data, enum > test_mode mode) > > reset_display(data); > > - for_each_pipe_with_valid_output(display, data->pipe, data- > >output) { > - prepare_crtc(data); > - > - igt_info("Beginning %s on pipe %s, connector %s\n", > - igt_subtest_name(), > - kmstest_pipe_name(data->pipe), > - igt_output_name(data->output)); > - > - if (!prepare_test(data, mode)) { > - igt_info("%s on pipe %s, connector %s: > SKIPPED\n", > - igt_subtest_name(), > - kmstest_pipe_name(data->pipe), > - igt_output_name(data->output)); > - continue; > + for (int tiling = I915_TILING_X; > + tiling <= I915_TILING_Y; tiling++) { What I don't understand is why this part of the code chooses to go with the tiling constants (I915_TILING_) only to later convert them to modifiers with igt_fb_tiling_to_mod(). If this loop iterated over the modifiers directly we wouldn't need that. The rest of the code only cares about the modifiers. > + for_each_pipe_with_valid_output(display, > + data->pipe, data- > >output) { > + prepare_crtc(data); > + > + igt_info("Beginning %s on pipe %s, connector > " > + "%s, %s-tiled\n", > + igt_subtest_name(), > + kmstest_pipe_name(data- > >pipe), > + igt_output_name(data- > >output), > + (tiling == I915_TILING_X) ? > "x":"y" ); This change is not keeping the indentation style, things should be aligned with the parens (although I see they're actually aligned with the quote, which is also weird). The same can be said for the other two igt_info() calls in this patch. > + > + if (!prepare_test(data, mode, > + igt_fb_tiling_to_mod(tilin > g))) { > + igt_info("%s on pipe %s, connector > %s: SKIPPED\n", > + igt_subtest_name(), > + kmstest_pipe_name(da > ta->pipe), > + igt_output_name(data > ->output)); This one is missing the "%s-tiled" part that was added in the other two messages. And we can probably create a "const char *tiling_name" variable to store the %s part in order to avoid the same ternary operator in the 3 if statements. > + continue; > + } > + > + valid_tests++; > + > + test_crc(data, mode); > + > + igt_info("%s on pipe %s, connector %s" > + "%s-tiled: PASSED\n", > + igt_subtest_name(), > + kmstest_pipe_name(data- > >pipe), > + igt_output_name(data- > >output), > + (tiling == I915_TILING_X) ? > "x":"y" ); > + > + finish_crtc(data, mode); > } > - > - valid_tests++; > - > - test_crc(data, mode); > - > - igt_info("%s on pipe %s, connector %s: PASSED\n", > - igt_subtest_name(), > - kmstest_pipe_name(data->pipe), > - igt_output_name(data->output)); > - > - finish_crtc(data, mode); > } > > igt_require_f(valid_tests, "no valid crtc/connector > combinations found\n"); _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
