[igt-dev] [PATCH i-g-t] panfrost: Test labeling functionality

[Steven Price]

On 28/05/2020 14:38, Rohan Garg wrote:
> Introduce tests to cover the new generic labeling ioctl's
> being reviewed here [1].
> 
> Signed-off-by: Rohan Garg <rohan.garg at collabora.com>
> 
> [1] https://patchwork.freedesktop.org/series/77267/
> 
> Signed-off-by: Rohan Garg <rohan.garg at collabora.com>

A few comments below.

> ---
>   include/drm-uapi/drm.h    |  23 ++++++-
>   tests/meson.build         |   1 +
>   tests/panfrost_bo_label.c | 129 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 152 insertions(+), 1 deletion(-)
>   create mode 100644 tests/panfrost_bo_label.c
> 
> diff --git a/include/drm-uapi/drm.h b/include/drm-uapi/drm.h
> index c7fd2a35..87124882 100644
> --- a/include/drm-uapi/drm.h
> +++ b/include/drm-uapi/drm.h
> @@ -620,6 +620,25 @@ struct drm_gem_open {
>   	__u64 size;
>   };
>   
> +/** struct drm_handle_label - ioctl argument for labelling BOs.
> + *
> + * This label's a BO with a userspace label
> + *
> + */
> +struct drm_handle_label {
> +	/** Handle for the object being labelled. */
> +	__u32 handle;
> +
> +	/** Label and label length (len includes the trailing NULL).
> +	 *  Label lenght *MUST* be smaller than PAGE_SIZE.
                   ^^^^^^
typo: s/lenght/length/

> +	 */
> +	__u32 len;
> +	__u64 label;
> +
> +	/** Flags .. Currently no flags are defined */
> +	__u32 flags;
> +};
> +
>   #define DRM_CAP_DUMB_BUFFER		0x1
>   #define DRM_CAP_VBLANK_HIGH_CRTC	0x2
>   #define DRM_CAP_DUMB_PREFERRED_DEPTH	0x3
> @@ -941,8 +960,10 @@ extern "C" {
>   #define DRM_IOCTL_SYNCOBJ_QUERY		DRM_IOWR(0xCB, struct drm_syncobj_timeline_array)
>   #define DRM_IOCTL_SYNCOBJ_TRANSFER	DRM_IOWR(0xCC, struct drm_syncobj_transfer)
>   #define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL	DRM_IOWR(0xCD, struct drm_syncobj_timeline_array)
> -
>   #define DRM_IOCTL_MODE_GETFB2		DRM_IOWR(0xCE, struct drm_mode_fb_cmd2)
> +#define DRM_IOCTL_HANDLE_SET_LABEL      DRM_IOWR(0xCF, struct drm_handle_label)
> +#define DRM_IOCTL_HANDLE_GET_LABEL      DRM_IOWR(0xD0, struct drm_handle_label)
> +
>   
>   /**
>    * Device specific ioctls should only be in their respective headers
> diff --git a/tests/meson.build b/tests/meson.build
> index e69bdb7d..ee91bf48 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -72,6 +72,7 @@ test_progs = [
>   	'kms_vblank',
>   	'kms_vrr',
>   	'meta_test',
> +        'panfrost_bo_label',

Looks like you've used spaces rather than tabs. Please also check the 
rest of the patch for correct spaces/tabs.

>   	'panfrost_get_param',
>   	'panfrost_gem_new',
>   	'panfrost_prime',
> diff --git a/tests/panfrost_bo_label.c b/tests/panfrost_bo_label.c
> new file mode 100644
> index 00000000..d3179458
> --- /dev/null
> +++ b/tests/panfrost_bo_label.c
> @@ -0,0 +1,129 @@
> +/*
> + * Copyright © 2020 Collabora Ltd.
> + *     Author: Rohan Garg <rohan.garg at collabora.com>
> + *
> + * 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_panfrost.h"
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <errno.h>
> +#include <sys/stat.h>
> +#include <sys/ioctl.h>
> +#include <poll.h>
> +#include "vc4_drm.h"
> +
> +static int
> +set_label(int fd, int handle, const char *label)
> +{
> +	struct drm_handle_label args = { 0 };
> +	if (label) {
> +		args.handle = handle;
> +		args.len = strlen(label) + 1;
> +		args.label= (uintptr_t)label;
> +	}
> +
> +	return drmIoctl(fd, DRM_IOCTL_HANDLE_SET_LABEL, &args);
> +}
> +
> +static const char*
> +get_label(int fd, int handle)
> +{
> +	struct drm_handle_label args = {
> +                .handle = handle,
> +		.len = 10,
> +		.label = (uintptr_t) malloc(sizeof(char*) * 10)
> +        };
> +
> +	drmIoctl(fd, DRM_IOCTL_HANDLE_GET_LABEL, &args);
> +
> +	if (!args.label)
> +		return NULL;

This seems out of place - assuming this is to check that malloc() 
succeeded then it would be normal to check before the ioctl call.

> +
> +	igt_assert(args.label);

Given the above check this can never fail.

Also nothing is checking the return from drmIoctl.

> +
> +        args.label = (uintptr_t) realloc(args.label, args.len);
> +
> +	drmIoctl(fd, DRM_IOCTL_HANDLE_GET_LABEL, &args);
> +
> +        return args.label;
> +}
> +
> +
> +igt_main
> +{
> +	int fd;
> +
> +	igt_fixture
> +		fd = drm_open_driver(DRIVER_ANY);
> +
> +	igt_subtest("set-label") {
> +		struct panfrost_bo *bo = igt_panfrost_gem_new(fd, 4096);
> +		int ret;
> +		ret = set_label(fd, bo->handle, "a test label");
> +		igt_assert(ret == 0);
> +
> +		ret = set_label(fd, bo->handle, "a new test label");
> +		igt_assert(ret == 0);
> +
> +		igt_panfrost_free_bo(fd, bo);
> +	}
> +
> +	igt_subtest("get-label") {
> +		struct panfrost_bo *bo = igt_panfrost_gem_new(fd, 4096);
> +		set_label(fd, bo->handle, "a test label");
> +		const char* label = get_label(fd, bo->handle);
> +		igt_assert(strcmp(label, "a test label") == 0);
> +		free(label);
> +		igt_panfrost_free_bo(fd, bo);
> +	}
> +
> +	igt_subtest("clear-label") {
> +		struct panfrost_bo *bo = igt_panfrost_gem_new(fd, 4096);
> +		set_label(fd, bo->handle, NULL);
> +		igt_assert_eq(NULL, get_label(fd, bo->handle));
> +		igt_panfrost_free_bo(fd, bo);
> +	}
> +
> +	igt_subtest("label-too-long") {
> +		char label[4096] = {[0 ... 4095] = 'a'};

Beware PAGE_SIZE isn't always 4096 bytes, e.g. see ARM64_64K_PAGES. You 
might want to try sysconf(_SC_PAGESIZE).

Steve

> +		int ret;
> +		struct panfrost_bo *bo = igt_panfrost_gem_new(fd, 4096);
> +		ret = set_label(fd, bo->handle, label);
> +		igt_assert_eq(-1, ret);
> +		igt_assert_eq(EINVAL, errno);
> +		igt_panfrost_free_bo(fd, bo);
> +	}
> +
> +	igt_subtest("set-bad-handle") {
> +		int ret = set_label(fd, 0xd0d0d0d0, "bad handle");
> +		igt_assert_eq(-1, ret);
> +		igt_assert_eq(ENOENT, errno);
> +	}
> +
> +	igt_fixture
> +		close(fd);
> +}
>