[Mesa-dev] [PATCH] wsi: allow to override the present mode with MESA_VK_WSI_PRESENT_MODE

[Samuel Pitoiset]

On 4/9/19 1:10 PM, Lionel Landwerlin wrote:
> On 09/04/2019 08:08, Samuel Pitoiset wrote:
>> This is common to all Vulkan drivers and all WSI.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107391
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>>   src/vulkan/wsi/wsi_common.c         | 19 +++++++++++++++++++
>>   src/vulkan/wsi/wsi_common_display.c |  2 ++
>>   src/vulkan/wsi/wsi_common_private.h |  2 ++
>>   src/vulkan/wsi/wsi_common_wayland.c |  2 ++
>>   src/vulkan/wsi/wsi_common_x11.c     |  2 ++
>>   5 files changed, 27 insertions(+)
>>
>> diff --git a/src/vulkan/wsi/wsi_common.c b/src/vulkan/wsi/wsi_common.c
>> index 3cba0a4b06e..77f369f686d 100644
>> --- a/src/vulkan/wsi/wsi_common.c
>> +++ b/src/vulkan/wsi/wsi_common.c
>> @@ -29,6 +29,7 @@
>>   #include <time.h>
>>   #include <unistd.h>
>>   #include <xf86drm.h>
>> +#include <stdlib.h>
>>     VkResult
>>   wsi_device_init(struct wsi_device *wsi,
>> @@ -202,6 +203,24 @@ fail:
>>      return result;
>>   }
>>   +void
>> +wsi_swapchain_override_present_mode(struct wsi_swapchain *swapchain)
>> +{
>> +   if (getenv("MESA_VK_WSI_PRESENT_MODE")) {
>> +      const char *mode = getenv("MESA_VK_WSI_PRESENT_MODE");
>> +
>> +      if (!strcmp(mode, "fifo")) {
>> +         swapchain->present_mode = VK_PRESENT_MODE_FIFO_KHR;
>> +      } else if (!strcmp(mode, "mailbox")) {
>> +         swapchain->present_mode = VK_PRESENT_MODE_MAILBOX_KHR;
>> +      } else if (!strcmp(mode, "immediate")) {
>> +         swapchain->present_mode = VK_PRESENT_MODE_IMMEDIATE_KHR;
>> +      } else {
>> +         unreachable("Invalid MESA_VK_WSI_PRESENT_MODE value");
>> +      }
>> +   }
>> +}
>> +
>
>
> I would make this function a bit more robust.
>
> Not all backends support all present modes.
>
> Wayland only supports 2, while X11 supports 3.
>
>
> There is some checking required to validate that we're not passing 
> something invalid no?
>
>
> I would also store the override in the wsi_device and only do the 
> getenv() in wsi_device_init().
>
> So it's only calling getenv once.
>
>
> And then at swapchain creation do something like this maybe :
>
>
> chain->base.present_mode =wsi_swapchain_get_present_mode(wsi_device, 
> pCreateInfo->presentMode);
The whole approach looks better to me. Though, the only way to get the 
list of supported present modes is to call get_present_mode() but it 
requires a surface and we don't know the platform at device creation.
>
>
> -Lionel
>
>
>>   void
>>   wsi_swapchain_finish(struct wsi_swapchain *chain)
>>   {
>> diff --git a/src/vulkan/wsi/wsi_common_display.c 
>> b/src/vulkan/wsi/wsi_common_display.c
>> index 09c18315623..36ddb07a5bf 100644
>> --- a/src/vulkan/wsi/wsi_common_display.c
>> +++ b/src/vulkan/wsi/wsi_common_display.c
>> @@ -1765,6 +1765,8 @@ wsi_display_surface_create_swapchain(
>>        chain->surface = (VkIcdSurfaceDisplay *) icd_surface;
>>   +   wsi_swapchain_override_present_mode(&chain->base);
>> +
>>      for (uint32_t image = 0; image < chain->base.image_count; 
>> image++) {
>>         result = wsi_display_image_init(device, &chain->base,
>>                                         create_info, allocator,
>> diff --git a/src/vulkan/wsi/wsi_common_private.h 
>> b/src/vulkan/wsi/wsi_common_private.h
>> index a6f49fc3124..5ed4029ed70 100644
>> --- a/src/vulkan/wsi/wsi_common_private.h
>> +++ b/src/vulkan/wsi/wsi_common_private.h
>> @@ -78,6 +78,8 @@ wsi_swapchain_init(const struct wsi_device *wsi,
>>                      VkDevice device,
>>                      const VkSwapchainCreateInfoKHR *pCreateInfo,
>>                      const VkAllocationCallbacks *pAllocator);
>> +void
>> +wsi_swapchain_override_present_mode(struct wsi_swapchain *swapchain);
>>     void wsi_swapchain_finish(struct wsi_swapchain *chain);
>>   diff --git a/src/vulkan/wsi/wsi_common_wayland.c 
>> b/src/vulkan/wsi/wsi_common_wayland.c
>> index 03a47028ef2..dcb2f8733f6 100644
>> --- a/src/vulkan/wsi/wsi_common_wayland.c
>> +++ b/src/vulkan/wsi/wsi_common_wayland.c
>> @@ -1015,6 +1015,8 @@ 
>> wsi_wl_surface_create_swapchain(VkIcdSurfaceBase *icd_surface,
>>      chain->vk_format = pCreateInfo->imageFormat;
>>      chain->drm_format = 
>> wl_drm_format_for_vk_format(chain->vk_format, alpha);
>>   +   wsi_swapchain_override_present_mode(&chain->base);
>> +
>>      if (pCreateInfo->oldSwapchain) {
>>         /* If we have an oldSwapchain parameter, copy the display 
>> struct over
>>          * from the old one so we don't have to fully re-initialize it.
>> diff --git a/src/vulkan/wsi/wsi_common_x11.c 
>> b/src/vulkan/wsi/wsi_common_x11.c
>> index c87b9312636..f4dafb3b55e 100644
>> --- a/src/vulkan/wsi/wsi_common_x11.c
>> +++ b/src/vulkan/wsi/wsi_common_x11.c
>> @@ -1373,6 +1373,8 @@ x11_surface_create_swapchain(VkIcdSurfaceBase 
>> *icd_surface,
>>      chain->status = VK_SUCCESS;
>>      chain->has_dri3_modifiers = wsi_conn->has_dri3_modifiers;
>>   +   wsi_swapchain_override_present_mode(&chain->base);
>> +
>>      /* If we are reallocating from an old swapchain, then we inherit 
>> its
>>       * last completion mode, to ensure we don't get into reallocation
>>       * cycles. If we are starting anew, we set 'COPY', as that is 
>> the only
>
>