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);
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
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 :-)
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
Jason Ekstrand jason@jlekstrand.net writes:
- 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.
On Tue, Jun 19, 2018 at 1:56 PM, Keith Packard keithp@keithp.com wrote:
Jason Ekstrand jason@jlekstrand.net writes:
- 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!
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 :-)
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
dri-devel@lists.freedesktop.org
-
Jason Ekstrand -
Keith Packard -
Ville Syrjälä