[Mesa-dev] [PATCH v3 3/3] anv: implement VK_EXT_global_priority extension
Tapani Pälli
tapani.palli at intel.com
Tue Jan 23 06:12:27 UTC 2018
On 01/22/2018 09:01 PM, Jason Ekstrand wrote:
> On Mon, Jan 22, 2018 at 4:29 AM, Tapani Pälli <tapani.palli at intel.com
> <mailto:tapani.palli at intel.com>> wrote:
>
> v2: add ANV_CONTEXT_REALTIME_PRIORITY (Chris)
> use unreachable with unknown priority (Samuel)
>
> v3: add stubs in gem_stubs.c (Emil)
> use priority defines from gen_defines.h
>
> Signed-off-by: Tapani Pälli <tapani.palli at intel.com
> <mailto:tapani.palli at intel.com>>
> Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com
> <mailto:siglesias at igalia.com>> (v2)
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk
> <mailto:chris at chris-wilson.co.uk>> (v2)
> ---
> src/intel/vulkan/anv_device.c | 25 +++++++++++++++++++
> src/intel/vulkan/anv_extensions.py | 2 ++
> src/intel/vulkan/anv_gem.c | 51
> ++++++++++++++++++++++++++++++++++++++
> src/intel/vulkan/anv_gem_stubs.c | 10 ++++++++
> src/intel/vulkan/anv_private.h | 3 +++
> 5 files changed, 91 insertions(+)
>
> diff --git a/src/intel/vulkan/anv_device.c
> b/src/intel/vulkan/anv_device.c
> index 777abd8757..42ebc19f2b 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -369,6 +369,9 @@ anv_physical_device_init(struct
> anv_physical_device *device,
> device->has_syncobj_wait = device->has_syncobj &&
> anv_gem_supports_syncobj_wait(fd);
>
> + if (anv_gem_has_context_priority(fd))
> + device->has_context_priority = true;
> +
> bool swizzled = anv_gem_get_bit6_swizzle(fd, I915_TILING_X);
>
> /* Starting with Gen10, the timestamp frequency of the command
> streamer may
> @@ -1205,6 +1208,15 @@ VkResult anv_CreateDevice(
> }
> }
>
> + /* Check if client specified queue priority. */
> + const VkDeviceQueueGlobalPriorityCreateInfoEXT *queue_priority =
> + vk_find_struct_const(pCreateInfo->pQueueCreateInfos[0].pNext,
> +
> DEVICE_QUEUE_GLOBAL_PRIORITY_CREATE_INFO_EXT);
> +
> + VkQueueGlobalPriorityEXT priority =
> + queue_priority ? queue_priority->globalPriority :
> + VK_QUEUE_GLOBAL_PRIORITY_MEDIUM_EXT;
> +
> device = vk_alloc2(&physical_device->instance->alloc, pAllocator,
> sizeof(*device), 8,
> VK_SYSTEM_ALLOCATION_SCOPE_DEVICE);
> @@ -1234,6 +1246,19 @@ VkResult anv_CreateDevice(
> goto fail_fd;
> }
>
> + /* As per spec, the driver implementation may deny requests to
> acquire
> + * a priority above the default priority (MEDIUM) if the caller
> does not
> + * have sufficient privileges. In this scenario
> VK_ERROR_NOT_PERMITTED_EXT
> + * is returned.
> + */
> + if (physical_device->has_context_priority) {
> + int err = anv_gem_set_context_priority(device, priority);
> + if (err != 0 && priority > VK_QUEUE_GLOBAL_PRIORITY_MEDIUM_EXT) {
> + result = vk_error(VK_ERROR_NOT_PERMITTED_EXT);
> + goto fail_fd;
> + }
> + }
> +
> device->info = physical_device->info;
> device->isl_dev = physical_device->isl_dev;
>
> diff --git a/src/intel/vulkan/anv_extensions.py
> b/src/intel/vulkan/anv_extensions.py
> index adfebca985..aacf39248f 100644
> --- a/src/intel/vulkan/anv_extensions.py
> +++ b/src/intel/vulkan/anv_extensions.py
> @@ -86,6 +86,8 @@ EXTENSIONS = [
> Extension('VK_KHX_multiview', 1, True),
> Extension('VK_EXT_debug_report', 8, True),
> Extension('VK_EXT_external_memory_dma_buf', 1, True),
> + Extension('VK_EXT_global_priority', 1,
> + 'device->has_context_priority'),
> ]
>
> class VkVersion:
> diff --git a/src/intel/vulkan/anv_gem.c b/src/intel/vulkan/anv_gem.c
> index 34c0989108..7f83820429 100644
> --- a/src/intel/vulkan/anv_gem.c
> +++ b/src/intel/vulkan/anv_gem.c
> @@ -30,6 +30,7 @@
> #include <fcntl.h>
>
> #include "anv_private.h"
> +#include "common/gen_defines.h"
>
> static int
> anv_ioctl(int fd, unsigned long request, void *arg)
> @@ -302,6 +303,56 @@ close_and_return:
> return swizzled;
> }
>
> +static int
> +vk_priority_to_anv(int priority)
> +{
> + switch (priority) {
> + case VK_QUEUE_GLOBAL_PRIORITY_LOW_EXT:
> + return GEN_CONTEXT_LOW_PRIORITY;
> + case VK_QUEUE_GLOBAL_PRIORITY_MEDIUM_EXT:
> + return GEN_CONTEXT_MEDIUM_PRIORITY;
> + case VK_QUEUE_GLOBAL_PRIORITY_HIGH_EXT:
> + return GEN_CONTEXT_HIGH_PRIORITY;
> + case VK_QUEUE_GLOBAL_PRIORITY_REALTIME_EXT:
> + return GEN_CONTEXT_REALTIME_PRIORITY;
> + default:
> + unreachable("Invalid priority");
> + }
> +}
>
>
> I think I'd rather have the conversion in anv_device.c and just make the
> anv_gem functions take an i915 priority.
ok will change that
> Other than that, and a couple other nits, this looks good to me.
>
> One other question, do we have tests? I quickly searched the piglit
> list and didn't see any. Writing a crucible test shouldn't be that
I wrote a small API test here:
https://cgit.freedesktop.org/~tpalli/crucible/commit/?h=VK_EXT_global_priority
> hard. You just have to submit a bunch of command buffers and show that
> they get re-ordered to favor the higher-priority context. You could do
> that with a bunch of compute shader invocations that "take a number"
> from a shared atomic or something like that.
I think overall problem with complex run time test is that spec is
written like 'will attempt' and 'may retain' which means driver does not
promise to do anything. So basically we can test it, but test must pass
whatever happens.
Having said that I've had plans to finish my EGL test for same
functionality. Plan there was to queue some complex work for 2 different
priority contexts and then query time used to complete work
(TIME_ELAPSED_EXT) and hopefully this would tell if higher priority
wins. Do you think similar test would work with Vulkan (or at all)?
> The current func.sync.semaphore-fd tests should also probably be
> modified to use it. They currently use queue priorities in an attempt
> to make things to out-of-sync and, when I was testing semaphores, I had
> that hooked up to the context priority. Now that there's a proper
> extension for this, we should use that.
ok will take a look at this one.
> +
> +static int
> +_anv_gem_set_context_priority(int fd,
> + int context_id,
> + int priority)
> +{
> + struct drm_i915_gem_context_param p = {
> + .ctx_id = context_id,
> + .param = I915_CONTEXT_PARAM_PRIORITY,
> + .value = vk_priority_to_anv(priority),
> + };
> + int err = 0;
> +
> + if (anv_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM, &p))
> + err = -errno;
> +
> + return err;
> +}
>
>
> We may as well make this anv_gem_set_context_param, similar to
> anv_gem_get_context_param. That way we can re-use it easier in the future.
>
> +
> +int
> +anv_gem_set_context_priority(struct anv_device *device,
> + int priority)
> +{
> + return _anv_gem_set_context_priority(device->fd, device->context_id,
> + priority);
> +}
> +
> +bool
> +anv_gem_has_context_priority(int fd)
> +{
> + return !_anv_gem_set_context_priority(fd, 0,
> +
> VK_QUEUE_GLOBAL_PRIORITY_MEDIUM_EXT);
> +}
> +
> int
> anv_gem_create_context(struct anv_device *device)
> {
> diff --git a/src/intel/vulkan/anv_gem_stubs.c
> b/src/intel/vulkan/anv_gem_stubs.c
> index 26eb5c8a61..ad27e877a2 100644
> --- a/src/intel/vulkan/anv_gem_stubs.c
> +++ b/src/intel/vulkan/anv_gem_stubs.c
> @@ -152,6 +152,16 @@ anv_gem_get_context_param(int fd, int context,
> uint32_t param, uint64_t *value)
> unreachable("Unused");
> }
>
> +bool anv_gem_has_context_priority(int fd)
> +{
> + unreachable("Unused");
> +}
> +
> +int anv_gem_set_context_priority(struct anv_device *device, int
> priority)
> +{
> + unreachable("Unused");
> +}
> +
> int
> anv_gem_get_aperture(int fd, uint64_t *size)
> {
> diff --git a/src/intel/vulkan/anv_private.h
> b/src/intel/vulkan/anv_private.h
> index ed711e9434..d3f74eb084 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -768,6 +768,7 @@ struct anv_physical_device {
> bool has_exec_fence;
> bool has_syncobj;
> bool has_syncobj_wait;
> + bool has_context_priority;
>
> uint32_t eu_total;
> uint32_t subslice_total;
> @@ -914,6 +915,8 @@ int anv_gem_execbuffer(struct anv_device *device,
> int anv_gem_set_tiling(struct anv_device *device, uint32_t gem_handle,
> uint32_t stride, uint32_t tiling);
> int anv_gem_create_context(struct anv_device *device);
> +bool anv_gem_has_context_priority(int fd);
> +int anv_gem_set_context_priority(struct anv_device *device, int
> priority);
> int anv_gem_destroy_context(struct anv_device *device, int context);
> int anv_gem_get_context_param(int fd, int context, uint32_t param,
> uint64_t *value);
> --
> 2.14.3
>
>
More information about the mesa-dev
mailing list