From 38af4c29e69326c5aa7fab82f4870d03bc4f71f6 Mon Sep 17 00:00:00 2001 From: Tom Date: Sat, 26 Jun 2021 11:04:01 -0600 Subject: [PATCH] WindowServer: Coalesce flushing buffers into one ioctl() call We regularily need to flush many rectangles, so instead of making many expensive ioctl() calls to the framebuffer driver, collect the rectangles and only make one call. And if we have too many rectangles then it may be cheaper to just update the entire region, in which case we simply convert them all into a union and just flush that one rectangle instead. --- Kernel/API/FB.h | 7 +- Kernel/Graphics/FramebufferDevice.cpp | 2 + .../VirtIOGPU/VirtIOFrameBufferDevice.cpp | 30 ++++--- Userland/Libraries/LibC/sys/ioctl_numbers.h | 9 ++- Userland/Services/WindowServer/Compositor.cpp | 16 ++-- Userland/Services/WindowServer/Screen.cpp | 81 +++++++++++++++++-- Userland/Services/WindowServer/Screen.h | 9 ++- 7 files changed, 123 insertions(+), 31 deletions(-) diff --git a/Kernel/API/FB.h b/Kernel/API/FB.h index 94b6c07677..25b00e8809 100644 --- a/Kernel/API/FB.h +++ b/Kernel/API/FB.h @@ -38,9 +38,12 @@ ALWAYS_INLINE int fb_set_buffer(int fd, int index) return ioctl(fd, FB_IOCTL_SET_BUFFER, index); } -ALWAYS_INLINE int fb_flush_buffer(int fd, FBRect* rect) +ALWAYS_INLINE int fb_flush_buffers(int fd, FBRect const* rects, unsigned count) { - return ioctl(fd, FB_IOCTL_FLUSH_BUFFER, rect); + FBRects fb_rects; + fb_rects.count = count; + fb_rects.rects = rects; + return ioctl(fd, FB_IOCTL_FLUSH_BUFFERS, &fb_rects); } __END_DECLS diff --git a/Kernel/Graphics/FramebufferDevice.cpp b/Kernel/Graphics/FramebufferDevice.cpp index b66583ff50..c77dd4b2e7 100644 --- a/Kernel/Graphics/FramebufferDevice.cpp +++ b/Kernel/Graphics/FramebufferDevice.cpp @@ -215,6 +215,8 @@ int FramebufferDevice::ioctl(FileDescription&, unsigned request, FlatPtr arg) return -EFAULT; return 0; } + case FB_IOCTL_FLUSH_BUFFERS: + return -ENOTSUP; default: return -EINVAL; }; diff --git a/Kernel/Graphics/VirtIOGPU/VirtIOFrameBufferDevice.cpp b/Kernel/Graphics/VirtIOGPU/VirtIOFrameBufferDevice.cpp index bbb69810ac..0c55be5887 100644 --- a/Kernel/Graphics/VirtIOGPU/VirtIOFrameBufferDevice.cpp +++ b/Kernel/Graphics/VirtIOGPU/VirtIOFrameBufferDevice.cpp @@ -147,18 +147,26 @@ int VirtIOFrameBufferDevice::ioctl(FileDescription&, unsigned request, FlatPtr a return -EFAULT; return 0; } - case FB_IOCTL_FLUSH_BUFFER: { - FBRect user_dirty_rect; - if (!copy_from_user(&user_dirty_rect, (FBRect*)arg)) + case FB_IOCTL_FLUSH_BUFFERS: { + FBRects user_dirty_rects; + if (!copy_from_user(&user_dirty_rects, (FBRects*)arg)) return -EFAULT; - VirtIOGPURect dirty_rect { - .x = user_dirty_rect.x, - .y = user_dirty_rect.y, - .width = user_dirty_rect.width, - .height = user_dirty_rect.height - }; - if (m_are_writes_active) - flush_dirty_window(dirty_rect); + if (Checked::multiplication_would_overflow(user_dirty_rects.count, sizeof(FBRect))) + return -EFAULT; + for (unsigned i = 0; i < user_dirty_rects.count; i++) { + FBRect user_dirty_rect; + if (!copy_from_user(&user_dirty_rect, &user_dirty_rects.rects[i])) + return -EFAULT; + if (m_are_writes_active) { + VirtIOGPURect dirty_rect { + .x = user_dirty_rect.x, + .y = user_dirty_rect.y, + .width = user_dirty_rect.width, + .height = user_dirty_rect.height + }; + flush_dirty_window(dirty_rect); + } + } return 0; } default: diff --git a/Userland/Libraries/LibC/sys/ioctl_numbers.h b/Userland/Libraries/LibC/sys/ioctl_numbers.h index 87d5e2cf1d..b568c24a95 100644 --- a/Userland/Libraries/LibC/sys/ioctl_numbers.h +++ b/Userland/Libraries/LibC/sys/ioctl_numbers.h @@ -30,6 +30,11 @@ struct FBRect { unsigned height; }; +struct FBRects { + unsigned count; + FBRect const* rects; +}; + __END_DECLS enum IOCtlNumber { @@ -50,7 +55,7 @@ enum IOCtlNumber { FB_IOCTL_SET_RESOLUTION, FB_IOCTL_GET_BUFFER, FB_IOCTL_SET_BUFFER, - FB_IOCTL_FLUSH_BUFFER, + FB_IOCTL_FLUSH_BUFFERS, SIOCSIFADDR, SIOCGIFADDR, SIOCGIFHWADDR, @@ -83,7 +88,7 @@ enum IOCtlNumber { #define FB_IOCTL_SET_RESOLUTION FB_IOCTL_SET_RESOLUTION #define FB_IOCTL_GET_BUFFER FB_IOCTL_GET_BUFFER #define FB_IOCTL_SET_BUFFER FB_IOCTL_SET_BUFFER -#define FB_IOCTL_FLUSH_BUFFER FB_IOCTL_FLUSH_BUFFER +#define FB_IOCTL_FLUSH_BUFFERS FB_IOCTL_FLUSH_BUFFERS #define SIOCSIFADDR SIOCSIFADDR #define SIOCGIFADDR SIOCGIFADDR #define SIOCGIFHWADDR SIOCGIFHWADDR diff --git a/Userland/Services/WindowServer/Compositor.cpp b/Userland/Services/WindowServer/Compositor.cpp index 16cc37cf88..af5b9dc3c7 100644 --- a/Userland/Services/WindowServer/Compositor.cpp +++ b/Userland/Services/WindowServer/Compositor.cpp @@ -537,7 +537,7 @@ void Compositor::compose() if (need_to_draw_cursor) { auto& screen_data = m_screen_data[cursor_screen.index()]; screen_data.draw_cursor(cursor_screen, cursor_rect); - screen_data.m_flush_rects.add(cursor_rect); + screen_data.m_flush_rects.add(cursor_rect.intersected(cursor_screen.rect())); if (previous_cursor_screen && cursor_rect != previous_cursor_rect) m_screen_data[previous_cursor_screen->index()].m_flush_rects.add(previous_cursor_rect); } @@ -563,10 +563,9 @@ void Compositor::flush(Screen& screen) screen_data.flip_buffers(screen); auto screen_rect = screen.rect(); - auto do_flush = [&](const Gfx::IntRect& a_rect) { - auto rect = Gfx::IntRect::intersection(a_rect, screen_rect); - if (rect.is_empty()) - return; + bool device_can_flush_buffers = screen.can_device_flush_buffers(); + auto do_flush = [&](Gfx::IntRect rect) { + VERIFY(screen_rect.contains(rect)); rect.translate_by(-screen_rect.location()); // Almost everything in Compositor is in logical coordinates, with the painters having @@ -602,7 +601,8 @@ void Compositor::flush(Screen& screen) from_ptr = (const Gfx::RGBA32*)((const u8*)from_ptr + pitch); to_ptr = (Gfx::RGBA32*)((u8*)to_ptr + pitch); } - screen.flush_display(a_rect.intersected(screen.rect())); + if (device_can_flush_buffers) + screen.queue_flush_display_rect(rect); }; for (auto& rect : screen_data.m_flush_rects.rects()) do_flush(rect); @@ -610,6 +610,8 @@ void Compositor::flush(Screen& screen) do_flush(rect); for (auto& rect : screen_data.m_flush_special_rects.rects()) do_flush(rect); + if (device_can_flush_buffers) + screen.flush_display(); } void Compositor::invalidate_screen() @@ -738,7 +740,7 @@ bool Compositor::render_animation_frame(Screen& screen, Gfx::DisjointRectSet& fl dbgln_if(MINIMIZE_ANIMATION_DEBUG, "Minimize animation from {} to {} frame# {} {} on screen #{}", from_rect, to_rect, animation_index, rect, screen.index()); painter.draw_rect(rect, Color::Transparent); // Color doesn't matter, we draw inverted - flush_rects.add(rect); + flush_rects.add(rect.intersected(screen.rect())); invalidate_screen(rect); did_render_any = true; diff --git a/Userland/Services/WindowServer/Screen.cpp b/Userland/Services/WindowServer/Screen.cpp index f8bb2ad8dc..ca4f6f7f45 100644 --- a/Userland/Services/WindowServer/Screen.cpp +++ b/Userland/Services/WindowServer/Screen.cpp @@ -25,6 +25,11 @@ Gfx::IntRect Screen::s_bounding_screens_rect {}; ScreenLayout Screen::s_layout; Vector Screen::s_scale_factors_in_use; +struct ScreenFBData { + Vector pending_flush_rects; + bool too_many_pending_flush_rects { false }; +}; + ScreenInput& ScreenInput::the() { static ScreenInput s_the; @@ -105,6 +110,7 @@ void Screen::update_scale_factors_in_use() Screen::Screen(ScreenLayout::Screen& screen_info) : m_virtual_rect(screen_info.location, { screen_info.resolution.width() / screen_info.scale_factor, screen_info.resolution.height() / screen_info.scale_factor }) + , m_framebuffer_data(adopt_own(*new ScreenFBData())) , m_info(screen_info) { open_device(); @@ -130,6 +136,7 @@ bool Screen::open_device() } m_can_set_buffer = (fb_set_buffer(m_framebuffer_fd, 0) == 0); + m_can_device_flush_buffers = true; // If the device can't do it we revert to false set_resolution(true); return true; } @@ -316,14 +323,72 @@ void ScreenInput::on_receive_keyboard_data(::KeyEvent kernel_event) Core::EventLoop::current().post_event(WindowManager::the(), move(message)); } -void Screen::flush_display(const Gfx::IntRect& flush_region) +void Screen::queue_flush_display_rect(Gfx::IntRect const& flush_region) { - FBRect rect { - .x = (static_cast(flush_region.x()) - m_virtual_rect.left()) * scale_factor(), - .y = (static_cast(flush_region.y()) - m_virtual_rect.top()) * scale_factor(), - .width = static_cast(flush_region.width()) * scale_factor(), - .height = static_cast(flush_region.height() * scale_factor()) - }; - fb_flush_buffer(m_framebuffer_fd, &rect); + // NOTE: we don't scale until in Screen::flush_display so that when + // there are too many rectangles that we end up throwing away, we didn't + // waste accounting for scale factor! + auto& fb_data = *m_framebuffer_data; + if (fb_data.too_many_pending_flush_rects) { + // We already have too many, just make sure we extend it if needed + VERIFY(!fb_data.pending_flush_rects.is_empty()); + if (fb_data.pending_flush_rects.size() == 1) { + auto& union_rect = fb_data.pending_flush_rects[0]; + auto new_union = flush_region.united(Gfx::IntRect((int)union_rect.x, (int)union_rect.y, (int)union_rect.width, (int)union_rect.height)); + union_rect.x = new_union.left(); + union_rect.y = new_union.top(); + union_rect.width = new_union.width(); + union_rect.height = new_union.height(); + } else { + // Convert all the rectangles into one union + auto new_union = flush_region; + for (auto& flush_rect : fb_data.pending_flush_rects) + new_union = new_union.united(Gfx::IntRect((int)flush_rect.x, (int)flush_rect.y, (int)flush_rect.width, (int)flush_rect.height)); + fb_data.pending_flush_rects.resize(1, true); + auto& union_rect = fb_data.pending_flush_rects[0]; + union_rect.x = new_union.left(); + union_rect.y = new_union.top(); + union_rect.width = new_union.width(); + union_rect.height = new_union.height(); + } + return; + } + VERIFY(fb_data.pending_flush_rects.size() < fb_data.pending_flush_rects.capacity()); + fb_data.pending_flush_rects.append({ (unsigned)flush_region.left(), + (unsigned)flush_region.top(), + (unsigned)flush_region.width(), + (unsigned)flush_region.height() }); + if (fb_data.pending_flush_rects.size() == fb_data.pending_flush_rects.capacity()) { + // If we get one more rectangle then we need to convert it to a single union rectangle + fb_data.too_many_pending_flush_rects = true; + } +} + +void Screen::flush_display() +{ + VERIFY(m_can_device_flush_buffers); + auto& fb_data = *m_framebuffer_data; + if (fb_data.pending_flush_rects.is_empty()) + return; + + // Now that we have a final set of rects, apply the scale factor + auto scale_factor = this->scale_factor(); + for (auto& flush_rect : fb_data.pending_flush_rects) { + flush_rect.x *= scale_factor; + flush_rect.y *= scale_factor; + flush_rect.width *= scale_factor; + flush_rect.height *= scale_factor; + } + + if (fb_flush_buffers(m_framebuffer_fd, fb_data.pending_flush_rects.data(), (unsigned)fb_data.pending_flush_rects.size()) < 0) { + int err = errno; + if (err == ENOTSUP) + m_can_device_flush_buffers = false; + else + dbgln("Screen #{}: Error ({}) flushing display: {}", index(), err, strerror(err)); + } + + fb_data.too_many_pending_flush_rects = false; + fb_data.pending_flush_rects.clear_with_capacity(); } } diff --git a/Userland/Services/WindowServer/Screen.h b/Userland/Services/WindowServer/Screen.h index 3e653174d1..520b1fa5fe 100644 --- a/Userland/Services/WindowServer/Screen.h +++ b/Userland/Services/WindowServer/Screen.h @@ -8,6 +8,7 @@ #include "ScreenLayout.h" #include +#include #include #include #include @@ -57,6 +58,8 @@ private: unsigned m_scroll_step_size { 1 }; }; +struct ScreenFBData; + class Screen { public: template @@ -160,7 +163,9 @@ public: Gfx::IntSize size() const { return { m_virtual_rect.width(), m_virtual_rect.height() }; } Gfx::IntRect rect() const { return m_virtual_rect; } - void flush_display(const Gfx::IntRect& rect); + bool can_device_flush_buffers() const { return m_can_device_flush_buffers; } + void queue_flush_display_rect(Gfx::IntRect const& rect); + void flush_display(); private: Screen(ScreenLayout::Screen&); @@ -189,10 +194,12 @@ private: Gfx::RGBA32* m_framebuffer { nullptr }; bool m_can_set_buffer { false }; + bool m_can_device_flush_buffers { true }; // If the device can't do it we revert to false int m_pitch { 0 }; Gfx::IntRect m_virtual_rect; int m_framebuffer_fd { -1 }; + NonnullOwnPtr m_framebuffer_data; ScreenLayout::Screen& m_info; };