Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Vsync #18

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Vsync #18

wants to merge 10 commits into from

Conversation

mbernat
Copy link
Collaborator

@mbernat mbernat commented Dec 21, 2023

Resolves #17

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

@mbernat
Copy link
Collaborator Author

mbernat commented Dec 22, 2023

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

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

@bschwind bschwind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/window.rs Outdated

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>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/window.rs Outdated
Comment on lines 94 to 95
let _events =
self.drm_display.gbm_device.receive_events().expect("Could not receive events");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/window.rs Outdated
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/window.rs Outdated
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/window.rs Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, a state enum would be cleanest.

src/window.rs Outdated
}

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/window.rs Outdated
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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

src/window.rs Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/window.rs Outdated
/// # 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this still need to be marked unsafe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator Author

mbernat commented Jan 2, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support vsync
2 participants