Re: [igt-dev] [PATCH v16 2/8] lib: ioctl_wrappers: reach engines by index as well

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 28/03/2019 19:22, Andi Shyti wrote:
> With the new engine query method engines are reachable through
> an index and context they are combined with.
> 
> The 'gem_has_ring()' becomes 'gem_context_has_engine()' that
> requires the index that the engine is mapped within the driver.
> The function has been moved from lib/ioctl_wappers to
> lib/i915/gem_context where it is more appropriate.
> 
> The previous 'gem_has_ring()' function becomes a wrapper to the
> new 'gem_context_has_engine()'.
> 
> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   lib/i915/gem_context.c | 28 ++++++++++++++++++++++++++++
>   lib/i915/gem_context.h |  2 ++
>   lib/ioctl_wrappers.c   | 19 -------------------
>   lib/ioctl_wrappers.h   |  3 ++-
>   4 files changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/i915/gem_context.c b/lib/i915/gem_context.c
> index 8b4d5b704650..732f66b4273e 100644
> --- a/lib/i915/gem_context.c
> +++ b/lib/i915/gem_context.c
> @@ -271,3 +271,31 @@ void gem_context_set_priority(int fd, uint32_t ctx_id, int prio)
>   {
>   	igt_assert_eq(__gem_context_set_priority(fd, ctx_id, prio), 0);
>   }
> +
> +bool gem_context_has_engine(int fd, uint64_t ctx, uint64_t engine)
> +{
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct drm_i915_gem_exec_object2 exec;
> +
> +	/*
> +	 * 'engine' value can either store an execbuf engine selector
> +	 * or a context map index; for the latter case we do not expect
> +	 * to have any value at bit 13 and 14 (BSD1/2 selector),
> +	 * therefore, we assume that the following check is safe it wouldn't
> +	 * produce any result and we don't exepct anyway to have more than
> +	 * one engine mapped in 'unit64_t engine'.

typo in expect and uint.

I think here you meant that we don't expect more than (1 << 13 - 1) engines?

> +	 */
> +	if ((engine & ~(3<<13)) == I915_EXEC_BSD) {
> +		if (engine & (3 << 13) && !gem_has_bsd2(fd))
> +			return false;
> +	}
> +
> +	memset(&exec, 0, sizeof(exec));
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = to_user_pointer(&exec);
> +	execbuf.buffer_count = 1;
> +	execbuf.flags = engine;
> +	execbuf.rsvd1 = ctx;
> +
> +	return __gem_execbuf(fd, &execbuf) == -ENOENT;
> +}
> diff --git a/lib/i915/gem_context.h b/lib/i915/gem_context.h
> index aef68dda6b26..d4e8fec3acf0 100644
> --- a/lib/i915/gem_context.h
> +++ b/lib/i915/gem_context.h
> @@ -45,4 +45,6 @@ int __gem_context_get_param(int fd, struct drm_i915_gem_context_param *p);
>   int __gem_context_set_priority(int fd, uint32_t ctx, int prio);
>   void gem_context_set_priority(int fd, uint32_t ctx, int prio);
>   
> +bool gem_context_has_engine(int fd, uint64_t ctx, uint64_t engine);
> +
>   #endif /* GEM_CONTEXT_H */
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index a66eb4bc0f4f..3f8f0874bc4b 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -1253,25 +1253,6 @@ void igt_require_gem(int fd)
>   	igt_require_f(err == 0, "Unresponsive i915/GEM device\n");
>   }
>   
> -bool gem_has_ring(int fd, unsigned ring)
> -{
> -	struct drm_i915_gem_execbuffer2 execbuf;
> -	struct drm_i915_gem_exec_object2 exec;
> -
> -	/* silly ABI, the kernel thinks everyone who has BSD also has BSD2 */
> -	if ((ring & ~(3<<13)) == I915_EXEC_BSD) {
> -		if (ring & (3 << 13) && !gem_has_bsd2(fd))
> -			return false;
> -	}
> -
> -	memset(&exec, 0, sizeof(exec));
> -	memset(&execbuf, 0, sizeof(execbuf));
> -	execbuf.buffers_ptr = to_user_pointer(&exec);
> -	execbuf.buffer_count = 1;
> -	execbuf.flags = ring;
> -	return __gem_execbuf(fd, &execbuf) == -ENOENT;
> -}
> -
>   /**
>    * gem_require_ring:
>    * @fd: open i915 drm file descriptor
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index ad93daffcfd5..e712f1973142 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -142,11 +142,12 @@ bool gem_has_exec_fence(int fd);
>   
>   /* check functions which auto-skip tests by calling igt_skip() */
>   void gem_require_caching(int fd);
> -bool gem_has_ring(int fd, unsigned ring);
>   void gem_require_ring(int fd, unsigned ring);
>   bool gem_has_mocs_registers(int fd);
>   void gem_require_mocs_registers(int fd);
>   
> +#define gem_has_ring(f, r) gem_context_has_engine(f, 0, r)
> +
>   /* prime */
>   struct local_dma_buf_sync {
>   	uint64_t flags;
> 

With the comment tidied:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev