Re: [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf

Tvrtko Ursulin
2017-07-31T09:26:27Z

On 28/07/2017 13:07, Chris Wilson wrote:
> The idea is to implement read(dmabuf) and write(dambuf) so provide some
> tests.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> tests/Makefile.sources | 1 +
> tests/prime_rw.c | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 180 insertions(+)
> create mode 100644 tests/prime_rw.c
>
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 5b98a5a3..18b99768 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -225,6 +225,7 @@ TESTS_progs = \
> prime_mmap \
> prime_mmap_coherency \
> prime_mmap_kms \
> + prime_rw \
> prime_self_import \
> prime_udl \
> prime_vgem \
> diff --git a/tests/prime_rw.c b/tests/prime_rw.c
> new file mode 100644
> index 00000000..65abec5d
> --- /dev/null
> +++ b/tests/prime_rw.c
> @@ -0,0 +1,179 @@
> +/*
> + * Copyright © 2017 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.
> + */
> +
> +/*
> + * Testcase: Check whether read()/write()ing dma-buf works
> + */
> +#define _GNU_SOURCE
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +
> +#include "drmtest.h"
> +#include "ioctl_wrappers.h"
> +
> +static int create_dmabuf(int fd, int size)
> +{
> + uint32_t handle;
> + int dmabuf;
> +
> + handle = gem_create(fd, size);
> + dmabuf = prime_handle_to_fd_for_mmap(fd, handle);
> + gem_close(fd, handle);
> +
> + return dmabuf;
> +}
> +
> +static bool has_dma_buf_read(int fd)
> +{
> + int dmabuf;
> + int ret;
> +
> + dmabuf = create_dmabuf(fd, 4096);
> + ret = read(dmabuf, NULL, 0);
> + close(dmabuf);
> +
> + errno = 0;
> + return ret == 0;
> +}
> +
> +static bool has_dma_buf_write(int fd)
> +{
> + int dmabuf;
> + int ret;
> +
> + dmabuf = create_dmabuf(fd, 4096);
> + ret = write(dmabuf, NULL, 0);
> + close(dmabuf);
> +
> + errno = 0;
> + return ret == 0;
> +}
> +
> +static void test_read(int fd)
> +{
> + uint32_t buf[4096];
> + uint32_t *ptr;
> + int i, len, offset;
> + int dmabuf;
> +
> + dmabuf = create_dmabuf(fd, sizeof(buf));
> + igt_assert(dmabuf != -1);
> +
> + ptr = mmap(NULL, sizeof(buf), PROT_WRITE, MAP_SHARED, dmabuf, 0);
> + igt_assert(ptr != MAP_FAILED);
> +
> + prime_sync_start(dmabuf, true);
> + for (i = 0; i < ARRAY_SIZE(buf); i++)
> + ptr[i] = i;
> + prime_sync_end(dmabuf, true);
> +
> + munmap(ptr, sizeof(buf));
> +
> + prime_sync_start(dmabuf, false);
> + for (len = 1; len <= 4096; len <<= 1) {
> + const int sz = len * sizeof(buf[0]);
> +
> + offset = len/2;

Spaces for coding style.

> + lseek(dmabuf, offset * sizeof(buf[0]), SEEK_SET);
> + for (; offset + len <= 4096; offset += len) {
> + igt_assert_eq(read(dmabuf, buf, sz), sz);
> + for (i = 0; i < len; i++)
> + igt_assert_eq(buf[i], offset + i);
> + }
> + igt_assert_eq(read(dmabuf, buf, sz),
> + (4096 - offset) * sizeof(buf[0]));
> + igt_assert_eq(read(dmabuf, buf, sz), 0);
> + }
> + prime_sync_end(dmabuf, false);

Why so elaborate reading pattern in a basic test? Could you drop a
comment before the block describing the pattern?

> +
> + close(dmabuf);
> +}
> +
> +static void test_write(int fd)
> +{
> + uint32_t buf[4096];
> + uint32_t *ptr;
> + int i, len, offset;
> + int dmabuf;
> +
> + dmabuf = create_dmabuf(fd, sizeof(buf));
> + igt_assert(dmabuf != -1);
> +
> + for (i = 0; i < ARRAY_SIZE(buf); i++)
> + buf[i] = i;
> +
> + ptr = mmap(NULL, sizeof(buf), PROT_READ, MAP_SHARED, dmabuf, 0);
> + igt_assert(ptr != MAP_FAILED);
> +
> + prime_sync_start(dmabuf, true);
> + for (len = 1; len <= 4096; len <<= 1) {
> + const int sz = len * sizeof(buf[0]);
> +
> + offset = len/2;
> + lseek(dmabuf, offset * sizeof(buf[0]), SEEK_SET);
> + for (; offset + len <= 4096; offset += len) {
> + igt_assert_eq(write(dmabuf, buf, sz), sz);
> + for (i = 0; i < len; i++)
> + igt_assert_eq(ptr[i + offset], i);
> + }
> + igt_assert_eq(write(dmabuf, buf, sz),
> + (4096 - offset) * sizeof(buf[0]));
> + igt_assert_eq(write(dmabuf, buf, sz), 0);
> + }
> + prime_sync_end(dmabuf, true);

Same comments as above.

Also, I don't know what is the prime API supposed to be when mixing
read/write and mmap accesses? Is intermixing with no
prime_sync_start/end every time OK?

> +
> + munmap(ptr, sizeof(buf));
> +
> + close(dmabuf);
> +}
> +
> +igt_main
> +{
> + int fd = -1;
> +
> + igt_fixture {
> + fd = drm_open_driver(DRIVER_INTEL);
> + }
> +
> + igt_subtest_group {
> + igt_fixture {
> + igt_require((has_dma_buf_read(fd)));
> + }
> +
> + igt_subtest("basic-read")
> + test_read(fd);
> + }
> +
> + igt_subtest_group {
> + igt_fixture {
> + igt_require((has_dma_buf_write(fd)));
> + }
> +
> + igt_subtest("basic-write")
> + test_write(fd);
> + }
> +
> + igt_fixture
> + close(fd);
> +}
>

Regards,

Tvrtko