[Mesa-dev] [PATCH 3/3] radv: make device extension setup dynamic
Andres Rodriguez
andresx7 at gmail.com
Fri Jan 13 23:34:36 UTC 2017
On 2017-01-13 06:30 PM, Bas Nieuwenhuizen wrote:
> On Sat, Jan 14, 2017 at 12:20 AM, Andres Rodriguez <andresx7 at gmail.com> wrote:
>>
>> On 2017-01-13 06:04 PM, Bas Nieuwenhuizen wrote:
>>> On Fri, Jan 13, 2017 at 11:06 PM, Andres Rodriguez <andresx7 at gmail.com>
>>> wrote:
>>>> Each physical may have different extensions than one another.
>>>> Furthermore, depending on the software stack, some extensions may not be
>>>> accessible.
>>>>
>>>> If an extension is conditional, it can be registered only when
>>>> necessary.
>>>>
>>>> Signed-off-by: Andres Rodriguez <andresx7 at gmail.com>
>>>> ---
>>>> src/amd/vulkan/radv_device.c | 196
>>>> ++++++++++++++++++++++++++++--------------
>>>> src/amd/vulkan/radv_private.h | 6 ++
>>>> 2 files changed, 137 insertions(+), 65 deletions(-)
>>>>
>>>> diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
>>>> index 5669fd7..0333688 100644
>>>> --- a/src/amd/vulkan/radv_device.c
>>>> +++ b/src/amd/vulkan/radv_device.c
>>>> @@ -77,6 +77,115 @@ radv_device_get_cache_uuid(enum radeon_family family,
>>>> void *uuid)
>>>> return 0;
>>>> }
>>>>
>>>> +static const VkExtensionProperties instance_extensions[] = {
>>>> + {
>>>> + .extensionName = VK_KHR_SURFACE_EXTENSION_NAME,
>>>> + .specVersion = 25,
>>>> + },
>>>> +#ifdef VK_USE_PLATFORM_XCB_KHR
>>>> + {
>>>> + .extensionName = VK_KHR_XCB_SURFACE_EXTENSION_NAME,
>>>> + .specVersion = 6,
>>>> + },
>>>> +#endif
>>>> +#ifdef VK_USE_PLATFORM_XLIB_KHR
>>>> + {
>>>> + .extensionName = VK_KHR_XLIB_SURFACE_EXTENSION_NAME,
>>>> + .specVersion = 6,
>>>> + },
>>>> +#endif
>>>> +#ifdef VK_USE_PLATFORM_WAYLAND_KHR
>>>> + {
>>>> + .extensionName = VK_KHR_WAYLAND_SURFACE_EXTENSION_NAME,
>>>> + .specVersion = 5,
>>>> + },
>>>> +#endif
>>>> +};
>>>> +
>>>> +static const VkExtensionProperties common_device_extensions[] = {
>>>> + {
>>>> + .extensionName =
>>>> VK_KHR_SAMPLER_MIRROR_CLAMP_TO_EDGE_EXTENSION_NAME,
>>>> + .specVersion = 1,
>>>> + },
>>>> + {
>>>> + .extensionName = VK_KHR_SWAPCHAIN_EXTENSION_NAME,
>>>> + .specVersion = 68,
>>>> + },
>>>> + {
>>>> + .extensionName =
>>>> VK_AMD_DRAW_INDIRECT_COUNT_EXTENSION_NAME,
>>>> + .specVersion = 1,
>>>> + },
>>>> + {
>>>> + .extensionName =
>>>> VK_AMD_NEGATIVE_VIEWPORT_HEIGHT_EXTENSION_NAME,
>>>> + .specVersion = 1,
>>>> + },
>>>> +};
>>>> +
>>>> +static VkResult
>>>> +radv_extensions_register(struct radv_instance *instance,
>>>> + struct radv_extensions
>>>> *extensions,
>>>> + const VkExtensionProperties
>>>> *new_ext,
>>>> + uint32_t num_ext)
>>>> +{
>>>> + size_t new_size;
>>>> + VkExtensionProperties *new_ptr;
>>>> +
>>>> + assert(new_ext && num_ext > 0);
>>>> +
>>>> + if (!new_ext)
>>>> + return VK_ERROR_INITIALIZATION_FAILED;
>>>> +
>>>> + new_size = (extensions->num_ext + num_ext) *
>>>> sizeof(VkExtensionProperties);
>>>> + new_ptr = vk_realloc(&instance->alloc, extensions->ext_array,
>>>> + new_size, 8,
>>>> VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE);
>>>> +
>>>> + /* Old array continues to be valid, update nothing */
>>>> + if (!new_ptr)
>>>> + return VK_ERROR_OUT_OF_HOST_MEMORY;
>>>> +
>>>> + memcpy(&new_ptr[extensions->num_ext], new_ext,
>>>> + num_ext * sizeof(VkExtensionProperties));
>>>> + extensions->ext_array = new_ptr;
>>>> + extensions->num_ext += num_ext;
>>>> +
>>>> + return VK_SUCCESS;
>>>> +}
>>>> +
>>>> +#define radv_extensions_register_single(instance, extensions, name,
>>>> version) \
>>>> + radv_extensions_register(instance, extensions, \
>>>> +
>>>> &(VkExtensionProperties){ \
>>>> +
>>>> .extensionName = name, \
>>>> +
>>>> .specVersion = version \
>>>> + }, 1);
>>> Please make this a function, I see no reason to keep this a macro. Or
>>> lose it, as I can't find an user in this patch.
>>>> +
>>>> +static void
>>>> +radv_extensions_finish(struct radv_instance *instance,
>>>> + struct radv_extensions
>>>> *extensions)
>>>> +{
>>>> + assert(extensions);
>>>> +
>>>> + if (!extensions)
>>>> + radv_loge("Attemted to free invalid extension struct\n");
>>>> +
>>>> + if (extensions->ext_array)
>>>> + vk_free(&instance->alloc, extensions->ext_array);
>>>> +}
>>>> +
>>>> +static bool
>>>> +is_extension_enabled(const VkExtensionProperties *extensions,
>>>> + size_t num_ext,
>>>> + const char *name)
>>>> +{
>>>> + assert(extensions && name);
>>>> +
>>>> + for (uint32_t i = 0; i < num_ext; i++) {
>>>> + if (strcmp(name, extensions[i].extensionName) == 0)
>>>> + return true;
>>>> + }
>>>> +
>>>> + return false;
>>>> +}
>>>> +
>>>> static VkResult
>>>> radv_physical_device_init(struct radv_physical_device *device,
>>>> struct radv_instance *instance,
>>>> @@ -130,6 +239,13 @@ radv_physical_device_init(struct
>>>> radv_physical_device *device,
>>>> goto fail;
>>>> }
>>>>
>>>> + result = radv_extensions_register(instance,
>>>> +
>>>> &device->extensions,
>>>> +
>>>> common_device_extensions,
>>>> +
>>>> ARRAY_SIZE(common_device_extensions));
>>>> + if (result != VK_SUCCESS)
>>>> + goto fail;
>>>> +
>>>> fprintf(stderr, "WARNING: radv is not a conformant vulkan
>>>> implementation, testing use only.\n");
>>>> device->name = device->rad_info.name;
>>>> close(fd);
>>>> @@ -143,53 +259,11 @@ fail:
>>>> static void
>>>> radv_physical_device_finish(struct radv_physical_device *device)
>>>> {
>>>> + radv_extensions_finish(device->instance, &device->extensions);
>>>> radv_finish_wsi(device);
>>>> device->ws->destroy(device->ws);
>>>> }
>>>>
>>>> -static const VkExtensionProperties instance_extensions[] = {
>>>> - {
>>>> - .extensionName = VK_KHR_SURFACE_EXTENSION_NAME,
>>>> - .specVersion = 25,
>>>> - },
>>>> -#ifdef VK_USE_PLATFORM_XCB_KHR
>>>> - {
>>>> - .extensionName = VK_KHR_XCB_SURFACE_EXTENSION_NAME,
>>>> - .specVersion = 6,
>>>> - },
>>>> -#endif
>>>> -#ifdef VK_USE_PLATFORM_XLIB_KHR
>>>> - {
>>>> - .extensionName = VK_KHR_XLIB_SURFACE_EXTENSION_NAME,
>>>> - .specVersion = 6,
>>>> - },
>>>> -#endif
>>>> -#ifdef VK_USE_PLATFORM_WAYLAND_KHR
>>>> - {
>>>> - .extensionName = VK_KHR_WAYLAND_SURFACE_EXTENSION_NAME,
>>>> - .specVersion = 5,
>>>> - },
>>>> -#endif
>>>> -};
>>>> -
>>>> -static const VkExtensionProperties common_device_extensions[] = {
>>>> - {
>>>> - .extensionName =
>>>> VK_KHR_SAMPLER_MIRROR_CLAMP_TO_EDGE_EXTENSION_NAME,
>>>> - .specVersion = 1,
>>>> - },
>>>> - {
>>>> - .extensionName = VK_KHR_SWAPCHAIN_EXTENSION_NAME,
>>>> - .specVersion = 68,
>>>> - },
>>>> - {
>>>> - .extensionName =
>>>> VK_AMD_DRAW_INDIRECT_COUNT_EXTENSION_NAME,
>>>> - .specVersion = 1,
>>>> - },
>>>> - {
>>>> - .extensionName =
>>>> VK_AMD_NEGATIVE_VIEWPORT_HEIGHT_EXTENSION_NAME,
>>>> - .specVersion = 1,
>>>> - },
>>>> -};
>>>>
>>>> static void *
>>>> default_alloc_func(void *pUserData, size_t size, size_t align,
>>>> @@ -257,15 +331,9 @@ VkResult radv_CreateInstance(
>>>> }
>>>>
>>>> for (uint32_t i = 0; i < pCreateInfo->enabledExtensionCount;
>>>> i++) {
>>>> - bool found = false;
>>>> - for (uint32_t j = 0; j < ARRAY_SIZE(instance_extensions);
>>>> j++) {
>>>> - if
>>>> (strcmp(pCreateInfo->ppEnabledExtensionNames[i],
>>>> - instance_extensions[j].extensionName)
>>>> == 0) {
>>>> - found = true;
>>>> - break;
>>>> - }
>>>> - }
>>>> - if (!found)
>>>> + if (!is_extension_enabled(instance_extensions,
>>>> +
>>>> ARRAY_SIZE(instance_extensions),
>>>> +
>>>> pCreateInfo->ppEnabledExtensionNames[i]))
>>>> return vk_error(VK_ERROR_EXTENSION_NOT_PRESENT);
>>>> }
>>>>
>>>> @@ -274,6 +342,8 @@ VkResult radv_CreateInstance(
>>>> if (!instance)
>>>> return vk_error(VK_ERROR_OUT_OF_HOST_MEMORY);
>>>>
>>>> + memset(instance, 0, sizeof(*instance));
>>>> +
>>> Why is this memset necessary for this patch?
>>>
>>> Otherwise, with fixed commit message for patch 1 and fixed alignment,
>>> this series looks good to me.
>>
>> We need a way to tell whether extensions->ext_array is initialized or not
>> for:
>>
>> + new_ptr = vk_realloc(&instance->alloc, extensions->ext_array,
>> + new_size, 8,
>> VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE);
>>
>> If it is unclear, I can create a radv_extensions_initialize() function to
>> clean our the relevant fields. But I was thinking that having the all other
>> instance members zero-initialized may be useful as well.
> But we aren't adding extensions as a member to the instance struct, so
> I don't think that should matter here?
struct radv_physical_device {
[snip]
struct radv_extensions extensions;
[snip]
};
struct radv_instance {
[snip]
struct radv_physical_device physicalDevice;
[snip]
};
I'm guessing this won't be true forever though, because support for
multiple physical devices in the future may change that layout.
>
>>
>>>> instance->_loader_data.loaderMagic = ICD_LOADER_MAGIC;
>>>>
>>>> if (pAllocator)
>>>> @@ -696,15 +766,9 @@ VkResult radv_CreateDevice(
>>>> struct radv_device *device;
>>>>
>>>> for (uint32_t i = 0; i < pCreateInfo->enabledExtensionCount;
>>>> i++) {
>>>> - bool found = false;
>>>> - for (uint32_t j = 0; j <
>>>> ARRAY_SIZE(common_device_extensions); j++) {
>>>> - if
>>>> (strcmp(pCreateInfo->ppEnabledExtensionNames[i],
>>>> -
>>>> common_device_extensions[j].extensionName) == 0) {
>>>> - found = true;
>>>> - break;
>>>> - }
>>>> - }
>>>> - if (!found)
>>>> + if
>>>> (!is_extension_enabled(physical_device->extensions.ext_array,
>>>> +
>>>> physical_device->extensions.num_ext,
>>>> +
>>>> pCreateInfo->ppEnabledExtensionNames[i]))
>>>> return vk_error(VK_ERROR_EXTENSION_NOT_PRESENT);
>>>> }
>>>>
>>>> @@ -850,15 +914,17 @@ VkResult radv_EnumerateDeviceExtensionProperties(
>>>> uint32_t* pPropertyCount,
>>>> VkExtensionProperties* pProperties)
>>>> {
>>>> + RADV_FROM_HANDLE(radv_physical_device, pdevice, physicalDevice);
>>>> +
>>>> if (pProperties == NULL) {
>>>> - *pPropertyCount = ARRAY_SIZE(common_device_extensions);
>>>> + *pPropertyCount = pdevice->extensions.num_ext;
>>>> return VK_SUCCESS;
>>>> }
>>>>
>>>> - *pPropertyCount = MIN2(*pPropertyCount,
>>>> ARRAY_SIZE(common_device_extensions));
>>>> - typed_memcpy(pProperties, common_device_extensions,
>>>> *pPropertyCount);
>>>> + *pPropertyCount = MIN2(*pPropertyCount,
>>>> pdevice->extensions.num_ext);
>>>> + typed_memcpy(pProperties, pdevice->extensions.ext_array,
>>>> *pPropertyCount);
>>>>
>>>> - if (*pPropertyCount < ARRAY_SIZE(common_device_extensions))
>>>> + if (*pPropertyCount < pdevice->extensions.num_ext)
>>>> return VK_INCOMPLETE;
>>>>
>>>> return VK_SUCCESS;
>>>> diff --git a/src/amd/vulkan/radv_private.h
>>>> b/src/amd/vulkan/radv_private.h
>>>> index ab4ede6..b095e3f 100644
>>>> --- a/src/amd/vulkan/radv_private.h
>>>> +++ b/src/amd/vulkan/radv_private.h
>>>> @@ -263,6 +263,11 @@ void *radv_lookup_entrypoint(const char *name);
>>>>
>>>> extern struct radv_dispatch_table dtable;
>>>>
>>>> +struct radv_extensions {
>>>> + VkExtensionProperties *ext_array;
>>>> + uint32_t num_ext;
>>>> +};
>>>> +
>>>> struct radv_physical_device {
>>>> VK_LOADER_DATA _loader_data;
>>>>
>>>> @@ -275,6 +280,7 @@ struct radv_physical_device {
>>>> uint8_t uuid[VK_UUID_SIZE];
>>>>
>>>> struct wsi_device wsi_device;
>>>> + struct radv_extensions extensions;
>>>> };
>>>>
>>>> struct radv_instance {
>>>> --
>>>> 2.9.3
>>>>
More information about the mesa-dev
mailing list