Re: [igt-dev] [PATCH 2/2] tests/prime_generic: add vendor-agnostic prime tests

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jul 11, 2019 at 11:09 AM Vasilev, Oleg <oleg.vasilev@intel.com> wrote:
>
> On Wed, 2019-07-10 at 18:35 +0200, Daniel Vetter wrote:
> > On Thu, Jul 04, 2019 at 12:56:04PM +0300, Oleg Vasilev 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@gmail.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Signed-off-by: Oleg Vasilev <oleg.vasilev@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 */
> > > +   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);
> > > +
> > > +   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);
> > > +           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);
> > > +           igt_debug("Direct CRC for %#08x is %s\n",
> > > +                     colors[i].color, colors[i].direct_str);
> > > +           igt_remove_fb(importer, &fb);
> > > +   }
> > > +   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_c
> > > rc,
> > > +                                                &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);
> >
> > This isn't any more generic than what we have e.g. in prime_vgem
> > already.
>
> Here non-generic part is only those 10 lines of subtests definitions.
> The rest is common code.
>
> I guess, this separate subtest definitions can document the device
> pairs the test is expected to work + we can blacklist tests which we
> don't have the hardware for.

But we already have tests for specific pairs, which can also test
specific stuff. In general we don't do tests with -gen9, gen11, -gen8
prefixes either. This here would be a first I'd say.

> > The way to do a generic kms testcase is to use DRIVER_ANY (yes some
> > of the
> > earlier conversion unfortunately used stuff like DRIVER_INTEL |
> > DRIVER_AMDGPU). With that you also don't need your library prep
> > patch,
> > because no need for a DRIVER_VKMS.
>
> But how can we open two different devices with DRIVER_ANY? If I put
>
> fd = drm_open_driver(DRIVER_ANY)
> fd2 = drm_open_driver(DRIVER_ANY)
>
> I get the same device opened two times, do I?

One of them would be DRIVER_VGEM. And I think we already make sure
that you don't accidentally get vgem for DRIVER_ANY. Otherwise I guess
we'd need a DRIVER_ANY_BUT_VGEM. Well really what we want is
DRIVER_ANY_KMS here, which would also exclude vgem. So if it doesn't
work already, small patch needed in lib/, but DRIVER_VKMS also needs
that. And I really don't want more DRIVER_FOO defines for a plain old
kms-only driver. kms is supposed to be a shared cross-driver standard,
if we can't even write shared testcases it's just epic fail :-)

> > Imo best to start out by converting prime_vgem.c for this, and adding
> > for
> > the i915 specific tests (but _only_ for those) a specific "is this
> > i915"
> > check, so we skip those tests. And replace DRIVER_INTEL by
> > DRIVER_ANY.
> > Well, anything except vgem :-)
>
> If I am not mistaken, all tests in prime_vgem are intel-specific :D
>
> Some ideas can be taken from it, but anyway, it probably needs a deep
> rewrite.
>
> And prime_vgem use only vgem as an exporter device. Here we can use any
> device. Sure, some tests would eventually use vgem-specific features,
> but some could remain more generic.

Maybe I wasn't all that clear then, I think what we want to test in a
generic prime test is vgem+DRIVER_ANY_KMS. Then drivers can test the
render side using DRIVER_FOO+vgem. This way you only need to have 1
generic kms test, plus N tests for all the rendering drivers. Instead
of M x N testcases like you're proposing here. vgem is just the
abstraction layer/mocking layer we use to separate these two worlds.
And of course for specific sharing you can still do that, e.g. to test
dma engine compatibility between amdgpu and i915, or nouveau and i915.
But those tests don't need to involve kms.

And yeah prime_vgem might need to be split into prime_i915_vgem and
prime_kms_vgem. But all the display related testcases should be
possible to extract into such a prime_kms_vgem framework.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev