[igt-dev] [PATCH 2/2] tests/prime_generic: add vendor-agnostic prime tests
Ser, Simon
simon.ser at intel.com
Fri Aug 16 10:07:56 UTC 2019
Hi,
Overall this looks good. Here are a few comments.
I agree with Chris and Daniel regarding naming.
On Fri, 2019-07-12 at 17:16 +0300, Oleg Vasilev wrote:
> Current, we have different sets of prime tests:
> - vgem+i915
> - amdgpu+i915
> - nouveau+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, while those devices have respective prime
> capabilities.
>
> Since vgem can be used as both exporter and importer, and there aren't
> any generic kms features which vgem doesn't support, it is sufficient
> to test DRIVER_ANY+DRIVER_VKMS.
>
> The first test is simple:
> 1. Exporter creates a dumb FB 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 | 250 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 251 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..f273a4f6
> --- /dev/null
> +++ b/tests/prime_generic.c
> @@ -0,0 +1,250 @@
> +/*
> + * 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>
> +
> +struct crc_info {
> + igt_crc_t crc;
> + char *str;
> + const char *name;
> +};
> +
> +static struct {
> + double r, g, b;
> + uint32_t color;
> + struct crc_info prime_crc, direct_crc;
> +} 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},
Style nit: add spaces before `}` too.
> +};
> +
> +IGT_TEST_DESCRIPTION("Generic tests, working with any prime device");
Maybe make it clear it's about KMS and prime?
> +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;
This returns a non-zero value when import is supported, but doesn't
return true. This can lead to strange bugs.
It's common to use !! to convert to a bool.
> +}
> +
> +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;
Ditto
> +}
> +
> +static void setup_display(int importer, igt_display_t *display, igt_output_t **output,
We can return the igt_output_t * instead of using an argument.
It's also probably a good idea to rename importer to e.g. importer_fd,
it's not immediately obvious this variable represents a file
descriptor.
> + enum pipe pipe)
> +{
> + 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);
> +}
> +
> +static void prepare_scratch(int exporter, struct vgem_bo *scratch,
> + drmModeModeInfo *mode, uint32_t color)
> +{
> + int i;
Nit: strictly speaking, this should be size_t
> + uint32_t *ptr;
> +
> + scratch->width = mode->hdisplay;
> + scratch->height = mode->vdisplay;
> + scratch->bpp = 32;
> + vgem_create(exporter, scratch);
The return value should be checked.
> + ptr = vgem_mmap(exporter, scratch, PROT_WRITE);
Ditto, we should check that ptr != NULL.
> + 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);
Maybe we could populate fb->strides[0] here, so that we don't need to
pass it around in import_fb.
> +}
> +
> +static void import_fb(int importer, struct igt_fb *fb,
> + int dmabuf_fd, uint32_t pitch)
> +{
> + uint32_t offsets[4], pitches[4], handles[4];
It's safer to zero-initialize these.
> + 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);
This assumes the first plane is a primary one. Actually requesting the
primary plane is safer:
primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
igt_assert(primary);
> + igt_plane_set_fb(primary, fb);
> + ret = igt_display_commit(display);
> +
> + igt_assert(ret == 0);
> +}
> +
> +static void collect_crc(int importer, struct igt_fb *fb, igt_display_t *display,
> + igt_output_t *output, igt_pipe_crc_t *pipe_crc,
> + uint32_t color, struct crc_info *info)
> +{
> + set_fb(fb, display, output);
> + igt_pipe_crc_collect_crc(pipe_crc, &info->crc);
> + info->str = igt_crc_to_string(&info->crc);
> + igt_debug("CRC through '%s' method for %#08x is %s\n",
> + info->name, color, info->str);
> + igt_remove_fb(importer, fb);
> +}
> +
> +static void test_crc(int exporter, int importer)
> +{
> + igt_display_t display;
> + igt_output_t *output;
> + igt_pipe_crc_t *pipe_crc;
> + enum pipe pipe = PIPE_A;
> + struct igt_fb fb;
> + int dmabuf_fd;
> + struct vgem_bo scratch; /* despite the name, it suits for any
> + * gem-compatible device
> + * TODO: rename
> + */
Can we initialize this to zero (= {0})? Since fields are populated
incrementally, it seems a little dangerous to use uninitialized memory.
> + int i, j;
> + drmModeModeInfo *mode;
> +
> + bool crc_equal = false;
> +
> + setup_display(importer, &display, &output, pipe);
> +
> + 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);
We should check dmabuf_fd >= 0.
> + gem_close(exporter, scratch.handle);
> +
> + prepare_fb(importer, &scratch, &fb);
> + import_fb(importer, &fb, dmabuf_fd, scratch.pitch);
We should release this FB after we're done.
> + close(dmabuf_fd);
> +
> + colors[i].prime_crc.name = "prime";
> + collect_crc(importer, &fb, &display, output,
> + pipe_crc, colors[i].color, &colors[i].prime_crc);
> +
> + 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);
Ditto, we should release this FB after we're done.
> +
> + colors[i].direct_crc.name = "direct";
> + collect_crc(importer, &fb, &display, output,
> + pipe_crc, colors[i].color, &colors[i].direct_crc);
> + }
> + 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_crc.str, colors[i].direct_crc.str);
> + free(colors[i].prime_crc.str);
> + free(colors[i].direct_crc.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.crc,
> + &colors[j].direct_crc.crc);
> + continue;
> + }
> + crc_equal = igt_check_crc_equal(&colors[i].prime_crc.crc,
> + &colors[j].direct_crc.crc);
> + igt_assert_f(!crc_equal, "CRC should be different");
> + }
> + }
> + igt_display_fini(&display);
> +}
> +
> +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();
Is this really needed?
> + }
> + igt_subtest("crc")
Please add a subtest description. See:
https://drm.pages.freedesktop.org/igt-gpu-tools/igt-gpu-tools-Core.html#igt-describe
> + run_test_crc(DRIVER_VGEM, DRIVER_ANY);
> +}
More information about the igt-dev
mailing list