On Friday 14 July 2017 07:55 PM, Paulo Zanoni wrote:
Em Sex, 2017-07-14 às 19:25 +0530, Praveen Paneri escreveu: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? IUnfortunately 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 mentionedMore 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.I chose to loop over tiling constants as they are in a simple arithmetic order. anyhow I will just change that.Just put the two local_format stuff in an array and iterate over it.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);You can't do this here because the same subtest tests X tiling.
Okay! what if I just skip the loop for Y tiling with some message?
Will it be possible in just one subtest. I thought we will have to duplicate all the subtests for Y-tile case? Plz suggestPerhaps we could make Y tiling be a separate subtest? I'm not a huge fan of single tests that do tons of stuff.
Thanks, Praveen
+ 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(t ilin g))) { + igt_info("%s on pipe %s, connector %s: SKIPPED\n", + igt_subtest_name (), + kmstest_pipe_nam e(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
