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 <praveen.paneri@xxxxxxxxx>
> ---
> 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
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
