| Message ID | 20200528133835.5087-1-rohan.garg@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] panfrost: Test labeling functionality | expand |
Hi Rohan, On Thu, 28 May 2020 at 14:38, Rohan Garg <rohan.garg@collabora.com> wrote: > > Introduce tests to cover the new generic labeling ioctl's > being reviewed here [1]. > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com> > > [1] https://patchwork.freedesktop.org/series/77267/ > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com> > --- > 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 > Listing high-level comments, haven't looked at the test itself: - make/name the test generic - I would imagine we can use dumb buffer on panfrost, vc4 or others - catch the missing locking - a) two writers, b) missing read lock would trigger user-after-free - use invalid handles - to highlight the mismatched gem_object_put (as mentioned off-list) The last two categories would produce various kernel warnings/warn/bugs with earlier revisions. Might be a good idea to run the test against those, to ensure the test triggers correctly. -Emil
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@collabora.com> > > [1] https://patchwork.freedesktop.org/series/77267/ > > Signed-off-by: Rohan Garg <rohan.garg@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@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); > +} >
On Thu, May 28, 2020 at 03:38:35PM +0200, Rohan Garg wrote: > Introduce tests to cover the new generic labeling ioctl's > being reviewed here [1]. > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com> > > [1] https://patchwork.freedesktop.org/series/77267/ > > Signed-off-by: Rohan Garg <rohan.garg@collabora.com> > --- > include/drm-uapi/drm.h | 23 ++++++- Please update drm.h in a separate commit, and state which kernel commit (and from which repo) it's copied from. Examples can be seen in the git log for the file.
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. + */ + __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', '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@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; + + igt_assert(args.label); + + 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'}; + 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); +}