Vsync #18

mbernat · 2023-12-21T15:39:22Z

Resolves #17

Adds some vsync code but it doesn't actually wait yet.

mbernat · 2023-12-22T16:33:32Z

The issue here was that we were not using double buffering, otherwise it all worked fine.

To achieve double buffering with GBM surface, you need to keep around objects popped from its buffer queue because otherwise they return to the queue early and the app becomes effectively single buffered.

The GBM docs of course don't mention any of this and it's quite tricky to figure it out from other impls but this example really helped: https://github.com/eyelash/tutorials/blob/master/drm-gbm.c

bschwind

Very nice! I haven't actually tested it yet, but I'm relieved to see vsync can be achieved with quite simple code. I left some comments for now, will be happy to test it later.

bschwind · 2023-12-23T08:47:05Z

src/window.rs


 pub struct Window {
 pub(crate) gbm_surface: Surface<FramebufferHandle>,
 pub(crate) drm_display: DrmDisplay,
- pub(crate) crtc_set: bool,
+ pub(crate) frame_count: usize,
+ previous_bo: Option<BufferObject<FramebufferHandle>>,


I have a slight preference to spell out the name if it's not terribly long

Suggested change

previous_bo: Option<BufferObject<FramebufferHandle>>,

previous_buffer_object: Option<BufferObject<FramebufferHandle>>,

or

Suggested change

previous_bo: Option<BufferObject<FramebufferHandle>>,

previous_buffer: Option<BufferObject<FramebufferHandle>>,

bschwind · 2023-12-23T09:17:40Z

src/window.rs

+ let _events =
 self.drm_display.gbm_device.receive_events().expect("Could not receive events");


Is receive_events() a blocking call? I'm not sure what the consequences of calling it are if we're not past frame 1.

Yes, it's blocking. If you haven't scheduled page flips or vblanks previously you get an infinite loop here, which is arguably terrible. And using frame numbers for this logic is really opaque, we should track this in some state enum, I'll make the change here.

Also, the underlying implementation is just a select on the DRM file descriptor and a decent implementation would select from multiple sources (e.g. keyboard), like the C impls do. I'm not sure the DRM crate gives us this option but it will be necessary if we want to support an event loop later.

Ohhh I see now, it's not doing much more than just reading from the file descriptor. That's good in a way, we could potentially add our own implementation which allows for timeouts or other behavior.

bschwind · 2023-12-23T09:21:01Z

src/window.rs

@@ -60,46 +59,45 @@ impl Window {
 };

 if self.frame_count == 0 {
+ // NOTE(mbernat): It's possible to avoid initial mode setting
+ // since we're keeping the previous mode: we could just call page_flip directly.


since we're keeping the previous mode

Do you say this because we're selecting the connector's preferred mode? I'm not sure where in the code we preserve the previous mode, otherwise.

Yeah, exactly. It seems to be quite a common scenario for displays to use their preferred modes. So if the previous mode matches the next mode you don't need to do full mode setting, it's sufficient (and I think recommended) to just change the buffer with a page flip.

But it's really just a tiny optimization that I wrote down in passing.

bschwind · 2023-12-23T09:24:16Z

src/window.rs

 self.drm_display.set_mode_with_framebuffer(Some(fb));
 } else {
 self.drm_display.page_flip(fb);
 }
+
+ // NOTE(mbernat): This buffer object returns to the surface's queue upon destruction.


Where is this behavior documented? I didn't see a Drop impl for gbm::BufferObject. I believe you on the behavior, I'm just trying to follow the underlying code and put the pieces together.

Unfortunately, I haven't found any documentation for this.

As for where it is implemented: https://docs.rs/gbm/latest/src/gbm/surface.rs.html#89
I haven't tracked that logic in detail but it's registering a callback with gbm_surface_release_buffer, which is presumably triggered by the buffer object's destruction. The C implementations call this function manually.

bschwind · 2023-12-23T09:25:58Z

src/window.rs


 pub struct Window {
 pub(crate) gbm_surface: Surface<FramebufferHandle>,
 pub(crate) drm_display: DrmDisplay,
- pub(crate) crtc_set: bool,
+ pub(crate) frame_count: usize,


Do you think we could get by without storing frame_count, and instead just check the presence or absence of previous_bo?

We could but it would be even better to use a more detailed state enum that tracks whether we have already set a mode, scheduled page flips, etc.

Agreed, a state enum would be cleanest.

bschwind · 2023-12-23T09:28:26Z

src/window.rs

 }

 pub fn restore_original_display(&self) {
- self.drm_display.set_mode_with_framebuffer(self.drm_display.crtc.framebuffer());
+ let handle = self.drm_display.crtc.framebuffer().unwrap();


Suggested change

let handle = self.drm_display.crtc.framebuffer().unwrap();

let handle = self.drm_display.crtc.framebuffer().expect("Window should have a CRTC framebuffer handle");

I postponed all error handling to #6 but it's a terrible approach, I really should be making nice errors as I go, at least with expect...

bschwind · 2023-12-23T09:28:53Z

src/window.rs

- self.drm_display.set_mode_with_framebuffer(self.drm_display.crtc.framebuffer());
+ let handle = self.drm_display.crtc.framebuffer().unwrap();
+ if self.drm_display.gbm_device.get_framebuffer(handle).is_ok() {
+ self.drm_display.set_mode_with_framebuffer(self.drm_display.crtc.framebuffer());


Suggested change

self.drm_display.set_mode_with_framebuffer(self.drm_display.crtc.framebuffer());

self.drm_display.set_mode_with_framebuffer(Some(handle));

Alternatively, we could change set_mode_with_framebuffer() to take FramebufferHandle instead of Option<FramebufferHandle>, since the function name implies we'll always be working with a framebuffer.

And in the two cases where we currently call it, we always have Some(handle).

bschwind · 2023-12-23T09:35:22Z

src/window.rs

+
+ pub fn draw(&mut self, mut drawer: impl FnMut()) {
+ drawer();
+ // SAFETY: eglSwapBuffers is called by `frame.finish()` in drawer()


We can't really be sure eglSwapBuffers() is called in the closure since this is a public function, so the SAFETY comment is a bit misleading. Are there any true unsafe consequences of calling self.present if eglSwapBuffers() has not been called?

I'm hoping in the worst case, just nothing is drawn on the screen. We can probably start by marking self.present() as safe, with more granular unsafe blocks within.

Yeah, this thing became unsound again after code shuffling. We need to call frame.finish() ourselves with a separate abstraction. I wonder whether glium already has some affordance for this as this frame API is very easy to misuse.

I'm not sure what the consequences are since this has to do again with the poorly documented GBM surface. My guess would also be just broken presentation but I can also imagine that the very first time you request a buffer from the surface it contains uninitialized memory and needs to be written to at least once (e.g. by eglSwapBuffers that implicitly calls glFlush: https://registry.khronos.org/EGL/sdk/docs/man/html/eglSwapBuffers.xhtml).

/// # Safety /// This function must be called exactly once after calling /// `eglSwapBuffers`. Calling it before any `eglSwapBuffers` has happened /// on the surface or two or more times after `eglSwapBuffers` is an /// error and may cause undefined behavior.

OTOH, all of this seems really fishy, GBM should not care whether we're using EGL at all. I guess the comment just reflects the most common way this stuff is being used, rather than the root safety requirements.

bschwind · 2023-12-23T09:36:36Z

src/window.rs

 /// # Safety
 /// this must be called exactly once after `eglSwapBuffers`,
 /// which happens e.g. in `Frame::finish()`.
- pub unsafe fn swap_buffers(&mut self) {
+ unsafe fn present(&mut self) {


Does this still need to be marked unsafe?

I think so, we cannot guarantee that the surface's buffers have been used properly from inside the function.

But it would be great if we could understand the safety situation better and make the preconditions clearer.

* Replace some `unwrap`s with `expect`s. * Improve `set_mode_with_framebuffer` API. * Remove the unsound `draw` function. * Add a `DisplayState` enum to track state. * Improve documentation & commens

mbernat · 2024-01-02T12:55:10Z

This should be ready for another review, I hope I addressed everything.

mbernat added 5 commits December 21, 2023 15:22

Try waiting for page flips

9c3ed68

Do not wait before page flip is called

9fa092f

Make vsync a bit more reasonable

abcf2ae

Vsync triangle

3d7547f

Remove unused import

300c27f

mbernat requested review from strohel, bschwind, PabloMansanet and goodhoko December 21, 2023 15:39

mbernat added 2 commits December 22, 2023 10:32

Playing with receive_events

60b2884

Use double buffering, which also fixes vsync

5180543

mbernat marked this pull request as ready for review December 22, 2023 16:33

bschwind reviewed Dec 23, 2023

View reviewed changes

mbernat added 3 commits January 2, 2024 13:16

Address review comments

3e9627f

* Replace some `unwrap`s with `expect`s. * Improve `set_mode_with_framebuffer` API. * Remove the unsound `draw` function. * Add a `DisplayState` enum to track state. * Improve documentation & commens

Minor fixes

40de94c

Improve page flip names

9e38b0e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vsync #18

Vsync #18

mbernat commented Dec 21, 2023

mbernat commented Dec 22, 2023

bschwind left a comment

bschwind Dec 23, 2023

bschwind Dec 23, 2023

mbernat Dec 23, 2023

bschwind Dec 27, 2023

bschwind Dec 23, 2023

mbernat Dec 23, 2023

bschwind Dec 23, 2023

mbernat Dec 23, 2023

bschwind Dec 23, 2023

mbernat Dec 23, 2023

bschwind Dec 27, 2023

bschwind Dec 23, 2023

mbernat Dec 23, 2023

bschwind Dec 23, 2023

bschwind Dec 23, 2023

bschwind Dec 23, 2023

mbernat Dec 23, 2023

bschwind Dec 23, 2023

mbernat Dec 23, 2023

mbernat commented Jan 2, 2024

	previous_bo: Option<BufferObject<FramebufferHandle>>,
	previous_buffer_object: Option<BufferObject<FramebufferHandle>>,

	previous_bo: Option<BufferObject<FramebufferHandle>>,
	previous_buffer: Option<BufferObject<FramebufferHandle>>,

		let _events =
		self.drm_display.gbm_device.receive_events().expect("Could not receive events");

	let handle = self.drm_display.crtc.framebuffer().unwrap();
	let handle = self.drm_display.crtc.framebuffer().expect("Window should have a CRTC framebuffer handle");

	self.drm_display.set_mode_with_framebuffer(self.drm_display.crtc.framebuffer());
	self.drm_display.set_mode_with_framebuffer(Some(handle));

Vsync #18

Are you sure you want to change the base?

Vsync #18

Conversation

mbernat commented Dec 21, 2023

mbernat commented Dec 22, 2023

bschwind left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbernat commented Jan 2, 2024