[Mesa-dev] [PATCH 3/3] radv: make device extension setup dynamic
Bas Nieuwenhuizen
bas at basnieuwenhuizen.nl
Fri Jan 13 23:40:25 UTC 2017
On Sat, Jan 14, 2017 at 12:34 AM, Andres Rodriguez <andresx7 at gmail.com> wrote:
>
>
> 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.
Ok, agreed, thanks.
>
>
>>
>>>
>>>>> 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