On Fri, 2015-07-03 at 10:52 +0100, Tvrtko Ursulin wrote:
>
> On 07/01/2015 10:26 AM, ankitprasad.r.sharma@xxxxxxxxx wrote:
> > From: Ankitprasad Sharma <ankitprasad.r.sharma@xxxxxxxxx>
> >
> > This test validates the two parameters (size and flags) GEM_CREATE ioctl.
> >
> > v2: Added IGT_TEST_DESCRIPTION (Thomas Wood)
> >
> > Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> > ---
> > tests/Makefile.sources | 1 +
> > tests/gem_create.c | 181 +++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 182 insertions(+)
> > create mode 100644 tests/gem_create.c
> >
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index 324cbb5..f5790df 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -15,6 +15,7 @@ TESTS_progs_M = \
> > gem_close_race \
> > gem_concurrent_blit \
> > gem_concurrent_all \
> > + gem_create \
> > gem_cs_tlb \
> > gem_ctx_param_basic \
> > gem_ctx_bad_exec \
> > diff --git a/tests/gem_create.c b/tests/gem_create.c
> > new file mode 100644
> > index 0000000..6581035
> > --- /dev/null
> > +++ b/tests/gem_create.c
> > @@ -0,0 +1,181 @@
> > +/*
> > + * Copyright © 2011 Intel Corporation
>
> Year correct?
>
> > + *
> > + * 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.
> > + *
> > + * Authors:
> > + * Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> > + *
> > + */
> > +
> > +/** @file gem_create.c
> > + *
> > + * This is a test for the extended and old gem_create ioctl, that
> > + * includes allocation of object from stolen memory and shmem
>
> Full stop.
>
> > + *
> > + * The goal is to simply ensure that basics work and invalid input
> > + * combinations are rejected.
> > + */
> > +
> > +#include <stdlib.h>
> > +#include <sys/ioctl.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <fcntl.h>
> > +#include <inttypes.h>
> > +#include <errno.h>
> > +#include <sys/stat.h>
> > +#include <sys/time.h>
> > +#include <getopt.h>
> > +
> > +#include <drm.h>
> > +
> > +#include "ioctl_wrappers.h"
> > +#include "intel_bufmgr.h"
> > +#include "intel_batchbuffer.h"
> > +#include "intel_io.h"
> > +#include "intel_chipset.h"
> > +#include "igt_aux.h"
> > +#include "drmtest.h"
> > +#include "drm.h"
> > +#include "i915_drm.h"
> > +
> > +IGT_TEST_DESCRIPTION("This is a test for the extended & old gem_create ioctl,"
> > + " that includes allocation of object from stolen memory"
> > + " and shmem");
> > +
> > +#define CLEAR(s) memset(&s, 0, sizeof(s))
> > +#define SIZE 2048
>
> Since PAGE_SIZE is the minimum object I think it would be better for
> default size to be that. And then have an explicit test to see if
> requests smaller than minimum are correctly rounded up.
>
> > +#define OFFSET 3072
> > +
> > +static struct drm_intel_bufmgr *bufmgr;
> > +static struct intel_batchbuffer *batch;
>
> What is batchbuffer for? (Cleanup includes as well.)
>
> > +
> > +static void invalid_flag_test(int fd)
> > +{
> > + int ret = 0;
>
> Don't need to initialize.
>
> > +
> > +struct local_i915_gem_create_v2 {
> > + uint64_t size;
> > + uint32_t handle;
> > + uint32_t pad;
> > +#define I915_CREATE_PLACEMENT_STOLEN (1<<0)
> > + uint32_t flags;
> > +} create;
> > +
> > +#define LOCAL_IOCTL_I915_GEM_CREATE DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_CREATE, struct local_i915_gem_create_v2)
> > + gem_require_stolen_support(fd);
> > +
> > + create.handle = 0;
> > + create.size = SIZE;
> > + create.flags = ~I915_CREATE_PLACEMENT_STOLEN;
>
> Also ~0 should be rejected.
>
> > + ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CREATE, &create);
> > +
> > + igt_assert(ret <= 0);
> > +}
> > +
> > +static void invalid_size_test(int fd)
> > +{
> > + int handle;
> > +
> > + handle = __gem_create(fd, 0);
> > + igt_assert(handle == 0);
> > +}
> > +/* Creating an object with non-aligned size and trying to access offset of
> > + * value (size+x) & length y, where (size+x+y) <= object's last page boundary.
> > + * pwrite here must be successful.
> > + */
> > +static void valid_nonaligned_size(int fd)
> > +{
> > + int handle, i;
> > + uint32_t buf[1024];
> > +
> > + for (i = 0; i < 1024; i ++)
> > + buf[i] = 0xdead;
> > +
> > + handle = gem_create(fd, SIZE);
> > + igt_assert(handle != 0);
> > +
> > + gem_write(fd, handle, OFFSET, buf, 256);
> > +
> > + gem_close(fd, handle);
> > +}
>
> I would move this subtest to gem_pread or gem_pwrite - this one claims
> to be only about create so kind of doesn't belong here.
This subtest is not about pwrite or pread but to check the behavior of
gem_create on passing non-page-aligned value. I might not have been able
to clarify this through my comments. Will update that.
Just using pwrite as a mechanism here to validate gem_create.
Regards,
Ankit
>
> > +/* Creating an object with non-aligned size and trying to access offset of
> > + * value (size+x) & length y, where (size+x+y) > object's last page boundary.
> > + * pwrite here must result in failure.
> > + */
> > +static void invalid_nonaligned_size(int fd)
> > +{
> > + int handle, i;
> > + uint32_t buf[1024];
> > + struct drm_i915_gem_pwrite gem_pwrite;
> > +
> > + for (i = 0; i < 1024; i ++)
>
> Suggest avoiding hardcoding.
>
> > + buf[i] = 0xdead;
> > +
> > + handle = gem_create(fd, SIZE);
> > +
> > + CLEAR(gem_pwrite);
> > + gem_pwrite.handle = handle;
> > + gem_pwrite.offset = OFFSET;
> > + /* Out of bound access */
> > + gem_pwrite.size = 2048;
>
> Mixing up defines and hardcoding could be improved so the numbers are
> more obvious when reading this. Perhaps OFFSET should not be defined at
> all but expressed relative to SIZE at relevant places so the reader
> immediately know what is the test trying to do.
>
> > + gem_pwrite.data_ptr = (uintptr_t)buf;
> > + igt_assert(drmIoctl(fd, DRM_IOCTL_I915_GEM_PWRITE, &gem_pwrite) != 0);
> > +
> > + gem_close(fd, handle);
> > +}
> > +
> > +igt_main
> > +{
> > + int i, fd, gtt_size_total, gtt_size_mappable;
> > + uint32_t devid;
> > +
> > + igt_skip_on_simulation();
> > +
> > + igt_fixture {
> > + fd = drm_open_any();
> > + devid = intel_get_drm_devid(fd);
> > +
> > + drm_intel_get_aperture_sizes(fd, (size_t*)>t_size_total,
> > + (size_t*)>t_size_mappable);
>
> Doesn't look like the test needs to know this.
>
> > + bufmgr = drm_intel_bufmgr_gem_init(fd, 4096);
>
> Or this.
>
> > + batch = intel_batchbuffer_alloc(bufmgr, devid);
> > + }
> > +
> > + igt_subtest("stolen-invalid-flag")
> > + invalid_flag_test(fd);
> > +
> > + igt_subtest("stolen-invalid-size")
> > + invalid_size_test(fd);
> > +
> > + igt_subtest("stolen-valid-nonaligned")
> > + valid_nonaligned_size(fd);
> > +
> > + igt_subtest("stolen-invalid-nonaligned")
> > + invalid_nonaligned_size(fd);
> > +
> > + igt_fixture {
> > + intel_batchbuffer_free(batch);
> > + drm_intel_bufmgr_destroy(bufmgr);
> > + }
> > +}
> >
>
> Regards,
>
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
