GSP: Clean up GX command processing a lot and treat command id as a u8 rather than a u32.

Anonymous structs are not standard C++, hence don't use them.

4 files changed, 99 insertions, 56 deletions

diff --git a/src/citra_qt/debugger/graphics.cpp b/src/citra_qt/debugger/graphics.cpp
index 9aaade8f9..0f911a015 100644
--- a/src/citra_qt/debugger/graphics.cpp
+++ b/src/citra_qt/debugger/graphics.cpp
@@ -28,22 +28,24 @@ QVariant GPUCommandStreamItemModel::data(const QModelIndex& index, int role) con
     const GSP_GPU::GXCommand& command = GetDebugger()->ReadGXCommandHistory(command_index);
     if (role == Qt::DisplayRole)
     {
-        std::map<GSP_GPU::GXCommandId, const char*> command_names;
-        command_names[GSP_GPU::GXCommandId::REQUEST_DMA] = "REQUEST_DMA";
-        command_names[GSP_GPU::GXCommandId::SET_COMMAND_LIST_FIRST] = "SET_COMMAND_LIST_FIRST";
-        command_names[GSP_GPU::GXCommandId::SET_MEMORY_FILL] = "SET_MEMORY_FILL";
-        command_names[GSP_GPU::GXCommandId::SET_DISPLAY_TRANSFER] = "SET_DISPLAY_TRANSFER";
-        command_names[GSP_GPU::GXCommandId::SET_TEXTURE_COPY] = "SET_TEXTURE_COPY";
-        command_names[GSP_GPU::GXCommandId::SET_COMMAND_LIST_LAST] = "SET_COMMAND_LIST_LAST";
-        QString str = QString("%1 %2 %3 %4 %5 %6 %7 %8 %9").arg(command_names[static_cast<GSP_GPU::GXCommandId>(command.id)])
-                        .arg(command.data[0], 8, 16, QLatin1Char('0'))
-                        .arg(command.data[1], 8, 16, QLatin1Char('0'))
-                        .arg(command.data[2], 8, 16, QLatin1Char('0'))
-                        .arg(command.data[3], 8, 16, QLatin1Char('0'))
-                        .arg(command.data[4], 8, 16, QLatin1Char('0'))
-                        .arg(command.data[5], 8, 16, QLatin1Char('0'))
-                        .arg(command.data[6], 8, 16, QLatin1Char('0'))
-                        .arg(command.data[7], 8, 16, QLatin1Char('0'));
+        std::map<GSP_GPU::GXCommandId, const char*> command_names = {
+            { GSP_GPU::GXCommandId::REQUEST_DMA, "REQUEST_DMA" },
+            { GSP_GPU::GXCommandId::SET_COMMAND_LIST_FIRST, "SET_COMMAND_LIST_FIRST" },
+            { GSP_GPU::GXCommandId::SET_MEMORY_FILL, "SET_MEMORY_FILL" },
+            { GSP_GPU::GXCommandId::SET_DISPLAY_TRANSFER, "SET_DISPLAY_TRANSFER" },
+            { GSP_GPU::GXCommandId::SET_TEXTURE_COPY, "SET_TEXTURE_COPY" },
+            { GSP_GPU::GXCommandId::SET_COMMAND_LIST_LAST, "SET_COMMAND_LIST_LAST" }
+        };
+        const u32* command_data = reinterpret_cast<const u32*>(&command);
+        QString str = QString("%1 %2 %3 %4 %5 %6 %7 %8 %9").arg(command_names[command.id])
+                        .arg(command_data[0], 8, 16, QLatin1Char('0'))
+                        .arg(command_data[1], 8, 16, QLatin1Char('0'))
+                        .arg(command_data[2], 8, 16, QLatin1Char('0'))
+                        .arg(command_data[3], 8, 16, QLatin1Char('0'))
+                        .arg(command_data[4], 8, 16, QLatin1Char('0'))
+                        .arg(command_data[5], 8, 16, QLatin1Char('0'))
+                        .arg(command_data[6], 8, 16, QLatin1Char('0'))
+                        .arg(command_data[7], 8, 16, QLatin1Char('0'));
         return QVariant(str);
     }
     else
diff --git a/src/core/hle/service/gsp.cpp b/src/core/hle/service/gsp.cpp
index 053c7dd2c..05753fa2c 100644
--- a/src/core/hle/service/gsp.cpp
+++ b/src/core/hle/service/gsp.cpp
@@ -160,60 +160,72 @@ void TriggerCmdReqQueue(Service::Interface* self) {
     };
 
     GX_CmdBufferHeader* header = (GX_CmdBufferHeader*)GX_GetCmdBufferPointer(g_thread_id);
-    u32* cmd_buff = (u32*)GX_GetCmdBufferPointer(g_thread_id, 0x20 + (header->index * 0x20));
+    auto& command = *(const GXCommand*)GX_GetCmdBufferPointer(g_thread_id, 0x20 + (header->index * 0x20));
 
-    switch (static_cast<GXCommandId>(cmd_buff[0])) {
+    switch (command.id) {
 
     // GX request DMA - typically used for copying memory from GSP heap to VRAM
     case GXCommandId::REQUEST_DMA:
-        memcpy(Memory::GetPointer(cmd_buff[2]), Memory::GetPointer(cmd_buff[1]), cmd_buff[3]);
+        memcpy(Memory::GetPointer(command.dma_request.dest_address),
+               Memory::GetPointer(command.dma_request.source_address),
+               command.dma_request.size);
         break;
 
     case GXCommandId::SET_COMMAND_LIST_LAST:
-        WriteGPURegister(GPU::Regs::CommandProcessor + 2, cmd_buff[1] >> 3); // command list data address
-        WriteGPURegister(GPU::Regs::CommandProcessor, cmd_buff[2] >> 3);     // command list address
-        WriteGPURegister(GPU::Regs::CommandProcessor + 4, 1);                // TODO: Not sure if we are supposed to always write this .. seems to trigger processing though
+    {
+        auto& params = command.set_command_list_last;
+        WriteGPURegister(GPU::Regs::CommandProcessor + 2, params.address >> 3);
+        WriteGPURegister(GPU::Regs::CommandProcessor, params.size >> 3);
+        WriteGPURegister(GPU::Regs::CommandProcessor + 4, 1); // TODO: Not sure if we are supposed to always write this .. seems to trigger processing though
 
         // TODO: Move this to GPU
         // TODO: Not sure what units the size is measured in
-        g_debugger.CommandListCalled(cmd_buff[1], (u32*)Memory::GetPointer(cmd_buff[1]), cmd_buff[2]);
+        g_debugger.CommandListCalled(params.address,
+                                     (u32*)Memory::GetPointer(params.address),
+                                     params.size);
         break;
+    }
 
     case GXCommandId::SET_MEMORY_FILL:
-        WriteGPURegister(GPU::Regs::MemoryFill, cmd_buff[1] >> 3);              // Start 1
-        WriteGPURegister(GPU::Regs::MemoryFill + 1, cmd_buff[3] >> 3);          // End 1
-        WriteGPURegister(GPU::Regs::MemoryFill + 2, cmd_buff[3] - cmd_buff[1]); // Size 1
-        WriteGPURegister(GPU::Regs::MemoryFill + 3, cmd_buff[2]);               // Value 1
-
-        WriteGPURegister(GPU::Regs::MemoryFill + 4, cmd_buff[4] >> 3);          // Start 2
-        WriteGPURegister(GPU::Regs::MemoryFill + 5, cmd_buff[6] >> 3);          // End 2
-        WriteGPURegister(GPU::Regs::MemoryFill + 6, cmd_buff[6] - cmd_buff[4]); // Size 2
-        WriteGPURegister(GPU::Regs::MemoryFill + 7, cmd_buff[5]);               // Value 2
+    {
+        auto& params = command.memory_fill;
+        WriteGPURegister(GPU::Regs::MemoryFill, params.start1 >> 3);
+        WriteGPURegister(GPU::Regs::MemoryFill + 1, params.end1 >> 3);
+        WriteGPURegister(GPU::Regs::MemoryFill + 2, params.end1 - params.start1);
+        WriteGPURegister(GPU::Regs::MemoryFill + 3, params.value1);
+
+        WriteGPURegister(GPU::Regs::MemoryFill + 4, params.start2 >> 3);
+        WriteGPURegister(GPU::Regs::MemoryFill + 5, params.end2 >> 3);
+        WriteGPURegister(GPU::Regs::MemoryFill + 6, params.end2 - params.start2);
+        WriteGPURegister(GPU::Regs::MemoryFill + 7, params.value2);
         break;
+    }
 
     // TODO: Check if texture copies are implemented correctly..
     case GXCommandId::SET_DISPLAY_TRANSFER:
     case GXCommandId::SET_TEXTURE_COPY:
-        WriteGPURegister(GPU::Regs::DisplayTransfer, cmd_buff[1] >> 3);     // input buffer address
-        WriteGPURegister(GPU::Regs::DisplayTransfer + 1, cmd_buff[2] >> 3); // output buffer address
-        WriteGPURegister(GPU::Regs::DisplayTransfer + 3, cmd_buff[3]);      // input buffer size
-        WriteGPURegister(GPU::Regs::DisplayTransfer + 2, cmd_buff[4]);      // output buffer size
-        WriteGPURegister(GPU::Regs::DisplayTransfer + 4, cmd_buff[5]);      // transfer flags
+    {
+        auto& params = command.image_copy;
+        WriteGPURegister(GPU::Regs::DisplayTransfer, params.in_buffer_address >> 3);
+        WriteGPURegister(GPU::Regs::DisplayTransfer + 1, params.out_buffer_address >> 3);
+        WriteGPURegister(GPU::Regs::DisplayTransfer + 3, params.in_buffer_size);
+        WriteGPURegister(GPU::Regs::DisplayTransfer + 2, params.out_buffer_size);
+        WriteGPURegister(GPU::Regs::DisplayTransfer + 4, params.flags);
 
         // TODO: Should this only be ORed with 1 for texture copies?
-        WriteGPURegister(GPU::Regs::DisplayTransfer + 6, 1);                // trigger transfer
+        // trigger transfer
+        WriteGPURegister(GPU::Regs::DisplayTransfer + 6, 1);
         break;
+    }
 
     case GXCommandId::SET_COMMAND_LIST_FIRST:
     {
-        //u32* buf0_data = (u32*)Memory::GetPointer(cmd_buff[1]);
-        //u32* buf1_data = (u32*)Memory::GetPointer(cmd_buff[3]);
-        //u32* buf2_data = (u32*)Memory::GetPointer(cmd_buff[5]);
+        // TODO
         break;
     }
 
     default:
-        ERROR_LOG(GSP, "unknown command 0x%08X", cmd_buff[0]);
+        ERROR_LOG(GSP, "unknown command 0x%08X", (int)command.id.Value());
     }
 
     GX_FinishCommand(g_thread_id);
diff --git a/src/core/hle/service/gsp.h b/src/core/hle/service/gsp.h
index fb50a928a..f36afb697 100644
--- a/src/core/hle/service/gsp.h
+++ b/src/core/hle/service/gsp.h
@@ -4,6 +4,7 @@
 
 #pragma once
 
+#include "common/bit_field.h"
 #include "core/hle/service/service.h"
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////
@@ -12,21 +13,50 @@
 namespace GSP_GPU {
 
 enum class GXCommandId : u32 {
-    REQUEST_DMA            = 0x00000000,
-    SET_COMMAND_LIST_LAST  = 0x00000001,
-    SET_MEMORY_FILL        = 0x01000102, // TODO: Confirm?
-    SET_DISPLAY_TRANSFER   = 0x00000003,
-    SET_TEXTURE_COPY       = 0x00000004,
-    SET_COMMAND_LIST_FIRST = 0x00000005,
+    REQUEST_DMA            = 0x00,
+    SET_COMMAND_LIST_LAST  = 0x01,
+    SET_MEMORY_FILL        = 0x02,
+    SET_DISPLAY_TRANSFER   = 0x03,
+    SET_TEXTURE_COPY       = 0x04,
+    SET_COMMAND_LIST_FIRST = 0x05,
 };
 
-union GXCommand {
-    struct {
-        GXCommandId id;
-    };
+struct GXCommand {
+    BitField<0, 8, GXCommandId> id;
+
+    union {
+        struct {
+            u32 source_address;
+            u32 dest_address;
+            u32 size;
+        } dma_request;
+
+        struct {
+            u32 address;
+            u32 size;
+        } set_command_list_last;
 
-    u32 data[0x20];
+        struct {
+            u32 start1;
+            u32 value1;
+            u32 end1;
+            u32 start2;
+            u32 value2;
+            u32 end2;
+        } memory_fill;
+
+        struct {
+            u32 in_buffer_address;
+            u32 out_buffer_address;
+            u32 in_buffer_size;
+            u32 out_buffer_size;
+            u32 flags;
+        } image_copy;
+
+        u8 raw_data[0x1C];
+    };
 };
+static_assert(sizeof(GXCommand) == 0x20, "GXCommand struct has incorrect size");
 
 /// Interface to "srv:" service
 class Interface : public Service::Interface {
diff --git a/src/video_core/gpu_debugger.h b/src/video_core/gpu_debugger.h
index ca1fb22d7..d92ceaa72 100644
--- a/src/video_core/gpu_debugger.h
+++ b/src/video_core/gpu_debugger.h
@@ -50,7 +50,7 @@ public:
         virtual void GXCommandProcessed(int total_command_count)
         {
             const GSP_GPU::GXCommand& cmd = observed->ReadGXCommandHistory(total_command_count-1);
-            ERROR_LOG(GSP, "Received command: id=%x", cmd.id);
+            ERROR_LOG(GSP, "Received command: id=%x", (int)cmd.id.Value());
         }
 
         /**
@@ -84,8 +84,7 @@ public:
         gx_command_history.push_back(GSP_GPU::GXCommand());
         GSP_GPU::GXCommand& cmd = gx_command_history[gx_command_history.size()-1];
 
-        const int cmd_length = sizeof(GSP_GPU::GXCommand);
-        memcpy(cmd.data, command_data, cmd_length);
+        memcpy(&cmd, command_data, sizeof(GSP_GPU::GXCommand));
 
         ForEachObserver([this](DebuggerObserver* observer) {
                           observer->GXCommandProcessed(this->gx_command_history.size());