Re: [PATCH i-g-t v3] lib: Move __gem_context_create to common ioctl wrapper library.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Oct 25, 2017 at 05:28:39PM -0700, Antonio Argenziano wrote:
> This patch adds a context creation ioctl wrapper that returns the error
> for the caller to consume. Multiple tests that implemented this already,
> have been changed to use the new library function.
> 
> v2:
> 	- Add gem_require_contexts() to check for contexts support (Chris)
> 
> v3:
> 	- Add gem_has_contexts to check for contexts support and change
> 	  gem_require_contexts to skip if contests support is not available.
> 	  (Chris)
> 
> Signed-off-by: Antonio Argenziano <antonio.argenziano@xxxxxxxxx>
> 
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> ---
>  benchmarks/gem_exec_ctx.c   |  6 ++---
>  benchmarks/gem_exec_trace.c |  4 +--
>  lib/i915/gem_context.c      | 62 +++++++++++++++++++++++++++++++++++++--------
>  lib/i915/gem_context.h      |  3 +++
>  tests/gem_ctx_create.c      | 10 ++++----
>  tests/gem_ctx_switch.c      | 13 ----------
>  tests/gem_eio.c             | 13 +---------
>  tests/gem_exec_await.c      | 14 ++--------
>  tests/gem_exec_nop.c        | 13 ----------
>  tests/gem_exec_parallel.c   | 15 +++--------
>  tests/gem_exec_reuse.c      | 13 ----------
>  tests/gem_exec_whisper.c    | 13 ----------
>  12 files changed, 71 insertions(+), 108 deletions(-)
> 
> diff --git a/benchmarks/gem_exec_ctx.c b/benchmarks/gem_exec_ctx.c
> index 0eac04b0..a1c6e5d7 100644
> --- a/benchmarks/gem_exec_ctx.c
> +++ b/benchmarks/gem_exec_ctx.c
> @@ -64,7 +64,7 @@ static uint32_t batch(int fd)
>  	return handle;
>  }
>  
> -static uint32_t __gem_context_create(int fd)
> +static uint32_t __gem_context_create_local(int fd)

We only need this _local helper in negative parameters checks (in
tests/gem_ctx_create - and perhaps we can open-code those two ioctl calls?).
Both gem_exec_trace, gem_exec_ctx can use the helpers from lib.

>  {
>  	struct drm_i915_gem_context_create create;
>  
> @@ -101,7 +101,7 @@ static int loop(unsigned ring,
>  	execbuf.flags |= LOCAL_I915_EXEC_HANDLE_LUT;
>  	execbuf.flags |= LOCAL_I915_EXEC_NO_RELOC;
>  	if (mode != DEFAULT) {
> -		execbuf.rsvd1 = __gem_context_create(fd);
> +		execbuf.rsvd1 = __gem_context_create_local(fd);
>  		if (execbuf.rsvd1 == 0)
>  			return 77;
>  	}
> @@ -125,7 +125,7 @@ static int loop(unsigned ring,
>  			uint32_t ctx = 0;
>  
>  			if (mode != DEFAULT && mode != NOP) {
> -				execbuf.rsvd1 = __gem_context_create(fd);
> +				execbuf.rsvd1 = __gem_context_create_local(fd);
>  				ctx = gem_context_create(fd);
>  			}
>  
> diff --git a/benchmarks/gem_exec_trace.c b/benchmarks/gem_exec_trace.c
> index 12577649..2724ee92 100644
> --- a/benchmarks/gem_exec_trace.c
> +++ b/benchmarks/gem_exec_trace.c
> @@ -105,7 +105,7 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
>  	return 1e3*(end->tv_sec - start->tv_sec) + 1e-6*(end->tv_nsec - start->tv_nsec);
>  }
>  
> -static uint32_t __gem_context_create(int fd)
> +static uint32_t __gem_context_create_local(int fd)
>  {
>  	struct drm_i915_gem_context_create arg = {};
>  	drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &arg);
> @@ -216,7 +216,7 @@ static double replay(const char *filename, long nop, long range)
>  				num_ctx = new_ctx;
>  			}
>  
> -			ctx[t->handle] = __gem_context_create(fd);
> +			ctx[t->handle] = __gem_context_create_local(fd);
>  			break;
>  		}
>  	case DEL_CTX:
> diff --git a/lib/i915/gem_context.c b/lib/i915/gem_context.c
> index 6d9edf5e..a6c37051 100644
> --- a/lib/i915/gem_context.c
> +++ b/lib/i915/gem_context.c
> @@ -43,6 +43,22 @@
>   * software features improving submission model (context priority).
>   */
>  
> +int __gem_context_create(int fd, uint32_t *ctx_id)
> +{
> +       struct drm_i915_gem_context_create create;
> +       int err = 0;
> +
> +       memset(&create, 0, sizeof(create));
> +       if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &create) == 0)
> +               *ctx_id = create.ctx_id;
> +       else
> +               err = -errno;
> +
> +       errno = 0;
> +       return err;
> +}
> +
> +

Stray newline

>  /**
>   * gem_context_create:
>   * @fd: open i915 drm file descriptor
> @@ -55,18 +71,44 @@
>   */
>  uint32_t gem_context_create(int fd)
>  {
> -	struct drm_i915_gem_context_create create;
> +	uint32_t ctx_id;
> +	gem_require_contexts(fd);
>  
> -	memset(&create, 0, sizeof(create));
> -	if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &create)) {
> -		int err = -errno;
> -		igt_skip_on(err == -ENODEV || errno == -EINVAL);
> -		igt_assert_eq(err, 0);
> -	}
> -	igt_assert(create.ctx_id != 0);
> -	errno = 0;
> +	igt_assert_eq(__gem_context_create(fd, &ctx_id), 0);
> +	igt_assert(ctx_id != 0);
> +
> +	return ctx_id;
> +}
>  
> -	return create.ctx_id;
> +/**
> + * gem_has_contexts:
> + * @fd: open i915 drm file descriptor
> + *
> + * Queries whether context creation is supported or not.
> + *
> + * Returns: Context creation availability.
> + */
> +bool gem_has_contexts(int fd)
> +{
> +	uint32_t ctx_id = 0;
> +
> +	__gem_context_create(fd, &ctx_id);
> +	if (ctx_id)
> +		gem_context_destroy(fd, ctx_id);
> +
> +	return ctx_id;
> +}
> +
> +/**
> + * gem_require_contexts:
> + * @fd: open i915 drm file descriptor
> + *
> + * This helper will automatically skip the test on platforms where context
> + * support is not available.
> + */
> +void gem_require_contexts(int fd)
> +{
> +	igt_require(gem_has_contexts(fd));
>  }

Since we're exporting all of the functions, perhaps we can move
gem_request_contexts and gem_has_contexts, to keep create/destroy
pair next to each other?

>  
>  int __gem_context_destroy(int fd, uint32_t ctx_id)
> diff --git a/lib/i915/gem_context.h b/lib/i915/gem_context.h
> index a2339c4b..efff96b6 100644
> --- a/lib/i915/gem_context.h
> +++ b/lib/i915/gem_context.h
> @@ -24,6 +24,7 @@
>  #ifndef GEM_CONTEXT_H
>  #define GEM_CONTEXT_H
>  
> +int __gem_context_create(int fd, uint32_t *ctx_id);
>  uint32_t gem_context_create(int fd);
>  void gem_context_destroy(int fd, uint32_t ctx_id);
>  int __gem_context_destroy(int fd, uint32_t ctx_id);

Move __gem_context_create under gem_context_create to be consistent with
gem_context_destroy, but that's just my OCD talking.

> @@ -38,6 +39,8 @@ struct local_i915_gem_context_param {
>  #define LOCAL_CONTEXT_PARAM_BANNABLE	0x5
>  	uint64_t value;
>  };
> +bool gem_has_contexts(int fd);
> +void gem_require_contexts(int fd);
>  void gem_context_require_bannable(int fd);
>  void gem_context_require_param(int fd, uint64_t param);
>  void gem_context_get_param(int fd, struct local_i915_gem_context_param *p);
> diff --git a/tests/gem_ctx_create.c b/tests/gem_ctx_create.c
> index ae0825a1..abe9a32e 100644
> --- a/tests/gem_ctx_create.c
> +++ b/tests/gem_ctx_create.c
> @@ -45,7 +45,7 @@ static unsigned all_nengine;
>  static unsigned ppgtt_engines[16];
>  static unsigned ppgtt_nengine;
>  
> -static int __gem_context_create(int fd, struct drm_i915_gem_context_create *arg)
> +static int __gem_context_create_local(int fd, struct drm_i915_gem_context_create *arg)
>  {
>  	int ret = 0;
>  	if (drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, arg))
> @@ -255,7 +255,7 @@ static void maximum(int fd, int ncpus, unsigned mode)
>  
>  		err = -ENOMEM;
>  		if (avail_mem > (count + 1) * ctx_size)
> -			err = __gem_context_create(fd, &create);
> +			err = __gem_context_create_local(fd, &create);

We can also use the helper from lib here.

With that:

Reviewed-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>

-Michał
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux