Merge pull request #482 from yuriks/fix-vblank

Correctness fixes for GPU flipping and interrupts

src/core/hle/service/gsp_gpu.cpp

@@ -210,14 +210,27 @@ void SignalInterrupt(InterruptId interrupt_id) {
     }
     for (int thread_id = 0; thread_id < 0x4; ++thread_id) {
         InterruptRelayQueue* interrupt_relay_queue = GetInterruptRelayQueue(thread_id);
-        interrupt_relay_queue->number_interrupts = interrupt_relay_queue->number_interrupts + 1;
-
         u8 next = interrupt_relay_queue->index;
         next += interrupt_relay_queue->number_interrupts;
         next = next % 0x34; // 0x34 is the number of interrupt slots
 
+        interrupt_relay_queue->number_interrupts += 1;
+
         interrupt_relay_queue->slot[next] = interrupt_id;
         interrupt_relay_queue->error_code = 0x0; // No error
+
+        // Update framebuffer information if requested
+        // TODO(yuriks): Confirm where this code should be called. It is definitely updated without
+        //               executing any GSP commands, only waiting on the event.
+        for (int screen_id = 0; screen_id < 2; ++screen_id) {
+            FrameBufferUpdate* info = GetFrameBufferInfo(thread_id, screen_id);
+
+            if (info->is_dirty) {
+                SetBufferSwap(screen_id, info->framebuffer_info[info->index]);
+            }
+
+            info->is_dirty = false;
+        }
     }
     Kernel::SignalEvent(g_interrupt_event);
 }
@@ -269,8 +282,6 @@ static void ExecuteCommand(const Command& command, u32 thread_id) {
         WriteGPURegister(GPU_REG_INDEX(memory_fill_config[1].address_end), Memory::VirtualToPhysicalAddress(params.end2) >> 3);
         WriteGPURegister(GPU_REG_INDEX(memory_fill_config[1].size), params.end2 - params.start2);
         WriteGPURegister(GPU_REG_INDEX(memory_fill_config[1].value), params.value2);
-
-        SignalInterrupt(InterruptId::PSC0);
         break;
     }
 
@@ -283,22 +294,6 @@ static void ExecuteCommand(const Command& command, u32 thread_id) {
         WriteGPURegister(GPU_REG_INDEX(display_transfer_config.output_size), params.out_buffer_size);
         WriteGPURegister(GPU_REG_INDEX(display_transfer_config.flags), params.flags);
         WriteGPURegister(GPU_REG_INDEX(display_transfer_config.trigger), 1);
-
-        // TODO(bunnei): Determine if these interrupts should be signalled here.
-        SignalInterrupt(InterruptId::PSC1);
-        SignalInterrupt(InterruptId::PPF);
-
-        // Update framebuffer information if requested
-        for (int screen_id = 0; screen_id < 2; ++screen_id) {
-            FrameBufferUpdate* info = GetFrameBufferInfo(thread_id, screen_id);
-
-            if (info->is_dirty) {
-                SetBufferSwap(screen_id, info->framebuffer_info[info->index]);
-                info->framebuffer_info->active_fb = info->framebuffer_info->active_fb ^ 1;
-            }
-
-            info->is_dirty = false;
-        }
         break;
     }
 

src/core/hle/service/gsp_gpu.h

@@ -45,21 +45,16 @@ enum class CommandId : u32 {
 
 /// GSP thread interrupt relay queue
 struct InterruptRelayQueue {
-    union {
-        u32 hex;
-
-        // Index of last interrupt in the queue
-        BitField<0,8,u32>   index;
-
-        // Number of interrupts remaining to be processed by the userland code
-        BitField<8,8,u32>   number_interrupts;
-
-        // Error code - zero on success, otherwise an error has occurred
-        BitField<16,8,u32>  error_code;
-    };
-
-    u32 unk0;
-    u32 unk1;
+    // Index of last interrupt in the queue
+    u8 index;
+    // Number of interrupts remaining to be processed by the userland code
+    u8 number_interrupts;
+    // Error code - zero on success, otherwise an error has occurred
+    u8 error_code;
+    u8 padding1;
+
+    u32 missed_PDC0;
+    u32 missed_PDC1;
 
     InterruptId slot[0x34];   ///< Interrupt ID slots
 };

src/core/hw/gpu.cpp

@@ -9,6 +9,7 @@
 #include "core/settings.h"
 #include "core/core.h"
 #include "core/mem_map.h"
+#include "core/core_timing.h"
 
 #include "core/hle/hle.h"
 #include "core/hle/service/gsp_gpu.h"
@@ -24,14 +25,17 @@ namespace GPU {
 
 Regs g_regs;
 
-bool g_skip_frame = false;              ///< True if the current frame was skipped
+/// True if the current frame was skipped
+bool g_skip_frame = false;
 
-static u64 frame_ticks      = 0;        ///< 268MHz / gpu_refresh_rate frames per second
-static u64 line_ticks       = 0;        ///< Number of ticks for a screen line
-static u32 cur_line         = 0;        ///< Current screen line
-static u64 last_update_tick = 0;        ///< CPU ticl count from last GPU update
-static u64 frame_count      = 0;        ///< Number of frames drawn
-static bool last_skip_frame = false;    ///< True if the last frame was skipped
+/// 268MHz / gpu_refresh_rate frames per second
+static u64 frame_ticks;
+/// Event id for CoreTiming
+static int vblank_event;
+/// Total number of frames drawn
+static u64 frame_count;
+/// True if the last frame was skipped
+static bool last_skip_frame = false;
 
 template <typename T>
 inline void Read(T &var, const u32 raw_addr) {
@@ -79,6 +83,12 @@ inline void Write(u32 addr, const T data) {
                 *ptr = bswap32(config.value); // TODO: This is just a workaround to missing framebuffer format emulation
 
             LOG_TRACE(HW_GPU, "MemoryFill from 0x%08x to 0x%08x", config.GetStartAddress(), config.GetEndAddress());
+
+            if (!is_second_filler) {
+                GSP_GPU::SignalInterrupt(GSP_GPU::InterruptId::PSC0);
+            } else {
+                GSP_GPU::SignalInterrupt(GSP_GPU::InterruptId::PSC1);
+            }
         }
         break;
     }
@@ -90,22 +100,25 @@ inline void Write(u32 addr, const T data) {
             u8* source_pointer = Memory::GetPointer(Memory::PhysicalToVirtualAddress(config.GetPhysicalInputAddress()));
             u8* dest_pointer = Memory::GetPointer(Memory::PhysicalToVirtualAddress(config.GetPhysicalOutputAddress()));
 
+            // Cheap emulation of horizontal scaling: Just skip each second pixel of the
+            // input framebuffer. We keep track of this in the pixel_skip variable.
+            unsigned pixel_skip = (config.scale_horizontally != 0) ? 2 : 1;
+
+            u32 output_width = config.output_width / pixel_skip;
+
             for (u32 y = 0; y < config.output_height; ++y) {
                 // TODO: Why does the register seem to hold twice the framebuffer width?
-                for (u32 x = 0; x < config.output_width; ++x) {
+
+                for (u32 x = 0; x < output_width; ++x) {
                     struct {
                         int r, g, b, a;
                     } source_color = { 0, 0, 0, 0 };
 
-                    // Cheap emulation of horizontal scaling: Just skip each second pixel of the
-                    // input framebuffer. We keep track of this in the pixel_skip variable.
-                    unsigned pixel_skip = (config.scale_horizontally != 0) ? 2 : 1;
-
                     switch (config.input_format) {
                     case Regs::PixelFormat::RGBA8:
                     {
                         // TODO: Most likely got the component order messed up.
-                        u8* srcptr = source_pointer + x * 4 * pixel_skip + y * config.input_width * 4 * pixel_skip;
+                        u8* srcptr = source_pointer + (x * pixel_skip + y * config.input_width) * 4;
                         source_color.r = srcptr[0]; // blue
                         source_color.g = srcptr[1]; // green
                         source_color.b = srcptr[2]; // red
@@ -133,7 +146,7 @@ inline void Write(u32 addr, const T data) {
                     case Regs::PixelFormat::RGB8:
                     {
                         // TODO: Most likely got the component order messed up.
-                        u8* dstptr = dest_pointer + x * 3 + y * config.output_width * 3;
+                        u8* dstptr = dest_pointer + (x + y * output_width) * 3;
                         dstptr[0] = source_color.r; // blue
                         dstptr[1] = source_color.g; // green
                         dstptr[2] = source_color.b; // red
@@ -148,10 +161,12 @@ inline void Write(u32 addr, const T data) {
             }
 
             LOG_TRACE(HW_GPU, "DisplayTriggerTransfer: 0x%08x bytes from 0x%08x(%ux%u)-> 0x%08x(%ux%u), dst format %x",
-                      config.output_height * config.output_width * 4,
+                      config.output_height * output_width * 4,
                       config.GetPhysicalInputAddress(), (u32)config.input_width, (u32)config.input_height,
-                      config.GetPhysicalOutputAddress(), (u32)config.output_width, (u32)config.output_height,
+                      config.GetPhysicalOutputAddress(), (u32)output_width, (u32)config.output_height,
                       config.output_format.Value());
+
+            GSP_GPU::SignalInterrupt(GSP_GPU::InterruptId::PPF);
         }
         break;
     }
@@ -186,51 +201,39 @@ template void Write<u16>(u32 addr, const u16 data);
 template void Write<u8>(u32 addr, const u8 data);
 
 /// Update hardware
-void Update() {
+static void VBlankCallback(u64 userdata, int cycles_late) {
     auto& framebuffer_top = g_regs.framebuffer_config[0];
 
-    // Synchronize GPU on a thread reschedule: Because we cannot accurately predict a vertical
-    // blank, we need to simulate it. Based on testing, it seems that retail applications work more
-    // accurately when this is signalled between thread switches.
-
-    if (HLE::g_reschedule) {
-        u64 current_ticks = Core::g_app_core->GetTicks();
-        u32 num_lines = static_cast<u32>((current_ticks - last_update_tick) / line_ticks);
-
-        // Synchronize line...
-        if (num_lines > 0) {
-            GSP_GPU::SignalInterrupt(GSP_GPU::InterruptId::PDC0);
-            cur_line += num_lines;
-            last_update_tick += (num_lines * line_ticks);
-        }
-
-        // Synchronize frame...
-        if (cur_line >= framebuffer_top.height) {
-            cur_line = 0;
-            frame_count++;
-            last_skip_frame = g_skip_frame;
-            g_skip_frame = (frame_count & Settings::values.frame_skip) != 0;
-
-            // Swap buffers based on the frameskip mode, which is a little bit tricky. When
-            // a frame is being skipped, nothing is being rendered to the internal framebuffer(s).
-            // So, we should only swap frames if the last frame was rendered. The rules are:
-            //  - If frameskip == 0 (disabled), always swap buffers
-            //  - If frameskip == 1, swap buffers every other frame (starting from the first frame)
-            //  - If frameskip > 1, swap buffers every frameskip^n frames (starting from the second frame)
-            if ((((Settings::values.frame_skip != 1) ^ last_skip_frame) && last_skip_frame != g_skip_frame) || 
-                   Settings::values.frame_skip == 0) {
-                VideoCore::g_renderer->SwapBuffers();
-            }
-
-            // Signal to GSP that GPU interrupt has occurred
-            GSP_GPU::SignalInterrupt(GSP_GPU::InterruptId::PDC1);
-
-            // TODO(bunnei): Fake a DSP interrupt on each frame. This does not belong here, but
-            // until we can emulate DSP interrupts, this is probably the only reasonable place to do
-            // this. Certain games expect this to be periodically signaled.
-            DSP_DSP::SignalInterrupt();
-        }
+    frame_count++;
+    last_skip_frame = g_skip_frame;
+    g_skip_frame = (frame_count & Settings::values.frame_skip) != 0;
+
+    // Swap buffers based on the frameskip mode, which is a little bit tricky. When
+    // a frame is being skipped, nothing is being rendered to the internal framebuffer(s).
+    // So, we should only swap frames if the last frame was rendered. The rules are:
+    //  - If frameskip == 0 (disabled), always swap buffers
+    //  - If frameskip == 1, swap buffers every other frame (starting from the first frame)
+    //  - If frameskip > 1, swap buffers every frameskip^n frames (starting from the second frame)
+    if ((((Settings::values.frame_skip != 1) ^ last_skip_frame) && last_skip_frame != g_skip_frame) || 
+            Settings::values.frame_skip == 0) {
+        VideoCore::g_renderer->SwapBuffers();
     }
+
+    // Signal to GSP that GPU interrupt has occurred
+    // TODO(yuriks): hwtest to determine if PDC0 is for the Top screen and PDC1 for the Sub
+    // screen, or if both use the same interrupts and these two instead determine the
+    // beginning and end of the VBlank period. If needed, split the interrupt firing into
+    // two different intervals.
+    GSP_GPU::SignalInterrupt(GSP_GPU::InterruptId::PDC0);
+    GSP_GPU::SignalInterrupt(GSP_GPU::InterruptId::PDC1);
+
+    // TODO(bunnei): Fake a DSP interrupt on each frame. This does not belong here, but
+    // until we can emulate DSP interrupts, this is probably the only reasonable place to do
+    // this. Certain games expect this to be periodically signaled.
+    DSP_DSP::SignalInterrupt();
+
+    // Reschedule recurrent event
+    CoreTiming::ScheduleEvent(frame_ticks - cycles_late, vblank_event);
 }
 
 /// Initialize hardware
@@ -247,8 +250,8 @@ void Init() {
     framebuffer_top.address_right1 = 0x18273000;
     framebuffer_top.address_right2 = 0x182B9800;
     framebuffer_sub.address_left1  = 0x1848F000;
-    //framebuffer_sub.address_left2  = unknown;
-    framebuffer_sub.address_right1 = 0x184C7800;
+    framebuffer_sub.address_left2  = 0x184C7800;
+    //framebuffer_sub.address_right1 = unknown;
     //framebuffer_sub.address_right2 = unknown;
 
     framebuffer_top.width = 240;
@@ -264,12 +267,12 @@ void Init() {
     framebuffer_sub.active_fb = 0;
 
     frame_ticks = 268123480 / Settings::values.gpu_refresh_rate;
-    line_ticks = (GPU::frame_ticks / framebuffer_top.height);
-    cur_line = 0;
-    last_update_tick = Core::g_app_core->GetTicks();
     last_skip_frame = false;
     g_skip_frame = false;
 
+    vblank_event = CoreTiming::RegisterEvent("GPU::VBlankCallback", VBlankCallback);
+    CoreTiming::ScheduleEvent(frame_ticks, vblank_event);
+
     LOG_DEBUG(HW_GPU, "initialized OK");
 }
 

src/core/hw/gpu.h

@@ -252,9 +252,6 @@ void Read(T &var, const u32 addr);
 template <typename T>
 void Write(u32 addr, const T data);
 
-/// Update hardware
-void Update();
-
 /// Initialize hardware
 void Init();
 

src/core/hw/hw.cpp

@@ -75,7 +75,6 @@ template void Write<u8>(u32 addr, const u8 data);
 
 /// Update hardware
 void Update() {
-    GPU::Update();
 }
 
 /// Initialize hardware

src/video_core/renderer_opengl/renderer_opengl.cpp

@@ -88,10 +88,8 @@ void RendererOpenGL::SwapBuffers() {
 void RendererOpenGL::LoadFBToActiveGLTexture(const GPU::Regs::FramebufferConfig& framebuffer,
                                              const TextureInfo& texture) {
 
-    // TODO: Why are active_fb and the valid framebuffer flipped compared to 3dbrew documentation
-    // and GSP definitions?
     const VAddr framebuffer_vaddr = Memory::PhysicalToVirtualAddress(
-        framebuffer.active_fb == 0 ? framebuffer.address_left2 : framebuffer.address_left1);
+        framebuffer.active_fb == 0 ? framebuffer.address_left1 : framebuffer.address_left2);
 
     LOG_TRACE(Render_OpenGL, "0x%08x bytes from 0x%08x(%dx%d), fmt %x",
         framebuffer.stride * framebuffer.height,