[igt-dev] [PATCH 2/2] tests/prime_generic: add vendor-agnostic prime tests
Rodrigo Siqueira
rodrigosiqueiramelo at gmail.com
Tue Jul 9 13:48:14 UTC 2019
Hi Oleg,
I really liked this patch since it looks like documentation, which
means that someone like me can use it for understanding how to use
prime.
Btw, a few comments inline
On Thu, Jul 4, 2019 at 6:56 AM Oleg Vasilev <oleg.vasilev at intel.com> wrote:
>
> Currently, we have different sets of prime tests:
> - vgem+i915
> - amdgpu+i915
> - nv+i915
>
> Those tests use vendor-specific ioctls, therefore, not interchangeable.
> The idea is to create a set of tests which are expected to work on any
> prime-compatible driver. It can be run with any combination of
> exporter+importer, where
>
> The first test is simple:
> 1. Exporter creates a framebuffer and fills it with a plain color
> 2. Fb is transferred to the importer
> 3. Importer modesets and computes pipe CRC
> 4. Importer draws the same color through cairo and compares CRC
>
> The initial motivation comes from the need to test prime support in
> vkms.
>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo at gmail.com>
> Cc: Daniel Vetter <daniel at ffwll.ch>
> Signed-off-by: Oleg Vasilev <oleg.vasilev at intel.com>
> ---
> tests/meson.build | 1 +
> tests/prime_generic.c | 238 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 239 insertions(+)
> create mode 100644 tests/prime_generic.c
>
> diff --git a/tests/meson.build b/tests/meson.build
> index 34a74025..1c938e95 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -76,6 +76,7 @@ test_progs = [
> 'prime_self_import',
> 'prime_udl',
> 'prime_vgem',
> + 'prime_generic',
> 'syncobj_basic',
> 'syncobj_wait',
> 'template',
> diff --git a/tests/prime_generic.c b/tests/prime_generic.c
> new file mode 100644
> index 00000000..65e48763
> --- /dev/null
> +++ b/tests/prime_generic.c
> @@ -0,0 +1,238 @@
> +/*
> + * Copyright © 2019 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include "igt.h"
> +#include "igt_vgem.h"
> +
> +#include <sys/ioctl.h>
> +#include <sys/poll.h>
> +#include <time.h>
> +
> +static struct {
> + double r, g, b;
> + uint32_t color;
> + igt_crc_t prime_crc;
> + igt_crc_t direct_crc;
> + char *prime_str;
> + char *direct_str;
> +} colors[3] = {
> + { .r = 0.0, .g = 0.0, .b = 0.0, .color = 0xff000000},
> + { .r = 1.0, .g = 1.0, .b = 1.0, .color = 0xffffffff},
> + { .r = 1.0, .g = 0.0, .b = 0.0, .color = 0xffff0000},
> +};
> +
> +IGT_TEST_DESCRIPTION("Generic tests, working with any prime device");
> +
> +static bool has_prime_import(int fd)
> +{
> + uint64_t value;
> +
> + if (drmGetCap(fd, DRM_CAP_PRIME, &value))
> + return false;
> +
> + return value & DRM_PRIME_CAP_IMPORT;
> +}
> +
> +static bool has_prime_export(int fd)
> +{
> + uint64_t value;
> +
> + if (drmGetCap(fd, DRM_CAP_PRIME, &value))
> + return false;
> +
> + return value & DRM_PRIME_CAP_EXPORT;
> +}
> +
> +static void prepare_scratch(int exporter, struct vgem_bo *scratch,
> + drmModeModeInfo *mode, uint32_t color)
> +{
> + int i;
> + uint32_t *ptr;
> +
> + scratch->width = mode->hdisplay;
> + scratch->height = mode->vdisplay;
> + scratch->bpp = 32;
> + vgem_create(exporter, scratch);
> +
> + ptr = vgem_mmap(exporter, scratch, PROT_WRITE);
> + for (i = 0; i < scratch->size / sizeof(*ptr); ++i)
> + ptr[i] = color;
> +
> + munmap(ptr, scratch->size);
> +}
> +
> +static void prepare_fb(int importer, struct vgem_bo *scratch, struct igt_fb *fb)
> +{
> + enum igt_color_encoding color_encoding = IGT_COLOR_YCBCR_BT709;
> + enum igt_color_range color_range = IGT_COLOR_YCBCR_LIMITED_RANGE;
> +
> + igt_init_fb(fb, importer, scratch->width, scratch->height,
> + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> + color_encoding, color_range);
> +}
> +
> +static void import_fb(int importer, struct igt_fb *fb,
> + int dmabuf_fd, uint32_t pitch)
> +{
> + uint32_t offsets[4], pitches[4], handles[4];
> + int ret;
> +
> + fb->gem_handle = prime_fd_to_handle(importer, dmabuf_fd);
> +
> + handles[0] = fb->gem_handle;
> + pitches[0] = pitch;
> + offsets[0] = 0;
> +
> + ret = drmModeAddFB2(importer, fb->width, fb->height,
> + DRM_FORMAT_XRGB8888,
> + handles, pitches, offsets,
> + &fb->fb_id, 0);
> + igt_assert(ret == 0);
> +}
> +
> +static void set_fb(struct igt_fb *fb,
> + igt_display_t *display,
> + igt_output_t *output)
> +{
> + igt_plane_t *primary;
> + int ret;
> +
> + primary = igt_output_get_plane(output, 0);
> + igt_plane_set_fb(primary, fb);
> + ret = igt_display_commit(display);
> +
> + igt_assert(ret == 0);
> +}
> +
> +static void test_crc(int exporter, int importer)
> +{
> + enum pipe pipe = PIPE_A;
> + igt_display_t display;
> + igt_output_t *output;
> + igt_pipe_crc_t *pipe_crc;
> + struct igt_fb fb;
> + int dmabuf_fd;
> + struct vgem_bo scratch; /* despite the name, it suits for any
> + gem-compatible device */
Since vgem_bo is suiting for any gem-compatible device, how about send
a patch that added a better name for it? IMHO this is good for
readability sake - anyway, just an idea.
> + int i, j;
> + drmModeModeInfo *mode;
> +
> + bool crc_equal;
> +
> + igt_display_require(&display, importer);
> + igt_skip_on(pipe >= display.n_pipes);
> + output = igt_get_single_output_for_pipe(&display, pipe);
> + igt_require_f(output, "No connector found for pipe %s\n",
> + kmstest_pipe_name(pipe));
> +
> + igt_display_reset(&display);
> + igt_output_set_pipe(output, pipe);
The above code looks like a setup, how about move it for a static
function responsible for this action? IMHO, it could be easier to
understand test_crc, if it's focused on only on CRC test.
Additionally, I want to suggest that igt_display_fini() gets invoked
at the end of this function because you initialized some display stuff
and did not clean up them at the end.
> +
> + mode = igt_output_get_mode(output);
> + pipe_crc = igt_pipe_crc_new(importer, pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
> +
> + for (i = 0; i < ARRAY_SIZE(colors); i++) {
> + prepare_scratch(exporter, &scratch, mode, colors[i].color);
> + dmabuf_fd = prime_handle_to_fd(exporter, scratch.handle);
> + gem_close(exporter, scratch.handle);
> +
> + prepare_fb(importer, &scratch, &fb);
> + import_fb(importer, &fb, dmabuf_fd, scratch.pitch);
> + close(dmabuf_fd);
> +
> + set_fb(&fb, &display, output);
> + igt_pipe_crc_collect_crc(pipe_crc, &colors[i].prime_crc);
> + colors[i].prime_str = igt_crc_to_string(&colors[i].prime_crc);
Maybe I missed something, but I think you forgot to free the buffer
allocated by igt_crc_to_string().
> + igt_debug("Prime CRC for %#08x is %s\n",
> + colors[i].color, colors[i].prime_str);
> + igt_remove_fb(importer, &fb);
> +
> + igt_create_color_fb(importer,
> + mode->hdisplay, mode->vdisplay,
> + DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> + colors[i].r, colors[i].g, colors[i].b,
> + &fb);
> +
> + set_fb(&fb, &display, output);
> + igt_pipe_crc_collect_crc(pipe_crc, &colors[i].direct_crc);
> + colors[i].direct_str = igt_crc_to_string(&colors[i].direct_crc);
Ditto.
> + igt_debug("Direct CRC for %#08x is %s\n",
> + colors[i].color, colors[i].direct_str);
> + igt_remove_fb(importer, &fb);
How about creating a static function with a meaningful name that makes
the following steps:
set_fb() -> igt_pipe_crc_collect_crc() -> colors[i]* -> igt_debug()
-> igt_remove_fb()
The above steps are similar for 'prime' and 'direct'. Maybe a function
that only handles these steps makes it easy to understand the code and
also improve the maintainability.
Best regards
> + }
> + igt_pipe_crc_free(pipe_crc);
> +
> + igt_debug("CRC table:\n");
> + igt_debug("Color\t\tPrime\t\tDirect\n");
> + for (i = 0; i < ARRAY_SIZE(colors); i++) {
> + igt_debug("%#08x\t%.8s\t%.8s\n", colors[i].color,
> + colors[i].prime_str, colors[i].direct_str);
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(colors); i++) {
> + for (j = 0; j < ARRAY_SIZE(colors); j++) {
> + if (i == j) {
> + igt_assert_crc_equal(&colors[i].prime_crc,
> + &colors[j].direct_crc);
> + } else {
> + crc_equal = igt_check_crc_equal(&colors[i].prime_crc,
> + &colors[j].direct_crc);
> + igt_assert_f(!crc_equal, "CRC should be different");
> + }
> + }
> + }
> +}
> +
> +static void run_test_crc(int export_chipset, int import_chipset)
> +{
> + int importer = -1;
> + int exporter = -1;
> +
> + exporter = drm_open_driver(export_chipset);
> + importer = drm_open_driver_master(import_chipset);
> +
> + igt_require(has_prime_export(exporter));
> + igt_require(has_prime_import(importer));
> + igt_require_pipe_crc(importer);
> +
> + test_crc(exporter, importer);
> + close(importer);
> + close(exporter);
> +}
> +
> +igt_main
> +{
> + igt_fixture {
> + kmstest_set_vt_graphics_mode();
> + }
> + igt_subtest("crc-vgem-vkms")
> + run_test_crc(DRIVER_VGEM, DRIVER_VKMS);
> + igt_subtest("crc-i915-vkms")
> + run_test_crc(DRIVER_INTEL, DRIVER_VKMS);
> + igt_subtest("crc-vgem-i915")
> + run_test_crc(DRIVER_VGEM, DRIVER_INTEL);
> + igt_subtest("crc-amd-i915")
> + run_test_crc(DRIVER_AMDGPU, DRIVER_INTEL);
> + igt_subtest("crc-i915-amd")
> + run_test_crc(DRIVER_INTEL, DRIVER_AMDGPU);
> +}
> --
> 2.22.0
>
--
Rodrigo Siqueira
https://siqueira.tech
More information about the igt-dev
mailing list