[Mesa-dev] [PATCH 1/2] vulkan/wsi: Store the instance allocator in wsi_device
Eric Engestrom
eric.engestrom at intel.com
Thu Oct 18 14:06:41 UTC 2018
On Thursday, 2018-10-18 08:02:42 -0500, Jason Ekstrand wrote:
> On October 18, 2018 05:40:14 Eric Engestrom <eric.engestrom at intel.com> wrote:
>
> > On Wednesday, 2018-10-17 15:14:03 -0500, Jason Ekstrand wrote:
> > > We already have wsi_device and we know the instance allocator at
> > > wsi_device_init time so there's no need to pass it into the physical
> > > device queries. This also fixes a memory allocation domain bug that can
> > > occur if CreateSwapchain gets called prior to any queries (not likely)
> > > in which case the cached connection gets allocated off the device
> > > instead of the instance.
> > > ---
> > > src/amd/vulkan/radv_wsi.c | 1 -
> > > src/amd/vulkan/radv_wsi_x11.c | 2 --
> > > src/intel/vulkan/anv_wsi.c | 1 -
> > > src/intel/vulkan/anv_wsi_x11.c | 2 --
> > > src/vulkan/wsi/wsi_common.c | 4 ++--
> > > src/vulkan/wsi/wsi_common.h | 4 +++-
> > > src/vulkan/wsi/wsi_common_display.c | 1 -
> > > src/vulkan/wsi/wsi_common_private.h | 1 -
> > > src/vulkan/wsi/wsi_common_wayland.c | 1 -
> > > src/vulkan/wsi/wsi_common_x11.c | 25 +++++++++++--------------
> > > src/vulkan/wsi/wsi_common_x11.h | 1 -
> > > 11 files changed, 16 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/src/amd/vulkan/radv_wsi.c b/src/amd/vulkan/radv_wsi.c
> > > index 6479bea070b..8b165ea3916 100644
> > > --- a/src/amd/vulkan/radv_wsi.c
> > > +++ b/src/amd/vulkan/radv_wsi.c
> > > @@ -75,7 +75,6 @@ VkResult radv_GetPhysicalDeviceSurfaceSupportKHR(
> > > device->local_fd,
> > > queueFamilyIndex,
> > > surface,
> > > - &device->instance->alloc,
> > > pSupported);
> > > }
> > >
> > > diff --git a/src/amd/vulkan/radv_wsi_x11.c b/src/amd/vulkan/radv_wsi_x11.c
> > > index c65ac938772..9ef02ccc435 100644
> > > --- a/src/amd/vulkan/radv_wsi_x11.c
> > > +++ b/src/amd/vulkan/radv_wsi_x11.c
> > > @@ -44,7 +44,6 @@ VkBool32 radv_GetPhysicalDeviceXcbPresentationSupportKHR(
> > >
> > > return wsi_get_physical_device_xcb_presentation_support(
> > > &device->wsi_device,
> > > - &device->instance->alloc,
> > > queueFamilyIndex,
> > > device->local_fd, true,
> > > connection, visual_id);
> > > @@ -60,7 +59,6 @@ VkBool32 radv_GetPhysicalDeviceXlibPresentationSupportKHR(
> > >
> > > return wsi_get_physical_device_xcb_presentation_support(
> > > &device->wsi_device,
> > > - &device->instance->alloc,
> > > queueFamilyIndex,
> > > device->local_fd, true,
> > > XGetXCBConnection(dpy), visualID);
> > > diff --git a/src/intel/vulkan/anv_wsi.c b/src/intel/vulkan/anv_wsi.c
> > > index 5ed1d711689..1c9a54804e8 100644
> > > --- a/src/intel/vulkan/anv_wsi.c
> > > +++ b/src/intel/vulkan/anv_wsi.c
> > > @@ -92,7 +92,6 @@ VkResult anv_GetPhysicalDeviceSurfaceSupportKHR(
> > > device->local_fd,
> > > queueFamilyIndex,
> > > surface,
> > > - &device->instance->alloc,
> > > pSupported);
> > > }
> > >
> > > diff --git a/src/intel/vulkan/anv_wsi_x11.c b/src/intel/vulkan/anv_wsi_x11.c
> > > index 2feb5f13376..45c43f6f17f 100644
> > > --- a/src/intel/vulkan/anv_wsi_x11.c
> > > +++ b/src/intel/vulkan/anv_wsi_x11.c
> > > @@ -40,7 +40,6 @@ VkBool32 anv_GetPhysicalDeviceXcbPresentationSupportKHR(
> > >
> > > return wsi_get_physical_device_xcb_presentation_support(
> > > &device->wsi_device,
> > > - &device->instance->alloc,
> > > queueFamilyIndex,
> > > device->local_fd, false,
> > > connection, visual_id);
> > > @@ -56,7 +55,6 @@ VkBool32 anv_GetPhysicalDeviceXlibPresentationSupportKHR(
> > >
> > > return wsi_get_physical_device_xcb_presentation_support(
> > > &device->wsi_device,
> > > - &device->instance->alloc,
> > > queueFamilyIndex,
> > > device->local_fd, false,
> > > XGetXCBConnection(dpy), visualID);
> > > diff --git a/src/vulkan/wsi/wsi_common.c b/src/vulkan/wsi/wsi_common.c
> > > index 3416fef3076..1e3c4e0028b 100644
> > > --- a/src/vulkan/wsi/wsi_common.c
> > > +++ b/src/vulkan/wsi/wsi_common.c
> > > @@ -39,6 +39,7 @@ wsi_device_init(struct wsi_device *wsi,
> > >
> > > memset(wsi, 0, sizeof(*wsi));
> > >
> > > + wsi->instance_alloc = *alloc;
> > > wsi->pdevice = pdevice;
> > >
> > > #define WSI_GET_CB(func) \
> > > @@ -677,13 +678,12 @@ wsi_common_get_surface_support(struct
> > > wsi_device *wsi_device,
> > > int local_fd,
> > > uint32_t queueFamilyIndex,
> > > VkSurfaceKHR _surface,
> > > - const VkAllocationCallbacks *alloc,
> > > VkBool32* pSupported)
> > > {
> > > ICD_FROM_HANDLE(VkIcdSurfaceBase, surface, _surface);
> > > struct wsi_interface *iface = wsi_device->wsi[surface->platform];
> > >
> > > - return iface->get_support(surface, wsi_device, alloc,
> > > + return iface->get_support(surface, wsi_device,
> > > queueFamilyIndex, local_fd, pSupported);
> > > }
> > >
> > > diff --git a/src/vulkan/wsi/wsi_common.h b/src/vulkan/wsi/wsi_common.h
> > > index 14f65097bb3..424330de566 100644
> > > --- a/src/vulkan/wsi/wsi_common.h
> > > +++ b/src/vulkan/wsi/wsi_common.h
> > > @@ -90,6 +90,9 @@ struct wsi_interface;
> > > #define VK_ICD_WSI_PLATFORM_MAX (VK_ICD_WSI_PLATFORM_DISPLAY + 1)
> > >
> > > struct wsi_device {
> > > + /* Allocator for the instance */
> > > + VkAllocationCallbacks instance_alloc;
> > > +
> > > VkPhysicalDevice pdevice;
> > > VkPhysicalDeviceMemoryProperties memory_props;
> > > uint32_t queue_family_count;
> > > @@ -166,7 +169,6 @@ wsi_common_get_surface_support(struct wsi_device
> > > *wsi_device,
> > > int local_fd,
> > > uint32_t queueFamilyIndex,
> > > VkSurfaceKHR surface,
> > > - const VkAllocationCallbacks *alloc,
> > > VkBool32* pSupported);
> > >
> > > VkResult
> > > diff --git a/src/vulkan/wsi/wsi_common_display.c
> > > b/src/vulkan/wsi/wsi_common_display.c
> > > index 5cebde5b9bc..c004060a205 100644
> > > --- a/src/vulkan/wsi/wsi_common_display.c
> > > +++ b/src/vulkan/wsi/wsi_common_display.c
> > > @@ -802,7 +802,6 @@ wsi_create_display_surface(VkInstance instance,
> > > static VkResult
> > > wsi_display_surface_get_support(VkIcdSurfaceBase *surface,
> > > struct wsi_device *wsi_device,
> > > - const VkAllocationCallbacks *allocator,
> > > uint32_t queueFamilyIndex,
> > > int local_fd,
> > > VkBool32* pSupported)
> > > diff --git a/src/vulkan/wsi/wsi_common_private.h
> > > b/src/vulkan/wsi/wsi_common_private.h
> > > index ee7ae75b8f7..accc1170163 100644
> > > --- a/src/vulkan/wsi/wsi_common_private.h
> > > +++ b/src/vulkan/wsi/wsi_common_private.h
> > > @@ -100,7 +100,6 @@ wsi_destroy_image(const struct wsi_swapchain *chain,
> > > struct wsi_interface {
> > > VkResult (*get_support)(VkIcdSurfaceBase *surface,
> > > struct wsi_device *wsi_device,
> > > - const VkAllocationCallbacks *alloc,
> > > uint32_t queueFamilyIndex,
> > > int local_fd,
> > > VkBool32* pSupported);
> > > diff --git a/src/vulkan/wsi/wsi_common_wayland.c
> > > b/src/vulkan/wsi/wsi_common_wayland.c
> > > index 6b34e21bd98..67d5f8d4c80 100644
> > > --- a/src/vulkan/wsi/wsi_common_wayland.c
> > > +++ b/src/vulkan/wsi/wsi_common_wayland.c
> > > @@ -464,7 +464,6 @@ wsi_wl_get_presentation_support(struct
> > > wsi_device *wsi_device,
> > > static VkResult
> > > wsi_wl_surface_get_support(VkIcdSurfaceBase *surface,
> > > struct wsi_device *wsi_device,
> > > - const VkAllocationCallbacks *alloc,
> > > uint32_t queueFamilyIndex,
> > > int local_fd,
> > > VkBool32* pSupported)
> > > diff --git a/src/vulkan/wsi/wsi_common_x11.c b/src/vulkan/wsi/wsi_common_x11.c
> > > index 5f99b2ee473..75aa83bb7b4 100644
> > > --- a/src/vulkan/wsi/wsi_common_x11.c
> > > +++ b/src/vulkan/wsi/wsi_common_x11.c
> > > @@ -125,7 +125,7 @@ wsi_x11_check_dri3_compatible(xcb_connection_t
> > > *conn, int local_fd)
> > > }
> > >
> > > static struct wsi_x11_connection *
> > > -wsi_x11_connection_create(const VkAllocationCallbacks *alloc,
> > > +wsi_x11_connection_create(struct wsi_device *wsi_dev,
> >
> > Nit: I'd keep that `const`, it has no reason not to be
>
> I guess. It also doesn't buy us anything.
I like `const` because it allows the reader to know what touches
a variable or leaves it as is, which helps me understand the code I'm
reading. It also prevents silly bugs like `if (foo->bar = BLAH)`.
Like I said though, it's a nit, feel free to ignore :P
>
> >
> > > xcb_connection_t *conn)
> > > {
> > > xcb_query_extension_cookie_t dri3_cookie, pres_cookie, amd_cookie, nv_cookie;
> > > @@ -134,7 +134,7 @@ wsi_x11_connection_create(const
> > > VkAllocationCallbacks *alloc,
> > > bool has_present_v1_2 = false;
> > >
> > > struct wsi_x11_connection *wsi_conn =
> > > - vk_alloc(alloc, sizeof(*wsi_conn), 8,
> > > + vk_alloc(&wsi_dev->instance_alloc, sizeof(*wsi_conn), 8,
> > > VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE);
> > > if (!wsi_conn)
> > > return NULL;
> > > @@ -163,7 +163,7 @@ wsi_x11_connection_create(const
> > > VkAllocationCallbacks *alloc,
> > > free(pres_reply);
> > > free(amd_reply);
> > > free(nv_reply);
> > > - vk_free(alloc, wsi_conn);
> > > + vk_free(&wsi_dev->instance_alloc, wsi_conn);
> > > return NULL;
> > > }
> > >
> > > @@ -211,10 +211,10 @@ wsi_x11_connection_create(const
> > > VkAllocationCallbacks *alloc,
> > > }
> > >
> > > static void
> > > -wsi_x11_connection_destroy(const VkAllocationCallbacks *alloc,
> > > +wsi_x11_connection_destroy(struct wsi_device *wsi_dev,
> >
> > Ditto: `const`
> >
> > Reviewed-by: Eric Engestrom <eric.engestrom at intel.com>
> >
> > > struct wsi_x11_connection *conn)
> > > {
> > > - vk_free(alloc, conn);
> > > + vk_free(&wsi_dev->instance_alloc, conn);
> > > }
> > >
> > > static bool
> > > @@ -231,7 +231,6 @@ wsi_x11_check_for_dri3(struct wsi_x11_connection *wsi_conn)
> > >
> > > static struct wsi_x11_connection *
> > > wsi_x11_get_connection(struct wsi_device *wsi_dev,
> > > - const VkAllocationCallbacks *alloc,
> > > xcb_connection_t *conn)
> > > {
> > > struct wsi_x11 *wsi =
> > > @@ -247,7 +246,7 @@ wsi_x11_get_connection(struct wsi_device *wsi_dev,
> > > pthread_mutex_unlock(&wsi->mutex);
> > >
> > > struct wsi_x11_connection *wsi_conn =
> > > - wsi_x11_connection_create(alloc, conn);
> > > + wsi_x11_connection_create(wsi_dev, conn);
> > > if (!wsi_conn)
> > > return NULL;
> > >
> > > @@ -256,7 +255,7 @@ wsi_x11_get_connection(struct wsi_device *wsi_dev,
> > > entry = _mesa_hash_table_search(wsi->connections, conn);
> > > if (entry) {
> > > /* Oops, someone raced us to it */
> > > - wsi_x11_connection_destroy(alloc, wsi_conn);
> > > + wsi_x11_connection_destroy(wsi_dev, wsi_conn);
> > > } else {
> > > entry = _mesa_hash_table_insert(wsi->connections, conn, wsi_conn);
> > > }
> > > @@ -382,7 +381,6 @@ visual_has_alpha(xcb_visualtype_t *visual, unsigned depth)
> > >
> > > VkBool32 wsi_get_physical_device_xcb_presentation_support(
> > > struct wsi_device *wsi_device,
> > > - VkAllocationCallbacks *alloc,
> > > uint32_t queueFamilyIndex,
> > > int fd,
> > > bool can_handle_different_gpu,
> > > @@ -390,7 +388,7 @@ VkBool32 wsi_get_physical_device_xcb_presentation_support(
> > > xcb_visualid_t visual_id)
> > > {
> > > struct wsi_x11_connection *wsi_conn =
> > > - wsi_x11_get_connection(wsi_device, alloc, connection);
> > > + wsi_x11_get_connection(wsi_device, connection);
> > >
> > > if (!wsi_conn)
> > > return false;
> > > @@ -433,7 +431,6 @@ x11_surface_get_window(VkIcdSurfaceBase *icd_surface)
> > > static VkResult
> > > x11_surface_get_support(VkIcdSurfaceBase *icd_surface,
> > > struct wsi_device *wsi_device,
> > > - const VkAllocationCallbacks *alloc,
> > > uint32_t queueFamilyIndex,
> > > int local_fd,
> > > VkBool32* pSupported)
> > > @@ -442,7 +439,7 @@ x11_surface_get_support(VkIcdSurfaceBase *icd_surface,
> > > xcb_window_t window = x11_surface_get_window(icd_surface);
> > >
> > > struct wsi_x11_connection *wsi_conn =
> > > - wsi_x11_get_connection(wsi_device, alloc, conn);
> > > + wsi_x11_get_connection(wsi_device, conn);
> > > if (!wsi_conn)
> > > return VK_ERROR_OUT_OF_HOST_MEMORY;
> > >
> > > @@ -1279,7 +1276,7 @@ x11_surface_create_swapchain(VkIcdSurfaceBase
> > > *icd_surface,
> > >
> > > xcb_connection_t *conn = x11_surface_get_connection(icd_surface);
> > > struct wsi_x11_connection *wsi_conn =
> > > - wsi_x11_get_connection(wsi_device, pAllocator, conn);
> > > + wsi_x11_get_connection(wsi_device, conn);
> > > if (!wsi_conn)
> > > return VK_ERROR_OUT_OF_HOST_MEMORY;
> > >
> > > @@ -1501,7 +1498,7 @@ wsi_x11_finish_wsi(struct wsi_device *wsi_device,
> > > if (wsi) {
> > > struct hash_entry *entry;
> > > hash_table_foreach(wsi->connections, entry)
> > > - wsi_x11_connection_destroy(alloc, entry->data);
> > > + wsi_x11_connection_destroy(wsi_device, entry->data);
> > >
> > > _mesa_hash_table_destroy(wsi->connections, NULL);
> > >
> > > diff --git a/src/vulkan/wsi/wsi_common_x11.h b/src/vulkan/wsi/wsi_common_x11.h
> > > index b33540856cb..e9b3ecfb2e0 100644
> > > --- a/src/vulkan/wsi/wsi_common_x11.h
> > > +++ b/src/vulkan/wsi/wsi_common_x11.h
> > > @@ -27,7 +27,6 @@
> > >
> > > VkBool32 wsi_get_physical_device_xcb_presentation_support(
> > > struct wsi_device *wsi_device,
> > > - VkAllocationCallbacks *alloc,
> > > uint32_t queueFamilyIndex,
> > > int local_fd,
> > > bool can_handle_different_gpu,
> > > --
> > > 2.19.1
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
More information about the mesa-dev
mailing list