[virglrenderer-devel] [PATCH 3/5] vulkan: added Vulkan state creation code

Nathan Gauër nathan at gauer.org
Wed Jul 25 06:53:34 UTC 2018


Hi !

On 07/25/2018 03:15 AM, Gurchetan Singh wrote:
> On Tue, Jul 24, 2018 at 6:51 AM Nathan Gauer <nathan at gauer.org> wrote:
>>
>> When enabled, Virglrenderer will create a vulkan state along the current
>> OpenGL context.
>> Vulkan state is not linked to the OpenGL one, and both can cohexist.
>> Signed-off-by: Nathan Gauer <nathan at gauer.org>
>> ---
>>  configure.ac        |  15 +++++
>>  src/Makefile.am     |   9 +++
>>  src/virgl_vk.c      | 159 ++++++++++++++++++++++++++++++++++++++++++++
>>  src/virgl_vk.h      |  76 +++++++++++++++++++++
>>  src/virglrenderer.c |  26 ++++++++
>>  src/virglrenderer.h |   1 +
>>  6 files changed, 286 insertions(+)
>>  create mode 100644 src/virgl_vk.c
>>  create mode 100644 src/virgl_vk.h
>>
>> diff --git a/configure.ac b/configure.ac
>> index c4ce743..054afbd 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -143,6 +143,20 @@ AS_IF([test "x$with_glx" = "xyes"], [
>>  ])
>>  AM_CONDITIONAL([WITH_GLX], [test "x$with_glx" = "xyes"])
>>
>> +AC_ARG_WITH([vulkan], AS_HELP_STRING([--with-vulkan], [enable vulkan(experimental)]))
>> +AS_IF([test "x$with_vulkan" = "xyes"], [
>> +
>> +  AC_CHECK_LIB([vulkan], [vkCreateInstance])
>> +  AC_CHECK_PROGS([PYTHON3], [python3 python])
>> +  PKG_CHECK_MODULES([VULKAN], [vulkan])
>> +  with_vulkan=yes
>> +  AC_DEFINE([WITH_VULKAN], [1], [Vulkan is supported.])
>> +],[
>> +  with_vulkan=no
>> +])
>> +AM_CONDITIONAL([WITH_VULKAN], [test "x$with_vulkan" = "xyes"])
>> +
>> +
>>  AC_SUBST([DEFINES])
>>  AC_CONFIG_FILES([
>>                 virglrenderer.pc
>> @@ -166,6 +180,7 @@ AC_MSG_NOTICE([
>>
>>        glx:                      $with_glx
>>        egl:                      $epoxy_has_egl
>> +      vulkan:                   $with_vulkan
>>        debug:                    $enable_debug
>>        tests:                    $build_tests
>>        fuzzer:                   $enable_fuzzer
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 76f4162..dae0cf3 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -3,6 +3,7 @@ AM_LDFLAGS = -lm \
>>         $(GBM_LIBS) \
>>         $(EPOXY_LIBS) \
>>         $(X11_LIBS) \
>> +       $(VULKAN_LIBS) \
>>         $(CODE_COVERAGE_LDFLAGS)
>>
>>  AM_CFLAGS = \
>> @@ -31,6 +32,8 @@ libvrend_la_SOURCES = \
>>          vrend_formats.c \
>>          vrend_blitter.c \
>>          vrend_blitter.h \
>> +                 util/list.h           \
>> +                 util/list.c           \
>>          iov.c
>>
>>  if HAVE_EPOXY_EGL
>> @@ -45,6 +48,12 @@ libvrend_la_SOURCES += \
>>         virgl_glx_context.c
>>  endif
>>
>> +if WITH_VULKAN
>> +libvrend_la_SOURCES += \
>> +       virgl_vk.h                              \
>> +       virgl_vk.c
>> +endif
> 
> Should we drop the "virgl" part for Vulkan, since there is no GL
> involved?  Maybe vrend_vk.{h, c}?

Maybe, I have no opinion on this.

>> +
>>  lib_LTLIBRARIES = libvirglrenderer.la
>>  noinst_LTLIBRARIES = libvrend.la
>>
>> diff --git a/src/virgl_vk.c b/src/virgl_vk.c
>> new file mode 100644
>> index 0000000..efbcff6
>> --- /dev/null
>> +++ b/src/virgl_vk.c
>> @@ -0,0 +1,159 @@
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <vulkan/vulkan.h>
>> +
>> +#include "virgl_vk.h"
>> +#include "util/list.h"
>> +#include "util/macros.h"
>> +
>> +const char* vkresult_to_string(VkResult res)
>> +{
>> +   switch (res)
>> +   {
>> +#define VK2STR(Value) case Value: return #Value
>> +      VK2STR(VK_SUCCESS);
>> +      VK2STR(VK_NOT_READY);
>> +      VK2STR(VK_TIMEOUT);
>> +      VK2STR(VK_EVENT_SET);
>> +      VK2STR(VK_EVENT_RESET);
>> +      VK2STR(VK_INCOMPLETE);
>> +      VK2STR(VK_ERROR_OUT_OF_HOST_MEMORY);
>> +      VK2STR(VK_ERROR_OUT_OF_DEVICE_MEMORY);
>> +      VK2STR(VK_ERROR_INITIALIZATION_FAILED);
>> +      VK2STR(VK_ERROR_DEVICE_LOST);
>> +      VK2STR(VK_ERROR_MEMORY_MAP_FAILED);
>> +      VK2STR(VK_ERROR_LAYER_NOT_PRESENT);
>> +      VK2STR(VK_ERROR_EXTENSION_NOT_PRESENT);
>> +      VK2STR(VK_ERROR_FEATURE_NOT_PRESENT);
>> +      VK2STR(VK_ERROR_INCOMPATIBLE_DRIVER);
>> +      VK2STR(VK_ERROR_TOO_MANY_OBJECTS);
>> +      VK2STR(VK_ERROR_FORMAT_NOT_SUPPORTED);
>> +      VK2STR(VK_ERROR_FRAGMENTED_POOL);
>> +      VK2STR(VK_ERROR_OUT_OF_POOL_MEMORY);
>> +      VK2STR(VK_ERROR_INVALID_EXTERNAL_HANDLE);
>> +      VK2STR(VK_ERROR_SURFACE_LOST_KHR);
>> +      VK2STR(VK_ERROR_NATIVE_WINDOW_IN_USE_KHR);
>> +      VK2STR(VK_SUBOPTIMAL_KHR);
>> +      VK2STR(VK_ERROR_OUT_OF_DATE_KHR);
>> +      VK2STR(VK_ERROR_INCOMPATIBLE_DISPLAY_KHR);
>> +      VK2STR(VK_ERROR_VALIDATION_FAILED_EXT);
>> +      VK2STR(VK_ERROR_INVALID_SHADER_NV);
>> +      VK2STR(VK_ERROR_FRAGMENTATION_EXT);
>> +      VK2STR(VK_ERROR_NOT_PERMITTED_EXT);
>> +      VK2STR(VK_RESULT_MAX_ENUM);
>> +#undef VK2STR
>> +      default:
>> +      return "VK_UNKNOWN_RETURN_VALUE";
>> +   }
>> +}
>> +
>> +static int init_physical_devices(struct virgl_vk *state)
>> +{
>> +   TRACE_IN();
>> +
>> +   uint32_t device_count;
>> +   VkResult res;
>> +
>> +   state->devices = malloc(sizeof(*state->devices));
>> +   if (NULL == state->devices) {
>> +      RETURN(-1);
>> +   }
> 
> nit: memset, calloc, or CALLOC_STRUCT (it seems the plan is to include
> src/gallium/auxiliary/util atleast)

yes, fixed with a CALLOC_STRUCT
> 
>> +
> 
>> +   list_init(&state->devices->list);
>> +
>> +   res = vkEnumeratePhysicalDevices(state->vk_instance, &device_count, NULL);
>> +   if (VK_SUCCESS != res) {
>> +      fprintf(stderr, "vulkan device enumeration failed (%s)", vkresult_to_string(res));
>> +      RETURN(-1);
>> +   }
>> +   if (device_count == 0) {
>> +      fprintf(stderr, "No device supports Vulkan.\n");
>> +      RETURN(-1);
>> +   }
>> +
>> +   state->physical_devices = malloc(sizeof(*state->physical_devices) * device_count);
> 
> nit: calloc

fixed with a CALLOC

> 
>> +   if (state->physical_devices == NULL) {
>> +      RETURN(-1);
>> +   }
>> +
>> +   res = vkEnumeratePhysicalDevices(state->vk_instance,
>> +                                    &device_count,
>> +                                    state->physical_devices);
> 
> What's the line limit we're following here (80 is Mesa style)?  I know
> the GL renderer is widely varying for line limits -- maybe we should
> try to follow something for new code??

I tried to follow Gallium's coding style since a lot of VGL code
followed it.
For this long function call, I followed Intel's coding style on Vk
driver (align parameters on the first open parenthesis)

If we choose this one, I'll also have to fix function declarations.

> 
>> +   if (VK_SUCCESS != res) {
>> +      fprintf(stderr, "vulkan device enumeration failed (%s)", vkresult_to_string(res));
>> +      RETURN(-1);
>> +   }
> 
> I've seen macros used to check the result result, maybe we should do
> that here?  For example, see check_vk_success here:

Yes, I used them in the experimental repo. However, do we want the
function to exit(err) on such case since we return an error value ?
However, I could do a RETURN-like macro to check and return a specific
value on error.
> 
> https://chromium.googlesource.com/chromiumos/platform/drm-tests/+/master/vk_glow.c#50
> 
>> +
>> +   state->physical_device_count = device_count;
>> +   RETURN(0);
>> +}
>> +
>> +struct virgl_vk* virgl_vk_init(void)
>> +{
>> +   struct virgl_vk *state = NULL;
>> +   VkResult vk_res;
>> +   VkApplicationInfo application_info = { 0 };
>> +   VkInstanceCreateInfo info = { 0 };
>> +
>> +   TRACE_IN();
>> +
>> +   state = malloc(sizeof(*state));
> 
> nit: memset, calloc, or CALLOC_STRUCT

fixed with a CALLOC_STRUCT
> 
>> +   if (state == NULL) {
>> +      return state;
>> +   }
> 
> 
>> +
>> +   application_info.sType = VK_STRUCTURE_TYPE_APPLICATION_INFO;
>> +   application_info.pApplicationName = "virglrenderer";
>> +   application_info.applicationVersion = 1;
>> +   application_info.pEngineName = "potatoe";
> 
> Why "potatoe"? Why not just NULL?

Oops sorry 🙂, kept that from my early prototype. Cleaned up !
> 
>> +   application_info.engineVersion = 1;
>> +   application_info.apiVersion = VK_MAKE_VERSION(1,1,0);
>> +
>> +   const char *validation_layers[] = {
>> +#ifdef VIRGL_VK_DEBUG
>> +      "VK_LAYER_LUNARG_core_validation",
>> +      "VK_LAYER_LUNARG_object_tracker",
>> +      "VK_LAYER_LUNARG_parameter_validation",
>> +      "VK_LAYER_LUNARG_standard_validation",
>> +#endif
>> +   };
>> +
>> +   info.sType = VK_STRUCTURE_TYPE_INSTANCE_CREATE_INFO;
>> +   info.pApplicationInfo = &application_info;
>> +   info.enabledLayerCount = ARRAY_SIZE(validation_layers);
>> +   info.ppEnabledLayerNames = validation_layers;
>> +
>> +   do {
>> +      vk_res = vkCreateInstance(&info, NULL, &state->vk_instance);
>> +      if (vk_res != VK_SUCCESS) {
>> +         fprintf(stderr, "Vk init failed (%s)\n", vkresult_to_string(vk_res));
>> +         break;
>> +      }
>> +
>> +      if (init_physical_devices(state) < 0) {
>> +         break;
>> +      }
>> +
>> +      printf("Vulkan state created with %d devices.\n", state->physical_device_count);
>> +      /* success path */
>> +      return state;
>> +   } while (0);
> 
> What's with the while loop?  Can't you just call virgl_vk_destroy on
> failure and return NULL?  You can also get rid of passing a struct
> virgl_vk ** pointer to that functions.

Well, for now, not a lot. Later on I'll certainly add more init_*
functions, thus the design.
(Like command buffer pool for the instance)

I could remove it for now since it will only duplicate the destroy
call twice.

Concerning these two functions, yes, it's fixed.
Now, prototypes are as follows:
int
virgl_vk_init(void);

void
virgl_vk_destroy(void);

> 
>> +
>> +   /* failure branch */
>> +   virgl_vk_destroy(&state);
>> +   return state;
>> +}
>> +
>> +void virgl_vk_destroy(struct virgl_vk **state)
>> +{
>> +   if ((*state)->vk_instance != VK_NULL_HANDLE) {
>> +      vkDestroyInstance((*state)->vk_instance, NULL);
>> +      (*state)->vk_instance = VK_NULL_HANDLE;
>> +   }
>> +
>> +   free((*state)->devices);
>> +   free((*state)->physical_devices);
>> +   free(*state);
>> +   *state = NULL;
>> +}
>> diff --git a/src/virgl_vk.h b/src/virgl_vk.h
>> new file mode 100644
>> index 0000000..bf55f82
>> --- /dev/null
>> +++ b/src/virgl_vk.h
>> @@ -0,0 +1,76 @@
>> +#ifndef VIRGL_VK_H
>> +#define VIRGL_VK_H
>> +
>> +#include <vulkan/vulkan.h>
>> +
>> +#include "util/list.h"
>> +
>> +/* This struct contains the state of our Vulkan module
>> + *
>> + * vk_instance: one instance per virglrenderer process.
>> + * physical_devices: contiguous array of VkPhysicalDevice*.
>> + *                   Devices are enumerated on instance creation.
>> + *
>> + * devices: list of VkDevice wrappers. Each item in this list reprensents
>> + *          a vulkan application using virglrenderer.
>> + */
>> +struct virgl_vk {
>> +   VkInstance vk_instance;
>> +
>> +   VkPhysicalDevice *physical_devices;
>> +   uint32_t physical_device_count;
>> +
>> +   struct vk_device *devices;
>> +};
>> +
>> +/* This struct contains the state of ONE Vulkan application running using virgl
>> + *
>> + * list: the linked-list element. (vk_device is double-linked instrusive list)
>> + * physical_device_id: this is the index of the physical device in use
>> + *                     (in the virgl_vk.physical_devices array)
>> + *
>> + * handle: The VkDevice handle
>> + *
>> + * Queue creation if forwarded. Thus, we store the VkQueue handles here.
>> + *
>> + * queue_count: number of queues created by the application
>> + * queues: array of VkQueue handles
>> + *
>> + * next_handle: next handle the device will use when creating an object
>> + * objects: this hashtable stores every Vulkan objects.
>> + *          All objects are wrapped in a struct defined bellow.
>> + */
>> +typedef struct vk_device {
>> +   struct list list;
>> +
>> +   uint32_t physical_device_id;
>> +   VkDevice handle;
>> +
>> +   uint32_t queue_count;
>> +   VkQueue *queues;
>> +
>> +   uint32_t next_handle;
>> +   struct util_hash_table *objects;
>> +} vk_device_t;
>> +
>> +/* contains the state of the Vulkan module */
>> +extern struct virgl_vk *vk_info;
>> +
>> +/* converts a VkResult to the readable string */
>> +const char* vkresult_to_string(VkResult res);
>> +
>> +/* initializes the state of the Vulkan part.
>> + * This function will allocate and initialize
>> + * the vulkan state.
>> + *
>> + * returns NULL on failure.
>> + */
>> +struct virgl_vk* virgl_vk_init(void);
>> +
>> +/* Used to destroy a previously created vulkan state.
>> + * The pointer taken as a parameter must be the one you
>> + * got from virgl_vk_init(void);
>> + */
>> +void virgl_vk_destroy(struct virgl_vk **state);
>> +
>> +#endif
>> diff --git a/src/virglrenderer.c b/src/virglrenderer.c
>> index 86824f8..3490915 100644
>> --- a/src/virglrenderer.c
>> +++ b/src/virglrenderer.c
>> @@ -48,6 +48,11 @@ static struct virgl_egl *egl_info;
>>  static struct virgl_glx *glx_info;
>>  #endif
>>
>> +#ifdef WITH_VULKAN
>> +#include "virgl_vk.h"
>> +struct virgl_vk *vk_info;
>> +#endif
>> +
>>  enum {
>>     CONTEXT_NONE,
>>     CONTEXT_EGL,
>> @@ -295,6 +300,13 @@ void virgl_renderer_cleanup(UNUSED void *cookie)
>>        use_context = CONTEXT_NONE;
>>     }
>>  #endif
>> +
>> +#ifdef WITH_VULKAN
>> +   if (NULL != vk_info) {
>> +      virgl_vk_destroy(&vk_info);
>> +   }
>> +
>> +#endif
>>  }
>>
>>  int virgl_renderer_init(void *cookie, int flags, struct virgl_renderer_callbacks *cbs)
>> @@ -309,6 +321,20 @@ int virgl_renderer_init(void *cookie, int flags, struct virgl_renderer_callbacks
>>     dev_cookie = cookie;
>>     rcbs = cbs;
>>
>> +   /* Vulkan state is not linked to GL state. Both can coexist */
>> +   if (flags & VIRGL_RENDERER_USE_VULKAN) {
>> +#ifdef WITH_VULKAN
>> +       vk_info = virgl_vk_init();
>> +       if (!vk_info) {
>> +           return -1;
>> +       }
>> +#else
>> +      fprintf(stderr, "Vulkan is not supported on this platform\n");
>> +      return -1;
>> +#endif
>> +   }
>> +
>> +
>>     if (flags & VIRGL_RENDERER_USE_EGL) {
>>  #ifdef HAVE_EPOXY_EGL_H
>>        int fd = -1;
>> diff --git a/src/virglrenderer.h b/src/virglrenderer.h
>> index 5baecdd..5f2de64 100644
>> --- a/src/virglrenderer.h
>> +++ b/src/virglrenderer.h
>> @@ -66,6 +66,7 @@ struct virgl_renderer_callbacks {
>>   */
>>  #define VIRGL_RENDERER_THREAD_SYNC 2
>>  #define VIRGL_RENDERER_USE_GLX (1 << 2)
>> +#define VIRGL_RENDERER_USE_VULKAN (1 << 3)
>>
>>  VIRGL_EXPORT int virgl_renderer_init(void *cookie, int flags, struct virgl_renderer_callbacks *cb);
>>  VIRGL_EXPORT void virgl_renderer_poll(void); /* force fences */
>> --
>> 2.18.0
>>
>> _______________________________________________
>> virglrenderer-devel mailing list
>> virglrenderer-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel

Thank you !


-- 
Nathan Gauër


More information about the virglrenderer-devel mailing list