[Intel-gfx] [PATCH 3/3] igt/gem_create: Test to validate parameters for GEM_CREATE ioctl

[Ankitprasad Sharma]

On Fri, 2015-07-03 at 10:52 +0100, Tvrtko Ursulin wrote:
> 
> On 07/01/2015 10:26 AM, ankitprasad.r.sharma at intel.com wrote:
> > From: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> >
> > 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*)&gtt_size_total,
> > +				(size_t*)&gtt_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