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

["Ser, Simon" <simon.ser@intel.com>]

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@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 | 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);
> +}
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev