Kernel: Implement buffer flipping for VirtIOGPU framebuffers

This solves tearing issues and improves performance when updating the
VirtIOGPU framebuffers.

Kernel/Graphics/FramebufferDevice.cpp

@@ -215,6 +215,17 @@ int FramebufferDevice::ioctl(FileDescription&, unsigned request, FlatPtr arg)
             return -EFAULT;
         return 0;
     }
+    case FB_IOCTL_GET_BUFFER_OFFSET: {
+        FBBufferOffset buffer_offset;
+        if (!copy_from_user(&buffer_offset, (FBBufferOffset*)arg))
+            return -EFAULT;
+        if (buffer_offset.buffer_index != 0 && buffer_offset.buffer_index != 1)
+            return -EINVAL;
+        buffer_offset.offset = (size_t)buffer_offset.buffer_index * m_framebuffer_pitch * m_framebuffer_height;
+        if (!copy_to_user((FBBufferOffset*)arg, &buffer_offset))
+            return -EFAULT;
+        return 0;
+    }
     case FB_IOCTL_FLUSH_BUFFERS:
         return -ENOTSUP;
     default:

Kernel/Graphics/VirtIOGPU/VirtIOFrameBufferDevice.cpp

@@ -15,10 +15,8 @@ VirtIOFrameBufferDevice::VirtIOFrameBufferDevice(VirtIOGPU& virtio_gpu, VirtIOGP
     , m_gpu(virtio_gpu)
     , m_scanout(scanout)
 {
-    if (display_info().enabled) {
+    if (display_info().enabled)
-        Locker locker(m_gpu.operation_lock());
         create_framebuffer();
-    }
 }
 
 VirtIOFrameBufferDevice::~VirtIOFrameBufferDevice()
@@ -27,39 +25,53 @@ VirtIOFrameBufferDevice::~VirtIOFrameBufferDevice()
 
 void VirtIOFrameBufferDevice::create_framebuffer()
 {
-    auto& info = display_info();
-
-    size_t buffer_length = page_round_up(calculate_framebuffer_size(info.rect.width, info.rect.height));
-
     // First delete any existing framebuffers to free the memory first
     m_framebuffer = nullptr;
     m_framebuffer_sink_vmobject = nullptr;
 
-    // 1. Allocate frame buffer
+    // Allocate frame buffer for both front and back
-    m_framebuffer = MM.allocate_kernel_region(buffer_length, String::formatted("VirtGPU FrameBuffer #{}", m_scanout.value()), Region::Access::Read | Region::Access::Write, AllocationStrategy::AllocateNow);
+    auto& info = display_info();
+    m_buffer_size = calculate_framebuffer_size(info.rect.width, info.rect.height);
+    m_framebuffer = MM.allocate_kernel_region(m_buffer_size * 2, String::formatted("VirtGPU FrameBuffer #{}", m_scanout.value()), Region::Access::Read | Region::Access::Write, AllocationStrategy::AllocateNow);
     auto write_sink_page = MM.allocate_user_physical_page(MemoryManager::ShouldZeroFill::No).release_nonnull();
     auto num_needed_pages = m_framebuffer->vmobject().page_count();
+
     NonnullRefPtrVector<PhysicalPage> pages;
     for (auto i = 0u; i < num_needed_pages; ++i) {
         pages.append(write_sink_page);
     }
     m_framebuffer_sink_vmobject = AnonymousVMObject::create_with_physical_pages(move(pages));
 
-    // 2. Create BUFFER using VIRTIO_GPU_CMD_RESOURCE_CREATE_2D
+    Locker locker(m_gpu.operation_lock());
-    if (m_resource_id.value() != 0)
+    m_current_buffer = &buffer_from_index(m_last_set_buffer_index.load());
-        m_gpu.delete_resource(m_resource_id);
+    create_buffer(m_main_buffer, 0, m_buffer_size);
-    m_resource_id = m_gpu.create_2d_resource(info.rect);
+    create_buffer(m_back_buffer, m_buffer_size, m_buffer_size);
+}
 
-    // 3. Attach backing storage using  VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING
+void VirtIOFrameBufferDevice::create_buffer(Buffer& buffer, size_t framebuffer_offset, size_t framebuffer_size)
-    m_gpu.ensure_backing_storage(*m_framebuffer, buffer_length, m_resource_id);
+{
-    // 4. Use VIRTIO_GPU_CMD_SET_SCANOUT to link the framebuffer to a display scanout.
+    buffer.framebuffer_offset = framebuffer_offset;
-    m_gpu.set_scanout_resource(m_scanout.value(), m_resource_id, info.rect);
+    buffer.framebuffer_data = m_framebuffer->vaddr().as_ptr() + framebuffer_offset;
-    // 5. Render our test pattern
+
-    draw_ntsc_test_pattern();
+    auto& info = display_info();
-    // 6. Use VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D to update the host resource from guest memory.
+
-    transfer_framebuffer_data_to_host(info.rect);
+    // 1. Create BUFFER using VIRTIO_GPU_CMD_RESOURCE_CREATE_2D
-    // 7. Use VIRTIO_GPU_CMD_RESOURCE_FLUSH to flush the updated resource to the display.
+    if (buffer.resource_id.value() != 0)
-    flush_displayed_image(info.rect);
+        m_gpu.delete_resource(buffer.resource_id);
+    buffer.resource_id = m_gpu.create_2d_resource(info.rect);
+
+    // 2. Attach backing storage using  VIRTIO_GPU_CMD_RESOURCE_ATTACH_BACKING
+    m_gpu.ensure_backing_storage(*m_framebuffer, buffer.framebuffer_offset, framebuffer_size, buffer.resource_id);
+    // 3. Use VIRTIO_GPU_CMD_SET_SCANOUT to link the framebuffer to a display scanout.
+    if (&buffer == m_current_buffer)
+        m_gpu.set_scanout_resource(m_scanout.value(), buffer.resource_id, info.rect);
+    // 4. Render our test pattern
+    draw_ntsc_test_pattern(buffer);
+    // 5. Use VIRTIO_GPU_CMD_TRANSFER_TO_HOST_2D to update the host resource from guest memory.
+    transfer_framebuffer_data_to_host(info.rect, buffer);
+    // 6. Use VIRTIO_GPU_CMD_RESOURCE_FLUSH to flush the updated resource to the display.
+    if (&buffer == m_current_buffer)
+        flush_displayed_image(info.rect, buffer);
 
     info.enabled = 1;
 }
@@ -74,25 +86,19 @@ VirtIOGPURespDisplayInfo::VirtIOGPUDisplayOne& VirtIOFrameBufferDevice::display_
     return m_gpu.display_info(m_scanout);
 }
 
-size_t VirtIOFrameBufferDevice::size_in_bytes() const
+void VirtIOFrameBufferDevice::transfer_framebuffer_data_to_host(VirtIOGPURect const& rect, Buffer& buffer)
 {
-    auto& info = display_info();
+    m_gpu.transfer_framebuffer_data_to_host(m_scanout, rect, buffer.resource_id);
-    return info.rect.width * info.rect.height * sizeof(u32);
 }
 
-void VirtIOFrameBufferDevice::flush_dirty_window(VirtIOGPURect const& dirty_rect)
+void VirtIOFrameBufferDevice::flush_dirty_window(VirtIOGPURect const& dirty_rect, Buffer& buffer)
 {
-    m_gpu.flush_dirty_window(m_scanout, dirty_rect, m_resource_id);
+    m_gpu.flush_dirty_window(m_scanout, dirty_rect, buffer.resource_id);
 }
 
-void VirtIOFrameBufferDevice::transfer_framebuffer_data_to_host(VirtIOGPURect const& rect)
+void VirtIOFrameBufferDevice::flush_displayed_image(VirtIOGPURect const& dirty_rect, Buffer& buffer)
 {
-    m_gpu.transfer_framebuffer_data_to_host(m_scanout, rect, m_resource_id);
+    m_gpu.flush_displayed_image(dirty_rect, buffer.resource_id);
-}
-
-void VirtIOFrameBufferDevice::flush_displayed_image(VirtIOGPURect const& dirty_rect)
-{
-    m_gpu.flush_displayed_image(dirty_rect, m_resource_id);
 }
 
 bool VirtIOFrameBufferDevice::try_to_set_resolution(size_t width, size_t height)
@@ -114,13 +120,23 @@ bool VirtIOFrameBufferDevice::try_to_set_resolution(size_t width, size_t height)
     return true;
 }
 
+void VirtIOFrameBufferDevice::set_buffer(int buffer_index)
+{
+    auto& buffer = buffer_index == 0 ? m_main_buffer : m_back_buffer;
+    Locker locker(m_gpu.operation_lock());
+    if (&buffer == m_current_buffer)
+        return;
+    m_current_buffer = &buffer;
+    m_gpu.set_scanout_resource(m_scanout.value(), buffer.resource_id, display_info().rect);
+}
+
 int VirtIOFrameBufferDevice::ioctl(FileDescription&, unsigned request, FlatPtr arg)
 {
     REQUIRE_PROMISE(video);
     switch (request) {
     case FB_IOCTL_GET_SIZE_IN_BYTES: {
         auto* out = (size_t*)arg;
-        size_t value = size_in_bytes();
+        size_t value = m_buffer_size * 2;
         if (!copy_to_user(out, &value))
             return -EFAULT;
         return 0;
@@ -147,30 +163,55 @@ int VirtIOFrameBufferDevice::ioctl(FileDescription&, unsigned request, FlatPtr a
             return -EFAULT;
         return 0;
     }
+    case FB_IOCTL_SET_BUFFER: {
+        auto buffer_index = (int)arg;
+        if (!is_valid_buffer_index(buffer_index))
+            return -EINVAL;
+        if (m_last_set_buffer_index.exchange(buffer_index) != buffer_index && m_are_writes_active)
+            set_buffer(buffer_index);
+        return 0;
+    }
     case FB_IOCTL_FLUSH_BUFFERS: {
         FBFlushRects user_flush_rects;
         if (!copy_from_user(&user_flush_rects, (FBFlushRects*)arg))
             return -EFAULT;
-        if (user_flush_rects.buffer_index != 0)
+        if (!is_valid_buffer_index(user_flush_rects.buffer_index))
             return -EINVAL;
         if (Checked<unsigned>::multiplication_would_overflow(user_flush_rects.count, sizeof(FBRect)))
             return -EFAULT;
-        for (unsigned i = 0; i < user_flush_rects.count; i++) {
+        if (m_are_writes_active && user_flush_rects.count > 0) {
-            FBRect user_dirty_rect;
+            auto& buffer = buffer_from_index(user_flush_rects.buffer_index);
-            if (!copy_from_user(&user_dirty_rect, &user_flush_rects.rects[i]))
+            Locker locker(m_gpu.operation_lock());
-                return -EFAULT;
+            for (unsigned i = 0; i < user_flush_rects.count; i++) {
-            if (m_are_writes_active) {
+                FBRect user_dirty_rect;
+                if (!copy_from_user(&user_dirty_rect, &user_flush_rects.rects[i]))
+                    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
                 };
-                flush_dirty_window(dirty_rect);
+                transfer_framebuffer_data_to_host(dirty_rect, buffer);
+                if (&buffer == m_current_buffer) {
+                    // Flushing directly to screen
+                    flush_displayed_image(dirty_rect, buffer);
+                }
             }
         }
         return 0;
     }
+    case FB_IOCTL_GET_BUFFER_OFFSET: {
+        FBBufferOffset buffer_offset;
+        if (!copy_from_user(&buffer_offset, (FBBufferOffset*)arg))
+            return -EFAULT;
+        if (!is_valid_buffer_index(buffer_offset.buffer_index))
+            return -EINVAL;
+        buffer_offset.offset = (size_t)buffer_offset.buffer_index * m_buffer_size;
+        if (!copy_to_user((FBBufferOffset*)arg, &buffer_offset))
+            return -EFAULT;
+        return 0;
+    }
     default:
         return -EINVAL;
     };
@@ -181,9 +222,9 @@ KResultOr<Region*> VirtIOFrameBufferDevice::mmap(Process& process, FileDescripti
     REQUIRE_PROMISE(video);
     if (!shared)
         return ENODEV;
-    if (offset != 0)
+    if (offset != 0 || !m_framebuffer)
         return ENXIO;
-    if (range.size() != page_round_up(size_in_bytes()))
+    if (range.size() > m_framebuffer->size())
         return EOVERFLOW;
 
     // We only allow one process to map the region
@@ -217,24 +258,28 @@ void VirtIOFrameBufferDevice::deactivate_writes()
         region->set_vmobject(vm_object.release_nonnull());
         region->remap();
     }
+    set_buffer(0);
+    clear_to_black(buffer_from_index(0));
 }
 
 void VirtIOFrameBufferDevice::activate_writes()
 {
     m_are_writes_active = true;
+    auto last_set_buffer_index = m_last_set_buffer_index.load();
     if (m_userspace_mmap_region) {
         auto* region = m_userspace_mmap_region.unsafe_ptr();
         region->set_vmobject(m_framebuffer->vmobject());
         region->remap();
     }
+    set_buffer(last_set_buffer_index);
 }
 
-void VirtIOFrameBufferDevice::clear_to_black()
+void VirtIOFrameBufferDevice::clear_to_black(Buffer& buffer)
 {
     auto& info = display_info();
     size_t width = info.rect.width;
     size_t height = info.rect.height;
-    u8* data = m_framebuffer->vaddr().as_ptr();
+    u8* data = buffer.framebuffer_data;
     for (size_t i = 0; i < width * height; ++i) {
         data[4 * i + 0] = 0x00;
         data[4 * i + 1] = 0x00;
@@ -243,7 +288,7 @@ void VirtIOFrameBufferDevice::clear_to_black()
     }
 }
 
-void VirtIOFrameBufferDevice::draw_ntsc_test_pattern()
+void VirtIOFrameBufferDevice::draw_ntsc_test_pattern(Buffer& buffer)
 {
     static constexpr u8 colors[12][4] = {
         { 0xff, 0xff, 0xff, 0xff }, // White
@@ -262,7 +307,7 @@ void VirtIOFrameBufferDevice::draw_ntsc_test_pattern()
     auto& info = display_info();
     size_t width = info.rect.width;
     size_t height = info.rect.height;
-    u8* data = m_framebuffer->vaddr().as_ptr();
+    u8* data = buffer.framebuffer_data;
     // Draw NTSC test card
     for (size_t y = 0; y < height; ++y) {
         for (size_t x = 0; x < width; ++x) {
@@ -302,7 +347,7 @@ void VirtIOFrameBufferDevice::draw_ntsc_test_pattern()
 
 u8* VirtIOFrameBufferDevice::framebuffer_data()
 {
-    return m_framebuffer->vaddr().as_ptr();
+    return m_current_buffer->framebuffer_data;
 }
 
 }

Kernel/Graphics/VirtIOGPU/VirtIOFrameBufferDevice.h

@@ -14,6 +14,13 @@
 namespace Kernel::Graphics {
 
 class VirtIOFrameBufferDevice final : public BlockDevice {
+    friend class VirtIOGPUConsole;
+    struct Buffer {
+        size_t framebuffer_offset { 0 };
+        u8* framebuffer_data { nullptr };
+        VirtIOGPUResourceID resource_id { 0 };
+    };
+
 public:
     VirtIOFrameBufferDevice(VirtIOGPU& virtio_gpu, VirtIOGPUScanoutID);
     virtual ~VirtIOFrameBufferDevice() override;
@@ -22,23 +29,23 @@ public:
     virtual void activate_writes();
 
     bool try_to_set_resolution(size_t width, size_t height);
-    void clear_to_black();
+    void clear_to_black(Buffer&);
 
     size_t width() const { return display_info().rect.width; }
     size_t height() const { return display_info().rect.height; }
     size_t pitch() const { return display_info().rect.width * 4; }
 
-    size_t size_in_bytes() const;
     static size_t calculate_framebuffer_size(size_t width, size_t height)
     {
-        return sizeof(u32) * width * height;
+        // VirtIO resources can only map on page boundaries!
+        return page_round_up(sizeof(u32) * width * height);
     }
 
-    void flush_dirty_window(VirtIOGPURect const&);
+    void flush_dirty_window(VirtIOGPURect const&, Buffer&);
-    void transfer_framebuffer_data_to_host(VirtIOGPURect const&);
+    void transfer_framebuffer_data_to_host(VirtIOGPURect const&, Buffer&);
-    void flush_displayed_image(VirtIOGPURect const&);
+    void flush_displayed_image(VirtIOGPURect const&, Buffer&);
 
-    void draw_ntsc_test_pattern();
+    void draw_ntsc_test_pattern(Buffer&);
 
     u8* framebuffer_data();
 
@@ -49,6 +56,8 @@ private:
     VirtIOGPURespDisplayInfo::VirtIOGPUDisplayOne& display_info();
 
     void create_framebuffer();
+    void create_buffer(Buffer&, size_t, size_t);
+    void set_buffer(int);
 
     virtual int ioctl(FileDescription&, unsigned request, FlatPtr arg) override;
     virtual KResultOr<Region*> mmap(Process&, FileDescription&, const Range&, u64 offset, int prot, bool shared) override;
@@ -61,11 +70,26 @@ private:
     virtual mode_t required_mode() const override { return 0666; }
     virtual String device_name() const override { return String::formatted("fb{}", minor()); }
 
+    static bool is_valid_buffer_index(int buffer_index)
+    {
+        return buffer_index == 0 || buffer_index == 1;
+    }
+
+    Buffer& buffer_from_index(int buffer_index)
+    {
+        return buffer_index == 0 ? m_main_buffer : m_back_buffer;
+    }
+    Buffer& current_buffer() const { return *m_current_buffer; }
+
     VirtIOGPU& m_gpu;
     const VirtIOGPUScanoutID m_scanout;
-    VirtIOGPUResourceID m_resource_id { 0 };
+    Buffer* m_current_buffer { nullptr };
+    Atomic<int, AK::memory_order_relaxed> m_last_set_buffer_index { 0 };
+    Buffer m_main_buffer;
+    Buffer m_back_buffer;
     OwnPtr<Region> m_framebuffer;
     RefPtr<VMObject> m_framebuffer_sink_vmobject;
+    size_t m_buffer_size { 0 };
     bool m_are_writes_active { true };
     // FIXME: This needs to be cleaned up if the WindowServer exits while we are in a tty
     WeakPtr<Region> m_userspace_mmap_region;

Kernel/Graphics/VirtIOGPU/VirtIOGPU.cpp

@@ -127,12 +127,12 @@ VirtIOGPUResourceID VirtIOGPU::create_2d_resource(VirtIOGPURect rect)
     return resource_id;
 }
 
-void VirtIOGPU::ensure_backing_storage(Region& region, size_t buffer_length, VirtIOGPUResourceID resource_id)
+void VirtIOGPU::ensure_backing_storage(Region& region, size_t buffer_offset, size_t buffer_length, VirtIOGPUResourceID resource_id)
 {
     VERIFY(m_operation_lock.is_locked());
     // Allocate backing region
     auto& vm_object = region.vmobject();
-    size_t desired_num_pages = page_round_up(buffer_length);
+    size_t desired_num_pages = page_round_up(buffer_offset + buffer_length);
     auto& pages = vm_object.physical_pages();
     for (size_t i = pages.size(); i < desired_num_pages / PAGE_SIZE; ++i) {
         auto page = MM.allocate_user_physical_page();
@@ -141,7 +141,11 @@ void VirtIOGPU::ensure_backing_storage(Region& region, size_t buffer_length, Vir
         pages.append(move(page));
     }
     region.remap();
-    size_t num_mem_regions = vm_object.page_count();
+
+    VERIFY(buffer_offset % PAGE_SIZE == 0);
+    VERIFY(buffer_length % PAGE_SIZE == 0);
+    auto first_page_index = buffer_offset / PAGE_SIZE;
+    size_t num_mem_regions = buffer_length / PAGE_SIZE;
 
     // Send request
     auto& request = *reinterpret_cast<VirtIOGPUResourceAttachBacking*>(m_scratch_space->vaddr().as_ptr());
@@ -152,7 +156,7 @@ void VirtIOGPU::ensure_backing_storage(Region& region, size_t buffer_length, Vir
     request.resource_id = resource_id.value();
     request.num_entries = num_mem_regions;
     for (size_t i = 0; i < num_mem_regions; ++i) {
-        request.entries[i].address = region.physical_page(i)->paddr().get();
+        request.entries[i].address = region.physical_page(first_page_index + i)->paddr().get();
         request.entries[i].length = PAGE_SIZE;
     }
 

Kernel/Graphics/VirtIOGPU/VirtIOGPU.h

@@ -220,7 +220,7 @@ private:
     void query_display_information();
     VirtIOGPUResourceID create_2d_resource(VirtIOGPURect rect);
     void delete_resource(VirtIOGPUResourceID resource_id);
-    void ensure_backing_storage(Region& region, size_t buffer_length, VirtIOGPUResourceID resource_id);
+    void ensure_backing_storage(Region& region, size_t buffer_offset, size_t buffer_length, VirtIOGPUResourceID resource_id);
     void detach_backing_storage(VirtIOGPUResourceID resource_id);
     void set_scanout_resource(VirtIOGPUScanoutID scanout, VirtIOGPUResourceID resource_id, VirtIOGPURect rect);
     void transfer_framebuffer_data_to_host(VirtIOGPUScanoutID scanout, VirtIOGPURect const& rect, VirtIOGPUResourceID resource_id);

Kernel/Graphics/VirtIOGPU/VirtIOGPUConsole.cpp

@@ -66,7 +66,7 @@ void VirtIOGPUConsole::enqueue_refresh_timer()
                 .height = (u32)rect.height(),
             };
             g_io_work->queue([this, dirty_rect]() {
-                m_framebuffer_device->flush_dirty_window(dirty_rect);
+                m_framebuffer_device->flush_dirty_window(dirty_rect, m_framebuffer_device->current_buffer());
                 m_dirty_rect.clear();
             });
         }

Kernel/Graphics/VirtIOGPU/VirtIOGraphicsAdapter.cpp

@@ -40,7 +40,6 @@ void VirtIOGraphicsAdapter::enable_consoles()
     dbgln_if(VIRTIO_DEBUG, "VirtIOGPU: Enabling consoles");
     m_gpu_device->for_each_framebuffer([&](auto& framebuffer, auto& console) {
         framebuffer.deactivate_writes();
-        framebuffer.clear_to_black();
         console.enable();
         return IterationDecision::Continue;
     });