Re: [Mesa-dev] Playing with display timing -- VK_MESA_present_period
Michel Dänzer writes: > I assume 2. refers to the case of a single late frame, where the next > frame hitting the original absolute target would result in a second > visible stutter. While writing > https://gitlab.freedesktop.org/wayland/wayland-protocols/-/issues/22#note_548234 > , it occurred to me that although a relative target time may avoid > that second stutter, the audio stream needs to adapt to the visual > delay, likely producing audible artifacts instead? It's not obvious to > me that would be an overall win. (The only other way I can think of is > to re-synchronize later frames to the audio stream, but it's not clear > that this is possible without either producing visible stutter again, > or de-synced audio/video for a noticeable period of time) I think the idea is that the application would interpolate time values in the video stream to bring it back in sync with the expected time over a couple of frames. I think we could easily construct a demo which shows the difference and see what we think. I think we could ignore the audio stream as a 16ms lag between audio and video shouldn't be a big deal; we handle that in real life quite easily as that's the lag you get at a distance of about 5m. > P.S. Have you more formally proposed a Vulkan extension in the > meantime? If so, could you provide a reference to that here? No. If I had, the IP restrictions with Khronos would prevent me from discussing it here in any technical detail. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Why do vulkan display surfaces not support alpha blending?
Austin Shafer writes: > I'm just curious if there is a technical reason why blending isn't > allowed, as the vulkan spec seems to permit it. Just not implemented yet. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Playing with display timing -- VK_MESA_present_period
Michel Dänzer writes: > Should this extension specify how it interacts with the various > VK_PRESENT_MODE_* modes? Yes. It needs to be clear on how this extension interacts with all existing display stuff. Thanks for pointing out one pretty important interaction. > For one example: With VK_PRESENT_MODE_MAILBOX_KHR, does the period > specified by this extension correspond to: > > > a) The time between when the image is placed in the the queue of > pending presentation requests and when the next image is placed in the > queue > > b) The time between when the image is taken from the queue to be > actually presented and when the same thing happens for another image > (which happens to be in the queue at the time) > > c) Yet something else? > > If it's a), given the extension talks about rounding to the nearest > upcoming frame, does VK_PRESENT_MODE_MAILBOX_KHR effectively behave > the same as VK_PRESENT_MODE_FIFO(_RELAXED)_KHR with this extension? > > If it's b), there can be any number of images entering and leaving the > queue during the period, so it's not clear what purpose the period > would serve? Given that the period is defined as being relative to the time when the image was presented to the screen (not when the image is queued for presentation), and that the extension specifies that future images will be delayed by that period, I think the right definition will be that specifying non-zero present_period for a QueuePresent call will force images queued later to not replace the first image and be delayed for display until the specified present_period has passed. Which looks a lot like FIFO, but only for QueuePresent calls which specify a non-zero present_period. I think I've got enough to start writing a more 'formal' specification for the extension, which I'll do as a patch to the Vulkan specification. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Playing with display timing -- VK_MESA_present_period
Michel Dänzer writes: > I think at least the following needs to happen first: > > * At least a prototype of plumbing through this information to the > amdgpu kernel driver (or another one which may grow VRR support) and > making use of it for adjusting the refresh periods to allow hitting > the target as closely as possible. > > * At least a prototype of a game engine making use of it to control > the variable refresh rate. > > This will allow confirming that this approach actually works and > provides the sought benefit. Awesome. Thanks for the recommendation. I can work on this. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Playing with display timing -- VK_MESA_present_period
Michel Dänzer writes: > With variable refresh rate, it could certainly be useful for the kernel > to have this information as early as possible. It shouldn't make any > difference with fixed refresh though, since the kernel should always be > able to hit the next refresh in that case as long as the fences for the > flip signal in time for vertical blank. Although, if the application is mixing present_period and display_timing operations, things can still get mixed up -- a frame with present_period followed by a frame with display_timing can invalidate the computed next frame time. Applications doing that may not get the best possible result... Ok, it sounds like I should at least go clean up the implementation and make it into something not quite so embarrassing to me. Going forward, how can we provide this to application developers for experimentation? Would we consider including it in a release once reviewed within the Mesa community? -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Playing with display timing -- VK_MESA_present_period
Michel Dänzer writes: >> actual_period = n * r - ε > > Is that still an issue if the (minimal) length of a vertical blanking > period is subtracted from the computed period? What you're doing is defining a 'window' of times which match the desired frame -- any time within 'length of vertical blanking period' of the actual top of frame matches that frame. I'm simply making that window as large as possible by using 'half the frame time' instead of 'length of vertical blanking period'. > FWIW, one thing making "not before" semantics attractive is that > they're easy to achieve in the kernel: It can just make sure the page > flip isn't programmed to hardware before the target time. Yup, offering only 'not before' makes it easy to implement in the kernel, but very difficult for the application to get the right result. For fixed refresh rate monitors, converting from 'nearest to time T' to 'not before frame count F' conversion can be done by rounding T/rate to F and then using that to program the hardware using 'not before' semantics. For variable refresh rate monitors, the computation will be slightly more complicated as it may involve some multiple of a nominal frame rate plus some stretching of the final frame. > PresentOptionUST has never been functional, so there can't be any > clients relying on specific semantics (other than being a no-op :) for > it. Therefore, we could still change its semantics before making it > functional, FWIW. Oops. Someone should fix that :-) -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Playing with display timing -- VK_MESA_present_period
Michel Dänzer writes: > Hmm, I didn't fully realize this in my reading of the draft, thanks > Matias for pointing it out! > > That the timing of frame N+1 has to be specified when submitting frame > N for presentation is odd to me TBH. While I can imagine this might be > suitable for some apps such as video players, I'm skeptical about game > engines. They would need to calculate frame time budget and choose > simulation time for frame N+1 before submitting frame N for > presentation. Is that really how game engines (want to) work? It's not asking the application to figure this out much earlier -- the application needs to know the target time for the next frame before starting any of the frame computations, and that happens right after submitting the previous frame. The goal here is to provide the display system the timing information as soon as the application knows it, in case that helps the backend work better. > Instead, have you considered allowing the GOOGLE_display_timing > desiredPresentTime to be specified as a relative time, measured from > when the previous image of the swapchain was actually presented, > instead of as an absolute time? Could something like that also address > the motivation for VK_MESA_present_period? Yes, that would avoid the display problem. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Playing with display timing -- VK_MESA_present_period
"Matias N. Goldberg" writes: > I read your article. Thanks! > What I feel are missing are just minor pesky details: Yes, it's definitely a rough draft at best. Figuring out the right words to make it do precisely what we want is hard, and I'm hoping we can first figure out what we want, then figure out how to say that precisely. > 1. Written as is, the frame being submitted is rounded up to display > timing, delaying *future* frames.But there is no way to delay the > *currently displaying frame* (i.e. the 'previous' frame). Correct. The period provided controls how long the specified frame will be shown to the user (at a minimum). Future frames will be delayed by at least that long. > Right now if frame N was submitted without VkPresentTimeMESA but frame > N+1 is, then frame N+1 will be presented to screen ASAP. Correct. > What I'm saying is that if frame N was submitted without > VkPresentTimeMESA, then at frame N+1 I should be able to tell 'keep > frame N displayed for at least P nanoseconds since it was displayed, > and *then* present frame N+1, which is the frame I am now submitting' That's not what this extension does; if you wanted frame 'N' to be displayed for 'P' nanoseconds, then you would specify 'P' when queuing frame 'N'. > The API needs to be expanded further to explain Vulkan what a 'frame' > is.Is it the monitor's refresh rate? KHR_swapchain and EXT_display_surface_counter both mention 'vertical blanking periods'. GOOGLE_display_timing has vkGetRefreshCycleDurationGOOGLE which returns 'refreshDuration'. 'frame' is also used throughout Vulkan and seems to describe one of a sequence of images displayed to the user. Coming up with language which ties back into all of these would be helpful. > Or is it an arbitrary elapsed time defined in the form numerator and > denominator? (e.g. 60hz is numerator = 1, denominator = 60; 59.94hz is > numerator = 1001 denominator = 6000)By specifying arbitrary > definitions of a frame, it is possible to be compatible with variable > refresh rates e.g. for VRR monitors, applications may define > denominator = 240 or denominator = 120 When I talked about 'frames' in my informal description, I could have clarified that by referring to the GOOGLE_display_timing 'refreshDuration' value. I think that variable refresh rate monitors would be expected to set that to a useful value, possibly based on the nominal display period, but I don't know. I think that we'll need some more Vulkan extensions designed to expose the capabilities and limits of variable refresh rate monitors. Do we need to do that in conjunction with this extension, or can we define this extension in such a way that it should work with whatever we end up with in the future? -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Playing with display timing -- VK_MESA_present_period
Michel Dänzer writes: > Instead of rounding to the nearest upcoming frame, should it round to > the earliest frame after the specified period has passed? As written, it > seems to contradict the next paragraph below: Sorry for the poor wording; let me describe what I want informally here. For nanosecond periods to be easy to use on fixed-refresh-rate monitors, I want the näive computation to "just work". For a given refresh period, 'r', I want to select a period of 'n' frames using: computed_period = n * r Because of clock skew and rounding problems, it's quite possible that the period could easily end up being just slightly smaller than that: actual_period = n * r - ε When I said 'nearest', what I intended to describe was that the target frame would be as close as possible to the specified time. > (I'm not ruling out that rounding to nearest might be better, but there > should be a rationale for it, which also justifies being inconsistent > with GOOGLE_display_timing) Yes, this is intentionally inconsistent with GOOGLE_display_timing, which I believe is hard to use correctly. By specifying not before semantics, GOOGLE_display_timing requires applications compute a fake time guaranteed to be not after the actual target frame time. This really messes things up when you might have variable refresh rate monitors. I just went and read the time-based stuff from the X Present extension. That uses 'nearest' semantics too, when supported by the driver. When not supported, the server gets to do whatever it likes (oops!). > I like this extension in general. Thanks! I've been trying to get this posted for a few months now. > However, I think allowing the period to be specified in frames might be > a mistake, because it won't work well with variable refresh rate. But > it'll be tempting for application / toolkit developers not to bother > with proper time calculations but to just use frames instead. (It would > be good to seek feedback on this from AMD DC developers and the larger > DRM kernel driver community as well) Yeah, given that the application will need the refresh period to know what to display, it certainly doesn't provide much technical benefit here. I stuck it in to make the extension feel like GL's swap interval stuff (although it isn't the same), and because it seemed like a 'nice' thing to offer applications. > P.S. It would be good to create a WIP merge request for this in the main > Mesa project, to have a central place for gathering feedback and > tracking progress. Done, thanks for the suggestion. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Playing with display timing -- VK_MESA_present_period
I spent some time over the last week experimenting with a different way
of doing frame timing.
Instead of specifying *when* a particular frame should be displayed, how
about we specify how *long* a particular frame should be made visible
to the user?
This has a couple of advantages over the approach taken in
GOOGLE_display_timing:
1. It provides information to the backend about frame timing
a frame earlier.
2. Missing a frame can generate fewer artifacts.
Here's what I'm thinking the extension would look like:
An application uses VK_MESA_present_period by including a
VkPresentPeriodMESA structure in the pNext chain in the
VkPresentInfoKHR structure passed to the vkQueuePresentKHR call.
typedef struct VkPresentPeriodMESA {
VkStructureTypesType;
const void*pNext;
uint32_t swapchainCount;
const int64_t* pPresentPeriods;
} VkPresentPeriodMESA;
The fields in this structure are:
* sType. Set to VK_STRUCTURE_TYPE_PRESENT_PERIOD_MESA
* pNext. Points to the next extension structure in the chain (if any).
* swapchainCount. A copy of the swapchainCount field in the
VkPresentInfoKHR structure.
* pPresentPeriods. An array, length swapchainCount, of presentation
periods for each image in the call.
Positive presentation periods represent nanoseconds. Negative
presentation periods represent frames. A zero value means the
extension does not affect the associated presentation.
Nanosecond values are rounded to the nearest upcoming frame so that a
value of n * refresh_interval is the same as using a value of n
frames.
The presentation period causes *future* images to be delayed at least
until the specified interval after this image has been
presented. Specifying both a presentation period in a previous frame
and using GOOGLE_display_timing is well defined -- the presentation
will be delayed until the later of the two times.
I've got this kludged together to experiment with; I managed to make it
work purely within Vulkan using the infrastructure built for
GOOGLE_display_timing.
https://gitlab.freedesktop.org/keithp/mesa/commits/present-period
I wrote a longer article on my blog:
https://keithp.com/blogs/present-period/
I'm interested in hearing what people think about the general approach.
--
-keith
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] vulkan: Add VK_GOOGLE_display_timing extension (x11+display, anv+radv) [v8]
This adds support for the VK_GOOGLE_display timing extension, which
provides two things:
1) Detailed information about when frames are displayed, including
slack time between GPU execution and display frame.
2) Absolute time control over swapchain queue processing. This allows
the application to request frames be displayed at specific
absolute times, using the same timebase as that provided in vblank
events.
Support for this extension has been implemented for the x11 and
display backends; adding support to other backends should be
reasonable straightforward for one familiar with those systems and
should not require any additional device-specific code.
v2:
Adjust GOOGLE_display_timing earliest value. The
earliestPresentTime for an image cannot be before the previous
image was displayed, or even a frame later (in FIFO mode).
Make GOOGLE_display_timing use render completed time. Switch
from VK_PIPELINE_TOP_OF_PIPE_BIT to
VK_PIPELINE_STAGE_ALL_COMMANDS_BIT so that the time reported
to applications as the end of rendering reflects the latest
possible value to ensure that applications don't underestimate
the amount of work done in the frame.
v3:
Adopt Jason Ekstrand's coding conventions. Declare variables
at first use, eliminate extra whitespace between types and
names. Wrap lines to 80 columns.
Suggested-by: Jason Ekstrand
v4:
Adapt to changes in MESA_query_timestamp extension
v5:
Squash core bits and anv/radv wrappers into a single patch
Suggested-by: Jason Ekstrand
v6:
Switch from MESA_query_timestamp to EXT_calibrated_timestamps
v7:
Ensure we target frame no earlier than desired. This means
rounding the target frame up, rather than selecting the
nearest one.
Suggested-by: Michel Dänzer
v8:
Re-order display_timing in anv_extensions.py. That code
now requires extensions in alphabetical order.
Rename wsi_mark_time to wsi_present_complete to make
the functionality clearer.
Signed-off-by: Keith Packard
---
src/amd/vulkan/radv_extensions.py | 1 +
src/amd/vulkan/radv_wsi.c | 33 +++
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_wsi.c | 31 +++
src/vulkan/wsi/wsi_common.c | 301 +++-
src/vulkan/wsi/wsi_common.h | 32 +++
src/vulkan/wsi/wsi_common_display.c | 163 ++-
src/vulkan/wsi/wsi_common_private.h | 35
src/vulkan/wsi/wsi_common_x11.c | 71 ++-
9 files changed, 656 insertions(+), 12 deletions(-)
diff --git a/src/amd/vulkan/radv_extensions.py
b/src/amd/vulkan/radv_extensions.py
index 57aa67be616..c255b49437a 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -166,6 +166,7 @@ EXTENSIONS = [
Extension('VK_AMD_shader_trinary_minmax', 1, True),
Extension('VK_GOOGLE_decorate_string',1, True),
Extension('VK_GOOGLE_hlsl_functionality1',1, True),
+Extension('VK_GOOGLE_display_timing', 1, True),
Extension('VK_NV_compute_shader_derivatives', 1,
'device->rad_info.chip_class >= GFX8'),
]
diff --git a/src/amd/vulkan/radv_wsi.c b/src/amd/vulkan/radv_wsi.c
index a2b0afa48c3..b722e23ff53 100644
--- a/src/amd/vulkan/radv_wsi.c
+++ b/src/amd/vulkan/radv_wsi.c
@@ -316,3 +316,36 @@ VkResult radv_GetPhysicalDevicePresentRectanglesKHR(
surface,
pRectCount, pRects);
}
+
+/* VK_GOOGLE_display_timing */
+VkResult
+radv_GetRefreshCycleDurationGOOGLE(
+ VkDevice _device,
+ VkSwapchainKHR swapchain,
+ VkRefreshCycleDurationGOOGLE *pDisplayTimingProperties)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ struct radv_physical_device *pdevice = device->physical_device;
+
+ return wsi_common_get_refresh_cycle_duration(>wsi_device,
+_device,
+swapchain,
+pDisplayTimingProperties);
+}
+
+VkResult
+radv_GetPastPresentationTimingGOOGLE(VkDevice _device,
+VkSwapchainKHR swapchain,
+uint32_t *pPresentationTimingCount,
+VkPastPresentationTimingGOOGLE
+*pPresentationTimings)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ struct radv_physical_device *pdevice = device->physical_device;
+
+ return wsi_common_get_past_presentation_timing(>wsi_device,
+ _device,
+
Re: [Mesa-dev] [PATCH mesa] wsi/display: fix mem leak when freeing swapchains
Eric Engestrom writes: > Fixes: da997ebec92942193955 "vulkan: Add KHR_display extension using DRM > [v10]" > Cc: Keith Packard > Signed-off-by: Eric Engestrom Reviewed-by: Keith Packard I checked to ensure that this is sufficient to clean all allocated objects: * Creating a display swapchain allocates the chain, image_count images and call wsi_swapchain_init. Adding the wsi_swapchain_finish cleans the last of these allocations. * The queue of to-be-presented images exists only as state values within the images themselves, so there's no additional allocation there. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] vulkan: Add VK_GOOGLE_display_timing extension (x11+display, anv+radv) [v7]
This adds support for the VK_GOOGLE_display timing extension, which
provides two things:
1) Detailed information about when frames are displayed, including
slack time between GPU execution and display frame.
2) Absolute time control over swapchain queue processing. This allows
the application to request frames be displayed at specific
absolute times, using the same timebase as that provided in vblank
events.
Support for this extension has been implemented for the x11 and
display backends; adding support to other backends should be
reasonable straightforward for one familiar with those systems and
should not require any additional device-specific code.
v2:
Adjust GOOGLE_display_timing earliest value. The
earliestPresentTime for an image cannot be before the previous
image was displayed, or even a frame later (in FIFO mode).
Make GOOGLE_display_timing use render completed time. Switch
from VK_PIPELINE_TOP_OF_PIPE_BIT to
VK_PIPELINE_STAGE_ALL_COMMANDS_BIT so that the time reported
to applications as the end of rendering reflects the latest
possible value to ensure that applications don't underestimate
the amount of work done in the frame.
v3:
Adopt Jason Ekstrand's coding conventions. Declare variables
at first use, eliminate extra whitespace between types and
names. Wrap lines to 80 columns.
Suggested-by: Jason Ekstrand
v4:
Adapt to changes in MESA_query_timestamp extension
v5:
Squash core bits and anv/radv wrappers into a single patch
Suggested-by: Jason Ekstrand
v6:
Switch from MESA_query_timestamp to EXT_calibrated_timestamps
v7:
Ensure we target frame no earlier than desired. This means
rounding the target frame up, rather than selecting the
nearest one.
Suggested-by: Michel Dänzer
Signed-off-by: Keith Packard
---
src/amd/vulkan/radv_extensions.py | 1 +
src/amd/vulkan/radv_wsi.c | 33 +++
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_wsi.c | 33 +++
src/vulkan/wsi/wsi_common.c | 303 +++-
src/vulkan/wsi/wsi_common.h | 32 +++
src/vulkan/wsi/wsi_common_display.c | 165 ++-
src/vulkan/wsi/wsi_common_private.h | 35
src/vulkan/wsi/wsi_common_x11.c | 71 ++-
9 files changed, 662 insertions(+), 12 deletions(-)
diff --git a/src/amd/vulkan/radv_extensions.py
b/src/amd/vulkan/radv_extensions.py
index 6bdf988d117..76c3ade06f0 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -119,6 +119,7 @@ EXTENSIONS = [
Extension('VK_AMD_shader_trinary_minmax', 1, True),
Extension('VK_GOOGLE_decorate_string',1, True),
Extension('VK_GOOGLE_hlsl_functionality1',1, True),
+Extension('VK_GOOGLE_display_timing', 1, True),
]
class VkVersion:
diff --git a/src/amd/vulkan/radv_wsi.c b/src/amd/vulkan/radv_wsi.c
index 346fb43d675..ba24d07edfc 100644
--- a/src/amd/vulkan/radv_wsi.c
+++ b/src/amd/vulkan/radv_wsi.c
@@ -295,3 +295,36 @@ VkResult radv_GetPhysicalDevicePresentRectanglesKHR(
surface,
pRectCount, pRects);
}
+
+/* VK_GOOGLE_display_timing */
+VkResult
+radv_GetRefreshCycleDurationGOOGLE(
+ VkDevice _device,
+ VkSwapchainKHR swapchain,
+ VkRefreshCycleDurationGOOGLE *pDisplayTimingProperties)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ struct radv_physical_device *pdevice = device->physical_device;
+
+ return wsi_common_get_refresh_cycle_duration(>wsi_device,
+_device,
+swapchain,
+pDisplayTimingProperties);
+}
+
+VkResult
+radv_GetPastPresentationTimingGOOGLE(VkDevice _device,
+VkSwapchainKHR swapchain,
+uint32_t *pPresentationTimingCount,
+VkPastPresentationTimingGOOGLE
+*pPresentationTimings)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ struct radv_physical_device *pdevice = device->physical_device;
+
+ return wsi_common_get_past_presentation_timing(>wsi_device,
+ _device,
+ swapchain,
+ pPresentationTimingCount,
+ pPresentationTimings);
+}
diff --git a/src/intel/vulkan/anv_extensions.py
b/src/intel/vulkan/anv_extensions.py
index e9afe06bb13..8fcc4d1376e 100644
--- a/src/intel/vulkan/anv_extensions.py
+++
Re: [Mesa-dev] [PATCH] vulkan: Add VK_GOOGLE_display_timing extension (x11+display, anv+radv) [v6]
Michel Dänzer writes: Thanks for taking time to review this patch! >> + int64_t refresh = (int64_t) refresh_timing.refreshDuration; >> + int64_t frames = (delta_nsec + refresh/2) / refresh; > > desiredPresentTime has "no sooner than" semantics, so I think this should be > >int64_t frames = (delta_nsec + refresh-1) / refresh; Hrm. You're certainly right that we want to make sure to not hit the wrong frame, and we need to be very careful with this computation. And that turns out to be 'hard'. With a naïve computation of frame times: future_frame_time = past_frame_time + n * refresh If the reported refresh is longer than the actual interval, due to rounding of that value or clock skew, this computation might select frame n+1 if the driver uses a later frame for its basis than the application: desiredPresentTime = application_past_frame_time + n * refresh delta_nsec = (desiredPresentTime - driver_past_frame_time); frames = (delta_nsec + refresh-1) / refresh; If 'driver_past_frame_time' was sampled 'm' frames after 'application_past_frame time', and 'refresh' is longer than the actual frame time: desiredPresentTime > driver_past_frame_time + m * refresh Because desiredPresentTime is *past* our estimate of the beginning of the frame the application wants, and because we're rounding the selected frame up, we end up targeting one frame too late. Now, if we use my value for 'frames', then we hit the right frame using this value, as long as the error is less than 1/2 frame time: desiredPresentTime > driver_past_frame_time + m * refresh desiredPresentTime < driver_past_frame_time + m * refresh + refresh/2 delta_nsec > m * refresh delta_nsec < m * refresh + refresh / frames > (m * refresh + refresh/2) / refresh frames < (m * refresh + refresh) / refresh With this computation, frames = m, which is the desired result. So at least you can see where my code came from. But, it's clearly wrong according to the spec, as you'll see in the next section. An application can attempt to compensate for this by using an earlier time; a slightly less naïve computation might look like: future_frame_time = past_frame_time + 1 + (n-1) * refresh This makes 'future_frame_time' be the earliest possible time that should target the desired frame, given the 'no sooner than' semantics in the spec. If the reported refresh is shorter than the actual interval, this computation might hit frame (n-1). Ok, so now we make the application even 'smarter' by having it compute a time in the center of the target frame: future_frame_time = past_frame_time + (refresh/2) + (n-1) * refresh With your suggested code, this will hit the desired frame unless the error in the frame time is more than 1/2 of the refresh interval, which seems pretty good. Ok, so what can we do? I think we start with what we know: * driver_past_frame_time >= application_past_frame_time Because all application frame time information comes from the driver, we just need to use the latest possible frame time in the driver to keep this true. Now, what will cause errors in the 'refresh' value? 'refresh' error is a combination of rounding error and CPU vs GPU clock skew. * Rounding error. This is always less than 1ns. * Clock skew is related to the performance of a couple of crystals in the system. Even cheap crystals provide significantly better than 100ppm (parts per million) performance. At 30Hz, refresh_interval is 33.3ms, or 33,333,333ns, so each crystals will have a maximum error of 3300ns; combine two and we've a maximum error of 6600ns. As you can see, the rounding error is lost in the noise here, unless we find a system that uses CLOCK_MONOTONIC for display timing. It'll take 5000 frames before that error reaches a frame time. As long as the application_past_frame_time is within 2500 frames of the driver_past_frame_time, the error in the future_frame_time estimate will be within one-half frame, and our application will work reliably using the 'smarter' computation of future frame time. I would prefer to let applications use the initial naïve future_frame_time estimate, as I think that could also work with variable refresh timing, but that would require a fairly complicated change in the specification. >> + timing->target_msc = swapchain->frame_msc + frames; >> +} >> + } > > Note that MSC based timing won't work well with variable refresh rate. > In the long term, support for PresentOptionUST should be implemented and > used. Agreed. Given the above discussion, I think that will have to wait for a more sophisticated specification for what 'desiredPresentTime' means, as I think the current specification makes it "impossible" to provide an actual desiredPresentTime to the interface without that causing occasional incorrect frame
[Mesa-dev] [PATCH] vulkan: Add VK_GOOGLE_display_timing extension (x11+display, anv+radv) [v6]
This adds support for the VK_GOOGLE_display timing extension, which
provides two things:
1) Detailed information about when frames are displayed, including
slack time between GPU execution and display frame.
2) Absolute time control over swapchain queue processing. This allows
the application to request frames be displayed at specific
absolute times, using the same timebase as that provided in vblank
events.
Support for this extension has been implemented for the x11 and
display backends; adding support to other backends should be
reasonable straightforward for one familiar with those systems and
should not require any additional device-specific code.
v2:
Adjust GOOGLE_display_timing earliest value. The
earliestPresentTime for an image cannot be before the previous
image was displayed, or even a frame later (in FIFO mode).
Make GOOGLE_display_timing use render completed time. Switch
from VK_PIPELINE_TOP_OF_PIPE_BIT to
VK_PIPELINE_STAGE_ALL_COMMANDS_BIT so that the time reported
to applications as the end of rendering reflects the latest
possible value to ensure that applications don't underestimate
the amount of work done in the frame.
v3:
Adopt Jason Ekstrand's coding conventions. Declare variables
at first use, eliminate extra whitespace between types and
names. Wrap lines to 80 columns.
Suggested-by: Jason Ekstrand
v4:
Adapt to changes in MESA_query_timestamp extension
v5:
Squash core bits and anv/radv wrappers into a single patch
Suggested-by: Jason Ekstrand
v6:
Switch from MESA_query_timestamp to EXT_calibrated_timestamps
Signed-off-by: Keith Packard
---
src/amd/vulkan/radv_extensions.py | 1 +
src/amd/vulkan/radv_wsi.c | 33 +++
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_wsi.c | 33 +++
src/vulkan/wsi/wsi_common.c | 303 +++-
src/vulkan/wsi/wsi_common.h | 32 +++
src/vulkan/wsi/wsi_common_display.c | 165 ++-
src/vulkan/wsi/wsi_common_private.h | 35
src/vulkan/wsi/wsi_common_x11.c | 71 ++-
9 files changed, 662 insertions(+), 12 deletions(-)
diff --git a/src/amd/vulkan/radv_extensions.py
b/src/amd/vulkan/radv_extensions.py
index 6bdf988d117..76c3ade06f0 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -119,6 +119,7 @@ EXTENSIONS = [
Extension('VK_AMD_shader_trinary_minmax', 1, True),
Extension('VK_GOOGLE_decorate_string',1, True),
Extension('VK_GOOGLE_hlsl_functionality1',1, True),
+Extension('VK_GOOGLE_display_timing', 1, True),
]
class VkVersion:
diff --git a/src/amd/vulkan/radv_wsi.c b/src/amd/vulkan/radv_wsi.c
index 346fb43d675..ba24d07edfc 100644
--- a/src/amd/vulkan/radv_wsi.c
+++ b/src/amd/vulkan/radv_wsi.c
@@ -295,3 +295,36 @@ VkResult radv_GetPhysicalDevicePresentRectanglesKHR(
surface,
pRectCount, pRects);
}
+
+/* VK_GOOGLE_display_timing */
+VkResult
+radv_GetRefreshCycleDurationGOOGLE(
+ VkDevice _device,
+ VkSwapchainKHR swapchain,
+ VkRefreshCycleDurationGOOGLE *pDisplayTimingProperties)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ struct radv_physical_device *pdevice = device->physical_device;
+
+ return wsi_common_get_refresh_cycle_duration(>wsi_device,
+_device,
+swapchain,
+pDisplayTimingProperties);
+}
+
+VkResult
+radv_GetPastPresentationTimingGOOGLE(VkDevice _device,
+VkSwapchainKHR swapchain,
+uint32_t *pPresentationTimingCount,
+VkPastPresentationTimingGOOGLE
+*pPresentationTimings)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ struct radv_physical_device *pdevice = device->physical_device;
+
+ return wsi_common_get_past_presentation_timing(>wsi_device,
+ _device,
+ swapchain,
+ pPresentationTimingCount,
+ pPresentationTimings);
+}
diff --git a/src/intel/vulkan/anv_extensions.py
b/src/intel/vulkan/anv_extensions.py
index e9afe06bb13..8fcc4d1376e 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -130,6 +130,7 @@ EXTENSIONS = [
Extension('VK_EXT_calibrated_timestamps', 1, True),
Extension('VK_GOOGLE_decorate_string',
Re: [Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v5]
Bas Nieuwenhuizen writes: > Reviewed-by: Bas Nieuwenhuizen Thanks to you, Jason and Lionel for reviewing the code and helping improve it. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v5]
Jason Ekstrand writes: > I like it When the comments are longer than the code, you know you're done? -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v5]
Offers three clocks, device, clock monotonic and clock monotonic
raw. Could use some kernel support to reduce the deviation between
clock values.
v2:
Ensure deviation is at least as big as the GPU time interval.
v3:
Set device->lost when returning DEVICE_LOST.
Use MAX2 and DIV_ROUND_UP instead of open coding these.
Delete spurious TIMESTAMP in radv version.
Suggested-by: Jason Ekstrand
Suggested-by: Lionel Landwerlin
v4:
Add anv_gem_reg_read to anv_gem_stubs.c
Suggested-by: Jason Ekstrand
v5:
Adjust maxDeviation computation to max(sampled_clock_period) +
sample_interval.
Suggested-by: Bas Nieuwenhuizen
Suggested-by: Jason Ekstrand
Signed-off-by: Keith Packard
---
src/amd/vulkan/radv_device.c | 119 +++
src/amd/vulkan/radv_extensions.py | 1 +
src/intel/vulkan/anv_device.c | 127 +
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_gem.c | 13 +++
src/intel/vulkan/anv_gem_stubs.c | 7 ++
src/intel/vulkan/anv_private.h | 2 +
7 files changed, 270 insertions(+)
diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 174922780fc..4a705a724ef 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -4955,3 +4955,122 @@ radv_GetDeviceGroupPeerMemoryFeatures(
VK_PEER_MEMORY_FEATURE_GENERIC_SRC_BIT |
VK_PEER_MEMORY_FEATURE_GENERIC_DST_BIT;
}
+
+static const VkTimeDomainEXT radv_time_domains[] = {
+ VK_TIME_DOMAIN_DEVICE_EXT,
+ VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT,
+ VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT,
+};
+
+VkResult radv_GetPhysicalDeviceCalibrateableTimeDomainsEXT(
+ VkPhysicalDevice physicalDevice,
+ uint32_t *pTimeDomainCount,
+ VkTimeDomainEXT *pTimeDomains)
+{
+ int d;
+ VK_OUTARRAY_MAKE(out, pTimeDomains, pTimeDomainCount);
+
+ for (d = 0; d < ARRAY_SIZE(radv_time_domains); d++) {
+ vk_outarray_append(, i) {
+ *i = radv_time_domains[d];
+ }
+ }
+
+ return vk_outarray_status();
+}
+
+static uint64_t
+radv_clock_gettime(clockid_t clock_id)
+{
+ struct timespec current;
+ int ret;
+
+ ret = clock_gettime(clock_id, );
+ if (ret < 0 && clock_id == CLOCK_MONOTONIC_RAW)
+ ret = clock_gettime(CLOCK_MONOTONIC, );
+ if (ret < 0)
+ return 0;
+
+ return (uint64_t) current.tv_sec * 10ULL + current.tv_nsec;
+}
+
+VkResult radv_GetCalibratedTimestampsEXT(
+ VkDevice _device,
+ uint32_t timestampCount,
+ const VkCalibratedTimestampInfoEXT *pTimestampInfos,
+ uint64_t *pTimestamps,
+ uint64_t *pMaxDeviation)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ uint32_t clock_crystal_freq =
device->physical_device->rad_info.clock_crystal_freq;
+ int d;
+ uint64_t begin, end;
+uint64_t max_clock_period = 0;
+
+ begin = radv_clock_gettime(CLOCK_MONOTONIC_RAW);
+
+ for (d = 0; d < timestampCount; d++) {
+ switch (pTimestampInfos[d].timeDomain) {
+ case VK_TIME_DOMAIN_DEVICE_EXT:
+ pTimestamps[d] = device->ws->query_value(device->ws,
+
RADEON_TIMESTAMP);
+uint64_t device_period = DIV_ROUND_UP(100,
clock_crystal_freq);
+max_clock_period = MAX2(max_clock_period,
device_period);
+ break;
+ case VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT:
+ pTimestamps[d] = radv_clock_gettime(CLOCK_MONOTONIC);
+max_clock_period = MAX2(max_clock_period, 1);
+ break;
+
+ case VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT:
+ pTimestamps[d] = begin;
+ break;
+ default:
+ pTimestamps[d] = 0;
+ break;
+ }
+ }
+
+ end = radv_clock_gettime(CLOCK_MONOTONIC_RAW);
+
+/*
+ * The maximum deviation is the sum of the interval over which we
+ * perform the sampling and the maximum period of any sampled
+ * clock. That's because the maximum skew between any two sampled
+ * clock edges is when the sampled clock with the largest period is
+ * sampled at the end of that period but right at the beginning of the
+ * sampling interval and some other clock is sampled ri
Re: [Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v4]
Jason Ekstrand writes: > Doing all of the CPU sampling on one side or the other of the GPU sampling > would probably reduce our window. True, although as I said, it's taking several µs to get through the loop, and the gpu clock tick is far smaller than that, so even adding the two values together to make it fit the current implementation won't make the deviation that much larger. > This leaves us with a delta of I + max(P(M), P(R), P(G)). In > particular, any two real-number valued times are, instantaneously, > within that interval. That, at least, would be easy to compute, and scale nicely if we added more clocks in the future. > Personally, I'm completely content to have the delta just be a the first > one: a bound on the difference between any two real-valued times. At this > point, I can guarantee you that far more thought has been put into this > mesa-dev discussion than was put into the spec and I think we're rapidly > getting to the point of diminishing returns. :-) It seems likely. How about we do the above computation for the current code and leave it at that? -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v4]
Jason Ekstrand writes: > The result is that we're looking at something like "end - start + > monotonic_raw_tick + max(gpu_tick, monotonic_tick)" Have I just come > full-circle? Yes. As monotonic_raw_tick and monotonic_tick are both 1... -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v4]
Bas Nieuwenhuizen writes: > You can make the monotonic case the same as the raw case if you make > sure to always sample the CPU first by e.g. splitting the loops into > two and doing CPU in the first and GPU in the second. That way you > make the case above impossible. Right, I had thought of that, and it probably solves the problem for us. If more time domains are added, things become 'more complicated' though. > That said "start of the interval of the tick" is kinda arbitrary and > you could pick random other points on that interval, so depending on > what requirements you put on it (i.e. can the chosen position be > different per call, consistent but implicit or explicitly picked which > might be leaked through the interface) the max deviation changes. So > depending on interpretation this thing can be very moot ... It doesn't really matter what phase you use; the timer increments periodically, and what really matters is the time when that happens relative to other clocks changing. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v4]
Bas Nieuwenhuizen writes: > Well the complication here is that in the MONOTONIC (not > MONOTONIC_RAW) case the CPU measurement can happen at the end of the > MONOTONIC_RAW interval (as the order of measurements is based on > argument order), so you can get a tick that started `period` (5 in > this case) monotonic ticks before the start of the interval and a CPU > measurement at the end of the interval. Ah, that's an excellent point. Let's split out raw and monotonic and take a look. You want the GPU sampled at the start of the raw interval and monotonic sampled at the end, I think? w x y z 0 1 2 3 4 5 6 7 8 9 a b c d e f Raw -_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_- 0 1 2 3 GPU -_-_-_-_ x y z 0 1 2 3 4 5 6 7 8 9 a b c Monotonic -_-_-_-_-_-_-_-_-_-_-_-_-_-_-_- Interval <-> Deviation <--> start = read(raw) 2 gpu = read(GPU) 1 mono = read(monotonic) 2 end = read(raw) b In this case, the error between the monotonic pulse and the GPU is interval + gpu_period (probably plus one to include the measurement error of the raw clock). Thanks for finding this case. Now, I guess the question is whether we want to try and find the smallest maxDeviation possible for each query. For instance, if the application asks only for raw and gpu, the max_deviation could be max2(interval+1,gpu_period), but if it asks for monotonic and gpu, it would be interval+1+gpu_period. I'm not seeing a simple definition here... -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v4]
Jason Ekstrand writes: > You've got me almost convinced as well. Thanks for the diagrams! I think > instead of adding 1 perhaps what we want is > > max2(sample_interval_ns, gpu_tick_ns + monotonic_tick_ns) > > Where monotonic_tick_ns is maybe as low as 1. Am I following you correctly? Not quite; I was thinking that because the sample_interval_ns is measured by sampling the monotonic clock twice, the actual interval can be up to 1 tick longer, so max2(sample_interval_ns + monotonic_tick_ns, gpu_tick_ns) The gpu_tick_ns is computed, not measured, and so accurately reflects the maximum deviation in the measurements. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v4]
Jason Ekstrand writes: > I think what Bas is getting at is that there are two problems: > > 1) We are not sampling at exactly the same time > 2) The two clocks may not tick at exactly the same time. Thanks for the clarification. > If we want to be conservative, I suspect Bas may be right that adding is > the safer thing to do. Yes, it's certainly safe to increase the value of maxDeviation. Currently, the time it takes to sample all of the clocks is far larger than the GPU tick, so adding that in would not have a huge impact on the value returned to the application. I'd like to dig in a little further and actually understand if the current computation (which is derived directly from the Vulkan spec) is wrong, and if so, whether the spec needs to be adjusted. I think the question is what 'maxDeviation' is supposed to represent. All the spec says is: * pMaxDeviation is a pointer to a 64-bit unsigned integer value in which the strictly positive maximum deviation, in nanoseconds, of the calibrated timestamp values is returned. I interpret this as the maximum error in sampling the individual clocks, which is to say that the clock values are guaranteed to have been sampled within this interval of each other. So, if we have a monotonic clock and GPU clock: 0 1 2 3 4 5 6 7 8 9 a b c d e f Monotonic -_-_-_-_-_-_-_-_-_-_-_-_-_-_-_- 0 1 2 3 GPU -_-_-_-_ gpu_period in this case is 5 ticks of the monotonic clock. Now, I perform three operations: start = read(monotonic) gpu = read(GPU) end = read(monotonic) Let's say that: start = 2 GPU = 1 * 5 = 5 monotonic equivalent ticks end = b So, the question is only how large the error between GPU and start could possibly be. Certainly the GPU clock was sampled some time between when monotonic tick 2 started and monotonic tick b ended. But, we have no idea what phase the GPU clock was in when sampled. Let's imagine we manage to sample the GPU clock immediately after the first monotonic sample. I'll shift the offset of the monotonic and GPU clocks to retain the same values (start = 2, GPU = 1), but now the GPU clock is being sampled immediately after monotonic time 2: w x y z 0 1 2 3 4 5 6 7 8 9 a b c d e f Monotonic -_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_-_- 0 1 2 3 GPU -_-_-_-_ In this case, the GPU tick started at monotonic time y, nearly 5 monotonic ticks earlier than the measured monotonic time, so the deviation between GPU and monotonic would be 5 ticks. If we sample the GPU clock immediately before the second monotonic sample, then that GPU tick either starts earlier than the range, in which case the above evaluation holds, or the GPU tick is entirely contained within the range: 0 1 2 3 4 5 6 7 8 9 a b c d e f Monotonic -_-_-_-_-_-_-_-_-_-_-_-_-_-_-_- z 0 1 2 3 GPU __-_-_-_-_- In this case, the deviation between the first monotonic sample (that returned to the application as the monotonic time) and the GPU sample is the whole interval of measurement (b - 2). I think I've just managed to convince myself that Jason's first suggestion (max2(sample interval, gpu interval)) is correct, although I think we should add '1' to the interval to account for sampling phase errors in the monotonic clock. As that's measured in ns, and I'm currently getting values in the µs range, that's a small error in comparison. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v4]
Bas Nieuwenhuizen writes: >> + end = radv_clock_gettime(CLOCK_MONOTONIC_RAW); >> + >> + uint64_t clock_period = end - begin; >> + uint64_t device_period = DIV_ROUND_UP(100, clock_crystal_freq); >> + >> + *pMaxDeviation = MAX2(clock_period, device_period); > > Should this not be a sum? Those deviations can happen independently > from each other, so worst case both deviations happen in the same > direction which causes the magnitude to be combined. This use of MAX2 comes right from one of the issues raised during work on the extension: 8) Can the maximum deviation reported ever be zero? RESOLVED: Unless the tick of each clock corresponding to the set of time domains coincides and all clocks can literally be sampled simutaneously, there isn’t really a possibility for the maximum deviation to be zero, so by convention the maximum deviation is always at least the maximum of the length of the ticks of the set of time domains calibrated and thus can never be zero. I can't wrap my brain around this entirely, but I think that this says that the deviation reported is supposed to only reflect the fact that we aren't sampling the clocks at the same time, and so there may be a 'tick' of error for any sampled clock. If you look at the previous issue in the spec, that actually has the pseudo code I used in this implementation for computing maxDeviation which doesn't include anything about the time period of the GPU. Jason suggested using the GPU period as the minimum value for maxDeviation at the GPU time period to make sure we never accidentally returned zero, as that is forbidden by the spec. We might be able to use 1 instead, but it won't matter in practice as the time it takes to actually sample all of the clocks is far longer than a GPU tick. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v4]
Offers three clocks, device, clock monotonic and clock monotonic
raw. Could use some kernel support to reduce the deviation between
clock values.
v2:
Ensure deviation is at least as big as the GPU time interval.
v3:
Set device->lost when returning DEVICE_LOST.
Use MAX2 and DIV_ROUND_UP instead of open coding these.
Delete spurious TIMESTAMP in radv version.
Suggested-by: Jason Ekstrand
Suggested-by: Lionel Landwerlin
v4:
Add anv_gem_reg_read to anv_gem_stubs.c
Suggested-by: Jason Ekstrand
Signed-off-by: Keith Packard
---
src/amd/vulkan/radv_device.c | 81 +++
src/amd/vulkan/radv_extensions.py | 1 +
src/intel/vulkan/anv_device.c | 89 ++
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_gem.c | 13 +
src/intel/vulkan/anv_gem_stubs.c | 7 +++
src/intel/vulkan/anv_private.h | 2 +
7 files changed, 194 insertions(+)
diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 174922780fc..80050485e54 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -4955,3 +4955,84 @@ radv_GetDeviceGroupPeerMemoryFeatures(
VK_PEER_MEMORY_FEATURE_GENERIC_SRC_BIT |
VK_PEER_MEMORY_FEATURE_GENERIC_DST_BIT;
}
+
+static const VkTimeDomainEXT radv_time_domains[] = {
+ VK_TIME_DOMAIN_DEVICE_EXT,
+ VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT,
+ VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT,
+};
+
+VkResult radv_GetPhysicalDeviceCalibrateableTimeDomainsEXT(
+ VkPhysicalDevice physicalDevice,
+ uint32_t *pTimeDomainCount,
+ VkTimeDomainEXT *pTimeDomains)
+{
+ int d;
+ VK_OUTARRAY_MAKE(out, pTimeDomains, pTimeDomainCount);
+
+ for (d = 0; d < ARRAY_SIZE(radv_time_domains); d++) {
+ vk_outarray_append(, i) {
+ *i = radv_time_domains[d];
+ }
+ }
+
+ return vk_outarray_status();
+}
+
+static uint64_t
+radv_clock_gettime(clockid_t clock_id)
+{
+ struct timespec current;
+ int ret;
+
+ ret = clock_gettime(clock_id, );
+ if (ret < 0 && clock_id == CLOCK_MONOTONIC_RAW)
+ ret = clock_gettime(CLOCK_MONOTONIC, );
+ if (ret < 0)
+ return 0;
+
+ return (uint64_t) current.tv_sec * 10ULL + current.tv_nsec;
+}
+
+VkResult radv_GetCalibratedTimestampsEXT(
+ VkDevice _device,
+ uint32_t timestampCount,
+ const VkCalibratedTimestampInfoEXT *pTimestampInfos,
+ uint64_t *pTimestamps,
+ uint64_t *pMaxDeviation)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ uint32_t clock_crystal_freq =
device->physical_device->rad_info.clock_crystal_freq;
+ int d;
+ uint64_t begin, end;
+
+ begin = radv_clock_gettime(CLOCK_MONOTONIC_RAW);
+
+ for (d = 0; d < timestampCount; d++) {
+ switch (pTimestampInfos[d].timeDomain) {
+ case VK_TIME_DOMAIN_DEVICE_EXT:
+ pTimestamps[d] = device->ws->query_value(device->ws,
+
RADEON_TIMESTAMP);
+ break;
+ case VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT:
+ pTimestamps[d] = radv_clock_gettime(CLOCK_MONOTONIC);
+ break;
+
+ case VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT:
+ pTimestamps[d] = begin;
+ break;
+ default:
+ pTimestamps[d] = 0;
+ break;
+ }
+ }
+
+ end = radv_clock_gettime(CLOCK_MONOTONIC_RAW);
+
+ uint64_t clock_period = end - begin;
+ uint64_t device_period = DIV_ROUND_UP(100, clock_crystal_freq);
+
+ *pMaxDeviation = MAX2(clock_period, device_period);
+
+ return VK_SUCCESS;
+}
diff --git a/src/amd/vulkan/radv_extensions.py
b/src/amd/vulkan/radv_extensions.py
index 5dcedae1c63..4c81d3f0068 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -92,6 +92,7 @@ EXTENSIONS = [
Extension('VK_KHR_display', 23,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_direct_mode_display', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_acquire_xlib_display', 1,
'VK_USE_PLATFORM_XLIB_XRANDR_EXT'),
+Extension('VK_EXT_calibrated_timestamps', 1, True),
Extension('VK_EXT_conditional_rendering', 1, True),
Extension('VK_EXT_conservative_rasterization',
Re: [Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v3]
Jason Ekstrand writes: > You need to add this to anv_gem_stubs.c as well or else the unit tests > won't build. Sorry for not catching it earlier. I'm always missing this > too. Well, that's a bit hard to test as -Dbuild-tests=true fails in a bunch of glx tests, but I think I've got it. > With that fixed, the anv bits are > > Reviewed-by: Jason Ekstrand Thanks. I haven't heard from any radv developers, so I can either split the patch apart or wait for another day or two. In any case, I'll post v4 of the patch here with the anv_gem_reg_read addition made. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v3]
Offers three clocks, device, clock monotonic and clock monotonic
raw. Could use some kernel support to reduce the deviation between
clock values.
v2:
Ensure deviation is at least as big as the GPU time interval.
v3:
Set device->lost when returning DEVICE_LOST.
Use MAX2 and DIV_ROUND_UP instead of open coding these.
Delete spurious TIMESTAMP in radv version.
Suggested-by: Jason Ekstrand
Suggested-by: Lionel Landwerlin
Signed-off-by: Keith Packard
---
src/amd/vulkan/radv_device.c | 81 +++
src/amd/vulkan/radv_extensions.py | 1 +
src/intel/vulkan/anv_device.c | 89 ++
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_gem.c | 13 +
src/intel/vulkan/anv_private.h | 2 +
6 files changed, 187 insertions(+)
diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 174922780fc..80050485e54 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -4955,3 +4955,84 @@ radv_GetDeviceGroupPeerMemoryFeatures(
VK_PEER_MEMORY_FEATURE_GENERIC_SRC_BIT |
VK_PEER_MEMORY_FEATURE_GENERIC_DST_BIT;
}
+
+static const VkTimeDomainEXT radv_time_domains[] = {
+ VK_TIME_DOMAIN_DEVICE_EXT,
+ VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT,
+ VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT,
+};
+
+VkResult radv_GetPhysicalDeviceCalibrateableTimeDomainsEXT(
+ VkPhysicalDevice physicalDevice,
+ uint32_t *pTimeDomainCount,
+ VkTimeDomainEXT *pTimeDomains)
+{
+ int d;
+ VK_OUTARRAY_MAKE(out, pTimeDomains, pTimeDomainCount);
+
+ for (d = 0; d < ARRAY_SIZE(radv_time_domains); d++) {
+ vk_outarray_append(, i) {
+ *i = radv_time_domains[d];
+ }
+ }
+
+ return vk_outarray_status();
+}
+
+static uint64_t
+radv_clock_gettime(clockid_t clock_id)
+{
+ struct timespec current;
+ int ret;
+
+ ret = clock_gettime(clock_id, );
+ if (ret < 0 && clock_id == CLOCK_MONOTONIC_RAW)
+ ret = clock_gettime(CLOCK_MONOTONIC, );
+ if (ret < 0)
+ return 0;
+
+ return (uint64_t) current.tv_sec * 10ULL + current.tv_nsec;
+}
+
+VkResult radv_GetCalibratedTimestampsEXT(
+ VkDevice _device,
+ uint32_t timestampCount,
+ const VkCalibratedTimestampInfoEXT *pTimestampInfos,
+ uint64_t *pTimestamps,
+ uint64_t *pMaxDeviation)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ uint32_t clock_crystal_freq =
device->physical_device->rad_info.clock_crystal_freq;
+ int d;
+ uint64_t begin, end;
+
+ begin = radv_clock_gettime(CLOCK_MONOTONIC_RAW);
+
+ for (d = 0; d < timestampCount; d++) {
+ switch (pTimestampInfos[d].timeDomain) {
+ case VK_TIME_DOMAIN_DEVICE_EXT:
+ pTimestamps[d] = device->ws->query_value(device->ws,
+
RADEON_TIMESTAMP);
+ break;
+ case VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT:
+ pTimestamps[d] = radv_clock_gettime(CLOCK_MONOTONIC);
+ break;
+
+ case VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT:
+ pTimestamps[d] = begin;
+ break;
+ default:
+ pTimestamps[d] = 0;
+ break;
+ }
+ }
+
+ end = radv_clock_gettime(CLOCK_MONOTONIC_RAW);
+
+ uint64_t clock_period = end - begin;
+ uint64_t device_period = DIV_ROUND_UP(100, clock_crystal_freq);
+
+ *pMaxDeviation = MAX2(clock_period, device_period);
+
+ return VK_SUCCESS;
+}
diff --git a/src/amd/vulkan/radv_extensions.py
b/src/amd/vulkan/radv_extensions.py
index 5dcedae1c63..4c81d3f0068 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -92,6 +92,7 @@ EXTENSIONS = [
Extension('VK_KHR_display', 23,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_direct_mode_display', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_acquire_xlib_display', 1,
'VK_USE_PLATFORM_XLIB_XRANDR_EXT'),
+Extension('VK_EXT_calibrated_timestamps', 1, True),
Extension('VK_EXT_conditional_rendering', 1, True),
Extension('VK_EXT_conservative_rasterization',1,
'device->rad_info.chip_class >= GFX9'),
Extension('VK_EXT_display_surface_counter', 1,
'VK_USE_PLATFORM_
Re: [Mesa-dev] [PATCH] Add tests for VK_EXT_calibrated_timestamps [v2]
Jason Ekstrand writes: > We're using MRs for crucible. Please create one and make sure you check > the "Allow commits from members who can merge to the target branch" so it > can be rebased through the UI by someone other than yourself. OOo. Shiny! -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] vulkan: Add VK_EXT_calibrated_timestamps extension (radv and anv) [v2]
Offers three clocks, device, clock monotonic and clock monotonic
raw. Could use some kernel support to reduce the deviation between
clock values.
v2:
Ensure deviation is at least as big as the GPU time interval.
Signed-off-by: Keith Packard
---
src/amd/vulkan/radv_device.c | 84
src/amd/vulkan/radv_extensions.py | 1 +
src/intel/vulkan/anv_device.c | 88 ++
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_gem.c | 13 +
src/intel/vulkan/anv_private.h | 2 +
6 files changed, 189 insertions(+)
diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 174922780fc..29f0afbc69b 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -4955,3 +4955,87 @@ radv_GetDeviceGroupPeerMemoryFeatures(
VK_PEER_MEMORY_FEATURE_GENERIC_SRC_BIT |
VK_PEER_MEMORY_FEATURE_GENERIC_DST_BIT;
}
+
+static const VkTimeDomainEXT radv_time_domains[] = {
+ VK_TIME_DOMAIN_DEVICE_EXT,
+ VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT,
+ VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT,
+};
+
+VkResult radv_GetPhysicalDeviceCalibrateableTimeDomainsEXT(
+ VkPhysicalDevice physicalDevice,
+ uint32_t *pTimeDomainCount,
+ VkTimeDomainEXT *pTimeDomains)
+{
+ int d;
+ VK_OUTARRAY_MAKE(out, pTimeDomains, pTimeDomainCount);
+
+ for (d = 0; d < ARRAY_SIZE(radv_time_domains); d++) {
+ vk_outarray_append(, i) {
+ *i = radv_time_domains[d];
+ }
+ }
+
+ return vk_outarray_status();
+}
+
+static uint64_t
+radv_clock_gettime(clockid_t clock_id)
+{
+ struct timespec current;
+ int ret;
+
+ ret = clock_gettime(clock_id, );
+ if (ret < 0 && clock_id == CLOCK_MONOTONIC_RAW)
+ ret = clock_gettime(CLOCK_MONOTONIC, );
+ if (ret < 0)
+ return 0;
+
+ return (uint64_t) current.tv_sec * 10ULL + current.tv_nsec;
+}
+
+#define TIMESTAMP 0x2358
+
+VkResult radv_GetCalibratedTimestampsEXT(
+ VkDevice _device,
+ uint32_t timestampCount,
+ const VkCalibratedTimestampInfoEXT *pTimestampInfos,
+ uint64_t *pTimestamps,
+ uint64_t *pMaxDeviation)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ uint32_t clock_crystal_freq =
device->physical_device->rad_info.clock_crystal_freq;
+ int d;
+ uint64_t begin, end;
+
+ begin = radv_clock_gettime(CLOCK_MONOTONIC_RAW);
+
+ for (d = 0; d < timestampCount; d++) {
+ switch (pTimestampInfos[d].timeDomain) {
+ case VK_TIME_DOMAIN_DEVICE_EXT:
+ /* XXX older kernels don't support this interface. */
+ pTimestamps[d] = device->ws->query_value(device->ws,
+
RADEON_TIMESTAMP);
+ break;
+ case VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT:
+ pTimestamps[d] = radv_clock_gettime(CLOCK_MONOTONIC);
+ break;
+
+ case VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT:
+ pTimestamps[d] = begin;
+ break;
+ default:
+ pTimestamps[d] = 0;
+ break;
+ }
+ }
+
+ end = radv_clock_gettime(CLOCK_MONOTONIC_RAW);
+
+ uint64_t clock_period = end - begin;
+ uint64_t device_period = (100 + clock_crystal_freq - 1) /
clock_crystal_freq;
+
+ *pMaxDeviation = clock_period > device_period ? clock_period :
device_period;
+
+ return VK_SUCCESS;
+}
diff --git a/src/amd/vulkan/radv_extensions.py
b/src/amd/vulkan/radv_extensions.py
index 5dcedae1c63..4c81d3f0068 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -92,6 +92,7 @@ EXTENSIONS = [
Extension('VK_KHR_display', 23,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_direct_mode_display', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_acquire_xlib_display', 1,
'VK_USE_PLATFORM_XLIB_XRANDR_EXT'),
+Extension('VK_EXT_calibrated_timestamps', 1, True),
Extension('VK_EXT_conditional_rendering', 1, True),
Extension('VK_EXT_conservative_rasterization',1,
'device->rad_info.chip_class >= GFX9'),
Extension('VK_EXT_display_surface_counter', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index a25
[Mesa-dev] [PATCH] Add tests for VK_EXT_calibrated_timestamps [v2]
Five tests:
1) Check for non-null function pointers
2) Check for in-range time domains
3) Check monotonic domains for correct values
4) Check correlation between monotonic and device domains
5) Check to make sure times in device domain match queue times
Signed-off-by: Keith Packard
---
Makefile.am| 1 +
src/tests/func/calibrated-timestamps.c | 442 +
2 files changed, 443 insertions(+)
create mode 100644 src/tests/func/calibrated-timestamps.c
diff --git a/Makefile.am b/Makefile.am
index 0ca35bd..ba98c60 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -113,6 +113,7 @@ bin_crucible_SOURCES = \
src/tests/stress/lots-of-surface-state.c \
src/tests/stress/buffer_limit.c \
src/tests/self/concurrent-output.c \
+ src/tests/func/calibrated-timestamps.c \
src/util/cru_cleanup.c \
src/util/cru_format.c \
src/util/cru_image.c \
diff --git a/src/tests/func/calibrated-timestamps.c
b/src/tests/func/calibrated-timestamps.c
new file mode 100644
index 000..a98150b
--- /dev/null
+++ b/src/tests/func/calibrated-timestamps.c
@@ -0,0 +1,442 @@
+/*
+ * Copyright © 2018 Keith Packard
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ */
+
+#include "tapi/t.h"
+#include
+#include
+#include
+
+#define GET_DEVICE_FUNCTION_PTR(name) \
+PFN_vk##name name = (PFN_vk##name)vkGetDeviceProcAddr(t_device, "vk"#name)
+
+#define GET_INSTANCE_FUNCTION_PTR(name) \
+PFN_vk##name name = (PFN_vk##name)vkGetInstanceProcAddr(t_instance,
"vk"#name)
+
+/* Test 1: Make sure the function pointers promised by the extension
+ * are valid
+ */
+static void
+test_funcs(void)
+{
+t_require_ext("VK_EXT_calibrated_timestamps");
+
+GET_INSTANCE_FUNCTION_PTR(GetPhysicalDeviceCalibrateableTimeDomainsEXT);
+GET_DEVICE_FUNCTION_PTR(GetCalibratedTimestampsEXT);
+
+t_assert(GetPhysicalDeviceCalibrateableTimeDomainsEXT != NULL);
+t_assert(GetCalibratedTimestampsEXT != NULL);
+}
+
+test_define {
+.name = "func.calibrated-timestamps.funcs",
+.start = test_funcs,
+.no_image = true,
+};
+
+/* Test 2: Make sure all of the domains offered by the driver are in range
+ */
+static void
+test_domains(void)
+{
+t_require_ext("VK_EXT_calibrated_timestamps");
+
+GET_INSTANCE_FUNCTION_PTR(GetPhysicalDeviceCalibrateableTimeDomainsEXT);
+GET_DEVICE_FUNCTION_PTR(GetCalibratedTimestampsEXT);
+
+t_assert(GetPhysicalDeviceCalibrateableTimeDomainsEXT != NULL);
+t_assert(GetCalibratedTimestampsEXT != NULL);
+
+VkResult result;
+
+uint32_t timeDomainCount;
+result = GetPhysicalDeviceCalibrateableTimeDomainsEXT(
+t_physical_dev,
+,
+NULL);
+t_assert(result == VK_SUCCESS);
+t_assert(timeDomainCount > 0);
+
+VkTimeDomainEXT *timeDomains = calloc(timeDomainCount, sizeof
(VkTimeDomainEXT));
+t_assert(timeDomains != NULL);
+
+result = GetPhysicalDeviceCalibrateableTimeDomainsEXT(
+t_physical_dev,
+,
+timeDomains);
+
+t_assert(result == VK_SUCCESS);
+
+/* Make sure all reported domains are valid */
+for (uint32_t d = 0; d < timeDomainCount; d++) {
+t_assert(VK_TIME_DOMAIN_BEGIN_RANGE_EXT <= timeDomains[d] &&
+ timeDomains[d] <= VK_TIME_DOMAIN_END_RANGE_EXT);
+}
+}
+
+test_define {
+.name = "func.calibrated-timestamps.domains",
+.start = test_domains,
+.no_image = true,
+};
+
+static uint64_t
+crucible_clock_gettime(VkTimeDomainEXT domain)
+{
+struct timespec current;
+int ret;
+clockid_t clock_id;
+
+switch (domain) {
+case VK_TIME_DOMAIN_CLOCK_MONOTONIC_EXT:
+clock_id = CLOCK_MONOTONIC;
+break;
+case VK_TIME_DOMAIN_CLOCK_MONOTONIC_RAW_EXT:
+clock_id = CLOCK_MONOTONIC_RAW;
+break;
+default:
+t_assert(0);
+return 0;
+}
+
+ret = clock_gettime(clock_id, );
+t_assert (ret >= 0);
+if (ret < 0)
+return 0;
+
+return (uint64_t) current.tv_sec * 10ULL + current.tv_nsec;
+}
+
+/* Test 3: Make sure any monotonic domains return accurate data
+ */
+static void
+test_monotonic(void)
+{
+t_require_ext("VK_EXT_calibrated_timestamps");
+
+GET_INSTANCE_FUNCTION_PTR(GetPhysicalDeviceCalibrateableTimeDomainsEXT);
+GET_DEVICE_FUNCTION_PTR(GetCalibratedTimestampsEXT);
+
+t_assert(GetPhysicalDeviceCalibrateableTimeDomai
Re: [Mesa-dev] [PATCH] radv: Allow physical device interfaces to be included in device extensions
Jason Ekstrand writes: > Actually, I think anv is doing the right thing too. Now I have no idea why > Keith was having problems. anv is happily returning a valid pointer and radv is not? In any case, I've switched to using vkGetInstanceProcAddr for the VkPhysicalDevice function and it works fine with both drivers. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] radv: Allow physical device interfaces to be included in device extensions
According to the Vulkan spec:
"Vulkan 1.0 initially required all new physical-device-level extension
functionality to be structured within an instance extension. In order
to avoid using an instance extension, which often requires loader
support, physical-device-level extension functionality may be
implemented within device extensions"
The code that checks for enabled extension APIs currently only passes
functions with VkDevice or VkCommandBuffer as their first
argument. This patch extends that to also allow functions with
VkPhysicalDevice parameters, in support of the above quote from the
Vulkan spec.
Signed-off-by: Keith Packard
---
src/amd/vulkan/radv_entrypoints_gen.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/amd/vulkan/radv_entrypoints_gen.py
b/src/amd/vulkan/radv_entrypoints_gen.py
index 377b544c2aa..69e6fc3e0eb 100644
--- a/src/amd/vulkan/radv_entrypoints_gen.py
+++ b/src/amd/vulkan/radv_entrypoints_gen.py
@@ -352,7 +352,7 @@ class Entrypoint(EntrypointBase):
self.return_type = return_type
self.params = params
self.guard = guard
-self.device_command = len(params) > 0 and (params[0].type ==
'VkDevice' or params[0].type == 'VkQueue' or params[0].type ==
'VkCommandBuffer')
+self.device_command = len(params) > 0 and (params[0].type ==
'VkPhysicalDevice' or params[0].type == 'VkDevice' or params[0].type ==
'VkQueue' or params[0].type == 'VkCommandBuffer')
def prefixed_name(self, prefix):
assert self.name.startswith('vk')
--
2.19.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 4/6] vulkan/wsi/display: add comment
Eric Engestrom writes: > - struct list_head connectors; > + struct list_head connectors; /* list of all discovered > connectors */ Yeah, definitely not the list of all connectors. Reviewed-by: Keith Packard -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 3/6] vulkan/wsi/display: pass the plane's modifiers to the image
Eric Engestrom writes: > Signed-off-by: Eric Engestrom (I'll have to let someone familiar with the formats and modifiers stuff review this one; I'm not comfortable with that at all). -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 2/6] vulkan/wsi/display: also select a plane when selecting a crtc
Eric Engestrom writes:
> + /* if there's a plane is active on the connector's crtc, pick it */
> + for (size_t i = 0; i < plane_res->count_planes; i++) {
> + drmModePlane *plane = drmModeGetPlane(wsi->fd, plane_res->planes[i]);
> + if (!plane)
> + continue;
I think you can do these three operations in a single walk of the
planes; it's obviously not performance critical, but I think squashing
them together would reduce the amount of duplicate code and make it
at least shorter to read.
Just find three plane ids -- one in use on the crtc, one compatible with
the crtc and one idle one, then select the one to use after the loop is over.
--
-keith
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 1/6] vulkan/wsi/display: setup the connector earlier
Eric Engestrom writes:
> Instead of setting it up when the swapchain is presented, set it up when
> creating the swapchain. This means that multiple swapchains might use
> the same crtc, but only one can be active at a time, and the connectors
> are now refcounted.
>
> This is necessary for the next commit.
>
> Signed-off-by: Eric Engestrom
> ---
> src/vulkan/wsi/wsi_common_display.c | 39 +++--
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/src/vulkan/wsi/wsi_common_display.c
> b/src/vulkan/wsi/wsi_common_display.c
> index 1dbed08d8a750ce21846..2d378afe3d36fe7cc177 100644
> --- a/src/vulkan/wsi/wsi_common_display.c
> +++ b/src/vulkan/wsi/wsi_common_display.c
> @@ -20,6 +20,7 @@
> * OF THIS SOFTWARE.
> */
>
> +#include "util/u_atomic.h"
> #include "util/macros.h"
> #include
> #include
> @@ -76,6 +77,7 @@ typedef struct wsi_display_connector {
> char *name;
> bool connected;
> bool active;
> + int refcount; /* swapchains using this connector
> */
Given that you're allocating the crtc at the same time you're setting
this value, could you stick the refcount in the crtc structure and avoid
a bunch of list walking?
> for (uint32_t i = 0; i < chain->base.image_count; i++)
>wsi_display_image_finish(drv_chain, allocator, >images[i]);
> +
> + wsi_display_mode *display_mode =
> + wsi_display_mode_from_handle(chain->surface->displayMode);
> + if (p_atomic_dec_zero(_mode->connector->refcount))
> + display_mode->connector->crtc_id = 0;
> +
I'd suggest just sticking some huge mutexes around the allocation and
free process to make sure we don't have any races. Either that or assert
that the application needs to deal with the problem. In either case,
atomics don't seem indicated here. I fear any careful ordering of
operations will be hard to review and fragile in the face of future
changes.
Thanks for cleaning this up; it's much nicer in this form than what I
did.
--
-keith
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] vulkan: Define new VK_MESA_query_timestamp extension [v2]
Pekka Paalanen writes: > I did not mean you would be solving that problem. I meant that it would > be good to figure out what people actually want from the API to be able > to solve the problem themselves. Thanks for the clarification. I'd suggest that we not try and solve that problem until we have someone who needs it? What I'm using this extension for is to correlate GPU times with WSI times as required by the GOOGLE_display_timing extension. That extension reports the time gap between the end of rendering and when a frame is displayed as reported by the WSI backend, so I needed some way to translate between those two time domains. To do this, I call this new function once per presentation, which means I'm getting recent values for both clocks which should track any minor clock skew. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] vulkan: Define new VK_MESA_query_timestamp extension [v2]
Pekka Paalanen writes: > On Sat, 23 Jun 2018 12:13:53 -0500 > Jason Ekstrand wrote: > >> I haven't thought through this comment all that hard but would it make >> sense to have three timestamps, CPU, GPU, CPU so that you have error bars >> on the GPU timestamp? At the very least, two timestamps would be better >> than one so that, when we pull it into the kernel, it can provide something >> more accurate than userspace trying to grab a snapshot. > > Hi, > > three timestamps sounds like a good idea to me, but you might want to > reach out to media developers (e.g. gstreamer) who have experience in > synchronizing different clocks and what that will actually take. Oh, I know that's really hard, and I don't want to solve that problem here. I explicitly *don't* solve it though -- I simply expose the ability to get correlated values in the two application-visible time domains that the Vulkan API already exposes (surface time and GPU time). How to synchronize the application as those two clocks drift around is outside the domain of this extension. > When reading the CPU timestamp, I believe it would be necessary for > userspace to tell which clock it should be reading. I'm not sure all > userspace is always using CLOCK_MONOTONIC. But if you have a better > rationale, that would be interesting to record and document. The extension doesn't state which clock is returned, you provide a device and a surface and get timestamps for both of them. This allows applications to translate between the two Vulkan time domains. As far as the implementation goes, it asks the device driver to get correlated GPU and CLOCK_MONOTONIC values and then asks the WSI layer to translate between CLOCK_MONOTONIC and surface time. Right now, all three surface types already operate in CLOCK_MONOTONIC natively, so there isn't any translation needed to surface time. The anv and radv driver implementations are simplistic and could easily be improved; they both simply fetch the GPU clock and then CLOCK_MONOTONIC sequentially without attempting to bound the error between them. The entire reason for this extension is to allow me to implement the GOOGLE_display_timing extension using only public interfaces into the drivers. That patch is blocked on getting this functionality landed. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH mesa 3/3] radv: Add new VK_MESA_query_timestamp extension to radv driver [v2]
This extension adds a single function to query the current GPU
timestamp, just like glGetInteger64v(GL_TIMESTAMP, ). This
function is needed to complete the implementation of
GOOGLE_display_timing, which needs to be able to correlate GPU and CPU
timestamps.
Signed-off-by: Keith Packard
v2: Update to track extension API that now returns both device and
surface timestamps.
---
src/amd/vulkan/radv_device.c | 21 +
src/amd/vulkan/radv_extensions.py | 1 +
src/amd/vulkan/radv_private.h | 1 +
3 files changed, 23 insertions(+)
diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 73c48cef1f0..d60753b6d69 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -4773,3 +4773,24 @@ radv_GetDeviceGroupPeerMemoryFeatures(
VK_PEER_MEMORY_FEATURE_GENERIC_SRC_BIT |
VK_PEER_MEMORY_FEATURE_GENERIC_DST_BIT;
}
+
+VkResult radv_QueryCurrentTimestampMESA(VkDevice _device, VkSurfaceKHR
_surface,
+ VkCurrentTimestampMESA *timestamp)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ VkResult result;
+
+ timestamp->deviceTimestamp = device->ws->query_value(device->ws,
+RADEON_TIMESTAMP);
+
+ result = wsi_common_convert_timestamp(
+ >physical_device->wsi_device,
+ _device,
+ _surface,
+ radv_get_current_time(),
+ >surfaceTimestamp);
+ if (result != VK_SUCCESS)
+ return result;
+
+ return VK_SUCCESS;
+}
diff --git a/src/amd/vulkan/radv_extensions.py
b/src/amd/vulkan/radv_extensions.py
index c36559f48ef..0018eb9040f 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -109,6 +109,7 @@ EXTENSIONS = [
Extension('VK_AMD_shader_core_properties',1, True),
Extension('VK_AMD_shader_info', 1, True),
Extension('VK_AMD_shader_trinary_minmax', 1, True),
+Extension('VK_MESA_query_timestamp', 1, True),
]
class VkVersion:
diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h
index 4e4b3a6037a..f37136450cc 100644
--- a/src/amd/vulkan/radv_private.h
+++ b/src/amd/vulkan/radv_private.h
@@ -76,6 +76,7 @@ typedef uint32_t xcb_window_t;
#include
#include
#include
+#include
#include "radv_entrypoints.h"
--
2.18.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/3] Add MESA_query_timestamp extension (v2)
Here's an updated version of the extension to get GPU timestamps. This version returns both CPU and GPU timestamps which could (in theory) be more tightly correlated than just getting the time of one and then the other sequentially. The idea is that we'd eventually add a suitable kernel ioctl to get both. The implementation is complicated by needing to allow WSI layers to have different timebases; the drivers now get CLOCK_MONOTONIC and GPU timestamps and then let the current WSI layer convert the CLOCK_MONOTONIC value into a suitable WSI-relative value. In the current code, all of the WSI layers are using CLOCK_MONOTONIC and so they don't actually use this functionality, but it's available in case it's needed. Neither radv nor anv bother to try and align the clocks closely; that would involve some fancy code to fetch the clocks multiple times, and I'd rather spend time just fixing the kernel rather than adding kludges in user space. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH mesa 1/3] vulkan: Define new VK_MESA_query_timestamp extension [v3]
This extension adds a single function to query the current GPU
timestamp, just like glGetInteger64v(GL_TIMESTAMP, ). This
function is needed to complete the implementation of
GOOGLE_display_timing, which needs to be able to correlate GPU and CPU
timestamps.
v2: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Add extension to list in alphabetical order
Suggested-by: Jason Ekstrand
v3: Report both device and surface timestamps
This will allow us to get timestamps more closely aligned by
getting both in a single call from the kernel.
To make this independent of the timebase used by the WSI
layer, provide a new wsi hook which converts CLOCK_MONOTONIC
into the matching WSI timebase. Right now, all of our backends
use CLOCK_MONOTONIC, so there's nothing actually doing
conversions, but it seemed best to put the infrastructure in
place so that I could validate the extension interface would
work if that became necessary.
Signed-off-by: Keith Packard
---
include/vulkan/vk_mesa_query_timestamp.h | 46
src/vulkan/registry/vk.xml | 20 +++
src/vulkan/wsi/wsi_common.c | 17 +
src/vulkan/wsi/wsi_common.h | 7
src/vulkan/wsi/wsi_common_private.h | 4 +++
5 files changed, 94 insertions(+)
create mode 100644 include/vulkan/vk_mesa_query_timestamp.h
diff --git a/include/vulkan/vk_mesa_query_timestamp.h
b/include/vulkan/vk_mesa_query_timestamp.h
new file mode 100644
index 000..262f094db27
--- /dev/null
+++ b/include/vulkan/vk_mesa_query_timestamp.h
@@ -0,0 +1,46 @@
+/*
+ * Copyright © 2018 Keith Packard
+ *
+ * Permission to use, copy, modify, distribute, and sell this software and its
+ * documentation for any purpose is hereby granted without fee, provided that
+ * the above copyright notice appear in all copies and that both that copyright
+ * notice and this permission notice appear in supporting documentation, and
+ * that the name of the copyright holders not be used in advertising or
+ * publicity pertaining to distribution of the software without specific,
+ * written prior permission. The copyright holders make no representations
+ * about the suitability of this software for any purpose. It is provided "as
+ * is" without express or implied warranty.
+ *
+ * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
+ * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
+ * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+ * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
+ * OF THIS SOFTWARE.
+ */
+
+#ifndef __VK_MESA_QUERY_TIMESTAMP_H__
+#define __VK_MESA_QUERY_TIMESTAMP_H__
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+typedef struct VkCurrentTimestampMESA {
+uint64_tdeviceTimestamp;
+uint64_tsurfaceTimestamp;
+} VkCurrentTimestampMESA;
+
+typedef VkResult (VKAPI_PTR *PFN_vkQueryCurrentTimestampMESA)(
+VkDevice device, VkSurfaceKHR surface, VkCurrentTimestampMESA *timestamp);
+
+VKAPI_ATTR VkResult VKAPI_CALL vkQueryCurrentTimestampMESA(
+VkDevice device, VkSurfaceKHR surface, VkCurrentTimestampMESA *timestamp);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __VK_MESA_QUERY_TIMESTAMP_H__ */
+
diff --git a/src/vulkan/registry/vk.xml b/src/vulkan/registry/vk.xml
index 4419c6fbf96..9c5a2f79398 100644
--- a/src/vulkan/registry/vk.xml
+++ b/src/vulkan/registry/vk.xml
@@ -3198,6 +3198,10 @@ server.
VkBool32
conditionalRendering
VkBool32
inheritedConditionalRendering
+
+ uint64_t
deviceTimestamp
+ uint64_t
surfaceTimestamp
+
Vulkan enumerant (token) definitions
@@ -6239,6 +6243,12 @@ server.
uint32_t maxDrawCount
uint32_t stride
+
+VkResult
vkQueryCurrentTimestampMESA
+VkDevice device
+ VkSurfaceKHR surface
+VkCurrentTimestampMESA*
pTimestamp
+
@@ -9008,5 +9018,15 @@ server.
+
+
+
+
+
+
+
diff --git a/src/vulkan/wsi/wsi_common.c b/src/vulkan/wsi/wsi_common.c
index f2d90a6bba2..9316470ad20 100644
--- a/src/vulkan/wsi/wsi_common.c
+++ b/src/vulkan/wsi/wsi_common.c
@@ -975,3 +975,20 @@ wsi_common_queue_present(const struct wsi_device *wsi,
return final_result;
}
+
+VkResult
+wsi_common_convert_timestamp(con
[Mesa-dev] [PATCH mesa 2/3] anv: Add new VK_MESA_query_timestamp extension to anv driver [v3]
This extension adds a single function to query the current GPU
timestamp, just like glGetInteger64v(GL_TIMESTAMP, ). This
function is needed to complete the implementation of
GOOGLE_display_timing, which needs to be able to correlate GPU and CPU
timestamps.
v2: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Add extension to list in alphabetical order
Suggested-by: Jason Ekstrand
v3: Update to track extension API that now returns both device and
surface timestamps.
Signed-off-by: Keith Packard
---
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_gem.c | 13 +
src/intel/vulkan/anv_private.h | 3 +++
src/intel/vulkan/genX_query.c | 30 ++
4 files changed, 47 insertions(+)
diff --git a/src/intel/vulkan/anv_extensions.py
b/src/intel/vulkan/anv_extensions.py
index adc1d758982..2dbbd70976c 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -124,6 +124,7 @@ EXTENSIONS = [
Extension('VK_EXT_shader_viewport_index_layer', 1, True),
Extension('VK_EXT_shader_stencil_export', 1, 'device->info.gen
>= 9'),
Extension('VK_EXT_vertex_attribute_divisor', 2, True),
+Extension('VK_MESA_query_timestamp', 1, True),
]
class VkVersion:
diff --git a/src/intel/vulkan/anv_gem.c b/src/intel/vulkan/anv_gem.c
index 3ba6d198a8a..8a31940e7aa 100644
--- a/src/intel/vulkan/anv_gem.c
+++ b/src/intel/vulkan/anv_gem.c
@@ -423,6 +423,19 @@ anv_gem_fd_to_handle(struct anv_device *device, int fd)
return args.handle;
}
+int
+anv_gem_reg_read(struct anv_device *device, uint32_t offset, uint64_t *result)
+{
+ struct drm_i915_reg_read args = {
+ .offset = offset
+ };
+
+ int ret = anv_ioctl(device->fd, DRM_IOCTL_I915_REG_READ, );
+
+ *result = args.val;
+ return ret;
+}
+
#ifndef SYNC_IOC_MAGIC
/* duplicated from linux/sync_file.h to avoid build-time dependency
* on new (v4.7) kernel headers. Once distro's are mostly using
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index cec28427923..fa250b9c8d8 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -76,6 +76,7 @@ struct gen_l3_config;
#include
#include
#include
+#include
#include "anv_entrypoints.h"
#include "anv_extensions.h"
@@ -1078,6 +1079,8 @@ int anv_gem_get_aperture(int fd, uint64_t *size);
int anv_gem_gpu_get_reset_stats(struct anv_device *device,
uint32_t *active, uint32_t *pending);
int anv_gem_handle_to_fd(struct anv_device *device, uint32_t gem_handle);
+int anv_gem_reg_read(struct anv_device *device,
+ uint32_t offset, uint64_t *result);
uint32_t anv_gem_fd_to_handle(struct anv_device *device, int fd);
int anv_gem_set_caching(struct anv_device *device, uint32_t gem_handle,
uint32_t caching);
int anv_gem_set_domain(struct anv_device *device, uint32_t gem_handle,
diff --git a/src/intel/vulkan/genX_query.c b/src/intel/vulkan/genX_query.c
index e35e9b85844..05e4c32f5f2 100644
--- a/src/intel/vulkan/genX_query.c
+++ b/src/intel/vulkan/genX_query.c
@@ -552,6 +552,36 @@ void genX(CmdWriteTimestamp)(
}
}
+VkResult genX(QueryCurrentTimestampMESA)(
+ VkDevice _device,
+ VkSurfaceKHR _surface,
+ VkCurrentTimestampMESA *timestamp)
+{
+ ANV_FROM_HANDLE(anv_device, device, _device);
+ struct wsi_device *wsi_device =
>instance->physicalDevice.wsi_device;
+ int ret;
+
+ /* XXX older kernels don't support this interface. */
+ ret = anv_gem_reg_read(device, TIMESTAMP | 1,
+ >deviceTimestamp);
+
+ if (ret != 0)
+ return VK_ERROR_DEVICE_LOST;
+
+ struct timespec current;
+ clock_gettime(CLOCK_MONOTONIC, );
+
+ uint64_t current_ns = (uint64_t) current.tv_sec * 10ULL +
+ current.tv_nsec;
+
+
+ return wsi_common_convert_timestamp(wsi_device,
+ _device,
+ _surface,
+ current_ns,
+ >surfaceTimestamp);
+}
+
#if GEN_GEN > 7 || GEN_IS_HASWELL
static uint32_t
--
2.18.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/3] vulkan: Define new VK_MESA_query_timestamp extension [v3]
Jason Ekstrand writes: > I haven't thought through this comment all that hard but would it make > sense to have three timestamps, CPU, GPU, CPU so that you have error bars > on the GPU timestamp? At the very least, two timestamps would be better > than one so that, when we pull it into the kernel, it can provide something > more accurate than userspace trying to grab a snapshot. I've updated the extension to provide both device and surface timestamps. The supplied implementation doesn't attempt to do anything fancy to synchronize them, but it could if we like. Of course, what we should do is provide a generic DRM ioctl which provides both of these values directly from the kernel in one shot so they can be more closely synchronized. -keith ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH mesa 1/3] vulkan: Define new VK_MESA_query_timestamp extension [v3]
This extension adds a single function to query the current GPU
timestamp, just like glGetInteger64v(GL_TIMESTAMP, ). This
function is needed to complete the implementation of
GOOGLE_display_timing, which needs to be able to correlate GPU and CPU
timestamps.
v2: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Add extension to list in alphabetical order
Suggested-by: Jason Ekstrand
v3: Report both device and surface timestamps
This will allow us to get timestamps more closely aligned by
getting both in a single call from the kernel.
To make this independent of the timebase used by the WSI
layer, provide a new wsi hook which converts CLOCK_MONOTONIC
into the matching WSI timebase. Right now, all of our backends
use CLOCK_MONOTONIC, so there's nothing actually doing
conversions, but it seemed best to put the infrastructure in
place so that I could validate the extension interface would
work if that became necessary.
Signed-off-by: Keith Packard
---
include/vulkan/vk_mesa_query_timestamp.h | 46
src/vulkan/registry/vk.xml | 20 +++
src/vulkan/wsi/wsi_common.c | 17 +
src/vulkan/wsi/wsi_common.h | 7
src/vulkan/wsi/wsi_common_private.h | 4 +++
5 files changed, 94 insertions(+)
create mode 100644 include/vulkan/vk_mesa_query_timestamp.h
diff --git a/include/vulkan/vk_mesa_query_timestamp.h
b/include/vulkan/vk_mesa_query_timestamp.h
new file mode 100644
index 000..262f094db27
--- /dev/null
+++ b/include/vulkan/vk_mesa_query_timestamp.h
@@ -0,0 +1,46 @@
+/*
+ * Copyright © 2018 Keith Packard
+ *
+ * Permission to use, copy, modify, distribute, and sell this software and its
+ * documentation for any purpose is hereby granted without fee, provided that
+ * the above copyright notice appear in all copies and that both that copyright
+ * notice and this permission notice appear in supporting documentation, and
+ * that the name of the copyright holders not be used in advertising or
+ * publicity pertaining to distribution of the software without specific,
+ * written prior permission. The copyright holders make no representations
+ * about the suitability of this software for any purpose. It is provided "as
+ * is" without express or implied warranty.
+ *
+ * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
+ * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
+ * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+ * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
+ * OF THIS SOFTWARE.
+ */
+
+#ifndef __VK_MESA_QUERY_TIMESTAMP_H__
+#define __VK_MESA_QUERY_TIMESTAMP_H__
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+typedef struct VkCurrentTimestampMESA {
+uint64_tdeviceTimestamp;
+uint64_tsurfaceTimestamp;
+} VkCurrentTimestampMESA;
+
+typedef VkResult (VKAPI_PTR *PFN_vkQueryCurrentTimestampMESA)(
+VkDevice device, VkSurfaceKHR surface, VkCurrentTimestampMESA *timestamp);
+
+VKAPI_ATTR VkResult VKAPI_CALL vkQueryCurrentTimestampMESA(
+VkDevice device, VkSurfaceKHR surface, VkCurrentTimestampMESA *timestamp);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __VK_MESA_QUERY_TIMESTAMP_H__ */
+
diff --git a/src/vulkan/registry/vk.xml b/src/vulkan/registry/vk.xml
index 7018bbe8421..e7a0c657724 100644
--- a/src/vulkan/registry/vk.xml
+++ b/src/vulkan/registry/vk.xml
@@ -3106,6 +3106,10 @@ server.
void*
pNext
uint64_t
externalFormat
+
+ uint64_t
deviceTimestamp
+ uint64_t
surfaceTimestamp
+
Vulkan enumerant (token) definitions
@@ -6102,6 +6106,12 @@ server.
uint32_t maxDrawCount
uint32_t stride
+
+VkResult
vkQueryCurrentTimestampMESA
+VkDevice device
+ VkSurfaceKHR surface
+VkCurrentTimestampMESA*
pTimestamp
+
@@ -8826,5 +8836,15 @@ server.
+
+
+
+
+
+
+
diff --git a/src/vulkan/wsi/wsi_common.c b/src/vulkan/wsi/wsi_common.c
index f2d90a6bba2..9316470ad20 100644
--- a/src/vulkan/wsi/wsi_common.c
+++ b/src/vulkan/wsi/wsi_common.c
@@ -975,3 +975,20 @@ wsi_common_queue_present(const struct wsi_device *wsi,
return final_result;
}
+
+VkResult
+wsi_common_convert_timestamp(const st
[Mesa-dev] [PATCH mesa 2/3] anv: Add new VK_MESA_query_timestamp extension to anv driver [v3]
This extension adds a single function to query the current GPU
timestamp, just like glGetInteger64v(GL_TIMESTAMP, ). This
function is needed to complete the implementation of
GOOGLE_display_timing, which needs to be able to correlate GPU and CPU
timestamps.
v2: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Add extension to list in alphabetical order
Suggested-by: Jason Ekstrand
v3: Update to track extension API that now returns both device and
surface timestamps.
Signed-off-by: Keith Packard
---
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_gem.c | 13 +
src/intel/vulkan/anv_private.h | 3 +++
src/intel/vulkan/genX_query.c | 30 ++
4 files changed, 47 insertions(+)
diff --git a/src/intel/vulkan/anv_extensions.py
b/src/intel/vulkan/anv_extensions.py
index 0f99f58ecb1..37bace23857 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -120,6 +120,7 @@ EXTENSIONS = [
'device->has_context_priority'),
Extension('VK_EXT_shader_viewport_index_layer', 1, True),
Extension('VK_EXT_shader_stencil_export', 1, 'device->info.gen
>= 9'),
+Extension('VK_MESA_query_timestamp', 1, True),
]
class VkVersion:
diff --git a/src/intel/vulkan/anv_gem.c b/src/intel/vulkan/anv_gem.c
index 3ba6d198a8a..8a31940e7aa 100644
--- a/src/intel/vulkan/anv_gem.c
+++ b/src/intel/vulkan/anv_gem.c
@@ -423,6 +423,19 @@ anv_gem_fd_to_handle(struct anv_device *device, int fd)
return args.handle;
}
+int
+anv_gem_reg_read(struct anv_device *device, uint32_t offset, uint64_t *result)
+{
+ struct drm_i915_reg_read args = {
+ .offset = offset
+ };
+
+ int ret = anv_ioctl(device->fd, DRM_IOCTL_I915_REG_READ, );
+
+ *result = args.val;
+ return ret;
+}
+
#ifndef SYNC_IOC_MAGIC
/* duplicated from linux/sync_file.h to avoid build-time dependency
* on new (v4.7) kernel headers. Once distro's are mostly using
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index 510471da602..0fa2a46e333 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -76,6 +76,7 @@ struct gen_l3_config;
#include
#include
#include
+#include
#include "anv_entrypoints.h"
#include "anv_extensions.h"
@@ -1055,6 +1056,8 @@ int anv_gem_get_aperture(int fd, uint64_t *size);
int anv_gem_gpu_get_reset_stats(struct anv_device *device,
uint32_t *active, uint32_t *pending);
int anv_gem_handle_to_fd(struct anv_device *device, uint32_t gem_handle);
+int anv_gem_reg_read(struct anv_device *device,
+ uint32_t offset, uint64_t *result);
uint32_t anv_gem_fd_to_handle(struct anv_device *device, int fd);
int anv_gem_set_caching(struct anv_device *device, uint32_t gem_handle,
uint32_t caching);
int anv_gem_set_domain(struct anv_device *device, uint32_t gem_handle,
diff --git a/src/intel/vulkan/genX_query.c b/src/intel/vulkan/genX_query.c
index e35e9b85844..05e4c32f5f2 100644
--- a/src/intel/vulkan/genX_query.c
+++ b/src/intel/vulkan/genX_query.c
@@ -552,6 +552,36 @@ void genX(CmdWriteTimestamp)(
}
}
+VkResult genX(QueryCurrentTimestampMESA)(
+ VkDevice _device,
+ VkSurfaceKHR _surface,
+ VkCurrentTimestampMESA *timestamp)
+{
+ ANV_FROM_HANDLE(anv_device, device, _device);
+ struct wsi_device *wsi_device =
>instance->physicalDevice.wsi_device;
+ int ret;
+
+ /* XXX older kernels don't support this interface. */
+ ret = anv_gem_reg_read(device, TIMESTAMP | 1,
+ >deviceTimestamp);
+
+ if (ret != 0)
+ return VK_ERROR_DEVICE_LOST;
+
+ struct timespec current;
+ clock_gettime(CLOCK_MONOTONIC, );
+
+ uint64_t current_ns = (uint64_t) current.tv_sec * 10ULL +
+ current.tv_nsec;
+
+
+ return wsi_common_convert_timestamp(wsi_device,
+ _device,
+ _surface,
+ current_ns,
+ >surfaceTimestamp);
+}
+
#if GEN_GEN > 7 || GEN_IS_HASWELL
static uint32_t
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH mesa 3/3] radv: Add new VK_MESA_query_timestamp extension to radv driver [v2]
This extension adds a single function to query the current GPU
timestamp, just like glGetInteger64v(GL_TIMESTAMP, ). This
function is needed to complete the implementation of
GOOGLE_display_timing, which needs to be able to correlate GPU and CPU
timestamps.
Signed-off-by: Keith Packard
v2: Update to track extension API that now returns both device and
surface timestamps.
---
src/amd/vulkan/radv_device.c | 21 +
src/amd/vulkan/radv_extensions.py | 1 +
src/amd/vulkan/radv_private.h | 1 +
3 files changed, 23 insertions(+)
diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index ad3465f594e..2ea208f7d6f 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -4772,3 +4772,24 @@ radv_GetDeviceGroupPeerMemoryFeatures(
VK_PEER_MEMORY_FEATURE_GENERIC_SRC_BIT |
VK_PEER_MEMORY_FEATURE_GENERIC_DST_BIT;
}
+
+VkResult radv_QueryCurrentTimestampMESA(VkDevice _device, VkSurfaceKHR
_surface,
+ VkCurrentTimestampMESA *timestamp)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+ VkResult result;
+
+ timestamp->deviceTimestamp = device->ws->query_value(device->ws,
+RADEON_TIMESTAMP);
+
+ result = wsi_common_convert_timestamp(
+ >physical_device->wsi_device,
+ _device,
+ _surface,
+ radv_get_current_time(),
+ >surfaceTimestamp);
+ if (result != VK_SUCCESS)
+ return result;
+
+ return VK_SUCCESS;
+}
diff --git a/src/amd/vulkan/radv_extensions.py
b/src/amd/vulkan/radv_extensions.py
index a0f1038110b..e5e6f25fca1 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -108,6 +108,7 @@ EXTENSIONS = [
Extension('VK_AMD_shader_core_properties',1, True),
Extension('VK_AMD_shader_info', 1, True),
Extension('VK_AMD_shader_trinary_minmax', 1, True),
+Extension('VK_MESA_query_timestamp', 1, True),
]
class VkVersion:
diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h
index df335b43d8d..fe895ac777c 100644
--- a/src/amd/vulkan/radv_private.h
+++ b/src/amd/vulkan/radv_private.h
@@ -75,6 +75,7 @@ typedef uint32_t xcb_window_t;
#include
#include
#include
+#include
#include "radv_entrypoints.h"
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] vulkan/wsi_common_display: Return SURFACE_LOST for fatal DRM errors
Jason Ekstrand writes: > Is this the same thing that happens on VT switch? If so, we may want to > return SURFACE_LOST for leases and OUT_OF_DATE for things running directly > on the display. No, on VT switch, my code currently sits in the driver and waits for the VT to return. The errors here are unexpected and the driver doesn't currently have any way to recover from them. This patch came from reviewing the valid error returns from these functions with my broader understanding of Vulkan and evaluating which error message most accurately described the situation. SURFACE_LOST is more accurate than OUT_OF_DATE as a DRM lease termination (the only "normal" cause of these errors) definitely requires the application create a new surface. If there were errors we knew how to handle, and the way to handle them was to re-query the surface and re-create the swap chain, then we should return OUT_OF_DATE. On another note, I re-read the spec for vkAcquireXlibDisplayEXT when looking at this and found that it explicitly calls out VT switch as a case where the driver is supposed to return 'an appropriate error', but I can't see an error which would be appropriate in this case -- DEVICE_LOST and SURFACE_LOST are both effectively fatal errors to the application while OUT_OF_DATE requires the application to re-query the surface and re-construct the swap chain. I guess we could have the surface query block until the VT returns? Is that better than just having the presentation block? -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH mesa] vulkan/wsi_common_display: Return SURFACE_LOST for fatal DRM errors
Instead of encouraging the client to re-create the swapchain and keep
going with an OUT_OF_DATE error, tell the client that further use of
the current surface will not succeed as the associated kernel objects
are no longer valid.
In particular, when a DRM lease is revoked, then the client needs to
get another lease and create a new surface for that.
Signed-off-by: Keith Packard
Cc: Jason Ekstrand
---
src/vulkan/wsi/wsi_common_display.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/vulkan/wsi/wsi_common_display.c
b/src/vulkan/wsi/wsi_common_display.c
index c36b87c18c3..4a2d88ff77e 100644
--- a/src/vulkan/wsi/wsi_common_display.c
+++ b/src/vulkan/wsi/wsi_common_display.c
@@ -1092,7 +1092,7 @@ wsi_display_acquire_next_image(struct wsi_swapchain
*drv_chain,
ret = wsi_display_wait_for_event(wsi, timeout);
if (ret && ret != ETIMEDOUT) {
- result = VK_ERROR_OUT_OF_DATE_KHR;
+ result = VK_ERROR_SURFACE_LOST_KHR;
goto done;
}
}
@@ -1200,7 +1200,7 @@ wsi_display_setup_connector(wsi_display_connector
*connector,
if (errno == ENOMEM)
result = VK_ERROR_OUT_OF_HOST_MEMORY;
else
- result = VK_ERROR_OUT_OF_DATE_KHR;
+ result = VK_ERROR_SURFACE_LOST_KHR;
goto bail;
}
@@ -1211,7 +1211,7 @@ wsi_display_setup_connector(wsi_display_connector
*connector,
if (errno == ENOMEM)
result = VK_ERROR_OUT_OF_HOST_MEMORY;
else
- result = VK_ERROR_OUT_OF_DATE_KHR;
+ result = VK_ERROR_SURFACE_LOST_KHR;
goto bail_mode_res;
}
@@ -1220,7 +1220,7 @@ wsi_display_setup_connector(wsi_display_connector
*connector,
connector->crtc_id = wsi_display_select_crtc(connector,
mode_res, drm_connector);
if (!connector->crtc_id) {
- result = VK_ERROR_OUT_OF_DATE_KHR;
+ result = VK_ERROR_SURFACE_LOST_KHR;
goto bail_connector;
}
}
@@ -1238,7 +1238,7 @@ wsi_display_setup_connector(wsi_display_connector
*connector,
}
if (!drm_mode) {
- result = VK_ERROR_OUT_OF_DATE_KHR;
+ result = VK_ERROR_SURFACE_LOST_KHR;
goto bail_connector;
}
@@ -1425,7 +1425,7 @@ _wsi_display_queue_next(struct wsi_swapchain *drv_chain)
wsi_display_connector *connector = display_mode->connector;
if (wsi->fd < 0)
- return VK_ERROR_OUT_OF_DATE_KHR;
+ return VK_ERROR_SURFACE_LOST_KHR;
if (display_mode != connector->current_mode)
connector->active = false;
@@ -1497,7 +1497,7 @@ _wsi_display_queue_next(struct wsi_swapchain *drv_chain)
if (ret != -EACCES) {
connector->active = false;
image->state = WSI_IMAGE_IDLE;
- return VK_ERROR_OUT_OF_DATE_KHR;
+ return VK_ERROR_SURFACE_LOST_KHR;
}
/* Some other VT is currently active. Sit here waiting for
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 2/4] vulkan: add VK_EXT_display_control [v8]
Danylo Piliaiev writes: > Thanks, then should this dependency be expressed in autoconf and > meson? Yup; looks like we missed a step. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 2/4] vulkan: add VK_EXT_display_control [v8]
Danylo Piliaiev writes: > Hello Keith, > > I am not able to build Mesa after this patch: > > wsi/wsi_common_display.c:991:4: error: unknown field ‘sequence_handler’ > specified in initializer > .sequence_handler = wsi_display_sequence_handler, Sounds like you need a newer libdrm that includes DRM_EVENT_CONTEXT_VERSION 4 -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/3] vulkan: Define new VK_MESA_query_timestamp extension [v2]
This extension adds a single function to query the current GPU
timestamp, just like glGetInteger64v(GL_TIMESTAMP, ). This
function is needed to complete the implementation of
GOOGLE_display_timing, which needs to be able to correlate GPU and CPU
timestamps.
v2: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Add extension to list in alphabetical order
Suggested-by: Jason Ekstrand
Signed-off-by: Keith Packard
---
include/vulkan/vk_mesa_query_timestamp.h | 41
src/vulkan/registry/vk.xml | 15 +
2 files changed, 56 insertions(+)
create mode 100644 include/vulkan/vk_mesa_query_timestamp.h
diff --git a/include/vulkan/vk_mesa_query_timestamp.h
b/include/vulkan/vk_mesa_query_timestamp.h
new file mode 100644
index 000..4e0b50cb9cc
--- /dev/null
+++ b/include/vulkan/vk_mesa_query_timestamp.h
@@ -0,0 +1,41 @@
+/*
+ * Copyright © 2018 Keith Packard
+ *
+ * Permission to use, copy, modify, distribute, and sell this software and its
+ * documentation for any purpose is hereby granted without fee, provided that
+ * the above copyright notice appear in all copies and that both that copyright
+ * notice and this permission notice appear in supporting documentation, and
+ * that the name of the copyright holders not be used in advertising or
+ * publicity pertaining to distribution of the software without specific,
+ * written prior permission. The copyright holders make no representations
+ * about the suitability of this software for any purpose. It is provided "as
+ * is" without express or implied warranty.
+ *
+ * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
+ * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
+ * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+ * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
+ * OF THIS SOFTWARE.
+ */
+
+#ifndef __VK_MESA_QUERY_TIMESTAMP_H__
+#define __VK_MESA_QUERY_TIMESTAMP_H__
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+typedef VkResult (VKAPI_PTR *PFN_vkQueryCurrentTimestampMESA)(
+VkDevice device, uint64_t *timestamp);
+
+VKAPI_ATTR VkResult VKAPI_CALL vkQueryCurrentTimestampMESA(
+VkDevice device, uint64_t *timestamp);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __VK_MESA_QUERY_TIMESTAMP_H__ */
+
diff --git a/src/vulkan/registry/vk.xml b/src/vulkan/registry/vk.xml
index 7018bbe8421..3a5cecdc8e3 100644
--- a/src/vulkan/registry/vk.xml
+++ b/src/vulkan/registry/vk.xml
@@ -6102,6 +6102,11 @@ server.
uint32_t maxDrawCount
uint32_t stride
+
+VkResult
vkQueryCurrentTimestampMESA
+VkDevice device
+uint64_t* pTimestamp
+
@@ -8826,5 +8831,15 @@ server.
+
+
+
+
+
+
+
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/3] anv: Add new VK_MESA_query_timestamp extension to anv driver [v2]
This extension adds a single function to query the current GPU
timestamp, just like glGetInteger64v(GL_TIMESTAMP, ). This
function is needed to complete the implementation of
GOOGLE_display_timing, which needs to be able to correlate GPU and CPU
timestamps.
v2: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Add extension to list in alphabetical order
Suggested-by: Jason Ekstrand
Signed-off-by: Keith Packard
---
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_gem.c | 13 +
src/intel/vulkan/anv_private.h | 3 +++
src/intel/vulkan/genX_query.c | 15 +++
4 files changed, 32 insertions(+)
diff --git a/src/intel/vulkan/anv_extensions.py
b/src/intel/vulkan/anv_extensions.py
index 0f99f58ecb1..37bace23857 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -120,6 +120,7 @@ EXTENSIONS = [
'device->has_context_priority'),
Extension('VK_EXT_shader_viewport_index_layer', 1, True),
Extension('VK_EXT_shader_stencil_export', 1, 'device->info.gen
>= 9'),
+Extension('VK_MESA_query_timestamp', 1, True),
]
class VkVersion:
diff --git a/src/intel/vulkan/anv_gem.c b/src/intel/vulkan/anv_gem.c
index 3ba6d198a8a..8a31940e7aa 100644
--- a/src/intel/vulkan/anv_gem.c
+++ b/src/intel/vulkan/anv_gem.c
@@ -423,6 +423,19 @@ anv_gem_fd_to_handle(struct anv_device *device, int fd)
return args.handle;
}
+int
+anv_gem_reg_read(struct anv_device *device, uint32_t offset, uint64_t *result)
+{
+ struct drm_i915_reg_read args = {
+ .offset = offset
+ };
+
+ int ret = anv_ioctl(device->fd, DRM_IOCTL_I915_REG_READ, );
+
+ *result = args.val;
+ return ret;
+}
+
#ifndef SYNC_IOC_MAGIC
/* duplicated from linux/sync_file.h to avoid build-time dependency
* on new (v4.7) kernel headers. Once distro's are mostly using
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index 510471da602..0fa2a46e333 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -76,6 +76,7 @@ struct gen_l3_config;
#include
#include
#include
+#include
#include "anv_entrypoints.h"
#include "anv_extensions.h"
@@ -1055,6 +1056,8 @@ int anv_gem_get_aperture(int fd, uint64_t *size);
int anv_gem_gpu_get_reset_stats(struct anv_device *device,
uint32_t *active, uint32_t *pending);
int anv_gem_handle_to_fd(struct anv_device *device, uint32_t gem_handle);
+int anv_gem_reg_read(struct anv_device *device,
+ uint32_t offset, uint64_t *result);
uint32_t anv_gem_fd_to_handle(struct anv_device *device, int fd);
int anv_gem_set_caching(struct anv_device *device, uint32_t gem_handle,
uint32_t caching);
int anv_gem_set_domain(struct anv_device *device, uint32_t gem_handle,
diff --git a/src/intel/vulkan/genX_query.c b/src/intel/vulkan/genX_query.c
index e35e9b85844..980c543460e 100644
--- a/src/intel/vulkan/genX_query.c
+++ b/src/intel/vulkan/genX_query.c
@@ -552,6 +552,21 @@ void genX(CmdWriteTimestamp)(
}
}
+VkResult genX(QueryCurrentTimestampMESA)(
+ VkDevice _device,
+ uint64_t *timestamp)
+{
+ ANV_FROM_HANDLE(anv_device, device, _device);
+ int ret;
+
+ /* XXX older kernels don't support this interface. */
+ ret = anv_gem_reg_read(device, TIMESTAMP | 1, timestamp);
+
+ if (ret != 0)
+ return VK_ERROR_DEVICE_LOST;
+ return VK_SUCCESS;
+}
+
#if GEN_GEN > 7 || GEN_IS_HASWELL
static uint32_t
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/3] radv: Add new VK_MESA_query_timestamp extension to radv driver
This extension adds a single function to query the current GPU
timestamp, just like glGetInteger64v(GL_TIMESTAMP, ). This
function is needed to complete the implementation of
GOOGLE_display_timing, which needs to be able to correlate GPU and CPU
timestamps.
Signed-off-by: Keith Packard
---
src/amd/vulkan/radv_device.c | 8
src/amd/vulkan/radv_extensions.py | 1 +
src/amd/vulkan/radv_private.h | 1 +
3 files changed, 10 insertions(+)
diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 62e1b9dba66..c552030491f 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -4770,3 +4770,11 @@ radv_GetDeviceGroupPeerMemoryFeatures(
VK_PEER_MEMORY_FEATURE_GENERIC_SRC_BIT |
VK_PEER_MEMORY_FEATURE_GENERIC_DST_BIT;
}
+
+VkResult radv_QueryCurrentTimestampMESA(VkDevice _device, uint64_t *timestamp)
+{
+ RADV_FROM_HANDLE(radv_device, device, _device);
+
+ *timestamp = device->ws->query_value(device->ws, RADEON_TIMESTAMP);
+ return VK_SUCCESS;
+}
diff --git a/src/amd/vulkan/radv_extensions.py
b/src/amd/vulkan/radv_extensions.py
index ebc3f6bc2b5..008e5cfe960 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -107,6 +107,7 @@ EXTENSIONS = [
Extension('VK_AMD_shader_core_properties',1, True),
Extension('VK_AMD_shader_info', 1, True),
Extension('VK_AMD_shader_trinary_minmax', 1, True),
+Extension('VK_MESA_query_timestamp', 1, True),
]
class VkVersion:
diff --git a/src/amd/vulkan/radv_private.h b/src/amd/vulkan/radv_private.h
index f001b836c8f..a3e0a6f013c 100644
--- a/src/amd/vulkan/radv_private.h
+++ b/src/amd/vulkan/radv_private.h
@@ -75,6 +75,7 @@ typedef uint32_t xcb_window_t;
#include
#include
#include
+#include
#include "radv_entrypoints.h"
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/3] Add (internal) MESA_query_timestamp extension to anv/radv
This extension fetches the current GPU timestamp from the hardware, just like the OpenGL API glGetInteger64v(GL_TIMESTAMP, ) function. I need this to correlate GPU and CPU timestamps for the GOOGLE_display_timing extension, but I think it will be useful for applications as well. I'm not sure this is exactly the API we need for this; it might be better for it to return *two* timestamps, a GPU and a CPU one which were as closely correlated as possible down in the kernel. The kernel APIs that this calls on anv and radv don't do that, so I didn't want to pretend to offer functionality I couldn't actually supply. Suggestions on what to do here are welcome! -keith ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 4/4] radv: add VK_EXT_display_control to radv driver [v5]
Bas Nieuwenhuizen writes: > Reviewed-by: Bas Nieuwenhuizen > > Thanks! Thanks to you as well; I've rebased and pushed to master. Two down, two to go (extensions that is) -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 2/4] vulkan: add VK_EXT_display_control [v8]
Jason Ekstrand writes: >> Looks good. With that, patches 1-3 are > > Reviewed-by: Jason Ekstrand Thanks. > I'll let Dave or Bas review your fence hackery in radv. Sounds fine. I'll prod them if I don't get any response in the next day or so. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 2/4] vulkan: add VK_EXT_display_control [v8]
Jason Ekstrand writes:
>> + if (!ret)
>> + return VK_SUCCESS;
>> +
>> + if (errno != ENOMEM) {
>
> This strikes me as a bit odd. What does ENOMEM mean if not "out of
> memory"?
ENOMEM means that the queue is full and that we should drain it and try
again; that's what the wait_for_event call is below.
The other-than-ENOMEM case is for some other failure, such as VT switch
or lease revoke. For RegisterDisplayEvent, there aren't any return
values other than VK_SUCCESS defined, and we're already assuming we can
use VK_OUT_OF_HOST_MEMORY for any function which allocates memory.
I think the correct value might be VK_ERROR_DEVICE_LOST or
VK_ERROR_OUT_OF_DATE_KHR as something "bad" has clearly happened? The
other place this is called is from QueuePresent, where either of those
error codes are allowed. I could convert that message to
VK_OUT_OF_HOST_MEMORY for RegisterDisplayEvent if you think that's a
good idea.
The sleep prevents an application from spinning at this failure,
allowing the user to gracefully terminate the application.
>
>> + wsi_display_debug("queue vblank event %lu failed\n",
>> fence->sequence);
>> + struct timespec delay = {
>> +.tv_sec = 0,
>> +.tv_nsec = 1ull,
>> + };
>> + nanosleep(, NULL);
>> + return VK_ERROR_OUT_OF_HOST_MEMORY;
>
> Given your previous explanation, I think this is ok but I think it deserves
> a comment.
Wilco.
I've added comments to this section to try and explain what's going on:
if (!ret)
return VK_SUCCESS;
if (errno != ENOMEM) {
/* Something unexpected happened. Pause for a moment so the
* application doesn't just spin and then return a failure indication
*/
wsi_display_debug("queue vblank event %lu failed\n", fence->sequence);
struct timespec delay = {
.tv_sec = 0,
.tv_nsec = 1ull,
};
nanosleep(, NULL);
return VK_ERROR_OUT_OF_HOST_MEMORY;
}
/* The kernel event queue is full. Wait for some events to be
* processed and try again
*/
pthread_mutex_lock(>wait_mutex);
ret = wsi_display_wait_for_event(wsi, wsi_rel_to_abs_time(1ull));
pthread_mutex_unlock(>wait_mutex);
if (ret) {
wsi_display_debug("vblank queue full, event wait failed\n");
return VK_ERROR_OUT_OF_HOST_MEMORY;
}
--
-keith
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 3/4] anv: add VK_EXT_display_control to anv driver [v4]
Jason Ekstrand writes: >> + fence = vk_alloc2(>instance->alloc, allocator, sizeof (*fence), >> 8, >> + VK_SYSTEM_ALLOCATION_SCOPE_OBJECT); > > zalloc? Yes, definitely. Thanks for catching this. Updated and pushed the series. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 2/4] vulkan: add VK_EXT_display_control [v8]
Jason Ekstrand writes:
>> + /* Look for a DPMS property if we haven't already found one */
>> + for (int p = 0; connector->dpms_property == 0 && p <
>> drm_connector->count_props; p++) {
>
> I'm guessing this is well over 80 characters
Sorry I missed this -- I haven't managed to configure my editor to
highlight long lines.
>> + if (counters)
>> + result = wsi_display_surface_get_surface_counters(
>> + icd_surface,
>> + >supported_surface_counters);
>
> Please put braces around multi-line if statements.
Ok.
--
-keith
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH mesa 3/4] anv: add VK_EXT_display_control to anv driver [v4]
This extension provides fences and frame count information to direct
display contexts. It uses new kernel ioctls to provide 64-bits of
vblank sequence and nanosecond resolution.
v2: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Add extension to list in alphabetical order
Suggested-by: Jason Ekstrand
v3: Adapt to WSI fence API change. It now returns VkResult and
no longer has an option for relative timeouts.
v4: wsi_register_display_event and wsi_register_device_event now
use the default allocator when NULL is provided, so remove the
computation of 'alloc' here.
Signed-off-by: Keith Packard
---
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_private.h | 4 ++
src/intel/vulkan/anv_queue.c | 18 +++
src/intel/vulkan/anv_wsi_display.c | 85 ++
4 files changed, 108 insertions(+)
diff --git a/src/intel/vulkan/anv_extensions.py
b/src/intel/vulkan/anv_extensions.py
index ecef1b254bf..0f99f58ecb1 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -113,6 +113,7 @@ EXTENSIONS = [
Extension('VK_EXT_acquire_xlib_display', 1,
'VK_USE_PLATFORM_XLIB_XRANDR_EXT'),
Extension('VK_EXT_debug_report', 8, True),
Extension('VK_EXT_direct_mode_display', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
+Extension('VK_EXT_display_control', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_display_surface_counter', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_external_memory_dma_buf', 1, True),
Extension('VK_EXT_global_priority', 1,
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index edc5317bac4..510471da602 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -2134,6 +2134,7 @@ enum anv_fence_type {
ANV_FENCE_TYPE_NONE = 0,
ANV_FENCE_TYPE_BO,
ANV_FENCE_TYPE_SYNCOBJ,
+ ANV_FENCE_TYPE_WSI,
};
enum anv_bo_fence_state {
@@ -2168,6 +2169,9 @@ struct anv_fence_impl {
/** DRM syncobj handle for syncobj-based fences */
uint32_t syncobj;
+
+ /** WSI fence */
+ struct wsi_fence *fence_wsi;
};
};
diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c
index 6e275629e14..e0c0a42069f 100644
--- a/src/intel/vulkan/anv_queue.c
+++ b/src/intel/vulkan/anv_queue.c
@@ -324,6 +324,10 @@ anv_fence_impl_cleanup(struct anv_device *device,
anv_gem_syncobj_destroy(device, impl->syncobj);
break;
+ case ANV_FENCE_TYPE_WSI:
+ impl->fence_wsi->destroy(impl->fence_wsi);
+ break;
+
default:
unreachable("Invalid fence type");
}
@@ -673,6 +677,17 @@ done:
return result;
}
+static VkResult
+anv_wait_for_wsi_fence(struct anv_device *device,
+ const VkFence _fence,
+ uint64_t abs_timeout)
+{
+ ANV_FROM_HANDLE(anv_fence, fence, _fence);
+ struct anv_fence_impl *impl = >permanent;
+
+ return impl->fence_wsi->wait(impl->fence_wsi, abs_timeout);
+}
+
static VkResult
anv_wait_for_fences(struct anv_device *device,
uint32_t fenceCount,
@@ -695,6 +710,9 @@ anv_wait_for_fences(struct anv_device *device,
result = anv_wait_for_syncobj_fences(device, 1, [i],
true, abs_timeout);
break;
+ case ANV_FENCE_TYPE_WSI:
+result = anv_wait_for_wsi_fence(device, pFences[i], abs_timeout);
+break;
case ANV_FENCE_TYPE_NONE:
result = VK_SUCCESS;
break;
diff --git a/src/intel/vulkan/anv_wsi_display.c
b/src/intel/vulkan/anv_wsi_display.c
index ed679e85e13..6e1cb43aa35 100644
--- a/src/intel/vulkan/anv_wsi_display.c
+++ b/src/intel/vulkan/anv_wsi_display.c
@@ -174,3 +174,88 @@ anv_GetRandROutputDisplayEXT(VkPhysicalDevice
physical_device,
display);
}
#endif /* VK_USE_PLATFORM_XLIB_XRANDR_EXT */
+
+/* VK_EXT_display_control */
+
+VkResult
+anv_DisplayPowerControlEXT(VkDevice_device,
+VkDisplayKHRdisplay,
+const VkDisplayPowerInfoEXT *display_power_info)
+{
+ ANV_FROM_HANDLE(anv_device, device, _device);
+
+ return wsi_display_power_control(
+ _device, >instance->physicalDevice.wsi_device,
+ display, display_power_info);
+}
+
+VkResult
+anv_RegisterDeviceEventEXT(VkDevice _device,
+const VkDeviceEventInfoEXT *device_event_info,
+const VkAllocationCallbacks *allocator,
+VkFence *_fence)
+{
+ ANV_FROM_HANDLE(anv_device
[Mesa-dev] [PATCH mesa 2/4] vulkan: add VK_EXT_display_control [v8]
This extension provides fences and frame count information to direct
display contexts. It uses new kernel ioctls to provide 64-bits of
vblank sequence and nanosecond resolution.
v2: Remove DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT flag. This has
been removed from the proposed kernel API.
Add NULL parameter to drmCrtcQueueSequence ioctl as we
don't care what sequence the event was actually queued to.
v3: Adapt to pthread clock switch to MONOTONIC
v4: Fix scope for wsi_display_mode andwsi_display_connector allocs
Suggested-by: Jason Ekstrand
v5: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Use wsi_rel_to_abs_time helper function to convert relative
timeouts to absolute timeouts without causing overflow.
Suggested-by: Jason Ekstrand
v6:
Change WSI fence wait function to return VkResult instead of
bool. This makes the meaning of the return value easier to
understand, and allows for the indication of failure.
Also change the WSI fence wait function to take only absolute
timeouts and not provide an option for a relative timeout. No
users wanted relative timeouts, and it's simpler if that option
isn't available.
Terminate the DPMS property loop once we've found the property.
Assert that the fence hasn't already been destroyed in
wsi_display_fence_destroy.
Rearrange the event handler function order in the file to place
routines in an easier to find order.
Suggested-by: Jason Ekstrand
v7:
Adapt to API changes for surface_get_capabilities
v8:
Use wsi->alloc in register_display_event so that callers
don't have to dig out an allocator for us.
Signed-off-by: Keith Packard
---
src/vulkan/wsi/wsi_common.h | 10 +
src/vulkan/wsi/wsi_common_display.c | 329 +++-
src/vulkan/wsi/wsi_common_display.h | 29 +++
3 files changed, 366 insertions(+), 2 deletions(-)
diff --git a/src/vulkan/wsi/wsi_common.h b/src/vulkan/wsi/wsi_common.h
index 07d5e8353b0..33e4f849ac9 100644
--- a/src/vulkan/wsi/wsi_common.h
+++ b/src/vulkan/wsi/wsi_common.h
@@ -73,6 +73,16 @@ struct wsi_surface_supported_counters {
const void *pNext;
VkSurfaceCounterFlagsEXT supported_surface_counters;
+
+};
+
+struct wsi_fence {
+ VkDevice device;
+ const struct wsi_device *wsi_device;
+ VkDisplayKHR display;
+ const VkAllocationCallbacks *alloc;
+ VkResult (*wait)(struct wsi_fence *fence, uint64_t
abs_timeout);
+ void (*destroy)(struct wsi_fence *fence);
};
struct wsi_interface;
diff --git a/src/vulkan/wsi/wsi_common_display.c
b/src/vulkan/wsi/wsi_common_display.c
index 01150ffbb1b..c786d8befe5 100644
--- a/src/vulkan/wsi/wsi_common_display.c
+++ b/src/vulkan/wsi/wsi_common_display.c
@@ -79,6 +79,7 @@ typedef struct wsi_display_connector {
struct list_head display_modes;
wsi_display_mode *current_mode;
drmModeModeInfo current_drm_mode;
+ uint32_t dpms_property;
#ifdef VK_USE_PLATFORM_XLIB_XRANDR_EXT
xcb_randr_output_t output;
#endif
@@ -132,6 +133,15 @@ struct wsi_display_swapchain {
struct wsi_display_image images[0];
};
+struct wsi_display_fence {
+ struct wsi_fence base;
+ bool event_received;
+ bool destroyed;
+ uint64_t sequence;
+};
+
+static uint64_t fence_sequence;
+
ICD_DEFINE_NONDISP_HANDLE_CASTS(wsi_display_mode, VkDisplayModeKHR)
ICD_DEFINE_NONDISP_HANDLE_CASTS(wsi_display_connector, VkDisplayKHR)
@@ -307,6 +317,19 @@ wsi_display_get_connector(struct wsi_device *wsi_device,
connector->connected = drm_connector->connection != DRM_MODE_DISCONNECTED;
+ /* Look for a DPMS property if we haven't already found one */
+ for (int p = 0; connector->dpms_property == 0 && p <
drm_connector->count_props; p++) {
+ drmModePropertyPtr prop = drmModeGetProperty(wsi->fd,
+ drm_connector->props[p]);
+ if (!prop)
+ continue;
+ if (prop->flags & DRM_MODE_PROP_ENUM) {
+ if (!strcmp(prop->name, "DPMS"))
+connector->dpms_property = drm_connector->props[p];
+ }
+ drmModeFreeProperty(prop);
+ }
+
/* Mark all connector modes as invalid */
wsi_display_invalidate_connector_modes(wsi_device, connector);
@@ -673,15 +696,37 @@ wsi_display_surface_get_capabilities(VkIcdSurfaceBase
*surface_base,
return VK_SUCCESS;
}
+static VkResult
+wsi_display_surface_get_surface_counters(
+ VkIcdSurfaceBase *surface_base,
+ VkSurfaceCounterFlagsEXT *counters)
+{
+ *counters = VK_SURFACE_COUNTER_VBLANK_EXT;
+ return VK_SUCCESS;
+}
+
static VkResu
[Mesa-dev] [PATCH mesa 4/4] radv: add VK_EXT_display_control to radv driver [v5]
This extension provides fences and frame count information to direct
display contexts. It uses new kernel ioctls to provide 64-bits of
vblank sequence and nanosecond resolution.
v2:
Rework fence integration into the driver so that waiting for
any of a mixture of fence types (wsi, driver or syncobjs)
causes the driver to poll, while a list of just syncobjs or
just driver fences will block. When we get syncobjs for wsi
fences, we'll adapt to use them.
v3: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Suggested-by: Jason Ekstrand
v4: Adapt to WSI fence API change. It now returns VkResult and
no longer has an option for relative timeouts.
v5: wsi_register_display_event and wsi_register_device_event now
use the default allocator when NULL is provided, so remove the
computation of 'alloc' here.
Signed-off-by: Keith Packard
---
src/amd/vulkan/radv_device.c | 70 -
src/amd/vulkan/radv_extensions.py | 1 +
src/amd/vulkan/radv_private.h | 1 +
src/amd/vulkan/radv_wsi_display.c | 101 ++
4 files changed, 158 insertions(+), 15 deletions(-)
diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index ffeb6450b33..b560f1c3085 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -3241,6 +3241,7 @@ VkResult radv_CreateFence(
if (!fence)
return vk_error(device->instance, VK_ERROR_OUT_OF_HOST_MEMORY);
+ fence->fence_wsi = NULL;
fence->submitted = false;
fence->signalled = !!(pCreateInfo->flags &
VK_FENCE_CREATE_SIGNALED_BIT);
fence->temp_syncobj = 0;
@@ -3285,6 +3286,8 @@ void radv_DestroyFence(
device->ws->destroy_syncobj(device->ws, fence->syncobj);
if (fence->fence)
device->ws->destroy_fence(fence->fence);
+ if (fence->fence_wsi)
+ fence->fence_wsi->destroy(fence->fence_wsi);
vk_free2(>alloc, pAllocator, fence);
}
@@ -3310,7 +3313,19 @@ static bool radv_all_fences_plain_and_submitted(uint32_t
fenceCount, const VkFen
{
for (uint32_t i = 0; i < fenceCount; ++i) {
RADV_FROM_HANDLE(radv_fence, fence, pFences[i]);
- if (fence->syncobj || fence->temp_syncobj || (!fence->signalled
&& !fence->submitted))
+ if (fence->fence == NULL || fence->syncobj ||
+ fence->temp_syncobj ||
+ (!fence->signalled && !fence->submitted))
+ return false;
+ }
+ return true;
+}
+
+static bool radv_all_fences_syncobj(uint32_t fenceCount, const VkFence
*pFences)
+{
+ for (uint32_t i = 0; i < fenceCount; ++i) {
+ RADV_FROM_HANDLE(radv_fence, fence, pFences[i]);
+ if (fence->syncobj == 0 && fence->temp_syncobj == 0)
return false;
}
return true;
@@ -3326,7 +3341,9 @@ VkResult radv_WaitForFences(
RADV_FROM_HANDLE(radv_device, device, _device);
timeout = radv_get_absolute_timeout(timeout);
- if (device->always_use_syncobj) {
+ if (device->always_use_syncobj &&
+ radv_all_fences_syncobj(fenceCount, pFences))
+ {
uint32_t *handles = malloc(sizeof(uint32_t) * fenceCount);
if (!handles)
return vk_error(device->instance,
VK_ERROR_OUT_OF_HOST_MEMORY);
@@ -3396,21 +3413,34 @@ VkResult radv_WaitForFences(
if (fence->signalled)
continue;
- if (!fence->submitted) {
- while(radv_get_current_time() <= timeout &&
!fence->submitted)
- /* Do nothing */;
+ if (fence->fence) {
+ if (!fence->submitted) {
+ while(radv_get_current_time() <= timeout &&
+ !fence->submitted)
+ /* Do nothing */;
- if (!fence->submitted)
- return VK_TIMEOUT;
+ if (!fence->submitted)
+ return VK_TIMEOUT;
+
+ /* Recheck as it may have been set by
+* submitting operations. */
- /* Recheck as it may have been set by submitting
operations. */
- if (fence->signalled)
- continue;
+ if (fence->signalled)
+
[Mesa-dev] [PATCH mesa 1/4] anv: Support wait for heterogeneous list of fences [v3]
Handle the case where the set of fences to wait for is not all of the
same type by either waiting for them sequentially (waitAll), or
polling them until the timer has expired (!waitAll). We hope the
latter case is not common.
While the current code makes sure that it always has fences of only
one type, that will not be true when we add WSI fences. Split out this
refactoring to make merging that clearer.
v2: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Suggested-by: Jason Ekstrand
v2:
Cast INT64_MAX to uint64_t to make of its use as the maximum
possible timeout clearly unsigned to the reader.
Suggested-by: Jason Ekstrand
Make anv_wait_for_fences with !waitAll check all fences at least
once, even if the requested timeout has already passed.
Signed-off-by: Keith Packard
Reviewed-by: Jason Ekstrand
---
src/intel/vulkan/anv_queue.c | 108 +--
1 file changed, 90 insertions(+), 18 deletions(-)
diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c
index 6fe04a0a19d..6e275629e14 100644
--- a/src/intel/vulkan/anv_queue.c
+++ b/src/intel/vulkan/anv_queue.c
@@ -459,12 +459,33 @@ gettime_ns(void)
return (uint64_t)current.tv_sec * NSEC_PER_SEC + current.tv_nsec;
}
+static uint64_t anv_get_absolute_timeout(uint64_t timeout)
+{
+ if (timeout == 0)
+ return 0;
+ uint64_t current_time = gettime_ns();
+ uint64_t max_timeout = (uint64_t) INT64_MAX - current_time;
+
+ timeout = MIN2(max_timeout, timeout);
+
+ return (current_time + timeout);
+}
+
+static int64_t anv_get_relative_timeout(uint64_t abs_timeout)
+{
+ uint64_t now = gettime_ns();
+
+ if (abs_timeout < now)
+ return 0;
+ return abs_timeout - now;
+}
+
static VkResult
anv_wait_for_syncobj_fences(struct anv_device *device,
uint32_t fenceCount,
const VkFence *pFences,
bool waitAll,
-uint64_t _timeout)
+uint64_t abs_timeout_ns)
{
uint32_t *syncobjs = vk_zalloc(>alloc,
sizeof(*syncobjs) * fenceCount, 8,
@@ -484,19 +505,6 @@ anv_wait_for_syncobj_fences(struct anv_device *device,
syncobjs[i] = impl->syncobj;
}
- int64_t abs_timeout_ns = 0;
- if (_timeout > 0) {
- uint64_t current_ns = gettime_ns();
-
- /* Add but saturate to INT32_MAX */
- if (current_ns + _timeout < current_ns)
- abs_timeout_ns = INT64_MAX;
- else if (current_ns + _timeout > INT64_MAX)
- abs_timeout_ns = INT64_MAX;
- else
- abs_timeout_ns = current_ns + _timeout;
- }
-
/* The gem_syncobj_wait ioctl may return early due to an inherent
* limitation in the way it computes timeouts. Loop until we've actually
* passed the timeout.
@@ -539,7 +547,7 @@ anv_wait_for_bo_fences(struct anv_device *device,
* best we can do is to clamp the timeout to INT64_MAX. This limits the
* maximum timeout from 584 years to 292 years - likely not a big deal.
*/
- int64_t timeout = MIN2(_timeout, INT64_MAX);
+ int64_t timeout = MIN2(_timeout, (uint64_t) INT64_MAX);
VkResult result = VK_SUCCESS;
uint32_t pending_fences = fenceCount;
@@ -665,6 +673,67 @@ done:
return result;
}
+static VkResult
+anv_wait_for_fences(struct anv_device *device,
+uint32_t fenceCount,
+const VkFence *pFences,
+bool waitAll,
+uint64_t abs_timeout)
+{
+ VkResult result = VK_SUCCESS;
+
+ if (fenceCount <= 1 || waitAll) {
+ for (uint32_t i = 0; i < fenceCount; i++) {
+ ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
+ switch (fence->permanent.type) {
+ case ANV_FENCE_TYPE_BO:
+result = anv_wait_for_bo_fences(
+ device, 1, [i], true,
+ anv_get_relative_timeout(abs_timeout));
+break;
+ case ANV_FENCE_TYPE_SYNCOBJ:
+result = anv_wait_for_syncobj_fences(device, 1, [i],
+ true, abs_timeout);
+break;
+ case ANV_FENCE_TYPE_NONE:
+result = VK_SUCCESS;
+break;
+ }
+ if (result != VK_SUCCESS)
+return result;
+ }
+ } else {
+ do {
+ for (uint32_t i = 0; i < fenceCount; i++) {
+if (anv_wait_for_fences(device, 1, [i], true, 0) ==
VK_SUCCESS)
+ return VK_SUCCESS;
+ }
+ } while (gettime_ns() < abs_timeout);
+ result = VK_TIMEOUT;
+ }
+ return result;
+}
+
+static bool anv_all_fences_syncobj(uint32_t fenceCount, const VkFence *pFences)
+{
+ for (uint32_t i = 0; i < fenceCount; ++i) {
+ ANV_FROM_HANDLE(anv_fence, fence, pFences[i
[Mesa-dev] [PATCH mesa 0/4] Add EXT_display_control [v8]
Here's the latest version of this series with only a few minor changes. It adapts to the API changes for surface_get_capabilities and changes how the allocator is found for fences. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 5/7] vulkan: add VK_EXT_display_control [v5]
Jason Ekstrand writes: > That seems good to me. Unless, of course, DPMS is something we expect to > change over time somehow. Then again, we don't handle that at all right > now so meh. Let's go with what you wrote above for now. It's not even the dpms value, it's the dpms property itself, which DRM never changes. > They shouldn't be and that's why I'm a fan of making them asserts which get > compiled out instead of actual checks. Also, I find this pseudo reference > counting to be somewhat confusing and adding asserts informs the reader of > the assumptions made. Ok, I've added this. > What test suite? Honestly, I know of no code anywhere that actually uses > this API for anything other than VR headsets. > > I guess it's kind-of a question of how much effort we want to put into > this. One option would be to add VK_KHR_display support to vkcube and make > it automatically show up on all your displays using hotplug events. > > If we're going to not care, returning VK_ERROR_FEATURE_NOT_PRESENT is > probably the best thing to do since at least the app has feedback. Not caring seems best to me -- the Vulkan display API isn't capable of supporting a "real" window system; for that, you'd really want to use DRM directly and create some way to share that with Vulkan like the extension I wrote to pass the DRM master FD into the driver at init time. > Awesome. I think we're really close on this one. I'll send out the current series and you can see if you like it. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] anv: add VK_EXT_display_control to anv driver [v2]
Jason Ekstrand writes: > I believe that the WSI common code should be capable of fishing the > instance allocator out of the wsi_display so we need only pass the > allocator argument unmodified through to the core WSI code. Make sense? Thanks, I think I've sorted it out. I've pushed an updated series with this change. > Yeah, Vulkan allocator fishing is weird. Allowing custom allocators is one of the bad parts of the Vulkan spec; it will "never" get used, and the chances of it working correctly in any driver are pretty small. But, we do what we can to implement it. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] anv, radv: Add support for VK_KHR_get_display_properties2
Jason Ekstrand writes: > I just sent a v2 which allocates a temporary array, calls properties2, and > then copies it back over. It doesn't duplicate the iteration code and > instead just leverages propertie2. On the down side, it's a bit more > allocation and data motion but, compared to the ioctl that properties2 is > doing, it shouldn't be noticeable. Let me know what you think of that > version. Seems a bit better; this is not exactly a high-use API and clearer counts for a lot. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] anv, radv: Add support for VK_KHR_get_display_properties2
Jason Ekstrand writes: All looks good to me. Reviewed-by: Keith Packard -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] vulkan: EXT_acquire_xlib_display requires libXrandr headers to build
Eric Engestrom writes: > On Tuesday, 2018-06-19 16:06:14 -0700, Keith Packard wrote: >> When VK_USE_PLATFORM_XLIB_XRANDR_EXT is defined, vulkan.h includes >> X11/extensions/Xrandr.h for the RROutput typedef which is used in >> the vkGetRandROutputDisplayEXT interface. >> >> Make sure we have the required header by checking during the build, >> and also set CFLAGS to point at the right directory. >> >> We don't need to link against the library as we don't use any >> functions from there, so don't add the _LIBS value in the autotools >> build. >> >> Signed-off-by: Keith Packard > > Fixes: dbac8e25f851ed44c51f "radv: Add EXT_acquire_xlib_display to radv > driver [v2]" > Reviewed-by: Eric Engestrom Thanks for testing. I've pushed this to master. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
Jason Ekstrand writes: > You can have a full reviewed-by You're too kind :-) -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 6/7] anv: add VK_EXT_display_control to anv driver [v2]
Jason Ekstrand writes: >> + if (allocator) >> + alloc = allocator; >> + else >> + alloc = >instance->alloc; >> > > This is what vk_alloc2 is for. :-) ... > And vk_free2 ... > This isn't needed if you're using vk_alloc2 Yeah, but I need to pass the allocator down to the wsi common code, and that doesn't have any way to touch the device driver allocator pointer. I bet I'm just missing something here. Help? >> + >> + fence = vk_zalloc2(>alloc, allocator, sizeof (*fence), 8, >> + VK_SYSTEM_ALLOCATION_SCOPE_OBJECT); >> > > Above you used vk_alloc and here you're using vk_zalloc. Mind picking > one? I don't think zalloc is needed but it doesn't hurt so I don't care > which. Thanks. Existing code is using zalloc for fences; I'll use that everywhere. > Indentation could be consistent between the two functions you add. I don't > care which style. Sure; these function names are kinda long. I've wrapped the first call after the ( > vk_free2? I've had to compute 'alloc' to pass into wsi_common; I figured I might as well use it. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
Jason Ekstrand writes: > I don't think I have any more comments on this patch. It's gross but I > think it should work. I'll mark you down as 'Acked-by' then. Neither of us loves the implementation; I'll see about creating the kernel infrastructure necessary to supplant it. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] vulkan/wsi: Get rid of the get_capabilities hook
Jason Ekstrand writes: > They're being used as helpers and they're actually kind-of nice for > that. Sounds good. I hadn't actually looked at the details; just saw the functions going away in the initializers. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [counter-PATCH 2/2] Vulkan/wsi: Implement VK_EXT_display_surface_counter
Jason Ekstrand writes: > C99 guarantees that, one one field is initialized with a designated > initializer then all fields not explicitly initialized get > zero-initialized. I can add it if you'd like none the less. Yeah, I just thought it would be clearer to the reader if the field were explicitly initialized to the correct value, rather than having the compiler do it. >> This should probably be at the top of wsi_common.h with the other >> structure type values so we don't accidentally re-use this value. >> > > Sure. Awesome. Sounds like you're good to go. With the addition of the explicit initialization, this patch is Reviewed-by: Keith Packard -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] anv, radv: Add support for VK_KHR_get_display_properties2
Jason Ekstrand writes: > Thoughts? You've looked at the code more closely than I have; please feel free to leave it if you think it would seem worse as separate functions. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 5/7] vulkan: add VK_EXT_display_control [v5]
Jason Ekstrand writes:
>> + if (!prop)
>> + continue;
>> + if (prop->flags & DRM_MODE_PROP_ENUM) {
>> + if (!strcmp(prop->name, "DPMS"))
>> +connector->dpms_property = drm_connector->props[p];
>>
>
> break?
Not break; I need to free the property. However, an early exit from the
loop seems reasonable. How about:
for (int p = 0; connector->dpms_property == 0 && p <
drm_connector->count_props; p++) {
This skips the whole sequence if the property has already been found, or
stops as soon as it has.
>> +static bool
>> +wsi_display_fence_wait(struct wsi_fence *fence_wsi,
>> + bool absolute,
>> + uint64_t timeout)
>>
>
> Would it make more sense for this function to return a VkResult? Then you
> could tell the difference between success, timeout, and some other
> failure. I guess the only other thing to return would be
> VK_ERROR_DEVICE_LOST which seems pretty harsh but, then again,
> pthread_timed_wait just failed so that's also really bad.
That's a good idea. The boolean return is pretty ambiguous. I copied
that from the radv internal fence API, which could also benefit from
this change. I've changed the API and adjusted the anv and radv code to
match. It reads a lot better now.
>> + if (!absolute)
>> + timeout = wsi_rel_to_abs_time(timeout);
>>
>
> Are relative times really useful? I suspect it doesn't save you more than
> a couple of lines and it makes the interface weird.
No. Relative timeouts aren't actually used anywhere either. I've removed them.
I did catch a mistake in the anv driver looking at this -- the !waitAll
code wasn't bothering to check the fences if the time had already
passed, so an application polling would never catch the fences being
ready. I've changed the while (current_time < timeout) {} to a do {}
while (current_time < timeout) loop.
>> +static void
>> +wsi_display_fence_destroy(struct wsi_fence *fence_wsi)
>> +{
>> + struct wsi_display_fence *fence = (struct wsi_display_fence *)
>> fence_wsi;
>> +
>>
>
> An assert(!fence->destroyed) in here might be useful to guard against
> double-frees.
Sure. I was under the impression that application bugs weren't supposed
to be rigorously checked in the implementation though? When should I be
checking API usage issues?
>> + if (!ret)
>> + return VK_SUCCESS;
>> +
>> + if (errno != ENOMEM) {
>> + wsi_display_debug("queue vblank event %lu failed\n",
>> fence->sequence);
>> + struct timespec delay = {
>> +.tv_sec = 0,
>> +.tv_nsec = 1ull,
>> + };
>> + nanosleep(, NULL);
>> + return VK_ERROR_OUT_OF_HOST_MEMORY;
>>
>
> Why are we sleeping for 0.1s before we return? That seems fishy.
Yeah, the kernel API is not great. There's a finite queue which can be
consumed with both flip events and vblank wait events. If that fills,
we'll get an error back. The only way to empty it is to have some events
get delivered, and those will only get delivered after a vblank happens.
It's an application bug that triggers this -- requesting too many vblank
events. Throttling the application so it doesn't just spin makes it
possible to stop it.
>> + pthread_mutex_lock(>wait_mutex);
>> + ret = wsi_display_wait_for_event(wsi, wsi_rel_to_abs_time(
>> 1ull));
>>
>
> What's with the magic number?
0.1s -- a value which is longer than any display time, but short enough
to catch things like DPMS off or VT switch without unduly delaying the
application.
>> +VkResult
>> +wsi_register_device_event(VkDevice device,
>> + struct wsi_device *wsi_device,
>> + const VkDeviceEventInfoEXT *device_event_info,
>> + const VkAllocationCallbacks *allocator,
>> + struct wsi_fence **fence_p)
>> +{
>> + return VK_ERROR_FEATURE_NOT_PRESENT;
>>
>
> I don't think we're allowed to just not implemnet this. At the very least,
> we should accept the event and never trigger it. Better would be to
> actually wire up hotplug detection. I have no idea how insane that would
> be to do. :-P
It's not a big deal to implement, I just didn't need it. I suppose the
test suite will be unhappy with this? Let me know if you want to insist
on having it implemented.
> Both RegisterDeviceEvent and RegisterDisplayEvent say they can only return
> VK_SUCCESS. We should submit a MR against the extensions to also allow
> OUT_OF_HOST_MEMORY at the very least.
There's already weasel words in the section on memory allocation that
says the command must generate VK_ERROR_OUT_OF_HOST_MEMORY when
allocation fails. But, it would be nice for these APIs to be documented
as possibly returning that value.
> Any particular reason to put these all the way down here? I think my
> preference would be to move wsi_display_fence_event_handler to right after
> wsi_display_fence_check_free and give it a predeclaration
Re: [Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
Jason Ekstrand writes: > I suppose we probably shouldn't worry about current_time being greater than > INT64_MAX? I guess if that happens we have pretty big problems... Nope, we've already given up on that working -- it's a couple hundred years of system uptime. Neither of us have any concerns in this area. >>timeout = MIN2(max_timeout, timeout); >> >>return (current_time + timeout); >> } >> > > Yeah, I think that's better. Cool. I'll wait for further review :-) -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
Jason Ekstrand writes:
> I still don't really like this but I agree that this code really should not
> be getting hit very often so it's probably not too bad. Maybe one day
> we'll be able to drop the non-syncobj paths entirely. Wouldn't that be
> nice.
I agree. What I want to have is kernel-side syncobj support for all of
the WSI fences that we need here.
I thought about using syncobjs and signaling them from user-space, but
realized that we couldn't eliminate the non-syncobj path quite yet as
that requires a new enough kernel. And, if we want to get to
kernel-native WSI syncobjs, that would mean we'd eventually have three
code paths. I think it's better to have one 'legacy' path, which works
on all kernels, and then one 'modern' path which does what we want.
> In the mean time, this is probably fine. I did see one issue below
> with time conversions that should be fixed though.
Always a hard thing to get right.
>> +static uint64_t anv_get_absolute_timeout(uint64_t timeout)
>> +{
>> + if (timeout == 0)
>> + return 0;
>> + uint64_t current_time = gettime_ns();
>> +
>> + timeout = MIN2(INT64_MAX - current_time, timeout);
>> +
>> + return (current_time + timeout);
>> +}
>>
>
> This does not have the same behavior as the code it replaces. In
> particular, the old code saturates at INT64_MAX whereas this code does not
> do so properly anymore. If UINT64_MAX gets passed into timeout, I believe
> that will be implicitly casted to signed and the MIN will yield -1 which
> gets casted back to unsigned and you get UINT64_MAX again.
It won't not get implicitly casted to signed; the implicit cast works
the other way where OP implicitly casts the
operand to unsigned for types of the same 'rank' (where 'rank' is not
quite equivalent to size). Here's a fine article on this:
https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules
However, this is a rather obscure part of the ISO standard, and I don't
think we should expect that people reading the code know it well. Adding
a cast to make it clear what we want is a good idea. How about this?
static uint64_t anv_get_absolute_timeout(uint64_t timeout)
{
if (timeout == 0)
return 0;
uint64_t current_time = gettime_ns();
uint64_t max_timeout = (uint64_t) INT64_MAX - current_time;
timeout = MIN2(max_timeout, timeout);
return (current_time + timeout);
}
> This is a problem because the value we pass into the kernel ioctls is
> signed. The behavior of the kernel *should* be infinite when timeout
> < 0 but, on some older kernels, -1 is treated as 0 and you get no
> timeout.
Yeah, we definitely want to cap the values to INT64_MAX.
>> - else if (current_ns + _timeout > INT64_MAX)
>>
>
> I suspect we need to cast INT64_MAX to a uint64_t here to ensure we don't
> accidentally get an implicit conversion to signed.
Again, it's not necessary given the C conversion rules, but it is a good
way to clarify the intent.
--
-keith
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [counter-PATCH 2/2] Vulkan/wsi: Implement VK_EXT_display_surface_counter
Jason Ekstrand writes:
> Sometimes the best way to review a patch is with another patch. :-) I'm
> not sure what you think of this approach but I didn't really relish the
> idea of having 3 get_capabilities entrypoints. With these two patches,
> we're now down to one. In order to implement VK_EXT_display_control, all
> you have to do is add support in wsi_display_surface_get_capabilities2 for
> the little chain-in struct and off we go.
I like this plan. Some comments below.
> + VkSurfaceCapabilities2KHR caps2 = {
> + .sType = VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES_2_KHR,
> + .pNext = ,
> + };
I'd like to see an explicit initialization of the
supported_surface_counters field here to make it clear that a WSI layer
that doesn't look for this structure will end up using the right value (0).
> +/* This is guaranteed to not collide with anything because it's in the
> + * VK_KHR_swapchain namespace but not actually used by the extension.
> + */
> +#define VK_STRUCTURE_TYPE_WSI_SURFACE_SUPPORTED_COUNTERS_MESA \
> + (VkStructureType)101005
This should probably be at the top of wsi_common.h with the other
structure type values so we don't accidentally re-use this value.
--
-keith
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] vulkan/wsi: Get rid of the get_capabilities hook
Jason Ekstrand writes: > Instead, we can just use get_capabilities2. This way back-ends only > have to implement one hook. Yeah, this looks nice. Are you going to remove the unused functions at some point? Reviewed-by: Keith Packard -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] anv, radv: Add support for VK_KHR_get_display_properties2
Jason Ekstrand writes: I see two styles here -- get_physical_device_display_properties* both use a helper function that takes a pointer to either of the property returns while get_physical_device_display_plane_properties* and get_display_mode_properties* open-code things. I'm easy with which style you pick, but I think they should be the same. I have a mild preference for the second style as I think it's easier to read the code without all of the conditionals. As for the actual implementation of each function, it looks good, so I'll actually mark this as Reviewed-by: Keith Packard If you want to rework the first bit, I'll review whatever changes you make. If you just want to rebase and push, you've got my Rb above :-) -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH mesa] vulkan: EXT_acquire_xlib_display requires libXrandr headers to build
When VK_USE_PLATFORM_XLIB_XRANDR_EXT is defined, vulkan.h includes
X11/extensions/Xrandr.h for the RROutput typedef which is used in
the vkGetRandROutputDisplayEXT interface.
Make sure we have the required header by checking during the build,
and also set CFLAGS to point at the right directory.
We don't need to link against the library as we don't use any
functions from there, so don't add the _LIBS value in the autotools
build.
Signed-off-by: Keith Packard
---
configure.ac | 2 ++
meson.build | 2 ++
src/amd/vulkan/Makefile.am | 3 ++-
src/amd/vulkan/meson.build | 2 +-
src/intel/Makefile.vulkan.am | 3 ++-
src/intel/vulkan/meson.build | 2 +-
src/vulkan/Makefile.am | 2 ++
src/vulkan/wsi/meson.build | 2 +-
8 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/configure.ac b/configure.ac
index 06524107786..e4320e8da7a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1877,6 +1877,8 @@ fi
if test x"$have_xlease" = xyes; then
randr_modules="x11-xcb xcb-randr"
PKG_CHECK_MODULES([XCB_RANDR], [$randr_modules])
+xlib_randr_modules="xrandr"
+PKG_CHECK_MODULES([XLIB_RANDR], [$xlib_randr_modules])
fi
AM_CONDITIONAL(HAVE_PLATFORM_X11, echo "$platforms" | grep -q 'x11')
diff --git a/meson.build b/meson.build
index ce54393fded..82279aad26c 100644
--- a/meson.build
+++ b/meson.build
@@ -1301,6 +1301,7 @@ dep_xcb_sync = null_dep
dep_xcb_xfixes = null_dep
dep_xshmfence = null_dep
dep_xcb_xrandr = null_dep
+dep_xlib_xrandr = null_dep
if with_platform_x11
if with_glx == 'xlib' or with_glx == 'gallium-xlib'
dep_x11 = dependency('x11')
@@ -1349,6 +1350,7 @@ if with_platform_x11
endif
if with_xlib_lease
dep_xcb_xrandr = dependency('xcb-randr', version : '>= 1.12')
+dep_xlib_xrandr = dependency('xrandr', version : '>= 1.3')
endif
endif
diff --git a/src/amd/vulkan/Makefile.am b/src/amd/vulkan/Makefile.am
index 8279fe4a81f..f9d3622f744 100644
--- a/src/amd/vulkan/Makefile.am
+++ b/src/amd/vulkan/Makefile.am
@@ -90,7 +90,8 @@ endif
if HAVE_XLIB_LEASE
AM_CPPFLAGS += \
-DVK_USE_PLATFORM_XLIB_XRANDR_EXT \
- $(XCB_RANDR_CFLAGS)
+ $(XCB_RANDR_CFLAGS) \
+ $(XLIB_RANDR_CFLAGS)
VULKAN_LIB_DEPS += $(XCB_RANDR_LIBS)
endif
diff --git a/src/amd/vulkan/meson.build b/src/amd/vulkan/meson.build
index bcdf83e0609..22857926fa1 100644
--- a/src/amd/vulkan/meson.build
+++ b/src/amd/vulkan/meson.build
@@ -121,7 +121,7 @@ if with_platform_drm
endif
if with_xlib_lease
- radv_deps += dep_xcb_xrandr
+ radv_deps += [dep_xcb_xrandr, dep_xlib_xrandr]
radv_flags += '-DVK_USE_PLATFORM_XLIB_XRANDR_EXT'
endif
diff --git a/src/intel/Makefile.vulkan.am b/src/intel/Makefile.vulkan.am
index ae625695814..4a80c3ae412 100644
--- a/src/intel/Makefile.vulkan.am
+++ b/src/intel/Makefile.vulkan.am
@@ -202,7 +202,8 @@ endif
if HAVE_XLIB_LEASE
VULKAN_CPPFLAGS += \
-DVK_USE_PLATFORM_XLIB_XRANDR_EXT \
- $(XCB_RANDR_CFLAGS)
+ $(XCB_RANDR_CFLAGS) \
+ $(XLIB_RANDR_CFLAGS)
VULKAN_LIB_DEPS += $(XCB_RANDR_LIBS)
endif
diff --git a/src/intel/vulkan/meson.build b/src/intel/vulkan/meson.build
index 4b0652f757b..e427c7471f4 100644
--- a/src/intel/vulkan/meson.build
+++ b/src/intel/vulkan/meson.build
@@ -170,7 +170,7 @@ if with_platform_drm
endif
if with_xlib_lease
- anv_deps += dep_xcb_xrandr
+ anv_deps += [dep_xcb_xrandr, dep_xlib_xrandr]
anv_flags += '-DVK_USE_PLATFORM_XLIB_XRANDR_EXT'
endif
diff --git a/src/vulkan/Makefile.am b/src/vulkan/Makefile.am
index 9deb6e18ff0..ce1a79d0c48 100644
--- a/src/vulkan/Makefile.am
+++ b/src/vulkan/Makefile.am
@@ -63,6 +63,8 @@ endif
if HAVE_XLIB_LEASE
AM_CPPFLAGS += \
+ $(XCB_RANDR_CFLAGS) \
+ $(XLIB_RANDR_CFLAGS) \
-DVK_USE_PLATFORM_XLIB_XRANDR_EXT
endif
diff --git a/src/vulkan/wsi/meson.build b/src/vulkan/wsi/meson.build
index 3501a864e18..d073b23dc25 100644
--- a/src/vulkan/wsi/meson.build
+++ b/src/vulkan/wsi/meson.build
@@ -68,7 +68,7 @@ if with_platform_drm
endif
if with_xlib_lease
- vulkan_wsi_deps += dep_xcb_xrandr
+ vulkan_wsi_deps += [dep_xcb_xrandr, dep_xlib_xrandr]
vulkan_wsi_args += '-DVK_USE_PLATFORM_XLIB_XRANDR_EXT'
endif
--
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 writes: > Looks good. Passes the CTS. Push it! All done. Now just two more series to go in this set :-) -- -keith signature.asc Description: PGP signature ___ 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 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 signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/7] vulkan: Add VK_EXT_display_surface_counter [v4]
Jason Ekstrand writes: > I really don't like adding a third get_capabilities hook. Yeah, but this new function takes a different struct parameter which has a different (but not strict superset) of contents from either of the existing functions. Annoying. > An alternative way to do this would be to add a pseud-extension which > allows you do get he extra bit of data with a chain-in on > vkGetSurfaceCapabilities2KHR. I sent two patches which do just that. > If you like them, I'm happy do do the reabase for you if you'd like. I'll review that when I have some time Monday. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 6/7] anv: add VK_EXT_display_control to anv driver [v2]
This extension provides fences and frame count information to direct
display contexts. It uses new kernel ioctls to provide 64-bits of
vblank sequence and nanosecond resolution.
v2: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Add extension to list in alphabetical order
Suggested-by: Jason Ekstrand
Signed-off-by: Keith Packard
---
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_private.h | 4 ++
src/intel/vulkan/anv_queue.c | 22 +++
src/intel/vulkan/anv_wsi_display.c | 97 ++
4 files changed, 124 insertions(+)
diff --git a/src/intel/vulkan/anv_extensions.py
b/src/intel/vulkan/anv_extensions.py
index 68e545a40f8..8c010e9280b 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -111,6 +111,7 @@ EXTENSIONS = [
Extension('VK_EXT_acquire_xlib_display', 1,
'VK_USE_PLATFORM_XLIB_XRANDR_EXT'),
Extension('VK_EXT_debug_report', 8, True),
Extension('VK_EXT_direct_mode_display', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
+Extension('VK_EXT_display_control', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_display_surface_counter', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_external_memory_dma_buf', 1, True),
Extension('VK_EXT_global_priority', 1,
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index fb91bc33046..c81885979ad 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -2133,6 +2133,7 @@ enum anv_fence_type {
ANV_FENCE_TYPE_NONE = 0,
ANV_FENCE_TYPE_BO,
ANV_FENCE_TYPE_SYNCOBJ,
+ ANV_FENCE_TYPE_WSI,
};
enum anv_bo_fence_state {
@@ -2167,6 +2168,9 @@ struct anv_fence_impl {
/** DRM syncobj handle for syncobj-based fences */
uint32_t syncobj;
+
+ /** WSI fence */
+ struct wsi_fence *fence_wsi;
};
};
diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c
index 8df99c84549..073e65acf5e 100644
--- a/src/intel/vulkan/anv_queue.c
+++ b/src/intel/vulkan/anv_queue.c
@@ -324,6 +324,10 @@ anv_fence_impl_cleanup(struct anv_device *device,
anv_gem_syncobj_destroy(device, impl->syncobj);
break;
+ case ANV_FENCE_TYPE_WSI:
+ impl->fence_wsi->destroy(impl->fence_wsi);
+ break;
+
default:
unreachable("Invalid fence type");
}
@@ -672,6 +676,21 @@ done:
return result;
}
+static VkResult
+anv_wait_for_wsi_fence(struct anv_device *device,
+ const VkFence _fence,
+ uint64_t abs_timeout)
+{
+ ANV_FROM_HANDLE(anv_fence, fence, _fence);
+
+ struct anv_fence_impl *impl = >permanent;
+ bool expired = impl->fence_wsi->wait(impl->fence_wsi, true, abs_timeout);
+
+ if (!expired)
+ return VK_TIMEOUT;
+ return VK_SUCCESS;
+}
+
static VkResult
anv_wait_for_fences(struct anv_device *device,
uint32_t fenceCount,
@@ -694,6 +713,9 @@ anv_wait_for_fences(struct anv_device *device,
result = anv_wait_for_syncobj_fences(device, 1, [i],
true, abs_timeout);
break;
+ case ANV_FENCE_TYPE_WSI:
+result = anv_wait_for_wsi_fence(device, pFences[i], abs_timeout);
+break;
case ANV_FENCE_TYPE_NONE:
result = VK_SUCCESS;
break;
diff --git a/src/intel/vulkan/anv_wsi_display.c
b/src/intel/vulkan/anv_wsi_display.c
index f749a8d98f7..cd736bcdd74 100644
--- a/src/intel/vulkan/anv_wsi_display.c
+++ b/src/intel/vulkan/anv_wsi_display.c
@@ -168,3 +168,100 @@ anv_GetRandROutputDisplayEXT(VkPhysicalDevice
physical_device,
display);
}
#endif /* VK_USE_PLATFORM_XLIB_XRANDR_EXT */
+
+/* VK_EXT_display_control */
+
+VkResult
+anv_DisplayPowerControlEXT(VkDevice_device,
+VkDisplayKHRdisplay,
+const VkDisplayPowerInfoEXT *display_power_info)
+{
+ ANV_FROM_HANDLE(anv_device, device, _device);
+
+ return wsi_display_power_control(
+ _device, >instance->physicalDevice.wsi_device,
+ display, display_power_info);
+}
+
+VkResult
+anv_RegisterDeviceEventEXT(VkDevice _device,
+const VkDeviceEventInfoEXT *device_event_info,
+const VkAllocationCallbacks *allocator,
+VkFence *_fence)
+{
+ ANV_FROM_HANDLE(anv_device, device, _device);
+ const VkAllocationCallbacks *alloc;
+ struct anv_fence *fence;
+ VkResult ret;
+
+ if (allocator)
+ alloc = allocator;
+ else
+ alloc = >instance->alloc;
+
[Mesa-dev] [PATCH 7/7] radv: add VK_EXT_display_control to radv driver [v3]
This extension provides fences and frame count information to direct
display contexts. It uses new kernel ioctls to provide 64-bits of
vblank sequence and nanosecond resolution.
v2:
Rework fence integration into the driver so that waiting for
any of a mixture of fence types (wsi, driver or syncobjs)
causes the driver to poll, while a list of just syncobjs or
just driver fences will block. When we get syncobjs for wsi
fences, we'll adapt to use them.
v3: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Suggested-by: Jason Ekstrand
Signed-off-by: Keith Packard
---
src/amd/vulkan/radv_device.c | 68 +-
src/amd/vulkan/radv_extensions.py | 1 +
src/amd/vulkan/radv_private.h | 1 +
src/amd/vulkan/radv_wsi_display.c | 113 ++
4 files changed, 167 insertions(+), 16 deletions(-)
diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index 59ee503c8c2..fd80db1c9b4 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -3240,6 +3240,7 @@ VkResult radv_CreateFence(
if (!fence)
return vk_error(device->instance, VK_ERROR_OUT_OF_HOST_MEMORY);
+ fence->fence_wsi = NULL;
fence->submitted = false;
fence->signalled = !!(pCreateInfo->flags &
VK_FENCE_CREATE_SIGNALED_BIT);
fence->temp_syncobj = 0;
@@ -3284,6 +3285,8 @@ void radv_DestroyFence(
device->ws->destroy_syncobj(device->ws, fence->syncobj);
if (fence->fence)
device->ws->destroy_fence(fence->fence);
+ if (fence->fence_wsi)
+ fence->fence_wsi->destroy(fence->fence_wsi);
vk_free2(>alloc, pAllocator, fence);
}
@@ -3309,7 +3312,19 @@ static bool radv_all_fences_plain_and_submitted(uint32_t
fenceCount, const VkFen
{
for (uint32_t i = 0; i < fenceCount; ++i) {
RADV_FROM_HANDLE(radv_fence, fence, pFences[i]);
- if (fence->syncobj || fence->temp_syncobj || (!fence->signalled
&& !fence->submitted))
+ if (fence->fence == NULL || fence->syncobj ||
+ fence->temp_syncobj ||
+ (!fence->signalled && !fence->submitted))
+ return false;
+ }
+ return true;
+}
+
+static bool radv_all_fences_syncobj(uint32_t fenceCount, const VkFence
*pFences)
+{
+ for (uint32_t i = 0; i < fenceCount; ++i) {
+ RADV_FROM_HANDLE(radv_fence, fence, pFences[i]);
+ if (fence->syncobj == 0 && fence->temp_syncobj == 0)
return false;
}
return true;
@@ -3325,7 +3340,9 @@ VkResult radv_WaitForFences(
RADV_FROM_HANDLE(radv_device, device, _device);
timeout = radv_get_absolute_timeout(timeout);
- if (device->always_use_syncobj) {
+ if (device->always_use_syncobj &&
+ radv_all_fences_syncobj(fenceCount, pFences))
+ {
uint32_t *handles = malloc(sizeof(uint32_t) * fenceCount);
if (!handles)
return vk_error(device->instance,
VK_ERROR_OUT_OF_HOST_MEMORY);
@@ -3395,21 +3412,35 @@ VkResult radv_WaitForFences(
if (fence->signalled)
continue;
- if (!fence->submitted) {
- while(radv_get_current_time() <= timeout &&
!fence->submitted)
- /* Do nothing */;
+ if (fence->fence) {
+ if (!fence->submitted) {
+ while(radv_get_current_time() <= timeout &&
+ !fence->submitted)
+ /* Do nothing */;
- if (!fence->submitted)
- return VK_TIMEOUT;
+ if (!fence->submitted)
+ return VK_TIMEOUT;
- /* Recheck as it may have been set by submitting
operations. */
- if (fence->signalled)
- continue;
+ /* Recheck as it may have been set by
+* submitting operations. */
+
+ if (fence->signalled)
+ continue;
+ }
+
+ expired = device->ws->fence_wait(device->ws,
+fence->fence,
+true, timeout);
+
[Mesa-dev] [PATCH 0/7] vulkan: Add VK_EXT_display_control and VK_EXT_display_surface_counter
Here's a couple of reasonably straightforward extensions implemented for both anv and radv drivers. VK_EXT_display_surface_counter is a very simple extension which adds an API, vkGetPhysicalSurfaceCapabilities2EXT, to extend the existing vkGetPhysicalDeviceSurfaceCapabilitiesKHR and vkGetPhysicalDeviceSurfaceCapablities2KHR interfaces. The new interface uses a slightly different structure that includes a new flags word, "supportedSurfaceCounters". This new field describes the counters available from the underlying WSI layer. As this extension doesn't provide any counters itself, each WSI layer initializes this to zero for now. VK_EXT_display_control is an extension specific to display surfaces (those created via the KHR_display extension). This extension adds DPMS support and fences which signal when vblank or monitor hotplug events happen. I've implemented the DPMS support and the vblank fence stuff; I haven't bothered hooking up the monitor hotplug bits, although it would be fairly simple. To make the fences work on anv, I had to refactor the existing fence waiting code to add the ability to spin waiting for any of a set of heterogeneous fences (a mixture of syncobj and bo fences) to become ready; this mirrors the same functionality in the radv driver, although anv hadn't needed it before as it only supported homogenous fence types (either all syncobj or all bo). I've created a separate patch for this refactoring to try and reduce the difficulty in reviewing that part. -keith ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 5/7] vulkan: add VK_EXT_display_control [v5]
This extension provides fences and frame count information to direct
display contexts. It uses new kernel ioctls to provide 64-bits of
vblank sequence and nanosecond resolution.
v2: Remove DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT flag. This has
been removed from the proposed kernel API.
Add NULL parameter to drmCrtcQueueSequence ioctl as we
don't care what sequence the event was actually queued to.
v3: Adapt to pthread clock switch to MONOTONIC
v4: Fix scope for wsi_display_mode andwsi_display_connector allocs
Suggested-by: Jason Ekstrand
v5: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Use wsi_rel_to_abs_time helper function to convert relative
timeouts to absolute timeouts without causing overflow.
Suggested-by: Jason Ekstrand
Signed-off-by: Keith Packard
---
src/vulkan/wsi/wsi_common.h | 10 +
src/vulkan/wsi/wsi_common_display.c | 307 +++-
src/vulkan/wsi/wsi_common_display.h | 29 +++
3 files changed, 345 insertions(+), 1 deletion(-)
diff --git a/src/vulkan/wsi/wsi_common.h b/src/vulkan/wsi/wsi_common.h
index 054aad23c1c..081fe1dcf8d 100644
--- a/src/vulkan/wsi/wsi_common.h
+++ b/src/vulkan/wsi/wsi_common.h
@@ -66,6 +66,16 @@ struct wsi_format_modifier_properties_list {
struct wsi_format_modifier_properties *modifier_properties;
};
+struct wsi_fence {
+ VkDevice device;
+ const struct wsi_device *wsi_device;
+ VkDisplayKHR display;
+ const VkAllocationCallbacks *alloc;
+ bool (*wait)(struct wsi_fence *fence,
+bool absolute, uint64_t timeout);
+ void (*destroy)(struct wsi_fence *fence);
+};
+
struct wsi_interface;
#define VK_ICD_WSI_PLATFORM_MAX (VK_ICD_WSI_PLATFORM_DISPLAY + 1)
diff --git a/src/vulkan/wsi/wsi_common_display.c
b/src/vulkan/wsi/wsi_common_display.c
index 504f7741d73..40eaa6a322e 100644
--- a/src/vulkan/wsi/wsi_common_display.c
+++ b/src/vulkan/wsi/wsi_common_display.c
@@ -79,6 +79,7 @@ typedef struct wsi_display_connector {
struct list_head display_modes;
wsi_display_mode *current_mode;
drmModeModeInfo current_drm_mode;
+ uint32_t dpms_property;
#ifdef VK_USE_PLATFORM_XLIB_XRANDR_EXT
xcb_randr_output_t output;
#endif
@@ -132,6 +133,15 @@ struct wsi_display_swapchain {
struct wsi_display_image images[0];
};
+struct wsi_display_fence {
+ struct wsi_fence base;
+ bool event_received;
+ bool destroyed;
+ uint64_t sequence;
+};
+
+static uint64_t fence_sequence;
+
ICD_DEFINE_NONDISP_HANDLE_CASTS(wsi_display_mode, VkDisplayModeKHR)
ICD_DEFINE_NONDISP_HANDLE_CASTS(wsi_display_connector, VkDisplayKHR)
@@ -307,6 +317,19 @@ wsi_display_get_connector(struct wsi_device *wsi_device,
connector->connected = drm_connector->connection != DRM_MODE_DISCONNECTED;
+ /* Look for a DPMS property */
+ for (int p = 0; p < drm_connector->count_props; p++) {
+ drmModePropertyPtr prop = drmModeGetProperty(wsi->fd,
+ drm_connector->props[p]);
+ if (!prop)
+ continue;
+ if (prop->flags & DRM_MODE_PROP_ENUM) {
+ if (!strcmp(prop->name, "DPMS"))
+connector->dpms_property = drm_connector->props[p];
+ }
+ drmModeFreeProperty(prop);
+ }
+
/* Mark all connector modes as invalid */
wsi_display_invalidate_connector_modes(wsi_device, connector);
@@ -663,7 +686,7 @@ wsi_display_surface_get_capabilities2ext(VkIcdSurfaceBase
*icd_surface,
caps->currentTransform = khr_caps.currentTransform;
caps->supportedCompositeAlpha = khr_caps.supportedCompositeAlpha;
caps->supportedUsageFlags = khr_caps.supportedUsageFlags;
- caps->supportedSurfaceCounters = 0;
+ caps->supportedSurfaceCounters = VK_SURFACE_COUNTER_VBLANK_EXT;
return ret;
}
@@ -896,12 +919,21 @@ static void wsi_display_page_flip_handler(int fd,
wsi_display_page_flip_handler2(fd, frame, sec, usec, 0, data);
}
+static void wsi_display_vblank_handler(int fd, unsigned int frame,
+ unsigned int sec, unsigned int usec,
+ void *data);
+
+static void wsi_display_sequence_handler(int fd, uint64_t frame,
+ uint64_t ns, uint64_t user_data);
+
static drmEventContext event_context = {
.version = DRM_EVENT_CONTEXT_VERSION,
.page_flip_handler = wsi_display_page_flip_handler,
#if DRM_EVENT_CONTEXT_VERSION >= 3
.page_flip_handler2 = wsi_display_page_flip_handler2,
#endif
+ .vblank_handler = wsi_display_vblank_handler,
+ .
[Mesa-dev] [PATCH 4/7] anv: Support wait for heterogeneous list of fences [v2]
Handle the case where the set of fences to wait for is not all of the
same type by either waiting for them sequentially (waitAll), or
polling them until the timer has expired (!waitAll). We hope the
latter case is not common.
While the current code makes sure that it always has fences of only
one type, that will not be true when we add WSI fences. Split out this
refactoring to make merging that clearer.
v2: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace between
types and names. Wrap lines to 80 columns.
Suggested-by: Jason Ekstrand
Signed-off-by: Keith Packard
---
src/intel/vulkan/anv_queue.c | 105 +--
1 file changed, 88 insertions(+), 17 deletions(-)
diff --git a/src/intel/vulkan/anv_queue.c b/src/intel/vulkan/anv_queue.c
index 6fe04a0a19d..8df99c84549 100644
--- a/src/intel/vulkan/anv_queue.c
+++ b/src/intel/vulkan/anv_queue.c
@@ -459,12 +459,32 @@ gettime_ns(void)
return (uint64_t)current.tv_sec * NSEC_PER_SEC + current.tv_nsec;
}
+static uint64_t anv_get_absolute_timeout(uint64_t timeout)
+{
+ if (timeout == 0)
+ return 0;
+ uint64_t current_time = gettime_ns();
+
+ timeout = MIN2(INT64_MAX - current_time, timeout);
+
+ return (current_time + timeout);
+}
+
+static int64_t anv_get_relative_timeout(uint64_t abs_timeout)
+{
+ uint64_t now = gettime_ns();
+
+ if (abs_timeout < now)
+ return 0;
+ return abs_timeout - now;
+}
+
static VkResult
anv_wait_for_syncobj_fences(struct anv_device *device,
uint32_t fenceCount,
const VkFence *pFences,
bool waitAll,
-uint64_t _timeout)
+uint64_t abs_timeout_ns)
{
uint32_t *syncobjs = vk_zalloc(>alloc,
sizeof(*syncobjs) * fenceCount, 8,
@@ -484,19 +504,6 @@ anv_wait_for_syncobj_fences(struct anv_device *device,
syncobjs[i] = impl->syncobj;
}
- int64_t abs_timeout_ns = 0;
- if (_timeout > 0) {
- uint64_t current_ns = gettime_ns();
-
- /* Add but saturate to INT32_MAX */
- if (current_ns + _timeout < current_ns)
- abs_timeout_ns = INT64_MAX;
- else if (current_ns + _timeout > INT64_MAX)
- abs_timeout_ns = INT64_MAX;
- else
- abs_timeout_ns = current_ns + _timeout;
- }
-
/* The gem_syncobj_wait ioctl may return early due to an inherent
* limitation in the way it computes timeouts. Loop until we've actually
* passed the timeout.
@@ -665,6 +672,67 @@ done:
return result;
}
+static VkResult
+anv_wait_for_fences(struct anv_device *device,
+uint32_t fenceCount,
+const VkFence *pFences,
+bool waitAll,
+uint64_t abs_timeout)
+{
+ VkResult result = VK_SUCCESS;
+
+ if (fenceCount <= 1 || waitAll) {
+ for (uint32_t i = 0; i < fenceCount; i++) {
+ ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
+ switch (fence->permanent.type) {
+ case ANV_FENCE_TYPE_BO:
+result = anv_wait_for_bo_fences(
+ device, 1, [i], true,
+ anv_get_relative_timeout(abs_timeout));
+break;
+ case ANV_FENCE_TYPE_SYNCOBJ:
+result = anv_wait_for_syncobj_fences(device, 1, [i],
+ true, abs_timeout);
+break;
+ case ANV_FENCE_TYPE_NONE:
+result = VK_SUCCESS;
+break;
+ }
+ if (result != VK_SUCCESS)
+return result;
+ }
+ } else {
+ while (gettime_ns() < abs_timeout) {
+ for (uint32_t i = 0; i < fenceCount; i++) {
+if (anv_wait_for_fences(device, 1, [i], true, 0) ==
VK_SUCCESS)
+ return VK_SUCCESS;
+ }
+ }
+ result = VK_TIMEOUT;
+ }
+ return result;
+}
+
+static bool anv_all_fences_syncobj(uint32_t fenceCount, const VkFence *pFences)
+{
+ for (uint32_t i = 0; i < fenceCount; ++i) {
+ ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
+ if (fence->permanent.type != ANV_FENCE_TYPE_SYNCOBJ)
+ return false;
+ }
+ return true;
+}
+
+static bool anv_all_fences_bo(uint32_t fenceCount, const VkFence *pFences)
+{
+ for (uint32_t i = 0; i < fenceCount; ++i) {
+ ANV_FROM_HANDLE(anv_fence, fence, pFences[i]);
+ if (fence->permanent.type != ANV_FENCE_TYPE_BO)
+ return false;
+ }
+ return true;
+}
+
VkResult anv_WaitForFences(
VkDevice_device,
uint32_tfenceCount,
@@ -677,12 +745,15 @@ VkResult anv_WaitForFences(
if (unlikely(device->lost))
return VK_ERROR_DEVICE_LOST;
- if (device->instance->physicalDevice.has_syncobj_wait) {
+ if (anv_all_fences_syncobj(fenceCount,
[Mesa-dev] [PATCH 1/7] vulkan: Add VK_EXT_display_surface_counter [v4]
This extension is required to support EXT_display_control as it offers
a way to query whether the vblank counter is supported.
v2: Thanks to kisak
Fix spelling of VkSurfaceCapabilities2EXT in wsi_common_wayland.c,
it was using ext instead of EXT.
Fix spelling of VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES_2_EXT
v3: Fix wayland WSI, regularize spelling
Misspelled 'surface' in get_capabilities2ext
Remove extra _ from get_capabilities_2ext in a couple of places
v4: Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace
between types and names. Wrap lines to 80 columns.
Suggested-by: Jason Ekstrand
Signed-off-by: Keith Packard
---
src/vulkan/wsi/wsi_common.c | 12
src/vulkan/wsi/wsi_common.h | 6 ++
src/vulkan/wsi/wsi_common_display.c | 27 +++
src/vulkan/wsi/wsi_common_private.h | 3 +++
src/vulkan/wsi/wsi_common_wayland.c | 27 +++
src/vulkan/wsi/wsi_common_x11.c | 27 +++
6 files changed, 102 insertions(+)
diff --git a/src/vulkan/wsi/wsi_common.c b/src/vulkan/wsi/wsi_common.c
index 142c5d8fe58..91d0b72e1ba 100644
--- a/src/vulkan/wsi/wsi_common.c
+++ b/src/vulkan/wsi/wsi_common.c
@@ -710,6 +710,18 @@ wsi_common_get_surface_capabilities2(struct wsi_device
*wsi_device,
pSurfaceCapabilities);
}
+VkResult
+wsi_common_get_surface_capabilities2ext(
+ struct wsi_device *wsi_device,
+ VkSurfaceKHR _surface,
+ VkSurfaceCapabilities2EXT *pSurfaceCapabilities)
+{
+ ICD_FROM_HANDLE(VkIcdSurfaceBase, surface, _surface);
+ struct wsi_interface *iface = wsi_device->wsi[surface->platform];
+
+ return iface->get_capabilities2ext(surface, pSurfaceCapabilities);
+}
+
VkResult
wsi_common_get_surface_formats(struct wsi_device *wsi_device,
VkSurfaceKHR _surface,
diff --git a/src/vulkan/wsi/wsi_common.h b/src/vulkan/wsi/wsi_common.h
index 61b1de59d7f..054aad23c1c 100644
--- a/src/vulkan/wsi/wsi_common.h
+++ b/src/vulkan/wsi/wsi_common.h
@@ -178,6 +178,12 @@ wsi_common_get_surface_present_modes(struct wsi_device
*wsi_device,
uint32_t *pPresentModeCount,
VkPresentModeKHR *pPresentModes);
+VkResult
+wsi_common_get_surface_capabilities2ext(
+ struct wsi_device *wsi_device,
+ VkSurfaceKHR surface,
+ VkSurfaceCapabilities2EXT *pSurfaceCapabilities);
+
VkResult
wsi_common_get_images(VkSwapchainKHR _swapchain,
uint32_t *pSwapchainImageCount,
diff --git a/src/vulkan/wsi/wsi_common_display.c
b/src/vulkan/wsi/wsi_common_display.c
index e140e71c518..504f7741d73 100644
--- a/src/vulkan/wsi/wsi_common_display.c
+++ b/src/vulkan/wsi/wsi_common_display.c
@@ -641,6 +641,32 @@ wsi_display_surface_get_capabilities2(VkIcdSurfaceBase
*icd_surface,
>surfaceCapabilities);
}
+static VkResult
+wsi_display_surface_get_capabilities2ext(VkIcdSurfaceBase *icd_surface,
+ VkSurfaceCapabilities2EXT *caps)
+{
+ VkSurfaceCapabilitiesKHR khr_caps;
+ VkResult ret;
+
+ assert(caps->sType == VK_STRUCTURE_TYPE_SURFACE_CAPABILITIES_2_EXT);
+ ret = wsi_display_surface_get_capabilities(icd_surface, _caps);
+ if (ret)
+ return ret;
+
+ caps->minImageCount = khr_caps.minImageCount;
+ caps->maxImageCount = khr_caps.maxImageCount;
+ caps->currentExtent = khr_caps.currentExtent;
+ caps->minImageExtent = khr_caps.minImageExtent;
+ caps->maxImageExtent = khr_caps.maxImageExtent;
+ caps->maxImageArrayLayers = khr_caps.maxImageArrayLayers;
+ caps->supportedTransforms = khr_caps.supportedTransforms;
+ caps->currentTransform = khr_caps.currentTransform;
+ caps->supportedCompositeAlpha = khr_caps.supportedCompositeAlpha;
+ caps->supportedUsageFlags = khr_caps.supportedUsageFlags;
+ caps->supportedSurfaceCounters = 0;
+ return ret;
+}
+
static const struct {
VkFormat format;
uint32_t drm_format;
@@ -1393,6 +1419,7 @@ wsi_display_init_wsi(struct wsi_device *wsi_device,
wsi->base.get_support = wsi_display_surface_get_support;
wsi->base.get_capabilities = wsi_display_surface_get_capabilities;
wsi->base.get_capabilities2 = wsi_display_surface_get_capabilities2;
+ wsi->base.get_capabilities2ext = wsi_display_surface_get_capabilities2ext;
wsi->base.get_formats = wsi_display_surface_get_formats;
wsi->base.get_formats2 = wsi_display_surface_get_formats2;
wsi->base.get_present_modes = wsi_display_surface_get_present_modes;
diff --git a/src/vulkan/wsi/wsi_common_private.h
b/src/vulkan/wsi/wsi_common_private.h
index 3d502b9fc9d..b97d2d8ba06 100644
--- a/src/vulkan/wsi/wsi_common_private.h
+++ b/src/vulkan/wsi/wsi_common_private.h
@@ -10
[Mesa-dev] [PATCH 2/7] anv: Add VK_EXT_display_surface_counter to anv driver [v2]
This extension is required to support EXT_display_control as it offers
a way to query whether the vblank counter is supported.
v2:
Add extension to list in alphabetical order
Suggested-by: Jason Ekstrand
Signed-off-by: Keith Packard
---
src/intel/vulkan/anv_extensions.py | 1 +
src/intel/vulkan/anv_wsi.c | 12
2 files changed, 13 insertions(+)
diff --git a/src/intel/vulkan/anv_extensions.py
b/src/intel/vulkan/anv_extensions.py
index 4bffbcb5a2a..68e545a40f8 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -111,6 +111,7 @@ EXTENSIONS = [
Extension('VK_EXT_acquire_xlib_display', 1,
'VK_USE_PLATFORM_XLIB_XRANDR_EXT'),
Extension('VK_EXT_debug_report', 8, True),
Extension('VK_EXT_direct_mode_display', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
+Extension('VK_EXT_display_surface_counter', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_external_memory_dma_buf', 1, True),
Extension('VK_EXT_global_priority', 1,
'device->has_context_priority'),
diff --git a/src/intel/vulkan/anv_wsi.c b/src/intel/vulkan/anv_wsi.c
index a7efb1416fa..1403601e9c0 100644
--- a/src/intel/vulkan/anv_wsi.c
+++ b/src/intel/vulkan/anv_wsi.c
@@ -120,6 +120,18 @@ VkResult anv_GetPhysicalDeviceSurfaceCapabilities2KHR(
pSurfaceCapabilities);
}
+VkResult anv_GetPhysicalDeviceSurfaceCapabilities2EXT(
+ VkPhysicalDevicephysicalDevice,
+ VkSurfaceKHRsurface,
+ VkSurfaceCapabilities2EXT* pSurfaceCapabilities)
+{
+ ANV_FROM_HANDLE(anv_physical_device, device, physicalDevice);
+
+ return wsi_common_get_surface_capabilities2ext(>wsi_device,
+ surface,
+ pSurfaceCapabilities);
+}
+
VkResult anv_GetPhysicalDeviceSurfaceFormatsKHR(
VkPhysicalDevicephysicalDevice,
VkSurfaceKHRsurface,
--
2.17.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/7] radv: Add VK_EXT_display_surface_counter to radv driver
This extension is required to support EXT_display_control as it offers
a way to query whether the vblank counter is supported.
Signed-off-by: Keith Packard
---
src/amd/vulkan/radv_extensions.py | 1 +
src/amd/vulkan/radv_wsi.c | 12
2 files changed, 13 insertions(+)
diff --git a/src/amd/vulkan/radv_extensions.py
b/src/amd/vulkan/radv_extensions.py
index 65ce7349016..601a345b114 100644
--- a/src/amd/vulkan/radv_extensions.py
+++ b/src/amd/vulkan/radv_extensions.py
@@ -89,6 +89,7 @@ EXTENSIONS = [
Extension('VK_KHR_display', 23,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_direct_mode_display', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_acquire_xlib_display', 1,
'VK_USE_PLATFORM_XLIB_XRANDR_EXT'),
+Extension('VK_EXT_display_surface_counter', 1,
'VK_USE_PLATFORM_DISPLAY_KHR'),
Extension('VK_EXT_debug_report', 9, True),
Extension('VK_EXT_depth_range_unrestricted', 1, True),
Extension('VK_EXT_descriptor_indexing', 2, True),
diff --git a/src/amd/vulkan/radv_wsi.c b/src/amd/vulkan/radv_wsi.c
index 2840b666727..20484177135 100644
--- a/src/amd/vulkan/radv_wsi.c
+++ b/src/amd/vulkan/radv_wsi.c
@@ -103,6 +103,18 @@ VkResult radv_GetPhysicalDeviceSurfaceCapabilities2KHR(
pSurfaceCapabilities);
}
+VkResult radv_GetPhysicalDeviceSurfaceCapabilities2EXT(
+ VkPhysicalDevicephysicalDevice,
+ VkSurfaceKHRsurface,
+ VkSurfaceCapabilities2EXT* pSurfaceCapabilities)
+{
+ RADV_FROM_HANDLE(radv_physical_device, device, physicalDevice);
+
+ return wsi_common_get_surface_capabilities2ext(>wsi_device,
+ surface,
+ pSurfaceCapabilities);
+}
+
VkResult radv_GetPhysicalDeviceSurfaceFormatsKHR(
VkPhysicalDevicephysicalDevice,
VkSurfaceKHRsurface,
--
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 writes: > Looks good to me. With this properly sprinkled on the appropriate patches, > the entire series is > > Reviewed-by: Jason Ekstrand 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 signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] wsi_common_display: Deal with vscan values
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
---
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(_mode->list, >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 mesa 7/9] vulkan: Add EXT_acquire_xlib_display [v3]
Jason Ekstrand writes:
>> Signed-off-by: Keith Packard
>>
>> fixup for acquire
>>
>> fixup for RROutput type
>>
>> Signed-off-by: Keith Packard
>>
>> fixup
>>
>
> Lots of "fixup". Did you mean to actually comment on what that was?
Sorry; I was squashing patches and moving comments into the main message
and just left this noise below.
>> +static bool
>> +wsi_display_mode_matches_x(struct wsi_display_mode *wsi,
>> + xcb_randr_mode_info_t *xcb)
>> +{
>> + return wsi->clock == (xcb->dot_clock + 500) / 1000 &&
>> + wsi->hdisplay == xcb->width &&
>> + wsi->hsync_start == xcb->hsync_start &&
>> + wsi->hsync_end == xcb->hsync_end &&
>> + wsi->htotal == xcb->htotal &&
>> + wsi->hskew == xcb->hskew &&
>> + wsi->vdisplay == xcb->height &&
>> + wsi->vsync_start == xcb->vsync_start &&
>> + wsi->vsync_end == xcb->vsync_end &&
>> + wsi->vtotal == xcb->vtotal &&
>>
>
> You're not checking vscan here.
Yeah, I'm really unsure what vscan means exactly. X only has the
DOUBLE_SCAN flag, while vscan appears more flexible. DRM drivers appear
to use vscan == 0 to mean the same thing as single scan mode, which
seems like it is also covered by vscan == 1. I think what I probably
need is a function which returns the effective vscan value for both X
and DRM modes and then compare those. Maybe something like:
static int
wsi_display_drm_vscan(drmModeModeInfoPtr drm)
{
if (drm->vscan > 1)
return drm->vscan;
return 1;
}
static int
wsi_display_x_vscan(xcb_randr_mode_info_t *x_mode)
{
if (x_mode->mode_flags & XCB_RANDR_MODE_FLAG_DOUBLE_SCAN)
return 2;
return 1;
}
If these look reasonable, then I could use them as appropriate and the
values should all compare correctly.
> Why are you fetching these here and not lower down? The only uses of them
> inside the "if (!connector)" is to free them. Seems to be a bit of a
> waste.
Good point. I've moved them below that block, just above the code which
uses their values.
>> + /* XXX no support for multiple leases yet */
>> + if (wsi->fd >= 0)
>> + return VK_ERROR_OUT_OF_DATE_KHR;
>>
>
> This function is supposed to return either VK_SUCCESS or
> VK_ERROR_INITIALIZATION_FAILED. The errors here and below should probably
> all be VK_ERROR_INITIALIZATION_FAILED.
Thanks. Vulkan error values remain largely a mystery to me. I've changed
the values.
> Everything else looks good to me.
Awesome!
Here's an updated version of this patch:
From 3a47d4543a054a9a1d2333ea311c9f5c057d5e9f Mon Sep 17 00:00:00 2001
From: Keith Packard
Date: Fri, 9 Feb 2018 07:45:58 -0800
Subject: [PATCH 7/9] vulkan: Add EXT_acquire_xlib_display [v4]
This extension adds the ability to borrow an X RandR output for
temporary use directly by a Vulkan application. For DRM, we use the
Linux resource leasing mechanism.
v2:
Clean up xlib_lease detection
* Use separate temporary '_xlib_lease' variable to hold the
option value to avoid changin the type of a variable.
* Use boolean expressions instead of additional if statements
to compute resulting with_xlib_lease value.
* Simplify addition of VK_USE_PLATFORM_XLIB_XRANDR_KHR to
vulkan_wsi_args
Suggested-by: Eric Engestrom
Move mode list from wsi_display to wsi_display_connector
Fix scope for wsi_display_mode and wsi_display_connector allocs
Suggested-by: Jason Ekstrand
v3:
Adopt Jason Ekstrand's coding conventions
Declare variables at first use, eliminate extra whitespace
between types and names. Wrap lines to 80 columns.
Explicitly forbid multiple DRM leases. Making the code support
this looks tricky and will require additional thought.
Use xcb_randr_output_t throughout the internals of the
implementation. Convert at the public API
(wsi_get_randr_output_display).
Clean up check for usable active_crtc (possible when only the
desired output is connected to the crtc).
Suggested-by: Jason Ekstrand
v4:
Move output resource fetching closer to use in
wsi_display_get_output. This simplifies the error returns in
earlier parts of the code a bit.
Return VK_ERROR_INITIALIZATION_FAILED from
wsi_acquire_xlib_display. Jason says this is the right error
message.
Suggested-by: Jason Ekstrand
Signed-off-by: Keith Packard
---
configure.ac| 32 ++
meson.build | 11 +
meson_options.txt | 7 +
src/vulkan/Makefile.am | 5 +
src/vulkan/wsi/meson.build | 5 +
src/vulkan/wsi/wsi_common_display.c | 489 +
Re: [Mesa-dev] [PATCH mesa 2/9] anv: Add KHR_display extension to anv [v5]
Jason Ekstrand writes: >> Signed-off-by: Keith Packard >> >> fixup >> > > Did you mean to leave this in here? Nope; just rebasing/squashing noise. I noticed this in passing and have already removed it. -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 2/9] anv: Add KHR_display extension to anv [v5]
Jason Ekstrand writes:
>> + if (instance->enabled_extensions.KHR_display) {
>> + master_fd = open(path, O_RDWR | O_CLOEXEC);
>>
>
> Is this supposed to be opening primary_path instead?
Yes, and this section of code needs to occur before anv_init_wsi.
I appear to have skipped testing this path on ANV and only tested it on
RADV -- RADV has the code in the right order. Thanks for catching this;
sorry I messed up and didn't test it.
> This could just be
>
> if (anv_gem_get_param(master_fd, I915_PARAM_CHIPSET_ID) == 0) {
>close(master_fd);
>master_fd = -1;
> }
>
> No need to type out all that IOCTL stuff.
Thanks, that's lots shorter (RADV doesn't appear to have a similar helper).
Here's an amendment to the proposed patch which fixes the bug and
switches to the simpler detection method.
From f4dac824a2566367cc3c66e1eda27fe4aaf64543 Mon Sep 17 00:00:00 2001
From: Keith Packard
Date: Thu, 14 Jun 2018 11:31:20 -0700
Subject: [PATCH] anv: Open DRM master before initializing WSI layer. Close on
device_finish.
The DRM master FD is passed to the WSI layer during initialization, so
we need to open the device slightly earlier in the function.
Also, close the DRM master FD when the driver is being shut down.
v2:
Use anv_gem_get_param to detect working master_fd
Signed-off-by: Keith Packard
---
src/intel/vulkan/anv_device.c | 34 --
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index b3c6d1a8722..3507a91810f 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -437,36 +437,32 @@ anv_physical_device_init(struct anv_physical_device *device,
if (result != VK_SUCCESS)
goto fail;
- result = anv_init_wsi(device);
- if (result != VK_SUCCESS) {
- ralloc_free(device->compiler);
- goto fail;
- }
-
- anv_physical_device_get_supported_extensions(device,
->supported_extensions);
-
if (instance->enabled_extensions.KHR_display) {
- master_fd = open(path, O_RDWR | O_CLOEXEC);
+ master_fd = open(primary_path, O_RDWR | O_CLOEXEC);
if (master_fd >= 0) {
/* prod the device with a GETPARAM call which will fail if
* we don't have permission to even render on this device
*/
- drm_i915_getparam_t gp;
- memset(, '\0', sizeof(gp));
- int devid = 0;
- gp.param = I915_PARAM_CHIPSET_ID;
- gp.value =
- int ret = drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, );
- if (ret < 0) {
+ if (anv_gem_get_param(master_fd, I915_PARAM_CHIPSET_ID) == 0) {
close(master_fd);
master_fd = -1;
}
}
}
+ device->master_fd = master_fd;
+
+ result = anv_init_wsi(device);
+ if (result != VK_SUCCESS) {
+ ralloc_free(device->compiler);
+ goto fail;
+ }
+
+ anv_physical_device_get_supported_extensions(device,
+>supported_extensions);
+
device->local_fd = fd;
- device->master_fd = master_fd;
+
return VK_SUCCESS;
fail:
@@ -482,6 +478,8 @@ anv_physical_device_finish(struct anv_physical_device *device)
anv_finish_wsi(device);
ralloc_free(device->compiler);
close(device->local_fd);
+ if (device->master_fd >= 0)
+ close(device->master_fd);
}
static void *
--
2.17.1
--
-keith
signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa 1/9] vulkan: Add KHR_display extension using DRM [v8]
Jason Ekstrand writes: > I'm trusting that not much changed other than what was explicitly called > out. I didn't want to re-read in *that* much detail again. :-) You are correct, all of the changes from the previous patch were listed in the commit message. > Reviewed-by: Jason Ekstrand Thanks much! -- -keith signature.asc Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
