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

[Lionel Landwerlin]

On 09/04/2019 13:29, Samuel Pitoiset wrote:
>
> 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.


I forgot about the surface :)

Then checking should be done by 
wsi_swapchain_get_present_mode(wsi_device, pCreateInfo) at swapchain 
creation (the surface is available in pCreateInfo).

wsi_device_init() would just store the override.


-Lionel


>>
>>
>> -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
>>
>>
>