Pica/citra-qt: Replace command list view and command list debugging code with something more sophisticated.

src/citra_qt/debugger/graphics_cmdlists.cpp

@@ -2,53 +2,21 @@
 // Licensed under GPLv2
 // Refer to the license.txt file included.
 
-#include "graphics_cmdlists.hxx"
+#include <QListView>
+#include <QPushButton>
+#include <QVBoxLayout>
 #include <QTreeView>
 
-extern GraphicsDebugger g_debugger;
+#include "graphics_cmdlists.hxx"
 
-GPUCommandListModel::GPUCommandListModel(QObject* parent) : QAbstractItemModel(parent)
+GPUCommandListModel::GPUCommandListModel(QObject* parent) : QAbstractListModel(parent)
 {
-    root_item = new TreeItem(TreeItem::ROOT, 0, NULL, this);
 
-    connect(this, SIGNAL(CommandListCalled()), this, SLOT(OnCommandListCalledInternal()), Qt::UniqueConnection);
-}
-
-QModelIndex GPUCommandListModel::index(int row, int column, const QModelIndex& parent) const
-{
-    TreeItem* item;
-
-    if (!parent.isValid()) {
-        item = root_item;
-    } else {
-        item = (TreeItem*)parent.internalPointer();
-    }
-
-    return createIndex(row, column, item->children[row]);
-}
-
-QModelIndex GPUCommandListModel::parent(const QModelIndex& child) const
-{
-    if (!child.isValid())
-        return QModelIndex();
-
-    TreeItem* item = (TreeItem*)child.internalPointer();
-
-    if (item->parent == NULL)
-        return QModelIndex();
-
-    return createIndex(item->parent->index, 0, item->parent);
 }
 
 int GPUCommandListModel::rowCount(const QModelIndex& parent) const
 {
-    TreeItem* item;
+    return pica_trace.writes.size();
-    if (!parent.isValid()) {
-        item = root_item;
-    } else {
-        item = (TreeItem*)parent.internalPointer();
-    }
-    return item->children.size();
 }
 
 int GPUCommandListModel::columnCount(const QModelIndex& parent) const
@@ -61,79 +29,67 @@ QVariant GPUCommandListModel::data(const QModelIndex& index, int role) const
     if (!index.isValid())
         return QVariant();
 
-    const TreeItem* item = (const TreeItem*)index.internalPointer();
+    const auto& writes = pica_trace.writes;
+    const Pica::CommandProcessor::CommandHeader cmd{writes[index.row()].Id()};
+    const u32 val{writes[index.row()].Value()};
 
-    if (item->type == TreeItem::COMMAND_LIST)
+    if (role == Qt::DisplayRole) {
-    {
+        QString content;
-        const GraphicsDebugger::PicaCommandList& cmdlist = command_lists[item->index].second;
+        if (index.column() == 0) {
-        u32 address = command_lists[item->index].first;
+            content = QString::fromLatin1(Pica::Regs::GetCommandName(cmd.cmd_id).c_str());
-
+            content.append(" ");
-        if (role == Qt::DisplayRole && index.column() == 0)
+        } else if (index.column() == 1) {
-        {
+            content.append(QString("%1 ").arg(cmd.hex, 8, 16, QLatin1Char('0')));
-            return QVariant(QString("0x%1 bytes at 0x%2").arg(cmdlist.size(), 0, 16).arg(address, 8, 16, QLatin1Char('0')));
+            content.append(QString("%1 ").arg(val, 8, 16, QLatin1Char('0')));
         }
-    }
-    else
-    {
-        // index refers to a specific command
-        const GraphicsDebugger::PicaCommandList& cmdlist = command_lists[item->parent->index].second;
-        const GraphicsDebugger::PicaCommand& cmd = cmdlist[item->index];
-        const Pica::CommandProcessor::CommandHeader& header = cmd.GetHeader();
 
-        if (role == Qt::DisplayRole) {
+        return QVariant(content);
-            QString content;
-            if (index.column() == 0) {
-                content = QString::fromLatin1(Pica::Regs::GetCommandName(header.cmd_id).c_str());
-                content.append(" ");
-            } else if (index.column() == 1) {
-                for (int j = 0; j < cmd.size(); ++j)
-                    content.append(QString("%1 ").arg(cmd[j], 8, 16, QLatin1Char('0')));
-            }
-
-            return QVariant(content);
-        }
     }
 
     return QVariant();
 }
 
-void GPUCommandListModel::OnCommandListCalled(const GraphicsDebugger::PicaCommandList& lst, bool is_new)
+void GPUCommandListModel::OnPicaTraceFinished(const Pica::DebugUtils::PicaTrace& trace)
-{
-    emit CommandListCalled();
-}
-
-
-void GPUCommandListModel::OnCommandListCalledInternal()
 {
     beginResetModel();
 
-    command_lists = GetDebugger()->GetCommandLists();
+    pica_trace = trace;
-
-    // delete root item and rebuild tree
-    delete root_item;
-    root_item = new TreeItem(TreeItem::ROOT, 0, NULL, this);
-
-    for (int command_list_idx = 0; command_list_idx < command_lists.size(); ++command_list_idx) {
-        TreeItem* command_list_item = new TreeItem(TreeItem::COMMAND_LIST, command_list_idx, root_item, root_item);
-        root_item->children.push_back(command_list_item);
-
-        const GraphicsDebugger::PicaCommandList& command_list = command_lists[command_list_idx].second;
-        for (int command_idx = 0; command_idx < command_list.size(); ++command_idx) {
-            TreeItem* command_item = new TreeItem(TreeItem::COMMAND, command_idx, command_list_item, command_list_item);
-            command_list_item->children.push_back(command_item);
-        }
-    }
 
     endResetModel();
 }
 
+
 GPUCommandListWidget::GPUCommandListWidget(QWidget* parent) : QDockWidget(tr("Pica Command List"), parent)
 {
     GPUCommandListModel* model = new GPUCommandListModel(this);
-    g_debugger.RegisterObserver(model);
 
-    QTreeView* tree_widget = new QTreeView;
+    QWidget* main_widget = new QWidget;
-    tree_widget->setModel(model);
+
-    tree_widget->setFont(QFont("monospace"));
+    QTreeView* list_widget = new QTreeView;
-    setWidget(tree_widget);
+    list_widget->setModel(model);
+    list_widget->setFont(QFont("monospace"));
+    list_widget->setRootIsDecorated(false);
+
+    QPushButton* toggle_tracing = new QPushButton(tr("Start Tracing"));
+
+    connect(toggle_tracing, SIGNAL(clicked()), this, SLOT(OnToggleTracing()));
+    connect(this, SIGNAL(TracingFinished(const Pica::DebugUtils::PicaTrace&)),
+            model, SLOT(OnPicaTraceFinished(const Pica::DebugUtils::PicaTrace&)));
+
+    QVBoxLayout* main_layout = new QVBoxLayout;
+    main_layout->addWidget(list_widget);
+    main_layout->addWidget(toggle_tracing);
+    main_widget->setLayout(main_layout);
+
+    setWidget(main_widget);
+}
+
+void GPUCommandListWidget::OnToggleTracing()
+{
+    if (!Pica::DebugUtils::IsPicaTracing()) {
+        Pica::DebugUtils::StartPicaTracing();
+    } else {
+        pica_trace = Pica::DebugUtils::FinishPicaTracing();
+        emit TracingFinished(*pica_trace);
+    }
 }

src/citra_qt/debugger/graphics_cmdlists.hxx

@@ -4,53 +4,28 @@
 
 #pragma once
 
-#include <QAbstractItemModel>
+#include <QAbstractListModel>
 #include <QDockWidget>
 
 #include "video_core/gpu_debugger.h"
+#include "video_core/debug_utils/debug_utils.h"
 
-// TODO: Rename class, since it's not actually a list model anymore...
+class GPUCommandListModel : public QAbstractListModel
-class GPUCommandListModel : public QAbstractItemModel, public GraphicsDebugger::DebuggerObserver
 {
     Q_OBJECT
 
 public:
     GPUCommandListModel(QObject* parent);
 
-    QModelIndex index(int row, int column, const QModelIndex& parent = QModelIndex()) const;
-    QModelIndex parent(const QModelIndex& child) const;
     int columnCount(const QModelIndex& parent = QModelIndex()) const;
     int rowCount(const QModelIndex& parent = QModelIndex()) const override;
     QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override;
 
-public:
-    void OnCommandListCalled(const GraphicsDebugger::PicaCommandList& lst, bool is_new) override;
-
 public slots:
-    void OnCommandListCalledInternal();
+    void OnPicaTraceFinished(const Pica::DebugUtils::PicaTrace& trace);
-
-signals:
-    void CommandListCalled();
 
 private:
-    struct TreeItem : public QObject
+    Pica::DebugUtils::PicaTrace pica_trace;
-    {
-        enum Type {
-            ROOT,
-            COMMAND_LIST,
-            COMMAND
-        };
-
-        TreeItem(Type type, int index, TreeItem* item_parent, QObject* parent) : QObject(parent), type(type), index(index), parent(item_parent) {}
-
-        Type type;
-        int index;
-        std::vector<TreeItem*> children;
-        TreeItem* parent;
-    };
-
-    std::vector<std::pair<u32,GraphicsDebugger::PicaCommandList>> command_lists;
-    TreeItem* root_item;
 };
 
 class GPUCommandListWidget : public QDockWidget
@@ -60,5 +35,12 @@ class GPUCommandListWidget : public QDockWidget
 public:
     GPUCommandListWidget(QWidget* parent = 0);
 
+public slots:
+    void OnToggleTracing();
+
+signals:
+    void TracingFinished(const Pica::DebugUtils::PicaTrace&);
+
 private:
+    std::unique_ptr<Pica::DebugUtils::PicaTrace> pica_trace;
 };

src/citra_qt/main.cpp

@@ -52,11 +52,11 @@ GMainWindow::GMainWindow()
 
     graphicsWidget = new GPUCommandStreamWidget(this);
     addDockWidget(Qt::RightDockWidgetArea, graphicsWidget);
-    callstackWidget->hide();
+    graphicsWidget ->hide();
 
     graphicsCommandsWidget = new GPUCommandListWidget(this);
     addDockWidget(Qt::RightDockWidgetArea, graphicsCommandsWidget);
-    callstackWidget->hide();
+    graphicsCommandsWidget->hide();
 
     QMenu* debug_menu = ui.menu_View->addMenu(tr("Debugging"));
     debug_menu->addAction(disasmWidget->toggleViewAction());

src/core/hle/service/gsp.cpp

@@ -230,11 +230,6 @@ void ExecuteCommand(const Command& command, u32 thread_id) {
         // TODO: Not sure if we are supposed to always write this .. seems to trigger processing though
         WriteGPURegister(GPU_REG_INDEX(command_processor_config.trigger), 1);
 
-        // TODO: Move this to GPU
-        // TODO: Not sure what units the size is measured in
-        g_debugger.CommandListCalled(params.address,
-                                     (u32*)Memory::GetPointer(params.address),
-                                     params.size);
         SignalInterrupt(InterruptId::P3D);
         break;
     }

src/video_core/command_processor.cpp

@@ -33,6 +33,8 @@ static inline void WritePicaReg(u32 id, u32 value, u32 mask) {
     u32 old_value = registers[id];
     registers[id] = (old_value & ~mask) | (value & mask);
 
+    DebugUtils::OnPicaRegWrite(id, registers[id]);
+
     switch(id) {
         // It seems like these trigger vertex rendering
         case PICA_REG_INDEX(trigger_draw):

src/video_core/debug_utils/debug_utils.cpp

@@ -4,6 +4,7 @@
 
 #include <algorithm>
 #include <fstream>
+#include <mutex>
 #include <string>
 
 #include "video_core/pica.h"
@@ -260,6 +261,60 @@ void DumpShader(const u32* binary_data, u32 binary_size, const u32* swizzle_data
     }
 }
 
+static std::unique_ptr<PicaTrace> pica_trace;
+static std::mutex pica_trace_mutex;
+static int is_pica_tracing = false;
+
+void StartPicaTracing()
+{
+    if (is_pica_tracing) {
+        ERROR_LOG(GPU, "StartPicaTracing called even though tracing already running!");
+        return;
+    }
+
+    pica_trace_mutex.lock();
+    pica_trace = std::unique_ptr<PicaTrace>(new PicaTrace);
+
+    is_pica_tracing = true;
+    pica_trace_mutex.unlock();
+}
+
+bool IsPicaTracing()
+{
+    return is_pica_tracing;
+}
+
+void OnPicaRegWrite(u32 id, u32 value)
+{
+    // Double check for is_pica_tracing to avoid pointless locking overhead
+    if (!is_pica_tracing)
+        return;
+
+    std::unique_lock<std::mutex> lock(pica_trace_mutex);
+
+    if (!is_pica_tracing)
+        return;
+
+    pica_trace->writes.push_back({id, value});
+}
+
+std::unique_ptr<PicaTrace> FinishPicaTracing()
+{
+    if (!is_pica_tracing) {
+        ERROR_LOG(GPU, "FinishPicaTracing called even though tracing already running!");
+        return {};
+    }
+
+    // signalize that no further tracing should be performed
+    is_pica_tracing = false;
+
+    // Wait until running tracing is finished
+    pica_trace_mutex.lock();
+    std::unique_ptr<PicaTrace> ret(std::move(pica_trace));
+    pica_trace_mutex.unlock();
+    return std::move(ret);
+}
+
 } // namespace
 
 } // namespace

src/video_core/debug_utils/debug_utils.h

@@ -5,6 +5,7 @@
 #pragma once
 
 #include <array>
+#include <memory>
 #include <vector>
 
 #include "video_core/pica.h"
@@ -38,6 +39,26 @@ private:
 void DumpShader(const u32* binary_data, u32 binary_size, const u32* swizzle_data, u32 swizzle_size,
                 u32 main_offset, const Regs::VSOutputAttributes* output_attributes);
 
+
+// Utility class to log Pica commands.
+struct PicaTrace {
+    struct Write : public std::pair<u32,u32> {
+		Write(u32 id, u32 value) : std::pair<u32,u32>(id, value) {}
+
+        u32& Id() { return first; }
+        const u32& Id() const { return first; }
+
+        u32& Value() { return second; }
+        const u32& Value() const { return second; }
+    };
+    std::vector<Write> writes;
+};
+
+void StartPicaTracing();
+bool IsPicaTracing();
+void OnPicaRegWrite(u32 id, u32 value);
+std::unique_ptr<PicaTrace> FinishPicaTracing();
+
 } // namespace
 
 } // namespace

src/video_core/gpu_debugger.h

@@ -18,19 +18,6 @@
 class GraphicsDebugger
 {
 public:
-    // A few utility structs used to expose data
-    // A vector of commands represented by their raw byte sequence
-    struct PicaCommand : public std::vector<u32>
-    {
-        const Pica::CommandProcessor::CommandHeader& GetHeader() const
-        {
-            const u32& val = at(1);
-            return *(Pica::CommandProcessor::CommandHeader*)&val;
-        }
-    };
-
-    typedef std::vector<PicaCommand> PicaCommandList;
-
     // Base class for all objects which need to be notified about GPU events
     class DebuggerObserver
     {
@@ -55,16 +42,6 @@ public:
             ERROR_LOG(GSP, "Received command: id=%x", (int)cmd.id.Value());
         }
 
-        /**
-        * @param lst command list which triggered this call
-        * @param is_new true if the command list was called for the first time
-        * @todo figure out how to make sure called functions don't keep references around beyond their life time
-        */
-        virtual void OnCommandListCalled(const PicaCommandList& lst, bool is_new)
-        {
-            ERROR_LOG(GSP, "Command list called: %d", (int)is_new);
-        }
-
     protected:
         const GraphicsDebugger* GetDebugger() const
         {
@@ -93,49 +70,12 @@ public:
                         } );
     }
 
-    void CommandListCalled(u32 address, u32* command_list, u32 size_in_words)
-    {
-        if (observers.empty())
-            return;
-
-        PicaCommandList cmdlist;
-        for (u32* parse_pointer = command_list; parse_pointer < command_list + size_in_words;)
-        {
-            const Pica::CommandProcessor::CommandHeader& header = *(Pica::CommandProcessor::CommandHeader*)(&parse_pointer[1]);
-
-            cmdlist.push_back(PicaCommand());
-            auto& cmd = cmdlist.back();
-
-            size_t size = 2 + header.extra_data_length;
-            size = (size + 1) / 2 * 2; // align to 8 bytes
-            cmd.reserve(size);
-            std::copy(parse_pointer, parse_pointer + size, std::back_inserter(cmd));
-
-            parse_pointer += size;
-        }
-
-        auto obj = std::pair<u32,PicaCommandList>(address, cmdlist);
-        auto it = std::find(command_lists.begin(), command_lists.end(), obj);
-        bool is_new = (it == command_lists.end());
-        if (is_new)
-            command_lists.push_back(obj);
-
-        ForEachObserver([&](DebuggerObserver* observer) {
-                            observer->OnCommandListCalled(obj.second, is_new);
-                        } );
-    }
-
     const GSP_GPU::Command& ReadGXCommandHistory(int index) const
     {
         // TODO: Is this thread-safe?
         return gx_command_history[index];
     }
 
-    const std::vector<std::pair<u32,PicaCommandList>>& GetCommandLists() const
-    {
-        return command_lists;
-    }
-
     void RegisterObserver(DebuggerObserver* observer)
     {
         // TODO: Check for duplicates
@@ -158,7 +98,4 @@ private:
     std::vector<DebuggerObserver*> observers;
 
     std::vector<GSP_GPU::Command> gx_command_history;
-
-    // vector of pairs of command lists and their storage address
-    std::vector<std::pair<u32,PicaCommandList>> command_lists;
 };