wsi: allow to override the present mode with MESA_VK_WSI_PRESENT_MODE

Submitted by Samuel Pitoiset on April 9, 2019, 7:08 a.m.

Details

Message ID 20190409070836.11801-1-samuel.pitoiset@gmail.com
State Changes Requested
Headers show
Series "wsi: allow to override the present mode with MESA_VK_WSI_PRESENT_MODE" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Samuel Pitoiset April 9, 2019, 7:08 a.m.
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@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(+)

Patch hide | download patch | download mbox

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");
+      }
+   }
+}
+
 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

Comments

On Tuesday, 2019-04-09 09:08:36 +0200, 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@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");

const char *mode = getenv("MESA_VK_WSI_PRESENT_MODE");
if (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");

unreachable() means the compiler is allowed to do anything, including
considering the application as dead.

Not something we want user to be able to do with a simple
`MESA_VK_WSI_PRESENT_MODE=0` :)

With an assert() instead, this is:
Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>

> +      }
> +   }
> +}
> +
>  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
> -- 
> 2.21.0
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On 2019-04-09 10:03 a.m., Eric Engestrom wrote:
> On Tuesday, 2019-04-09 09:08:36 +0200, Samuel Pitoiset wrote:
>>
>> +      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");
> 
> unreachable() means the compiler is allowed to do anything, including
> considering the application as dead.
> 
> Not something we want user to be able to do with a simple
> `MESA_VK_WSI_PRESENT_MODE=0` :)
> 
> With an assert() instead, this is:
> Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>

With assertions enabled, the application is most certainly dead after a
failed assertion. :)

I'd just print something along the lines of "Invalid
MESA_VK_WSI_PRESENT_MODE value, ignoring" and continue as if it wasn't
set at all.
On 2019-04-09 at 10:56, Michel Dänzer <michel@daenzer.net> wrote:
> On 2019-04-09 10:03 a.m., Eric Engestrom wrote:
> > On Tuesday, 2019-04-09 09:08:36 +0200, Samuel Pitoiset wrote:
> >>
> >> +      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");
> > 
> > unreachable() means the compiler is allowed to do anything, including
> > considering the application as dead.
> > 
> > Not something we want user to be able to do with a simple
> > `MESA_VK_WSI_PRESENT_MODE=0` :)
> > 
> > With an assert() instead, this is:
> > Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
> 
> With assertions enabled, the application is most certainly dead after a
> failed assertion. :)

Sure, but the difference is in prod, where you want to just silently ignore the invalid override, not die ;)

> 
> I'd just print something along the lines of "Invalid
> MESA_VK_WSI_PRESENT_MODE value, ignoring" and continue as if it wasn't
> set at all.

Sure, that works too 👍
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@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);


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