[PATCH] wsi_common_display: Deal with vscan values

[PATCH] wsi_common_display: Deal with vscan values

[Keith Packard]

We sorted out what 'vscan' means and are trying to use it correctly.

vscan = 0 is the same as vscan = 1, which is slightly annoying; we use
MAX2(vscan, 1) everywhere.

randr doesn't pass vscan at all, so we set wsi mode vscan = 0.

The doublescan flag doubles the vscan value, so we don't need to deal
with that separately, we can just compare flags normally.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 src/vulkan/wsi/wsi_common_display.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/vulkan/wsi/wsi_common_display.c b/src/vulkan/wsi/wsi_common_display.c
index c7f794a0eff..de1c1826bd2 100644
--- a/src/vulkan/wsi/wsi_common_display.c
+++ b/src/vulkan/wsi/wsi_common_display.c
@@ -149,7 +149,7 @@ wsi_display_mode_matches_drm(wsi_display_mode *wsi,
       wsi->vsync_start == drm->vsync_start &&
       wsi->vsync_end == drm->vsync_end &&
       wsi->vtotal == drm->vtotal &&
-      wsi->vscan == drm->vscan &&
+      MAX2(wsi->vscan, 1) == MAX2(drm->vscan, 1) &&
       wsi->flags == drm->flags;
 }
 
@@ -158,7 +158,7 @@ wsi_display_mode_refresh(struct wsi_display_mode *wsi)
 {
    return (double) wsi->clock * 1000.0 / ((double) wsi->htotal *
                                           (double) wsi->vtotal *
-                                          (double) (wsi->vscan + 1));
+                                          (double) MAX2(wsi->vscan, 1));
 }
 
 static uint64_t wsi_get_current_monotonic(void)
@@ -1657,6 +1657,7 @@ wsi_display_mode_matches_x(struct wsi_display_mode *wsi,
       wsi->vsync_start == xcb->vsync_start &&
       wsi->vsync_end == xcb->vsync_end &&
       wsi->vtotal == xcb->vtotal &&
+      wsi->vscan <= 1 && 
       wsi->flags == xcb->mode_flags;
 }
 
@@ -1707,8 +1708,6 @@ wsi_display_register_x_mode(struct wsi_device *wsi_device,
    display_mode->vsync_end = x_mode->vsync_end;
    display_mode->vtotal = x_mode->vtotal;
    display_mode->vscan = 0;
-   if (x_mode->mode_flags & XCB_RANDR_MODE_FLAG_DOUBLE_SCAN)
-      display_mode->vscan = 1;
    display_mode->flags = x_mode->mode_flags;
 
    list_addtail(&display_mode->list, &connector->display_modes);
-- 
2.17.1

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] wsi_common_display: Deal with vscan values

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 2651 bytes --]

Looks good to me.  With this properly sprinkled on the appropriate patches,
the entire series is

Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>

On Thu, Jun 14, 2018 at 5:57 PM, Keith Packard <keithp@keithp.com> wrote:

> We sorted out what 'vscan' means and are trying to use it correctly.
>
> vscan = 0 is the same as vscan = 1, which is slightly annoying; we use
> MAX2(vscan, 1) everywhere.
>
> randr doesn't pass vscan at all, so we set wsi mode vscan = 0.
>
> The doublescan flag doubles the vscan value, so we don't need to deal
> with that separately, we can just compare flags normally.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  src/vulkan/wsi/wsi_common_display.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/src/vulkan/wsi/wsi_common_display.c
> b/src/vulkan/wsi/wsi_common_display.c
> index c7f794a0eff..de1c1826bd2 100644
> --- a/src/vulkan/wsi/wsi_common_display.c
> +++ b/src/vulkan/wsi/wsi_common_display.c
> @@ -149,7 +149,7 @@ wsi_display_mode_matches_drm(wsi_display_mode *wsi,
>        wsi->vsync_start == drm->vsync_start &&
>        wsi->vsync_end == drm->vsync_end &&
>        wsi->vtotal == drm->vtotal &&
> -      wsi->vscan == drm->vscan &&
> +      MAX2(wsi->vscan, 1) == MAX2(drm->vscan, 1) &&
>        wsi->flags == drm->flags;
>  }
>
> @@ -158,7 +158,7 @@ wsi_display_mode_refresh(struct wsi_display_mode *wsi)
>  {
>     return (double) wsi->clock * 1000.0 / ((double) wsi->htotal *
>                                            (double) wsi->vtotal *
> -                                          (double) (wsi->vscan + 1));
> +                                          (double) MAX2(wsi->vscan, 1));
>  }
>
>  static uint64_t wsi_get_current_monotonic(void)
> @@ -1657,6 +1657,7 @@ wsi_display_mode_matches_x(struct wsi_display_mode
> *wsi,
>        wsi->vsync_start == xcb->vsync_start &&
>        wsi->vsync_end == xcb->vsync_end &&
>        wsi->vtotal == xcb->vtotal &&
> +      wsi->vscan <= 1 &&
>        wsi->flags == xcb->mode_flags;
>  }
>
> @@ -1707,8 +1708,6 @@ wsi_display_register_x_mode(struct wsi_device
> *wsi_device,
>     display_mode->vsync_end = x_mode->vsync_end;
>     display_mode->vtotal = x_mode->vtotal;
>     display_mode->vscan = 0;
> -   if (x_mode->mode_flags & XCB_RANDR_MODE_FLAG_DOUBLE_SCAN)
> -      display_mode->vscan = 1;
>     display_mode->flags = x_mode->mode_flags;
>
>     list_addtail(&display_mode->list, &connector->display_modes);
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>

[-- Attachment #1.2: Type: text/html, Size: 3888 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] wsi_common_display: Deal with vscan values

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 584 bytes --]

Jason Ekstrand <jason@jlekstrand.net> writes:

> Looks good to me.  With this properly sprinkled on the appropriate patches,
> the entire series is
>
> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>

Thanks so much! I've rebased the series onto current master and pushed
it back to my gitlab repo here

        https://gitlab.freedesktop.org/keithp/mesa/tree/drm-lease

I'll be doing testing tomorrow morning, and if it all looks good on both
anv and radv, I'll get it merged and pushed to master.

Now to get the next series ready for review :-)


-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [PATCH] wsi_common_display: Deal with vscan values

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 1198 bytes --]

Before we get too happy to merge things, I ran the CTS tests and there are
some failures...  I've attached a fixup patch that fixes three bugs I found:

 1) We weren't setting planeReorderPossible at all and we were using 0
instead of VK_FALSE (they're the same but we should use the enum) for
persistentContent
 2) We weren't advertising disconnected connectors via
GetPhysicalDeviceDisplayProperties but were returning them from
GetPhysicalDeviceDisplayPlaneProperties.
 3) We weren't setting result if the condition variable failed to
initialize (thanks GCC!)
​
There is one outstanding issue that the CTS is complaining about, namely
that you can't create modes.  It tests that mode creation fails for a mode
with a zero width, height, or refresh rate and that's all fine.  It then
tries to re-create one of the modes that we've returned to it in
GetDisplayModeProperties and assumes that it will work.  We should probably
at least make sure that works by walking the list and looking for a mode
that matches the requested one and returning it.  I don't think anything
actually requires us to return a unique pointer so it can be a search
instead of a create.

--Jason

[-- Attachment #1.2: Type: text/html, Size: 1363 bytes --]

[-- Attachment #2: 0001-Fixups.patch --]
[-- Type: text/x-patch, Size: 1674 bytes --]

From 4fbd63dc00a17f3ec8f71ec65a0b444316bac95f Mon Sep 17 00:00:00 2001
From: Jason Ekstrand <jason.ekstrand@intel.com>
Date: Thu, 14 Jun 2018 23:22:17 -0700
Subject: [PATCH] Fixups

---
 src/vulkan/wsi/wsi_common_display.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/vulkan/wsi/wsi_common_display.c b/src/vulkan/wsi/wsi_common_display.c
index e140e71c518..d1453368dfd 100644
--- a/src/vulkan/wsi/wsi_common_display.c
+++ b/src/vulkan/wsi/wsi_common_display.c
@@ -384,7 +384,8 @@ wsi_display_fill_in_display_properties(struct wsi_device *wsi_device,
       floor(properties->physicalResolution.height * MM_PER_PIXEL + 0.5);
 
    properties->supportedTransforms = VK_SURFACE_TRANSFORM_IDENTITY_BIT_KHR;
-   properties->persistentContent = 0;
+   properties->planeReorderPossible = VK_FALSE;
+   properties->persistentContent = VK_FALSE;
 }
 
 /*
@@ -488,7 +489,7 @@ wsi_display_get_display_plane_supported_displays(
    int c = 0;
 
    wsi_for_each_connector(connector, wsi) {
-      if (c == plane_index) {
+      if (c == plane_index && connector->connected) {
          vk_outarray_append(&conn, display) {
             *display = wsi_display_connector_to_handle(connector);
          }
@@ -1387,8 +1388,10 @@ wsi_display_init_wsi(struct wsi_device *wsi_device,
       goto fail_mutex;
    }
 
-   if (!wsi_init_pthread_cond_monotonic(&wsi->wait_cond))
+   if (!wsi_init_pthread_cond_monotonic(&wsi->wait_cond)) {
+      result = VK_ERROR_OUT_OF_HOST_MEMORY;
       goto fail_cond;
+   }
 
    wsi->base.get_support = wsi_display_surface_get_support;
    wsi->base.get_capabilities = wsi_display_surface_get_capabilities;
-- 
2.17.1


[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] wsi_common_display: Deal with vscan values

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1460 bytes --]

Jason Ekstrand <jason@jlekstrand.net> writes:

>  1) We weren't setting planeReorderPossible at all and we were using 0
> instead of VK_FALSE (they're the same but we should use the enum) for
> persistentContent
>  2) We weren't advertising disconnected connectors via
> GetPhysicalDeviceDisplayProperties but were returning them from
> GetPhysicalDeviceDisplayPlaneProperties.
>  3) We weren't setting result if the condition variable failed to
> initialize (thanks GCC!)
> ​
> There is one outstanding issue that the CTS is complaining about, namely
> that you can't create modes.  It tests that mode creation fails for a mode
> with a zero width, height, or refresh rate and that's all fine.  It then
> tries to re-create one of the modes that we've returned to it in
> GetDisplayModeProperties and assumes that it will work.  We should probably
> at least make sure that works by walking the list and looking for a mode
> that matches the requested one and returning it.  I don't think anything
> actually requires us to return a unique pointer so it can be a search
> instead of a create.

And that seems to at least make CTS happy. I've merged your fixes into
the first patch, added support for vkCreateDisplayModeKHR, rebased to
master and pushed the results to my repository.

It looks like we're done here but I'll wait until I hear from you before
pushing to master in case you've got additional concerns.

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Mesa-dev] [PATCH] wsi_common_display: Deal with vscan values

[Jason Ekstrand]

[-- Attachment #1.1: Type: text/plain, Size: 1624 bytes --]

On Tue, Jun 19, 2018 at 1:56 PM, Keith Packard <keithp@keithp.com> wrote:

> Jason Ekstrand <jason@jlekstrand.net> writes:
>
> >  1) We weren't setting planeReorderPossible at all and we were using 0
> > instead of VK_FALSE (they're the same but we should use the enum) for
> > persistentContent
> >  2) We weren't advertising disconnected connectors via
> > GetPhysicalDeviceDisplayProperties but were returning them from
> > GetPhysicalDeviceDisplayPlaneProperties.
> >  3) We weren't setting result if the condition variable failed to
> > initialize (thanks GCC!)
> > ​
> > There is one outstanding issue that the CTS is complaining about, namely
> > that you can't create modes.  It tests that mode creation fails for a
> mode
> > with a zero width, height, or refresh rate and that's all fine.  It then
> > tries to re-create one of the modes that we've returned to it in
> > GetDisplayModeProperties and assumes that it will work.  We should
> probably
> > at least make sure that works by walking the list and looking for a mode
> > that matches the requested one and returning it.  I don't think anything
> > actually requires us to return a unique pointer so it can be a search
> > instead of a create.
>
> And that seems to at least make CTS happy. I've merged your fixes into
> the first patch, added support for vkCreateDisplayModeKHR, rebased to
> master and pushed the results to my repository.
>
> It looks like we're done here but I'll wait until I hear from you before
> pushing to master in case you've got additional concerns.
>

Looks good.  Passes the CTS.  Push it!

[-- Attachment #1.2: Type: text/html, Size: 2233 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] wsi_common_display: Deal with vscan values

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 166 bytes --]

Jason Ekstrand <jason@jlekstrand.net> writes:

> Looks good.  Passes the CTS.  Push it!

All done. Now just two more series to go in this set :-)

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [PATCH] wsi_common_display: Deal with vscan values

[Ville Syrjälä]

On Thu, Jun 14, 2018 at 05:57:01PM -0700, Keith Packard wrote:
> We sorted out what 'vscan' means and are trying to use it correctly.
> 
> vscan = 0 is the same as vscan = 1, which is slightly annoying; we use
> MAX2(vscan, 1) everywhere.
> 
> randr doesn't pass vscan at all, so we set wsi mode vscan = 0.
> 
> The doublescan flag doubles the vscan value, so we don't need to deal
> with that separately, we can just compare flags normally.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  src/vulkan/wsi/wsi_common_display.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/src/vulkan/wsi/wsi_common_display.c b/src/vulkan/wsi/wsi_common_display.c
> index c7f794a0eff..de1c1826bd2 100644
> --- a/src/vulkan/wsi/wsi_common_display.c
> +++ b/src/vulkan/wsi/wsi_common_display.c
> @@ -149,7 +149,7 @@ wsi_display_mode_matches_drm(wsi_display_mode *wsi,
>        wsi->vsync_start == drm->vsync_start &&
>        wsi->vsync_end == drm->vsync_end &&
>        wsi->vtotal == drm->vtotal &&
> -      wsi->vscan == drm->vscan &&
> +      MAX2(wsi->vscan, 1) == MAX2(drm->vscan, 1) &&
>        wsi->flags == drm->flags;
>  }
>  
> @@ -158,7 +158,7 @@ wsi_display_mode_refresh(struct wsi_display_mode *wsi)
>  {
>     return (double) wsi->clock * 1000.0 / ((double) wsi->htotal *
>                                            (double) wsi->vtotal *
> -                                          (double) (wsi->vscan + 1));
> +                                          (double) MAX2(wsi->vscan, 1));

Are you dealing with INTERLACE anywhere? The kernel generally operates
under the assumption that vrefresh == field rate.

>  }
>  
>  static uint64_t wsi_get_current_monotonic(void)
> @@ -1657,6 +1657,7 @@ wsi_display_mode_matches_x(struct wsi_display_mode *wsi,
>        wsi->vsync_start == xcb->vsync_start &&
>        wsi->vsync_end == xcb->vsync_end &&
>        wsi->vtotal == xcb->vtotal &&
> +      wsi->vscan <= 1 && 
>        wsi->flags == xcb->mode_flags;
>  }
>  
> @@ -1707,8 +1708,6 @@ wsi_display_register_x_mode(struct wsi_device *wsi_device,
>     display_mode->vsync_end = x_mode->vsync_end;
>     display_mode->vtotal = x_mode->vtotal;
>     display_mode->vscan = 0;
> -   if (x_mode->mode_flags & XCB_RANDR_MODE_FLAG_DOUBLE_SCAN)
> -      display_mode->vscan = 1;
>     display_mode->flags = x_mode->mode_flags;
>  
>     list_addtail(&display_mode->list, &connector->display_modes);
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev