Hi Paulo, On Thursday 13 July 2017 02:31 AM, Paulo Zanoni wrote:
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
Unfortunately I haven't...
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.
...but I will add the check as you have mentioned
I chose to loop over tiling constants as they are in a simple arithmetic order. anyhow I will just change that.More below.Signed-off-by: Praveen Paneri <praveen.pan...@intel.com> --- 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.
Also as mentioned above can I just add a check to skip Y-tiling tests for older platforms?
igt_skip_on(intel_gen(intel_get_drm_devid(drm_fd)) < 9 &&
tiling == I915_TILING_Y);
+ 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.
Will fix it
+ + 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.
make sense, will add.
+ 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 Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
